Bug 233364

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

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

<rdar://problem/85767076>

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)

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

Created attachment 446956 [details] Patch

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.

Created attachment 447113 [details] Patch

(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.

(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?

Created attachment 447182 [details] Patch

Created attachment 447183 [details] test capture on wincairo.

(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.

Committed r287081 (?): <https://commits.webkit.org/r287081> All reviewed patches have been landed. Closing bug and clearing flags on attachment 447182 [details].