Integrate jxl-rs JPEG XL decoder into Blink [chromium/src : main]
Philip Jägenstedt (Gerrit)
Philip Jägenstedt added 3 comments![]( "Open in Gerrit")
kJxl = 417,In order for the data to show up at https://webstatus.dev/features/jpegxl this will need to be based on the web-features ID, like this:
```suggestion
kJpegxl = 417,
```
use_counter->CountUse(WebFeature::kJXLImage);Note that you could call `use_counter->CountWebDXFeature()` here and get rid of WebFeature::kJXLImage, but it's totally fine either way.
base::FeatureList::IsEnabled(features::kJXLImageFormat)) {Is there another feature check needed for `<picture><source type="image/jxl">`? I assume that won't reach this code path because at the time we decide which image to load we're not yet creating a decoder instance.
Related details
- Helmut Januschka
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. |
Helmut Januschka (Gerrit)
Helmut Januschka added 16 comments![]( "Open in Gerrit")
thanks, rebased, conflicts solved and feedback has been addressed!
return ScopedImageType::kOther;Helmut JanuschkaThis should be kJxl
Done
#if BUILDFLAG(ENABLE_JXL_DECODER)Helmut JanuschkaNot necessary. The array will contain it based on the build flag.
true, but other comments changed that `return true` to `base::FeatureList::IsEnabled(features::kJXLImageFormat);` to be able to killswitch it. let me know if that works for you.
TEST(MimeUtilTest, LookupTypes) {Helmut JanuschkaLikely should be parameterized (assuming that this isn't enabled by default in this CL).
done
In order for the data to show up at https://webstatus.dev/features/jpegxl this will need to be based on the web-features ID, like this:
```suggestion
kJpegxl = 417,
```
thanks, also for the link, nice to know that!
Note that you could call `use_counter->CountWebDXFeature()` here and get rid of WebFeature::kJXLImage, but it's totally fine either way.
Done
LOG(INFO) << "Creating JXL decoder for mime_type: " << mime_type;Helmut JanuschkaRemove
Done
base::FeatureList::IsEnabled(features::kJXLImageFormat)) {Is there another feature check needed for `<picture><source type="image/jxl">`? I assume that won't reach this code path because at the time we decide which image to load we're not yet creating a decoder instance.
thanks, moved it to IsSupportedImageMimeType, makes more sense there, but still with killswitch able flag.
LOG(INFO) << "JXL decoder created successfully";Helmut JanuschkaSame
Done
const uint8_t* contents =Helmut JanuschkaGetConsecutiveData(0, sizeof(buffer), buffer) returns a span. We should examine only that range, not sizeof(buffer) on line 78.
Marked as resolved.
// Return the number of frames we've discoveredHelmut Januschkacomments should terminate with period, here and other places
Done
pixel_buffer_.resize(pixel_size);Helmut JanuschkaLikely should worry about the sizing here. You are truncating a size_t to a wtf_size_t. Probably want to check overflow.
Does this need to resize it always? Like can we skip this if the buffer is already of required size?
thanks, using `base::CheckedNumeric` now, about the resize, there is a if around now
size_t src_idx = (y * basic_info_.width + x) * 4;Helmut JanuschkaThis looks like it could use some skia macros for creating these colors.
Done
if (index >= frame_info_.size()) {Helmut JanuschkaIf you have the item in the frame_buffer_cache_ you likely can just get the timestamp directly no?
thanks!
Related details
- Dave Tapuska
- Philip Jägenstedt
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dale Curtis (Gerrit)
Dale Curtis added 2 comments![]( "Open in Gerrit")
I defer to image/ and Skia OWNERs, but FWIW our other rust based codec is implemented in Skia so that it can reuse SkiaImageDecoderBase:
- https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/image-decoders/png/png_image_decoder.h;l=15
- https://source.chromium.org/chromium/chromium/src/+/main:third_party/skia/src/codec/SkPngRustCodec.h
There's even already a JXL one here:
That maybe should be replaced with the rust version?
@luk...@chromium.org who did the Rust PNG decoder and may have opinions.
# Enable JXL decoder in Blink for JPEG XL image decoding.Since none of this code is in media, this should be defined elsewhere. Maybe in blink build flags somewhere?
Related details
- Dave Tapuska
- Helmut Januschka
- Philip Jägenstedt
- Łukasz Anforowicz
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. |
Helmut Januschka (Gerrit)
Helmut Januschka added 1 comment![]( "Open in Gerrit")
I defer to image/ and Skia OWNERs, but FWIW our other rust based codec is implemented in Skia so that it can reuse SkiaImageDecoderBase:
- https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/image-decoders/png/png_image_decoder.h;l=15
- https://source.chromium.org/chromium/chromium/src/+/main:third_party/skia/src/codec/SkPngRustCodec.h
There's even already a JXL one here:
That maybe should be replaced with the rust version?
@luk...@chromium.org who did the Rust PNG decoder and may have opinions.
@dalec...@chromium.org i see png rust uses a small wrapper in chromium around the skia decoder. is that the route we should take?
so - i'd need to do a CL in skia first?
Related details
- Dale Curtis
- Dave Tapuska
- Philip Jägenstedt
- Łukasz Anforowicz
Dale Curtis (Gerrit)
Dale Curtis added 1 comment![]( "Open in Gerrit")
Helmut JanuschkaI defer to image/ and Skia OWNERs, but FWIW our other rust based codec is implemented in Skia so that it can reuse SkiaImageDecoderBase:
- https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/image-decoders/png/png_image_decoder.h;l=15
- https://source.chromium.org/chromium/chromium/src/+/main:third_party/skia/src/codec/SkPngRustCodec.h
There's even already a JXL one here:
That maybe should be replaced with the rust version?
@luk...@chromium.org who did the Rust PNG decoder and may have opinions.
@dalec...@chromium.org i see png rust uses a small wrapper in chromium around the skia decoder. is that the route we should take?
so - i'd need to do a CL in skia first?
Possibly, but I defer to Skia and folks like Lukas who are more familiar with this area.
Related details
- Dave Tapuska
- Helmut Januschka
- Philip Jägenstedt
- Łukasz Anforowicz
Łukasz Anforowicz (Gerrit)
Łukasz Anforowicz added 1 comment![]( "Open in Gerrit")
Helmut JanuschkaI defer to image/ and Skia OWNERs, but FWIW our other rust based codec is implemented in Skia so that it can reuse SkiaImageDecoderBase:
- https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/image-decoders/png/png_image_decoder.h;l=15
- https://source.chromium.org/chromium/chromium/src/+/main:third_party/skia/src/codec/SkPngRustCodec.h
There's even already a JXL one here:
That maybe should be replaced with the rust version?
@luk...@chromium.org who did the Rust PNG decoder and may have opinions.
Dale Curtis@dalec...@chromium.org i see png rust uses a small wrapper in chromium around the skia decoder. is that the route we should take?
so - i'd need to do a CL in skia first?
Possibly, but I defer to Skia and folks like Lukas who are more familiar with this area.
This CL (i.e. direct Blink => `jxl-rs` integration) LGTM (speaking as one of `//third_party/rust/OWNERS`). I think this is the simplest path forward for now (and it doesn't necessarily prevent having an `jxl-rs`-based `SkCodec` in the future). That said, I ultimately defer to `//third_party/blink/renderer/platform/image-decoders/OWNERS` here.
Related details
- Dave Tapuska
- Helmut Januschka
- Philip Jägenstedt
Philip Jägenstedt (Gerrit)
Philip Jägenstedt added 1 comment![]( "Open in Gerrit")
# Enable JXL decoder in Blink for JPEG XL image decoding.Since none of this code is in media, this should be defined elsewhere. Maybe in blink build flags somewhere?
Maybe others will have input, but I think in third_party/blink/renderer/platform/BUILD.gn looks like the best candidate.
Related details
- Dave Tapuska
- Helmut Januschka
Dave Tapuska (Gerrit)
Dave Tapuska added 1 comment![]( "Open in Gerrit")
# Enable JXL decoder in Blink for JPEG XL image decoding.Philip JägenstedtSince none of this code is in media, this should be defined elsewhere. Maybe in blink build flags somewhere?
Maybe others will have input, but I think in third_party/blink/renderer/platform/BUILD.gn looks like the best candidate.
- Helmut Januschka
Helmut Januschka (Gerrit)
Helmut Januschka added 2 comments![]( "Open in Gerrit")
Helmut JanuschkaI defer to image/ and Skia OWNERs, but FWIW our other rust based codec is implemented in Skia so that it can reuse SkiaImageDecoderBase:
- https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/image-decoders/png/png_image_decoder.h;l=15
- https://source.chromium.org/chromium/chromium/src/+/main:third_party/skia/src/codec/SkPngRustCodec.h
There's even already a JXL one here:
That maybe should be replaced with the rust version?
@luk...@chromium.org who did the Rust PNG decoder and may have opinions.
Dale Curtis@dalec...@chromium.org i see png rust uses a small wrapper in chromium around the skia decoder. is that the route we should take?
so - i'd need to do a CL in skia first?
Łukasz AnforowiczPossibly, but I defer to Skia and folks like Lukas who are more familiar with this area.
This CL (i.e. direct Blink => `jxl-rs` integration) LGTM (speaking as one of `//third_party/rust/OWNERS`). I think this is the simplest path forward for now (and it doesn't necessarily prevent having an `jxl-rs`-based `SkCodec` in the future). That said, I ultimately defer to `//third_party/blink/renderer/platform/image-decoders/OWNERS` here.
thanks, i would also like to go ahead with blink approach. lets see what image-decoders/owners suggest!
# Enable JXL decoder in Blink for JPEG XL image decoding.Philip JägenstedtSince none of this code is in media, this should be defined elsewhere. Maybe in blink build flags somewhere?
Dave TapuskaMaybe others will have input, but I think in third_party/blink/renderer/platform/BUILD.gn looks like the best candidate.
Let's put it in https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/config.gni
- Dale Curtis
- Dave Tapuska
- Philip Jägenstedt
- Łukasz Anforowicz
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. |
Helmut Januschka (Gerrit)
Helmut Januschka added 1 comment![]( "Open in Gerrit")
hello reviewers,
thanks in advance for your time! please let me know if you want me to address anything.
cheers,
helmut
Related details
- Dave Tapuska
- Hans Wennborg
- Luca Versari
- Philip Jägenstedt
- Vladimir Levin
- mmenke
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. |
Luca Versari (Gerrit)
Luca Versari added 10 comments![]( "Open in Gerrit")
BASE_FEATURE(kJXLImageFormat, base::FEATURE_ENABLED_BY_DEFAULT);I imagine this shouldn't be enabled by default yet.
// Convert float32 to float16 (IEEE 754 binary16).We should add a TODO to let jxl-rs handle this once it is supported.
// jxl-rs doesn't support incremental input, so we must reset and startI don't believe that is true.
need_reset = true;I don't think this is necessary. If we only need the size, we can pause decoding as soon as we get the image's metadata, and resume decoding actual frames from there.
}There probably should be a need_rewind flag instead of need_reset - for now they can both call reset(), but rewind() should eventually be cheaper.
if (need_new_decoder) {this probably should just be `decoder_.has_value()` since nothing else can set the value.
// Feed data only when we have a new/reset decoder or new data.As I mentioned above, I don't think this is necessary.
num_frame_events_in_scan_++;Do we *need* to figure out frame count in advance?
if (decode_to_half_float_) {I don't think ~any of this logic should be here, instead it should go in jxl-rs itself and we should write directly into Chrome's framebuffers from there using the correct data types.
I am not sure why this file exists, instead of using directly JxlDecoderInner (which is intended for usage from FFI situations).
Related details
- Dave Tapuska
- Hans Wennborg
- Helmut Januschka
- Philip Jägenstedt
- Vladimir Levin
- mmenke
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. |
Hans Wennborg (Gerrit)
Hans Wennborg added 2 comments![]( "Open in Gerrit")
hello reviewers,
thanks in advance for your time! please let me know if you want me to address anything.
cheers,
helmut
Any particular areas you want the different reviewers to focus on? I just looked at BUILD.gn for now.
"//third_party/rust/jxl/v0_1:lib", # Prevents build pruning for CL #1Can you expand on the "CL #1" comment?
Actually you probably don't really need to add this to "all_rust". I think that was mostly used during the Rust toolchain bringup. Today I believe we have a lot more Rust dependencies than what's listed here.
Related details
- Dave Tapuska
mmenke (Gerrit)
mmenke added 1 comment![]( "Open in Gerrit")
{"image/avif", "avif"},
{"image/jxl", "jxl"},The CL description implies jxl is already supported via libjxl. Why are we adding these as part of swapping jxl decoding library (and adding an accept header)?
I don't oppose this change, I just want to better understand why this wasn't needed before, the previous state of jxl support, and why this CL is so large.
Related details
- Dave Tapuska
- Helmut Januschka
- Philip Jägenstedt
- Vladimir Levin
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. |
mmenke (Gerrit)
mmenke added 1 comment![]( "Open in Gerrit")
{"image/avif", "avif"},
{"image/jxl", "jxl"},The CL description implies jxl is already supported via libjxl. Why are we adding these as part of swapping jxl decoding library (and adding an accept header)?
I don't oppose this change, I just want to better understand why this wasn't needed before, the previous state of jxl support, and why this CL is so large.
Oh, and a response would best be added to the CL description, rather than as an answer to this comment, so it's more discoverable by folks digging through commit history.
Helmut Januschka (Gerrit)
Helmut Januschka added 13 comments![]( "Open in Gerrit")
Helmut JanuschkaI defer to image/ and Skia OWNERs, but FWIW our other rust based codec is implemented in Skia so that it can reuse SkiaImageDecoderBase:
- https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/image-decoders/png/png_image_decoder.h;l=15
- https://source.chromium.org/chromium/chromium/src/+/main:third_party/skia/src/codec/SkPngRustCodec.h
There's even already a JXL one here:
That maybe should be replaced with the rust version?
@luk...@chromium.org who did the Rust PNG decoder and may have opinions.
Dale Curtis@dalec...@chromium.org i see png rust uses a small wrapper in chromium around the skia decoder. is that the route we should take?
so - i'd need to do a CL in skia first?
Łukasz AnforowiczPossibly, but I defer to Skia and folks like Lukas who are more familiar with this area.
Helmut JanuschkaThis CL (i.e. direct Blink => `jxl-rs` integration) LGTM (speaking as one of `//third_party/rust/OWNERS`). I think this is the simplest path forward for now (and it doesn't necessarily prevent having an `jxl-rs`-based `SkCodec` in the future). That said, I ultimately defer to `//third_party/blink/renderer/platform/image-decoders/OWNERS` here.
thanks, i would also like to go ahead with blink approach. lets see what image-decoders/owners suggest!
Done
"//third_party/rust/jxl/v0_1:lib", # Prevents build pruning for CL #1Can you expand on the "CL #1" comment?
Actually you probably don't really need to add this to "all_rust". I think that was mostly used during the Rust toolchain bringup. Today I believe we have a lot more Rust dependencies than what's listed here.
removed it
{"image/avif", "avif"},
{"image/jxl", "jxl"},mmenkeThe CL description implies jxl is already supported via libjxl. Why are we adding these as part of swapping jxl decoding library (and adding an accept header)?
I don't oppose this change, I just want to better understand why this wasn't needed before, the previous state of jxl support, and why this CL is so large.
Oh, and a response would best be added to the CL description, rather than as an answer to this comment, so it's more discoverable by folks digging through commit history.
The CL is large because it re-adds the complete end-to-end integration that was previously removed:
- New Blink image decoder (now using jxl-rs)
- MIME type registration (was removed in M110)
- Accept header support for image/jxl (was removed in M110)
- Feature flag (kJXLImageFormat) for killswitch capability
- Use counters, devtools instrumentation, tests, and fuzzer
BASE_FEATURE(kJXLImageFormat, base::FEATURE_ENABLED_BY_DEFAULT);I imagine this shouldn't be enabled by default yet.
Done
We should add a TODO to let jxl-rs handle this once it is supported.
Done
// jxl-rs doesn't support incremental input, so we must reset and startI don't believe that is true.
Done
I don't think this is necessary. If we only need the size, we can pause decoding as soon as we get the image's metadata, and resume decoding actual frames from there.
JXL format doesn't store frame count in the header. Each frame has is_last flag - you must parse through frames to know the count.
There probably should be a need_rewind flag instead of need_reset - for now they can both call reset(), but rewind() should eventually be cheaper.
Done
this probably should just be `decoder_.has_value()` since nothing else can set the value.
Done
// Feed data only when we have a new/reset decoder or new data.As I mentioned above, I don't think this is necessary.
we need frame count upfront, we should do a framecount() in jxl-rs, atleast from my current understanding of blink and my trial and error
Do we *need* to figure out frame count in advance?
yes i need that for blink integration
I don't think ~any of this logic should be here, instead it should go in jxl-rs itself and we should write directly into Chrome's framebuffers from there using the correct data types.
same as below, will do PRs in jxl-rs
I am not sure why this file exists, instead of using directly JxlDecoderInner (which is intended for usage from FFI situations).
The wrapper exists primarily for the CXX bridge to enable Rust<->C++ interop with Blink. The typestate pattern in JxlDecoder<State> can't cross FFI boundaries, so this flattens it into a runtime state machine. (and follows the pattern of rust png decoder, that also does rust<->c++)
I agree with your point that much of the logic here—especially pixel format conversion—should ideally live in jxl-rs itself. i'll do some PR's there and then we can thin out the wrapper!
Related details
- Dale Curtis
- Dave Tapuska
- Hans Wennborg
- Luca Versari
- Philip Jägenstedt
- Vladimir Levin
- mmenke
- Łukasz Anforowicz
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. |
mmenke (Gerrit)
mmenke voted and added 3 comments![]( "Open in Gerrit")
Votes added by mmenke
| Code-Review | +1 |
3 comments
net/, content/browser/network/, third_party/blink/common/loader/, and third_party/blink/common/mime_util/ LGTM
{"image/avif", "avif"},
{"image/jxl", "jxl"},mmenkeThe CL description implies jxl is already supported via libjxl. Why are we adding these as part of swapping jxl decoding library (and adding an accept header)?
I don't oppose this change, I just want to better understand why this wasn't needed before, the previous state of jxl support, and why this CL is so large.
Helmut JanuschkaOh, and a response would best be added to the CL description, rather than as an answer to this comment, so it's more discoverable by folks digging through commit history.
The CL is large because it re-adds the complete end-to-end integration that was previously removed:
- New Blink image decoder (now using jxl-rs)
- MIME type registration (was removed in M110)
- Accept header support for image/jxl (was removed in M110)
- Feature flag (kJXLImageFormat) for killswitch capability
- Use counters, devtools instrumentation, tests, and fuzzer
Thanks so much for the explanation!
#if BUILDFLAG(ENABLE_JXL_DECODER)
"image/jxl",
#endifOut of caution, suggest not including this here for now, and adding a TODO referencing the linked bug to IsSupportedImageMimeType() to move add it here once it's enabled by default.
Alternatively, we could inline this as-is in IsSupportedImageMimeType(), to prevent anything else from using it without the extra jxl check that's in IsSupportedImageMimeType().
Related details
- Dave Tapuska
- Hans Wennborg
- Helmut Januschka
- Luca Versari
- Philip Jägenstedt
- Vladimir Levin
- Łukasz Anforowicz
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
mmenke (Gerrit)
mmenke added 1 comment![]( "Open in Gerrit")
#if BUILDFLAG(ENABLE_JXL_DECODER)
"image/jxl",
#endifOut of caution, suggest not including this here for now, and adding a TODO referencing the linked bug to IsSupportedImageMimeType() to move add it here once it's enabled by default.
Alternatively, we could inline this as-is in IsSupportedImageMimeType(), to prevent anything else from using it without the extra jxl check that's in IsSupportedImageMimeType().
Sorry, not once it's enabled by default, once the base::Feature is removed, and it's always enabled.
Łukasz Anforowicz (Gerrit)
Łukasz Anforowicz added 1 comment![]( "Open in Gerrit")
need_reset = true;Helmut JanuschkaI don't think this is necessary. If we only need the size, we can pause decoding as soon as we get the image's metadata, and resume decoding actual frames from there.
JXL format doesn't store frame count in the header. Each frame has is_last flag - you must parse through frames to know the count.
FWIW PNG decoder doesn't actually discover all the frame metadata upfront. For example, you can take a look at [`AnimatedPNGTests.ByteByByteMetaData`](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/image-decoders/png/png_image_decoder_test.cc;l=356;drc=d13bec8226f277ce88e32a23ca0741346e9cf053) to see how `image_decoder->FrameCount()` expectations change as the read offset moves through the input data.
Also note that the doc comment of the (`virtual`/per-codec) `blink::ImageDecoder::DecodeFrameCount` method [says](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/image-decoders/image_decoder.h;l=508-511;drc=d13bec8226f277ce88e32a23ca0741346e9cf053):
```
// This method may return an increasing frame count as frames are received and
// parsed. Alternatively, if the total frame count is available in the image
// header, this method may return the total frame count without checking how
// many frames are received.
```
Related details
- Dave Tapuska
- Hans Wennborg
- Helmut Januschka
- Luca Versari
- Philip Jägenstedt
- Vladimir Levin
Vladimir Levin (Gerrit)
Vladimir Levin added 1 comment![]( "Open in Gerrit")
This looks fine to me from the plumbing perspective, albeit a little strange to have both a build flag and a runtime flag. I'm not at all qualified to figure out the correctness of the decoder, so happy to stamp when everyone else is happy with the work.
(also +ccameron)
Related details
- Dave Tapuska
- Hans Wennborg
- Helmut Januschka
- Luca Versari
- Philip Jägenstedt
- ccameron chromium
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. |
Dave Tapuska (Gerrit)
Dave Tapuska added 7 comments![]( "Open in Gerrit")
#if BUILDFLAG(ENABLE_JXL_DECODER)
"image/jxl",
#endifmmenkeOut of caution, suggest not including this here for now, and adding a TODO referencing the linked bug to IsSupportedImageMimeType() to move add it here once it's enabled by default.
Alternatively, we could inline this as-is in IsSupportedImageMimeType(), to prevent anything else from using it without the extra jxl check that's in IsSupportedImageMimeType().
Sorry, not once it's enabled by default, once the base::Feature is removed, and it's always enabled.
+1
enable_jxl_decoder = use_blinkI feel like this should just be true. If we are in here including this gni file, I'd expect that use_blink is true.
"//media:media_buildflags",Is this left over from before?
#if BUILDFLAG(ENABLE_JXL_DECODER)conditional includes come after standard includes (with whitespace in between)
#include <memory>where is this used in this header?
Related details
Luca Versari (Gerrit)
Luca Versari added 5 comments![]( "Open in Gerrit")
constexpr uint64_t kMaxJxlFileSize = 0x10000000; // 256 MBFYI, rather than having a limit on the file size, it might be better to limit the total number of decoded pixels (which the API now supports).
Leaving a TODO to do this would also be OK with me.
if (IsAllDataReceived() && !all_frames_discovered_ && !Failed()) {What happens if we return something like
`all_frames_discovered_ ? num_decoded_frames_ : num_decoded_frames_ + 1`
?
(or possibly smarter logic, but in general avoiding to decode the entire file here)
// Feed data only when we have a new/reset decoder or new data.Helmut JanuschkaAs I mentioned above, I don't think this is necessary.
we need frame count upfront, we should do a framecount() in jxl-rs, atleast from my current understanding of blink and my trial and error
Done
// Copy pixels to frame buffer.
const uint32_t width = basic_info_.width;
const uint32_t height = basic_info_.height;
if (decode_to_half_float_) {
base::span<const uint8_t> buffer_bytes(pixel_buffer_);
for (uint32_t y = 0; y < height; ++y) {
for (uint32_t x = 0; x < width; ++x) {
size_t byte_offset = (y * width + x) * 8;
auto pixel_bytes = buffer_bytes.subspan(byte_offset, 8u);
uint16_t r =
base::U16FromNativeEndian(pixel_bytes.subspan<0, 2>());
uint16_t g =
base::U16FromNativeEndian(pixel_bytes.subspan<2, 2>());
uint16_t b =
base::U16FromNativeEndian(pixel_bytes.subspan<4, 2>());
uint16_t a =
base::U16FromNativeEndian(pixel_bytes.subspan<6, 2>());
(void)premultiply;
uint64_t* dst = frame.GetAddrF16(x, y);
*dst = (static_cast<uint64_t>(a) << 48) |
(static_cast<uint64_t>(b) << 32) |
(static_cast<uint64_t>(g) << 16) |
static_cast<uint64_t>(r);
}
}
} else {
base::span<const uint8_t> src_bytes(pixel_buffer_);
const size_t row_stride = width * 4;
if (premultiply) {
for (uint32_t y = 0; y < height; ++y) {
auto row = src_bytes.subspan(y * row_stride, row_stride);
for (uint32_t x = 0; x < width; ++x) {
auto pixel = row.subspan(x * 4, 4u);
uint8_t r = pixel[0];
uint8_t g = pixel[1];
uint8_t b = pixel[2];
uint8_t a = pixel[3];
r = (r * a + 128) >> 8;
g = (g * a + 128) >> 8;
b = (b * a + 128) >> 8;
ImageFrame::PixelData* dst = frame.GetAddr(x, y);
*dst = (a << SK_A32_SHIFT) | (r << SK_R32_SHIFT) |
(g << SK_G32_SHIFT) | (b << SK_B32_SHIFT);
}
}
} else {
for (uint32_t y = 0; y < height; ++y) {
auto row = src_bytes.subspan(y * row_stride, row_stride);
for (uint32_t x = 0; x < width; ++x) {
auto pixel = row.subspan(x * 4, 4u);
ImageFrame::PixelData* dst = frame.GetAddr(x, y);
*dst = (pixel[3] << SK_A32_SHIFT) | (pixel[0] << SK_R32_SHIFT) |
(pixel[1] << SK_G32_SHIFT) | (pixel[2] << SK_B32_SHIFT);
}
}
}
}I think we should decode directly into the frame buffer, instead of copying the data over after the fact.
Relatedly, I believe for progressive rendering to work properly we should also `frame.SetPixelsChanged(true);` even if `decode_frame` returns `JxlRsStatus::NeedMoreInput`.
Helmut JanuschkaI am not sure why this file exists, instead of using directly JxlDecoderInner (which is intended for usage from FFI situations).
The wrapper exists primarily for the CXX bridge to enable Rust<->C++ interop with Blink. The typestate pattern in JxlDecoder<State> can't cross FFI boundaries, so this flattens it into a runtime state machine. (and follows the pattern of rust png decoder, that also does rust<->c++)
I agree with your point that much of the logic here—especially pixel format conversion—should ideally live in jxl-rs itself. i'll do some PR's there and then we can thin out the wrapper!
- Hans Wennborg
- Helmut Januschka
- Philip Jägenstedt
- ccameron chromium
- mmenke
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. |
Łukasz Anforowicz (Gerrit)
Łukasz Anforowicz added 5 comments![]( "Open in Gerrit")
@veluca asked me to take a quick peek at the FFI / `#[cxx::bridge]` - I am sharing a few minor nits/questions below.
#[cxx::bridge]Please specify a namespace - e.g. `#[cxx::bridge(namespace = "blink")]` (or `#[cxx::bridge(namespace = "blink::jxl_rs")]`).
pub use ffi::*;Is this `pub` (and other `pub` below) necessary? This API won't be called from any other Rust crates, right? (i.e. this API will only be called over the `#[cxx::bridge]` / C++ API / FFI API - and this doesn't require `pub` IIUC)
Ok(ProcessingResult::Complete { .. }) => {I am focusing my review on cxx::bridge usage and I defer to @veluca for `jxl` API usage, but I noticed that `fn parse_basic_info` here only tries to access-and-store `self.decoder.embedded_color_profile()`, when `self.decoder.process(...)` returns `ProcessingResult::Complete`. With PNG the ICC data comes before IDAT/pixels, so even incomplete decoding can provide ICC info (and I imagine that the partially-decoded pixels should also be color-profile-corrected). So I wanted to double-check if this is ok?
self.error_message = format!("{}", e);Does Blink ever use the error message? Maybe `error_message: String` field can be replaced with `had_error: Result<(), ()>` or `has_failed: bool`?
Related details
- Hans Wennborg
- Helmut Januschka
- Philip Jägenstedt
- ccameron chromium
Helmut Januschka (Gerrit)
Helmut Januschka added 16 comments![]( "Open in Gerrit")
thanks to all for taking time to review - feedback has been addressed.
mmenkeOut of caution, suggest not including this here for now, and adding a TODO referencing the linked bug to IsSupportedImageMimeType() to move add it here once it's enabled by default.
Alternatively, we could inline this as-is in IsSupportedImageMimeType(), to prevent anything else from using it without the extra jxl check that's in IsSupportedImageMimeType().
Dave TapuskaSorry, not once it's enabled by default, once the base::Feature is removed, and it's always enabled.
+1
thanks, removed it from here.
I feel like this should just be true. If we are in here including this gni file, I'd expect that use_blink is true.
Done
Is this left over from before?
Done
#include "media/media_buildflags.h"Helmut Januschkaagain
Done
conditional includes come after standard includes (with whitespace in between)
Done
where is this used in this header?
thanks for the catch!
constexpr uint64_t kMaxJxlFileSize = 0x10000000; // 256 MBFYI, rather than having a limit on the file size, it might be better to limit the total number of decoded pixels (which the API now supports).
Leaving a TODO to do this would also be OK with me.
done, without todo
if (IsAllDataReceived() && !all_frames_discovered_ && !Failed()) {What happens if we return something like
`all_frames_discovered_ ? num_decoded_frames_ : num_decoded_frames_ + 1`
?
(or possibly smarter logic, but in general avoiding to decode the entire file here)
tried your solution, ended up with a peek-ahead 2 frame approach that works, and is eliminating the entire file decoding.
need_reset = true;I don't think this is necessary. If we only need the size, we can pause decoding as soon as we get the image's metadata, and resume decoding actual frames from there.
Łukasz AnforowiczJXL format doesn't store frame count in the header. Each frame has is_last flag - you must parse through frames to know the count.
FWIW PNG decoder doesn't actually discover all the frame metadata upfront. For example, you can take a look at [`AnimatedPNGTests.ByteByByteMetaData`](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/image-decoders/png/png_image_decoder_test.cc;l=356;drc=d13bec8226f277ce88e32a23ca0741346e9cf053) to see how `image_decoder->FrameCount()` expectations change as the read offset moves through the input data.
Also note that the doc comment of the (`virtual`/per-codec) `blink::ImageDecoder::DecodeFrameCount` method [says](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/image-decoders/image_decoder.h;l=508-511;drc=d13bec8226f277ce88e32a23ca0741346e9cf053):
```
// This method may return an increasing frame count as frames are received and
// parsed. Alternatively, if the total frame count is available in the image
// header, this method may return the total frame count without checking how
// many frames are received.
```
think with the last PS and 2 frame peek it works! and did not regress any animation glitches - thank you Łukas!.
// Copy pixels to frame buffer.I think we should decode directly into the frame buffer, instead of copying the data over after the fact.
Relatedly, I believe for progressive rendering to work properly we should also `frame.SetPixelsChanged(true);` even if `decode_frame` returns `JxlRsStatus::NeedMoreInput`.
decoding fixed, should be now
-> jxl-rs -> frame_buffer -> in-place permultiply
no more copy.
setpixelchange also addressed.
Please specify a namespace - e.g. `#[cxx::bridge(namespace = "blink")]` (or `#[cxx::bridge(namespace = "blink::jxl_rs")]`).
Done. Added `namespace = "blink::jxl_rs"`
Is this `pub` (and other `pub` below) necessary? This API won't be called from any other Rust crates, right? (i.e. this API will only be called over the `#[cxx::bridge]` / C++ API / FFI API - and this doesn't require `pub` IIUC)
thanks, removed pub
I am focusing my review on cxx::bridge usage and I defer to @veluca for `jxl` API usage, but I noticed that `fn parse_basic_info` here only tries to access-and-store `self.decoder.embedded_color_profile()`, when `self.decoder.process(...)` returns `ProcessingResult::Complete`. With PNG the ICC data comes before IDAT/pixels, so even incomplete decoding can provide ICC info (and I imagine that the partially-decoded pixels should also be color-profile-corrected). So I wanted to double-check if this is ok?
This is correct for JXL. The color profile is part of the image header metadata, and jxl-rs makes it available via `embedded_color_profile()` only after the header is fully parsed (when `process()` returns `Complete`). On `NeedsMoreInput`, the header isn't complete yet, so the ICC profile wouldn't be available. Unlike PNG where ICC comes as a separate chunk before IDAT, JXL bundles it atomically in the header structure.
Does Blink ever use the error message? Maybe `error_message: String` field can be replaced with `had_error: Result<(), ()>` or `has_failed: bool`?
Done. Removed error_message storage entirely - C++ only checks status codes, doesn't use get_error(). Simplified to just return JxlRsStatus::Error
Related details
- Dave Tapuska
- Hans Wennborg
- Luca Versari
- Philip Jägenstedt
- ccameron chromium
- mmenke
- Łukasz Anforowicz
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Luca Versari (Gerrit)
Luca Versari added 3 comments![]( "Open in Gerrit")
constexpr uint64_t kMaxDecodedPixels = 256 * 1024 * 1024;I think this is way too low, it should be at least 4 times this value (level 5 limits want 268435456 *pixels*, and this limit is in *samples*)
buffer.SetPremultiplyAlpha(premultiply_alpha_);(I write this message under the assumption that premultiply_alpha_ is an optimization hint, and we can, if we so wish, use SetPremultiplyAlpha to do differently. Someone with more knowledge of Skia should probably comment on this).
I don't think this is quite right. JPEG XL can have associated alpha channels in the bitstream, and jxl-rs currently does not either multiply or un-premultiply them (especially because that operation is not really possible to do in all cases).
Instead, I think we should set premultiply_alpha_ to whether or not the first alpha channel is premultiplied in the bitstream (it's probably fine to assume it is not for now and leave a todo), and in either case we should not do the premultiplication manually here, as either Skia or jxl-rs are in a better position to do so performance-wise.
// Post-process: apply premultiplication if needed.
// jxl-rs outputs non-premultiplied, Skia may need premultiplied.
if (premultiply) {
// SAFETY: frame_pixels is valid for buffer_size bytes as verified
// above.
base::span<uint8_t> buffer_span =
UNSAFE_BUFFERS(base::span(frame_pixels, buffer_size));
if (decode_to_half_float_) {
// In-place premultiplication for RGBA F16.
// RGBA F16 layout: R=bytes 0-1, G=bytes 2-3, B=bytes 4-5, A=bytes
// 6-7
for (uint32_t y = 0; y < height; ++y) {
auto row = buffer_span.subspan(y * row_stride, width * 8);
for (uint32_t x = 0; x < width; ++x) {
auto pixel = row.subspan(x * 8, 8u);
uint16_t r_half = static_cast<uint16_t>(pixel[0]) |
(static_cast<uint16_t>(pixel[1]) << 8);
uint16_t g_half = static_cast<uint16_t>(pixel[2]) |
(static_cast<uint16_t>(pixel[3]) << 8);
uint16_t b_half = static_cast<uint16_t>(pixel[4]) |
(static_cast<uint16_t>(pixel[5]) << 8);
uint16_t a_half = static_cast<uint16_t>(pixel[6]) |
(static_cast<uint16_t>(pixel[7]) << 8);
float r = HalfToFloat(r_half);
float g = HalfToFloat(g_half);
float b = HalfToFloat(b_half);
float a = HalfToFloat(a_half);
r *= a;
g *= a;
b *= a;
r_half = FloatToHalf(r);
g_half = FloatToHalf(g);
b_half = FloatToHalf(b);
pixel[0] = static_cast<uint8_t>(r_half & 0xFF);
pixel[1] = static_cast<uint8_t>(r_half >> 8);
pixel[2] = static_cast<uint8_t>(g_half & 0xFF);
pixel[3] = static_cast<uint8_t>(g_half >> 8);
pixel[4] = static_cast<uint8_t>(b_half & 0xFF);
pixel[5] = static_cast<uint8_t>(b_half >> 8);
// Alpha stays the same
}
}
} else {
// In-place premultiplication for BGRA8.
// BGRA layout: B=0, G=1, R=2, A=3
for (uint32_t y = 0; y < height; ++y) {
auto row = buffer_span.subspan(y * row_stride, width * 4);
for (uint32_t x = 0; x < width; ++x) {
auto pixel = row.subspan(x * 4, 4u);
uint8_t b = pixel[0];
uint8_t g = pixel[1];
uint8_t r = pixel[2];
uint8_t a = pixel[3];
pixel[0] = (b * a + 128) >> 8;
pixel[1] = (g * a + 128) >> 8;
pixel[2] = (r * a + 128) >> 8;
// Alpha stays the same
}
}
}
}
See comment above on premultiplication. I also think this doesn't work as written, since with partial rendering some parts of the image will be multiplied twice.
Related details
- Dave Tapuska
- Hans Wennborg
- Helmut Januschka
- Philip Jägenstedt
- ccameron chromium
- mmenke
- Łukasz Anforowicz
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. |
mmenke (Gerrit)
mmenke voted and added 2 comments![]( "Open in Gerrit")
Votes added by mmenke
| Code-Review | +1 |
2 comments
Signing off now, since I'm out for the next two weeks. Given the size of this CL, though, probably best to hold off until January before landing, anyways, just so folks are around in the case of breakage.
EXPECT_TRUE(IsSupportedImageMimeType("Image/JPEG"));
EXPECT_FALSE(IsSupportedImageMimeType("image/jxl"));We should still make sure this isn't supported when !BUILDFLAG(ENABLE_JXL_DECODER)
Related details
- Dave Tapuska
- Hans Wennborg
- Helmut Januschka
- Philip Jägenstedt
- ccameron chromium
- Łukasz Anforowicz
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
Łukasz Anforowicz (Gerrit)
Łukasz Anforowicz added 1 comment![]( "Open in Gerrit")
buffer.SetPremultiplyAlpha(premultiply_alpha_);(I write this message under the assumption that premultiply_alpha_ is an optimization hint, and we can, if we so wish, use SetPremultiplyAlpha to do differently. Someone with more knowledge of Skia should probably comment on this).
I don't think this is quite right. JPEG XL can have associated alpha channels in the bitstream, and jxl-rs currently does not either multiply or un-premultiply them (especially because that operation is not really possible to do in all cases).
Instead, I think we should set premultiply_alpha_ to whether or not the first alpha channel is premultiplied in the bitstream (it's probably fine to assume it is not for now and leave a todo), and in either case we should not do the premultiplication manually here, as either Skia or jxl-rs are in a better position to do so performance-wise.
[...] premultiply_alpha_ is an optimization hint [...] Someone with more knowledge of Skia should probably comment on this).
I don't have _much_ more knowledge of Skia, but I think you want to do something similar to the code here: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/image-decoders/skia/skia_image_decoder_base.cc;l=365-372;drc=d13bec8226f277ce88e32a23ca0741346e9cf053 where Blink-side `premultiply_alpha_` (whether output should use premultiplied alpha or not) is translated into `SkImageInfo image_info = ...` that tells a Skia's `SkCodec` how the output should look like. I am guessing that `ImageDecoder::premultiply_alpha_` is based on `AlphaOption` passed to the constructor of `SkiaImageDecoderBase`.
So:
- IIUC treatment of alpha is not just a hint, but a requirement
- It does seem to make sense to process alpha within the codec (there are exceptions of course - the old libpng-based PNG codec did this in Blink)
Related details
- Dave Tapuska
- Hans Wennborg
- Helmut Januschka
- Philip Jägenstedt
- ccameron chromium
Helmut Januschka (Gerrit)
Helmut Januschka added 3 comments![]( "Open in Gerrit")
constexpr uint64_t kMaxDecodedPixels = 256 * 1024 * 1024;I think this is way too low, it should be at least 4 times this value (level 5 limits want 268435456 *pixels*, and this limit is in *samples*)
Done
(I write this message under the assumption that premultiply_alpha_ is an optimization hint, and we can, if we so wish, use SetPremultiplyAlpha to do differently. Someone with more knowledge of Skia should probably comment on this).
I don't think this is quite right. JPEG XL can have associated alpha channels in the bitstream, and jxl-rs currently does not either multiply or un-premultiply them (especially because that operation is not really possible to do in all cases).
Instead, I think we should set premultiply_alpha_ to whether or not the first alpha channel is premultiplied in the bitstream (it's probably fine to assume it is not for now and leave a todo), and in either case we should not do the premultiplication manually here, as either Skia or jxl-rs are in a better position to do so performance-wise.
Done. Removed manual premultiplication entirely - jxl-rs should handle this.
this CL is broken right now, it requires https://github.com/libjxl/jxl-rs/pull/574 to land and roll!
// Post-process: apply premultiplication if needed.See comment above on premultiplication. I also think this doesn't work as written, since with partial rendering some parts of the image will be multiplied twice.
- Dave Tapuska
- Hans Wennborg
- Luca Versari
- Philip Jägenstedt
- ccameron chromium
- mmenke
- Łukasz Anforowicz
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. |