Add dedicated PipelineStatus UMAs for encrypted playback

content/browser/media/media_internals.cc

Index: content/browser/media/media_internals.cc
diff --git a/content/browser/media/media_internals.cc b/content/browser/media/media_internals.cc
index 5256064e05b867641e6ec3d07d3bdea5927d07fd..d99d70c232b1b622b9c4f55b8233d3dd1932f052 100644
--- a/content/browser/media/media_internals.cc
+++ b/content/browser/media/media_internals.cc
@@ -17,6 +17,7 @@
 #include "content/public/browser/web_ui.h"
 #include "media/audio/audio_parameters.h"
 #include "media/base/media_log_event.h"
+#include "media/filters/decrypting_video_decoder.h"
 #include "media/filters/gpu_video_decoder.h"
 
 namespace {
@@ -198,18 +199,25 @@ class MediaInternals::MediaInternalsUMAHandler : public NotificationObserver {
     media::PipelineStatus last_pipeline_status;
     bool has_audio;
     bool has_video;
+    bool video_dds;
+    bool audio_dds;

[xhwang, 2014/12/03 21:39:50]

nit: typically audio goes before video

[prabhur1, 2014/12/03 23:24:31]

Done.

     std::string audio_codec_name;
     std::string video_codec_name;
     std::string video_decoder;
     PipelineInfo()
         : last_pipeline_status(media::PIPELINE_OK),
           has_audio(false),
-          has_video(false) {}
+          has_video(false),
+          video_dds(false),
+          audio_dds(false) {}
   };
 
   // Helper function to report PipelineStatus associated with a player to UMA.
   void ReportUMAForPipelineStatus(const PipelineInfo& player_info);
 
+  // Helper to generate pipelinestatus_UMA_prefix for AudioVideo streams.
+  std::string GetUMANameForAVStream(const PipelineInfo& player_info);
+
   // Key is playerid
   typedef std::map<int, PipelineInfo> PlayerInfoMap;
 
@@ -286,6 +294,12 @@ void MediaInternals::MediaInternalsUMAHandler::SavePlayerState(
         event.params.GetString("video_decoder",
                                &player_info[event.id].video_decoder);
       }
+      if (event.params.HasKey("video_dds")) {
+        event.params.GetBoolean("video_dds", &player_info[event.id].video_dds);
+      }
+      if (event.params.HasKey("audio_dds")) {
+        event.params.GetBoolean("audio_dds", &player_info[event.id].audio_dds);
+      }
       break;
     default:
       break;
@@ -293,33 +307,42 @@ void MediaInternals::MediaInternalsUMAHandler::SavePlayerState(
   return;
 }
 
+std::string MediaInternals::MediaInternalsUMAHandler::GetUMANameForAVStream(
+    const PipelineInfo& player_info) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  std::string pipeline_uma_prefix = "Media.PipelineStatus.AudioVideo.";
+  if (player_info.video_codec_name == "vp8") {
+    pipeline_uma_prefix += "VP8.";
+  } else if (player_info.video_codec_name == "vp9") {
+    pipeline_uma_prefix += "VP9.";
+  } else if (player_info.video_codec_name == "h264") {
+    pipeline_uma_prefix += "H264.";
+  } else {
+    return pipeline_uma_prefix + "Other";
+  }
+  if (player_info.video_decoder ==
+      media::DecryptingVideoDecoder::kDecoderName) {
+    pipeline_uma_prefix += "DVD";
+  } else if (player_info.video_dds || player_info.audio_dds) {

[xhwang, 2014/12/03 21:39:49]

Now AudioVideo.VP8.DDS could be because we use audio DDS, which will be very
confusing. Since we are not checking DecryptingAudioDecoder(DAD) either, I
suggest we don't check audio_dds as well.

[prabhur1, 2014/12/03 23:24:31]

Done.

+    pipeline_uma_prefix += "DDS";

[xhwang, 2014/12/03 21:39:50]

When we use DDS, we are not checking HW vs SW anymore. Actually it'll be
interesting to check how often we use DDS with SW. This should be rare, but if
we see that a lot, then something is broken.

So is it possible we could have?
codec.HW
codec.SW
codec.DVD
codec.DDS.HW
codec.DDS.SW

[prabhur1, 2014/12/03 23:24:31]

Done.

+  } else if (player_info.video_decoder ==
+             media::GpuVideoDecoder::kDecoderName) {
+    pipeline_uma_prefix += "HW";
+  } else {
+    pipeline_uma_prefix += "SW";
+  }
+  return pipeline_uma_prefix;

[xhwang, 2014/12/03 21:39:50]

Returning a uma prefix seems weird. How about something like:

static const char kPipelineUmaPrefix[] = "Media.PipelineStatus.AudioVideo.";

std::string uma_name = kPipelineUmaPrefix;
uma_name += ...

return uma_name;

[prabhur1, 2014/12/03 23:24:31]

Done.

+}
+
 void MediaInternals::MediaInternalsUMAHandler::ReportUMAForPipelineStatus(
     const PipelineInfo& player_info) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   if (player_info.has_video && player_info.has_audio) {
-    if (player_info.video_codec_name == "vp8") {
-      UMA_HISTOGRAM_ENUMERATION("Media.PipelineStatus.AudioVideo.VP8.SW",
-                                player_info.last_pipeline_status,
-                                media::PIPELINE_STATUS_MAX + 1);
-    } else if (player_info.video_codec_name == "vp9") {
-      UMA_HISTOGRAM_ENUMERATION("Media.PipelineStatus.AudioVideo.VP9.SW",
-                                player_info.last_pipeline_status,
-                                media::PIPELINE_STATUS_MAX + 1);
-    } else if (player_info.video_codec_name == "h264") {
-      if (player_info.video_decoder == media::GpuVideoDecoder::kDecoderName) {
-        UMA_HISTOGRAM_ENUMERATION("Media.PipelineStatus.AudioVideo.H264.HW",
-                                  player_info.last_pipeline_status,
-                                  media::PIPELINE_STATUS_MAX + 1);
-      } else {
-        UMA_HISTOGRAM_ENUMERATION("Media.PipelineStatus.AudioVideo.H264.SW",
-                                  player_info.last_pipeline_status,
-                                  media::PIPELINE_STATUS_MAX + 1);
-      }
-    } else {
-      UMA_HISTOGRAM_ENUMERATION("Media.PipelineStatus.AudioVideo.Other",
-                                player_info.last_pipeline_status,
-                                media::PIPELINE_STATUS_MAX + 1);
-    }
+    base::LinearHistogram::FactoryGet(

[DaleCurtis, 2014/12/02 23:21:17]

Is this the (new?) hotness for dynamic UMA names?

[prabhur1, 2014/12/03 02:04:32]

Done.

[prabhur1, 2014/12/03 02:04:32]

Yep. I think this API has been around for sometime.

+        GetUMANameForAVStream(player_info), media::PIPELINE_OK,

[xhwang, 2014/12/03 21:39:50]

media::PIPELINE_OK is 0, which is not recommended. This is a common pitfall in
the base::LinearHistogram::FactoryGet():

https://code.google.com/p/chromium/codesearch#chromium/src/base/metrics/histo...

https://code.google.com/p/chromium/codesearch#chromium/src/base/metrics/histo...

[prabhur1, 2014/12/03 23:24:31]

Done.

+        media::PIPELINE_STATUS_MAX, media::PIPELINE_STATUS_MAX + 1,
+        base::HistogramBase::kUmaTargetedHistogramFlag)
+        ->Add(player_info.last_pipeline_status);
   } else if (player_info.has_audio) {
     UMA_HISTOGRAM_ENUMERATION("Media.PipelineStatus.AudioOnly",
                               player_info.last_pipeline_status,