Bug 233364 – JPEG XL decoder should support understand color profiles

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Michael Catanzaro]

The JPEG and PNG decoders use PlatformDisplay::sharedDisplay().colorProfile() to pass a color profile to the image library. This function always returns an sRGB color profile except when running under X11, so that is not good, but it's a step towards having color management work properly for these images. The JPEG XL decoder ought to as well.

[Jon Sneyers]

Likely this is something you'll want to do for _all_ image formats, e.g. also for WebP, AVIF and JPEG 2000.

[Michael Catanzaro]

OK, reported bug #233370 as a catch-all bug for getting an understanding of our color management issues.

[Radar WebKit Bug Importer]

<rdar://problem/85767076>

[Yoshiaki Jitsukawa]

Created attachment 445628 [details]
Test contents based on https://github.com/libjxl/conformance

With this test contents we can see there're several color differences from refernce png images. Especially "bench_oriented_BRG" shows completelty differnt color.
(bench_oriented_BRG is for Container, VarDCT mode, JPEG reconstruction, Orientation, ICC profile)

[Yoshiaki Jitsukawa]

Let me take this as I'm experienced in handling HDR images and ICC profile.

[Yoshiaki Jitsukawa]

Created attachment 446956 [details]
Patch

[Michael Catanzaro]

Comment on attachment 446956 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=446956&action=review

Nice!

I'm not giving r+ because I think something is wrong with the test. The expected result looks weird, and it's passing on Apple platforms even though those don't support jxl and you didn't skip it. Looks like the test only passes if jxl is not supported at all?

> Source/WebCore/platform/image-decoders/jpegxl/JPEGXLImageDecoder.cpp:40
> +static constexpr int eventsWanted()

Alternatively you could do:

#if USE(LCMS)
static constexpr int s_eventsWanted = ...;
#else
static constexpr int s_eventsWanted = ...;
#endif

to avoid the function call. Of course whichever you prefer is fine.

> Source/WebCore/platform/image-decoders/jpegxl/JPEGXLImageDecoder.cpp:49
> +
> +

Only one blank line here.

> Source/WebCore/platform/image-decoders/jpegxl/JPEGXLImageDecoder.h:108
> +    cmsHTRANSFORM m_iccTransform { nullptr };

I'd be more comfortable with this if you put it into a std::unique_ptr. I see there's no way for it to leak as the code is currently written, but it'd be more robust.

[Yoshiaki Jitsukawa]

Created attachment 447113 [details]
Patch

[Yoshiaki Jitsukawa]

(In reply to Michael Catanzaro from comment #7)

Thank you for the review!

> I'm not giving r+ because I think something is wrong with the test. The
> expected result looks weird, and it's passing on Apple platforms even though
> those don't support jxl and you didn't skip it. Looks like the test only
> passes if jxl is not supported at all?

I marked the added test as [Skip] in the root TestExpectations.
 
> > Source/WebCore/platform/image-decoders/jpegxl/JPEGXLImageDecoder.cpp:40
> > +static constexpr int eventsWanted()
> 
> Alternatively you could do:
> 
> #if USE(LCMS)
> static constexpr int s_eventsWanted = ...;
> #else
> static constexpr int s_eventsWanted = ...;
> #endif
> 
> to avoid the function call. Of course whichever you prefer is fine.

I changed the code to avoid the function call ;)
 
> > Source/WebCore/platform/image-decoders/jpegxl/JPEGXLImageDecoder.cpp:49
> > +
> > +
> 
> Only one blank line here.

Fixed.

> > Source/WebCore/platform/image-decoders/jpegxl/JPEGXLImageDecoder.h:108
> > +    cmsHTRANSFORM m_iccTransform { nullptr };
> 
> I'd be more comfortable with this if you put it into a std::unique_ptr. I
> see there's no way for it to leak as the code is currently written, but it'd
> be more robust.

There're similar use in the PNG and JPEG decoders, so I'd like to keep it in this patch for now and afterwards introduce unique_ptrs for LCMS in a wider area.

[Michael Catanzaro]

(In reply to Yoshiaki Jitsukawa from comment #9)
> I marked the added test as [Skip] in the root TestExpectations.

I see, but the expected result still assumes jxl is not supported, which isn't a useful test. What result do you get when running the test locally with WinCairo?

[Yoshiaki Jitsukawa]

Created attachment 447182 [details]
Patch

[Yoshiaki Jitsukawa]

Created attachment 447183 [details]
test capture on wincairo.

[Yoshiaki Jitsukawa]

(In reply to Michael Catanzaro from comment #10)
> (In reply to Yoshiaki Jitsukawa from comment #9)
> > I marked the added test as [Skip] in the root TestExpectations.
> 
> I see, but the expected result still assumes jxl is not supported, which
> isn't a useful test. What result do you get when running the test locally
> with WinCairo?

I updated the test to compare with the original jpeg image as a reference to make it platform-independent (in terms of platform color profile).

I've attached a captuer image to show how the test html looks like on wincairo.

[EWS]

Committed r287081 (?): <https://commits.webkit.org/r287081>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 447182 [details].