media: Add HDR watch time histograms (no UKM) [chromium/src : main]
AI Code Reviewer (Gerrit)
AI Code Reviewer added 1 comment![]( "Open in Gerrit")
WebMediaPlayer::DisplayType display_type);To improve readability at call sites, please consider using a `base::StrongAlias` for the `is_eme` parameter, as this function is called with boolean literals. For example, you could define `using IsEme = base::StrongAlias<class IsEmeTag, bool>;` and change the function signature accordingly. (Blink Style Guide: Prefer enums or StrongAliases to bare bools for function parameters)
_To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **Won't fix**: reason | **b/<bug_id>** | **Invalid:** reason
This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent).AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve.[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
Related details
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sangbaek Park (Gerrit)
Sangbaek Park added 1 comment![]( "Open in Gerrit")
To improve readability at call sites, please consider using a `base::StrongAlias` for the `is_eme` parameter, as this function is called with boolean literals. For example, you could define `using IsEme = base::StrongAlias<class IsEmeTag, bool>;` and change the function signature accordingly. (Blink Style Guide: Prefer enums or StrongAliases to bare bools for function parameters)
_To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **Won't fix**: reason | **b/<bug_id>** | **Invalid:** reasonThis comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent).AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve.[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
Won't fix: It's an internal method.
Related details
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sangbaek Park (Gerrit)
Sangbaek Park added 1 comment![]( "Open in Gerrit")
@dalec...@chromium.org This CL is about reporting HDR watch times (UMA only). PTAL, thank you!
Related details
- Dale Curtis
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dale Curtis (Gerrit)
Dale Curtis added 4 comments![]( "Open in Gerrit")
bool VideoColorSpace::IsHdr() const {Lets match gfx::ColorSpace here. `IsHDR`
constexpr base::TimeDelta kWatchTime = base::Seconds(25);This set of actions doesn't reflect real usage. Can we rework into a couple tests instead?
// Updates whether HDR is enabled for the video.I don't think this is the right spot, color space is a secondary property, so it should be in the properties structure above. It only changes at the same time as the codec so they should be grouped together.
if (codec_change || codec_profile_change)Now this should be `|| hdr_changed`
Related details
- Sangbaek Park
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sangbaek Park (Gerrit)
Sangbaek Park voted and added 1 comment![]( "Open in Gerrit")
Votes added by Sangbaek Park
| Commit-Queue | +1 |
1 comment
// Updates whether HDR is enabled for the video.I don't think this is the right spot, color space is a secondary property, so it should be in the properties structure above. It only changes at the same time as the codec so they should be grouped together.
I have another prototype with adding is_hdr boolean flag into `SecondaryPlaybackProperties`. Sounds like that approach is the right way?
Related details
- Dale Curtis
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| 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")
// Updates whether HDR is enabled for the video.Sangbaek ParkI don't think this is the right spot, color space is a secondary property, so it should be in the properties structure above. It only changes at the same time as the codec so they should be grouped together.
I have another prototype with adding is_hdr boolean flag into `SecondaryPlaybackProperties`. Sounds like that approach is the right way?
- Sangbaek Park
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sangbaek Park (Gerrit)
Sangbaek Park added 1 comment![]( "Open in Gerrit")
bool VideoColorSpace::IsHdr() const {Lets match gfx::ColorSpace here. `IsHDR`
This is gfx::ColorSpace::IsHDR's impl:
```
bool ColorSpace::IsHDR() const {
return transfer_ == TransferID::PQ || transfer_ == TransferID::HLG ||
transfer_ == TransferID::LINEAR_HDR ||
transfer_ == TransferID::SRGB_HDR ||
transfer_ == TransferID::CUSTOM_HDR ||
transfer_ == TransferID::SCRGB_LINEAR_80_NITS;
}
```
And we conver from gfx::ColorSpace in [VideoColorSpace::FromGfxColorSpace()](https://source.chromium.org/chromium/chromium/src/+/main:media/base/video_color_space.cc;drc=a1e73d6ea7d3340382cf5539c54a55cc784e9a78;l=99). We ignore all other TransferID types (LINEAR_HDR,SRGB_HDR,CUSTOM_HDR,SCRGB_LINEAR_80_NITS) but PG & HLG as below:
```
case gfx::ColorSpace::TransferID::PQ:
transfer = VideoColorSpace::TransferID::SMPTEST2084;
break;
case gfx::ColorSpace::TransferID::HLG:
transfer = VideoColorSpace::TransferID::ARIB_STD_B67;
break;
```
So the current code is matched. But, do you think we need to update `VideoColorSpace::FromGfxColorSpace()`?
Related details
- Dale Curtis
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| 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")
bool VideoColorSpace::IsHdr() const {Sangbaek ParkLets match gfx::ColorSpace here. `IsHDR`
This is gfx::ColorSpace::IsHDR's impl:
```
bool ColorSpace::IsHDR() const {
return transfer_ == TransferID::PQ || transfer_ == TransferID::HLG ||
transfer_ == TransferID::LINEAR_HDR ||
transfer_ == TransferID::SRGB_HDR ||
transfer_ == TransferID::CUSTOM_HDR ||
transfer_ == TransferID::SCRGB_LINEAR_80_NITS;
}
```
And we conver from gfx::ColorSpace in [VideoColorSpace::FromGfxColorSpace()](https://source.chromium.org/chromium/chromium/src/+/main:media/base/video_color_space.cc;drc=a1e73d6ea7d3340382cf5539c54a55cc784e9a78;l=99). We ignore all other TransferID types (LINEAR_HDR,SRGB_HDR,CUSTOM_HDR,SCRGB_LINEAR_80_NITS) but PG & HLG as below:
```
case gfx::ColorSpace::TransferID::PQ:
transfer = VideoColorSpace::TransferID::SMPTEST2084;
break;
case gfx::ColorSpace::TransferID::HLG:
transfer = VideoColorSpace::TransferID::ARIB_STD_B67;
break;
```So the current code is matched. But, do you think we need to update `VideoColorSpace::FromGfxColorSpace()`?
I just meant the spelling of the method name should be `HDR` instead of `Hdr`. Technically we don't need a new method since you can just use `ToGfxColorSpace().IsHDR()`
Related details
- Sangbaek Park
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sangbaek Park (Gerrit)
Sangbaek Park added 4 comments![]( "Open in Gerrit")
Sangbaek ParkLets match gfx::ColorSpace here. `IsHDR`
Dale CurtisThis is gfx::ColorSpace::IsHDR's impl:
```
bool ColorSpace::IsHDR() const {
return transfer_ == TransferID::PQ || transfer_ == TransferID::HLG ||
transfer_ == TransferID::LINEAR_HDR ||
transfer_ == TransferID::SRGB_HDR ||
transfer_ == TransferID::CUSTOM_HDR ||
transfer_ == TransferID::SCRGB_LINEAR_80_NITS;
}
```
And we conver from gfx::ColorSpace in [VideoColorSpace::FromGfxColorSpace()](https://source.chromium.org/chromium/chromium/src/+/main:media/base/video_color_space.cc;drc=a1e73d6ea7d3340382cf5539c54a55cc784e9a78;l=99). We ignore all other TransferID types (LINEAR_HDR,SRGB_HDR,CUSTOM_HDR,SCRGB_LINEAR_80_NITS) but PG & HLG as below:
```
case gfx::ColorSpace::TransferID::PQ:
transfer = VideoColorSpace::TransferID::SMPTEST2084;
break;
case gfx::ColorSpace::TransferID::HLG:
transfer = VideoColorSpace::TransferID::ARIB_STD_B67;
break;
```So the current code is matched. But, do you think we need to update `VideoColorSpace::FromGfxColorSpace()`?
I just meant the spelling of the method name should be `HDR` instead of `Hdr`. Technically we don't need a new method since you can just use `ToGfxColorSpace().IsHDR()`
Oh, I see. Done
constexpr base::TimeDelta kWatchTime = base::Seconds(25);This set of actions doesn't reflect real usage. Can we rework into a couple tests instead?
Done
Sangbaek ParkI don't think this is the right spot, color space is a secondary property, so it should be in the properties structure above. It only changes at the same time as the codec so they should be grouped together.
Dale CurtisI have another prototype with adding is_hdr boolean flag into `SecondaryPlaybackProperties`. Sounds like that approach is the right way?
Yup!
Done. Thanks for the suggestion!
Now this should be `|| hdr_changed`
- Chromium IPC Reviews
- Dale Curtis
- Evan Liu
- Tomas Gunnarsson
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
From googleclient/chrome/chromium_gwsq/ipc/config.gwsq:
IPC: na...@chromium.org
📎 It looks like you’re making a possibly security-sensitive change! 📎 IPC security review isn’t a rubberstamp, so your friendly security reviewer will need a fair amount of context to review your CL effectively. Please review your CL description and code comments to make sure they provide context for someone unfamiliar with your project/area. Pay special attention to where data comes from and which processes it flows between (and their privilege levels). Feel free to point your security reviewer at design docs, bugs, or other links if you can’t reasonably make a self-contained CL description. (Also see https://cbea.ms/git-commit/).
IPC reviewer(s): na...@chromium.org
Reviewer source(s):
na...@chromium.org is from context(googleclient/chrome/chromium_gwsq/ipc/config.gwsq)
Related details
- Dale Curtis
- Evan Liu
- Nasko Oskov
- Tomas Gunnarsson
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Evan Liu (Gerrit)
Evan Liu voted Code-Review+1![]( "Open in Gerrit")
- Dale Curtis
- Nasko Oskov
- Sangbaek Park
- Tomas Gunnarsson
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |