1684688 - AVIF Image with alpha/transparency isn't displayed properly

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Yuan Tong]

Attachment: firefox-10bit-420.avif

Here are multiple problems cause AVIF image with alpha not display properly:

1. ConvertYCbCrAToARGB call libyuv's I420AlphaToARGB whenever the image's yuvtype is YUV420 (YV12 in gfx/ycbcr/YCbCrUtils.cpp), but I420AlphaToARGB can only do 8bit. (Defect: firefox-10bit-420.avif)
2. FillAlphaToRGBA only consider 8bit case. (Defect: firefox-10bit-422.avif, firefox-10bit-444.avif)
3. Only MacOS's graphics stack expect image to be alpha-premultiplied (libyuv calls it preattenuate). On Windows and Linux half-transparent area (the border) appeared darkened. (Defect: firefox-10bit-420.avif, firefox-10bit-422.avif, firefox-10bit-444.avif)

All images should look the same as the PNG one, but Firefox is handling full range AVIF as limited range (see bug 1654461) so they will look slightly brighter even all the problems listed above are fixed.

Comment 1

[Yuan Tong]

Attachment: firefox-10bit-422.avif

Comment 2

[Yuan Tong]

Attachment: firefox-10bit-444.avif

Comment 3

[Yuan Tong]

Attachment: firefox.png

Comment 4

[Yuan Tong]

I'm getting something wrong with premultiply alpha. I've heard that Mac OS always wants premultiply, but I only have Windows and Linux to test on, so I'm assuming it's system difference. But It turns out to be we are doing premultiply twice: once in YUVA->RGBA, and we also asked downstream SurfacePipe to do premultiply too.

Now I'm able to fix eveything myself, if no one is already working on it, I would like to submit a patch.

Comment 5

[Yuan Tong]

Attachment: Bug 1684688 - Fix alpha support in nsAVIFDecoder r?jbauman

This patch fix transparency issues for AVIF image files:

1. Fix handling of high depth (>8) images.
2. Fix handling of premultiplied alpha.

For 1, this patch limited usage of libyuv::I420AlphaToARGB to
8bit I420 only. Other cases rely on ConvertYCbCrToRGB and alpha
is filled into ARGB buffer after.

For 2, this patch follows nsPNGDecoder to construct a SurfacePipe
to do alpha (un)premultiply if necessary.

Comment 6

[Jon Bauman [:jbauman:]]

Thanks for the submission! I'm working through the post-holiday backlog, but I'll try to review it this week.

Comment 7

[Jon Bauman [:jbauman:]]

Thanks again for the submission! I have started looking at this, and should be able to complete my review early this coming week.

Comment 8

[Jon Bauman [:jbauman:]]

My apologies for the delay. I've been reviewing your patch and it led to some further investigation around the code in that area. I definitely want to address the bug, and the code you submitted does that well. However, for the sake of future maintainability, I'd like to take a slightly different approach which I think make future work and maintenance in this area simpler and less error-prone. These come down to a few broad categories:

1. Rather than adding additional functionality to the surface pipe and swizzle filter to do the alpha premultiplication (or unpremultiplication) and having that logic split across libyuv and Gecko's surface pipe code, I'd like to handle all the alpha multiplication related operations via libyuv. Since we already use ARGBAttenuate, which is a thin wrapper around the libyuv::ARGBAttenuate function, it's relatively simple to add a wrapper for libyuv::ARGBUnattenuate, thereby allowing us to handle all cases with less new code.

2. Additionally, keeping all this functionality in libyuv, allows the logic in nsAVIFDecoder to be simplified. Rather than having gfx::ConvertYCbCrAToARGB called with premultiplyAlpha always set to false and later setting the surface pipe flags to either premultiply or unpremultiply, we can change the boolean premultiplyAlpha parameter to gfx::ConvertYCbCrAToARGB into a new 3-state PremultOpType enum, allowing this function to premultiply, unpremultiply or leave the RGB channels unchanged as needed:

     if (hasAlpha) {
       const auto wantPremultiply =
           !bool(GetSurfaceFlags() & SurfaceFlags::NO_PREMULTIPLY_ALPHA);
       const bool& hasPremultiply = decodedData.mPremultipliedAlpha;
   
       auto premultOp = PremultOpType::Noop;
       if (wantPremultiply && !hasPremultiply) {
         premultOp = PremultOpType::Premult;
       } else if (!wantPremultiply && hasPremultiply) {
         premultOp = PremultOpType::Unpremult;
       }
   
       gfx::ConvertYCbCrAToARGB(decodedData, format, rgbSize, rgbBuf.get(),
                                rgbStride.value(), premultOp);
     } else {
   

3. Rather than modifying FillAlphaToRGBA to handle different color depths, I'd prefer to extend ConvertYCbCrToRGBInternal to be able to handle input with an alpha plane itself, allowing us to reuse the existing ConvertYCbCr16to8Line to downsample the alpha channel and then move the call to the existing version of FillAlphaToRGBA inside ConvertYCbCrToRGBInternal since all the input will be 8-bit at that point. This keeps the bit-depth assumptions more localized, hopefully making the transition to higher bit depth support in the future simpler.

Additionally, looking at your fixes has made me realize there are two other things I want to clean up while doing this work:

1. Having a special case for in ConvertYCbCrAToARGB to call ConvertI420AlphaToARGB no longer seems necessary. Since we're using the libyuv ARGBAttenuate and ARGBUnattenuate functions, we can make things simpler and less redundant by removing it.
2. In checking that all the cases for alpha conversion were properly handled, I looked at the code in ConvertYCbCrToRGBInternal. The if (aDestSize != srcData.mPicSize) case is quite complicated and hard to read due to the extensive use of #ifdef. However, I don't believe this path can ever be reached any longer since we have higher quality scaling applied later in the pipeline, so I'm inclined to remove it after I confirm the code is dead.

I realize all these changes add up to a very different diff than what you've suggested, so if you'd like me to start over with a new phabricator review based on the approach I describe above, I'd be happy to do that and you're welcome to provide feedback. On the other hand, if it's important to you to be the author of the patch and you're willing to incorporate my feedback (which I can provide as a diff for you to apply on top of yours), I'm happy to go that route as well. Please let me know what you prefer.

Comment 9

[Yuan Tong]

I realize all these changes add up to a very different diff than what you've suggested, so if you'd like me to start over with a new phabricator review based on the approach I describe above, I'd be happy to do that and you're welcome to provide feedback. On the other hand, if it's important to you to be the author of the patch and you're willing to incorporate my feedback (which I can provide as a diff for you to apply on top of yours), I'm happy to go that route as well. Please let me know what you prefer.

I'm fine with both ways. I would prefer the one if it can be faster. (I'd like to see this bug and the full range one fixed before Firefox 86 is released)

Beside that, I would like to give some information. These may affect how you're going to change the code:

I checked ConvertI420AlphaToARGB (libyuv::I420AlphaToARGB), it's quite easy to extend it to I422 and I444 variant, which fills alpha faster than the scalar function FillAlphaToRGBA. I've done implementing and testing it on x86 and arm, now just a bit struggling to set up test environment for mips simd.

I'm also making a change to libyuv to make it able to init struct YuvConstants to proper value with given matrix coefficients and range. There are many colorspaces AVIF has but firefox is not supported currently, and this change will help firefox supports many of them (all the common ones and also some less common ones). Besides that, it will also solve the full range bug. To use custom struct YuvConstants instead of hard-coded one, libyuv::I4??ToARGBMatrix and libyuv::I4??AlphaToARGBMatrix should be used instead.

Comment 10

[Jon Bauman [:jbauman:]]

I'm fine with both ways. I would prefer the one if it can be faster.

Ok, I'll start a separate revision, that should be the fastest way forward.

(I'd like to see this bug and the full range one fixed before Firefox 86 is released)

I would've liked that as well, but it's unfortunately too late in the release process for this change to make it in for 86. It's a high priority, so it should be in nightly very soon barring any unexpected problems.

Regarding libyuv::I420AlphaToARGB, I would like so see support for 422 and 444 in libyuv and will be happy to make use of that code once it exists. Similarly, proper color space support is a high priority, but those discussions belong in bug 1654461 and bug 1634741.

Comment 11

[Jon Bauman [:jbauman:]]

Attachment: Bug 1684688 - Fix alpha support in nsAVIFDecoder. r=aosmond

Comment 12

[Jon Bauman [:jbauman:]]

Attachment: Bug 1684688 - Fix alpha support in nsAVIFDecoder. r=aosmond

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7c938c3c9942
Fix alpha support in nsAVIFDecoder. r=aosmond

Comment 14

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/7c938c3c9942

Comment 15

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:jbauman, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.