Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2160963002:
Add watch time metrics for HTML5 media playback. (Closed)
|
Created:
4 years, 5 months ago by DaleCurtis Modified:
4 years, 4 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org, mlamouri (slow - plz ping) Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd watch time metrics for HTML5 media playback.
Watch time for our purposes is defined as the amount of elapsed media time
for audio+video media. A minimum of 7 seconds of unmuted, foreground media
must be watched to start watch time monitoring. Watch time is checked every
5 seconds from then on and reported to multiple buckets: All, MSE, SRC, EME,
AC, and battery.
Any one of paused, hidden, or muted is sufficient to stop watch time metric
reports. Each of these has a hysteresis where if the state change is undone
within 5 seconds, the watch time will be counted as uninterrupted.
Power events (on/off battery power) have a similar hysteresis, but unlike
the aforementioned properties, will not stop metric collection.
Each seek event will result in a new watch time metric being started and the
old metric finalized as accurately as possible.
BUG=633743
TEST=new unittest.
Committed: https://crrev.com/04bdb589651d194444ed3844a5495e432c0f6b3d
Cr-Commit-Position: refs/heads/master@{#412664}
Patch Set 1 : Hookup, cleanup. #Patch Set 2 : Add more tests. #
Total comments: 10
Patch Set 3 : Comments, couple more tests. #
Total comments: 11
Patch Set 4 : Only log watch time once. #
Total comments: 2
Patch Set 5 : Add tests. Report on WMP destruction. DCHECK EME. #Patch Set 6 : Fix headers, min watch time, add non-base-zero test. #Patch Set 7 : Send finalize signal. Allow recreation for EME. #Patch Set 8 : Fix seeking before metadata crash. #
Total comments: 6
Patch Set 9 : Comments. #
Messages
Total messages: 59 (29 generated)
Description was changed from ========== Add watch time metrics for HTML5 media playback. BUG= ========== to ========== Add watch time metrics for HTML5 media playback. Watch time for our purposes is defined as the amount of elapsed media time for audio+video media. A minimum of 7 seconds of unmuted, foreground media must be watched to start watch time monitoring. Watch time is checked every 5 seconds from then on and reported to multiple buckets: All, MSE, SRC, EME, AC, and battery. Any one of paused, hidden, or muted is sufficient to stop watch time metric reports. Each of these has a hysteresis where if the state change is undone within 5 seconds, the watch time will be counted as uninterrupted. Power events (on/off battery power) have a similar hysteresis, but unlike the aforementioned properties, will not stop metric collection. Each seek event will result in a new watch time metric being started and the old metric finalized as accurately as possible. BUG=633743 TEST=new unittest. ==========
dalecurtis@chromium.org changed reviewers: + sandersd@chromium.org
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
This is now ready for review. I'm still in the process of adding unit tests though.
dalecurtis@chromium.org changed reviewers: + holte@google.com
+holte for UMA review, but prior to review please help me figure out how I'm using StatisticsRecorder incorrectly :) For my unittests which are trying to peek at the stored histogram values, the current set of tests will all pass individually, but if run together, the WatchTimeReporterFinalizeOnDestruction test will fail due to not having any histogram values recorded in the statistics reporter. Any ideas on what I might be doing wrong here? It's hard to find a good example of recorder tests.
On 2016/08/03 00:32:41, DaleCurtis wrote: > +holte for UMA review, but prior to review please help me figure out how I'm > using StatisticsRecorder incorrectly :) For my unittests which are trying to > peek at the stored histogram values, the current set of tests will all pass > individually, but if run together, the WatchTimeReporterFinalizeOnDestruction > test will fail due to not having any histogram values recorded in the statistics > reporter. Any ideas on what I might be doing wrong here? It's hard to find a > good example of recorder tests. I assume you mean it fails due to histograms being recorded by the other tests? I think the correct thing to do is to make sure that each test has it's own StatisticsRecorder by creating/destroying them in SetUp and TearDown rather than your test constructor. This is seems to be a good example: https://codesearch.corp.google.com/piper///depot/google3/third_party/dart_lan... Disclaimer: I am not super familiar with StatisticsRecorder.
On 2016/08/03 at 20:49:59, holte wrote: > On 2016/08/03 00:32:41, DaleCurtis wrote: > > +holte for UMA review, but prior to review please help me figure out how I'm > > using StatisticsRecorder incorrectly :) For my unittests which are trying to > > peek at the stored histogram values, the current set of tests will all pass > > individually, but if run together, the WatchTimeReporterFinalizeOnDestruction > > test will fail due to not having any histogram values recorded in the statistics > > reporter. Any ideas on what I might be doing wrong here? It's hard to find a > > good example of recorder tests. > > I assume you mean it fails due to histograms being recorded by the other tests? I think the correct thing to do is to make sure that each test has it's own StatisticsRecorder by creating/destroying them in SetUp and TearDown rather than your test constructor. This is seems to be a good example: https://codesearch.corp.google.com/piper///depot/google3/third_party/dart_lan... > > Disclaimer: I am not super familiar with StatisticsRecorder. Thanks, I'll try that. The issue seems to be that the metrics are not recorded at all for the second test; hopefully using SetUp/TearDown instead fixes this.
https://codereview.chromium.org/2160963002/diff/60001/media/blink/watch_time_... File media/blink/watch_time_reporter.cc (right): https://codereview.chromium.org/2160963002/diff/60001/media/blink/watch_time_... media/blink/watch_time_reporter.cc:105: FinalizeWatchTime(FinalizeTime::ON_NEXT_UPDATE); It looks like this is correct in the background playing enabled case (at least for now, where the video is supposed to be paused on hide every time). However, it seems strange to use this as a signal if we're not reporting based on it. Would it be better to hook more directly into the play state? https://codereview.chromium.org/2160963002/diff/60001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2160963002/diff/60001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:135: // are a variety of cases in which that time is not accurate. Can you expand on those cases here? (That said, currentTime() is the publicly exposed information, it's not surprising that it's the correct one to use.) https://codereview.chromium.org/2160963002/diff/60001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:1066: base::Bind(&GetCurrentTimeInternal, this))); What's the reason to not just directly bind currentTime? https://codereview.chromium.org/2160963002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2160963002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:23158: + Elapsed watch time for a single HTML5 audio/video playback session. Put some of the explanation from the CL description here?
dalecurtis@chromium.org changed reviewers: + holte@chromium.org - holte@google.com
Addressed comments, added a few more tests. Still adding more tests, but in the interest of keeping the main review going, new patch set uploaded :) holte@ the TearDown/SetUp trick did not work... but I found base::HistogramTester which apparently has the magic sauce necessary to use StatisticsRecorder properly; so I've just switched to using that. https://codereview.chromium.org/2160963002/diff/60001/media/blink/watch_time_... File media/blink/watch_time_reporter.cc (right): https://codereview.chromium.org/2160963002/diff/60001/media/blink/watch_time_... media/blink/watch_time_reporter.cc:105: FinalizeWatchTime(FinalizeTime::ON_NEXT_UPDATE); On 2016/08/03 at 22:45:58, sandersd wrote: > It looks like this is correct in the background playing enabled case (at least for now, where the video is supposed to be paused on hide every time). However, it seems strange to use this as a signal if we're not reporting based on it. Would it be better to hook more directly into the play state? This signal is a proxy for "not visible," so even if we're playing, we want to disregard this as counting towards watch time unless the user returns to the video within the hysteresis. I've clarified this in the method signature. https://codereview.chromium.org/2160963002/diff/60001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2160963002/diff/60001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:135: // are a variety of cases in which that time is not accurate. On 2016/08/03 at 22:45:59, sandersd wrote: > Can you expand on those cases here? (That said, currentTime() is the publicly exposed information, it's not surprising that it's the correct one to use.) Done. https://codereview.chromium.org/2160963002/diff/60001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:1066: base::Bind(&GetCurrentTimeInternal, this))); On 2016/08/03 at 22:45:59, sandersd wrote: > What's the reason to not just directly bind currentTime? The detail of currentTime returning a double is specific to the blink layer and shouldn't leak into other components (they should use TimeDelta). https://codereview.chromium.org/2160963002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2160963002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:23158: + Elapsed watch time for a single HTML5 audio/video playback session. On 2016/08/03 at 22:45:59, sandersd wrote: > Put some of the explanation from the CL description here? Done, I elided some of the time constants though.
https://codereview.chromium.org/2160963002/diff/60001/media/blink/watch_time_... File media/blink/watch_time_reporter.cc (right): https://codereview.chromium.org/2160963002/diff/60001/media/blink/watch_time_... media/blink/watch_time_reporter.cc:105: FinalizeWatchTime(FinalizeTime::ON_NEXT_UPDATE); On 2016/08/04 19:14:59, DaleCurtis wrote: > On 2016/08/03 at 22:45:58, sandersd wrote: > > It looks like this is correct in the background playing enabled case (at least > for now, where the video is supposed to be paused on hide every time). However, > it seems strange to use this as a signal if we're not reporting based on it. > Would it be better to hook more directly into the play state? > > This signal is a proxy for "not visible," so even if we're playing, we want to > disregard this as counting towards watch time unless the user returns to the > video within the hysteresis. I've clarified this in the method signature. Do you still want to start the reporting timer in OnPlaying() while hidden?
https://codereview.chromium.org/2160963002/diff/60001/media/blink/watch_time_... File media/blink/watch_time_reporter.cc (right): https://codereview.chromium.org/2160963002/diff/60001/media/blink/watch_time_... media/blink/watch_time_reporter.cc:105: FinalizeWatchTime(FinalizeTime::ON_NEXT_UPDATE); On 2016/08/04 at 20:01:00, sandersd wrote: > On 2016/08/04 19:14:59, DaleCurtis wrote: > > On 2016/08/03 at 22:45:58, sandersd wrote: > > > It looks like this is correct in the background playing enabled case (at least > > for now, where the video is supposed to be paused on hide every time). However, > > it seems strange to use this as a signal if we're not reporting based on it. > > Would it be better to hook more directly into the play state? > > > > This signal is a proxy for "not visible," so even if we're playing, we want to > > disregard this as counting towards watch time unless the user returns to the > > video within the hysteresis. I've clarified this in the method signature. > > Do you still want to start the reporting timer in OnPlaying() while hidden? StartReportingTimer() will bail if start conditions are not met.
lgtm
https://codereview.chromium.org/2160963002/diff/80001/media/blink/watch_time_... File media/blink/watch_time_reporter.cc (right): https://codereview.chromium.org/2160963002/diff/80001/media/blink/watch_time_... media/blink/watch_time_reporter.cc:205: UMA_HISTOGRAM_LONG_TIMES(kHistogramAudioVideoAll, elapsed); It looks like this will report repeatedly, with a growing 'elapsed'. What does the actual data in the reported UMA looks like? Is there code somewhere that is coalescing these? https://codereview.chromium.org/2160963002/diff/80001/media/blink/watch_time_... File media/blink/watch_time_reporter.h (right): https://codereview.chromium.org/2160963002/diff/80001/media/blink/watch_time_... media/blink/watch_time_reporter.h:54: using GetMediaTimeCB = base::Callback<base::TimeDelta(void)>; Move above comment. https://codereview.chromium.org/2160963002/diff/80001/media/blink/watch_time_... media/blink/watch_time_reporter.h:111: void StartReportingTimer(base::TimeDelta start_timestamp); nit: MaybeStart/MaybeFinalize? https://codereview.chromium.org/2160963002/diff/80001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2160963002/diff/80001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:489: watch_time_reporter_->OnSeeking(time); Since the actual seek time may differ from the requested seek time, perhaps it would be worth hooking the seek completion as well? https://codereview.chromium.org/2160963002/diff/80001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.h (right): https://codereview.chromium.org/2160963002/diff/80001/media/blink/webmediapla... media/blink/webmediaplayer_impl.h:540: bool is_encrypted_; Swap these two declarations.
https://codereview.chromium.org/2160963002/diff/80001/media/blink/watch_time_... File media/blink/watch_time_reporter.cc (right): https://codereview.chromium.org/2160963002/diff/80001/media/blink/watch_time_... media/blink/watch_time_reporter.cc:205: UMA_HISTOGRAM_LONG_TIMES(kHistogramAudioVideoAll, elapsed); On 2016/08/04 at 23:06:26, sandersd wrote: > It looks like this will report repeatedly, with a growing 'elapsed'. What does the actual data in the reported UMA looks like? Is there code somewhere that is coalescing these? No coalescing, we end up with categories like the following: 50% watched 1hr, 30% watched 2hr, 20% watched 3hr, 10% watched 4hr, We're only able to make relative statements about the data using this model. I.e. we can only say that some percentage watched at least x hours. We won't get an absolute watch time number with this approach. To get that we have to move this to MediaInternals and record the data in the browser process like we do with pipeline status. I thought this would be fine, but I need to check with the PM side to see exactly what type of data they'd like to see here.
> No coalescing, we end up with categories like the following: > > 50% watched 1hr, > 30% watched 2hr, > 20% watched 3hr, > 10% watched 4hr, But the histogram only goes to 1hr?
https://codereview.chromium.org/2160963002/diff/80001/media/blink/watch_time_... File media/blink/watch_time_reporter.cc (right): https://codereview.chromium.org/2160963002/diff/80001/media/blink/watch_time_... media/blink/watch_time_reporter.cc:205: UMA_HISTOGRAM_LONG_TIMES(kHistogramAudioVideoAll, elapsed); On 2016/08/04 23:34:21, DaleCurtis wrote: > On 2016/08/04 at 23:06:26, sandersd wrote: > > It looks like this will report repeatedly, with a growing 'elapsed'. What does > the actual data in the reported UMA looks like? Is there code somewhere that is > coalescing these? > > No coalescing, we end up with categories like the following: > > 50% watched 1hr, > 30% watched 2hr, > 20% watched 3hr, > 10% watched 4hr, > > We're only able to make relative statements about the data using this model. > I.e. we can only say that some percentage watched at least x hours. We won't get > an absolute watch time number with this approach. To get that we have to move > this to MediaInternals and record the data in the browser process like we do > with pipeline status. > > I thought this would be fine, but I need to check with the PM side to see > exactly what type of data they'd like to see here. holte@: It's not obvious that we can get the above information from UMA_HISTOGRAM_LONG_TIMES(), because we can't be sure that we can compare the differently-sized buckets without a lot of post-processing. If we want to record this kind of data, are there UMA design patterns that are known to work?
On 2016/08/04 23:57:18, sandersd wrote: > https://codereview.chromium.org/2160963002/diff/80001/media/blink/watch_time_... > File media/blink/watch_time_reporter.cc (right): > > https://codereview.chromium.org/2160963002/diff/80001/media/blink/watch_time_... > media/blink/watch_time_reporter.cc:205: > UMA_HISTOGRAM_LONG_TIMES(kHistogramAudioVideoAll, elapsed); > On 2016/08/04 23:34:21, DaleCurtis wrote: > > On 2016/08/04 at 23:06:26, sandersd wrote: > > > It looks like this will report repeatedly, with a growing 'elapsed'. What > does > > the actual data in the reported UMA looks like? Is there code somewhere that > is > > coalescing these? > > > > No coalescing, we end up with categories like the following: > > > > 50% watched 1hr, > > 30% watched 2hr, > > 20% watched 3hr, > > 10% watched 4hr, > > > > We're only able to make relative statements about the data using this model. > > I.e. we can only say that some percentage watched at least x hours. We won't > get > > an absolute watch time number with this approach. To get that we have to move > > this to MediaInternals and record the data in the browser process like we do > > with pipeline status. > > > > I thought this would be fine, but I need to check with the PM side to see > > exactly what type of data they'd like to see here. > > holte@: It's not obvious that we can get the above information from > UMA_HISTOGRAM_LONG_TIMES(), because we can't be sure that we can compare the > differently-sized buckets without a lot of post-processing. If we want to record > this kind of data, are there UMA design patterns that are known to work? Can you elaborate on what kind of analysis you are looking to do? Do you want to know the final times only or something else? If it's possible just delay recording in the histogram until the end of playback, that's probably best. If you really want to record as things elapse because you won't get a chance to record at the end, you probably want to record exactly once in each time bucket as you continue you to play. In this case, you probably want to specify your own time intervals. If you want fixed time intervals, you could just use an enumerated histogram (bucket widths always 1) to count # of intervals. If you want some other spacing, you can use base::CustomHistogram to specify the bounds and share them with your recording code.
Latest patch set sends MediaLog events over to the browser process on a regular basis where they are saved and reported once during process termination. Still working on the unit tests. There is an open question about the histogram size though. LONG_TIMES goes up to 1 hour, but I suspect our data will have a lot of points in the 1-30 second range then hits at 10 minutes, 20 minutes, 30 minutes, 40 minutes, 1 hr, 1.5 hrs, 2 hrs and then data points will trail off from there. holte@ what do you recommend for our histogram sizing in that case? https://codereview.chromium.org/2160963002/diff/80001/media/blink/watch_time_... File media/blink/watch_time_reporter.h (right): https://codereview.chromium.org/2160963002/diff/80001/media/blink/watch_time_... media/blink/watch_time_reporter.h:54: using GetMediaTimeCB = base::Callback<base::TimeDelta(void)>; On 2016/08/04 at 23:06:26, sandersd wrote: > Move above comment. Done. https://codereview.chromium.org/2160963002/diff/80001/media/blink/watch_time_... media/blink/watch_time_reporter.h:111: void StartReportingTimer(base::TimeDelta start_timestamp); On 2016/08/04 at 23:06:26, sandersd wrote: > nit: MaybeStart/MaybeFinalize? Done. https://codereview.chromium.org/2160963002/diff/80001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2160963002/diff/80001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:489: watch_time_reporter_->OnSeeking(time); On 2016/08/04 at 23:06:26, sandersd wrote: > Since the actual seek time may differ from the requested seek time, perhaps it would be worth hooking the seek completion as well? Reworked to avoid needing seek time and just call OnPlaying() if playing upon seek completion. https://codereview.chromium.org/2160963002/diff/80001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.h (right): https://codereview.chromium.org/2160963002/diff/80001/media/blink/webmediapla... media/blink/webmediaplayer_impl.h:540: bool is_encrypted_; On 2016/08/04 at 23:06:26, sandersd wrote: > Swap these two declarations. Done.
lgtm https://codereview.chromium.org/2160963002/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2160963002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:23306: + media. A minimum of 7 seconds of unmuted, foreground media must be watched Add explanation that this is aggregated per player instance.
On 2016/08/08 23:16:42, DaleCurtis wrote:
> Latest patch set sends MediaLog events over to the browser process on a
regular
> basis where they are saved and reported once during process termination. Still
> working on the unit tests.
>
> There is an open question about the histogram size though. LONG_TIMES goes up
to
> 1 hour, but I suspect our data will have a lot of points in the 1-30 second
> range then hits at 10 minutes, 20 minutes, 30 minutes, 40 minutes, 1 hr, 1.5
> hrs, 2 hrs and then data points will trail off from there. holte@ what do you
> recommend for our histogram sizing in that case?
>
I would suggest using something like
UMA_HISTOGRAM_CUSTOM_TIMES("Name", sample, base::TimeDelta::FromSeconds(1),
base::TimeDelta::FromHours(10), 50)
since that seems to cover the range of times you care about,
For reference, that would give you the following buckets in milliseconds:
[(0, 0), (0, 1000), (1000, 1244), (1244, 1548), (1548, 1926), (1926, 2397),
(2397, 2983), (2983, 3712), (3712, 4619), (4619, 5747), (5747, 7151), (7151,
8898), (8898, 11072), (11072, 13777), (13777, 17143), (17143, 21331), (21331,
26542), (26542, 33026), (33026, 41094), (41094, 51133), (51133, 63624), (63624,
79166), (79166, 98505), (98505, 122568), (122568, 152510), (152510, 189766),
(189766, 236123), (236123, 293805), (293805, 365577), (365577, 454882), (454882,
566003), (566003, 704270), (704270, 876313), (876313, 1090384), (1090384,
1356749), (1356749, 1688184), (1688184, 2100584), (2100584, 2613727), (2613727,
3252224), (3252224, 4046697), (4046697, 5035248), (5035248, 6265288), (6265288,
7795810), (7795810, 9700216), (9700216, 12069842), (12069842, 15018334),
(15018334, 18687101), (18687101, 23252097), (23252097, 28932257), (28932257,
36000000), (36000000, 2147483647)]
Patchset #5 (id:120001) has been deleted
Patchset #5 (id:140001) has been deleted
Patchset #5 (id:160001) has been deleted
Thanks for the review. I've used the suggested buckets and added a whole bunch of tests. https://codereview.chromium.org/2160963002/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2160963002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:23306: + media. A minimum of 7 seconds of unmuted, foreground media must be watched On 2016/08/09 at 18:31:20, sandersd wrote: > Add explanation that this is aggregated per player instance. Done.
The CQ bit was checked by dalecurtis@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from holte@chromium.org, sandersd@chromium.org Link to the patchset: https://codereview.chromium.org/2160963002/#ps180001 (title: "Add tests. Report on WMP destruction. DCHECK EME.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
This is minor, but should we start the buckets at a higher number since we're only recording 7+? We have 9-10 unused buckets with this spacing.
Patchset #6 (id:200001) has been deleted
The CQ bit was checked by dalecurtis@chromium.org
Good catch Chris. I've set the histogram to start at 7 seconds. I've also moved the keys into MediaLog to avoid a media/blink DEPS in content/browser.
The patchset sent to the CQ was uploaded after l-g-t-m from holte@chromium.org, sandersd@chromium.org Link to the patchset: https://codereview.chromium.org/2160963002/#ps220001 (title: "Fix headers, min watch time, add non-base-zero test.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Exceeded global retry quota
Patchset #7 (id:240001) has been deleted
sandersd@ PTAL, in the transition to MediaInternals we lost some intrinsic behavior around finalization of the histograms. There now needs to be an explicit signal since the histogram won't otherwise finalize until the WMP instance is destroyed. Yet that is insufficient when there are looping tracks or seek events require finalization. There's also an issue where EME may not be known until later. In this case we need to finalize the non-eme watch time and count further watch time as EME.
The CQ bit was checked by dalecurtis@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #8 (id:280001) has been deleted
The CQ bit was checked by dalecurtis@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Exceeded global retry quota
lgtm, just nits. https://codereview.chromium.org/2160963002/diff/300001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2160963002/diff/300001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:412: watch_time_reporter_->OnPlaying(); Perhaps add conditions (or at least DCHECKs) that |watch_time_reporter_| is set throughout? There are enough WMPI states that it's hard for me to be certain this is always safe. https://codereview.chromium.org/2160963002/diff/300001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2160963002/diff/300001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:23486: + is checked on a regular basis from then on and reported to multiple buckets. Nit: "multiple buckets" is ambiguous (although I suppose we always call the histogram components "bins"). https://codereview.chromium.org/2160963002/diff/300001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:23496: + old metric finalized as accurately as possible. I had assumed that what we wanted to record was the total time each player played for. What's the intuition for why this approach (breaking on seek) is better for our analysis?
The CQ bit was checked by dalecurtis@chromium.org
https://codereview.chromium.org/2160963002/diff/300001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2160963002/diff/300001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:412: watch_time_reporter_->OnPlaying(); On 2016/08/17 at 01:11:25, sandersd wrote: > Perhaps add conditions (or at least DCHECKs) that |watch_time_reporter_| is set throughout? There are enough WMPI states that it's hard for me to be certain this is always safe. We don't typically DCHECK() prior to dereferences, though since I spent some time debugging the seek issue I acquiesce here :)o https://codereview.chromium.org/2160963002/diff/300001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2160963002/diff/300001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:23486: + is checked on a regular basis from then on and reported to multiple buckets. On 2016/08/17 at 01:11:25, sandersd wrote: > Nit: "multiple buckets" is ambiguous (although I suppose we always call the histogram components "bins"). Done; removed bucket terminology. https://codereview.chromium.org/2160963002/diff/300001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:23496: + old metric finalized as accurately as possible. On 2016/08/17 at 01:11:25, sandersd wrote: > I had assumed that what we wanted to record was the total time each player played for. What's the intuition for why this approach (breaking on seek) is better for our analysis? I think this is worth exploring more, but for the first cut capping to [seek_time, duration] simplifies the metrics collection by avoiding any additive metrics. We can change this later without any harm or decide that these metrics should always be bounded by the duration of the clip (common) and only have aggregate metrics at the scope of media internals. I prefer the latter but am happy to debate.
The patchset sent to the CQ was uploaded after l-g-t-m from holte@chromium.org, sandersd@chromium.org Link to the patchset: https://codereview.chromium.org/2160963002/#ps320001 (title: "Comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #9 (id:320001)
Message was sent while issue was closed.
Description was changed from ========== Add watch time metrics for HTML5 media playback. Watch time for our purposes is defined as the amount of elapsed media time for audio+video media. A minimum of 7 seconds of unmuted, foreground media must be watched to start watch time monitoring. Watch time is checked every 5 seconds from then on and reported to multiple buckets: All, MSE, SRC, EME, AC, and battery. Any one of paused, hidden, or muted is sufficient to stop watch time metric reports. Each of these has a hysteresis where if the state change is undone within 5 seconds, the watch time will be counted as uninterrupted. Power events (on/off battery power) have a similar hysteresis, but unlike the aforementioned properties, will not stop metric collection. Each seek event will result in a new watch time metric being started and the old metric finalized as accurately as possible. BUG=633743 TEST=new unittest. ========== to ========== Add watch time metrics for HTML5 media playback. Watch time for our purposes is defined as the amount of elapsed media time for audio+video media. A minimum of 7 seconds of unmuted, foreground media must be watched to start watch time monitoring. Watch time is checked every 5 seconds from then on and reported to multiple buckets: All, MSE, SRC, EME, AC, and battery. Any one of paused, hidden, or muted is sufficient to stop watch time metric reports. Each of these has a hysteresis where if the state change is undone within 5 seconds, the watch time will be counted as uninterrupted. Power events (on/off battery power) have a similar hysteresis, but unlike the aforementioned properties, will not stop metric collection. Each seek event will result in a new watch time metric being started and the old metric finalized as accurately as possible. BUG=633743 TEST=new unittest. Committed: https://crrev.com/04bdb589651d194444ed3844a5495e432c0f6b3d Cr-Commit-Position: refs/heads/master@{#412664} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/04bdb589651d194444ed3844a5495e432c0f6b3d Cr-Commit-Position: refs/heads/master@{#412664} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
