gfx::HDRMetadata: Remove gfx::HdrMetadataAgtm [chromium/src : main]
ccameron chromium (Gerrit)
ccameron chromium added 1 comment![]( "Open in Gerrit")
ptal -- more in the same vein of pushing HDR to Skia
Related details
- Dale Curtis
- Daniel Cheng
- Philip Rogers
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 added 3 comments![]( "Open in Gerrit")
if (!a && !b) {This reminds me a lot of https://source.chromium.org/chromium/chromium/src/+/main:base/memory/values_equivalent.h;l=26;drc=397107e399d4b0f4a6e801e7a888d7d71778529c and maybe it's something we should add there if we'll need more of it.
result.assign(data->bytes(), std::next(data->bytes(), data->size()));Can we just pull a span out here? Do we need to strongly own this?
(It's kind of unfortunate that even just calling this refs the SkData, but nothing I can do about that here)
if (!agtm_data.is_null() && agtm_data.size() > 0) {Out of curiosity why the additional size check now? Is this just to save an allocation?
Related details
- Dale Curtis
- Philip Rogers
- 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. |
Philip Rogers (Gerrit)
Philip Rogers voted and added 1 comment![]( "Open in Gerrit")
Votes added by Philip Rogers
| Code-Review | +1 |
1 comment
third_party/blink/renderer/core/html/canvas/predefined_color_space.cc LGTM
Related details
- Dale Curtis
- 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. |
Dale Curtis (Gerrit)
Dale Curtis added 1 comment![]( "Open in Gerrit")
sk_sp<SkData> payload;I had forgotten this is a raw payload. Do we sanitize this before sending it out of the renderer process?
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 added 3 comments![]( "Open in Gerrit")
result.assign(data->bytes(), std::next(data->bytes(), data->size()));Can we just pull a span out here? Do we need to strongly own this?
(It's kind of unfortunate that even just calling this refs the SkData, but nothing I can do about that here)
Added a helper function that takes a `const SkData*` so we can avoid the ref.
if (!agtm_data.is_null() && agtm_data.size() > 0) {Out of curiosity why the additional size check now? Is this just to save an allocation?
It was because I had un-optionalized the array in mojom, which made "an SkData of size 0" and "a nullptr SkData" indistinguishable. In practice, they are (a zero-size agtm will fail parsing later on), but that was sloppy, so I've re-optionalized it and removed the check.
Related details
- Daniel Cheng
- Philip Rogers
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 1 comment![]( "Open in Gerrit")
Votes added by ccameron chromium
| Commit-Queue | +1 |
1 comment
sk_sp<SkData> payload;I had forgotten this is a raw payload. Do we sanitize this before sending it out of the renderer process?
We only look at the bytes at render time (and we'll likely hash based on the data).
Skia doesn't have an interface to the internals of this metadata (just a "parse it" and "make a shader from it" interface), so there doesn't exist a parsed form to pass around.
If parsing at a higher privilege level is a concern (the parse is fairly trivial), then we can parse-then-reserialize so that only Chromium-produced metadata goes out from the renderer. But that will need to wait for some test updates because ...
...the payload is "sometimes JSON, sometimes binary", because we used JSON for prototyping and only now are moving to binary because it's stable enough. Some tests and test files will need to be updated before we can delete the JSON version.
(It's still all behind a flag, of course)
Related details
- Dale Curtis
- Daniel Cheng
- Philip Rogers
- 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. |
Dale Curtis (Gerrit)
Dale Curtis added 1 comment![]( "Open in Gerrit")
sk_sp<SkData> payload;ccameron chromiumI had forgotten this is a raw payload. Do we sanitize this before sending it out of the renderer process?
We only look at the bytes at render time (and we'll likely hash based on the data).
Skia doesn't have an interface to the internals of this metadata (just a "parse it" and "make a shader from it" interface), so there doesn't exist a parsed form to pass around.
If parsing at a higher privilege level is a concern (the parse is fairly trivial), then we can parse-then-reserialize so that only Chromium-produced metadata goes out from the renderer. But that will need to wait for some test updates because ...
...the payload is "sometimes JSON, sometimes binary", because we used JSON for prototyping and only now are moving to binary because it's stable enough. Some tests and test files will need to be updated before we can delete the JSON version.
(It's still all behind a flag, of course)
I defer to @dch...@chromium.org from the security perspective. Hearing sometimes JSON, sometimes binary, does make me think it should be pre-processed in sandboxed process first.
Related details
- Daniel Cheng
- Philip Rogers
- 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 1 comment![]( "Open in Gerrit")
sk_sp<SkData> payload;ccameron chromiumI had forgotten this is a raw payload. Do we sanitize this before sending it out of the renderer process?
Dale CurtisWe only look at the bytes at render time (and we'll likely hash based on the data).
Skia doesn't have an interface to the internals of this metadata (just a "parse it" and "make a shader from it" interface), so there doesn't exist a parsed form to pass around.
If parsing at a higher privilege level is a concern (the parse is fairly trivial), then we can parse-then-reserialize so that only Chromium-produced metadata goes out from the renderer. But that will need to wait for some test updates because ...
...the payload is "sometimes JSON, sometimes binary", because we used JSON for prototyping and only now are moving to binary because it's stable enough. Some tests and test files will need to be updated before we can delete the JSON version.
(It's still all behind a flag, of course)
I defer to @dch...@chromium.org from the security perspective. Hearing sometimes JSON, sometimes binary, does make me think it should be pre-processed in sandboxed process first.
Oh, the JSON part is prototyping stuff that is going to be deleted once tests are updated (so, like, next-week-ish). Never to see the light of day.
Related details
- Dale Curtis
- Daniel Cheng
- Philip Rogers
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 1 comment![]( "Open in Gerrit")
Votes added by Dale Curtis
| Code-Review | +1 |
1 comment
media/ lgtm % the security Q, which isn't any worse at least in this CL.
Related details
- Daniel Cheng
- Philip Rogers
- 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 3 comments![]( "Open in Gerrit")
Votes added by Daniel Cheng
| Code-Review | +1 |
3 comments
sk_sp<SkData> payload;ccameron chromiumI had forgotten this is a raw payload. Do we sanitize this before sending it out of the renderer process?
Dale CurtisWe only look at the bytes at render time (and we'll likely hash based on the data).
Skia doesn't have an interface to the internals of this metadata (just a "parse it" and "make a shader from it" interface), so there doesn't exist a parsed form to pass around.
If parsing at a higher privilege level is a concern (the parse is fairly trivial), then we can parse-then-reserialize so that only Chromium-produced metadata goes out from the renderer. But that will need to wait for some test updates because ...
...the payload is "sometimes JSON, sometimes binary", because we used JSON for prototyping and only now are moving to binary because it's stable enough. Some tests and test files will need to be updated before we can delete the JSON version.
(It's still all behind a flag, of course)
ccameron chromiumI defer to @dch...@chromium.org from the security perspective. Hearing sometimes JSON, sometimes binary, does make me think it should be pre-processed in sandboxed process first.
Oh, the JSON part is prototyping stuff that is going to be deleted once tests are updated (so, like, next-week-ish). Never to see the light of day.
I did notice this originally and it's not great. But I think it's pre-existing, right?
(I'd probably file a bug about this to explicitly track fixing it though)
result.assign(data->bytes(), std::next(data->bytes(), data->size()));ccameron chromiumCan we just pull a span out here? Do we need to strongly own this?
(It's kind of unfortunate that even just calling this refs the SkData, but nothing I can do about that here)
Added a helper function that takes a `const SkData*` so we can avoid the ref.
Thanks!
if (!agtm_data.is_null() && agtm_data.size() > 0) {ccameron chromiumOut of curiosity why the additional size check now? Is this just to save an allocation?
It was because I had un-optionalized the array in mojom, which made "an SkData of size 0" and "a nullptr SkData" indistinguishable. In practice, they are (a zero-size agtm will fail parsing later on), but that was sloppy, so I've re-optionalized it and removed the check.
- Philip Rogers
- 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 1 comment![]( "Open in Gerrit")
sk_sp<SkData> payload;ccameron chromiumI had forgotten this is a raw payload. Do we sanitize this before sending it out of the renderer process?
Dale CurtisWe only look at the bytes at render time (and we'll likely hash based on the data).
Skia doesn't have an interface to the internals of this metadata (just a "parse it" and "make a shader from it" interface), so there doesn't exist a parsed form to pass around.
If parsing at a higher privilege level is a concern (the parse is fairly trivial), then we can parse-then-reserialize so that only Chromium-produced metadata goes out from the renderer. But that will need to wait for some test updates because ...
...the payload is "sometimes JSON, sometimes binary", because we used JSON for prototyping and only now are moving to binary because it's stable enough. Some tests and test files will need to be updated before we can delete the JSON version.
(It's still all behind a flag, of course)
ccameron chromiumI defer to @dch...@chromium.org from the security perspective. Hearing sometimes JSON, sometimes binary, does make me think it should be pre-processed in sandboxed process first.
Daniel ChengOh, the JSON part is prototyping stuff that is going to be deleted once tests are updated (so, like, next-week-ish). Never to see the light of day.
I did notice this originally and it's not great. But I think it's pre-existing, right?
(I'd probably file a bug about this to explicitly track fixing it though)
Yes, it's pre-existing. The follow-on to remove the JSON part of it is at:
https://chromium-review.googlesource.com/c/chromium/src/+/7256035
If indeed we don't want to send unparsed T35 then we'll need to create a parsed representation, etc (which is a sort of messy API to maintain).
Related details
- Dale Curtis
- Daniel Cheng
- Philip Rogers
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 1 comment![]( "Open in Gerrit")
sk_sp<SkData> payload;ccameron chromiumI had forgotten this is a raw payload. Do we sanitize this before sending it out of the renderer process?
Dale CurtisWe only look at the bytes at render time (and we'll likely hash based on the data).
Skia doesn't have an interface to the internals of this metadata (just a "parse it" and "make a shader from it" interface), so there doesn't exist a parsed form to pass around.
If parsing at a higher privilege level is a concern (the parse is fairly trivial), then we can parse-then-reserialize so that only Chromium-produced metadata goes out from the renderer. But that will need to wait for some test updates because ...
...the payload is "sometimes JSON, sometimes binary", because we used JSON for prototyping and only now are moving to binary because it's stable enough. Some tests and test files will need to be updated before we can delete the JSON version.
(It's still all behind a flag, of course)
ccameron chromiumI defer to @dch...@chromium.org from the security perspective. Hearing sometimes JSON, sometimes binary, does make me think it should be pre-processed in sandboxed process first.
Daniel ChengOh, the JSON part is prototyping stuff that is going to be deleted once tests are updated (so, like, next-week-ish). Never to see the light of day.
ccameron chromiumI did notice this originally and it's not great. But I think it's pre-existing, right?
(I'd probably file a bug about this to explicitly track fixing it though)
Yes, it's pre-existing. The follow-on to remove the JSON part of it is at:
https://chromium-review.googlesource.com/c/chromium/src/+/7256035If indeed we don't want to send unparsed T35 then we'll need to create a parsed representation, etc (which is a sort of messy API to maintain).
Done
Related details
- Dale Curtis
- Daniel Cheng
- Philip Rogers
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")
| Commit-Queue | +2 |
Daniel Cheng (Gerrit)
Daniel Cheng added 1 comment![]( "Open in Gerrit")
sk_sp<SkData> payload;ccameron chromiumI had forgotten this is a raw payload. Do we sanitize this before sending it out of the renderer process?
Dale CurtisWe only look at the bytes at render time (and we'll likely hash based on the data).
Skia doesn't have an interface to the internals of this metadata (just a "parse it" and "make a shader from it" interface), so there doesn't exist a parsed form to pass around.
If parsing at a higher privilege level is a concern (the parse is fairly trivial), then we can parse-then-reserialize so that only Chromium-produced metadata goes out from the renderer. But that will need to wait for some test updates because ...
...the payload is "sometimes JSON, sometimes binary", because we used JSON for prototyping and only now are moving to binary because it's stable enough. Some tests and test files will need to be updated before we can delete the JSON version.
(It's still all behind a flag, of course)
ccameron chromiumI defer to @dch...@chromium.org from the security perspective. Hearing sometimes JSON, sometimes binary, does make me think it should be pre-processed in sandboxed process first.
Daniel ChengOh, the JSON part is prototyping stuff that is going to be deleted once tests are updated (so, like, next-week-ish). Never to see the light of day.
ccameron chromiumI did notice this originally and it's not great. But I think it's pre-existing, right?
(I'd probably file a bug about this to explicitly track fixing it though)
ccameron chromiumYes, it's pre-existing. The follow-on to remove the JSON part of it is at:
https://chromium-review.googlesource.com/c/chromium/src/+/7256035If indeed we don't want to send unparsed T35 then we'll need to create a parsed representation, etc (which is a sort of messy API to maintain).
Done
To be clear, parsing of any sort is potentially problematic if the source of truth is an untrustworthy process. It sounds like this may not be following the rule of 2, hence the request for a followup bug.
Related details
- Dale Curtis
- Philip Rogers
- ccameron chromium
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 voted and added 1 comment![]( "Open in Gerrit")
Votes added by Dale Curtis
| Code-Review | +1 |
1 comment
sk_sp<SkData> payload;ccameron chromiumI had forgotten this is a raw payload. Do we sanitize this before sending it out of the renderer process?
Dale CurtisWe only look at the bytes at render time (and we'll likely hash based on the data).
Skia doesn't have an interface to the internals of this metadata (just a "parse it" and "make a shader from it" interface), so there doesn't exist a parsed form to pass around.
If parsing at a higher privilege level is a concern (the parse is fairly trivial), then we can parse-then-reserialize so that only Chromium-produced metadata goes out from the renderer. But that will need to wait for some test updates because ...
...the payload is "sometimes JSON, sometimes binary", because we used JSON for prototyping and only now are moving to binary because it's stable enough. Some tests and test files will need to be updated before we can delete the JSON version.
(It's still all behind a flag, of course)
ccameron chromiumI defer to @dch...@chromium.org from the security perspective. Hearing sometimes JSON, sometimes binary, does make me think it should be pre-processed in sandboxed process first.
Daniel ChengOh, the JSON part is prototyping stuff that is going to be deleted once tests are updated (so, like, next-week-ish). Never to see the light of day.
ccameron chromiumI did notice this originally and it's not great. But I think it's pre-existing, right?
(I'd probably file a bug about this to explicitly track fixing it though)
ccameron chromiumYes, it's pre-existing. The follow-on to remove the JSON part of it is at:
https://chromium-review.googlesource.com/c/chromium/src/+/7256035If indeed we don't want to send unparsed T35 then we'll need to create a parsed representation, etc (which is a sort of messy API to maintain).
Done
If the AGTM parser is pretty simple, maybe rewriting in Rust is an option?
Related details
- Philip Rogers
- ccameron chromium
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
gfx::HDRMetadata: Remove gfx::HdrMetadataAgtm
This is towards moving gfx::HDRMetadata to be skhdr::Metadata, so that
we can share HDR rendering across Chrome and Android, and so we can
share metadata parsing between Chrome and Skia codecs.
skhdr::Metadata just stores an sk_sp<SkData> of the serialize metadata.
We do not parse the metadata until the very last moment, when we are
rendering the image/video that uses it (and when we do that, we should
have a cache of the parsed result and its shaders, etc).
Remove the Remove gfx::HdrMetadataAgtm structure, and make it just be a
(lonely) private member on gfx::HDRMetadata. We will do the same with
the remaining members (adding accessors to mirror the Skia structures),
and then we will eventually replace the structure with skhdr::Metadata.
- M cc/paint/tone_map_util.cc
- M components/viz/service/display/display_resource_provider.cc
- M components/viz/service/display/skia_renderer.cc
- M media/base/agtm.cc
- M media/base/agtm.h
- M media/base/agtm_unittest.cc
- M media/filters/dav1d_video_decoder.cc
- M media/filters/dav1d_video_decoder_unittest.cc
- M media/filters/vpx_video_decoder.cc
- M media/filters/vpx_video_decoder_unittest.cc
- M media/gpu/av1_decoder.cc
- M media/gpu/av1_decoder_unittest.cc
- M third_party/blink/renderer/core/html/canvas/predefined_color_space.cc
- M ui/gfx/BUILD.gn
- M ui/gfx/hdr_metadata.cc
- M ui/gfx/hdr_metadata.h
- M ui/gfx/hdr_metadata_agtm.cc
- M ui/gfx/hdr_metadata_agtm.h
- 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/gfx/skia_span_util.cc
- M ui/gfx/skia_span_util.h
- M ui/ozone/platform/wayland/host/wayland_wp_color_manager.cc
Code-Review: +1 by Daniel Cheng, +1 by Dale Curtis
| 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")
Yes, if we decided it was problematic, Rust would be one way to solve this.
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |