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.
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Commit-Queue | +1 |
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.
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
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.
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
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?
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
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&`.
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
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!
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
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.
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
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.
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
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!
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Code-Review | +1 |
Oops sorry I missed this!
<histogram
optional nit: you could use patterned histograms to avoid having to define so many different histograms
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
optional nit: you could use patterned histograms to avoid having to define so many different histograms
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
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.
```
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.
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |