Bug 233113

Created attachment 444319 [details] Patch for preliminary review.

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?

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

Created attachment 444592 [details] Patch

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.

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

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

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 =)

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.

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

<rdar://problem/85556384>

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.

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

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

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

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

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

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