Bug 233364 - JPEG XL decoder should support understand color profiles
Summary: JPEG XL decoder should support understand color profiles
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: PC Linux
: P2 Normal
Assignee: Yoshiaki Jitsukawa
URL:
Keywords: InRadar
Depends on:
Blocks: 208235 233370
  Show dependency treegraph
 
Reported: 2021-11-19 06:17 PST by Michael Catanzaro
Modified: 2021-12-20 00:29 PST (History)
6 users (show)

See Also:


Attachments
Test contents based on https://github.com/libjxl/conformance (42.90 MB, application/x-zip-compressed)
2021-12-01 16:03 PST, Yoshiaki Jitsukawa
no flags Details
Patch (166.79 KB, patch)
2021-12-12 17:46 PST, Yoshiaki Jitsukawa
no flags Details | Formatted Diff | Diff
Patch (168.96 KB, patch)
2021-12-14 00:35 PST, Yoshiaki Jitsukawa
no flags Details | Formatted Diff | Diff
Patch (193.37 KB, patch)
2021-12-14 17:23 PST, Yoshiaki Jitsukawa
no flags Details | Formatted Diff | Diff
test capture on wincairo. (177.79 KB, image/jpeg)
2021-12-14 17:35 PST, Yoshiaki Jitsukawa
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Catanzaro 2021-11-19 06:17:00 PST
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.
Comment 1 Jon Sneyers 2021-11-19 08:16:27 PST
Likely this is something you'll want to do for _all_ image formats, e.g. also for WebP, AVIF and JPEG 2000.
Comment 2 Michael Catanzaro 2021-11-19 08:56:33 PST
OK, reported bug #233370 as a catch-all bug for getting an understanding of our color management issues.
Comment 3 Radar WebKit Bug Importer 2021-11-26 06:17:22 PST
<rdar://problem/85767076>
Comment 4 Yoshiaki Jitsukawa 2021-12-01 16:03:54 PST
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)
Comment 5 Yoshiaki Jitsukawa 2021-12-10 15:44:46 PST
Let me take this as I'm experienced in handling HDR images and ICC profile.
Comment 6 Yoshiaki Jitsukawa 2021-12-12 17:46:49 PST
Created attachment 446956 [details]
Patch
Comment 7 Michael Catanzaro 2021-12-13 07:24:59 PST
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.
Comment 8 Yoshiaki Jitsukawa 2021-12-14 00:35:22 PST
Created attachment 447113 [details]
Patch
Comment 9 Yoshiaki Jitsukawa 2021-12-14 01:03:58 PST
(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.
Comment 10 Michael Catanzaro 2021-12-14 06:14:02 PST
(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?
Comment 11 Yoshiaki Jitsukawa 2021-12-14 17:23:35 PST
Created attachment 447182 [details]
Patch
Comment 12 Yoshiaki Jitsukawa 2021-12-14 17:35:08 PST
Created attachment 447183 [details]
test capture on wincairo.
Comment 13 Yoshiaki Jitsukawa 2021-12-14 17:37:09 PST
(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.
Comment 14 EWS 2021-12-15 10:05:38 PST
Committed r287081 (?): <https://commits.webkit.org/r287081>

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