Colours in jpegxl images are displayed wrong #2033

I first noticed this on my one website where a red thing in an image was displayed as blue.

As example the image on: https://jpegxl.info/

Palemoon:

Firefox:

Someone on twitter thinks it is because of a mixup between BGR and RGB

image.png

205 KiB

image.png

196 KiB

I assumed it was supposed to be yellow, not teal.

@athenian200 I thought we had already solved this?

The discussion on this issue was a while back, and this issue only appears on certain images I believe?

I think either this problem was forgotten about when we started discussing compiler issues, or else we decided to implement it first and fix this problem later.

So much was going on with that issue that this particular part of it seems to have gotten lost in the shuffle. Probably better that we use this issue to track what's going on with it from here on out.

#1928

This was the last status of this as mentioned, and it was only briefly early on, in a single post on Job's PR (I created the PR for him because his computer couldn't handle it).

Right. seems though that quite a few of the jxls currently out there have the channels swapped. Just didn't catch it because i assumed that was what all the jpegxl.info images were supposed to look like ;P

https://old.lucaversari.it/jpeg_xl_data/bees.jxl

Clearly that's not right ;)

I'm guessing we have a B8G8R8A8 instead of an R8G8B8A8 (or v.v.) somewhere... :P

It indeed seems to be an B8G8R8A8 versus R8G8B8A8 issue.
Up to now, the libjxl API does not provide output in arbitrary channel order (although that is something we plan to add).

The first attempt of changing SurfaceFormat::B8G8R8A8 to SurfaceFormat::R8G8B8A8 didn't work for a reason that I don't ye understand; in that case the images where still displayed with the wrong colors, but one gets
Crash Annotation GraphicsCriticalError: |[0][GFX1]: Unknown image format 2 (t=6.67418) [GFX1]: Unknown image format 2
as output when looking at an jxl image.

To confirm that images look correct when b and r are swapped, we can manually fix the order:

diff --git a/image/decoders/nsJXLDecoder.cpp b/image/decoders/nsJXLDecoder.cpp
index 07a7849e28..c8f0c14630 100644
--- a/image/decoders/nsJXLDecoder.cpp
+++ b/image/decoders/nsJXLDecoder.cpp
@@ -141,6 +141,11 @@ nsJXLDecoder::ReadJXLData(const char* aData, size_t aLength)
             Nothing(), SurfacePipeFlags());
         for (uint8_t* rowPtr = mOutBuffer.begin(); rowPtr < mOutBuffer.end();
              rowPtr += mInfo.xsize * 4) {
+          for (uint8_t* pixPtr = rowPtr; pixPtr < rowPtr + mInfo.xsize * 4; pixPtr+=4){
+            uint8_t temp = pixPtr[0];
+            pixPtr[0] = pixPtr[2];
+            pixPtr[2] = temp;
+          }
           pipe->WriteBuffer(reinterpret_cast<uint32_t*>(rowPtr));
         }

We can certainly do a quick and dirty conversion on the buffer for the time being but it'd be a fairly hot path so if that becomes permanent then we'll need a more performant one.

I can prepare a pull request with the hack and a comment saying that this needs fixing. One more step towards making jxl-decoding in Pale Moon better would be to it it in a streaming fashion using a callback like it is done in chrome.

The first attempt of changing SurfaceFormat::B8G8R8A8 to SurfaceFormat::R8G8B8A8 didn't work for a reason that I don't ye understand

Indeed, it didn't work for me either when I was working on the PR. I didn't really know how the decoder worked exactly in detail, I only copied the code from Mozilla's implementation at bug 1707590 and made some few changes to make it build. I thought the inverted color issue was either solved or just deferred for fixing in another PR, but looks like I should've been more vocal about the issue (even if it meant delaying the JPEG-XL support to 31.5.0) I faced 5 months ago... Sorry.

I did some research and we use gfxPackedPixel to expand RGB to BGRA specifically in the image decoders. So we don't have direct support for RGBA AFAICT.
Plugging that into search landed me on this BZ bug which we may need to port to have better plumbing. Seems mostly mechanical to port. Unfortunately I don't have time in the coming month or so to go into that as I have family matters to attend to (which involves international travel) and this probably needs ample time to do right.
Transparency might rely on this as well, although we may also just look more closely at how our APNG is done instead of falling into the refactoring rabbit hole and getting stuck there.
All of that is kinda offtopic here though, aside from us just needing to use the workaround for the time being until it can be handled properly otherwise with native RGBA surfaces.

We probably will need to backport bug 1255106 as well for QCMS in SurfaceFilter.

I've split off the different things we'd want to port in other bugs like CMS. In good Mozilla fashion it's a bit of a potpourri though.