WordPress.org

Make WordPress Core

Opened 11 months ago

Last modified 6 months ago

#39387 reviewing defect (bug)

Responsive Images Broken When Full Size <= 300 px

Reported by: miqrogroove Owned by: joemcgill
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)

miqro-39387.diff (1.3 KB) - added by miqrogroove 11 months ago.
Add filter for GIF image handling.

Download all attachments as: .zip

Change History (16)

#4 @miqrogroove
11 months ago

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. :(

#5 @miqrogroove
11 months 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 @miqrogroove
11 months 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 @miqrogroove
11 months 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.

Last edited 11 months ago by miqrogroove (previous) (diff)

#8 @miqrogroove
11 months 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.

Last edited 11 months ago by miqrogroove (previous) (diff)

#9 @miqrogroove
11 months 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 @miqrogroove
11 months 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: @markoheijnen
11 months 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).

@miqrogroove
11 months ago

Add filter for GIF image handling.

#12 in reply to: ↑ 11 @miqrogroove
11 months 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 @joemcgill
11 months 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: @miqrogroove
11 months 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: @joemcgill
11 months 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 @miqrogroove
11 months 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?

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


6 months ago

#18 @flixos90
6 months ago

  • Milestone changed from 4.8 to Future Release
Note: See TracTickets for help on using tickets.