Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2218963003:
Added metrics for background video playback. (Closed)
|
Created:
4 years, 4 months ago by whywhat Modified:
4 years, 4 months ago CC:
asvitkine+watch_chromium.org, chromium-reviews, darin-cc_chromium.org, feature-media-reviews_chromium.org, jam, mcasas+watch+vc_chromium.org, miu+watch_chromium.org, mlamouri+watch-content_chromium.org, posciak+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdded metrics for background video playback.
Record the amount of time continouous background video playback happens.
Record each time the render frame media is shown/hidden/resumed/paused.
Record each time the render frame media is resumed and paused in the background.
BUG=576248
TEST=browser test + manual testing
Committed: https://crrev.com/534a9e9d5c8b52484023821e69736d4ce63bc925
Cr-Commit-Position: refs/heads/master@{#412665}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Added PowerObserver #
Total comments: 18
Patch Set 3 : Removed the PowerObserver logic, revised metrics descriptions #
Total comments: 4
Patch Set 4 : Got UserMetricsAction back in action and fixed a typo in the histogram description #Patch Set 5 : Renamed the histogram and changed the buckets. Added a TODO #Patch Set 6 : Updated the browser test #
Messages
Total messages: 44 (19 generated)
avayvod@chromium.org changed reviewers: + dalecurtis@chromium.org, isherman@chromium.org, mlamouri@chromium.org
PTaL mlamouri for the logic isherman for tools/metrics/ dalecurtis for content/renderer/media/
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.
https://codereview.chromium.org/2218963003/diff/1/content/renderer/media/rend... File content/renderer/media/renderer_webmediaplayer_delegate.cc (right): https://codereview.chromium.org/2218963003/diff/1/content/renderer/media/rend... content/renderer/media/renderer_webmediaplayer_delegate.cc:222: base::TimeTicks::Now() - background_video_playing_start_time_); Keep in mind that once you start talking about elapsed real time you are including things like system suspend, etc. So some massive numbers can show up in your data set unexpectedly. If you don't want this the typical thing to do is use a PowerObserver to cancel during suspend.
Added PowerObserver
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...
https://codereview.chromium.org/2218963003/diff/1/content/renderer/media/rend... File content/renderer/media/renderer_webmediaplayer_delegate.cc (right): https://codereview.chromium.org/2218963003/diff/1/content/renderer/media/rend... content/renderer/media/renderer_webmediaplayer_delegate.cc:222: base::TimeTicks::Now() - background_video_playing_start_time_); On 2016/08/05 at 17:39:22, DaleCurtis wrote: > Keep in mind that once you start talking about elapsed real time you are including things like system suspend, etc. So some massive numbers can show up in your data set unexpectedly. If you don't want this the typical thing to do is use a PowerObserver to cancel during suspend. Good point. I assume nothing can happen between OnSuspend and OnResume? I've added some logic to subtract time spend in power suspended mode from the measurements.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Didn't look at the tests yet but apart from the powermonitor usage, it looks good. Also, I wonder if some actions should be marked as non_user_triggered but I will leave this to the metrics review :) https://codereview.chromium.org/2218963003/diff/20001/content/renderer/media/... File content/renderer/media/renderer_webmediaplayer_delegate.cc (right): https://codereview.chromium.org/2218963003/diff/20001/content/renderer/media/... content/renderer/media/renderer_webmediaplayer_delegate.cc:21: content::RenderThread::Get()->RecordAction(action); You could also make it take a std::string and do the base::UserMetricsAction casting in there. https://codereview.chromium.org/2218963003/diff/20001/content/renderer/media/... content/renderer/media/renderer_webmediaplayer_delegate.cc:45: power_suspended_time_ = base::TimeTicks::Now(); I'm a bit confused about this change. If I understand correctly, we want to substract this time from the playback time. However, Suspend state seems to be related to: ApplicationState.HAS_PAUSED_ACTIVITIES (see https://cs.chromium.org/chromium/src/base/android/java/src/org/chromium/base/...) Which seems to be linked with the activity being paused: https://cs.chromium.org/chromium/src/base/android/java/src/org/chromium/base/... My understanding is that the Activity will receive onPause() when it is partially visible and onStop() when fully hidden (see https://developer.android.com/training/basics/activity-lifecycle/pausing.html). It seems that all these events are noise given that we want to record video playback in the background. Dale, does suspend behave differently on other platforms? Given that we are aiming Android, it sounds that we shouldn't actually use PowerMonitor here unless I'm missing something :( https://codereview.chromium.org/2218963003/diff/20001/content/renderer/media/... content/renderer/media/renderer_webmediaplayer_delegate.cc:49: if (is_playing_background_video_) { nit: here and above, you could do an early return. Feel free to ignore. https://codereview.chromium.org/2218963003/diff/20001/content/renderer/media/... File content/renderer/media/renderer_webmediaplayer_delegate_browsertest.cc (right): https://codereview.chromium.org/2218963003/diff/20001/content/renderer/media/... content/renderer/media/renderer_webmediaplayer_delegate_browsertest.cc:49: ~DummyPowerMonitorSource() override {} nit: `= default;` FTW https://codereview.chromium.org/2218963003/diff/20001/tools/metrics/actions/a... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/2218963003/diff/20001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:8501: <description>A renderer media delegate is notified it's hidden.</description> I'm not sure "renderer media delegate" is the right term to use. Unless you know the code very well it will be unclear what this means and I think we want the description to be readable by others than the engineers who work on this code :) https://codereview.chromium.org/2218963003/diff/20001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:8506: <description>Media delegate is requested to pause media.</description> Same as above. It's very unclear what "Media delegate" or "RemoteControls" is in this case. Even as someone who knows the code, what's the difference with this and media session? https://codereview.chromium.org/2218963003/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2218963003/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:22861: + <owner>mlamouri@chromium.org</owner> Maybe add zqzhang@?
https://codereview.chromium.org/2218963003/diff/20001/content/renderer/media/... File content/renderer/media/renderer_webmediaplayer_delegate.cc (right): https://codereview.chromium.org/2218963003/diff/20001/content/renderer/media/... content/renderer/media/renderer_webmediaplayer_delegate.cc:45: power_suspended_time_ = base::TimeTicks::Now(); On 2016/08/08 at 13:19:30, Mounir Lamouri wrote: > I'm a bit confused about this change. If I understand correctly, we want to substract this time from the playback time. However, Suspend state seems to be related to: ApplicationState.HAS_PAUSED_ACTIVITIES (see https://cs.chromium.org/chromium/src/base/android/java/src/org/chromium/base/...) > > Which seems to be linked with the activity being paused: https://cs.chromium.org/chromium/src/base/android/java/src/org/chromium/base/... > > My understanding is that the Activity will receive onPause() when it is partially visible and onStop() when fully hidden (see https://developer.android.com/training/basics/activity-lifecycle/pausing.html). It seems that all these events are noise given that we want to record video playback in the background. > > Dale, does suspend behave differently on other platforms? Given that we are aiming Android, it sounds that we shouldn't actually use PowerMonitor here unless I'm missing something :( Yes, suspend behaves differently on other platforms. I also think this wouldn't work even on those other platforms since you don't know exactly when base::TimeTicks::Now() will start returning the correct time. I don't think it's guaranteed to do so between the bounds of OnSuspend / OnResume -- Instead of subtracting the time you probably want to drop the measurement completely. OnSuspend/OnResume on android are not sent with any real effective meaning for media. The OnSuspend happens ~30 seconds after going into the background, but doesn't actually mean the process is suspended like on other platforms. If you limit this to Android only you can avoid having to worry about this.
https://codereview.chromium.org/2218963003/diff/20001/tools/metrics/actions/a... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/2218963003/diff/20001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:8499: <action name="Media.Hidden"> Is this always a user-initiated action, or can it be programmatically initiated as well? Please either try to write the description in the form of "The user did X", or possibly reconsider whether you really want to record this event as a "user action". Mounir recently went through and marked a number of actions as not_user_triggered because they were interfering with metrics analysis. It would be a shame if these metrics led to similar noise. (Please note that this comment applies to all of the metrics, even though I am only mentioning it here.) https://codereview.chromium.org/2218963003/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2218963003/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:22863: + Time between the moment a backgrounded video is resumed by the user and Hmm, does the user explicitly need to resume the video whenever it is backgrounded? If not, and the video was backgrounded, continued to play, and then was foregrounded again, will that duration be recorded in this histogram?
Removed the PowerObserver logic, revised metrics descriptions
PTAL https://codereview.chromium.org/2218963003/diff/20001/content/renderer/media/... File content/renderer/media/renderer_webmediaplayer_delegate.cc (right): https://codereview.chromium.org/2218963003/diff/20001/content/renderer/media/... content/renderer/media/renderer_webmediaplayer_delegate.cc:21: content::RenderThread::Get()->RecordAction(action); On 2016/08/08 at 13:19:30, Mounir Lamouri wrote: > You could also make it take a std::string and do the base::UserMetricsAction casting in there. Done. I don't think extract_actions.py would extract all the actions correctly though in this case. https://codereview.chromium.org/2218963003/diff/20001/content/renderer/media/... content/renderer/media/renderer_webmediaplayer_delegate.cc:45: power_suspended_time_ = base::TimeTicks::Now(); On 2016/08/08 at 18:26:45, DaleCurtis wrote: > On 2016/08/08 at 13:19:30, Mounir Lamouri wrote: > > I'm a bit confused about this change. If I understand correctly, we want to substract this time from the playback time. However, Suspend state seems to be related to: ApplicationState.HAS_PAUSED_ACTIVITIES (see https://cs.chromium.org/chromium/src/base/android/java/src/org/chromium/base/...) > > > > Which seems to be linked with the activity being paused: https://cs.chromium.org/chromium/src/base/android/java/src/org/chromium/base/... > > > > My understanding is that the Activity will receive onPause() when it is partially visible and onStop() when fully hidden (see https://developer.android.com/training/basics/activity-lifecycle/pausing.html). It seems that all these events are noise given that we want to record video playback in the background. > > > > Dale, does suspend behave differently on other platforms? Given that we are aiming Android, it sounds that we shouldn't actually use PowerMonitor here unless I'm missing something :( > > Yes, suspend behaves differently on other platforms. I also think this wouldn't work even on those other platforms since you don't know exactly when base::TimeTicks::Now() will start returning the correct time. I don't think it's guaranteed to do so between the bounds of OnSuspend / OnResume -- Instead of subtracting the time you probably want to drop the measurement completely. > > OnSuspend/OnResume on android are not sent with any real effective meaning for media. The OnSuspend happens ~30 seconds after going into the background, but doesn't actually mean the process is suspended like on other platforms. > > If you limit this to Android only you can avoid having to worry about this. #ifdef'ed with OS_ANDROID, if that's what you meant. Removed the PowerObserver logic. https://codereview.chromium.org/2218963003/diff/20001/content/renderer/media/... content/renderer/media/renderer_webmediaplayer_delegate.cc:49: if (is_playing_background_video_) { On 2016/08/08 at 13:19:30, Mounir Lamouri wrote: > nit: here and above, you could do an early return. Feel free to ignore. N/A https://codereview.chromium.org/2218963003/diff/20001/tools/metrics/actions/a... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/2218963003/diff/20001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:8499: <action name="Media.Hidden"> On 2016/08/08 at 22:09:10, Ilya Sherman (Away Aug 11-12) wrote: > Is this always a user-initiated action, or can it be programmatically initiated as well? Please either try to write the description in the form of "The user did X", or possibly reconsider whether you really want to record this event as a "user action". Mounir recently went through and marked a number of actions as not_user_triggered because they were interfering with metrics analysis. It would be a shame if these metrics led to similar noise. > > (Please note that this comment applies to all of the metrics, even though I am only mentioning it here.) Rewrote the description. We're trying to only track user interaction here. https://codereview.chromium.org/2218963003/diff/20001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:8501: <description>A renderer media delegate is notified it's hidden.</description> On 2016/08/08 at 13:19:31, Mounir Lamouri wrote: > I'm not sure "renderer media delegate" is the right term to use. Unless you know the code very well it will be unclear what this means and I think we want the description to be readable by others than the engineers who work on this code :) Done. https://codereview.chromium.org/2218963003/diff/20001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:8506: <description>Media delegate is requested to pause media.</description> On 2016/08/08 at 13:19:31, Mounir Lamouri wrote: > Same as above. It's very unclear what "Media delegate" or "RemoteControls" is in this case. Even as someone who knows the code, what's the difference with this and media session? Done. https://codereview.chromium.org/2218963003/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2218963003/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:22861: + <owner>mlamouri@chromium.org</owner> On 2016/08/08 at 13:19:31, Mounir Lamouri wrote: > Maybe add zqzhang@? Done. https://codereview.chromium.org/2218963003/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:22863: + Time between the moment a backgrounded video is resumed by the user and On 2016/08/08 at 22:09:10, Ilya Sherman (Away Aug 11-12) wrote: > Hmm, does the user explicitly need to resume the video whenever it is backgrounded? Yes. Clarified that in the new description. If not, and the video was backgrounded, continued to play, and then was foregrounded again, will that duration be recorded in this histogram? No.
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: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
That's not really what I meant :) It works, but we really shouldn't be building any more Android only features in this code. We've unified the paths and things like this move us away from that again. If you can solve the issue on all platforms I think that'd be better. You might instead consider something similar to how I implemented watch time here https://codereview.chromium.org/2160963002
On 2016/08/12 at 00:39:55, dalecurtis wrote: > That's not really what I meant :) It works, but we really shouldn't be building any more Android only features in this code. We've unified the paths and things like this move us away from that again. > > If you can solve the issue on all platforms I think that'd be better. You might instead consider something similar to how I implemented watch time here https://codereview.chromium.org/2160963002 Sorry, Dale, could you be more specific? WatchTimeReporter in your CL doesn't observe suspend/resume at all. How does it workaround the issue then? If we want to observe those events for background playback and they behave differently on desktop and Android, I'm not sure how we could avoid Android-specific logic. I'm kind of tempted into adding the background time metric to the list of what WatchTimeReporter records :) WMPA would have to have its own logic though but that would be Android only by definition. Ilya, do the metric descriptions look better now?
That CL operates on media time which is invariant under suspend/resume. Mostly I was suggesting it as an example of a more contained monitor for background playback "watch time."
metrics lgtm, thanks. https://codereview.chromium.org/2218963003/diff/40001/content/renderer/media/... File content/renderer/media/renderer_webmediaplayer_delegate.cc (right): https://codereview.chromium.org/2218963003/diff/40001/content/renderer/media/... content/renderer/media/renderer_webmediaplayer_delegate.cc:20: content::RenderThread::Get()->RecordAction(base::UserMetricsAction(action)); The purpose of base::UserMetricsAction is to help the script automatically pick up user actions. However, this only works when it's used with string literals. So, I'd prefer that you drop this wrapper function, so that the script continues to work as intended. Possibly, you could write this helper to accept a base::UserMetricsAction param, and that would suffice to get the script to work -- if so, feel free to do so. https://codereview.chromium.org/2218963003/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2218963003/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:22937: + Android-specific metrics. The time between the moment the backgrounded video nit: s/metrics/metric
Got UserMetricsAction back in action and fixed a typo in the histogram description
https://codereview.chromium.org/2218963003/diff/40001/content/renderer/media/... File content/renderer/media/renderer_webmediaplayer_delegate.cc (right): https://codereview.chromium.org/2218963003/diff/40001/content/renderer/media/... content/renderer/media/renderer_webmediaplayer_delegate.cc:20: content::RenderThread::Get()->RecordAction(base::UserMetricsAction(action)); On 2016/08/17 at 01:17:14, Ilya Sherman wrote: > The purpose of base::UserMetricsAction is to help the script automatically pick up user actions. However, this only works when it's used with string literals. So, I'd prefer that you drop this wrapper function, so that the script continues to work as intended. Possibly, you could write this helper to accept a base::UserMetricsAction param, and that would suffice to get the script to work -- if so, feel free to do so. Got it. It actually helped to catch a mismatch between the name used in .cc and in .xml files so happy to revert to using UserMetricsAction explicitly. https://codereview.chromium.org/2218963003/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2218963003/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:22937: + Android-specific metrics. The time between the moment the backgrounded video On 2016/08/17 at 01:17:14, Ilya Sherman wrote: > nit: s/metrics/metric Done.
lgtm if you want to keep this android only, but I think the metric name should probably be Media.Android.blah then.
See also the discussion on my CL about bucket sizes since you might end up with buckets which aren't particularly relevant for your use case. You may want to use the same range I did to capture common media times.
On 2016/08/17 at 20:37:43, dalecurtis wrote: > See also the discussion on my CL about bucket sizes since you might end up with buckets which aren't particularly relevant for your use case. You may want to use the same range I did to capture common media times. Ok, I changed the buckets to match the ones you used and the name of the histogram to Media.Android.BackgroundVideoTime I filed a follow-up bug to unify metrics collection with desktop and use the media time. Thanks!
Renamed the histogram and changed the buckets. Added a TODO
The CQ bit was checked by avayvod@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from isherman@chromium.org, dalecurtis@chromium.org Link to the patchset: https://codereview.chromium.org/2218963003/#ps80001 (title: "Renamed the histogram and changed the buckets. Added a TODO")
The CQ bit was unchecked by avayvod@chromium.org
Updated the browser test
The CQ bit was checked by avayvod@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from isherman@chromium.org, dalecurtis@chromium.org Link to the patchset: https://codereview.chromium.org/2218963003/#ps100001 (title: "Updated the browser test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/08/17 21:06:26, whywhat wrote: > Renamed the histogram and changed the buckets. Added a TODO This CL still LGTM; but for future reference, please double-check with a metrics reviewer prior to landing changes to a histogram, even if they're just to the bucket layout or name. Both of those are things that I pay attention to in my review. Thanks!
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Added metrics for background video playback. Record the amount of time continouous background video playback happens. Record each time the render frame media is shown/hidden/resumed/paused. Record each time the render frame media is resumed and paused in the background. BUG=576248 TEST=browser test + manual testing ========== to ========== Added metrics for background video playback. Record the amount of time continouous background video playback happens. Record each time the render frame media is shown/hidden/resumed/paused. Record each time the render frame media is resumed and paused in the background. BUG=576248 TEST=browser test + manual testing Committed: https://crrev.com/534a9e9d5c8b52484023821e69736d4ce63bc925 Cr-Commit-Position: refs/heads/master@{#412665} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/534a9e9d5c8b52484023821e69736d4ce63bc925 Cr-Commit-Position: refs/heads/master@{#412665}
Message was sent while issue was closed.
On 2016/08/17 at 21:21:02, isherman wrote: > On 2016/08/17 21:06:26, whywhat wrote: > > Renamed the histogram and changed the buckets. Added a TODO > > This CL still LGTM; but for future reference, please double-check with a metrics reviewer prior to landing changes to a histogram, even if they're just to the bucket layout or name. Both of those are things that I pay attention to in my review. Thanks! Sure! Sorry :) |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
