Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2631633003:
[Video] Only start tracking foreground time if the video was resumed. (Closed)
|
Created:
3 years, 11 months ago by whywhat Modified:
3 years, 11 months ago CC:
apacible+watch_chromium.org, chromium-reviews, erickung+watch_chromium.org, feature-media-reviews_chromium.org, miu+watch_chromium.org, xjz+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Video] Fix the TimeFromForegroundToFirstFrame metric.
Only start tracking the foreground time if the video we had or would optimize is playing or will be playing when shown.
Thus we don't track the time for paused videos or videos we wouldn't optimize like streams so they don't improve the average results.
BUG=670150
TEST=manual
Review-Url: https://codereview.chromium.org/2631633003
Cr-Commit-Position: refs/heads/master@{#444816}
Committed: https://chromium.googlesource.com/chromium/src/+/cc273ddad35337b82ddfdb77248986fc928d035e
Patch Set 1 #
Total comments: 1
Patch Set 2 : Only set the time when playing or were paused when hidden #
Total comments: 3
Patch Set 3 : Record the metric only for should be optimized videos #Patch Set 4 : Split the histogram into paused and disabled track #
Total comments: 10
Patch Set 5 : Restored the old histogram #Patch Set 6 : Pass the time back to WMPI #
Total comments: 10
Patch Set 7 : Clarified code, updated unittests #
Total comments: 2
Patch Set 8 : Added a comment #
Messages
Total messages: 49 (16 generated)
avayvod@chromium.org changed reviewers: + dalecurtis@chromium.org, sandersd@chromium.org
PTaL I based it off the refactoring CL Dan is submitting.
https://codereview.chromium.org/2631633003/diff/1/media/blink/webmediaplayer_... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2631633003/diff/1/media/blink/webmediaplayer_... media/blink/webmediaplayer_impl.cc:1455: if (!paused_) { This is going to race with the playback start attempt above; i.e., the above methods will start playback, a frame may be delivered and then you'll set the foreground time.
On 2017/01/13 at 21:52:19, dalecurtis wrote: > https://codereview.chromium.org/2631633003/diff/1/media/blink/webmediaplayer_... > File media/blink/webmediaplayer_impl.cc (right): > > https://codereview.chromium.org/2631633003/diff/1/media/blink/webmediaplayer_... > media/blink/webmediaplayer_impl.cc:1455: if (!paused_) { > This is going to race with the playback start attempt above; i.e., the above methods will start playback, a frame may be delivered and then you'll set the foreground time. I thought it would be async in UpdatePlayState. What if I check for (!paused_ || paused_when_hidden_) before setting the time where it is now? Seems like paused_when_hidden_ would be the only time when OnShown would transition from paused to playing although I'm not sure how to preserve that behavior in the future if other conditions are added.
That seems okay for now. The OnPlay() is the only thing that might cause the race.
Only set the time when playing or were paused when hidden
Description was changed from ========== [Video] Only start tracking foreground time if the video was resumed. BUG=670150 TEST=manual ========== to ========== [Video] Only start tracking foreground time if the video is playing or will be playing when shown. BUG=670150 TEST=manual ==========
The CQ bit was checked by avayvod@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...
PTAL
lgtm
lgtm https://codereview.chromium.org/2631633003/diff/20001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2631633003/diff/20001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:1446: if (!paused_ || paused_when_hidden_) { Quick check: Is this what we want to do for audio-only content? I'm assuming it makes no difference but I wanted to check. (I suppose the same question for background-playing video where the track was not disabled, eg. streaming.)
https://codereview.chromium.org/2631633003/diff/20001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2631633003/diff/20001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:1446: if (!paused_ || paused_when_hidden_) { On 2017/01/18 at 21:30:04, sandersd wrote: > Quick check: Is this what we want to do for audio-only content? I'm assuming it makes no difference but I wanted to check. > > (I suppose the same question for background-playing video where the track was not disabled, eg. streaming.) I assume VFC doesn't do anything for audio-only content? Dale? For videos we want to track the metric despite the optimization working since we want to compare it with the baseline. Might be we want to only record it for optimization eligible videos though so that non-affected videos don't make the results too optimistic.
https://codereview.chromium.org/2631633003/diff/20001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2631633003/diff/20001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:1446: if (!paused_ || paused_when_hidden_) { On 2017/01/18 at 21:57:40, whywhat wrote: > On 2017/01/18 at 21:30:04, sandersd wrote: > > Quick check: Is this what we want to do for audio-only content? I'm assuming it makes no difference but I wanted to check. > > > > (I suppose the same question for background-playing video where the track was not disabled, eg. streaming.) > > I assume VFC doesn't do anything for audio-only content? Dale? > For videos we want to track the metric despite the optimization working since we want to compare it with the baseline. Might be we want to only record it for optimization eligible videos though so that non-affected videos don't make the results too optimistic. VFC is unused for audio only so this is a noop, though it could be excluded with hasVideo() if we want.
https://codereview.chromium.org/2631633003/diff/20001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2631633003/diff/20001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:1446: if (!paused_ || paused_when_hidden_) { On 2017/01/18 at 21:57:40, whywhat wrote: > On 2017/01/18 at 21:30:04, sandersd wrote: > > Quick check: Is this what we want to do for audio-only content? I'm assuming it makes no difference but I wanted to check. > > > > (I suppose the same question for background-playing video where the track was not disabled, eg. streaming.) > > I assume VFC doesn't do anything for audio-only content? Dale? > For videos we want to track the metric despite the optimization working since we want to compare it with the baseline. Might be we want to only record it for optimization eligible videos though so that non-affected videos don't make the results too optimistic. VFC is unused for audio only so this is a noop, though it could be excluded with hasVideo() if we want.
Record the metric only for should be optimized videos
Description was changed from ========== [Video] Only start tracking foreground time if the video is playing or will be playing when shown. BUG=670150 TEST=manual ========== to ========== [Video] Fix the TimeFromForegroundToFirstFrame metric. Only start tracking the foreground time if the video we had or would optimize is playing or will be playing when shown. Thus we don't track the time for paused videos or videos we wouldn't optimize like streams so they don't improve the average results. BUG=670150 TEST=manual ==========
On 2017/01/18 at 22:10:01, dalecurtis wrote: > https://codereview.chromium.org/2631633003/diff/20001/media/blink/webmediapla... > File media/blink/webmediaplayer_impl.cc (right): > > https://codereview.chromium.org/2631633003/diff/20001/media/blink/webmediapla... > media/blink/webmediaplayer_impl.cc:1446: if (!paused_ || paused_when_hidden_) { > On 2017/01/18 at 21:57:40, whywhat wrote: > > On 2017/01/18 at 21:30:04, sandersd wrote: > > > Quick check: Is this what we want to do for audio-only content? I'm assuming it makes no difference but I wanted to check. > > > > > > (I suppose the same question for background-playing video where the track was not disabled, eg. streaming.) > > > > I assume VFC doesn't do anything for audio-only content? Dale? > > For videos we want to track the metric despite the optimization working since we want to compare it with the baseline. Might be we want to only record it for optimization eligible videos though so that non-affected videos don't make the results too optimistic. > > VFC is unused for audio only so this is a noop, though it could be excluded with hasVideo() if we want. Thanks. Dan suggested recording two histograms: one for paused videos and one for disabled video tracks. WDYT? I could pass an enum param to SetForegroundTime and choose the metric name to record in VFC based on the value.
That probably fine too. For paused videos the play time would be the full resume time. For video-track you'd have some number likely less than that.
The CQ bit was checked by avayvod@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...
Split the histogram into paused and disabled track
avayvod@chromium.org changed reviewers: + isherman@chromium.org
+Ilya for histograms.xml Dale, Dan, one more look after I split the metrics, please? :)
https://codereview.chromium.org/2631633003/diff/60001/media/blink/video_frame... File media/blink/video_frame_compositor.cc (right): https://codereview.chromium.org/2631633003/diff/60001/media/blink/video_frame... media/blink/video_frame_compositor.cc:194: BackgroundVideoOptimization optimization) { This is polluting VFC with knowledge about BackgroudnVideoOptimization. It seems instead we should register a callback ala SetOnNextRenderedFrameCallback() or similar. This cb can bind in the |when| value and record the histogram value inside WMPI who has all this knowledge. https://codereview.chromium.org/2631633003/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2631633003/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:26207: +<histogram name="Media.Video.TimeFromForegroundToFirstFrame.DisabledTrack" Can't drop histograms like this. Need to mark the old one as obsolete.
Metrics LGTM % comments https://codereview.chromium.org/2631633003/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2631633003/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:26207: +<histogram name="Media.Video.TimeFromForegroundToFirstFrame.DisabledTrack" On 2017/01/18 23:39:02, DaleCurtis wrote: > Can't drop histograms like this. Need to mark the old one as obsolete. +1 https://codereview.chromium.org/2631633003/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:26223: + be paused in the background is brought to the foreground and when the video What does "or could be" in this histogram description mean? Is this basically saying there might be false positive noise in the data?
Restored the old histogram
https://codereview.chromium.org/2631633003/diff/60001/media/blink/video_frame... File media/blink/video_frame_compositor.cc (right): https://codereview.chromium.org/2631633003/diff/60001/media/blink/video_frame... media/blink/video_frame_compositor.cc:194: BackgroundVideoOptimization optimization) { On 2017/01/18 at 23:39:01, DaleCurtis wrote: > This is polluting VFC with knowledge about BackgroudnVideoOptimization. It seems instead we should register a callback ala SetOnNextRenderedFrameCallback() or similar. This cb can bind in the |when| value and record the histogram value inside WMPI who has all this knowledge. Hm, ok. Will upload a follow up with this. https://codereview.chromium.org/2631633003/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2631633003/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:26207: +<histogram name="Media.Video.TimeFromForegroundToFirstFrame.DisabledTrack" On 2017/01/18 at 23:39:02, DaleCurtis wrote: > Can't drop histograms like this. Need to mark the old one as obsolete. Done. https://codereview.chromium.org/2631633003/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:26223: + be paused in the background is brought to the foreground and when the video On 2017/01/19 at 02:03:39, Ilya Sherman wrote: > What does "or could be" in this histogram description mean? Is this basically saying there might be false positive noise in the data? It will only be paused if the feature flag is set, but we want to collect the metrics in any case to compare.
https://codereview.chromium.org/2631633003/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2631633003/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:26223: + be paused in the background is brought to the foreground and when the video On 2017/01/19 02:33:58, whywhat wrote: > On 2017/01/19 at 02:03:39, Ilya Sherman wrote: > > What does "or could be" in this histogram description mean? Is this basically > saying there might be false positive noise in the data? > > It will only be paused if the feature flag is set, but we want to collect the > metrics in any case to compare. Ah, I see. Could you please clarify that in the histogram description? Thanks!
Pass the time back to WMPI
PTAL https://codereview.chromium.org/2631633003/diff/60001/media/blink/video_frame... File media/blink/video_frame_compositor.cc (right): https://codereview.chromium.org/2631633003/diff/60001/media/blink/video_frame... media/blink/video_frame_compositor.cc:194: BackgroundVideoOptimization optimization) { On 2017/01/19 at 02:33:58, whywhat wrote: > On 2017/01/18 at 23:39:01, DaleCurtis wrote: > > This is polluting VFC with knowledge about BackgroudnVideoOptimization. It seems instead we should register a callback ala SetOnNextRenderedFrameCallback() or similar. This cb can bind in the |when| value and record the histogram value inside WMPI who has all this knowledge. > > Hm, ok. Will upload a follow up with this. Done. Looks neat to me :) https://codereview.chromium.org/2631633003/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2631633003/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:26223: + be paused in the background is brought to the foreground and when the video On 2017/01/19 at 02:39:57, Ilya Sherman wrote: > On 2017/01/19 02:33:58, whywhat wrote: > > On 2017/01/19 at 02:03:39, Ilya Sherman wrote: > > > What does "or could be" in this histogram description mean? Is this basically > > saying there might be false positive noise in the data? > > > > It will only be paused if the feature flag is set, but we want to collect the > > metrics in any case to compare. > > Ah, I see. Could you please clarify that in the histogram description? Thanks! Done. Let me know if I could use better wording.
Logic is getting hard to follow here. Do the existing tests pass without modification? https://codereview.chromium.org/2631633003/diff/100001/media/blink/video_fram... File media/blink/video_frame_compositor.h (right): https://codereview.chromium.org/2631633003/diff/100001/media/blink/video_fram... media/blink/video_frame_compositor.h:62: using OnNewProcessedFrameCB = base::Callback<void(base::TimeTicks)>; Would be better as a base::OnceCallback() since it's clearer it can only run one time. https://codereview.chromium.org/2631633003/diff/100001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2631633003/diff/100001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:131: bool IsBackgroundVideoPauseEnabled() { Roll into ShouldPauseWhenHidden? https://codereview.chromium.org/2631633003/diff/100001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1455: if ((!paused_ && ShouldDisableVideoWhenHidden()) || ShouldPauseWhenHidden()) { Is this right? Why is it not just !paused_ || paused_when_hidden_ ? https://codereview.chromium.org/2631633003/diff/100001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1456: VideoFrameCompositor::OnNewProcessedFrameCB new_processed_frame_cb = Can probably inlien this if you want. https://codereview.chromium.org/2631633003/diff/100001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:2192: if (!video_track_disabled_ && IsBackgroundVideoTrackOptimizationEnabled() && Please keep the IsXXX methods as part of the Should() methods, having both is a bit confusing.
Clarified code, updated unittests
https://codereview.chromium.org/2631633003/diff/100001/media/blink/video_fram... File media/blink/video_frame_compositor.h (right): https://codereview.chromium.org/2631633003/diff/100001/media/blink/video_fram... media/blink/video_frame_compositor.h:62: using OnNewProcessedFrameCB = base::Callback<void(base::TimeTicks)>; On 2017/01/19 at 03:37:19, DaleCurtis wrote: > Would be better as a base::OnceCallback() since it's clearer it can only run one time. I tried that but then BIND_TO_RENDER_LOOP doesn't work as is (is there some template magic that would allow me to make media::BindToRenderLoop accept OnceCallback without duplicating everything there?) and the code doesn't become shorter. Assuming it's a nit? https://codereview.chromium.org/2631633003/diff/100001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2631633003/diff/100001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:131: bool IsBackgroundVideoPauseEnabled() { On 2017/01/19 at 03:37:20, DaleCurtis wrote: > Roll into ShouldPauseWhenHidden? Done. https://codereview.chromium.org/2631633003/diff/100001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1455: if ((!paused_ && ShouldDisableVideoWhenHidden()) || ShouldPauseWhenHidden()) { On 2017/01/19 at 03:37:20, DaleCurtis wrote: > Is this right? Why is it not just !paused_ || paused_when_hidden_ ? Because paused_when_hidden is not set when the flag is off while ShouldPauseWhenHidden would return true still. So when the optimization is on, we should collect metrics only if paused_when_hidden_ or if !paused_ and ShouldDisable... If the optimization is off, we should collect the metrics only if !paused and either ShouldDisable... or ShouldPaused... (so I miss one case above where ShouldPaused... is true, paused_ is true but paused_when_hidden_ is not - i.e. when the page paused the element while in the background). In fact, it should be: if (!paused_ && (ShouldDisableVideoWhenHidden() || ShouldPauseWhenHidden()) || paused_when_hidden_) Now ShouldPause... and ShouldDisable... only differ in hasAudio() (and how the flag is treated on Android -- but we ignore the flag for the metrics) -> extracted the common part. https://codereview.chromium.org/2631633003/diff/100001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1456: VideoFrameCompositor::OnNewProcessedFrameCB new_processed_frame_cb = On 2017/01/19 at 03:37:19, DaleCurtis wrote: > Can probably inlien this if you want. Ack. I think it reads slightly better. https://codereview.chromium.org/2631633003/diff/100001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:2192: if (!video_track_disabled_ && IsBackgroundVideoTrackOptimizationEnabled() && On 2017/01/19 at 03:37:20, DaleCurtis wrote: > Please keep the IsXXX methods as part of the Should() methods, having both is a bit confusing. Done.
The CQ bit was checked by avayvod@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: This issue passed the CQ dry run.
Thanks, the updated wording is clearer and very helpful. (Still LGTM)
lgtm, thanks for the test. https://codereview.chromium.org/2631633003/diff/120001/media/blink/video_fram... File media/blink/video_frame_compositor.h (right): https://codereview.chromium.org/2631633003/diff/120001/media/blink/video_fram... media/blink/video_frame_compositor.h:110: // Sets the callback to be run when the new frame has been processed. Document that it's only used once.
Added a comment
I wonder if there're plans to be able to mock pipeline_ in WMPI in tests so I could avoid adding these optional params every time :) https://codereview.chromium.org/2631633003/diff/120001/media/blink/video_fram... File media/blink/video_frame_compositor.h (right): https://codereview.chromium.org/2631633003/diff/120001/media/blink/video_fram... media/blink/video_frame_compositor.h:110: // Sets the callback to be run when the new frame has been processed. On 2017/01/19 at 17:32:07, DaleCurtis wrote: > Document that it's only used once. Done.
The CQ bit was checked by avayvod@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sandersd@chromium.org, isherman@chromium.org, dalecurtis@chromium.org Link to the patchset: https://codereview.chromium.org/2631633003/#ps140001 (title: "Added a comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
There's already a MockPipeline IIRC, just need a way to inject it instead of creating one in StartPipeline.
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1484848551835180,
"parent_rev": "535b89a267d880c41efed60526213483ae79e707", "commit_rev":
"cc273ddad35337b82ddfdb77248986fc928d035e"}
Message was sent while issue was closed.
Description was changed from ========== [Video] Fix the TimeFromForegroundToFirstFrame metric. Only start tracking the foreground time if the video we had or would optimize is playing or will be playing when shown. Thus we don't track the time for paused videos or videos we wouldn't optimize like streams so they don't improve the average results. BUG=670150 TEST=manual ========== to ========== [Video] Fix the TimeFromForegroundToFirstFrame metric. Only start tracking the foreground time if the video we had or would optimize is playing or will be playing when shown. Thus we don't track the time for paused videos or videos we wouldn't optimize like streams so they don't improve the average results. BUG=670150 TEST=manual Review-Url: https://codereview.chromium.org/2631633003 Cr-Commit-Position: refs/heads/master@{#444816} Committed: https://chromium.googlesource.com/chromium/src/+/cc273ddad35337b82ddfdb772489... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/cc273ddad35337b82ddfdb772489... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
