Make WordPress Core

Opened 6 weeks ago

Last modified 4 days ago

#62285 assigned defect (bug)

HDR AVIF is corrupt in the media library (encoded as SDR)

Reported by: gregbenz's profile gregbenz Owned by: adamsilverstein's profile adamsilverstein
Milestone: 6.8 Priority: normal
Severity: normal Version: 4.5
Component: Media Keywords: has-patch needs-testing
Focuses: Cc:

Description

When a 10-bit HDR AVIF image is uploaded to the WP media library, it gets transcoded to 8-bit with an SDR EOTF. The result is the image is unusable as it is significantly degraded from the original (the change in EOTF here results in invalid interpretation of the pixel values, and the degradation from 10 to 8-bit creates significant risk of banding for an HDR image).

expected result: the following EXIFTOOL command should show the same values for both source and the newly transcoded image when the source is HDR (ie has a PQ or HLG transfer function):

exiftool -imagepixeldepth -transfercharacteristics myFile.JPG

Note that the following test with ImageMagick 7.1.1-39 Q16-HDRI aarch64 22428 produced a valid output. (I tested WP with a slightly different version in my hosting environment: ImageMagick 7.1.1-38 Q16-HDRI x86_64 22398). Test command for local instance of IM:

magick sourceHDR.avif -resize 50% transcodedHDR.avif

The concern here is specific to HDR images. If the image is SDR, then converting 10-bit to 8-bit output would typically be a reasonable way to compress the image (the risk of banding is much lower for an SDR image). So the optimal result would be:

  • keep the original EOTF for all images
  • keep the original bit depth if HDR, but allow conversion to 8-bit if the the image is SDR.

Attachments (1)

wpConvertsHdrToSdr.zip (10.4 KB) - added by gregbenz 6 weeks ago.

Download all attachments as: .zip

Change History (13)

#1 @desrosj
6 weeks ago

  • Component changed from General to Media

#2 @adamsilverstein
6 weeks ago

  • Owner set to adamsilverstein
  • Status changed from new to assigned

#3 follow-up: @adamsilverstein
6 days ago

  • Milestone changed from Awaiting Review to 6.8

@gregbenz the 10-bit to 8bit was a good clue and I found we may actually be able to fix this for Imagick: I noticed we have some depth limiting functionality that might be the cause of what you are seeing. We currently have a hard coded maximum bit depth of 8 - anything higher and Imagick will reduce the output image depth to 8.

I assume this limit was introduced to ensure users weren't getting unnecessarily large / high bit depth images when the website users probably couldn't see these extra colors anyway. This assumption may no longer be true, what do you think?

For now, I feel adding a filter so users who explicitly want HDR photography can use it would be a safe and effective change.

I am proposing a filter in https://github.com/WordPress/wordpress-develop/pull/7923/files.

You can test this filter by adding this bit of code - raising the max bit depth to 10 - to your functions.php and trying your upload again:

add_filter( 'imagick_resized_image_max_bit_depth', function() { return 10; } );

I noticed we don't have any unit tests for getImageDepth so it would be nice to add some of those here as well.

cc: @kirasong who added the original depth limiting code.

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


6 days ago
#4

  • Keywords has-patch has-unit-tests added

#5 @adamsilverstein
6 days ago

  • Keywords has-patch has-unit-tests removed

I noticed we don't have any unit tests for getImageDepth so it would be nice to add some of those here as well.

I added a test that validates the max depth is set correctly, running now in CI.

#6 @gregbenz
6 days ago

@adamsilverstein Agreed, 8-bit was a safer choice in the past when most images had SDR luminance with Rec709 primaries. That no longer holds true.

10-bit is the minimum for quality HDR, and in some cases 12-bit may be required to avoid banding. I believe preserving the bit depth up to 12-bits is ideal to avoid risk of banding (visible quantization error).

10-bit actually matters for some SDR (standard images) too. We can see banding in smooth gradients at 8-bits even in SDR, and wider gamuts further increase risk of quantization error (since the bits are spread across a wider range of values). So allowing preservation of 10-bit even for SDR is probably ideal, especially as encoding with Rec2020 primaries may become more common over time.

#7 @adamsilverstein
6 days ago

8-bit was a safer choice in the past when most images had SDR luminance with Rec709 primaries. That no longer holds true.

10-bit is the minimum for quality HDR, and in some cases 12-bit may be required to avoid banding

@gregbenz - so this means you recommend changing the default maximum bit depth to 10 or 12?

What would the impact on file size be?

We would certainly need this filter if we change the defaults, so adding it makes sense. Manual testing with the filter welcome.

#8 @gregbenz
6 days ago

@adamsilverstein I believe the simplest approach would be to stay at the same bit depth, up to a maximum of 12. This would ensure retention of suitable quality without penalty if the original is encoded in an appropriate bit depth.

If your assumption is that many people will create originals with higher bit depth than necessary and you wish to prioritize small file size, you could take a more complex approach - at the risk of some edge cases where unwanted banding is created. For example, you could use 8-bits for SDR EOTFs and 10-bit for HDR EOTFs. That’s fairly crude, but probably safe for 99% of images. If a way for a user to select higher quality could be offered, that could be a good choice.

Ultimately, the benefit for higher bit depths (SDr at 10, HDR at 12) would mostly likely show when there is a smooth gradient across a sufficient tonal range. 10-bit encoding does generally work well for HDR capabilities we have today, 12-bit is probably more of a consideration for future proofing.

I’m not aware of an efficient way to assess that in code, and I would recommend keeping source depth or perhaps using the 10-bit HDR with a user option to go higher if needed.

As far as file size impact, I don’t have data on it. I expect that 12-bit encoding will rarely be used where not necessary, so probably minimal impact in real use. Compression may impact the results, but you might estimate 20% bloat if 12-bit were needlessly used in the source (ie missed opportunity to improve). I would err on the side of assuming that if the image was exported at 12-bit that it is more likely beneficial and squeezing to 10 risks banding.

@adamsilverstein commented on PR #7923:


5 days ago
#9

Looks good to me besides the noted Coding Standards error.

thanks for reviewing. we still need to determine why some unit tests are failing.

#10 in reply to: ↑ 3 @kirasong
4 days ago

Replying to adamsilverstein:

cc: @kirasong who added the original depth limiting code.

Thanks so much for the ping!
I love changes like this to help folks' images look their best.

Yes, it's my recollection that it was done for the purpose of file size.

This was a long time ago, but I also have a suspicion that a bit depth of some sort needs to be set for resizing to work properly on various versions of Imagick.

I think it's a great idea to set it to the same bit depth as the source image, and when I was looking, it seems that was my preference 9 years ago (wow, it's been some time!).
https://core.trac.wordpress.org/ticket/33642#comment:33

I also see @joemcgill dug into this, and might remember more details on the specifics.

He noted that at the time, it seem[ed] to be the default way a few image editors (including Photoshop) handle optimizing images for web (even PNG24s).
https://core.trac.wordpress.org/ticket/33642#comment:36

#11 @kirasong
4 days ago

  • Keywords has-patch added

#12 @kirasong
4 days ago

  • Keywords needs-testing added
  • Version set to 4.5
Note: See TracTickets for help on using tickets.