gfx::HDRMetadata: Use skhdr types [chromium/src : main]
ccameron chromium (Gerrit)
ccameron chromium voted and added 1 comment![]( "Open in Gerrit")
Votes added by ccameron chromium
| Commit-Queue | +1 |
1 comment
ptal. This should have no functional effects -- just a long-overdue de-duping of structures that have moved to Skia.
sandv for chromecast
dalecurtis for media and friends
dcheng for IPC
Related details
- Dale Curtis
- Daniel Cheng
- Sandeep Vijayasekar
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 6 comments![]( "Open in Gerrit")
return static_cast<int32_t>(hdr_metadata_->HasCLLI()Should we change the return type here instead?
int32_t JniHdrMetadata::MaxFrameAverageLuminance(JNIEnv* env) {Ditto
static_cast<float>(level_information.max_content_light_level),Should we align the types to avoid the casts?
static_cast<float>(level_information.max_content_light_level),Ditto
return {static_cast<float>(max_content_light_level),Ditto on type alignment
v8_metadata->minimumLuminance(),Gemini: It looks like `minimumLuminance` and `maximumLuminance` are swapped here. The `skhdr::MasteringDisplayColorVolume` struct takes maximum luminance as the second argument and minimum as the third.
To prevent this kind of error, I'd recommend using designated initializers, which is done in many other places in this CL:
```cpp
hdr_metadata.SetMDCV(
skhdr::MasteringDisplayColorVolume{
.fDisplayPrimaries = {
v8_metadata->redPrimaryX(), v8_metadata->redPrimaryY(),
v8_metadata->greenPrimaryX(), v8_metadata->greenPrimaryY(),
v8_metadata->bluePrimaryX(), v8_metadata->bluePrimaryY(),
v8_metadata->whitePointX(), v8_metadata->whitePointY(),
},
.fMaximumDisplayMasteringLuminance = v8_metadata->maximumLuminance(),
.fMinimumDisplayMasteringLuminance = v8_metadata->minimumLuminance(),
});
```
Related details
- Daniel Cheng
- Sandeep Vijayasekar
- 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. |
Daniel Cheng (Gerrit)
Daniel Cheng added 1 comment![]( "Open in Gerrit")
static_cast<float>(container->clli.maxCLL),Do we need to care about rounding at all here? What happened previously?
Related details
- Sandeep Vijayasekar
- 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. |
ccameron chromium (Gerrit)
ccameron chromium added 9 comments![]( "Open in Gerrit")
return static_cast<int32_t>(hdr_metadata_->HasCLLI()Should we change the return type here instead?
E
return static_cast<int32_t>(hdr_metadata_->HasCLLI()Should we change the return type here instead?
The types for CLLI are an unfortunate mess. There are two different definitions -- one used by CTA and one by W3C. There's some background [here](https://source.chromium.org/chromium/chromium/src/+/main:third_party/skia/include/private/SkHdrMetadata.h;l=26;drc=7578ad6ba4a241db855165753be9fe75f6439d32).
The CTA definition only supports integer values. The W3C version is non-integer. When I created the skhdr type I used floats so we could round-trip PNG encode/decode without losing precision (support for skia-based decoders was a forcing function to add HDR metadata to skia).
So that's why we have this mismatch, and why we now have to cast the ints to floats. The alternatives are:
- Have two types: CLLI_W3C and CLLI_CTA
- Use integers, and accept that PNGs won't round-trip
- To skhdr::ContentLightLevelInformation, add a `MakeUint16` function that takes `uint16_t`s, and `getUint16MaxContentLightLevel` function
The last option is probably the best, so I added it [here](https://skia-review.googlesource.com/c/skia/+/1190056). It will require a skia roll, etc, so I'd rather land the massive patch (having it hang out and get stale makes me anxious!) and follow-on after I add the Skia interface.
When I do that second pass I'll also probably want to change the default values being used here (they're zeros, but not obviously so).
int32_t JniHdrMetadata::MaxFrameAverageLuminance(JNIEnv* env) {ccameron chromiumDitto
(see above)
static_cast<float>(level_information.max_content_light_level),Should we align the types to avoid the casts?
(see above)
static_cast<float>(level_information.max_content_light_level),ccameron chromiumDitto
(see above)
return {static_cast<float>(max_content_light_level),ccameron chromiumDitto on type alignment
(see above)
Gemini: It looks like `minimumLuminance` and `maximumLuminance` are swapped here. The `skhdr::MasteringDisplayColorVolume` struct takes maximum luminance as the second argument and minimum as the third.
To prevent this kind of error, I'd recommend using designated initializers, which is done in many other places in this CL:
```cpp
hdr_metadata.SetMDCV(
skhdr::MasteringDisplayColorVolume{
.fDisplayPrimaries = {
v8_metadata->redPrimaryX(), v8_metadata->redPrimaryY(),
v8_metadata->greenPrimaryX(), v8_metadata->greenPrimaryY(),
v8_metadata->bluePrimaryX(), v8_metadata->bluePrimaryY(),
v8_metadata->whitePointX(), v8_metadata->whitePointY(),
},
.fMaximumDisplayMasteringLuminance = v8_metadata->maximumLuminance(),
.fMinimumDisplayMasteringLuminance = v8_metadata->minimumLuminance(),
});
```
Fixed.
Gemini review is so great. (Gemini-CLI was the one that wrote this code, so ... room for improvement).
(And hopefully this prototype API will never see the light of day ... so glad we didn't expose this metadata to the web).
static_cast<float>(container->clli.maxCLL),Do we need to care about rounding at all here? What happened previously?
(see the above comment about CLLI's multiple inconsistent rounding behaviors)
Related details
- Dale Curtis
- Daniel Cheng
- Sandeep Vijayasekar
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. |
Dale Curtis (Gerrit)
Dale Curtis voted and added 3 comments![]( "Open in Gerrit")
Votes added by Dale Curtis
| Code-Review | +1 |
3 comments
return static_cast<int32_t>(hdr_metadata_->HasCLLI()ccameron chromiumShould we change the return type here instead?
The types for CLLI are an unfortunate mess. There are two different definitions -- one used by CTA and one by W3C. There's some background [here](https://source.chromium.org/chromium/chromium/src/+/main:third_party/skia/include/private/SkHdrMetadata.h;l=26;drc=7578ad6ba4a241db855165753be9fe75f6439d32).
The CTA definition only supports integer values. The W3C version is non-integer. When I created the skhdr type I used floats so we could round-trip PNG encode/decode without losing precision (support for skia-based decoders was a forcing function to add HDR metadata to skia).
So that's why we have this mismatch, and why we now have to cast the ints to floats. The alternatives are:
- Have two types: CLLI_W3C and CLLI_CTA
- Use integers, and accept that PNGs won't round-trip
- To skhdr::ContentLightLevelInformation, add a `MakeUint16` function that takes `uint16_t`s, and `getUint16MaxContentLightLevel` function
The last option is probably the best, so I added it [here](https://skia-review.googlesource.com/c/skia/+/1190056). It will require a skia roll, etc, so I'd rather land the massive patch (having it hang out and get stale makes me anxious!) and follow-on after I add the Skia interface.
When I do that second pass I'll also probably want to change the default values being used here (they're zeros, but not obviously so).
Acknowledged
return {static_cast<float>(cll.max_cll), static_cast<float>(cll.max_fall)};Gemini: Same here, designated initializers are preferred:
```cpp
return {.fMaxCLL = static_cast<float>(cll.max_cll),
.fMaxFALL = static_cast<float>(cll.max_fall)};
```
return {static_cast<float>(max_content_light_level),Gemini: Prefer using designated initializers here for consistency and safety, as you did for MDCV in other files.
```cpp
return {.fMaxCLL = static_cast<float>(max_content_light_level),
.fMaxFALL = static_cast<float>(max_picture_average_light_level)};
```
Related details
- Daniel Cheng
- Sandeep Vijayasekar
- 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. |
Daniel Cheng (Gerrit)
Daniel Cheng voted and added 2 comments![]( "Open in Gerrit")
Votes added by Daniel Cheng
| Code-Review | +1 |
2 comments
static_cast<float>(container->clli.maxCLL),ccameron chromiumDo we need to care about rounding at all here? What happened previously?
(see the above comment about CLLI's multiple inconsistent rounding behaviors)
I guess skia should have updated by now but I'm not going to push on this and I'll leave it to you. I personally think it's harder to make sure to go back and clean things up with the way C++ works but shrug.
Related details
- Sandeep Vijayasekar
- ccameron chromium
Sandeep Vijayasekar (Gerrit)
Sandeep Vijayasekar voted and added 1 comment![]( "Open in Gerrit")
Votes added by Sandeep Vijayasekar
| Code-Review | +1 |
1 comment
Related details
- 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. |
ccameron chromium (Gerrit)
ccameron chromium voted and added 5 comments![]( "Open in Gerrit")
Votes added by ccameron chromium
| Commit-Queue | +2 |
5 comments
return static_cast<int32_t>(hdr_metadata_->HasCLLI()ccameron chromiumShould we change the return type here instead?
Dale CurtisThe types for CLLI are an unfortunate mess. There are two different definitions -- one used by CTA and one by W3C. There's some background [here](https://source.chromium.org/chromium/chromium/src/+/main:third_party/skia/include/private/SkHdrMetadata.h;l=26;drc=7578ad6ba4a241db855165753be9fe75f6439d32).
The CTA definition only supports integer values. The W3C version is non-integer. When I created the skhdr type I used floats so we could round-trip PNG encode/decode without losing precision (support for skia-based decoders was a forcing function to add HDR metadata to skia).
So that's why we have this mismatch, and why we now have to cast the ints to floats. The alternatives are:
- Have two types: CLLI_W3C and CLLI_CTA
- Use integers, and accept that PNGs won't round-trip
- To skhdr::ContentLightLevelInformation, add a `MakeUint16` function that takes `uint16_t`s, and `getUint16MaxContentLightLevel` function
The last option is probably the best, so I added it [here](https://skia-review.googlesource.com/c/skia/+/1190056). It will require a skia roll, etc, so I'd rather land the massive patch (having it hang out and get stale makes me anxious!) and follow-on after I add the Skia interface.
When I do that second pass I'll also probably want to change the default values being used here (they're zeros, but not obviously so).
Acknowledged
Updated to use the new Skia interface
int32_t JniHdrMetadata::MaxFrameAverageLuminance(JNIEnv* env) {ccameron chromiumDitto
(see above)
Updated to use the new Skia interface
return {static_cast<float>(cll.max_cll), static_cast<float>(cll.max_fall)};Gemini: Same here, designated initializers are preferred:
```cpp
return {.fMaxCLL = static_cast<float>(cll.max_cll),
.fMaxFALL = static_cast<float>(cll.max_fall)};
```
Changed to use `skhdr::ContentLightLevelInformation::MakeUint16`
return {static_cast<float>(max_content_light_level),Gemini: Prefer using designated initializers here for consistency and safety, as you did for MDCV in other files.
```cpp
return {.fMaxCLL = static_cast<float>(max_content_light_level),
.fMaxFALL = static_cast<float>(max_picture_average_light_level)};
```
Changed to use `skhdr::ContentLightLevelInformation::MakeUint16`
static_cast<float>(container->clli.maxCLL),ccameron chromiumDo we need to care about rounding at all here? What happened previously?
Daniel Cheng(see the above comment about CLLI's multiple inconsistent rounding behaviors)
I guess skia should have updated by now but I'm not going to push on this and I'll leave it to you. I personally think it's harder to make sure to go back and clean things up with the way C++ works but shrug.
Updated to use `skhdr::ContentLightLevelInformation::MakeUint16`
Related details
- Dale Curtis
- Daniel Cheng
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. |
ccameron chromium (Gerrit)
ccameron chromium voted and added 3 comments![]( "Open in Gerrit")
Votes added by ccameron chromium
| Commit-Queue | +2 |
3 comments
return {static_cast<float>(cll.max_cll), static_cast<float>(cll.max_fall)};ccameron chromiumGemini: Same here, designated initializers are preferred:
```cpp
return {.fMaxCLL = static_cast<float>(cll.max_cll),
.fMaxFALL = static_cast<float>(cll.max_fall)};
```
Changed to use `skhdr::ContentLightLevelInformation::MakeUint16`
Done
return {static_cast<float>(max_content_light_level),ccameron chromiumGemini: Prefer using designated initializers here for consistency and safety, as you did for MDCV in other files.
```cpp
return {.fMaxCLL = static_cast<float>(max_content_light_level),
.fMaxFALL = static_cast<float>(max_picture_average_light_level)};
```
Changed to use `skhdr::ContentLightLevelInformation::MakeUint16`
Done
static_cast<float>(container->clli.maxCLL),ccameron chromiumDo we need to care about rounding at all here? What happened previously?
Daniel Cheng(see the above comment about CLLI's multiple inconsistent rounding behaviors)
ccameron chromiumI guess skia should have updated by now but I'm not going to push on this and I'll leave it to you. I personally think it's harder to make sure to go back and clean things up with the way C++ works but shrug.
Updated to use `skhdr::ContentLightLevelInformation::MakeUint16`
Done
Related details
- Dale Curtis
- Daniel Cheng
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
ccameron chromium (Gerrit)
ccameron chromium voted Commit-Queue+2![]( "Open in Gerrit")
- Daniel Cheng
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Cheng (Gerrit)
Daniel Cheng voted![]( "Open in Gerrit")
- ccameron chromium
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Cheng (Gerrit)
Daniel Cheng voted Commit-Queue+2![]( "Open in Gerrit")
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
gfx::HDRMetadata: Use skhdr types
The HDR metadata structures have been added to Skia (so that they can be
communicated from Skia decoders). Remove the now-redundant gfx metadata
structure.
Take this opportunity to use setters and getters for the members of
gfx::HDRMetadata. This will allow future refactoring to reduce the size
of the structure.
Leave the messy AGTM interface in place. It will be addressed by a
future patch. Also leave the extended range in place, because it, too,
will need a nontrivial clean-up.
- M cc/paint/gpu_raster_pixeltest.cc
- M cc/paint/tone_map_util.cc
- M chromecast/media/cma/base/decoder_config_adapter.cc
- M chromecast/starboard/media/renderer/chromium_starboard_conversions.cc
- M chromecast/starboard/media/renderer/chromium_starboard_conversions_test.cc
- M components/viz/service/display/overlay_dc_unittest.cc
- M components/viz/service/display/skia_renderer.cc
- M media/base/android/jni_hdr_metadata.cc
- M media/base/media_serializers.h
- M media/ffmpeg/ffmpeg_common.cc
- M media/ffmpeg/ffmpeg_common_unittest.cc
- M media/formats/mp4/box_definitions.cc
- M media/formats/mp4/hevc.cc
- M media/formats/mp4/mp4_stream_parser_unittest.cc
- M media/formats/webm/webm_colour_parser.cc
- M media/formats/webm/webm_colour_parser.h
- M media/formats/webm/webm_stream_parser_unittest.cc
- M media/gpu/android/media_codec_video_decoder_unittest.cc
- M media/gpu/av1_decoder.cc
- M media/gpu/h264_decoder.cc
- M media/gpu/h265_decoder.cc
- M media/gpu/mac/vt_config_util_unittest.mm
- M media/mojo/mojom/video_decoder_config_mojom_traits_unittest.cc
- M media/parsers/h264_parser.cc
- M media/parsers/h264_parser.h
- M media/parsers/h265_parser.cc
- M media/parsers/h265_parser.h
- M media/renderers/video_resource_updater_unittest.cc
- M media/renderers/win/media_foundation_video_stream.cc
- M media/video/mappable_shared_image_video_frame_pool_unittest.cc
- M third_party/blink/renderer/core/html/canvas/predefined_color_space.cc
- M third_party/blink/renderer/platform/image-decoders/avif/avif_image_decoder.cc
- M third_party/blink/renderer/platform/image-decoders/png/png_image_decoder_test.cc
- M third_party/blink/tools/blinkpy/presubmit/audit_non_blink_usage.py
- M ui/gfx/android/android_surface_control_compat.cc
- M ui/gfx/hdr_metadata.cc
- M ui/gfx/hdr_metadata.h
- M ui/gfx/hdr_metadata_mac.mm
- M ui/gfx/mojom/BUILD.gn
- M ui/gfx/mojom/hdr_metadata.mojom
- M ui/gfx/mojom/hdr_metadata_mojom_traits.cc
- M ui/gfx/mojom/hdr_metadata_mojom_traits.h
- M ui/gfx/mojom/mojom_traits_unittest.cc
- M ui/gl/hdr_metadata_helper_win.cc
- M ui/gl/swap_chain_presenter.cc
- M ui/ozone/platform/wayland/host/wayland_wp_color_manager.cc
Code-Review: +1 by Dale Curtis, +1 by Sandeep Vijayasekar, +1 by Daniel Cheng
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |