Implement JPEG XL image decoder using libjxl

• Component: Images
• Status: RESOLVED FIXED

[Yoshiaki Jitsukawa]

As an experimental feature, I'd like to implement this for wincairo (and maybe for PlayStation?)

[Yoshiaki Jitsukawa]

Created attachment 444319 [details]
Patch for preliminary review.

[Michael Catanzaro]

Comment on attachment 444319 [details]
Patch for preliminary review.

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

I see Chrome and Firefox both have this supported, but guarded behind an experimental feature flag. So we're the last major engine to add JXL to our source tree, and I suppose that answers the question of whether we should accept it or not.

As far as enabling it, WinCairo is a pretty low-risk port to start with. I think I'd hesitate to enable in WPE/GTK until Firefox or Chrome goes first, since they are better at evaluating such things than we are.

Nice to see it built as a system dependency and not bundled. Also nice to see it has a modern API.

> Source/WebCore/platform/image-decoders/jpegxl/JPEGXLImageDecoder.cpp:40
> +    clear();

This clear() isn't needed, but it doesn't hurt either, good hygiene I suppose, so OK.

> Source/WebCore/platform/image-decoders/jpegxl/JPEGXLImageDecoder.cpp:46
> +ScalableImageDecoderFrame* JPEGXLImageDecoder::frameBufferAtIndex(size_t index)
> +{
> +    if (index)
> +        return nullptr;

Hm, this doesn't seem good? Are you missing a FIXME here? It's only implemented for index=0?

> Source/WebCore/platform/image-decoders/jpegxl/JPEGXLImageDecoder.cpp:69
> +void JPEGXLImageDecoder::decode(bool onlySize, bool allDataReceived)

This function is only called from one place, so you don't need either parameter. onlySize is always false, and the allDataReceived parameter can be replaced with a call to isAllDataReceived() within this function.

> Source/WebCore/platform/image-decoders/jpegxl/JPEGXLImageDecoder.cpp:125
> +        if (status == JXL_DEC_ERROR) {
> +            return status;
> +        }
> +        if (status == JXL_DEC_SUCCESS) {
> +            return status;
> +        }
> +        if (status == JXL_DEC_NEED_MORE_INPUT) {
> +            return status;
> +        }

I would probably write it all in one conditional. Regardless of what you prefer, be sure to placate the style checker EWS. It's not going to like braces around single-line conditionals.

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

What needs fixed?

[Yoshiaki Jitsukawa]

(In reply to Michael Catanzaro from comment #2)> 
> > Source/WebCore/platform/image-decoders/jpegxl/JPEGXLImageDecoder.cpp:46
> > +ScalableImageDecoderFrame* JPEGXLImageDecoder::frameBufferAtIndex(size_t index)
> > +{
> > +    if (index)
> > +        return nullptr;
> 
> Hm, this doesn't seem good? Are you missing a FIXME here? It's only
> implemented for index=0?

Ah, yes, I should have added a TODO beccause talking with Don I started implementation without animations.


> > Source/WebCore/platform/image-decoders/jpegxl/JPEGXLImageDecoder.cpp:69
> > +void JPEGXLImageDecoder::decode(bool onlySize, bool allDataReceived)
> 
> This function is only called from one place, so you don't need either
> parameter. onlySize is always false, and the allDataReceived parameter can
> be replaced with a call to isAllDataReceived() within this function.

This function is also called within tryDecodeSize() (defined in the header)so both seems to be needed.

> > Source/WebCore/platform/image-decoders/jpegxl/JPEGXLImageDecoder.cpp:125
> > +        if (status == JXL_DEC_ERROR) {
> > +            return status;
> > +        }
> > +        if (status == JXL_DEC_SUCCESS) {
> > +            return status;
> > +        }
> > +        if (status == JXL_DEC_NEED_MORE_INPUT) {
> > +            return status;
> > +        }
> 
> I would probably write it all in one conditional. Regardless of what you
> prefer, be sure to placate the style checker EWS. It's not going to like
> braces around single-line conditionals.

I see. I'll fix that.
 
> > Source/WebCore/platform/image-decoders/jpegxl/JPEGXLImageDecoder.cpp:180
> > +        // FIXME:
> 
> What needs fixed?

I wonder if there's a RGBA/BGRA conversion utility. I think I saw something long time ago...

[Yoshiaki Jitsukawa]

Created attachment 444592 [details]
Patch

[Yoshiaki Jitsukawa]

Created attachment 444594 [details]
Patch (with --binary)

 - Tests added.

 - Check JPEG XL signature by using JxlSignatureCheck().

> > Source/WebCore/platform/image-decoders/jpegxl/JPEGXLImageDecoder.cpp:46
> > +ScalableImageDecoderFrame* JPEGXLImageDecoder::frameBufferAtIndex(size_t index)
> > +{
> > +    if (index)
> > +        return nullptr;
> 
> Hm, this doesn't seem good? Are you missing a FIXME here? It's only
> implemented for index=0?

 - A TODO comment Added.

> > Source/WebCore/platform/image-decoders/jpegxl/JPEGXLImageDecoder.cpp:125
> > +        if (status == JXL_DEC_ERROR) {
> > +            return status;
> > +        }
> > +        if (status == JXL_DEC_SUCCESS) {
> > +            return status;
> > +        }
> > +        if (status == JXL_DEC_NEED_MORE_INPUT) {
> > +            return status;
> > +        }
> 
> I would probably write it all in one conditional. Regardless of what you
> prefer, be sure to placate the style checker EWS. It's not going to like
> braces around single-line conditionals.

 - Merged into one conditional.
 
> > Source/WebCore/platform/image-decoders/jpegxl/JPEGXLImageDecoder.cpp:180
> > +        // FIXME:
> 
> What needs fixed?

 - Set pixel with setPixel() function.

[Michael Catanzaro]

Would be good to try to get at least one more reviewer to look this over before landing, but it LGTM.

[Yoshiaki Jitsukawa]

(In reply to Michael Catanzaro from comment #6)
> Would be good to try to get at least one more reviewer to look this over
> before landing, but it LGTM.

Thank you for the review! Will try.

[Adrian Perez]

Patch LGTM for a first pass at this, thanks for working on it!

Probably we want to add an “USE_JPEGXL“ CMake option at some point,
instead of building the support if libjxl is found, to make the build
process more deterministic. That would be the same as we currently do
with other optional dependencies like USE_WOFF2 or USE_OPENJPEG. It
would be nice to try to enable the code for other ports as well; my
expectation is that it won't be much more than adding the relevant
CMake bits to the Options${PORT}.cmake files — but I think we could
land this one as-is and continue improving things in follo-up patches =)

[Michael Catanzaro]

What Adrian suggests would certainly be required for WPE/GTK. WinCairo does things automagically, though. I understand this is less of a problem there because the dependencies are more tightly controlled.

> It would be nice to try to enable the code for other ports as well; my expectation is that it won't be much more than adding the relevant CMake bits to the Options${PORT}.cmake files

Could be easily done in this patch, or in a follow-up patch. Either way seems fine.

[EWS]

Committed r286011 (244404@main): <https://commits.webkit.org/244404@main>

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

[Radar WebKit Bug Importer]

<rdar://problem/85556384>

[Said Abou-Hallawa]

Comment on attachment 444594 [details]
Patch (with --binary)

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

> ChangeLog:11
> +        Chrome, FireFox, and Edge.

The other browsers added the support to JPEG XL as an experimental feature. I checked Chrome and it looks like they have the sources of libjxl built in their build system. So I am not sure why WebKit GTK port took a different approach by having the image format enabled by default. More importantly, neither the sources nor the binaries is included in your build system. You just search the environment and if they are found you enable USE_JPEGXL. For Apple ports, either the system has to support the image format or we have to sources built in our build system (like what other browsers do).

I would also urge you to send an email to webkit-dev about these kinds of changes. I think it is important to keep WebKit has the same kind of behavior as much as we can on all platforms.

[Don Olmstead]

(In reply to Said Abou-Hallawa from comment #12)
> Comment on attachment 444594 [details]
> Patch (with --binary)
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=444594&action=review
> 
> > ChangeLog:11
> > +        Chrome, FireFox, and Edge.
> 
> The other browsers added the support to JPEG XL as an experimental feature.
> I checked Chrome and it looks like they have the sources of libjxl built in
> their build system. So I am not sure why WebKit GTK port took a different
> approach by having the image format enabled by default. More importantly,
> neither the sources nor the binaries is included in your build system. You
> just search the environment and if they are found you enable USE_JPEGXL. For
> Apple ports, either the system has to support the image format or we have to
> sources built in our build system (like what other browsers do).
> 
> I would also urge you to send an email to webkit-dev about these kinds of
> changes. I think it is important to keep WebKit has the same kind of
> behavior as much as we can on all platforms.

Apologies as I didn't assume this change would be at all contentious. There was a discussion on the mailing list from the libjxl developers asking if WebKit wanted to integrate with their library but the conversation stopped when they learned that support would most likely come from a system level library on Apple platforms. Igalia also added an AVIF image decoder on their own.

Our interest is only for WinCairo and PlayStation where we have very specific requirements libraries that we build and provide so we didn't do any guarding and assume if its there and detected then we want to enable it. I'd assume Igalia would add an actual option for it like they do other image decoders.

[Adrian Perez]

(In reply to Don Olmstead from comment #13)
> (In reply to Said Abou-Hallawa from comment #12)
> > Comment on attachment 444594 [details]
> > Patch (with --binary)
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=444594&action=review
> > 
> > > ChangeLog:11
> > > +        Chrome, FireFox, and Edge.
> > 
> > The other browsers added the support to JPEG XL as an experimental feature.
> > I checked Chrome and it looks like they have the sources of libjxl built in
> > their build system. So I am not sure why WebKit GTK port took a different
> > approach by having the image format enabled by default. More importantly,
> > neither the sources nor the binaries is included in your build system. You
> > just search the environment and if they are found you enable USE_JPEGXL. For
> > Apple ports, either the system has to support the image format or we have to
> > sources built in our build system (like what other browsers do).
> > 
> > I would also urge you to send an email to webkit-dev about these kinds of
> > changes. I think it is important to keep WebKit has the same kind of
> > behavior as much as we can on all platforms.
> 
> Apologies as I didn't assume this change would be at all contentious. There
> was a discussion on the mailing list from the libjxl developers asking if
> WebKit wanted to integrate with their library but the conversation stopped
> when they learned that support would most likely come from a system level
> library on Apple platforms. Igalia also added an AVIF image decoder on their
> own.
> 
> Our interest is only for WinCairo and PlayStation where we have very
> specific requirements libraries that we build and provide so we didn't do
> any guarding and assume if its there and detected then we want to enable it.
> I'd assume Igalia would add an actual option for it like they do other image
> decoders.

We do not jxl support enabled (yet) at all in the GTK and WPE ports. If/when
we do, we will *not* bundle a copy of libjxl and provide a build-time option
(as Don suspected). Initially the default value for the build option for us
would be ${ENABLE_EXPERIMENTAL_FEATURES}: disabled in release builds (as
done from source tarballs) and enabled for testing in developer builds.

A few Linux distributions (for example Fedora or Arch) are already shipping
libjxl and packages that depend on it—same for libavif—which is the reason
why we don't need vendoring a copy under Source/ThirdParty/.

I was also under the impression that this would be non-controversial, sorry!

[Michael Catanzaro]

> Our interest is only for WinCairo and PlayStation where we have very specific requirements libraries that we build and provide so we didn't do any guarding and assume if its there and detected then we want to enable it. I'd assume Igalia would add an actual option for it like they do other image decoders.

Right, the way WinCairo handles dependencies is an anti-pattern on Linux. Detecting whether the system dependency exists and enabling if found is an "automagic" dependency and while it's very common on Linux, it has serious problems, most notably that it frequently leads to features being disabled by mistake. I noticed that during review, but I didn't say anything because all the surrounding dependencies in OptionsWinCairo.cmake were handled the same way. Adrian noticed it as well (comment #8). I guess it doesn't matter nearly as much for WinCairo, but we would certainly require a more explicit choice to enable or disable the library when building for Linux. We would pick a default, either enabled or disabled, and require that you explicitly pass a flag to choose one way or another.

(In reply to Said Abou-Hallawa from comment #12)
> I would also urge you to send an email to webkit-dev about these kinds of
> changes. I think it is important to keep WebKit has the same kind of
> behavior as much as we can on all platforms.

FWIW we discussed this about on webkit-dev here:

https://lists.webkit.org/pipermail/webkit-dev/2021-May/031844.html

and nobody really objected. I was skeptical about a new image format, but the fact that Firefox and Chrome both implemented it as an experimental feature seems reason enough for WebKit to do the same. At least it seems clearly more popular than JPEG 2000.

It is currently enabled by default for WinCairo. No strong opinion on that. Other ports currently have no way to enable it.

>  More importantly, neither the sources nor the binaries is included in your build system. You just search the environment and if they are found you enable USE_JPEGXL. For Apple ports, either the system has to support the image format or we have to sources built in our build system (like what other browsers do).

WebKitGTK does not bundle libraries except for two specific problematic dependencies: ANGLE and xdgmime. These dependencies are problems to be fixed, not success stories. The other bundled stuff under Source/ThirdParty is either for tests only (gtest), or else is not distributed in our tarballs. So libjxl is not needed under Source/ThirdParty unless Apple needs it there. Even then, we would not distribute that in our releases.

[Michael Catanzaro]

> We would pick a default, either enabled or disabled, and require that you explicitly pass a flag to choose one way or another.

Sorry, I meant: we would require that you explicitly pass a flag to choose to deviate from the default we pick.

[Michael Catanzaro]

(In reply to Adrian Perez from comment #14)
> We do not jxl support enabled (yet) at all in the GTK and WPE ports. If/when
> we do, we will *not* bundle a copy of libjxl and provide a build-time option
> (as Don suspected). Initially the default value for the build option for us
> would be ${ENABLE_EXPERIMENTAL_FEATURES}: disabled in release builds (as
> done from source tarballs) and enabled for testing in developer builds.

I just found you proposed bug #233325 to do exactly that. Anyway, I agree it's the right approach. Will be very easy to walk back in the future if browsers wind up deciding not to ship this after all.

[Yoshiaki Jitsukawa]

Sorry for not discussing this on the mailing list.I'll do so next time for changes like this.

(In reply to Michael Catanzaro from comment #17)
> (In reply to Adrian Perez from comment #14)
> > We do not jxl support enabled (yet) at all in the GTK and WPE ports. If/when
> > we do, we will *not* bundle a copy of libjxl and provide a build-time option
> > (as Don suspected). Initially the default value for the build option for us
> > would be ${ENABLE_EXPERIMENTAL_FEATURES}: disabled in release builds (as
> > done from source tarballs) and enabled for testing in developer builds.
> 
> I just found you proposed bug #233325 to do exactly that. Anyway, I agree
> it's the right approach. Will be very easy to walk back in the future if
> browsers wind up deciding not to ship this after all.

I also prefer this approach. Let's revisit this matter. > Don