We should fully support uploading images and image derivatives to S3, so the public file system doesn't have to be used at all.

This might require a patch to core's image.module to work around it's dependency on public:// , which if needed we should at least get into Pressflow.

Also, we should validate that we don't need access to any local filesystem beyond temp://.

Related issues:

CommentFileSizeAuthor
#6 2470047.6-image-styles.patch18.39 KBdeviantintegral
#3 2470047.3-image-styles.patch14.91 KBdeviantintegral
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

deviantintegral’s picture

deviantintegral CreditAttribution: deviantintegral at Lullabot for NBCUniversal commented
Issue summary: View changes
deviantintegral’s picture

deviantintegral CreditAttribution: deviantintegral at Lullabot for NBCUniversal commented
Version: 7.x-1.x-dev » 7.x-2.x-dev
deviantintegral’s picture

deviantintegral CreditAttribution: deviantintegral at Lullabot for NBCUniversal commented
Status: Active » Needs review
FileSize
2470047.3-image-styles.patch14.91 KB

Here's a patch that re-implements image style support, without relying on the private file system. A few problems I ran into:

  • Image module is hardcoded to assume that derivatives are served from the public file system.
  • Amazon S3 API calls are slow. I've seen up to 6 seconds to generate and upload an image.
  • Waiting to upload even a 2KB image to S3 and serving that was adding a second or two to an image request.

So how does this work?

  1. We first generate an internal s3:// image style URL in getImageStyleUrl() via amazons3_image_style_path_alter(). This ensures we have the proper bucket in the URL, which the default implementation doesn't get us.
  2. When rendering an image style URL to the page, we call StreamWrapper->getExternalUrl(). If it detects an image style, and that image style doesn't exist on S3, it will return a URL that points to our style callback at amazons3/image-derivative/.... Otherwise, it returns a URL to S3 directly skipping the rest of this.
  3. When the browser hits our image callback at amazons3_image_deliver(), we first check if the derivative already exists. If it does, we 301 redirect to it.
  4. If the derivative needs to be generated, we acquire a lock (to prevent generation stampedes), and create the derivative in the temporary file system. This allows us to serve the generated image from temp, and then upload it in a shutdown function to S3. In my testing this is giving us a 150%-300% performance improvement for when a derivative is generated, according to when the browser actually displays the image. Yay!
  5. If we can't acquire the lock, we assume another thread is generating the image. We wait, and then serve it from the same temp file when it shows up.
deviantintegral’s picture

deviantintegral CreditAttribution: deviantintegral at Lullabot for NBCUniversal commented

Also: This totally requires this core patch to image.module: https://www.drupal.org/node/1358896#comment-9297197

ericduran’s picture

ericduran CreditAttribution: ericduran commented

This is great Andrew. I'll give it a better pass tomorrow. That being said from the description this sounds awesome. Especially serving the initial one from tmp.

deviantintegral’s picture

deviantintegral CreditAttribution: deviantintegral at Lullabot for NBCUniversal commented
FileSize
2470047.6-image-styles.patch18.39 KB

An update that adds:

  • Image token (itok=) support.
  • Fix not adding a leading slash in fromKey().
  • Fix not using MENU_NOT_FOUND constant.
ericduran’s picture

ericduran CreditAttribution: ericduran commented 
+++ b/amazons3.module
@@ -47,10 +48,145 @@ function amazons3_menu() {
+  $items[\Drupal\amazons3\StreamWrapper::stylesCallback] = array(

Does it need full path here? Can't tell from the snippet.

I only had one small note. Everything else look great. I'm ok with merging as is.

deviantintegral’s picture

deviantintegral CreditAttribution: deviantintegral at Lullabot for NBCUniversal commented

Thanks!

That callback isn't a full path, but something like amazons3/image-derivative. It's a constant so I can reference it when generating image URLs.

deviantintegral’s picture

deviantintegral CreditAttribution: deviantintegral at Lullabot for NBCUniversal commented
Status: Needs review » Fixed

ericduran’s picture

ericduran CreditAttribution: ericduran commented

@deviantintegral yea, I noticed it was a constant. I meant Full PHP path so `\Drupal\amazons3\StreamWrapper::stylesCallback` instead of `StreamWrapper::stylesCallback` assuming we were already using `use \Drupal\amazons3\StreamWrapper`

deviantintegral’s picture

deviantintegral CreditAttribution: deviantintegral at Lullabot for NBCUniversal commented

Oh, that! The module doesn't declare a namespace because I've seen that break things without requiring xautoload. So it's either use statements or full paths, and that class is only used once at the moment.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

tim520’s picture

tim520 CreditAttribution: tim520 commented

@deviantintegral Thanks for this great patch. But I still have some images can not be generated styles in Amazon S3, most of them are succeed.

jadsay’s picture

jadsay CreditAttribution: jadsay commented

@deviantegral: the image derivates are not being generated and uploaded to my s3 bucket only the original image is being uploaded.
I applied all the patches mentioned in the module documentation but still every image rendered with image styles is not working.

jadsay’s picture

jadsay CreditAttribution: jadsay commented
Status: Closed (fixed) » Needs work
al.ex’s picture

al.ex CreditAttribution: al.ex commented

+1 confirmed that image derivate generation isn't working properly yet. I've actually resorted to trigger it manually upon changes using custom code in order to make this work somehow.

Another issue I'm facing here is that under certain circumstances, derivates aren't getting uploaded to S3. The problem is that I can't reproduce these circumstances, it seems to occur randomly. The logs don't tell anything either. Could this be related to the shutdown function implementation?

Kristen Pol’s picture

Kristen Pol
she/her
Primary language English
Location Santa Cruz, CA, USA
CreditAttribution: Kristen Pol at Hook 42 commented
Status: Needs work » Fixed

Since the last activity was 3 years ago, marking this back as fixed. If you are having trouble with this feature, please create a new issue and add steps to reproduce.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.