WordPress.org

Make WordPress Core

Opened 2 years ago

Last modified 18 months ago

#36477 accepted defect (bug)

Responsive images (srcset) can include images larger than the full size

Reported by: peterdavehello Owned by: joemcgill
Milestone: Future Release Priority: normal
Severity: normal Version: 4.4.2
Component: Media Keywords: has-patch needs-testing dev-feedback
Focuses: performance Cc:

Description

In many cases, I saw the resized and smaller images are much larger than the origin image, especially for the optimized images, it will make no sense to do that resize in this case, the worst case I've seen is about 13x larger than the origin and bigger image.

If an example can help to explain the problem, please take this picture: https://cdn2.peterdavehello.org/wp-content/uploads/2016/04/status.png

Many thanks!

Attachments (2)

36477_1.diff (1.0 KB) - added by codex-m 2 years ago.
Retain number of unique colors when resizing, save file size
36477_2.diff (2.0 KB) - added by codex-m 20 months ago.
Use getImageType() to check if the image is indexed color encoded

Download all attachments as: .zip

Change History (32)

#1 @Presskopp
2 years ago

The owner of this website (cdn2.peterdavehello.org) does not allow hotlinking to that resource (/wp-content/uploads/2016/04/status.png).

with http instead of https it works

Last edited 2 years ago by Presskopp (previous) (diff)

#2 @joemcgill
2 years ago

  • Owner set to joemcgill
  • Status changed from new to reviewing

Hi @peterdavehello,

Welcome to the Trac, and thank you for the report.

I assume that your are referring to the file size (KB) of the images and not the dimensional size of the images (width x height). In which case, you are correct. Resizing an indexed color image like a PNG or GIF to a smaller size can oftentimes increase the overall file size because of the way the resampling process works (see: #30402). This is not an uncommon result among other image editors as well, but is most noticeable in use cases where the source image has gone through an optimization process before being uploaded.

The default image compression improvements that will be included in WordPress 4.5 (see: #33642) help with this problem, but the smaller images can still be larger than the full size images at times. However, for users who aren't optimizing their images before uploading (a majority of users, I suspect), using srcset and sizes often results in a significant reduction in file size, so we

Since WordPress has no awareness of the file size of each image when it is writing the image markup, I'm not sure what we can do to avoid this situation entirely, except to continue to make improvements on the default compression strategy for indexed color images.

#3 follow-up: @peterdavehello
2 years ago

Hi @joemcgill,

Thanks for your prompt reply in detail, yes, I'm talking about the file size, not the dimensional size, and my basic idea is try to get the size before and after the resize, once the resized images is larger, than we drop that size of image, what do you think?

#4 in reply to: ↑ 3 @joemcgill
2 years ago

  • Keywords needs-patch added

Replying to peterdavehello:

Hi @joemcgill,

Thanks for your prompt reply in detail, yes, I'm talking about the file size, not the dimensional size, and my basic idea is try to get the size before and after the resize, once the resized images is larger, than we drop that size of image, what do you think?

I would be careful about dropping images altogether, but it may be possible to save the file size as attachment metadata and factor that into our srcset considerations in wp_calculate_image_srcset().

#5 @peterdavehello
2 years ago

Hi @joemcgill,

Seems the problem is worse then I said originally, actually I found that some pictures didn't be manually optimized still have the same problem with this resize, for example, I use the PrintScreen key under Linuxmint 17.3 Cinnamon to catch the screen, the process never takes more than few seconds which means there is no additional process to optimize the image, but it's still become larger after the resize in WordPress, that makes no sense.

#6 follow-up: @joemcgill
2 years ago

  • Milestone changed from Awaiting Review to 4.6
  • Status changed from reviewing to accepted

The idea of trying to explicitly make file size a consideration when calculating srcset attributes. However, this will require us to add the file size of each image as attachment metadata. Perhaps we could use Imagick::getImageLength during the resize process to grab this info?

This ticket was mentioned in Slack in #core-images by joemcgill. View the logs.


2 years ago

#8 @azaozz
2 years ago

the resized and smaller images are much larger than the origin image, especially for the optimized images.

I'm thinking we need a better workflow for people that optimize their images before uploading them. Currently the steps are:

  1. Upload optimized image. WordPress creates non-optimized sub-sizes and the image meta to go with them.
  2. Create images with the exact dimensions like the WordPress sub-sizes.
  3. Rename them to match the file names generated by WordPress.
  4. Upload the optimized sub-size images with FTP replacing the non-optimized.

Of course setting GD and Imagick to create optimized images would be best, but I'm not sure if we can enhance that further.

Another possibility is to make a case with the hosting companies to install one of the jpeg optimizers (libjpeg/jpegtran?) and add cron job to scan the uploads directory and optimize the sub-sizes (if they don't do that already). This will reduce both bandwidth and space usage :)

In WordPress we could probably make the uploader "recognize" when the user is uploading a sub-size of an image and just replace the file instead of treating it as a new upload. That will let the users create optimized sub-sizes and upload them together.

#9 follow-up: @nosilver4u
2 years ago

Just curious, is the problem generally negated if you are "post-optimizing" your images? That is, using a plugin like EWWW Image Optimizer or one of the other I.O. plugins...

#10 in reply to: ↑ 9 @azaozz
2 years ago

Replying to nosilver4u:

Of course. Post-upload optimizing is probably the best way to do this as long as the plugin optimizes the images well and doesn't make you over-use the resources if on a shared server.

This is also better for the hosting company, hence my suggestion to do it automatically by a script when server load is low.

#11 in reply to: ↑ 6 @nosilver4u
2 years ago

Replying to joemcgill:

The idea of trying to explicitly make file size a consideration when calculating srcset attributes.

The unfun side-effect of that is all the I.O. plugins need to make sure to update that information in the metadata after optimization as well. And I don't know that you need to use the Imagick method to get the file size, do you? Wouldn't a quick call to filesize() be sufficient after the resizes has been saved? Also, filesize() would be "editor-agnostic" since you have the potential for GD, Imagick, maybe even Gmagick or others...

Last edited 2 years ago by nosilver4u (previous) (diff)

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


2 years ago

#13 @joemcgill
2 years ago

  • Component changed from Upload to Media
  • Milestone changed from 4.6 to Future Release

Punting due to a lack of patch and moving to the Media component so this can get more visibility in 4.7.

@codex-m
2 years ago

Retain number of unique colors when resizing, save file size

#14 @codex-m
2 years ago

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

@joemcgill I think we can handle this in coming 4.6. The trick is to check for unique colors before resizing. If its 256 or below, its indexed color type or already optimized. We implement a file size optimizing filter. In my test, FILTER_POINT works best.

I did several tests and it handles already compressed PNG images very well. So it means that after resizing, resulting file size is not very large. Since the number of unique colors will be preserved while not affecting quality ;)

Let me know what you think. Cheers!

Emerson

#15 @joemcgill
22 months ago

  • Milestone changed from Future Release to 4.7

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


21 months ago

#17 @desrosj
21 months ago

This will get some attention during the next media component meeting. A decision to land in 4.7 will be made.

This ticket was mentioned in Slack in #core-images by joemcgill. View the logs.


21 months ago

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


21 months ago

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


21 months ago

#21 @mikeschroder
21 months ago

  • Keywords needs-patch added; has-patch needs-testing removed
  • Milestone changed from 4.7 to Future Release
  • Summary changed from Responsive images(srcset) feature makes images fatter. to Responsive images (srcset) can include images larger than the full size

I tested 36477_1.diff today -- Thanks for the patch!

Unfortunately, it looks like running getImageColors is between 8% and 75% of image upload times, with only small images (~120x120) being nearer to that 8%.

In the tests I ran (on a standard shared server), this added up to an additional 19.4 seconds to resize time for iPhone 7-sized images.

This means that if we're going to go with a different method of resizing for different amounts of colors present, it'll have to have a different way to detect the amount of colors.

I'd be interested in seeing what a solution that includes saving the sizes of images on upload/resize would look like. This would make the data available to the srcset code, for better choices on which images are good to include.

I'm going to remove the 4.7 milestone, but if this ticket gets a patch that is ready for 4.7, am more than happy to take another look. Also updating the title to more accurately reflect the nature of the bug.

Edit: Fixed incorrect number of seconds

Last edited 21 months ago by mikeschroder (previous) (diff)

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


20 months ago

#23 follow-up: @codex-m
20 months ago

Thank you @mikeschroder for feedback and testing! I agree that getImageColors() will introduce a performance problem especially when re-sizing very large images. It is because processing time is directly proportional to the total number of pixels on the image.

I'm currently testing a new patch which does not use this function while still solving the main issue and not introducing any new issues. It's looking good so far and will post it here very soon ;) I'm hoping we could introduce this in 4.7. For your update. Cheers.

#24 in reply to: ↑ 23 @mikeschroder
20 months ago

Replying to codex-m:

Thank you @mikeschroder for feedback and testing! I agree that getImageColors() will introduce a performance problem especially when re-sizing very large images. It is because processing time is directly proportional to the total number of pixels on the image.

I'm currently testing a new patch which does not use this function while still solving the main issue and not introducing any new issues. It's looking good so far and will post it here very soon ;) I'm hoping we could introduce this in 4.7. For your update. Cheers.

No problem! Absolutely, if there is a patch before the deadline, it'll be considered for commit. Thanks for digging into this!

@codex-m
20 months ago

Use getImageType() to check if the image is indexed color encoded

#25 @codex-m
20 months ago

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

@mikeschroder Here it is. I attached 36477_2.diff. Basically I dig deeper into the issue and found out that this only affects indexed-color images. These images are limited to 256 colors. And when resized by WordPress, additional colors are added. And the image type is then converted from indexed color type to RGB true color. This increases the file size.

I also found out that to eliminate performance issue while checking for image colors. Its best to check for image type only using Imagick::getImageType. This will return if the image is indexed-color encoded or RGB.

The fix is simple, it then checks if the image type is indexed color. If yes, retains the image type after saving. This dramatically reduces the file size for affected image types while retaining our existing filters/quality settings ;)

In my test, this fix reduces the file size of indexed color images resizing to around 50%. This fix also resolves the following existing WordPress trac tickets like this: https://core.trac.wordpress.org/ticket/30402. It is because after resizing, the image type is now retained.

Let me know what you think. Thanks!

#26 @peterdavehello
20 months ago

AWESOME!!!!!!! Thanks @codex-m !!!!!!!!

#27 @codex-m
20 months ago

  • Keywords dev-feedback added

Any chance we can move this to 4.7 milestone? We are managing several big sites. It would be good to have this included in the coming release because it would lead to smaller backup files for uploads directory in the future :) Thanks for any feedback.

#28 @joemcgill
20 months ago

Hi @codex-m,

Thanks for following up on this and for continuing to push it forward. Unfortunately, we're a few weeks too late for this cycle, but definitely something we can evaluate for 4.8.

#29 @joemcgill
18 months ago

#39236 was marked as a duplicate.

#30 @peterdavehello
18 months ago

As this looks to be a strange bug which makes the size of the uploaded png images super fat and then wastes the bandwidth and disk space, would you guys consider to provide fix tool or plugin or something like that to scan the scaled png images and make them to use the indexed-color as the origin images to "fix" the size problem? Could be super helpful! Thanks a lot!

Note: See TracTickets for help on using tickets.