Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
297773004:
Add UMA to study the effect of defaulting fullscreen video to landscape mode (Closed)
|
Created:
6 years, 7 months ago by qinmin Modified:
6 years, 3 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, jar (doing other things), asvitkine+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd UMA to study the effect of defaulting fullscreen video to landscape mode
Added UMAs for:
1) The % of fullscreen video that is opened in portrait mode
2) The % of the video is suited for portrait mode
3) How long does a video play before user rotate the screen to landscape mode
4) How long video plays after user rotate the screen to landscape mode
5) How long does a video play before user rotate to portrait mode
6) The % of a video that the user switched from portrait to landscape mode
BUG=326572
Committed: https://crrev.com/d81a8daf739624ad9cb9b117d39841ad66a14d34
Cr-Commit-Position: refs/heads/master@{#293045}
Patch Set 1 #
Total comments: 16
Patch Set 2 : filter out orientation locked devices #
Total comments: 4
Patch Set 3 : update comments #
Total comments: 2
Patch Set 4 : nits #
Total comments: 6
Patch Set 5 : removing noise if user quickly switch orientations #
Messages
Total messages: 25 (1 generated)
PTAL
https://codereview.chromium.org/297773004/diff/1/tools/metrics/histograms/his... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/297773004/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:10383: + rotates from portrait to landscape mode. If there is no device rotation or I understand when the timer starts, but it's not clear when it ends. If I rotate the device from portrait to landscape mode, let the video play for a while, then rotate it back and let it play more, what time gets recorded? Does the time stop when I rotate it back, or does it continue as long as the video is playing? Another case to consider: start video in portrait, rotate to landscape, rotate to portrait, rotate to landscape. Does the second rotate from portrait to landscape cause a second emission to this histogram? https://codereview.chromium.org/297773004/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:10393: + playback, only the time period before the switch is accounted. Is this emitted when the rotation event happens or when the video stops playing? i.e., suppose I play video for 5 seconds in landscape mode, then I switch to portrait mode, play for 10 more seconds, and then switch to landscape mode again and play for 20 more seconds. What do you record: * 5 seconds (and nothing else) * 5 seconds and 20 seconds (two separate buckets) * 25 seconds ? Once we agree on a better description for this histogram, we should revise PortraitDuration to be analogous. https://codereview.chromium.org/297773004/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:10401: + Android: Records the device orientation when video enters fullscreen. The Does it have to be playing at the time? Can it, for instance, be paused? https://codereview.chromium.org/297773004/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:10401: + Android: Records the device orientation when video enters fullscreen. The nit: when video -> when a video https://codereview.chromium.org/297773004/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:10415: +<histogram name="MobileFullscreenVideo.PortraitRotation" enum="BooleanEnabled"> You might want to have a LandscapeRotation for comparison purposes. https://codereview.chromium.org/297773004/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:10418: + Android: Records the number of times that fullscreen video is switched from This histogram doesn't record a number as the description states. It records a boolean value. Perhaps you want to replace your entire description with something like: Android: Records whether a fullscreen video is switched from portrait to landscape mode at any point during playback. (I think this covers everything that you tried to say in your original description.) https://codereview.chromium.org/297773004/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:10428: + Android: Records the number of fullscreen video that have a larger height nit: video -> videos https://codereview.chromium.org/297773004/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:10430: + otherwise. This histogram need to explicitly say when it is recorded.
I don't see anything here that controls for users who have a locked screen (i.e., they will be included in the sample of those who don't change the orientation, when they never had that chance).
On 2014/05/22 19:41:19, jdduke wrote: > I don't see anything here that controls for users who have a locked screen > (i.e., they will be included in the sample of those who don't change the > orientation, when they never had that chance). If the screen is locked, SurfaceView.onMeasure() will not get called since the surfaceView's size is locked by its size in portrait mode. And if a user is accustomed to the current fullscreen video behavior, he should expect that rotating video in fullscreen mode doesn't work when screen is locked. So the majority of the user with screen locks will stick to portrait/landscape mode without rotating the device during video playback.
On 2014/05/22 20:20:21, qinmin wrote: > On 2014/05/22 19:41:19, jdduke wrote: > > I don't see anything here that controls for users who have a locked screen > > (i.e., they will be included in the sample of those who don't change the > > orientation, when they never had that chance). > > If the screen is locked, SurfaceView.onMeasure() will not get called since the > surfaceView's size is locked by its size in portrait mode. > And if a user is accustomed to the current fullscreen video behavior, he should > expect that rotating video in fullscreen mode doesn't work when screen is > locked. > So the majority of the user with screen locks will stick to portrait/landscape > mode without rotating the device during video playback. Can we programatically determine if the screen orientation is locked? If so, we probably want to exclude those from the UMA tracking to avoid polluting the data with people that can not "feasibly" rotate their devices. Otherwise, we should track separately the number of people that start a video in a screen locked state and explicitly go and change it and come back to the video (i.e. how bad are we making their experience). Or we could add a button to quickly disable screen locking in the fullscreen video view (for those with the setting set) and allow them to change. Or we just disable the orientation lock without the UI and see the number of people that change orientation themselves.
On 2014/05/22 20:59:38, Ted C wrote: > On 2014/05/22 20:20:21, qinmin wrote: > > On 2014/05/22 19:41:19, jdduke wrote: > > > I don't see anything here that controls for users who have a locked screen > > > (i.e., they will be included in the sample of those who don't change the > > > orientation, when they never had that chance). > > > > If the screen is locked, SurfaceView.onMeasure() will not get called since the > > surfaceView's size is locked by its size in portrait mode. > > And if a user is accustomed to the current fullscreen video behavior, he > should > > expect that rotating video in fullscreen mode doesn't work when screen is > > locked. > > So the majority of the user with screen locks will stick to portrait/landscape > > mode without rotating the device during video playback. > > Can we programatically determine if the screen orientation is locked? If so, we > probably want to exclude those from the UMA tracking to avoid polluting the data > with people that can not "feasibly" rotate their devices. > > Otherwise, we should track separately the number of people that start a video > in a screen locked state and explicitly go and change it and come back to the > video (i.e. how bad are we making their experience). Or we could add a button > to quickly disable screen locking in the fullscreen video view (for those with > the > setting set) and allow them to change. Or we just disable the orientation lock > without the UI and see the number of people that change orientation themselves. Ok, added the filter in ContentVideoView to filter out devices with rotation lock on
https://codereview.chromium.org/297773004/diff/1/tools/metrics/histograms/his... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/297773004/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:10383: + rotates from portrait to landscape mode. If there is no device rotation or Modified the comments a little bit. The recorded time period is the time after the first portrait->landscape rotation happens and before video exits fullscreen. So 2nd rotate from portrait to landscape won't cause an emission. On 2014/05/21 18:17:45, Mark P wrote: > I understand when the timer starts, but it's not clear when it ends. If I > rotate the device from portrait to landscape mode, let the video play for a > while, then rotate it back and let it play more, what time gets recorded? Does > the time stop when I rotate it back, or does it continue as long as the video is > playing? > > Another case to consider: start video in portrait, rotate to landscape, rotate > to portrait, rotate to landscape. Does the second rotate from portrait to > landscape cause a second emission to this histogram? https://codereview.chromium.org/297773004/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:10393: + playback, only the time period before the switch is accounted. Only one record is emitted during one fullscreen playback. So if user plays video for 5 seconds in landscape mode for 5 seconds, and switch back and force between landscape and portrait mode, only the first 5 seconds is recorded. On 2014/05/21 18:17:45, Mark P wrote: > Is this emitted when the rotation event happens or when the video stops playing? > i.e., suppose I play video for 5 seconds in landscape mode, then I switch to > portrait mode, play for 10 more seconds, and then switch to landscape mode again > and play for 20 more seconds. > What do you record: > * 5 seconds (and nothing else) > * 5 seconds and 20 seconds (two separate buckets) > * 25 seconds > ? > > Once we agree on a better description for this histogram, we should revise > PortraitDuration to be analogous. https://codereview.chromium.org/297773004/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:10401: + Android: Records the device orientation when video enters fullscreen. The On 2014/05/21 18:17:45, Mark P wrote: > nit: when video -> when a video Done. https://codereview.chromium.org/297773004/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:10401: + Android: Records the device orientation when video enters fullscreen. The No, video doesn't needs to be playing. Screenshot will be shown if video is paused, so a user may switch between landscape and portrait mode once they saw the screenshot. On 2014/05/21 18:17:45, Mark P wrote: > Does it have to be playing at the time? Can it, for instance, be paused? https://codereview.chromium.org/297773004/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:10415: +<histogram name="MobileFullscreenVideo.PortraitRotation" enum="BooleanEnabled"> On 2014/05/21 18:17:45, Mark P wrote: > You might want to have a LandscapeRotation for comparison purposes. Done. https://codereview.chromium.org/297773004/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:10418: + Android: Records the number of times that fullscreen video is switched from On 2014/05/21 18:17:45, Mark P wrote: > This histogram doesn't record a number as the description states. It records a > boolean value. Perhaps you want to replace your entire description with > something like: > Android: Records whether a fullscreen video is switched from portrait to > landscape mode at any point during playback. > > (I think this covers everything that you tried to say in your original > description.) Done. https://codereview.chromium.org/297773004/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:10428: + Android: Records the number of fullscreen video that have a larger height changed to "a fullscreen video" On 2014/05/21 18:17:45, Mark P wrote: > nit: video -> videos https://codereview.chromium.org/297773004/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:10430: + otherwise. On 2014/05/21 18:17:45, Mark P wrote: > This histogram need to explicitly say when it is recorded. Done.
https://codereview.chromium.org/297773004/diff/20001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/297773004/diff/20001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:10428: + portrait and landscape mode, only one record is emitted. Does this record the whole elapsed time the video is playing, or the time the video is playing until exiting landscape mode? i.e., does it behalf like the last sentence in the next histogram? https://codereview.chromium.org/297773004/diff/20001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:10438: + before the first switch is accounted. If a video starts playing in portrait mode, then the user switches to landscape mode, do we also count those entries? note to self: once we agree on a description, make PortraitDuration read the same.
https://codereview.chromium.org/297773004/diff/20001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/297773004/diff/20001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:10428: + portrait and landscape mode, only one record is emitted. This is equal to the time period from the first switch to the moment when video exits fullscreen. Updated the comment. On 2014/05/23 19:09:14, Mark P wrote: > Does this record the whole elapsed time the video is playing, or the time the > video is playing until exiting landscape mode? > i.e., does it behalf like the last sentence in the next histogram? https://codereview.chromium.org/297773004/diff/20001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:10438: + before the first switch is accounted. no, it was not counted. Added the comment here. On 2014/05/23 19:09:14, Mark P wrote: > If a video starts playing in portrait mode, then the user switches to landscape > mode, do we also count those entries? > > note to self: once we agree on a description, make PortraitDuration read the > same.
histograms.xml lgtm baring one minor comment below https://codereview.chromium.org/297773004/diff/40001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/297773004/diff/40001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:10441: + mode, it is not recorded. Please make PortraitDuration needs analogously.
https://codereview.chromium.org/297773004/diff/40001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/297773004/diff/40001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:10441: + mode, it is not recorded. On 2014/05/23 22:01:40, Mark P wrote: > Please make PortraitDuration needs analogously. Done.
On 2014/05/23 22:09:42, qinmin wrote: > https://codereview.chromium.org/297773004/diff/40001/tools/metrics/histograms... > File tools/metrics/histograms/histograms.xml (right): > > https://codereview.chromium.org/297773004/diff/40001/tools/metrics/histograms... > tools/metrics/histograms/histograms.xml:10441: + mode, it is not recorded. > On 2014/05/23 22:01:40, Mark P wrote: > > Please make PortraitDuration needs analogously. > > Done. Ping?
On 2014/07/11 05:37:03, aurimas wrote: > On 2014/05/23 22:09:42, qinmin wrote: > > > https://codereview.chromium.org/297773004/diff/40001/tools/metrics/histograms... > > File tools/metrics/histograms/histograms.xml (right): > > > > > https://codereview.chromium.org/297773004/diff/40001/tools/metrics/histograms... > > tools/metrics/histograms/histograms.xml:10441: + mode, it is not recorded. > > On 2014/05/23 22:01:40, Mark P wrote: > > > Please make PortraitDuration needs analogously. > > > > Done. > > Ping? Are you pinging me? I LGTMed this change more than a month ago. (I follow what I think is standard Chrome practice of LGTMing a change even if I have comments if I think the comments are minor enough that the person can revise the code in response to the comments and submit it without asking needing further review.) still lgtm --mark
On 2014/07/11 18:10:22, Mark P wrote: > On 2014/07/11 05:37:03, aurimas wrote: > > On 2014/05/23 22:09:42, qinmin wrote: > > > > > > https://codereview.chromium.org/297773004/diff/40001/tools/metrics/histograms... > > > File tools/metrics/histograms/histograms.xml (right): > > > > > > > > > https://codereview.chromium.org/297773004/diff/40001/tools/metrics/histograms... > > > tools/metrics/histograms/histograms.xml:10441: + mode, it is not > recorded. > > > On 2014/05/23 22:01:40, Mark P wrote: > > > > Please make PortraitDuration needs analogously. > > > > > > Done. > > > > Ping? > > Are you pinging me? I LGTMed this change more than a month ago. > > (I follow what I think is standard Chrome practice of LGTMing a change even if I > have comments if I think the comments are minor enough that the person can > revise the code in response to the comments and submit it without asking needing > further review.) > > still lgtm > > --mark I am pinging the owner of the CL - qinmin. I was not sure if he is still working on this.
On 2014/07/11 18:11:11, aurimas wrote: > On 2014/07/11 18:10:22, Mark P wrote: > > On 2014/07/11 05:37:03, aurimas wrote: > > > On 2014/05/23 22:09:42, qinmin wrote: > > > > > > > > > > https://codereview.chromium.org/297773004/diff/40001/tools/metrics/histograms... > > > > File tools/metrics/histograms/histograms.xml (right): > > > > > > > > > > > > > > https://codereview.chromium.org/297773004/diff/40001/tools/metrics/histograms... > > > > tools/metrics/histograms/histograms.xml:10441: + mode, it is not > > recorded. > > > > On 2014/05/23 22:01:40, Mark P wrote: > > > > > Please make PortraitDuration needs analogously. > > > > > > > > Done. > > > > > > Ping? > > > > Are you pinging me? I LGTMed this change more than a month ago. > > > > (I follow what I think is standard Chrome practice of LGTMing a change even if > I > > have comments if I think the comments are minor enough that the person can > > revise the code in response to the comments and submit it without asking > needing > > further review.) > > > > still lgtm > > > > --mark > > I am pinging the owner of the CL - qinmin. I was not sure if he is still working > on this. I need OWNER approval for content/public/android and content/browser/android. BTW, there is currently a new issue to default video into landscape mode. With new fullscreen video implementation, media controls are no longer attached to the contentVideoView. Instead, they are drawn on the page content. As a result, when rotating the video into landscape mode, we also need to rotate the whole tab, and rotate them back when leaving fullscreen. Not sure whether the animations and the transitions will be smooth during these steps.
I don't think it hurts to collect these stats and then we can reason how we would implement this. On Fri Jul 11 2014 at 11:27:36 AM, <qinmin@chromium.org> wrote: > On 2014/07/11 18:11:11, aurimas wrote: > > On 2014/07/11 18:10:22, Mark P wrote: > > > On 2014/07/11 05:37:03, aurimas wrote: > > > > On 2014/05/23 22:09:42, qinmin wrote: > > > > > > > > > > > > > > https://codereview.chromium.org/297773004/diff/40001/ > tools/metrics/histograms/histograms.xml > > > > > File tools/metrics/histograms/histograms.xml (right): > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/297773004/diff/40001/ > tools/metrics/histograms/histograms.xml#newcode10441 > > > > > tools/metrics/histograms/histograms.xml:10441: + mode, it is > not > > > recorded. > > > > > On 2014/05/23 22:01:40, Mark P wrote: > > > > > > Please make PortraitDuration needs analogously. > > > > > > > > > > Done. > > > > > > > > Ping? > > > > > > Are you pinging me? I LGTMed this change more than a month ago. > > > > > > (I follow what I think is standard Chrome practice of LGTMing a change > > even > if > > I > > > have comments if I think the comments are minor enough that the person > > can > > > revise the code in response to the comments and submit it without > asking > > needing > > > further review.) > > > > > > still lgtm > > > > > > --mark > > > I am pinging the owner of the CL - qinmin. I was not sure if he is still > working > > on this. > > I need OWNER approval for content/public/android and > content/browser/android. > BTW, there is currently a new issue to default video into landscape mode. > With new fullscreen video implementation, media controls are no longer > attached > to the contentVideoView. Instead, they are drawn on the page content. > As a result, when rotating the video into landscape mode, we also need to > rotate > the whole tab, and rotate them back when leaving fullscreen. > Not sure whether the animations and the transitions will be smooth during > these > steps. > > > https://codereview.chromium.org/297773004/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Ted, would you please take a look at this?
overall looks fine...just wondering about the accidental noise and whether you think that is an issue. https://codereview.chromium.org/297773004/diff/60001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java (right): https://codereview.chromium.org/297773004/diff/60001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java:127: mOrientationChangedTime = System.currentTimeMillis(); What about the case where you accidentally change the orientation and then quickly switch back to the original one? Just wondering if there is a way to limit the noise we'll get from this. https://codereview.chromium.org/297773004/diff/60001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java:296: return; -2 indent https://codereview.chromium.org/297773004/diff/60001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java:516: if (outputSize.x > outputSize.y) { I would just: return outputSize.x < outputSize.y;
On 2014/07/23 16:13:59, Ted C wrote: > overall looks fine...just wondering about the accidental noise and whether you > think that is an issue. > > https://codereview.chromium.org/297773004/diff/60001/content/public/android/j... > File > content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java > (right): > > https://codereview.chromium.org/297773004/diff/60001/content/public/android/j... > content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java:127: > mOrientationChangedTime = System.currentTimeMillis(); > What about the case where you accidentally change the orientation and then > quickly switch back to the original one? Just wondering if there is a way to > limit the noise we'll get from this. > > https://codereview.chromium.org/297773004/diff/60001/content/public/android/j... > content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java:296: > return; > -2 indent > > https://codereview.chromium.org/297773004/diff/60001/content/public/android/j... > content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java:516: > if (outputSize.x > outputSize.y) { > I would just: > > return outputSize.x < outputSize.y; qinmin: any progress on this?
https://codereview.chromium.org/297773004/diff/60001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java (right): https://codereview.chromium.org/297773004/diff/60001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java:127: mOrientationChangedTime = System.currentTimeMillis(); ok, added a check here, if user switch the orientation back and force within 5 seconds, then we will not record the UMA on how long the user spent in each orientation. However, there are still lots of ways to generate other noises. I think we just need a rough estimation here, no need to be very accurate. On 2014/07/23 16:13:59, Ted C wrote: > What about the case where you accidentally change the orientation and then > quickly switch back to the original one? Just wondering if there is a way to > limit the noise we'll get from this. https://codereview.chromium.org/297773004/diff/60001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java:296: return; On 2014/07/23 16:13:59, Ted C wrote: > -2 indent Done. https://codereview.chromium.org/297773004/diff/60001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java:516: if (outputSize.x > outputSize.y) { On 2014/07/23 16:13:59, Ted C wrote: > I would just: > > return outputSize.x < outputSize.y; Done.
lgtm
The CQ bit was checked by qinmin@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/qinmin@chromium.org/297773004/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as 100daed61c833daab596536046bbd24133194e2c
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/d81a8daf739624ad9cb9b117d39841ad66a14d34 Cr-Commit-Position: refs/heads/master@{#293045} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
