media: Reporting different levels of Visibility of WebPlayer [chromium/src : main]
Vikram Pasupathy (Gerrit)
Vikram Pasupathy voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Sangbaek Park
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sangbaek Park (Gerrit)
Sangbaek Park added 8 comments![]( "Open in Gerrit")
Thanks for the great work! I finished my first review, PTAL.
Can we have a new test page with hidden video (some obvious case where V1 reports 0 or low ratio) so that we can add one more test case?
int intersection_ratio_final_v1_ = 0;Any reason to have inconsistent types `int` versus `int32_t` for input param in APIs? Is this because of mojo call interface?
int intersection_ratio_final_v1_ = 0;I mentioned this in another comment, to distiguish between no report and 0, can we initialize it to -1? And add a comment what -1 means?
({0.0f, 0.1f, 0.2f, 0.3f, 0.4f, 0.5f, 0.6f, 0.7f, 0.8f, 0.9f, 1.0f}));Do we still need this even though we are not monitoring the thresholds for MaxV1?
latest_intersection_ratio_v1_);`latest_intersection_ratio_v1_` may not be reported at all for some cases. Then we can't distingush between no report (unknown) and 0. Maybe consider initializing it to -1?
nit: Remove unnecessary whitespace changes. ditto for #1103
EME API call, as a percentage from 0 to 100. This usesCan we add what 0 means?
Related details
- Vikram Pasupathy
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. |
Vikram Pasupathy (Gerrit)
Vikram Pasupathy added 6 comments![]( "Open in Gerrit")
I mentioned this in another comment, to distiguish between no report and 0, can we initialize it to -1? And add a comment what -1 means?
Done
Any reason to have inconsistent types `int` versus `int32_t` for input param in APIs? Is this because of mojo call interface?
Yeah, the mojo call interface, but you're right, I will update everything to be `int32_t` for consistency.
({0.0f, 0.1f, 0.2f, 0.3f, 0.4f, 0.5f, 0.6f, 0.7f, 0.8f, 0.9f, 1.0f}));Do we still need this even though we are not monitoring the thresholds for MaxV1?
No, thanks so much.
`latest_intersection_ratio_v1_` may not be reported at all for some cases. Then we can't distingush between no report (unknown) and 0. Maybe consider initializing it to -1?
Done
nit: Remove unnecessary whitespace changes. ditto for #1103
Done
EME API call, as a percentage from 0 to 100. This usesCan we add what 0 means?
- Sangbaek Park
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. |
Sangbaek Park (Gerrit)
Sangbaek Park added 2 comments![]( "Open in Gerrit")
Please fix test failures and ClangTidy warnings (dependency issue?), thanks!
EME API call, as a percentage from 0 to 100. This usesVikram PasupathyCan we add what 0 means?
Done
- Vikram Pasupathy
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. |
Vikram Pasupathy (Gerrit)
Vikram Pasupathy added 3 comments![]( "Open in Gerrit")
Please fix test failures and ClangTidy warnings (dependency issue?), thanks!
Done (the clangtidy shows up on a lot of unrelated CLs, I think its a bug)
Vikram PasupathyCan we have a new test page with hidden video (some obvious case where V1 reports 0 or low ratio) so that we can add one more test case?
ptal, hidden video would report 100% (but maybe we want to add that in case we do explore the v2), but I added an offscreen eme playback.
Would you also want a case where it is hidden (via css, or the `hidden` tag), and that would have V1 report greater than 0 visibility?
EME API call, as a percentage from 0 to 100. This usesVikram PasupathyCan we add what 0 means?
Sangbaek ParkDone
Also add what -1 means here too?
- Sangbaek Park
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. |
Sangbaek Park (Gerrit)
Sangbaek Park added 1 comment![]( "Open in Gerrit")
Vikram PasupathyCan we have a new test page with hidden video (some obvious case where V1 reports 0 or low ratio) so that we can add one more test case?
ptal, hidden video would report 100% (but maybe we want to add that in case we do explore the v2), but I added an offscreen eme playback.
Would you also want a case where it is hidden (via css, or the `hidden` tag), and that would have V1 report greater than 0 visibility?
I see, V1 reports 100% with css/hidden. I agree with that it would be worth it if we have v2. Could you please explain what offscreen eme playback means?
Related details
- Vikram Pasupathy
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. |
Vikram Pasupathy (Gerrit)
Vikram Pasupathy added 1 comment![]( "Open in Gerrit")
Vikram PasupathyCan we have a new test page with hidden video (some obvious case where V1 reports 0 or low ratio) so that we can add one more test case?
Sangbaek Parkptal, hidden video would report 100% (but maybe we want to add that in case we do explore the v2), but I added an offscreen eme playback.
Would you also want a case where it is hidden (via css, or the `hidden` tag), and that would have V1 report greater than 0 visibility?
I see, V1 reports 100% with css/hidden. I agree with that it would be worth it if we have v2. Could you please explain what offscreen eme playback means?
By offscreen, I mean that it is moved to the left of the viewport by `position: absolute; left: -9999px;` so that the v1 detects that it is not in the screen at all during playback start.
I also refactored so that we don't create a new html file, and instead add it as a parameter within the existing html file to be offscreen or not.
I also added a new addition so that we test the hidden property, and verify that the v1 mechanism does not record it.
Related details
- Sangbaek Park
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. |
Sangbaek Park (Gerrit)
Sangbaek Park added 10 comments![]( "Open in Gerrit")
PTAL and fix the test bot failures (or retry), thanks!
Vikram PasupathyCan we have a new test page with hidden video (some obvious case where V1 reports 0 or low ratio) so that we can add one more test case?
Sangbaek Parkptal, hidden video would report 100% (but maybe we want to add that in case we do explore the v2), but I added an offscreen eme playback.
Would you also want a case where it is hidden (via css, or the `hidden` tag), and that would have V1 report greater than 0 visibility?
Vikram PasupathyI see, V1 reports 100% with css/hidden. I agree with that it would be worth it if we have v2. Could you please explain what offscreen eme playback means?
By offscreen, I mean that it is moved to the left of the viewport by `position: absolute; left: -9999px;` so that the v1 detects that it is not in the screen at all during playback start.
I also refactored so that we don't create a new html file, and instead add it as a parameter within the existing html file to be offscreen or not.
I also added a new addition so that we test the hidden property, and verify that the v1 mechanism does not record it.
Ack. Thanks!
IntersectionRatio) {nit: Can we add what is expected or what to test in this test method name? i.e., `IntersectionRatio_VisibleVideo`?
builder.SetIntersectionRatioFinalV1(intersection_ratio_final_v1_);
// We report it even in cases of non EME playback to distinguish
// between a non-visible web player, where this value would be 0,
// and a playback without any EME, where this value would be -1.
builder.SetIntersectionRatioAtFirstEmeApiCallV1(
intersection_ratio_at_first_eme_api_call_v1_);Sorry for this commment that could swipe the previous job (setting -1 by default).
After looking at other code for handdling no report case + some UKM fields are not reported if data is not avaialble, should we consider `std::optional<int32_t>` for both `intersection_ratio_final_v1_` and `intersection_ratio_at_first_eme_api_call_v1_` instead of using `-1`? We can remove all descriptions for `-1` here and there. And then we don't report those values if they are not available (=no report). In the UKM query, we still can distiguish bewteen the report case and no report case (if this field is NULL).
WDYT?
//media/test/data/vp9-agtm-country-code-extension.webmnit: Can we drop this change since no change other than the order?
int latest_intersection_ratio_v1_ = 0;Should this be `std::optional<int>` since we can't distinguish between 0 and not reported?
void HTMLVideoElement::OnSetMediaKeys() {Question: Would it be always the case where calling `OnSetMediaKeys()` occurs once? Or we just take the first call into consideration by design?
web_media_player_->SetIntersectionRatioAtFirstEmeApiCallV1(GetWebMediaPlayer()? ditto for others?
intersection_ratio_at_first_eme_api_call_ = latest_intersection_ratio_v1_;If the default value is just 0 and never reported then it will be set to 0 which doesn't seem right. Maybe we should make `latest_intersection_ratio_v1_` optional?
if (web_media_player_) {GetWebMediaPlayer()?
Related details
- Vikram Pasupathy
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. |
Vikram Pasupathy (Gerrit)
Vikram Pasupathy added 9 comments![]( "Open in Gerrit")
PTAL and fix the test bot failures (or retry), thanks!
Done, bot failures keep happening due to tree closures 😞, please ignore them for now
nit: Can we add what is expected or what to test in this test method name? i.e., `IntersectionRatio_VisibleVideo`?
Done
builder.SetIntersectionRatioFinalV1(intersection_ratio_final_v1_);
// We report it even in cases of non EME playback to distinguish
// between a non-visible web player, where this value would be 0,
// and a playback without any EME, where this value would be -1.
builder.SetIntersectionRatioAtFirstEmeApiCallV1(
intersection_ratio_at_first_eme_api_call_v1_);Sorry for this commment that could swipe the previous job (setting -1 by default).
After looking at other code for handdling no report case + some UKM fields are not reported if data is not avaialble, should we consider `std::optional<int32_t>` for both `intersection_ratio_final_v1_` and `intersection_ratio_at_first_eme_api_call_v1_` instead of using `-1`? We can remove all descriptions for `-1` here and there. And then we don't report those values if they are not available (=no report). In the UKM query, we still can distiguish bewteen the report case and no report case (if this field is NULL).
WDYT?
Sounds good to me.
//media/test/data/vp9-agtm-country-code-extension.webmnit: Can we drop this change since no change other than the order?
Done
Should this be `std::optional<int>` since we can't distinguish between 0 and not reported?
Done
void HTMLVideoElement::OnSetMediaKeys() {Question: Would it be always the case where calling `OnSetMediaKeys()` occurs once? Or we just take the first call into consideration by design?
The spec says the html media element can only be associated with a single MediaKeys object at a time, and replacing and clearing the existing MediaKeys object, especially during playback "is a quality of implementation issue" and can lead to a "bad user experience or rejected promise." (From the spec).
web_media_player_->SetIntersectionRatioAtFirstEmeApiCallV1(Vikram PasupathyGetWebMediaPlayer()? ditto for others?
SG. Seems like this file arbitrarily uses either. I will fix the other occurrences later that are not made in this PS.
intersection_ratio_at_first_eme_api_call_ = latest_intersection_ratio_v1_;If the default value is just 0 and never reported then it will be set to 0 which doesn't seem right. Maybe we should make `latest_intersection_ratio_v1_` optional?
Done
if (web_media_player_) {Vikram PasupathyGetWebMediaPlayer()?
Done
Related details
- Sangbaek Park
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. |
Sangbaek Park (Gerrit)
Sangbaek Park voted and added 3 comments![]( "Open in Gerrit")
Votes added by Sangbaek Park
| Code-Review | +1 |
3 comments
// TODO(crbug.com/40498529): The values can start being checked after
// setMediaKeys happens after play.nit: Move this comment between line #1950 and #1951?
void HTMLVideoElement::OnSetMediaKeys() {Vikram PasupathyQuestion: Would it be always the case where calling `OnSetMediaKeys()` occurs once? Or we just take the first call into consideration by design?
The spec says the html media element can only be associated with a single MediaKeys object at a time, and replacing and clearing the existing MediaKeys object, especially during playback "is a quality of implementation issue" and can lead to a "bad user experience or rejected promise." (From the spec).
- Vikram Pasupathy
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. |
Vikram Pasupathy (Gerrit)
Vikram Pasupathy added 1 comment![]( "Open in Gerrit")
// TODO(crbug.com/40498529): The values can start being checked after
// setMediaKeys happens after play.nit: Move this comment between line #1950 and #1951?
- Xiaohan Wang
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vikram Pasupathy (Gerrit)
Vikram Pasupathy added 1 comment![]( "Open in Gerrit")
- Dale Curtis
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Xiaohan Wang (Gerrit)
Xiaohan Wang added 1 comment![]( "Open in Gerrit")
To be clear, I provided comments in the design doc, mostly trying to make sure we have proper test coverage. I hope we can get the testing part sorted out before landing this.
Related details
- Dale Curtis
- Vikram Pasupathy
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Xiaohan Wang (Gerrit)
Xiaohan Wang added 1 comment![]( "Open in Gerrit")
Bug: 435526104use b:<bug> format for internal bugs
Related details
- Dale Curtis
- Vikram Pasupathy
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. |
Vikram Pasupathy (Gerrit)
Vikram Pasupathy added 1 comment![]( "Open in Gerrit")
use b:<bug> format for internal bugs
- Dale Curtis
- Xiaohan Wang
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 1 comment![]( "Open in Gerrit")
Do you need to know precise ratios to answer the questions posed in that doc? We have a dominant-detection mechanism that was recently added https://chromium-review.googlesource.com/c/chromium/src/+/6998039
Skimming the doc it seems like it would be sufficient to just record if the content ever became dominant or not.
Related details
- Vikram Pasupathy
- Xiaohan Wang
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. |
Vikram Pasupathy (Gerrit)
Vikram Pasupathy voted and added 1 comment![]( "Open in Gerrit")
Votes added by Vikram Pasupathy
| Commit-Queue | +1 |
Do you need to know precise ratios to answer the questions posed in that doc? We have a dominant-detection mechanism that was recently added https://chromium-review.googlesource.com/c/chromium/src/+/6998039
Skimming the doc it seems like it would be sufficient to just record if the content ever became dominant or not.
Refactored the CL to use the IO from the dominance detection mechanism. We will go forward with recording the ratio for granularity. The dominance detection is too high, at 85%, and we think that being able to set our own ratio is the way to go.
Related details
- Dale Curtis
- Sangbaek Park
- Vikram Pasupathy
- Xiaohan Wang
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 10 comments![]( "Open in Gerrit")
Vikram PasupathyDo you need to know precise ratios to answer the questions posed in that doc? We have a dominant-detection mechanism that was recently added https://chromium-review.googlesource.com/c/chromium/src/+/6998039
Skimming the doc it seems like it would be sufficient to just record if the content ever became dominant or not.
Refactored the CL to use the IO from the dominance detection mechanism. We will go forward with recording the ratio for granularity. The dominance detection is too high, at 85%, and we think that being able to set our own ratio is the way to go.
Acknowledged
SetIntersectionRatioFinalV1(int32 ratio_in_percent);V1 doesn't make much sense in this context. Are you planning a V2 or something later? Otherwise lets just call these SetFinalIntersectionRatio and SetInitialIntersectionRatio.
bool did_report_first_api_call_ratio_ = false;Seems unnecessary with optional fields?
friend class HTMLMediaElementEncryptedMedia;This doesn't seem like the right pattern for this use case. Why friend versus another pattern?
custom_controls_fullscreen_detector_->SetIntersectionUpdateCallback(Why use a callback setter if you're just going to poll the value? Just add an accessor on the detector?
if (GetWebMediaPlayer()) {`if (auto* wmp = GetWebMediaPlayer())`
if (GetWebMediaPlayer()) {`if (auto* wmp = GetWebMediaPlayer())`
if (GetWebMediaPlayer() && latest_intersection_ratio_percent_.has_value()) {`if (auto* wmp = GetWebMediaPlayer(); wmp && ...`
intersection_update_callback_ = std::move(callback);DCHECK it's not already set.
if (element.IsHTMLVideoElement()) {Don't run this until after l.406?
Related details
Sangbaek Park (Gerrit)
Sangbaek Park added 1 comment![]( "Open in Gerrit")
int ratio_in_percent =
static_cast<int>(entries.back()->intersectionRatio() * 100);nit: move into if-block
Related details
- Vikram Pasupathy
- Xiaohan Wang
Vikram Pasupathy (Gerrit)
Vikram Pasupathy added 10 comments![]( "Open in Gerrit")
V1 doesn't make much sense in this context. Are you planning a V2 or something later? Otherwise lets just call these SetFinalIntersectionRatio and SetInitialIntersectionRatio.
Yeah, planning on adding a V2 and hopefully working to make videos better in V2. (Or, repurposing other existing logic, but in any case, we want to make it clear the data is from the V1 observer).
Seems unnecessary with optional fields?
Removed.
This doesn't seem like the right pattern for this use case. Why friend versus another pattern?
Done
custom_controls_fullscreen_detector_->SetIntersectionUpdateCallback(Why use a callback setter if you're just going to poll the value? Just add an accessor on the detector?
Done
`if (auto* wmp = GetWebMediaPlayer())`
Done
`if (auto* wmp = GetWebMediaPlayer())`
Done
if (GetWebMediaPlayer() && latest_intersection_ratio_percent_.has_value()) {`if (auto* wmp = GetWebMediaPlayer(); wmp && ...`
Done
DCHECK it's not already set.
Done
int ratio_in_percent =
static_cast<int>(entries.back()->intersectionRatio() * 100);Vikram Pasupathynit: move into if-block
Changed the logic, so this no longer applies.
Don't run this until after l.406?
- Dale Curtis
- Sangbaek Park
- Xiaohan Wang
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 1 comment![]( "Open in Gerrit")
SetIntersectionRatioFinalV1(int32 ratio_in_percent);Vikram PasupathyV1 doesn't make much sense in this context. Are you planning a V2 or something later? Otherwise lets just call these SetFinalIntersectionRatio and SetInitialIntersectionRatio.
Yeah, planning on adding a V2 and hopefully working to make videos better in V2. (Or, repurposing other existing logic, but in any case, we want to make it clear the data is from the V1 observer).
Wouldn't that all happen behind the scenes though? I.e., we don't need the method names to have V1 in them, just the UKM entry?
I'd still recommend using `SetFinalIntersectionRatioV1` and `SetInitialIntersectionRatioV1` since the readability is better IMHO.
Related details
- Sangbaek Park
- Vikram Pasupathy
- Xiaohan Wang
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. |
Vikram Pasupathy (Gerrit)
Vikram Pasupathy added 1 comment![]( "Open in Gerrit")
SetIntersectionRatioFinalV1(int32 ratio_in_percent);Vikram PasupathyV1 doesn't make much sense in this context. Are you planning a V2 or something later? Otherwise lets just call these SetFinalIntersectionRatio and SetInitialIntersectionRatio.
Dale CurtisYeah, planning on adding a V2 and hopefully working to make videos better in V2. (Or, repurposing other existing logic, but in any case, we want to make it clear the data is from the V1 observer).
Wouldn't that all happen behind the scenes though? I.e., we don't need the method names to have V1 in them, just the UKM entry?
I'd still recommend using `SetFinalIntersectionRatioV1` and `SetInitialIntersectionRatioV1` since the readability is better IMHO.
Modified it to your suggestion, TY!
Do you think the UKM fields should also be modified?
Related details
- Dale Curtis
- Sangbaek Park
- Xiaohan Wang
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")
SetIntersectionRatioFinalV1(int32 ratio_in_percent);Vikram PasupathyV1 doesn't make much sense in this context. Are you planning a V2 or something later? Otherwise lets just call these SetFinalIntersectionRatio and SetInitialIntersectionRatio.
Dale CurtisYeah, planning on adding a V2 and hopefully working to make videos better in V2. (Or, repurposing other existing logic, but in any case, we want to make it clear the data is from the V1 observer).
Vikram PasupathyWouldn't that all happen behind the scenes though? I.e., we don't need the method names to have V1 in them, just the UKM entry?
I'd still recommend using `SetFinalIntersectionRatioV1` and `SetInitialIntersectionRatioV1` since the readability is better IMHO.
Modified it to your suggestion, TY!
Do you think the UKM fields should also be modified?
I'd keep them consistent. I'm still not clear on if you need the method names to contain V1 though?
Related details
- Sangbaek Park
- Vikram Pasupathy
- Xiaohan Wang
Vikram Pasupathy (Gerrit)
Vikram Pasupathy added 1 comment![]( "Open in Gerrit")
SetIntersectionRatioFinalV1(int32 ratio_in_percent);Vikram PasupathyV1 doesn't make much sense in this context. Are you planning a V2 or something later? Otherwise lets just call these SetFinalIntersectionRatio and SetInitialIntersectionRatio.
Dale CurtisYeah, planning on adding a V2 and hopefully working to make videos better in V2. (Or, repurposing other existing logic, but in any case, we want to make it clear the data is from the V1 observer).
Vikram PasupathyWouldn't that all happen behind the scenes though? I.e., we don't need the method names to have V1 in them, just the UKM entry?
I'd still recommend using `SetFinalIntersectionRatioV1` and `SetInitialIntersectionRatioV1` since the readability is better IMHO.
Dale CurtisModified it to your suggestion, TY!
Do you think the UKM fields should also be modified?
I'd keep them consistent. I'm still not clear on if you need the method names to contain V1 though?
`FinalIntersectionRatio` and `InitialEncryptedIntersectionRatio`, sorry, dropped the v1, the v2 observer doesn't report an intersection ratio.
Related details
- Dale Curtis
- Sangbaek Park
- Xiaohan Wang
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. |