Make WordPress Core

Opened 4 years ago

Last modified 3 years ago

#48522 new defect (bug)

Attachment size not generated when large images uploaded

Reported by: vanyukov's profile vanyukov Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 5.3
Component: Media Keywords: 2nd-opinion needs-testing needs-unit-tests needs-refresh
Focuses: Cc:

Description

I've noticed that some images during upload do not generate attachment sizes at all, or generate the sizes partially. An example of such an image: https://upload.wikimedia.org/wikipedia/commons/f/ff/Pizigani_1367_Chart_10MB.jpg

Upload is successful, but there's no scaled image, as well as no attachments.

Attachments (2)

48522.diff (1.9 KB) - added by azaozz 4 years ago.
48522.2.diff (3.8 KB) - added by vanyukov 4 years ago.

Download all attachments as: .zip

Change History (39)

This ticket was mentioned in Slack in #core-media by vanyukov. View the logs.


4 years ago

#2 @vanyukov
4 years ago

Just some more info. I've traced the issue to the thumbnail_image() failing inside the wp_create_image_subsizes() function with a "cache resources exhausted" error during the generation of the scaled image. Later on when WordPress tries to _wp_make_subsizes(), the image editor is still returned with an error. A possible solution would be to generate attachments from smallest to largest. That way ImageMagick is able to work... it's magic... and users will, at least, get some attachments generated. Now in 5.3, there is absolutely nothing being generated, compared to 5.2.4

Another possible solution would be to resample images to a smaller destination size? Or even make this dynamic? First try to resample to the default 5x size, if that fails, try the 4x and so on... That would probably require revisiting the DSSIM tests and making sure the resample does not change the image quality. But, again, from the users perspective - any image is better than nothing.

Will happily submit a patch to any of the above changes

#3 @johnbillion
4 years ago

  • Component changed from Upload to Media
  • Milestone changed from Awaiting Review to 5.3
  • Version set to trunk

Thank you for the report @vanyukov and the offer of help.

Moving to 5.3 for visibility.

This ticket was mentioned in Slack in #core-media by mike. View the logs.


4 years ago

#5 @dinhtungdu
4 years ago

In my testing, the problem only occurs with ImageMagick as the media handler. Image sizes were generated normally with the default GD library. I have 11 image sizes generated when GD is the media handler, while it's only 5 sizes with ImageMagick.

The screenshot of VM when generating image sizes with ImageMagick:

https://i.imgur.com/XC2D766.png

My env:

  • Virtualbox Ubuntu 16.04.6 LTS
  • 1 Core
  • 1024GB RAM
  • PHP 7.3.9 with max_execution_time and max_input_time set to 30 (900 has the same result)
  • Latest WordPress svn revision.
  • ImageMagick 6.8.9-9
  • Imagick module version 3.4.4

#6 @azaozz
4 years ago

  • Keywords reporter-feedback added
  • Milestone changed from 5.3 to 5.3.1

Yes, image upload can still fail despite all the retry requests. The last one should be to delete the attachment and any existing sub-sizes that were created. Failing to delete it (to do cleanup) is a bug.

Testing with the linked image, it works properly here and creates all sub-sizes after 2-3 retries. Limiting the server resources makes it fail, but the attachment is deleted at the end. If it is not deleted in your tests, could you look in the last AJAX request ant/or possibly add some debugging in wp_ajax_media_create_image_subsizes() in wp-admin/includes/ajax-actions.php to try to figure out what's stopping it.

Moving this to 5.3.1 for now as it seems to be working as expected.

#7 follow-up: @vanyukov
4 years ago

@azaozz, the file upload doesn't fail. the wp_ajax_media_create_image_subsizes() is not triggered on my test install.

#8 in reply to: ↑ 7 @azaozz
4 years ago

Replying to vanyukov:

the wp_ajax_media_create_image_subsizes() is not triggered on my test install.

What requests do you see in the browser tools network tab?

There should be one to async-upload.php that will end with an HTTP 5.x.x error when the server runs out of resources. That should trigger an admin-ajax.php request which is the first retry attempt. If that also ends with HTTP 5.x.x error, another one should follow, up to four times. Finally if all retries fail there should be another admin-ajax.php request to do a "cleanup" of the failed upload. That should delete the newly created attachment and show the error message "...please scale down the image and try to upload again".

A possible solution would be to generate attachments from smallest to largest.

This is already the case. The image sub-sizes are sorted by "usage priority" in wp_create_image_subsizes(). However if generating a registered sub-size fails, the upload is deemed unsuccessful. This may change in the future but keep in mind that currently there are five attempts to generate the sub-sizes. If all fail, chances are that the image cannot be resizes as uploaded and best next step would be for the user to scale it down and try uploading again.

Eventually these retries can be "exposed" to the users by having a "partial" status for attachments with some UI/a button to retry to create the missing sub-sizes. But that will bring more complexity and in most cases will still fail at the end. Thinking that for now that can be added by plugins. If such plugins become popular, can perhaps be merged to core :)

#9 follow-up: @uglyrobot
4 years ago

None of this answers why larger images like this sample fail all resizes on 5.3 when it works fine on 5.2 with the same server and ImageMagick configuration. We can bump up the default ImageMagick config to 2gb disk cache from default 1GB which seems to fix it but that seems excessive and likely must affect many other hosting providers who no doubt use the default config. There is some sort of performance regression with ImageMagick in 5.3.

As far as network requests, I just get a 200 response from the initial upload with attachment id, then the ajax response with the image row HTML. No http errors or php errors in logs. But no thumbs are generated, only the original image sits in the uploads directory. It fails silently.

#10 @uglyrobot
4 years ago

I'm just theorizing here that it's the new 5.3 really large original resize that fails, and that there is a bug that prevents the rest of the smaller thumbs (that would work in 5.2) to also get skipped due to:

Later on when WordPress tries to _wp_make_subsizes(), the image editor is still returned with an error

#11 in reply to: ↑ 9 @azaozz
4 years ago

Replying to uglyrobot:

There is some sort of performance regression with ImageMagick in 5.3.

As far as network requests, I just get a 200 response from the initial upload with attachment id, then the ajax response with the image row HTML. No http errors or php errors in logs. But no thumbs are generated, only the original image sits in the uploads directory. It fails silently.

Testing with the 10MB image using GD in 5.2 fails with (the dreaded) HTTP Error here. I don't currently have access to ImageMagick. Would you be able to "get to the bottom of it" and determine the exact ImageMagick error and where it comes from?

I'm just theorizing here that it's the new 5.3 really large original resize that fails...

Yes, it's possible it fails as the size is larger than the previous default sizes. If it fails at this point, the retries will still try to create the same -scaled image. However there are even larger "sub-sizes" added by themes/plugins, and generally wp-admin should be able to handle this kind of sub-sizes.

The bug in this case is that it fails silently and the user is not notified to scale down the image and try to upload it again. Would be great to catch the error and show that error message in the UI.

Last edited 4 years ago by azaozz (previous) (diff)

#12 @vanyukov
4 years ago

@azaozz,

thumbnail_image() is failing inside the wp_create_image_subsizes() function with a "cache resources exhausted" error. the async-upload.php never fails, because the resources do not run out during upload, they run out during an attempt to create the new big image and the error for that is not passed down to async-upload.php. I've attached a trace call:

https://i.imgur.com/4vsHSGrr.png

Also, I've asked in Slack, but haven't yet received a response. I'm not sure I understand how everyone is defaulting to GD during tests, when _wp_image_editor_choose() is set to return an instance of WP_Image_Editor_Imagick first. Am I missing something?

This ticket was mentioned in Slack in #core by vanyukov. View the logs.


4 years ago

#14 follow-up: @vanyukov
4 years ago

As per discussion from Slack, I'm moving the discussion to trac.

I see three possible solutions to the problem.

  1. Adjust the resample size

In my tests, this change fixes the issue: inside wp-includes/class-wp-image-editor-imagick.php (around line 343) instead of:

$this->image->sampleImage( $dst_w * $sample_factor, $dst_h * $sample_factor );

try to dynamically adjust the $sample_factor:

do {
    try {
        unset( $e );
        $this->image->sampleImage( $dst_w * $sample_factor, $dst_h * $sample_factor );
    } catch ( Exception $e ) {
        $sample_factor--;
        continue;
    }

    break;
} while ( $sample_factor > 0 );

if ( isset( $e ) && $e instanceof ImagickException ) {
    throw $e;
}

My main concern is that this change might not be the best way to go forward without doing proper DSSIM tests to make sure that there is no visible change in images with the new resample size. The previous tests were done a while back and, I believe, the max-width tested was 1200px. Much has changed since then.

  1. Error handle this.

The problem with this approach - it does not fix the initial issue. There will still be no guaranteed way to resize large attachments.

  1. Fallback to GD

I can see that this is not an issue with GD. So maybe use GD as a default editor when both ImageMagick and GD are installed? Currently _wp_image_editor_choose() is set to return WP_Image_Editor_Imagick by default. Also an easy fix.

#15 in reply to: ↑ 14 @azaozz
4 years ago

  • Keywords needs-patch 2nd-opinion added; reporter-feedback removed

Replying to vanyukov:

Thanks for moving the discussion here.

Adjust the resample size
...
My main concern is that this change might not be the best way to go forward without doing proper DSSIM tests to make sure that there is no visible change in images with the new resample size.

Yeah, this is a possible solution but will need to ensure the image sub-sizes maintain best quality. This also seems the most complex of the three, and reducing the $sample_factor in a try/catch "loop" may be too "costly" in some cases.

A possible variant would be to reduce the $sample_factor by a bit larger value and only a set number of times. Of course, this will need testing/experimenting and can be added in 5.4.

Error handle this.

The problem with this approach - it does not fix the initial issue. There will still be no guaranteed way to resize large attachments.

Right, there's no guarantee that a subsequent attempt to resize the image will succeed. On the other hand this is how "post processing" errors are generally handled now. If it fails, the client does another request and the post processing "picks up" from where the error occurred.

Thinking this would be the best solution for 5.3.1. It is "safe" as it doesn't introduce more "failure points", and will fix at least some of the cases.

Fallback to GD

This is also a possible solution. The way it can be implemented would be to try ImageMagick first, and if it fails with specific error(s), proceed with GD. Generally ImageMagick is the preferred "editor" as it has more features, produces better quality image sub-sizes, and handles EXIF.

@mikeschroder, @joemcgill any thoughts/suggestions/ideas? :)

Last edited 4 years ago by azaozz (previous) (diff)

@azaozz
4 years ago

#16 @azaozz
4 years ago

  • Keywords has-patch needs-testing needs-unit-tests added; needs-patch removed

In 48522.diff: Do not (silently) ignore errors while resizing or saving resized images. Instead call trigger_error( $error_code, E_USER_ERROR ); when GD or ImageMagick return an error. This will allow the client to do another post-processing request.

@vanyukov
4 years ago

#17 follow-up: @vanyukov
4 years ago

@azaozz,

Now it does error out and do several attempts, but they all fail...

What I was thinking is something like this. 48522.2.diff is in no way a patch, just illustrating my logic. I'm sure there is a much better way to achieve this.

#18 in reply to: ↑ 17 @azaozz
4 years ago

Replying to vanyukov:

Looking at 48522.2.diff: the change there is to move creation of the scaled image to happen after the other sub-sizes were created. Don't think this fixes anything though. The logic is to do several attempts to post-process the image, and if all fail, to delete the attachment and the sub-sizes that were created. Then ask the user to scale the image and upload it again.

If we move the creation of (presumably) the largest sub-size to last, and if it fails, we'll still need to delete the attachment and the rest of the sub-sizes. Otherwise some users will end up with inconsistently post-processed images.

As far as I understand, the problem here is that ImageMagick (and probably GD) has a limit of how large a file can be processed (that depends on settings, etc.). Thinking that the best option when that limit is exceeded would be to "tell" the user to upload a smaller image.

#19 follow-up: @azaozz
4 years ago

Looking at 48522.diff again, it changes the behaviour in _wp_make_subsizes(). Until now errors while creating a particular sub-size were silently ignored. As the sub-sizes are created in a loop, that allowed the loop to continue and (attempt to) create the next sub-size.

The change in behaviour makes sense while post-processing uploaded images, but may be unexpected when "recreating thumbnails", etc. Arguably when one sub-size fails, the whole "operation" should fail too however throwing a PHP error and exiting will prevent any follow-up handling. Working on a revised patch.

#20 in reply to: ↑ 19 ; follow-up: @vanyukov
4 years ago

Replying to azaozz:

Looking at 48522.diff again, it changes the behaviour in _wp_make_subsizes(). Until now errors while creating a particular sub-size were silently ignored. As the sub-sizes are created in a loop, that allowed the loop to continue and (attempt to) create the next sub-size.

Yes, I did see that part. That's why I said it was not a patch. I believe you were planning, at some point, on having WordPress auto regenerate attachment sizes during a theme switch, for example? There would have to be a way to continue on if one of the attachment sizes is not generated for some reason.

Regarding 48522.diff. Won't that seriously limit the capabilities of hosts that are not able to accommodate the heavy lifting of ImageMagick? With the patch, I'm getting no images at all and am required to downsize the original. What if that's not an option? I can see so many scenarios that would make me want to upload attachments as large as possible. I'm fine with a host not being able to generate the largest attachment sizes. That's my concern. But I don't want WordPress to make that decision for me.

#21 in reply to: ↑ 20 ; follow-up: @azaozz
4 years ago

Replying to vanyukov:

Won't that seriously limit the capabilities of hosts that are not able to accommodate the heavy lifting of ImageMagick? With the patch, I'm getting no (sub-size) images at all and am required to downsize the original. What if that's not an option?

Sure, but what are the alternatives? When an image to too big to be post-processed generally there are three possibilities:

  • Attempt to create as many sub-sizes as possible and leave the attachment inconsistent. In this case there may be from 0 to 6 sub-sizes (by default). This will cause many problems down the line, as everything (WP, themes, plugins) expects to have at least the default sub-sizes available.
  • Attempt to (dynamically) adjust quality settings when creating sub-sizes. This will make larger images work, but will still fail for very large images. Also the quality of the sub-sizes will be inconsistent and may get pretty bad in some cases.
  • Fail the upload and ask the user to upload a smaller image that is more suitable for web use.

As WordPress is a web publishing software, thinking that the third option above is best for the great majority of users. Of course there may be cases where images are not uploaded for "web use", but these cases would be best served by more specifically targeted plugins?

I can see so many scenarios that would make me want to upload attachments as large as possible. I'm fine with a host not being able to generate the largest attachment sizes. That's my concern. But I don't want WordPress to make that decision for me.

Yeah, I understand. But that would be a specific use case. Most users upload images that are for "web use". They expect the images "to just work". Failing to create some of the sub-sizes will prevent these images from working properly.

Also it is pretty simple to change (or disable) the creation of image sub-sizes from a theme or a plugin. Specific use cases are best handled by plugins, they shouldn't limit the main functionality in WP :)

BTW, thinking that WP could/should include better support for "images that are not for web use". Perhaps things like:

  • Uploading an image as-a-file, i.e. without post-processing.
  • Allowing sub-sizes to be uploaded separately and "importing" them in the image meta.
  • Having an UI to generate missing sub-sizes for a particular attachment.

Currently all of these are available in plugins but I can see them as part of an updated/refactored "Image Editor". Then there's the possibility to scale images in the browser before uploading them. Would be great to start on that, even as part of 5.4 :)

Last edited 4 years ago by azaozz (previous) (diff)

#22 in reply to: ↑ 21 ; follow-up: @vanyukov
4 years ago

Replying to azaozz:

As WordPress is a web publishing software, thinking that the third option above is best for the great majority of users. Of course there may be cases where images are not uploaded for "web use", but these cases would be best served by more specifically targeted plugins?

To a point, I do agree. But there will come a time when this won't be true. You can easily get your hands on a retina 6K monitor or 8k TV, so uploading a large image is not that uncommon. Future-proofing for the next 2-3 years is a great idea, in my opinion.

But for the time being, I guess, this will have to do:

Fail the upload and ask the user to upload a smaller image that is more suitable for web use.

As for:

Uploading an image as-a-file, i.e. without post-processing.
Allowing sub-sizes to be uploaded separately and "importing" them in the image meta.
Having an UI to generate missing sub-sizes for a particular attachment.

Those are some really nice ideas. A UI to generate missing attachments + a UI to indicate that there are missing attachments for an image - that would be the best-case scenario. Even if there's some small notice when adding an image that not all attachment sizes are available, that would also resolve

They expect the images "to just work". Failing to create some of the sub-sizes will prevent these images from working properly.

Having a market share of 1/3 of the web, my opinion, WordPress should be a universal platform. All the use case scenarios should be supported "out of the box", without plugins and modifications. If it was my decision, I would prioritize for the future, not for the past. But it's easier to say than do, I understand that :-)

#23 in reply to: ↑ 22 @azaozz
4 years ago

Replying to vanyukov:

...uploading a large image is not that uncommon. Future-proofing for the next 2-3 years is a great idea, in my opinion.

Exactly! This is the reason two new larger image sub-sizes were added in 5.3, to better handle larger and high-density displays. This is also the reason to make another sub-size and set it as "max size for web use". Typically many users upload unedited photos. These images are anywhere from about 3MB to 10MB in size. Serving these images for large screen devices is impractical, hence the 2560px "full" size is made and is about 700KB to 900KB.

Having a market share of 1/3 of the web, my opinion, WordPress should be a universal platform. All the use case scenarios should be supported "out of the box", without plugins and modifications. If it was my decision, I would prioritize for the future, not for the past. But it's easier to say than do, I understand that :-)

Haha, I wish it was possible to "support all use case scenarios out of the box". However often one "edge case" is the exact opposite of another. Supporting one will make WP hard to use or incompatible with the other... This is one of the reasons why so many plugins exist, they add support for literally all possible use cases :)

But for the time being, I guess, this will have to do:

Fail the upload and ask the user to upload a smaller image that is more suitable for web use.

Yeah, this is a hard decision, but for now I think it is the proper one. Perhaps we can try to enhance this a bit by looking at the image file size before upload and warn the user when it is too large? That may make the user experience a little better (save some time) but still doesn't solve the underlying problem.

Another possible solution would be to "tell" the user the image is too large and the server cannot post-process it. Then have the ability for the user to create the sub-sizes "by hand" and upload them separately. However that's going to be... a pretty complex task for many users: resizing/scaling the original image, saving each scaled image with different file name, cropping some of the scaled images... Still sounds like an "advanced" plugin.

In these terms, what is the best solution to this ticket? Perhaps "surfacing" ImageMagick errors by adding trigger_error(); in wp_create_image_subsizes() may be good. This functions is intended to initially create the sub-sizes after upload. But it should not be in _wp_make_subsizes() which is for more general use.

Also, should it do trigger_error(); on all internal/silent GD and ImageMagick errors or just on some?

#24 @azaozz
4 years ago

Related: #48842.

Found an error in the calculations when resampling the original image while making a much smaller sub-size. This affects large images and (likely) causes some of the errors in ImageMagick.

@vanyukov could you test the patch on #48842 please. Seems it was up-scaling the original image (10315px to 12800px) before resizing it to 2560px.

Last edited 4 years ago by azaozz (previous) (diff)

This ticket was mentioned in Slack in #core-media by azaozz. View the logs.


4 years ago

#26 follow-up: @vanyukov
4 years ago

@azaozz ,

Just a quick test - it does fix the upload issue. But it doesn't seem like the right way to calculate the resample ratio. Will reply in the other task.

This ticket was mentioned in Slack in #core-media by azaozz. View the logs.


4 years ago

#28 in reply to: ↑ 26 @azaozz
4 years ago

Replying to vanyukov:

Right. This code seems to come (more or less) from the ImageMagick -thumbnail option, see: https://github.com/ImageMagick/ImageMagick/blob/master/MagickCore/resize.c#L4491. It also has a hard-coded $sample_factor = 5 and does upscaling with SampleImage in some cases.

See #48842 for more details.

Last edited 4 years ago by azaozz (previous) (diff)

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


4 years ago

#30 @azaozz
4 years ago

  • Keywords has-patch removed
  • Milestone changed from 5.3.1 to 5.4

Thinking this ticket needs more work. As @vanyukov mentions above exiting when this error in ImageMagick happens doesn't fix it. The underlying issue is with the ImageMagick settings, in particular the correlation between maximum allowed image size and amount of disk cache. This is not a regression in 5.3, the same happens when creating large image sub-sizes in 5.2 and earlier WP versions.

Moving this to the 5.4 milestone in order to find better solution that can be tested better and iterated on if needed.

This ticket was mentioned in Slack in #core-media by antpb. View the logs.


4 years ago

#32 @antpb
4 years ago

  • Milestone changed from 5.4 to 5.5

With 5.4 release fast approaching, I am going to move this ticket to the 5.5 milestone as it seems more work is still to be done. If you feel confident this can come back into the 5.4 milestone, feel free to move it. :)

This ticket was mentioned in Slack in #core by david.baumwald. View the logs.


4 years ago

#34 @davidbaumwald
4 years ago

  • Keywords needs-refresh added

Latest patch no longer applies cleanly to trunk, marking for a refresh.

#35 @desrosj
4 years ago

  • Milestone changed from 5.5 to 5.6

With 5.5 beta 1 tomorrow, it looks like this one will require more time.

This ticket was mentioned in Slack in #core-media by florian-tiar. View the logs.


3 years ago

#37 @helen
3 years ago

  • Milestone changed from 5.6 to Future Release

Moving this out of 5.6, hasn't had attention in months and really should have been punted weeks ago, not sure how we missed it.

Last edited 3 years ago by helen (previous) (diff)
Note: See TracTickets for help on using tickets.