2079663 - jxl decoder doesn't handle files that are premult alpha correctly

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Timothy Nikkel (:tnikkel)]

conformance image and ref
https://github.com/libjxl/conformance/blob/94e422259caa9390da2ea7653cf610a8dd3d529f/testcases/alpha_premultiplied/input.jxl
https://github.com/libjxl/conformance/blob/94e422259caa9390da2ea7653cf610a8dd3d529f/testcases/alpha_premultiplied/ref.png

Comment 1

[Timothy Nikkel (:tnikkel)]

Attachment: Bug 2079663. Rename nsJXLDecoder::mU8RowBuf to mPipeInputRowBuf. r?#gfx-reviewers,saschanaz

The name says what type it holds rather than what it is for. The next patch
adds a second row buffer, so name this one after its role: the row we hand to
the surface pipe when it can't read mPixelBuffer directly.

Comment 2

[Timothy Nikkel (:tnikkel)]

Attachment: Bug 2079663. Don't premultiply JXL images that have premultiplied alpha a second time. r?#gfx-reviewers,saschanaz

https://searchfox.org/firefox-main/rev/3c71a541b0e7ac6e086907b38670dabca9174c45/image/decoders/nsJXLDecoder.cpp#607

We assume jxl-rs always outputs unpremultiplied alpha, but for images with
premultiplied (associated) alpha it outputs the premultiplied values stored in
the file. We then premultiply them again, so semi-transparent areas come out
too dark. Color management and surfaces that want unpremultiplied data
(NO_PREMULTIPLY_ALPHA) also got premultiplied data.

Now we unpremultiply when color management or the surface needs it, and
otherwise tell the surface pipe not to premultiply. The spec doesn't say what
color values larger than alpha mean; we clamp them to alpha so the result
doesn't depend on whether color management runs.

Comment 3

[Timothy Nikkel (:tnikkel)]

Attachment: Bug 2079663. Add WPT reftests for JXL images with premultiplied alpha. r?#gfx-reviewers,saschanaz

Premultiplied and non-premultiplied versions of the same RGB and gray grids,
each compared against one PNG, plus the conformance suite's
alpha_premultiplied image compared against its reference PNG. cjxl
--premultiply=1 only marks the pixels as premultiplied, so the premultiplied
grids are encoded from PNGs that were premultiplied first.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/d51576d514bb
https://hg.mozilla.org/integration/autoland/rev/d495883d79a7
Rename nsJXLDecoder::mU8RowBuf to mPipeInputRowBuf. r=saschanaz
https://github.com/mozilla-firefox/firefox/commit/31349ea2b83e
https://hg.mozilla.org/integration/autoland/rev/cda3ed9c9aaa
Don't premultiply JXL images that have premultiplied alpha a second time. r=saschanaz
https://github.com/mozilla-firefox/firefox/commit/d6ba1a3081a9
https://hg.mozilla.org/integration/autoland/rev/895ec42d7aba
Add WPT reftests for JXL images with premultiplied alpha. r=saschanaz

Comment 5

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/d495883d79a7
https://hg.mozilla.org/mozilla-central/rev/cda3ed9c9aaa
https://hg.mozilla.org/mozilla-central/rev/895ec42d7aba

Comment 6

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/63495 for changes under testing/web-platform/tests

Comment 7

[Timothy Nikkel (:tnikkel)]

Attachment: Bug 2079663. Rename nsJXLDecoder::mU8RowBuf to mPipeInputRowBuf.

The name says what type it holds rather than what it is for. The next patch
adds a second row buffer, so name this one after its role: the row we hand to
the surface pipe when it can't read mPixelBuffer directly.

Original Revision: https://phabricator.services.mozilla.com/D331947

Comment 8

[Timothy Nikkel (:tnikkel)]

Attachment: Bug 2079663. Don't premultiply JXL images that have premultiplied alpha a second time.

https://searchfox.org/firefox-main/rev/3c71a541b0e7ac6e086907b38670dabca9174c45/image/decoders/nsJXLDecoder.cpp#607

We assume jxl-rs always outputs unpremultiplied alpha, but for images with
premultiplied (associated) alpha it outputs the premultiplied values stored in
the file. We then premultiply them again, so semi-transparent areas come out
too dark. Color management and surfaces that want unpremultiplied data
(NO_PREMULTIPLY_ALPHA) also got premultiplied data.

Now we unpremultiply when color management or the surface needs it, and
otherwise tell the surface pipe not to premultiply. The spec doesn't say what
color values larger than alpha mean; we clamp them to alpha so the result
doesn't depend on whether color management runs.

Original Revision: https://phabricator.services.mozilla.com/D331951

Comment 9

[Timothy Nikkel (:tnikkel)]

Attachment: Bug 2079663. Add WPT reftests for JXL images with premultiplied alpha.

Premultiplied and non-premultiplied versions of the same RGB and gray grids,
each compared against one PNG, plus the conformance suite's
alpha_premultiplied image compared against its reference PNG. cjxl
--premultiply=1 only marks the pixels as premultiplied, so the premultiplied
grids are encoded from PNGs that were premultiplied first.

Original Revision: https://phabricator.services.mozilla.com/D331952

Comment 10

[Phabricator Automation]

firefox-beta Uplift Approval Request

• User impact if declined/Reason for urgency: incorrect rendering of premultiplied alpha jpeg xl files
• Code covered by automated testing?: yes
• Fix verified in Nightly?: no
• Needs manual QE testing?: no
• Steps to reproduce for manual QE testing:
• Risk associated with taking this patch: low
• Explanation of risk level: The change only affects JPEG XL images with premultiplied (associated) alpha, which are uncommon. For all other JPEG XL images the decoding path is unchanged. I've added good test coverage for premult files with new WPT reftests cover the RGB and grayscale cases and the JPEG XL conformance image.
• String changes made/needed?: none
• Is Android affected?: yes

Comment 11

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot

Comment 12

[Pulsebot]

https://github.com/mozilla-firefox/firefox/commit/40d84da44f2c
https://hg.mozilla.org/releases/mozilla-beta/rev/cf4f85a1a7a1
https://github.com/mozilla-firefox/firefox/commit/c95e2831662e
https://hg.mozilla.org/releases/mozilla-beta/rev/07fa6d1b5d8c
https://github.com/mozilla-firefox/firefox/commit/6f96cc37eb97
https://hg.mozilla.org/releases/mozilla-beta/rev/57a5fffc326a

Comment 13

[Timothy Nikkel (:tnikkel)]

Attachment: Bug 2079663. Rename nsJXLDecoder::mU8RowBuf to mPipeInputRowBuf. a=RyanVM

The name says what type it holds rather than what it is for. The next patch
adds a second row buffer, so name this one after its role: the row we hand to
the surface pipe when it can't read mPixelBuffer directly.

Original Revision: https://phabricator.services.mozilla.com/D331947

Comment 14

[Timothy Nikkel (:tnikkel)]

Attachment: Bug 2079663. Don't premultiply JXL images that have premultiplied alpha a second time. a=RyanVM

https://searchfox.org/firefox-main/rev/3c71a541b0e7ac6e086907b38670dabca9174c45/image/decoders/nsJXLDecoder.cpp#607

We assume jxl-rs always outputs unpremultiplied alpha, but for images with
premultiplied (associated) alpha it outputs the premultiplied values stored in
the file. We then premultiply them again, so semi-transparent areas come out
too dark. Color management and surfaces that want unpremultiplied data
(NO_PREMULTIPLY_ALPHA) also got premultiplied data.

Now we unpremultiply when color management or the surface needs it, and
otherwise tell the surface pipe not to premultiply. The spec doesn't say what
color values larger than alpha mean; we clamp them to alpha so the result
doesn't depend on whether color management runs.

Original Revision: https://phabricator.services.mozilla.com/D331951

Comment 15

[Timothy Nikkel (:tnikkel)]

Attachment: Bug 2079663. Add WPT reftests for JXL images with premultiplied alpha. a=RyanVM

Premultiplied and non-premultiplied versions of the same RGB and gray grids,
each compared against one PNG, plus the conformance suite's
alpha_premultiplied image compared against its reference PNG. cjxl
--premultiply=1 only marks the pixels as premultiplied, so the premultiplied
grids are encoded from PNGs that were premultiplied first.

Original Revision: https://phabricator.services.mozilla.com/D331952

Comment 16

[Phabricator Automation]

firefox-release Uplift Approval Request

• User impact if declined/Reason for urgency: incorrect rendering of premultiplied alpha jpeg xl files
• Code covered by automated testing?: yes
• Fix verified in Nightly?: no
• Needs manual QE testing?: no
• Steps to reproduce for manual QE testing:
• Risk associated with taking this patch: low
• Explanation of risk level: The change only affects JPEG XL images with premultiplied (associated) alpha, which are uncommon. For all other JPEG XL images the decoding path is unchanged. I've added good test coverage for premult files with new WPT reftests cover the RGB and grayscale cases and the JPEG XL conformance image.
• String changes made/needed?: none
• Is Android affected?: yes

Comment 17

[Pulsebot]

https://github.com/mozilla-firefox/firefox/commit/e88b5bd78fef
https://hg.mozilla.org/releases/mozilla-release/rev/52b029896993
https://github.com/mozilla-firefox/firefox/commit/b59523d4eb6f
https://hg.mozilla.org/releases/mozilla-release/rev/630877ab99b5
https://github.com/mozilla-firefox/firefox/commit/20fb550ae50e
https://hg.mozilla.org/releases/mozilla-release/rev/aa3a74f0e7cf