Make WordPress Core

Opened 8 years ago

Last modified 3 days ago

#36477 assigned defect (bug)

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

Reported by: peterdavehello's profile peterdavehello Owned by: adamsilverstein's profile adamsilverstein
Milestone: Future Release Priority: normal
Severity: normal Version: 4.4.2
Component: Media Keywords: has-patch needs-testing dev-feedback has-unit-tests
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 (3)

36477_1.diff (1.0 KB) - added by codex-m 8 years ago.
Retain number of unique colors when resizing, save file size
36477_2.diff (2.0 KB) - added by codex-m 8 years ago.
Use getImageType() to check if the image is indexed color encoded
basi0g16.png (299 bytes) - added by nosilver4u 11 days ago.
16-bit grayscale PNG image

Download all attachments as: .zip

Change History (71)

#1 @Presskopp
8 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 8 years ago by Presskopp (previous) (diff)

#2 @joemcgill
8 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
8 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
8 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
8 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
8 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.


8 years ago

#8 @azaozz
8 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
8 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
8 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
8 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 8 years ago by nosilver4u (previous) (diff)

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


8 years ago

#13 @joemcgill
8 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
8 years ago

Retain number of unique colors when resizing, save file size

#14 @codex-m
8 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
8 years ago

  • Milestone changed from Future Release to 4.7

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


8 years ago

#17 @desrosj
8 years 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.


8 years ago

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


8 years ago

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


8 years ago

#21 @kirasong
8 years 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 8 years ago by kirasong (previous) (diff)

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


8 years ago

#23 follow-up: @codex-m
8 years 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 @kirasong
8 years 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
8 years ago

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

#25 @codex-m
8 years 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
8 years ago

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

#27 @codex-m
8 years 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
8 years 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
8 years ago

#39236 was marked as a duplicate.

#30 @peterdavehello
8 years 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!

#31 @justlevine
5 years ago

Just wondering: does this address that WordPress upscales all 8-bit pngs to 16-bit, essentially doubling the size?

Because that's what's currently happening on all my sites.

While I saw mentioned how optimization on a resize might map new colors, but not sure if that translates to 'we need ImageMagick to change our pngs to 16-bit'.

If it's a separate issue, let me know and I'd be happy to open a new ticket.

#32 @cezza
5 years ago

What's happening to this ticket? Happy that @justlevine commented recently, after 2 years of inactivity. This is a very big issue that actually is preventing a lot of website to switch to the built-in solution wordpress provides.

https://www.dropbox.com/s/j2t5yu0rhhwyfdy/Screenshot%202019-01-04%2010.20.13.png

Check this screenshot to see how bad this problem can be.

#33 @peterdavehello
5 years ago

Friendly ping @joemcgill as it's been a few years here, it was v4.4.2 and it's almost v5.2 now, WordPress still growths rapidly during this period, would be really great if this issue can be resolved, many thanks again!

#34 @peterwilsoncc
5 years ago

#47311 was marked as a duplicate.

#35 @azaozz
5 years ago

  • Milestone changed from Future Release to 5.4

Lets try to get 36477_2.diff in 5.4. Ideally it should be tested to confirm that it doesn't reduce image quality, and perhaps a unit test should be added.

#36 @jokanane
4 years ago

Is the patch 36477_2.diff still valid? I applied it in my local environment (WP 5.3.2), but I am still seeing the issue mentioned by @justlevine and #30402, where PNGs I've exported as 8-bit in Photoshop have generated pixel sizes that are much larger in kb than the original file.

346895 image-8bit-1024x520.png
23022  image-8bit-150x150.png
747603 image-8bit-1536x780.png
39277  image-8bit-300x152.png
214964 image-8bit-768x390.png
275028 image-8bit-768x432.png
303507 image-8bit-800x450.png
391111 image-8bit-800x600.png
163554 image-8bit.png

The original file in the test above is 1600x812px. The generated 1536x780px is 4.5 times larger.

I've tried both uploading an image file and running wp media regenerate, and see no difference with the patch.

I hope I'm wrong, and WP 5.4 with the patch will fix the issue...

#37 @audrasjb
4 years ago

  • Milestone changed from 5.4 to Future Release

Hi,

With 5.4 Beta 3 approaching and the Beta period reserved for bugs introduced during the cycle, this is being moved to Future Release. If any maintainer or committer feels this should be included or wishes to assume ownership during a specific cycle, feel free to update the milestone accordingly.

#38 @sallyruchman
4 years ago

Hello,
I have this issue with WordPress Version 5.3.2. On Dev Website all Plugins disabled, just Theme (Divi Theme 4.4.0) activated. File Optimization done before with Shortpixel, after Upload Images to WordPress, the created Responsive Image Sizes have bigger File Size than the Original File.

Filename: Filesize:
Image-shortpixel-lossy.jpg 58465
Image-shortpixel-lossy-980x735.jpg 144868
Image-shortpixel-lossy-768x576.jpg. 100274
Image-shortpixel-lossy-1280x960.jpg 215971
Image-shortpixel-lossy-1080x810.jpg. 168313
Image-shortpixel-lossy-1080x675.jpg 149977
Image-shortpixel-lossy-1024x768.jpg. 154929

Is there a solution / fix ?
Thx

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

#39 @jokanane
4 years ago

What is the process to flag this for a higher priority? The generated PNGs are just way too big. Thanks.

The original image:

  • 1600x812px
  • 8-bit / 256 colors / palette
  • 164kb

Generated files:

  • 1536x780px: 748kb
  • 768x432px: 275kb
  • etc.
Last edited 4 years ago by jokanane (previous) (diff)

#40 @JeffPaul
4 years ago

  • Keywords needs-unit-tests added

#41 @wpwebdevj
4 years ago

After 5 years this still hasn't been fixed? Is anyone working on it?

#42 @wpfed
3 years ago

I noticed that when uploading a 8bit png image WordPress would resize and change bit depth to 24 or 32 bit, which created file sizes multiple times bigger. Opened ticket https://core.trac.wordpress.org/ticket/53408#ticket

#43 @mukesh27
2 years ago

Hi there,

No progress in the last some time.

@azaozz is this ticket is in your radar for the upcoming release? Happy to test the new patch or another approach.

#44 @wpfed
2 years ago

@mukesh27 They're looking at it in the WP performance plugin https://github.com/WordPress/performance/issues/264

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


11 months ago

#46 @joemcgill
10 months ago

  • Owner joemcgill deleted
  • Status changed from accepted to assigned

Removing myself as owner on this ticket (that I didn't realize I was still assigned to) to help unblock others from picking it up, if desired.

This ticket was mentioned in PR #6455 on WordPress/wordpress-develop by @pbearne.


7 weeks ago
#47

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

A new functionality has been added to the WP Image Editor class to check if an image is indexed-color encoded. This property is considered when saving a resized image to preserve its original image type. This reduces instances where the resized image filesize would end up larger than the original. A test to verify smaller file size for resized images has also been included.

#48 @pbearne
7 weeks ago

  • Owner set to pbearne

I have refreshed the code and added a test
But the 8 bit level is not being changed when resizing has this been fixed elsewhere?

#49 @nosilver4u
5 weeks ago

I've been working on a fix for this in EWWW Image Optimizer, and pretty sure it has not been fixed. I have multiple paletted PNG images that have thumbs much larger than the original. I would note, checking that an image is 8-bit is not sufficient, one needs to make sure it is also paletted (PNG color type 3).
I am definitely curious if your fix works though, as I've tried a couple other pure-PHP fixes: quantizing and/or prefixing the filename with 'PNG8:'. The former introduces some ugly "noise" around transparent boundaries, and the latter just flat-out failed for me on both IM 6.9 and 7.1.
Because EWWW IO has pngquant available (or an image-processing API), I ended up using pngquant to reduce the palette accordingly, and that worked very nicely.

#50 @pbearne
5 weeks ago

@nosilver4u Can you provide an example image for me to test with?

I wonder if we can add pngquant to WP :-)

#51 @nosilver4u
5 weeks ago

Since I don't own some of these, I can't attach them directly. I've got one provided by a customer for testing, several from related tickets here on trac, and a couple free/public domain images:
https://ewwwio-downloads.b-cdn.net/png-tests/17-c3-duplicate-entries.png
https://ewwwio-downloads.b-cdn.net/png-tests/cloudflare-status.png
https://ewwwio-downloads.b-cdn.net/png-tests/deskcat8.png
https://ewwwio-downloads.b-cdn.net/png-tests/Palette_icon.png
https://ewwwio-downloads.b-cdn.net/png-tests/rabbit-time-paletted.png
https://ewwwio-downloads.b-cdn.net/png-tests/test8.png

Out of all of these, only Palette_icon.png seems to be "fixed" by your patch. I haven't done any debugging to find out why, but I suspect it might be similar to this "fix": https://stackoverflow.com/a/26434303/2882573
When I tried that, I got no thumbs at all because Imagemagick was throwing an error very similar to this: https://github.com/ImageMagick/ImageMagick/discussions/3269
The quantizeImage() idea "works" but on deskcat8.png it introduces (lots of) black pixels around the edges, similar to observations here: https://stackoverflow.com/questions/14031965/convert-32-bit-png-to-8-bit-png-with-imagemagick-by-preserving-semi-transparent/14032098#14032098

Unfortunately, as pngquant is a command line binary, I'm pretty sure that's a non-starter for core WP.

#52 @pbearne
5 weeks ago

I have added the images to the test and @nosilver4u said only one image doesn't increase in size

We need to see it we can find a way to stop this

#53 @nosilver4u
5 weeks ago

Do you think continuing this discussion would help:
https://github.com/ImageMagick/ImageMagick/discussions/3825

Or perhaps starting a new one?

#54 @nosilver4u
4 weeks ago

Well, I went ahead and asked for ideas on the Imagemagick discussion board, and the closest we got was this:

<?php
$colors = min( 255, $this->image->getImageColors() );
$this->image->thresholdImage( .5, imagick::CHANNEL_ALPHA );
$this->image->quantizeImage( $colors, $this->image->getColorspace(), 0, false, false );

Which worked fine for deskcat8.png, but I tried it with another PNG8 that had transparency, and it put random dots in a grid across the whole image. They thought perhaps the deskcat8.png image just had a weird (hidden?) black background that was the source of the issues. I'm not certain how that is possible, but I'm no expert on the PNG spec.

At any rate, the quantizeImage() method is probably the best route forward, given that it solves the file size issues across all my test images and only has a small amount of degradation on that one image.

I'd recommend doing something a bit more dynamic than the existing patch though, and checking the pixel depth via https://www.php.net/manual/en/imagick.getimagedepth.php
Then one can limit the colors based on the depth:
depth 8: 256 colors
depth 4: 16 colors
depth 2: 4 colors
depth 1: 2 colors

#55 @pbearne
4 weeks ago

@nosilver4u

I had a go at getting to work and failed
can you share your working code?

Paul

#56 @nosilver4u
4 weeks ago

Hmm, I'm trying to implement it now using quantizeImage() as a fallback when pngquant isn't available in EWWW IO, but having trouble there also. I have a few more things to try/check, so I'll let you know what I find out.

#57 @nosilver4u
3 weeks ago

Alright, first off, I realized I gave you the wrong versions of a couple images. One had some weird artifacts, though I can't remember exactly how that happened. The other simply wasn't an indexed PNG, though it had few enough colors to be indexed, so here are the corrected images for testing:
https://ewwwio-downloads.b-cdn.net/png-tests/Palette_icon-or8.png
https://ewwwio-downloads.b-cdn.net/png-tests/rabbit-time-paletted-or8.png

Second, ImageMagick, or at least the imagick extension, I'm not sure which, seems to struggle at identifying palette PNG images. Any image with transparency was identified as color type 7, which is imagick::IMGTYPE_TRUECOLORMATTE. So that’s not helpful for the detection side. There are ways of getting around that, which I had already implemented for EWWW IO, and you’ll see included in my code (link below).

As I said before, the quantizeImage() method seems to be the key, but I was struggling to get the same results I saw earlier, when I first started fiddling with this. After further examination, it is also helpful to check how many colors the original image has. For example, test8.png only has 9 colors, but is saved with a bit/pixel depth of 8, which allows 256 colors. If we allow quantizeImage() to use 256 colors, we get a much larger file size.

Instead, we could limit the new/resized image to the same number of colors as the original. That gives us a smaller file size, but in some cases, I noticed the coloration wasn’t as good, so I would recommend giving a buffer of 8. With test8.png, that means we allow it to use up to 17 colors, and I always cap the maximum colors to the corresponding bit depth--256 for PNG8, 16 colors for bit depth 4, and so on.

There was still a niggling issue where Imagick would refuse to save images as color type 3 (palette) when there was transparency. Thanks to Danack (maintainer of Imagick), I found the key to fixing this was to preserve the tRNS chunk.

Unfortunately, even after reducing the colors and ensuring the use of a palette, some thumbs might still be a bit larger than the original. However, things are a LOT better than where we started.

Ultimately, I’m not certain how one can build tests for this, given there will always be some thumbs with file sizes larger than the original, but here is what I’ve put together for quantizing PNG images after resizing and ensuring they use indexed mode:
https://github.com/WordPress/wordpress-develop/compare/trunk...nosilver4u:wordpress-develop:png8-resize

Perhaps one can just build in some “grace” for the encoder in the tests, and consider things “passed” if the resized image is within 10% of the original. Or, you could just avoid trying to generate a 1500px version of a 2000px PNG during testing. For example, with deskcat8.png, resizing to 1140x1102 is just fine, but 1536x1485 was about 8kb larger than the original (2000x1934).

This ticket was mentioned in PR #6686 on WordPress/wordpress-develop by nosilver4u.


2 weeks ago
#58

This does the following to improve resizing of indexed PNG images:

  1. Added a method to get the PNG color type and pixel depth directly from the original image.
  2. Get the original number of colors in an indexed PNG prior to scaling the image.
  3. Preserve the tRNS chunk if an indexed PNG also has transparency.
  4. Use quantizeImage() to reduce the number of colors in the image after scaling.
  5. In cases where quantizeImage() converts the colorspace to grayscale, set png:format=png8 to force ImageMagick to output an indexed PNG.

Trac ticket: https://core.trac.wordpress.org/ticket/36477

This ticket was mentioned in Slack in #core-performance by pbearne. View the logs.


12 days ago

This ticket was mentioned in Slack in #core-performance by mukeshpanchal27. View the logs.


11 days ago

#61 @adamsilverstein
11 days ago

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

@peterdavehello thanks for reporting this, very interesting.

Thanks @nosilver4u for the detailed analysis and PR, I assume that replaces the two patched linked earlier in the ticket?

These fixes are for Imagick - does GD suffer from the same issue, or have you tested this? Fixing both would make sense if possible.

@peterdavehello have you considered using WebP as your output format (or even AVIF if your server supports it), I'm curious if that would get you smaller file sizes that way? Something like this gist should work.
note: Adding "neeeds unit tests" tag

#62 @nosilver4u
11 days ago

@adamsilverstein My patch mostly replaces that from @pbearne, but his has the unit tests, though they may need some tweaking still. I'll let him speak to that further so I'm not making commitments for him :)

As for WebP/AVIF, that doesn't solve the problem of reducing the palette back to the original number of colors. I tested WebP, and while it does admirably in some cases, it can still double or triple the file size without the palette reduction in place.

With palette reduction included via quantizeImage(), the WebP file size is even smaller than the resized & indexed PNGs, except for test8.png that Imagick likes to force into grayscale mode rather than indexed. I don't know what the equivalent fix is for WebP, but I had to force the PNG8 format for that particular case.

GD does suffer from the same issue, though it looks like there was a potential fix, but it didn't work for me: https://github.com/WordPress/wordpress-develop/blob/ac84bd53c6ab38cb6d808d30bc985ae84de2f0dc/src/wp-includes/class-wp-image-editor-gd.php#L525
The imageistruecolor() and imagecolorstotal() functions should run on the original image, and not the resized one, otherwise it doesn't convert the image back to palette/indexed. But even when you do, transparency is completely lost, so that's no good. That's as much as I care to mess with GD, so I wouldn't block this for lack of a GD fix which may not even be possible.

#63 @adamsilverstein
11 days ago

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

Thanks again for all the details @nosilver4u - I'll try combining the code from your PR with the unit tests from @pbearne and see if that passes.

As for WebP/AVIF, that doesn't solve the problem of reducing the palette back to the original number of colors. I tested WebP, and while it does admirably in some cases, it can still double or triple the file size without the palette reduction in place.

Ah, right - that is the critical piece here, the palette reduction.

I'll try giving this a test locally where I have several different versions of PHP/GD/Imagick running. Fine to fix just for Imagick for now, we can follow up with a GD fix.

Last edited 11 days ago by adamsilverstein (previous) (diff)

#64 @adamsilverstein
11 days ago

  • Owner changed from pbearne to adamsilverstein

@pbearne - I'm going to self-assign this one as a reminder to get it over the finish line, thanks for keeping it moving forward!

#65 @pbearne
11 days ago

I have been trying to create tests for WP_Image_Editor_Imagick::get_png_color_depth

But I can't get the code to return 16 or 32 bit they return 8

4-bit and 8-bit works

these tests fail tests/phpunit/tests/image/getPngColorDepth.php

#66 @nosilver4u
11 days ago

The terminology is a bit confusing there, but what WP_Image_Editor_Imagick::get_png_color_depth() checks is typically called the "pixel depth" or "bits per sample". Valid values for pixel depth are 1, 2, 4, 8, 16, and codeispoetry.png has a pixel depth of 8, color type 6 (RGBA), which means 32 bits per pixel.
http://www.libpng.org/pub/png/book/chapter08.html has more detail on all of that. I didn't see the 16-bit.png file in your fork, so I wasn't able to check that one. It can certainly be confusing, as I've seen people say "bit depth" in place of the "bits per pixel", but afaik the bit depth is how many bits are used for a single color/channel (R, G, B or A).

@nosilver4u
11 days ago

16-bit grayscale PNG image

#67 @nosilver4u
10 days ago

http://www.schaik.com/pngsuite/ if you need any more test cases for the detection side of things. Sadly, they aren't particularly useful for resizing, since they are all pretty small.

#68 @tb1909
3 days ago

Test report with PR https://github.com/WordPress/wordpress-develop/pull/6686:

I tested with the mentioned 8-bit images above:
https://ewwwio-downloads.b-cdn.net/png-tests/rabbit-time-paletted-or8.png
https://ewwwio-downloads.b-cdn.net/png-tests/Palette_icon-or8.png

Without the PR as tested with the current WordPress master the image was converted to a 32-bit-colored image on resizing
With the PR implemented the image stayed an 8-bit-image as it should

So the issue seems to be resolved with the PR.

Screenshots:
Before: https://pasteboard.co/GW3u1kMnT2DR.png
After: https://pasteboard.co/JKQsw719ggW9.png

Note: See TracTickets for help on using tickets.