media: Implement VideoVisibility static tracking [chromium/src : main]
Vikram Pasupathy (Gerrit)
New activity on the change![]( "Open in Gerrit")
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 12 comments![]( "Open in Gerrit")
// Builders sometime do not display the whole window viewport, so verify some
// of the viewport is detected as visible.
EXPECT_GT(playback_start_it->second, 50);50 sounds still flaky in the future. `EXPECT_GT(playback_start_it->second, 0);` might be better? WDYT?
If you take a param name (hidden, occluded, partial_occluded) to be "true" and the expected ratio param into each test, you can remove duplicate codes.
query_params.emplace_back("occluded", "true");I don't see the test for `partial_occluded`?
if (urlParams.get('opacity') === '0') { // New blockWhat does this mean?
// Ensure that the |visibility_tracker_| is detached when |this| is moved to
// a new document. Calling |ElementDidMoveToNewDocument| on the tracker at
// this point prevents having the tracker attached to an old document. The
// subsequent call to |UpdateVideoVisibilityTracker| will re-attach
// the tracker to the new document if needed.Why did this get removed?
ListBasedHitTestBehavior ComputeOcclusion(const ClientIdsSet& client_ids_set,Why did you move this method? Just regrouping? Or unnecessary change.
DCHECK(report_visibility_cb_);Removed this because now `report_visibility_cb_` can be null? Because no continous report is needed?
if (request_visibility_ratio_callback_) {
std::move(request_visibility_ratio_callback_).Run(0.0);
}Why do you call the callback with 0.0 here unnecessarily? If you intended to dispose it. `request_visibility_ratio_callback_ = nullptr;` should be enough?
auto intersection_area = ComputeArea(occlusion_state_.intersection_rect);Is this always zero rect since you did `occlusion_state_ = {};`?
auto intersection_area = ComputeArea(occlusion_state_.intersection_rect);Can you please clarify what `intersection_area` represents? Occlusion area is obvious to me but intersection area is not (intersection with what?).
// Detach if no continuous reporting is needed.This behavior is different from the previous behavior. Is this intentional? Because this MediaVideoVisibilityTracker has been there but the change will affect the existing behaviors.
You're changing the existing behavior not adding one more observer to report once for encrypted video visibility check.
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 12 comments![]( "Open in Gerrit")
// Builders sometime do not display the whole window viewport, so verify some
// of the viewport is detected as visible.
EXPECT_GT(playback_start_it->second, 50);50 sounds still flaky in the future. `EXPECT_GT(playback_start_it->second, 0);` might be better? WDYT?
Makes sense. Updated.
If you take a param name (hidden, occluded, partial_occluded) to be "true" and the expected ratio param into each test, you can remove duplicate codes.
Done
I don't see the test for `partial_occluded`?
Done
if (urlParams.get('opacity') === '0') { // New blockVikram PasupathyWhat does this mean?
Removed.
// Ensure that the |visibility_tracker_| is detached when |this| is moved to
// a new document. Calling |ElementDidMoveToNewDocument| on the tracker at
// this point prevents having the tracker attached to an old document. The
// subsequent call to |UpdateVideoVisibilityTracker| will re-attach
// the tracker to the new document if needed.Why did this get removed?
Done
}Vikram Pasupathynit: Unnecessary change
Its the result of git cl format
ListBasedHitTestBehavior ComputeOcclusion(const ClientIdsSet& client_ids_set,Why did you move this method? Just regrouping? Or unnecessary change.
Unnecessary, moved back down.
DCHECK(report_visibility_cb_);Removed this because now `report_visibility_cb_` can be null? Because no continous report is needed?
Yeah, it can be null because in `HTMLVideoElement::CreateVisibilityTrackerIfNeeded`, we sometimes don't pass a callback because continuous report is not needed.
if (request_visibility_ratio_callback_) {
std::move(request_visibility_ratio_callback_).Run(0.0);
}Why do you call the callback with 0.0 here unnecessarily? If you intended to dispose it. `request_visibility_ratio_callback_ = nullptr;` should be enough?
I will just drop the old callback and replace it with the new one.
auto intersection_area = ComputeArea(occlusion_state_.intersection_rect);Is this always zero rect since you did `occlusion_state_ = {};`?
`ComputeAreaOccludedByViewport` assigns values to `total_area` and `intersection_area`
auto intersection_area = ComputeArea(occlusion_state_.intersection_rect);Can you please clarify what `intersection_area` represents? Occlusion area is obvious to me but intersection area is not (intersection with what?).
Replaced with `total_area` and `intersection_area`, and added comments below in `ComputeAreaOccludedByViewport` for what they represent.
// Detach if no continuous reporting is needed.This behavior is different from the previous behavior. Is this intentional? Because this MediaVideoVisibilityTracker has been there but the change will affect the existing behaviors.
You're changing the existing behavior not adding one more observer to report once for encrypted video visibility check.
Refactored to keep most of it similar, and then added a comment on why we do the `base::TimeTicks::Now() - last_hit_test_timestamp_ >= hit_test_interval_` check.
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 3 comments![]( "Open in Gerrit")
auto intersection_area = ComputeArea(occlusion_state_.intersection_rect);Vikram PasupathyIs this always zero rect since you did `occlusion_state_ = {};`?
`ComputeAreaOccludedByViewport` assigns values to `total_area` and `intersection_area`
I see but this is so confusing since you reset `occlusion_state_` and `ComputeAreaOccludedByViewport()` updates the `occlusion_state_` internally.
Why don't you move `occlusion_state_ = {};` into `ComputeVisibility()`?
auto intersection_area = ComputeArea(occlusion_state_.intersection_rect);Vikram PasupathyCan you please clarify what `intersection_area` represents? Occlusion area is obvious to me but intersection area is not (intersection with what?).
Replaced with `total_area` and `intersection_area`, and added comments below in `ComputeAreaOccludedByViewport` for what they represent.
Acknowledged
return;Now the line #837-838 are not being hit due to this `return;`. Is it intended?
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 3 comments![]( "Open in Gerrit")
auto intersection_area = ComputeArea(occlusion_state_.intersection_rect);Vikram PasupathyIs this always zero rect since you did `occlusion_state_ = {};`?
Sangbaek Park`ComputeAreaOccludedByViewport` assigns values to `total_area` and `intersection_area`
I see but this is so confusing since you reset `occlusion_state_` and `ComputeAreaOccludedByViewport()` updates the `occlusion_state_` internally.
Why don't you move `occlusion_state_ = {};` into `ComputeVisibility()`?
Both use ComputeAreaOccludedByViewport, so moved it there.
// Detach if no continuous reporting is needed.Vikram PasupathyThis behavior is different from the previous behavior. Is this intentional? Because this MediaVideoVisibilityTracker has been there but the change will affect the existing behaviors.
You're changing the existing behavior not adding one more observer to report once for encrypted video visibility check.
Refactored to keep most of it similar, and then added a comment on why we do the `base::TimeTicks::Now() - last_hit_test_timestamp_ >= hit_test_interval_` check.
Done
Now the line #837-838 are not being hit due to this `return;`. Is it intended?
- 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 2 comments![]( "Open in Gerrit")
Votes added by Sangbaek Park
| Code-Review | +1 |
2 comments
auto intersection_area = ComputeArea(occlusion_state_.intersection_rect);Vikram PasupathyIs this always zero rect since you did `occlusion_state_ = {};`?
Sangbaek Park`ComputeAreaOccludedByViewport` assigns values to `total_area` and `intersection_area`
Vikram PasupathyI see but this is so confusing since you reset `occlusion_state_` and `ComputeAreaOccludedByViewport()` updates the `occlusion_state_` internally.
Why don't you move `occlusion_state_ = {};` into `ComputeVisibility()`?
Both use ComputeAreaOccludedByViewport, so moved it there.
- Vikram Pasupathy
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. |
Dale Curtis (Gerrit)
Dale Curtis added 1 comment![]( "Open in Gerrit")
=>bkeen to do a pass first since he wrote a lot of this code.
Related details
- Benjamin Keen
- Vikram Pasupathy
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")
FWIW, in the future I'd recommend splitting up a change like this. E.g., the visibility tracker changes seem like they'd more easily reviewed in a smaller more scoped change w/ tests.
Related details
- Benjamin Keen
- 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. |
Benjamin Keen (Gerrit)
Benjamin Keen added 2 comments![]( "Open in Gerrit")
void HTMLVideoElement::CreateVisibilityTrackerIfNeeded(bool force_create) {I think this could be confusing, specially since what you really want is to configure based on the features. How about something like:
```
const bool autopip_video_heuristics_enabled =
RuntimeEnabledFeatures::AutoPictureInPictureVideoHeuristicsEnabled();
const bool encryption_media_occlussion_tracking_enabled =
base::FeatureList::IsEnabled(media::kEncryptedMediaOcclusionTracking);
// Exit if neither feature needs the tracker.
if (!autopip_video_heuristics_enabled && !encryption_media_occlussion_tracking_enabled) {
return;
}
if (autopip_video_heuristics_enabled) {
...
}
``` last_visibility_ratio_ = ComputeVisibilityRatio();Here you are missing some important checks that happen in `MaybeComputeVisibility`. For example: If the tracker is attached, checking we are in the outer main frame, etc.
Additionally, we shouldn't access the paint artifact (`GetPaintArtifact`) if the document is not in the paint clean [state](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/frame/local_frame_view.cc;l=4180;drc=6b7247341b28f7d52babcbbde257523c43b019e5).
Related details
- Vikram Pasupathy
Vikram Pasupathy (Gerrit)
Vikram Pasupathy added 2 comments![]( "Open in Gerrit")
void HTMLVideoElement::CreateVisibilityTrackerIfNeeded(bool force_create) {I think this could be confusing, specially since what you really want is to configure based on the features. How about something like:
```
const bool autopip_video_heuristics_enabled =
RuntimeEnabledFeatures::AutoPictureInPictureVideoHeuristicsEnabled();
const bool encryption_media_occlussion_tracking_enabled =
base::FeatureList::IsEnabled(media::kEncryptedMediaOcclusionTracking);// Exit if neither feature needs the tracker.
if (!autopip_video_heuristics_enabled && !encryption_media_occlussion_tracking_enabled) {
return;
}if (autopip_video_heuristics_enabled) {
...
}
```
Thanks. Done!
last_visibility_ratio_ = ComputeVisibilityRatio();Here you are missing some important checks that happen in `MaybeComputeVisibility`. For example: If the tracker is attached, checking we are in the outer main frame, etc.
Additionally, we shouldn't access the paint artifact (`GetPaintArtifact`) if the document is not in the paint clean [state](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/frame/local_frame_view.cc;l=4180;drc=6b7247341b28f7d52babcbbde257523c43b019e5).
The `HasValidFrameAndLayout() && IsPaintClean()` may also go above in line 810, but I don't think that makes sense, because the `HasValidFrameAndLayout() && IsPaintClean()` checks are only for `ComputeVisibility()`.
Just commenting this, to make sure that it is the intended behavior, and for you to keep me honest. Thanks!
Related details
- Benjamin Keen
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. |
Benjamin Keen (Gerrit)
Benjamin Keen added 3 comments![]( "Open in Gerrit")
void MediaVideoVisibilityTracker::RequestVisibilityRatio(I'm not familiar with the encryption code path. But, can you get multiple requests for the visibility ratio, and if so, would that be a problem? You'd only get the notification for the latest request.
if (ratio_requested_ && HasValidFrameAndLayout() && IsPaintClean()) {Optional: Please move this (lines 814-827) into a dedicated method (`MaybeComputeVisibilityRatio`?). And for the header comment mention that this will detach the tracker.
last_visibility_ratio_ = ComputeVisibilityRatio();Vikram PasupathyHere you are missing some important checks that happen in `MaybeComputeVisibility`. For example: If the tracker is attached, checking we are in the outer main frame, etc.
Additionally, we shouldn't access the paint artifact (`GetPaintArtifact`) if the document is not in the paint clean [state](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/frame/local_frame_view.cc;l=4180;drc=6b7247341b28f7d52babcbbde257523c43b019e5).
The `HasValidFrameAndLayout() && IsPaintClean()` may also go above in line 810, but I don't think that makes sense, because the `HasValidFrameAndLayout() && IsPaintClean()` checks are only for `ComputeVisibility()`.
Just commenting this, to make sure that it is the intended behavior, and for you to keep me honest. Thanks!
This looks good, thanks!
Yeah, moving the check to line 810 wouldn't make sense, since it would mean we could drop pending visibility requests.
Related details
- Vikram Pasupathy
Benjamin Keen (Gerrit)
Benjamin Keen voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Dale Curtis (Gerrit)
Dale Curtis added 2 comments![]( "Open in Gerrit")
Did you already get UKM approval for this change? Should be trivial, but is required IIRC.
void EncryptedMediaInitDataEncountered() final;`OnEncryptedMediaInitData()` naming would be more consistent?
Vikram Pasupathy (Gerrit)
Vikram Pasupathy added 5 comments![]( "Open in Gerrit")
FWIW, in the future I'd recommend splitting up a change like this. E.g., the visibility tracker changes seem like they'd more easily reviewed in a smaller more scoped change w/ tests.
Acknowledged
Did you already get UKM approval for this change? Should be trivial, but is required IIRC.
Added corresponding UKM approval bug.
`OnEncryptedMediaInitData()` naming would be more consistent?
Done
void MediaVideoVisibilityTracker::RequestVisibilityRatio(I'm not familiar with the encryption code path. But, can you get multiple requests for the visibility ratio, and if so, would that be a problem? You'd only get the notification for the latest request.
Currently, we are only requesting once, at playback start. But I was requesting it multiple times with this previously, and it was working as expected. What would be the issue with this?
if (ratio_requested_ && HasValidFrameAndLayout() && IsPaintClean()) {Optional: Please move this (lines 814-827) into a dedicated method (`MaybeComputeVisibilityRatio`?). And for the header comment mention that this will detach the tracker.
- Benjamin Keen
- Dale Curtis
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. |
Benjamin Keen (Gerrit)
Benjamin Keen voted and added 1 comment![]( "Open in Gerrit")
Votes added by Benjamin Keen
| Code-Review | +1 |
1 comment
void MediaVideoVisibilityTracker::RequestVisibilityRatio(Vikram PasupathyI'm not familiar with the encryption code path. But, can you get multiple requests for the visibility ratio, and if so, would that be a problem? You'd only get the notification for the latest request.
Currently, we are only requesting once, at playback start. But I was requesting it multiple times with this previously, and it was working as expected. What would be the issue with this?
No issues at all, as long as you only need the latest visibility ratio.
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. |
Dale Curtis (Gerrit)
Dale Curtis voted Code-Review+1![]( "Open in Gerrit")
- Vikram Pasupathy