Opened 8 years ago
Last modified 8 years ago
#39387 reviewing defect (bug)
Responsive Images Broken When Full Size <= 300 px
Reported by: |
|
Owned by: |
|
---|---|---|---|
Milestone: | Future Release | Priority: | normal |
Severity: | normal | Version: | 4.4 |
Component: | Media | Keywords: | has-patch |
Focuses: | Cc: |
Description
A quick test of the srcset feature today showed me that WordPress never includes the "full" size image in the srcset when the "thumbnail" size is inserted in a post. As a result, if the full size was 300 px, the responsive image feature effectively does nothing.
Tested 4.7. Suspect this might go back to 4.4.
Attachments (1)
Change History (16)
#5
@
8 years ago
After 3 hours of working my way down the rabbit hole, CABOOM! WordPress is silently crashing!!!
WordPress calls $resized = $this->_save( $this->image );
and NEVER checks the result value!
ImagickException::__set_state(array( 'message' => 'memory allocation failed `/wp-content/uploads/2016/12/2016-12-18-19-36_20161218_18-768x577.gif\' @ error/gif.c/WriteGIFImage/1633', 'string' => '', 'code' => 400, 'file' => '/wp-includes/class-wp-image-editor.php', 'line' => 423, 'trace' => array ( 0 => array ( 'function' => 'writeimage', 'class' => 'Imagick', 'type' => '->', 'args' => array ( 0 => '/wp-content/uploads/2016/12/2016-12-18-19-36_20161218_18-768x577.gif', ), ), 1 => array ( 'file' => '/wp-includes/class-wp-image-editor.php', 'line' => 423, 'function' => 'call_user_func_array', 'args' => array ( 0 => array ( 0 => Imagick::__set_state(array( )), 1 => 'writeImage', ), 1 => array ( 0 => '/wp-content/uploads/2016/12/2016-12-18-19-36_20161218_18-768x577.gif', ), ), ), 2 => array ( 'file' => '/wp-includes/class-wp-image-editor-imagick.php', 'line' => 486, 'function' => 'make_image', 'class' => 'WP_Image_Editor', 'type' => '->', 'args' => array ( 0 => '/wp-content/uploads/2016/12/2016-12-18-19-36_20161218_18-768x577.gif', 1 => array ( 0 => Imagick::__set_state(array( )), 1 => 'writeImage', ), 2 => array ( 0 => '/wp-content/uploads/2016/12/2016-12-18-19-36_20161218_18-768x577.gif', ), ), ), 3 => array ( 'file' => '/wp-includes/class-wp-image-editor-imagick.php', 'line' => 321, 'function' => '_save', 'class' => 'WP_Image_Editor_Imagick', 'type' => '->', 'args' => array ( 0 => Imagick::__set_state(array( )), ), ), 4 => array ( 'file' => '/wp-admin/includes/image.php', 'line' => 124, 'function' => 'multi_resize', 'class' => 'WP_Image_Editor_Imagick', 'type' => '->', 'args' => array ( 0 => array ( 'thumbnail' => array ( 'width' => '150', 'height' => '150', 'crop' => '', ), 'medium' => array ( 'width' => '300', 'height' => '300', 'crop' => false, ), 'medium_large' => array ( 'width' => '768', 'height' => '0', 'crop' => false, ), 'large' => array ( 'width' => '1024', 'height' => '1024', 'crop' => false, ), ), ), ), 5 => array ( 'file' => '/wp-admin/includes/media.php', 'line' => 373, 'function' => 'wp_generate_attachment_metadata', 'args' => array ( 0 => 313, 1 => '/wp-content/uploads/2016/12/2016-12-18-19-36_20161218_18.gif', ), ), 6 => array ( 'file' => '/wp-admin/includes/ajax-actions.php', 'line' => 2037, 'function' => 'media_handle_upload', 'args' => array ( 0 => 'async-upload', 1 => NULL, 2 => array ( ), ), ), 7 => array ( 'file' => '/wp-admin/async-upload.php', 'line' => 43, 'function' => 'wp_ajax_upload_attachment', 'args' => array ( ), ), ), 'previous' => NULL, ))
#6
@
8 years ago
Successfully hacked my way around the crash using the 'wp_image_editors' filter to disable the WordPress Imagick library. Now looking to see if there is still a problem with responsive images...
#7
@
8 years ago
Yes, bug confirmed. I uploaded a new GIF of exactly 600x600 using the GD library, inserted the thumbnail size into a new post, previewed the post, the srcset attribute contains only the thumbnail and medium sizes. The full size image is still broken for responsive image purposes.
#8
@
8 years ago
Next test:
Uploaded a new GIF of 700x700, inserted the thumbnail into a post, srcset shows thumbnail, medium, and my custom medium_hidpi sizes. The full size is still missing.
#9
@
8 years ago
- Version changed from 4.7 to 4.4
Confirmed the same bug existed in the original 4.4 feature set. However, behavior was different. If I insert a thumbnail size into a post in 4.4, the srcset attribute is missing completely. When I insert a medium size into a post, the srcset magically reappears.
#10
@
8 years ago
- Milestone changed from Awaiting Review to 4.8
Bug located in wp-includes/media.php:
/* * WordPress flattens animated GIFs into one frame when generating intermediate sizes. * To avoid hiding animation in user content, if src is a full size GIF, a srcset attribute is not generated. * If src is an intermediate size GIF, the full size is excluded from srcset to keep a flattened GIF from becoming animated. */
I'm going to propose a filter to turn off the broken logic for responsive GIF images. Setting milestone 4.8.
#11
follow-up:
↓ 12
@
8 years ago
Basically there are two issues. We need to look how to deal better when imagick breaks.
A filter for response images can be introduced but it would mean when the original is included, that other images disappear. Unless the editor did create images with animation saved but till now we don't have that (yet).
#12
in reply to:
↑ 11
@
8 years ago
- Keywords has-patch added
Replying to markoheijnen:
Basically there are two issues. We need to look how to deal better when imagick breaks.
A different ticket perhaps? I could not find a root cause of the crashes, other than pointing to the Imagick library itself.
A filter for response images can be introduced but it would mean when the original is included, that other images disappear. Unless the editor did create images with animation saved but till now we don't have that (yet).
Patch for new filter attached. Yes, it will "flatten" animated GIFs. I have no animated GIFs on my website whatsoever, which means my image quality suffers noticeably on HiDPI devices by default. Using this filter, I can inform core that my preference is to always have the srcset.
The only alternative I could think of was to use the 'wp_calculate_image_srcset_meta' filter to erase the mime-type value, which does not seem like a future-proof strategy.
#13
@
8 years ago
- Owner set to joemcgill
- Status changed from new to reviewing
Thanks @miqrogroove.
Briefly, I wanted to note that what you're experiencing is currently the intended behavior based on #34528. However, I understand how this could be less than ideal when the GIF is not animated and including the full size in the srcset
would be desired.
I don't think we actually need a new filter here, because you should be able to accomplish what you are after with the wp_calculate_image_srcset
filter by adding the full size to the srcset
after it has been calculated. You can even pass the $image_meta
directly to your filter to check the MIME type and add the information for the full size image.
A potential fix would be finding a better way to determine when a GIF is animated. But we would need to do so without adding unnecessary processing time to wp_calculate_image_srcset()
since it's run as a display filter on the front end. If we aren't able to do so, I'm afraid this will probably be a wontfix.
#14
follow-up:
↓ 15
@
8 years ago
As I mentioned above, if the new filter isn't accepted into core, I would simply use the existing 'wp_calculate_image_srcset_meta' filter to delete the mime-type value for all images. This isn't rocket science, and I'm not trying to re-invent the wheel.
Let the GIF logic run during upload and add needed details to the metadata. There is no need to do this on the front end.
#15
in reply to:
↑ 14
;
follow-up:
↓ 16
@
8 years ago
Replying to miqrogroove:
Let the GIF logic run during upload and add needed details to the metadata. There is no need to do this on the front end.
Unfortunately we still have to handle GIFs that are already uploaded to media libraries, so adding new logic when a GIF is processed is only a partial (though potential) solution.
#16
in reply to:
↑ 15
@
8 years ago
Replying to joemcgill:
Replying to miqrogroove:
Unfortunately we still have to handle GIFs that are already uploaded to media libraries, so adding new logic when a GIF is processed is only a partial (though potential) solution.
Why? They are already broken and a few comments ago you were ready to wontfix the whole thing?
Starting to look like a bizarre GIF-related problem. When I tried to upload a fresh image to test this bug, all of the resized image files were 10 bytes. :(