User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/139.0.0.0 Safari/537.36

Expected results:

This issue tracks the work to replace the current pref disabled C++ JXL decoder with a Rust based JXL decoder.

Creating this issue here after discussing with tnikkel@mozilla.com.

is there a meta bug to follow the work on this? thanks
(or this is the main bug ?)

I guess it depends on what you are interested in tracking. This is the bug to land the changeover from a C++ jpegxl library to a rust jpegxl library. We have bug 1539075 that tracks jpeg-xl overall that is still existing from our initial c++ backed implementation (I'll add this to that bug now). If there is something specific you want to track I can file a new bug as needed.

this is good, thanks
it was to add the release notes item

Release Note Request (optional, but appreciated)
[Why is this notable]: Rust rewrite and tech press loves writing about it
[Affects Firefox for Android]: I guess ?
[Suggested wording]: The jpeg-xl library has been ported to Rust code for better security
(and performance?)

[Links (documentation, blog post, etc)]:

All of this code is only compiled on nightly, and is pref disabled by default there. So I'm not sure a release note is appropriate at this point.

Comment #8 is true, but also there are people who are watching closely, so maybe we can declare this is a special case.

And there's a question worth asking - do we still want to build it only on Nightly or are we okay to build it for stable/beta too? (With the pref disabled, of course)

You can keep "Differential Revision: (URL)" comment in the commit message to prevent recreating the patch.

(In reply to Kagami Rosylight [:saschanaz] (they/them) from comment #13)

You can keep "Differential Revision: (URL)" comment in the commit message to prevent recreating the patch.

Hm, yes, but when I reordered them it remembered the parentage and everything got very upside down. I removed the later 2 patches and uploaded them anew. I hope that works.

moz-phab reorg is the way to fix that!

(In reply to Timothy Nikkel (:tnikkel) from comment #8)

All of this code is only compiled on nightly, and is pref disabled by default there. So I'm not sure a release note is appropriate at this point.

It doesn't really matter. The goal is to make sure release managers are aware when it is ready and enabled.
(they will know when to add it :)

(In reply to Kagami Rosylight [:saschanaz] (they/them) from comment #16)

moz-phab reorg is the way to fix that!

Ah, I think I tried that, but maybe in the wrong way?

(In reply to Martin Bruse from comment #15)

(In reply to Kagami Rosylight [:saschanaz] (they/them) from comment #13)

You can keep "Differential Revision: (URL)" comment in the commit message to prevent recreating the patch.

Hm, yes, but when I reordered them it remembered the parentage and everything got very upside down. I removed the later 2 patches and uploaded them anew. I hope that works.

You can also always change the order of revisions on the phabricator web ui. We always try not to create new revisions as it loses all of the review state.

(In reply to Kagami Rosylight [:saschanaz] (they/them) from comment #10)

And there's a question worth asking - do we still want to build it only on Nightly or are we okay to build it for stable/beta too? (With the pref disabled, of course)

I think we should take things one step at a time: keep it on nightly for now, we can make that change at some point after this.

(In reply to Timothy Nikkel (:tnikkel) from comment #19)

(In reply to Martin Bruse from comment #15)

(In reply to Kagami Rosylight [:saschanaz] (they/them) from comment #13)

You can keep "Differential Revision: (URL)" comment in the commit message to prevent recreating the patch.

Hm, yes, but when I reordered them it remembered the parentage and everything got very upside down. I removed the later 2 patches and uploaded them anew. I hope that works.

You can also always change the order of revisions on the phabricator web ui. We always try not to create new revisions as it loses all of the review state.

Thank you, I'll avoid doing it in the future!

Should we also prepare a new intent to prototype?

For posterity and reference, I'd just like to document here that as of right now, the jxl-rs decoder that we're integrating is not of production performance, It currently decodes ~4M pixels/s on a mediocre laptop, which we expect to improve a lot in the future.

Comment on attachment 9513722 [details]
Bug 1986393 - Added basic CMYK support to the SurfacePipeFactory. r=tnikkel

Revision D265077 was moved to bug 1989218. Setting attachment 9513722 [details] to obsolete.

Hi, I'd like to report that the current Rust-based JPEG XL decoder in Firefox still lacks support for two parts of the JXL spec:

Alpha transparency — images with an alpha channel render incorrectly or lose transparency.

Animation — animated JXL files display only the first frame.

Both features work correctly in the reference decoder (libjxl), so this appears to be a missing implementation detail in the Rust decoder rather than an issue with the files themselves.

Just leaving this note so the team is aware. Thanks for all the work on JPEG XL support!

Firefox Profiler link if needed: https://share.firefox.dev/48pgw6e

Thanks for the report! I'll have to check with alpha, but for animation we'd like to skip that part for initial code for simplicity (although I think the current patch has that part).

And yes Martin, this is a request to skip that part (and potentially anything that's not supported in the current Firefox) as I strongly think we should land a minimal feature set and grow from there.

(In reply to Kagami Rosylight [:saschanaz] (they/them) from comment #29)

Thanks for the report! I'll have to check with alpha, but for animation we'd like to skip that part for initial code for simplicity (although I think the current patch has that part).

And yes Martin, this is a request to skip that part (and potentially anything that's not supported in the current Firefox) as I strongly think we should land a minimal feature set and grow from there.

Both alpha and animation is a tiny tiny part of the patch, so I don't feel it makes sense to skip it, TBH.

I'm rebasing the patch now, and upgrading it to jxl-rs 0.1.4, so I'll soon see what the bug report is about :D

(In reply to teguhiman488 from comment #28)

Hi, I'd like to report that the current Rust-based JPEG XL decoder in Firefox still lacks support for two parts of the JXL spec:

If you are testing on Firefox Nightly then you are testing the old libjxl based decoder (which was not fully implemented). The only way to test jxl-rs in Firefox is to make your own build.

Both alpha and animation is a tiny tiny part of the patch, so I don't feel it makes sense to skip it, TBH.

alpha is fine, animation is what I'm requesting to skip or at least split. We don't believe we'll accept everything the decoder supports at day one.

Adds ICC color profile support for RGB/grayscale and CMYK images via QCMS.
For CMYK images, QCMS transforms to RGB_8 (3 bytes per pixel), which the
SwizzleFilter then expands to BGRA using UnpackRowRGB24_To_.

Key implementation details:

• JXL uses inverted CMYK convention (0=max ink, 1=no ink)
• QCMS expects standard ICC convention (0=no ink, 255=max ink)
• Rust decoder inverts values and packs as [C,M,Y,K] in memory order

here to subscribe, thanks for the work in bringin jxl-rs in, just fixed animations in the libjxl ff implementation here: https://phabricator.services.mozilla.com/D277649

maybe the patches help otherwise i am happy todo a follow up and do it with the jxl-rs implementation once landed, i think animation should be onboard.

For testing we should eventually add these tests (we don't need to do it right away because jxl is prefed off by default):

Integration test of downscaling jxl file
https://searchfox.org/firefox-main/rev/69888f58354c8d44e854945761a9e8fa954512d1/image/test/reftest/downscaling/reftest.list#195

Could use a few more reftests testing a few more cases, grayscale, alpha, no alpha, a few other basic features/options, probably as wpt reftests
https://searchfox.org/firefox-main/source/image/test/reftest/jxl

Basic color management of jxl file reftest. reftests are run outputting to srgb, so a jxl file with a color space tag that is not srgb and a reference png
https://searchfox.org/firefox-main/source/image/test/reftest/color-management/reftest.list

Basic animation gtest
https://searchfox.org/firefox-main/rev/69888f58354c8d44e854945761a9e8fa954512d1/image/test/gtest/TestFrameAnimator.cpp#128

Add an infinite and finite length animated jxl testcase to
https://searchfox.org/firefox-main/source/image/test/mochitest/test_discardAnimatedImage.html

Add GreenFirstFrameAnimatedJXLTestCase and use it everywhere GreenFirstFrameAnimated*TestCase is used
https://searchfox.org/firefox-main/rev/f5171bd6a83982663d6bb33e78fa89237a42a8ac/image/test/gtest/Common.cpp#755

Add BlendAnimatedJXLTestCase like BlendAnimatedAVIFTestCase
https://searchfox.org/firefox-main/rev/69888f58354c8d44e854945761a9e8fa954512d1/image/test/gtest/Common.cpp#778

Corrupt jxl file
https://searchfox.org/firefox-main/rev/69888f58354c8d44e854945761a9e8fa954512d1/image/test/gtest/Common.cpp#819

Need to run jxl reftests an Android (and eventually release and beta)
https://searchfox.org/firefox-main/rev/69888f58354c8d44e854945761a9e8fa954512d1/image/test/reftest/reftest.list#35

Test animation operators
https://searchfox.org/firefox-main/rev/69888f58354c8d44e854945761a9e8fa954512d1/image/test/mochitest/test_animation_operators.html#37

RunDecodeToSurface for non-animated jxl
https://searchfox.org/firefox-main/rev/f5171bd6a83982663d6bb33e78fa89237a42a8ac/image/test/gtest/TestDecodeToSurface.cpp#118

RunDecodeToSurface for animated jxl
https://searchfox.org/firefox-main/rev/f5171bd6a83982663d6bb33e78fa89237a42a8ac/image/test/gtest/TestDecodeToSurface.cpp#120

Need a test that transparent jxl files draw correctly (currently they do not and nothing is failing), this could be gtest or reftest or wpt reftest

Add a testcase that gets to this line with an animated jxl to test the frame count decodes work for animated images
https://searchfox.org/firefox-main/rev/33fd6bd39c625067a29f153adce6a4646e45750f/image/test/gtest/TestMetadata.cpp#90

Bug 2015015 - Add jpeg xl conformance tests to WPT

chromium gtests
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/image-decoders/jxl/jxl_image_decoder_test.cc

re-enable jxl reftests on beta/release
https://searchfox.org/firefox-main/source/image/test/reftest/jxl/reftest.list

enable jxl mochitests on beta/release
https://searchfox.org/firefox-main/source/image/test/mochitest/mochitest.toml

This should probably be listed at https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Experimental_features.

Sebastian

(In reply to Sebastian Zartner [:sebo] from comment #42)

This should probably be listed at https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Experimental_features.

Sebastian

It's already there 👀 (although always has been strictly nightly only)

https://hg.mozilla.org/mozilla-central/rev/45aab1808108
https://hg.mozilla.org/mozilla-central/rev/2160418b9a4f

Reopened for unpublished changesets.

Let's keep this resolved and move the further changesets to new bugs. Otherwise tracking this will get complicated.

Is a release note needed for the nightly channel?? Thanks

So far all thats changed is that we replaced the C++ library we used for processing jpeg xl files with a rust library. Jpeg xl is still disabled via a pref by default. So does that warrant a nightly channel release note? Mostly it's a behind the scenes change so far, the new rust library hasn't been optimized as much as the C++ one yet, so it is slower to decode jpeg xl files now.

Thanks!

Comment on attachment 9534144 [details]
Bug 1986393 - Add CMS support to JXL decoder. r=tnikkel,saschanaz

Revision D277146 was moved to bug 1709818. Setting attachment 9534144 [details] to obsolete.

Comment on attachment 9534141 [details]
Bug 1986393 - Add animation support to JXL decoder. r=tnikkel,saschanaz

Revision D277143 was moved to bug 1709818. Setting attachment 9534141 [details] to obsolete.

(In reply to Kagami Rosylight [:saschanaz] (they/them) from comment #44)

(In reply to Sebastian Zartner [:sebo] from comment #42)

This should probably be listed at https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Experimental_features.

Sebastian

It's already there 👀 (although always has been strictly nightly only)

Right. I should have checked better. Sorry for that!

Sebastian