Reland "Add JXL image decoder using jxl-rs" [chromium/src : main]
Helmut Januschka (Gerrit)
Helmut Januschka has uploaded the change for review![]( "Open in Gerrit")
Helmut Januschka would like Chromium LUCI CQ, Wan-Teh Chang and David Bokan to review this change.
Commit message
Reland "Add JXL image decoder using jxl-rs"
This is a reland of commit 3badff27281339878293e935a5e0fbb41da553bf
Original change's description:
> Add JXL image decoder using jxl-rs
>
> Adds JXLImageDecoder that uses the jxl-rs Decoder. Includes tests and
> fuzzer.
>
> Bug: 462919304
> Change-Id: I90047f35f214eb2d8eeca3054a5ac601a5d9738f
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7319379
> Reviewed-by: David Bokan <bo...@chromium.org>
> Commit-Queue: Helmut Januschka <hel...@januschka.com>
> Reviewed-by: Wan-Teh Chang <w...@google.com>
> Cr-Commit-Position: refs/heads/main@{#1566591}
Change diff
Change information
- M third_party/blink/renderer/platform/BUILD.gn
- M third_party/blink/renderer/platform/blink_platform_unittests_bundle_data.filelist
- M third_party/blink/renderer/platform/image-decoders/BUILD.gn
- M third_party/blink/renderer/platform/image-decoders/image_decoder_fuzzer_utils.cc
- M third_party/blink/renderer/platform/image-decoders/image_decoder_fuzzer_utils.h
- A third_party/blink/renderer/platform/image-decoders/jxl/OWNERS
- A third_party/blink/renderer/platform/image-decoders/jxl/jxl_image_decoder.cc
- A third_party/blink/renderer/platform/image-decoders/jxl/jxl_image_decoder.h
- A third_party/blink/renderer/platform/image-decoders/jxl/jxl_image_decoder_fuzzer.cc
- A third_party/blink/renderer/platform/image-decoders/jxl/jxl_image_decoder_test.cc
- A third_party/blink/web_tests/external/wpt/jpegxl/resources/5_frames_numbered.jxl
- A third_party/blink/web_tests/images/resources/newtons_cradle.jxl
- A third_party/blink/web_tests/images/resources/zoltan.jxl
- A third_party/rust/jxl/v0_2/wrapper/BUILD.gn
- A third_party/rust/jxl/v0_2/wrapper/lib.rs
Related details
- David Bokan
- Wan-Teh Chang
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Bokan (Gerrit)
David Bokan voted Code-Review+1![]( "Open in Gerrit")
- Helmut Januschka
- Wan-Teh Chang
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Helmut Januschka (Gerrit)
Helmut Januschka added 1 comment![]( "Open in Gerrit")
if let Some(icc) = profile.try_as_icc() {fixes the fuzzer
Related details
- Wan-Teh Chang
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Wan-Teh Chang (Gerrit)
Wan-Teh Chang added 1 comment![]( "Open in Gerrit")
if let Some(icc) = profile.try_as_icc() {fixes the fuzzer
Helmut: Is patchset 1 the same as the reverted code?
Luca, Moritz: Could you review the changes to third_party/rust/jxl/v0_2/wrapper/lib.rs?
Related details
- Helmut Januschka
- Luca Versari
- Moritz Firsching
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Moritz Firsching (Gerrit)
Moritz Firsching added 1 comment![]( "Open in Gerrit")
Related details
- Helmut Januschka
- Luca Versari
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Wan-Teh Chang (Gerrit)
Wan-Teh Chang voted and added 3 comments![]( "Open in Gerrit")
Votes added by Wan-Teh Chang
| Code-Review | +1 |
3 comments
../../web_tests/external/wpt/jpegxl/resources/5_frames_numbered.jxlJust curious: Why do we need to list these six files here?
This CL only adds the sixth file (5_frames_numbered.jxl). The first five files already exist, right?
../../web_tests/external/wpt/jpegxl/resources/*.jxlJust curious: Does this line go with the six new lines in blink_platform_unittests_bundle_data.filelist?
Some(d) if d.is_finite() && d >= 0.0 => d as u32,I am not familiar with Rust. It seems that the code here will hide errors. Should we report an error if `d` is infinite or negative?
Related details
- Helmut Januschka
- Luca Versari
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Wan-Teh Chang (Gerrit)
Wan-Teh Chang added 1 comment![]( "Open in Gerrit")
if let Some(icc) = profile.try_as_icc() {Wan-Teh Changfixes the fuzzer
Wan-Teh ChangHelmut: Is patchset 1 the same as the reverted code?
Luca, Moritz: Could you review the changes to third_party/rust/jxl/v0_2/wrapper/lib.rs?
Helmut: Is patchset 1 the same as the reverted code?
I am answering my question. I verified patchset 1 is the same as the reverted code.
Helmut Januschka (Gerrit)
Helmut Januschka added 4 comments![]( "Open in Gerrit")
feedback addressed! hope it sticks this time, sorry for the inconvenience and thanks for all your support!
../../web_tests/external/wpt/jpegxl/resources/5_frames_numbered.jxlJust curious: Why do we need to list these six files here?
This CL only adds the sixth file (5_frames_numbered.jxl). The first five files already exist, right?
yes they exist, but somehow missed, it was mentioned here by @bat...@chromium.org https://issues.chromium.org/issues/462919304#comment16
Just curious: Does this line go with the six new lines in blink_platform_unittests_bundle_data.filelist?
same, from the CL that was mentioned to do with the reland
I am not familiar with Rust. It seems that the code here will hide errors. Should we report an error if `d` is infinite or negative?
The current behavior defaults invalid durations (infinite, negative, or None) to 0ms, which is a valid duration meaning "display as fast as possible." This follows the principle of graceful degradation. if we error/reject decoding it might be stricter but may reject otherwise viewable files.
Related details
- Luca Versari
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
Exportable changes to web-platform-tests were detected in this CL and a pull request in the upstream repo has been made: https://github.com/web-platform-tests/wpt/pull/57115.
When this CL lands, the bot will automatically merge the PR on GitHub if the required GitHub checks pass; otherwise, ecosystem-infra@ team will triage the failures and may contact you.
WPT Export docs:
https://chromium.googlesource.com/chromium/src/+/main/docs/testing/web_platform_tests.md#Automatic-export-process
Helmut Januschka (Gerrit)
Helmut Januschka voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Wan-Teh Chang (Gerrit)
Wan-Teh Chang voted and added 1 comment![]( "Open in Gerrit")
Votes added by Wan-Teh Chang
| Code-Review | +1 |
1 comment
Some(d) if d.is_finite() && d >= 0.0 => d as u32,Helmut JanuschkaI am not familiar with Rust. It seems that the code here will hide errors. Should we report an error if `d` is infinite or negative?
The current behavior defaults invalid durations (infinite, negative, or None) to 0ms, which is a valid duration meaning "display as fast as possible." This follows the principle of graceful degradation. if we error/reject decoding it might be stricter but may reject otherwise viewable files.
In this particular case, the degradation may be so graceful that it can't be detected by viewing. So an encoder-side bug may be hard to discover. As long as you have considered this and made an informed decision, it is fine.
Related details
- Helmut Januschka
- Luca Versari
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Reland "Add JXL image decoder using jxl-rs"
This is a reland of commit 3badff27281339878293e935a5e0fbb41da553bf
Original change's description:
> Add JXL image decoder using jxl-rs
>
> Adds JXLImageDecoder that uses the jxl-rs Decoder. Includes tests and
> fuzzer.
>
> Bug: 462919304, 474417800
> Change-Id: I90047f35f214eb2d8eeca3054a5ac601a5d9738f
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7319379
> Reviewed-by: David Bokan <bo...@chromium.org>
> Commit-Queue: Helmut Januschka <hel...@januschka.com>
> Reviewed-by: Wan-Teh Chang <w...@google.com>
> Cr-Commit-Position: refs/heads/main@{#1566591}
- M third_party/blink/renderer/platform/BUILD.gn
- M third_party/blink/renderer/platform/blink_platform_unittests_bundle_data.filelist
- M third_party/blink/renderer/platform/blink_platform_unittests_bundle_data.globlist
- M third_party/blink/renderer/platform/image-decoders/BUILD.gn
- M third_party/blink/renderer/platform/image-decoders/image_decoder_fuzzer_utils.cc
- M third_party/blink/renderer/platform/image-decoders/image_decoder_fuzzer_utils.h
- A third_party/blink/renderer/platform/image-decoders/jxl/OWNERS
- A third_party/blink/renderer/platform/image-decoders/jxl/jxl_image_decoder.cc
- A third_party/blink/renderer/platform/image-decoders/jxl/jxl_image_decoder.h
- A third_party/blink/renderer/platform/image-decoders/jxl/jxl_image_decoder_fuzzer.cc
- A third_party/blink/renderer/platform/image-decoders/jxl/jxl_image_decoder_test.cc
- A third_party/blink/web_tests/external/wpt/jpegxl/resources/5_frames_numbered.jxl
- A third_party/blink/web_tests/images/resources/newtons_cradle.jxl
- A third_party/blink/web_tests/images/resources/zoltan.jxl
- A third_party/rust/jxl/v0_2/wrapper/BUILD.gn
- A third_party/rust/jxl/v0_2/wrapper/lib.rs
Code-Review: +1 by Wan-Teh Chang, +1 by David Bokan
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
The WPT PR for this CL has been merged upstream! https://github.com/web-platform-tests/wpt/pull/57115
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Wan-Teh Chang (Gerrit)
Wan-Teh Chang added 1 comment![]( "Open in Gerrit")
../../web_tests/external/wpt/jpegxl/resources/5_frames_numbered.jxlHelmut: 5_frames_numbered.jxl is only used by jxl_image_decoder_test.cc. Are you planning to use 5_frames_numbered.jxl in a Web Platform Test (WPT)?
If not, please move 5_frames_numbered.jxl to the third_party/blink/web_tests/images/resources/ directory.
Related details
- Helmut Januschka
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Helmut Januschka (Gerrit)
Helmut Januschka added 1 comment![]( "Open in Gerrit")
../../web_tests/external/wpt/jpegxl/resources/5_frames_numbered.jxlHelmut: 5_frames_numbered.jxl is only used by jxl_image_decoder_test.cc. Are you planning to use 5_frames_numbered.jxl in a Web Platform Test (WPT)?
If not, please move 5_frames_numbered.jxl to the third_party/blink/web_tests/images/resources/ directory.
sorry, that was a left over ðŸ˜, should have catched that, initially i thought, and aimed for a WPT to validate that animations work, to help other engines to move on with animations, sadly i haven't found a way to do a proper WPT. in the last CL of JXL integration https://chromium-review.googlesource.com/c/chromium/src/+/7184969 - i just pushed PS79 that moves the file! hope that works, let me know if it is important enough todo a standalone CL - and again sorry for the miss!.
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Wan-Teh Chang (Gerrit)
Wan-Teh Chang added 1 comment![]( "Open in Gerrit")
../../web_tests/external/wpt/jpegxl/resources/5_frames_numbered.jxlHelmut JanuschkaHelmut: 5_frames_numbered.jxl is only used by jxl_image_decoder_test.cc. Are you planning to use 5_frames_numbered.jxl in a Web Platform Test (WPT)?
If not, please move 5_frames_numbered.jxl to the third_party/blink/web_tests/images/resources/ directory.
sorry, that was a left over ðŸ˜, should have catched that, initially i thought, and aimed for a WPT to validate that animations work, to help other engines to move on with animations, sadly i haven't found a way to do a proper WPT. in the last CL of JXL integration https://chromium-review.googlesource.com/c/chromium/src/+/7184969 - i just pushed PS79 that moves the file! hope that works, let me know if it is important enough todo a standalone CL - and again sorry for the miss!.
It would be better to write a separate CL to correct this error. Reasons include:
- The ["Wire up JXL decoder."](https://chromium-review.googlesource.com/c/chromium/src/+/7184969/74) CL is quite large, so it would be hard to see it also fixes this error.
- If the "Wire up JXL decoder." CL were reverted, the fix would also be reverted.
Helmut Januschka (Gerrit)
Helmut Januschka added 1 comment![]( "Open in Gerrit")
../../web_tests/external/wpt/jpegxl/resources/5_frames_numbered.jxlHelmut JanuschkaHelmut: 5_frames_numbered.jxl is only used by jxl_image_decoder_test.cc. Are you planning to use 5_frames_numbered.jxl in a Web Platform Test (WPT)?
If not, please move 5_frames_numbered.jxl to the third_party/blink/web_tests/images/resources/ directory.
Wan-Teh Changsorry, that was a left over ðŸ˜, should have catched that, initially i thought, and aimed for a WPT to validate that animations work, to help other engines to move on with animations, sadly i haven't found a way to do a proper WPT. in the last CL of JXL integration https://chromium-review.googlesource.com/c/chromium/src/+/7184969 - i just pushed PS79 that moves the file! hope that works, let me know if it is important enough todo a standalone CL - and again sorry for the miss!.
It would be better to write a separate CL to correct this error. Reasons include:
- The ["Wire up JXL decoder."](https://chromium-review.googlesource.com/c/chromium/src/+/7184969/74) CL is quite large, so it would be hard to see it also fixes this error.
- If the "Wire up JXL decoder." CL were reverted, the fix would also be reverted.
@w...@google.com thank you, totally makes sense, here is the standalone CL: https://chromium-review.googlesource.com/c/chromium/src/+/7456869