Record various UMA metrics relevant to the MediaVideoVisibilityTracker [chromium/src : main]
Frank Liberato (Gerrit)
Frank Liberato added 3 comments![]( "Open in Gerrit")
const int kNumBuckets = 3;from our offline discussion: this will create "low / medium / high" buckets, which is likely not what you want.
RecordHitTestNodeAction(HitTestNodeAction::kIgnoreVideoElement);per offline conversation, this is going to count the number of occlusion computations, since there's exactly one video per computation.
RecordHitTestNodeAction(HitTestNodeAction::kIgnoreZeroEffectiveOpacity);per offline conversation, it might be better to sum up the number of these for the entire computation, similar to `total_hit_tested_nodes_`, else we'll just get a single total count of all zero opacity elements observed by all invocations of this algorithm by all clients.
in contrast, keeping the total here and recording that total once when the algorithm completes will show the distribution of # of zero opacity elements encountered per run of the algorithm by all clients -- we can see what percentage has 0, or 1, or 10 elements. won't tell us the total number of elements so maybe percent would be better / in addition to, but not sure.
Related details
- Benjamin Keen
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Benjamin Keen (Gerrit)
Benjamin Keen voted and added 3 comments![]( "Open in Gerrit")
Votes added by Benjamin Keen
| Commit-Queue | +1 |
3 comments
from our offline discussion: this will create "low / medium / high" buckets, which is likely not what you want.
Thanks for pointing this out.
After some investigation, there is a way to get fine grained info by using [linear histograms]. With the caveat that the `exclusive_max` is capped at `101`.
Chatting with Evan, seems like the best solution here is to record the lower values with a linear histogram, and in addition use a count histogram. That way we can have the fine granularity for the smaller values + overall distribution.
[linear histograms]: https://source.chromium.org/chromium/chromium/src/+/main:base/metrics/histogram_macros.h;l=127;drc=a0782e8a696dc65f1bf525c5ded9df265eb3cd36
RecordHitTestNodeAction(HitTestNodeAction::kIgnoreVideoElement);per offline conversation, this is going to count the number of occlusion computations, since there's exactly one video per computation.
Ackd, just removed this.
RecordHitTestNodeAction(HitTestNodeAction::kIgnoreZeroEffectiveOpacity);per offline conversation, it might be better to sum up the number of these for the entire computation, similar to `total_hit_tested_nodes_`, else we'll just get a single total count of all zero opacity elements observed by all invocations of this algorithm by all clients.
in contrast, keeping the total here and recording that total once when the algorithm completes will show the distribution of # of zero opacity elements encountered per run of the algorithm by all clients -- we can see what percentage has 0, or 1, or 10 elements. won't tell us the total number of elements so maybe percent would be better / in addition to, but not sure.
Updated the metrics to be similar to `total_hit_tested_nodes_`. That way we can tell the counts/percentages per algorithm run (complete meets visibility check) for all clients.
Related details
- Frank Liberato
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Frank Liberato (Gerrit)
Frank Liberato added 2 comments![]( "Open in Gerrit")
int total_hit_tested_nodes_ = 0;this is works, but it depends on resetting these each time and not forgetting. i think it'd be less fragile if they weren't members at all.
for example, consider creating a nested struct:
```
struct Metrics {
int total_hit_tested... = 0;
/// and so on
};
```
then you can create an instance on the stack in `OnIntersectionChanged`, and pass it to the bound function and the reporting function. that will make sure it's always initialized to zero at the start. it's also a little clearer in that it doesn't look like long-running state anymore; it shows up, is used, and is destroyed in the scope of one function.
from the .h file: this is where you'd have a local struct with the counters, and bind it @336 and send it as an argument to @340. if i understand correctly, then it's only within this scope that it needs to exist.
Related details
- Benjamin Keen
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Benjamin Keen (Gerrit)
Benjamin Keen added 1 comment![]( "Open in Gerrit")
from the .h file: this is where you'd have a local struct with the counters, and bind it @336 and send it as an argument to @340. if i understand correctly, then it's only within this scope that it needs to exist.
Doing this would mean changing the `ComputeOcclusion` signature. At the moment it just expects a `const Node& node` [1]. This is why I have the counters as member variables, unless I am misunderstanding your suggestion?
Related details
- Frank Liberato
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Frank Liberato (Gerrit)
Frank Liberato added 1 comment![]( "Open in Gerrit")
Benjamin Keenfrom the .h file: this is where you'd have a local struct with the counters, and bind it @336 and send it as an argument to @340. if i understand correctly, then it's only within this scope that it needs to exist.
Doing this would mean changing the `ComputeOcclusion` signature. At the moment it just expects a `const Node& node` [1]. This is why I have the counters as member variables, unless I am misunderstanding your suggestion?
it would only change the signature of the method in this class to include an extra argument, but the resulting BindRepeating signature would not change since you'd also bind an extra argument. i.e., it's just a local change to this class, and the hit tester would still send in a `const Node&`.
Related details
- Benjamin Keen
Code-Coverage
Code-Review
No-Unresolved-Comments
| 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")
this is works, but it depends on resetting these each time and not forgetting. i think it'd be less fragile if they weren't members at all.
for example, consider creating a nested struct:
```
struct Metrics {
int total_hit_tested... = 0;
/// and so on
};
```then you can create an instance on the stack in `OnIntersectionChanged`, and pass it to the bound function and the reporting function. that will make sure it's always initialized to zero at the start. it's also a little clearer in that it doesn't look like long-running state anymore; it shows up, is used, and is destroyed in the scope of one function.
Done
Benjamin Keenfrom the .h file: this is where you'd have a local struct with the counters, and bind it @336 and send it as an argument to @340. if i understand correctly, then it's only within this scope that it needs to exist.
Frank LiberatoDoing this would mean changing the `ComputeOcclusion` signature. At the moment it just expects a `const Node& node` [1]. This is why I have the counters as member variables, unless I am misunderstanding your suggestion?
it would only change the signature of the method in this class to include an extra argument, but the resulting BindRepeating signature would not change since you'd also bind an extra argument. i.e., it's just a local change to this class, and the hit tester would still send in a `const Node&`.
Here I thought I was starting to understand callbacks :).
That makes sense and this is much cleaner. Thanks for the suggestion!
Related details
- Frank Liberato
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Frank Liberato (Gerrit)
Frank Liberato added 1 comment![]( "Open in Gerrit")
struct Metrics;i think this would be better off as a nested struct inside MediaVideoVisibilityTracker, rather than an struct in an anonymous namespace. the reason is that everything that includes this file will also get a forward declaration of a `Metrics` struct in an anonymous namespace, which isn't great. if they tried to declare a `Metrics` (e.g.) class or anything else in their anonymous namespace, then i think it'd conflict.
putting it inside the main class hides it, since we're already forced to assume that MVVT is uniquely named. putting the Metrics struct inside MVVT is pretty much the same as putting it in the MVVT namespace, at least conceptually. so, it won't conflict with anything there.
Related details
- Benjamin Keen
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Benjamin Keen (Gerrit)
Benjamin Keen added 1 comment![]( "Open in Gerrit")
i think this would be better off as a nested struct inside MediaVideoVisibilityTracker, rather than an struct in an anonymous namespace. the reason is that everything that includes this file will also get a forward declaration of a `Metrics` struct in an anonymous namespace, which isn't great. if they tried to declare a `Metrics` (e.g.) class or anything else in their anonymous namespace, then i think it'd conflict.
putting it inside the main class hides it, since we're already forced to assume that MVVT is uniquely named. putting the Metrics struct inside MVVT is pretty much the same as putting it in the MVVT namespace, at least conceptually. so, it won't conflict with anything there.
- Frank Liberato
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Frank Liberato (Gerrit)
Frank Liberato voted Code-Review+1![]( "Open in Gerrit")
- Benjamin Keen
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Benjamin Keen (Gerrit)
Benjamin Keen added 1 comment![]( "Open in Gerrit")
Adding:
evliu@ for: t/m/h/m/m/histograms.xml and
dalecurtis@ for: t/b/r/c/h/m/media_video_visibility_tracker.h|cc
Thanks!
Related details
- Dale Curtis
- Evan Liu
Code-Coverage
Code-Review
| 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")
- Benjamin Keen
- Evan Liu
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Evan Liu (Gerrit)
Evan Liu voted and added 2 comments![]( "Open in Gerrit")
Votes added by Evan Liu
| Code-Review | +1 |
2 comments
Oops sorry I missed this!
<histogramoptional nit: you could use patterned histograms to avoid having to define so many different histograms
Related details
- Benjamin Keen
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Benjamin Keen (Gerrit)
Benjamin Keen added 1 comment![]( "Open in Gerrit")
optional nit: you could use patterned histograms to avoid having to define so many different histograms
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Benjamin Keen (Gerrit)
Benjamin Keen voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
6 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: tools/metrics/histograms/metadata/media/histograms.xml
Insertions: 48, Deletions: 146.
The diff is too large to show. Please review the diff.
```
```
The name of the file: third_party/blink/renderer/core/html/media/media_video_visibility_tracker.cc
Insertions: 10, Deletions: 5.
The diff is too large to show. Please review the diff.
```
```
The name of the file: chrome/browser/picture_in_picture/auto_picture_in_picture_tab_helper_browsertest.cc
Insertions: 13, Deletions: 9.
The diff is too large to show. Please review the diff.
```
Change information
Record various UMA metrics relevant to the MediaVideoVisibilityTracker
The new metrics will help track information regarding:
* How much time is spent computing occluding area
* Various counts/percentages
All these metrics will help us identify bottlenecks, prioritize
performance improvements, and overall better understand how the
MediaVideoVisibilityTracker perform/behaves on the Web.
- M chrome/browser/picture_in_picture/auto_picture_in_picture_tab_helper_browsertest.cc
- M third_party/blink/renderer/core/html/media/media_video_visibility_tracker.cc
- M third_party/blink/renderer/core/html/media/media_video_visibility_tracker.h
- M tools/metrics/histograms/metadata/media/histograms.xml
Code-Review: +1 by Dale Curtis, +1 by Frank Liberato, +1 by Evan Liu
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |