media: Report initial video height to UMA

media/blink/webmediaplayer_impl.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "media/blink/webmediaplayer_impl.h"
 
 #include <algorithm>
 #include <cmath>
 #include <limits>
 #include <string>
@@ skipping 63 matching lines @@
 using blink::WebMediaPlayer;
 using blink::WebRect;
 using blink::WebSize;
 using blink::WebString;
 using gpu::gles2::GLES2Interface;
 
 #define STATIC_ASSERT_ENUM(a, b)                            \
   static_assert(static_cast<int>(a) == static_cast<int>(b), \
                 "mismatching enums: " #a)
 
+#define UMA_HISTOGRAM_VIDEO_HEIGHT(name, sample) \
+  UMA_HISTOGRAM_CUSTOM_COUNTS(name, sample, 64, 10000, 120)
+
 namespace media {
 
 namespace {
 
 // Limits the range of playback rate.
 //
 // TODO(kylep): Revisit these.
 //
 // Vista has substantially lower performance than XP or Windows7.  If you speed
 // up a video too much, it can't keep up, and rendering stops updating except on
@@ skipping 1297 matching lines @@
   // TODO(jrummell): didResumePlaybackBlockedForKey() should only be called
   // when a key has been successfully added (e.g. OnSessionKeysChange() with
   // |has_additional_usable_key| = true). http://crbug.com/461903
   encrypted_client_->DidResumePlaybackBlockedForKey();
 }
 
 void WebMediaPlayerImpl::OnVideoNaturalSizeChange(const gfx::Size& size) {
   DCHECK(main_task_runner_->BelongsToCurrentThread());
   DCHECK_NE(ready_state_, WebMediaPlayer::kReadyStateHaveNothing);
 
+  TRACE_EVENT0("media", "WebMediaPlayerImpl::OnNaturalSizeChanged");
+
   // The input |size| is from the decoded video frame, which is the original
   // natural size and need to be rotated accordingly.
   gfx::Size rotated_size =
       GetRotatedVideoSize(pipeline_metadata_.video_rotation, size);
 
-  if (rotated_size == pipeline_metadata_.natural_size)
+  RecordVideoNaturalSize(rotated_size);

[DaleCurtis, 2017/04/14 18:30:19]

Put below l.1415 to avoid duplicate size events?

[xhwang, 2017/04/14 20:30:35]

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...

+
+  gfx::Size old_size = pipeline_metadata_.natural_size;
+  if (rotated_size == old_size)
     return;
 
-  TRACE_EVENT0("media", "WebMediaPlayerImpl::OnNaturalSizeChanged");
-  media_log_->AddEvent(media_log_->CreateVideoSizeSetEvent(
-      rotated_size.width(), rotated_size.height()));
+  pipeline_metadata_.natural_size = rotated_size;
+
+  // WatchTimeReporter doesn't report metrics for empty videos. Re-create
+  // |watch_time_reporter_| if we didn't originally know the video size.
+  if (old_size.IsEmpty())
+    CreateWatchTimeReporter();
 
   if (overlay_enabled_ && surface_manager_)
     surface_manager_->NaturalSizeChanged(rotated_size);
 
-  gfx::Size old_size = pipeline_metadata_.natural_size;
-  pipeline_metadata_.natural_size = rotated_size;
-  if (old_size.IsEmpty()) {
-    // WatchTimeReporter doesn't report metrics for empty videos. Re-create
-    // |watch_time_reporter_| if we didn't originally know the video size.
-    CreateWatchTimeReporter();
-  }
   client_->SizeChanged();
 
   if (observer_)
     observer_->OnMetadataChanged(pipeline_metadata_);
 }
 
 void WebMediaPlayerImpl::OnVideoOpacityChange(bool opaque) {
   DCHECK(main_task_runner_->BelongsToCurrentThread());
   DCHECK_NE(ready_state_, WebMediaPlayer::kReadyStateHaveNothing);
 
@@ skipping 908 matching lines @@
 }
 
 void WebMediaPlayerImpl::RecordUnderflowDuration(base::TimeDelta duration) {
   DCHECK(data_source_ || chunk_demuxer_);
   if (data_source_)
     UMA_HISTOGRAM_TIMES("Media.UnderflowDuration", duration);
   else
     UMA_HISTOGRAM_TIMES("Media.UnderflowDuration.MSE", duration);
 }
 
+void WebMediaPlayerImpl::RecordVideoNaturalSize(const gfx::Size& natural_size) {
+  // Always report video natural size to MediaLog.
+  media_log_->AddEvent(media_log_->CreateVideoSizeSetEvent(
+      natural_size.width(), natural_size.height()));
+
+  // TODO(xhwang): Also report "average" video height for this playback session.

[DaleCurtis, 2017/04/14 18:30:19]

Why not just record every size change? Why the focus on initial?

[xhwang, 2017/04/14 20:30:35]

As discussed offline, this is useful to measure the "first impression".

+  // See http://crbug.com/709354
+
+  if (initial_video_height_recorded_)
+    return;
+
+  initial_video_height_recorded_ = true;
+
+  int height = natural_size.height();
+  // For vertical videos, use it's width instead of  height for UMA reporting.

[DaleCurtis, 2017/04/14 18:48:57]

Hmmm, why? Seems like this might muddy the metrics?

[xhwang, 2017/04/14 20:30:35]

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.

+  if (height > natural_size.width())
+    height = natural_size.width();
+
+  if (load_type_ == kLoadTypeURL)
+    UMA_HISTOGRAM_VIDEO_HEIGHT("Media.VideoHeight.Initial.SRC", height);
+  else if (load_type_ == kLoadTypeMediaSource)
+    UMA_HISTOGRAM_VIDEO_HEIGHT("Media.VideoHeight.Initial.MSE", height);
+
+  if (is_encrypted_)
+    UMA_HISTOGRAM_VIDEO_HEIGHT("Media.VideoHeight.Initial.EME", height);
+
+  UMA_HISTOGRAM_VIDEO_HEIGHT("Media.VideoHeight.Initial.ALL", height);

[DaleCurtis, 2017/04/14 18:48:57]

All

[xhwang, 2017/04/14 20:30:35]

Done. Thanks for catching this!

+}
+
 }  // namespace media