Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2814043005:
media: Report initial video height to UMA (Closed)
|
Created:
3 years, 8 months ago by xhwang Modified:
3 years, 8 months ago CC:
apacible+watch_chromium.org, asvitkine+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/heads/master Project:
chromium Visibility:
Public. |
Descriptionmedia: Report initial video height to UMA
Also fix the issue where the initial video natural size is
not reported to MediaLog.
BUG=709354
TEST=Manually tested
Review-Url: https://codereview.chromium.org/2814043005
Cr-Commit-Position: refs/heads/master@{#465518}
Committed: https://chromium.googlesource.com/chromium/src/+/60802656141458897882b4283908e75bb444b420
Patch Set 1 #
Total comments: 8
Patch Set 2 : comments #
Total comments: 7
Patch Set 3 : average #
Total comments: 3
Patch Set 4 : reabse and owner #
Total comments: 15
Patch Set 5 : comments addressed #Patch Set 6 : remove "average" #
Total comments: 2
Messages
Total messages: 66 (33 generated)
The CQ bit was checked by xhwang@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: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== media: Report initial video height to UMA Also fix the code so that we alwasy report the natural size to MediaLog. BUG=709354 TEST=Manually tested ========== to ========== media: Report initial video height to UMA Also fix the issue where the initial video natural size is not reported to MediaLog. BUG=709354 TEST=Manually tested ==========
xhwang@chromium.org changed reviewers: + dalecurtis@chromium.org
The CQ bit was checked by xhwang@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...
Patchset #1 (id:1) has been deleted
dalecurtis: PTAL
https://codereview.chromium.org/2814043005/diff/20001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2814043005/diff/20001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:1411: RecordVideoNaturalSize(rotated_size); Put below l.1415 to avoid duplicate size events? https://codereview.chromium.org/2814043005/diff/20001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:2360: // TODO(xhwang): Also report "average" video height for this playback session. Why not just record every size change? Why the focus on initial?
https://codereview.chromium.org/2814043005/diff/20001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2814043005/diff/20001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:2369: // For vertical videos, use it's width instead of height for UMA reporting. Hmmm, why? Seems like this might muddy the metrics? https://codereview.chromium.org/2814043005/diff/20001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:2381: UMA_HISTOGRAM_VIDEO_HEIGHT("Media.VideoHeight.Initial.ALL", height); All
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by xhwang@chromium.org to run a CQ dry run
Thanks for the comments. PTAL again! https://codereview.chromium.org/2814043005/diff/20001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2814043005/diff/20001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:1411: RecordVideoNaturalSize(rotated_size); On 2017/04/14 18:30:19, DaleCurtis wrote: > Put below l.1415 to avoid duplicate size events? OnVideoNaturalSizeChange() should only be called on the first frame and on size change. So the check on l.1414 is really about the first frame where the natural_size could already be set in OnMetadata (I think). Currently, because we call media_log_->AddEvent(...) after the check, we are not reporting the natural size for the first frame. It'll only be reported after a the first config change. This is not ideal since players using MSE might not have config change in long time, and then we won't see width/height in media-internals for long time. Then, after the first frame, since OnVideoNaturalSizeChange() should only be called when natural size changes [1], we should not have duplicate size events with the same size. If it's not the case, it seems we have something else to fix. [1] https://cs.chromium.org/chromium/src/media/base/pipeline.h?rcl=c6b447c44b838f... https://codereview.chromium.org/2814043005/diff/20001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:2360: // TODO(xhwang): Also report "average" video height for this playback session. On 2017/04/14 18:30:19, DaleCurtis wrote: > Why not just record every size change? Why the focus on initial? As discussed offline, this is useful to measure the "first impression". https://codereview.chromium.org/2814043005/diff/20001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:2369: // For vertical videos, use it's width instead of height for UMA reporting. On 2017/04/14 18:48:57, DaleCurtis wrote: > Hmmm, why? Seems like this might muddy the metrics? I was debating on this myself. Unless we report the area (w x h), there's no clean solution. So I agree with you maybe just take the simplest solution. Maybe we should have separate metrics for vertical videos. But that's a much lower priority. https://codereview.chromium.org/2814043005/diff/20001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:2381: UMA_HISTOGRAM_VIDEO_HEIGHT("Media.VideoHeight.Initial.ALL", height); On 2017/04/14 18:48:57, DaleCurtis wrote: > All Done. Thanks for catching this!
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
tguilbert@chromium.org changed reviewers: + tguilbert@chromium.org
https://codereview.chromium.org/2814043005/diff/40001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2814043005/diff/40001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:2363: if (initial_video_height_recorded_) Drive-by: For HLS, our original video size is always 0, because we do not know the video size until we start playing. I am not sure if the initial size would be reported correctly in this case.
https://codereview.chromium.org/2814043005/diff/40001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2814043005/diff/40001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:84: #define UMA_HISTOGRAM_VIDEO_HEIGHT(name, sample) \ Define near where used and then use #undef afterward. https://codereview.chromium.org/2814043005/diff/40001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:2360: // TODO(xhwang): Also report "average" video height for this playback session. Want to go ahead and just add this now?
https://codereview.chromium.org/2814043005/diff/40001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2814043005/diff/40001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:2363: if (initial_video_height_recorded_) On 2017/04/14 22:00:49, tguilbert wrote: > Drive-by: For HLS, our original video size is always 0, because we do not know > the video size until we start playing. I am not sure if the initial size would > be reported correctly in this case. Good point. Will OnVideoNaturalSizeChange() be called with 0 size in the HLS case? If so, we can define the "initial size" to be the "first non-zero size", and adjust the code to accommodate this case.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Also I just changed that code here :) https://codereview.chromium.org/2822543006/
comment-only https://codereview.chromium.org/2814043005/diff/40001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2814043005/diff/40001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:84: #define UMA_HISTOGRAM_VIDEO_HEIGHT(name, sample) \ On 2017/04/14 22:03:38, DaleCurtis wrote: > Define near where used and then use #undef afterward. Done. https://codereview.chromium.org/2814043005/diff/40001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:2360: // TODO(xhwang): Also report "average" video height for this playback session. On 2017/04/14 22:03:38, DaleCurtis wrote: > Want to go ahead and just add this now? I want to merge this back to M59 to collect data early, so I want to keep this CL simple. The average video height might need to get MediaInternalsUMAHandler involved since we will only have the final "average" value after playback finished.
The CQ bit was checked by xhwang@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...
xhwang@chromium.org changed reviewers: + isherman@chromium.org
xhwang@chromium.org changed reviewers: + isherman@chromium.org
Add "average". PTAL again! https://codereview.chromium.org/2814043005/diff/40001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2814043005/diff/40001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:2360: // TODO(xhwang): Also report "average" video height for this playback session. On 2017/04/14 22:18:06, xhwang_slow wrote: > On 2017/04/14 22:03:38, DaleCurtis wrote: > > Want to go ahead and just add this now? > > I want to merge this back to M59 to collect data early, so I want to keep this > CL simple. The average video height might need to get MediaInternalsUMAHandler > involved since we will only have the final "average" value after playback > finished. Done.
Add "average". PTAL again! https://codereview.chromium.org/2814043005/diff/40001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2814043005/diff/40001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:2360: // TODO(xhwang): Also report "average" video height for this playback session. On 2017/04/14 22:18:06, xhwang_slow wrote: > On 2017/04/14 22:03:38, DaleCurtis wrote: > > Want to go ahead and just add this now? > > I want to merge this back to M59 to collect data early, so I want to keep this > CL simple. The average video height might need to get MediaInternalsUMAHandler > involved since we will only have the final "average" value after playback > finished. Done.
isherman: Please UMA OWNERS review!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
media/ lgtm https://codereview.chromium.org/2814043005/diff/60001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2814043005/diff/60001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:1416: // WatchTimeReporter doesn't report metrics for empty videos. Re-create Needs rebase. https://codereview.chromium.org/2814043005/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2814043005/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:28400: + <owner>xhwang@chromium.org</owner> mlamouri@ suggested adding media-dev@chromium.org for new metrics owners. WDYT?
The CQ bit was checked by xhwang@chromium.org to run a CQ dry run
https://codereview.chromium.org/2814043005/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2814043005/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:28400: + <owner>xhwang@chromium.org</owner> On 2017/04/17 19:05:59, DaleCurtis wrote: > mlamouri@ suggested adding mailto:media-dev@chromium.org for new metrics owners. WDYT? SGTM!
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
isherman: Please UMA OWNERS review!
https://codereview.chromium.org/2814043005/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2814043005/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:28411: +<histogram name="Media.VideoHeight.Average" units="pixels"> nit: Please add the attribute base="true", since this name is only ever a prefix to an actually recorded histogram name. https://codereview.chromium.org/2814043005/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:28415: + video frame size is available (e.g. during MSE config change). Hmm, the average is updated whenever the size changes? Won't that result in weirdly weighted data, where videos with many frame size changes report many more samples to this metric? https://codereview.chromium.org/2814043005/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:28419: +<histogram name="Media.VideoHeight.Initial" units="pixels"> nit: Please add the attribute base="true", since this name is only ever a prefix to an actually recorded histogram name. https://codereview.chromium.org/2814043005/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:120495: + <affected-histogram name="Media.VideoHeight.Initial"/> You are currently adding almost 1000 buckets-worth of histograms, across these 8 histograms of 120 buckets each. On the bug, you mentioned that you might be able to use ~50 buckets per histogram. Is that still feasible? Reducing the number of buckets used for these by a factor of two seems worthwhile if the data provided would be comparably good.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
comments addressed
isherman: Please see the change and reply to your comments. Thanks! https://codereview.chromium.org/2814043005/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2814043005/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:28411: +<histogram name="Media.VideoHeight.Average" units="pixels"> On 2017/04/17 20:10:14, Ilya Sherman wrote: > nit: Please add the attribute base="true", since this name is only ever a prefix > to an actually recorded histogram name. Done. https://codereview.chromium.org/2814043005/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:28415: + video frame size is available (e.g. during MSE config change). On 2017/04/17 20:10:14, Ilya Sherman wrote: > Hmm, the average is updated whenever the size changes? Won't that result in > weirdly weighted data, where videos with many frame size changes report many > more samples to this metric? See previous discussion on this in the code review. We discussed about playback-time weighted resolution over the entire playback instance, but felt that it's probably over-kill for our purpose. Also, even if we report a "playback-time weighted resolution" per playback, it's still not a fair comparison. For example, you could be watching 1080p for 2 hours in one playback instance, and 480p for 2 minutes in another. Then reporting one sample of 1080 and another sample of 480 won't really tell us the real story... So, this is really reporting all sizes we encountered during playbacks. I thought about s/Average/All, but Media.VideoHeight.All.All would be more confusing... https://codereview.chromium.org/2814043005/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:28419: +<histogram name="Media.VideoHeight.Initial" units="pixels"> On 2017/04/17 20:10:14, Ilya Sherman wrote: > nit: Please add the attribute base="true", since this name is only ever a prefix > to an actually recorded histogram name. Done. https://codereview.chromium.org/2814043005/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:120495: + <affected-histogram name="Media.VideoHeight.Initial"/> On 2017/04/17 20:10:14, Ilya Sherman wrote: > You are currently adding almost 1000 buckets-worth of histograms, across these 8 > histograms of 120 buckets each. On the bug, you mentioned that you might be > able to use ~50 buckets per histogram. Is that still feasible? Reducing the > number of buckets used for these by a factor of two seems worthwhile if the data > provided would be comparably good. When I said ~50 buckets, it's for UMA_HISTOGRAM_CUSTOM_ENUMERATION. But after I played it more, I felt it's probably easier and cleaner to just use a normal histogram. Then I played with the range and bucket number a bit more and found 120 buckets would give us the granularity we'd like to have. I can probably tweak it more to use less buckets, but probably cannot reduce it to ~50. So, how sensitive are we on adding 1000 buckets-worth of histograms, vs, say, 500 buckets-worth of histograms. Will that make a huge difference?
https://codereview.chromium.org/2814043005/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2814043005/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:28415: + video frame size is available (e.g. during MSE config change). On 2017/04/18 05:52:37, xhwang_slow wrote: > On 2017/04/17 20:10:14, Ilya Sherman wrote: > > Hmm, the average is updated whenever the size changes? Won't that result in > > weirdly weighted data, where videos with many frame size changes report many > > more samples to this metric? > > See previous discussion on this in the code review. We discussed about > playback-time weighted resolution over the entire playback instance, but felt > that it's probably over-kill for our purpose. Also, even if we report a > "playback-time weighted resolution" per playback, it's still not a fair > comparison. For example, you could be watching 1080p for 2 hours in one playback > instance, and 480p for 2 minutes in another. Then reporting one sample of 1080 > and another sample of 480 won't really tell us the real story... > > So, this is really reporting all sizes we encountered during playbacks. I > thought about s/Average/All, but Media.VideoHeight.All.All would be more > confusing... Could you please help me to understand how you plan to use this metric? I think I'll be able to offer better guidance if I better understand your intended use for it. https://codereview.chromium.org/2814043005/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:120495: + <affected-histogram name="Media.VideoHeight.Initial"/> On 2017/04/18 05:52:36, xhwang_slow wrote: > On 2017/04/17 20:10:14, Ilya Sherman wrote: > > You are currently adding almost 1000 buckets-worth of histograms, across these > 8 > > histograms of 120 buckets each. On the bug, you mentioned that you might be > > able to use ~50 buckets per histogram. Is that still feasible? Reducing the > > number of buckets used for these by a factor of two seems worthwhile if the > data > > provided would be comparably good. > > When I said ~50 buckets, it's for UMA_HISTOGRAM_CUSTOM_ENUMERATION. But after I > played it more, I felt it's probably easier and cleaner to just use a normal > histogram. Then I played with the range and bucket number a bit more and found > 120 buckets would give us the granularity we'd like to have. I can probably > tweak it more to use less buckets, but probably cannot reduce it to ~50. > > So, how sensitive are we on adding 1000 buckets-worth of histograms, vs, say, > 500 buckets-worth of histograms. Will that make a huge difference? It makes a fairly significant difference, yes. What's the reason to disprefer UMA_HISTOGRAM_CUSTOM_ENUMERATION?
Comments-only. https://codereview.chromium.org/2814043005/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2814043005/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:28415: + video frame size is available (e.g. during MSE config change). On 2017/04/18 05:58:54, Ilya Sherman wrote: > On 2017/04/18 05:52:37, xhwang_slow wrote: > > On 2017/04/17 20:10:14, Ilya Sherman wrote: > > > Hmm, the average is updated whenever the size changes? Won't that result in > > > weirdly weighted data, where videos with many frame size changes report many > > > more samples to this metric? > > > > See previous discussion on this in the code review. We discussed about > > playback-time weighted resolution over the entire playback instance, but felt > > that it's probably over-kill for our purpose. Also, even if we report a > > "playback-time weighted resolution" per playback, it's still not a fair > > comparison. For example, you could be watching 1080p for 2 hours in one > playback > > instance, and 480p for 2 minutes in another. Then reporting one sample of 1080 > > and another sample of 480 won't really tell us the real story... > > > > So, this is really reporting all sizes we encountered during playbacks. I > > thought about s/Average/All, but Media.VideoHeight.All.All would be more > > confusing... > > Could you please help me to understand how you plan to use this metric? I think > I'll be able to offer better guidance if I better understand your intended use > for it. The original intention was to use this as the indication of "average video resolution" across all playback instances. Ideally, we want to get something like sum_over_all_playabacks(resolution * playback_time_on_this_resolution) / sum_over_all_playabacks(playback_time). One way to do this is by sampling, e.g. we report resolution every 1 or 10 minute. Will that be something you would prefer? Or do you have better suggestions? Getting playtime involved would be more complicated in terms of implementation. https://codereview.chromium.org/2814043005/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:120495: + <affected-histogram name="Media.VideoHeight.Initial"/> On 2017/04/18 05:58:54, Ilya Sherman wrote: > On 2017/04/18 05:52:36, xhwang_slow wrote: > > On 2017/04/17 20:10:14, Ilya Sherman wrote: > > > You are currently adding almost 1000 buckets-worth of histograms, across > these > > 8 > > > histograms of 120 buckets each. On the bug, you mentioned that you might be > > > able to use ~50 buckets per histogram. Is that still feasible? Reducing > the > > > number of buckets used for these by a factor of two seems worthwhile if the > > data > > > provided would be comparably good. > > > > When I said ~50 buckets, it's for UMA_HISTOGRAM_CUSTOM_ENUMERATION. But after > I > > played it more, I felt it's probably easier and cleaner to just use a normal > > histogram. Then I played with the range and bucket number a bit more and found > > 120 buckets would give us the granularity we'd like to have. I can probably > > tweak it more to use less buckets, but probably cannot reduce it to ~50. > > > > So, how sensitive are we on adding 1000 buckets-worth of histograms, vs, say, > > 500 buckets-worth of histograms. Will that make a huge difference? > > It makes a fairly significant difference, yes. What's the reason to disprefer > UMA_HISTOGRAM_CUSTOM_ENUMERATION? Because without seeing the real world data, it's hard to come up with a good custom range. You can check the current Media.VideoVisibleWidth UMA (BTW, after I land the current CL, we can probably drop that), where most of the reports are concentrated in some buckets. Tweaks all the buckets based on some theoretical assumption ends up pointless to me. I didn't know 120 buckets vs 50 would make a big difference. So I played with it a bit more, I think I can do UMA_HISTOGRAM_CUSTOM_COUNTS(name, sample, 100, 10000, 50), which will give us the following ranges: 0, 100, 110, 121, 133, 146, 161, 177, 195, 215, 237, 261, 287, 316, 348, 383, 422, 464, 511, 562, 619, 681, 750, 826, 909, 1001, 1102, 1213, 1335, 1469, 1617, 1780, 1959, 2156, 2373, 2612, 2875, 3164, 3482, 3832, 4218, 4643, 5110, 5624, 6190, 6813, 7499, 8254, 9085, 10000, 2147483647 Note that 320p will be in [316, 348), 480p will be in [464, 511), 720p will be in [681, 750), and 1080p will be in [1001, 1102). It's probably hard to answer exact questions like what's the number of playbacks on 1080p+, but we should have good estimates. Dale/Ilya: Does this look good to you?
nit: 360p, don't know of any 320p :) https://codereview.chromium.org/2814043005/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2814043005/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:28415: + video frame size is available (e.g. during MSE config change). On 2017/04/18 at 06:39:50, xhwang_slow wrote: > On 2017/04/18 05:58:54, Ilya Sherman wrote: > > On 2017/04/18 05:52:37, xhwang_slow wrote: > > > On 2017/04/17 20:10:14, Ilya Sherman wrote: > > > > Hmm, the average is updated whenever the size changes? Won't that result in > > > > weirdly weighted data, where videos with many frame size changes report many > > > > more samples to this metric? > > > > > > See previous discussion on this in the code review. We discussed about > > > playback-time weighted resolution over the entire playback instance, but felt > > > that it's probably over-kill for our purpose. Also, even if we report a > > > "playback-time weighted resolution" per playback, it's still not a fair > > > comparison. For example, you could be watching 1080p for 2 hours in one > > playback > > > instance, and 480p for 2 minutes in another. Then reporting one sample of 1080 > > > and another sample of 480 won't really tell us the real story... > > > > > > So, this is really reporting all sizes we encountered during playbacks. I > > > thought about s/Average/All, but Media.VideoHeight.All.All would be more > > > confusing... > > > > Could you please help me to understand how you plan to use this metric? I think > > I'll be able to offer better guidance if I better understand your intended use > > for it. > > The original intention was to use this as the indication of "average video resolution" across all playback instances. Ideally, we want to get something like sum_over_all_playabacks(resolution * playback_time_on_this_resolution) / sum_over_all_playabacks(playback_time). One way to do this is by sampling, e.g. we report resolution every 1 or 10 minute. Will that be something you would prefer? Or do you have better suggestions? Getting playtime involved would be more complicated in terms of implementation. we could use a base::flat_set to ensure we only report a given height once; this avoids the overcounting issue.
On 2017/04/18 17:46:22, DaleCurtis wrote: > nit: 360p, don't know of any 320p :) oops, thanks for the catch. 360p should be in [348, 383) bucket, which also seems good. > https://codereview.chromium.org/2814043005/diff/80001/tools/metrics/histogram... > File tools/metrics/histograms/histograms.xml (right): > > https://codereview.chromium.org/2814043005/diff/80001/tools/metrics/histogram... > tools/metrics/histograms/histograms.xml:28415: + video frame size is > available (e.g. during MSE config change). > On 2017/04/18 at 06:39:50, xhwang_slow wrote: > > On 2017/04/18 05:58:54, Ilya Sherman wrote: > > > On 2017/04/18 05:52:37, xhwang_slow wrote: > > > > On 2017/04/17 20:10:14, Ilya Sherman wrote: > > > > > Hmm, the average is updated whenever the size changes? Won't that > result in > > > > > weirdly weighted data, where videos with many frame size changes report > many > > > > > more samples to this metric? > > > > > > > > See previous discussion on this in the code review. We discussed about > > > > playback-time weighted resolution over the entire playback instance, but > felt > > > > that it's probably over-kill for our purpose. Also, even if we report a > > > > "playback-time weighted resolution" per playback, it's still not a fair > > > > comparison. For example, you could be watching 1080p for 2 hours in one > > > playback > > > > instance, and 480p for 2 minutes in another. Then reporting one sample of > 1080 > > > > and another sample of 480 won't really tell us the real story... > > > > > > > > So, this is really reporting all sizes we encountered during playbacks. I > > > > thought about s/Average/All, but Media.VideoHeight.All.All would be more > > > > confusing... > > > > > > Could you please help me to understand how you plan to use this metric? I > think > > > I'll be able to offer better guidance if I better understand your intended > use > > > for it. > > > > The original intention was to use this as the indication of "average video > resolution" across all playback instances. Ideally, we want to get something > like sum_over_all_playabacks(resolution * playback_time_on_this_resolution) / > sum_over_all_playabacks(playback_time). One way to do this is by sampling, e.g. > we report resolution every 1 or 10 minute. Will that be something you would > prefer? Or do you have better suggestions? Getting playtime involved would be > more complicated in terms of implementation. > > we could use a base::flat_set to ensure we only report a given height once; this > avoids the overcounting issue. It does avoid the overcounting issue, but what exactly are we collecting? If it's hard for us to describe, it'll be harder for other people to use this UMA in a sensible way. It seems we need more discussion about the "average" UMA. May I suggest we drop the "average" ones from this CL, as I originally did, given I want to merge this CL back to M59? Then we can take time to discuss and implement the "average" ones in trunk.
sgtm, I didn't expect the repeated logging to be contentious. As discussed offline though, I think initial height is going to always be wrong for Netflix since they seem to defer to ABR always.
Narrowing this CL to only report initial height SGTM2. https://codereview.chromium.org/2814043005/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2814043005/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:120495: + <affected-histogram name="Media.VideoHeight.Initial"/> On 2017/04/18 06:39:50, xhwang_slow wrote: > On 2017/04/18 05:58:54, Ilya Sherman wrote: > > On 2017/04/18 05:52:36, xhwang_slow wrote: > > > On 2017/04/17 20:10:14, Ilya Sherman wrote: > > > > You are currently adding almost 1000 buckets-worth of histograms, across > > these > > > 8 > > > > histograms of 120 buckets each. On the bug, you mentioned that you might > be > > > > able to use ~50 buckets per histogram. Is that still feasible? Reducing > > the > > > > number of buckets used for these by a factor of two seems worthwhile if > the > > > data > > > > provided would be comparably good. > > > > > > When I said ~50 buckets, it's for UMA_HISTOGRAM_CUSTOM_ENUMERATION. But > after > > I > > > played it more, I felt it's probably easier and cleaner to just use a normal > > > histogram. Then I played with the range and bucket number a bit more and > found > > > 120 buckets would give us the granularity we'd like to have. I can probably > > > tweak it more to use less buckets, but probably cannot reduce it to ~50. > > > > > > So, how sensitive are we on adding 1000 buckets-worth of histograms, vs, > say, > > > 500 buckets-worth of histograms. Will that make a huge difference? > > > > It makes a fairly significant difference, yes. What's the reason to disprefer > > UMA_HISTOGRAM_CUSTOM_ENUMERATION? > > Because without seeing the real world data, it's hard to come up with a good > custom range. You can check the current Media.VideoVisibleWidth UMA (BTW, after > I land the current CL, we can probably drop that), where most of the reports are > concentrated in some buckets. Tweaks all the buckets based on some theoretical > assumption ends up pointless to me. > > I didn't know 120 buckets vs 50 would make a big difference. So I played with it > a bit more, I think I can do > UMA_HISTOGRAM_CUSTOM_COUNTS(name, sample, 100, 10000, 50), which will give us > the following ranges: > > 0, 100, 110, 121, 133, 146, 161, 177, 195, 215, 237, 261, 287, 316, 348, 383, > 422, 464, 511, 562, 619, 681, 750, 826, 909, 1001, 1102, 1213, 1335, 1469, 1617, > 1780, 1959, 2156, 2373, 2612, 2875, 3164, 3482, 3832, 4218, 4643, 5110, 5624, > 6190, 6813, 7499, 8254, 9085, 10000, 2147483647 > > Note that 320p will be in [316, 348), 480p will be in [464, 511), 720p will be > in [681, 750), and 1080p will be in [1001, 1102). It's probably hard to answer > exact questions like what's the number of playbacks on 1080p+, but we should > have good estimates. > > Dale/Ilya: Does this look good to you? Thanks. To be clear, if 120 buckets gives significantly better data, then using 120 buckets is fine. But if 50 buckets gives roughly comparable-quality data, then 50 buckets is definitely preferred.
https://codereview.chromium.org/2814043005/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2814043005/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:28415: + video frame size is available (e.g. during MSE config change). On 2017/04/18 17:46:22, DaleCurtis wrote: > On 2017/04/18 at 06:39:50, xhwang_slow wrote: > > On 2017/04/18 05:58:54, Ilya Sherman wrote: > > > On 2017/04/18 05:52:37, xhwang_slow wrote: > > > > On 2017/04/17 20:10:14, Ilya Sherman wrote: > > > > > Hmm, the average is updated whenever the size changes? Won't that > result in > > > > > weirdly weighted data, where videos with many frame size changes report > many > > > > > more samples to this metric? > > > > > > > > See previous discussion on this in the code review. We discussed about > > > > playback-time weighted resolution over the entire playback instance, but > felt > > > > that it's probably over-kill for our purpose. Also, even if we report a > > > > "playback-time weighted resolution" per playback, it's still not a fair > > > > comparison. For example, you could be watching 1080p for 2 hours in one > > > playback > > > > instance, and 480p for 2 minutes in another. Then reporting one sample of > 1080 > > > > and another sample of 480 won't really tell us the real story... > > > > > > > > So, this is really reporting all sizes we encountered during playbacks. I > > > > thought about s/Average/All, but Media.VideoHeight.All.All would be more > > > > confusing... > > > > > > Could you please help me to understand how you plan to use this metric? I > think > > > I'll be able to offer better guidance if I better understand your intended > use > > > for it. > > > > The original intention was to use this as the indication of "average video > resolution" across all playback instances. Ideally, we want to get something > like sum_over_all_playabacks(resolution * playback_time_on_this_resolution) / > sum_over_all_playabacks(playback_time). One way to do this is by sampling, e.g. > we report resolution every 1 or 10 minute. Will that be something you would > prefer? Or do you have better suggestions? Getting playtime involved would be > more complicated in terms of implementation. > > we could use a base::flat_set to ensure we only report a given height once; this > avoids the overcounting issue. Sampling seems like a reasonable solution to me. (You'd probably want to randomize it a bit, in case some videos do something weird.) If your goal is just to see what heights are ever used, then reporting just once is okay too. But in that case it probably makes sense to rename the histogram to match the use case.
The CQ bit was checked by xhwang@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...
On 2017/04/19 00:32:50, Ilya Sherman wrote: > Narrowing this CL to only report initial height SGTM2. > > https://codereview.chromium.org/2814043005/diff/80001/tools/metrics/histogram... > File tools/metrics/histograms/histograms.xml (right): > > https://codereview.chromium.org/2814043005/diff/80001/tools/metrics/histogram... > tools/metrics/histograms/histograms.xml:120495: + <affected-histogram > name="Media.VideoHeight.Initial"/> > On 2017/04/18 06:39:50, xhwang_slow wrote: > > On 2017/04/18 05:58:54, Ilya Sherman wrote: > > > On 2017/04/18 05:52:36, xhwang_slow wrote: > > > > On 2017/04/17 20:10:14, Ilya Sherman wrote: > > > > > You are currently adding almost 1000 buckets-worth of histograms, across > > > these > > > > 8 > > > > > histograms of 120 buckets each. On the bug, you mentioned that you > might > > be > > > > > able to use ~50 buckets per histogram. Is that still feasible? > Reducing > > > the > > > > > number of buckets used for these by a factor of two seems worthwhile if > > the > > > > data > > > > > provided would be comparably good. > > > > > > > > When I said ~50 buckets, it's for UMA_HISTOGRAM_CUSTOM_ENUMERATION. But > > after > > > I > > > > played it more, I felt it's probably easier and cleaner to just use a > normal > > > > histogram. Then I played with the range and bucket number a bit more and > > found > > > > 120 buckets would give us the granularity we'd like to have. I can > probably > > > > tweak it more to use less buckets, but probably cannot reduce it to ~50. > > > > > > > > So, how sensitive are we on adding 1000 buckets-worth of histograms, vs, > > say, > > > > 500 buckets-worth of histograms. Will that make a huge difference? > > > > > > It makes a fairly significant difference, yes. What's the reason to > disprefer > > > UMA_HISTOGRAM_CUSTOM_ENUMERATION? > > > > Because without seeing the real world data, it's hard to come up with a good > > custom range. You can check the current Media.VideoVisibleWidth UMA (BTW, > after > > I land the current CL, we can probably drop that), where most of the reports > are > > concentrated in some buckets. Tweaks all the buckets based on some theoretical > > assumption ends up pointless to me. > > > > I didn't know 120 buckets vs 50 would make a big difference. So I played with > it > > a bit more, I think I can do > > UMA_HISTOGRAM_CUSTOM_COUNTS(name, sample, 100, 10000, 50), which will give us > > the following ranges: > > > > 0, 100, 110, 121, 133, 146, 161, 177, 195, 215, 237, 261, 287, 316, 348, 383, > > 422, 464, 511, 562, 619, 681, 750, 826, 909, 1001, 1102, 1213, 1335, 1469, > 1617, > > 1780, 1959, 2156, 2373, 2612, 2875, 3164, 3482, 3832, 4218, 4643, 5110, 5624, > > 6190, 6813, 7499, 8254, 9085, 10000, 2147483647 > > > > Note that 320p will be in [316, 348), 480p will be in [464, 511), 720p will be > > in [681, 750), and 1080p will be in [1001, 1102). It's probably hard to answer > > exact questions like what's the number of playbacks on 1080p+, but we should > > have good estimates. > > > > Dale/Ilya: Does this look good to you? > > Thanks. To be clear, if 120 buckets gives significantly better data, then using > 120 buckets is fine. But if 50 buckets gives roughly comparable-quality data, > then 50 buckets is definitely preferred. Thanks! I chat with Dale about this, we can use this API as the first step to collect data. If we really want to more detailed data, we can add another enum based UMA (e.g. 360p, 480p, 720p etc). So 50 buckets should work.
> Sampling seems like a reasonable solution to me. (You'd probably want to > randomize it a bit, in case some videos do something weird.) Yeah, I think sampling would be the easiest/best way to get the "real" average. Does it matter what the sampling frequency is, e.g one sample per minute vs one sample every 10 minutes? What exactly do you mean by "randomize it a bit"? > If your goal is > just to see what heights are ever used, then reporting just once is okay too. > But in that case it probably makes sense to rename the histogram to match the > use case. If we want to know what resolutions are out there, the "initial" one should just work. Having a collection of all resolutions used is okay, but it's hard to tell a story about it, especially since different players could choose to change resolution more frequently than others. What I was trying to focus is the user experience, not player behavior. So let's land the initial one first, then I'll think more about how to get the "average" one implemented in a sensible way.
I updated the CL to only report the initial height UMA, and to use 50 buckets. PTAL!
On 2017/04/19 05:35:42, xhwang_slow wrote: > > Sampling seems like a reasonable solution to me. (You'd probably want to > > randomize it a bit, in case some videos do something weird.) > > Yeah, I think sampling would be the easiest/best way to get the "real" average. > Does it matter what the sampling frequency is, e.g one sample per minute vs one > sample every 10 minutes? What exactly do you mean by "randomize it a bit"? The sampling frequency mostly matters in that you'll want to have a fine enough frequency to be able to detect changes in practice; and not such a fine one that you'll generate tons and tons of samples (though UMA histograms are quite efficient, so it's ok to err on the side of generating too many samples). By randomizing, I meant not sampling at the 1, 2, 3, 4, etc. minute markers, but instead drawing a random sample from the first minute of video, from the second, and so on -- so you might sample at 0:35, 1:12, 2:15, 3:57, etc. This is not strictly required, but it might help you avoid some blind spots if certain videos shift resolutions multiple times in a minute, say. You can also get much the same effect by simply choosing a higher sampling frequency.
The metrics in the latest patch set LGTM, by the way. Thanks! https://codereview.chromium.org/2814043005/diff/120001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2814043005/diff/120001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:2395: #undef UMA_HISTOGRAM_VIDEO_HEIGHT Optional nit: It seems a bit weird to #undef in an implementation file, since no other files should ever include this file. But, you're welcome to keep it if you really like it.
https://codereview.chromium.org/2814043005/diff/120001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2814043005/diff/120001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:2395: #undef UMA_HISTOGRAM_VIDEO_HEIGHT On 2017/04/19 05:52:45, Ilya Sherman wrote: > Optional nit: It seems a bit weird to #undef in an implementation file, since no > other files should ever include this file. But, you're welcome to keep it if > you really like it. Thanks for catching the weirdness. I am okay either way. But Dale suggested this so I'll just keep it.
The CQ bit was unchecked by xhwang@chromium.org
The CQ bit was checked by xhwang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dalecurtis@chromium.org Link to the patchset: https://codereview.chromium.org/2814043005/#ps120001 (title: "remove "average"")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1492581866934230,
"parent_rev": "2b7d0fcd0261f43efa4592f49238270f500cc39a", "commit_rev":
"60802656141458897882b4283908e75bb444b420"}
Message was sent while issue was closed.
Description was changed from ========== media: Report initial video height to UMA Also fix the issue where the initial video natural size is not reported to MediaLog. BUG=709354 TEST=Manually tested ========== to ========== media: Report initial video height to UMA Also fix the issue where the initial video natural size is not reported to MediaLog. BUG=709354 TEST=Manually tested Review-Url: https://codereview.chromium.org/2814043005 Cr-Commit-Position: refs/heads/master@{#465518} Committed: https://chromium.googlesource.com/chromium/src/+/60802656141458897882b4283908... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:120001) as https://chromium.googlesource.com/chromium/src/+/60802656141458897882b4283908... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
