Add dedicated PipelineStatus UMAs for encrypted playback

content/browser/media/media_internals.cc

 // Copyright (c) 2012 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 "content/browser/media/media_internals.h"
 
 #include "base/metrics/histogram.h"
 #include "base/strings/string16.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/stringprintf.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/notification_observer.h"
 #include "content/public/browser/notification_registrar.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/notification_types.h"
 #include "content/public/browser/render_process_host.h"
 #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 {
 
 static base::LazyInstance<content::MediaInternals>::Leaky g_media_internals =
     LAZY_INSTANCE_INITIALIZER;
 
 base::string16 SerializeUpdate(const std::string& function,
                                const base::Value* value) {
   return content::WebUI::GetJavascriptCall(
@@ skipping 161 matching lines @@
 
   // Helper function to save the event payload to RendererPlayerMap.
   void SavePlayerState(const media::MediaLogEvent& event,
                        int render_process_id);
 
  private:
   struct PipelineInfo {
     media::PipelineStatus last_pipeline_status;
     bool has_audio;
     bool has_video;
+    bool video_dds;
     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) {}
   };
 
   // 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.

[xhwang, 2014/12/04 00:37:33]

pipelinestatus_UMA_prefix is not correct

[prabhur1, 2014/12/04 01:49:02]

Done.

+  std::string GetUMANameForAVStream(const PipelineInfo& player_info);
+
   // Key is playerid
   typedef std::map<int, PipelineInfo> PlayerInfoMap;
 
   // Key is renderer id
   typedef std::map<int, PlayerInfoMap> RendererPlayerMap;

[xhwang, 2014/12/04 00:37:33]

Not related to your change, why a map of PipelineInfo is called PlayerInfoMap?
Can we make the name consistent?

[prabhur1, 2014/12/04 01:49:02]

The map stores some state (pipelineInfo in this case) for every player - keyed
by playerId.
Let me know if you feel strongly about changing it - I can change it.

 
   // Stores player information per renderer
   RendererPlayerMap renderer_info_;
 
   NotificationRegistrar registrar_;
 
   DISALLOW_COPY_AND_ASSIGN(MediaInternalsUMAHandler);
 };
 
 MediaInternals::MediaInternalsUMAHandler::MediaInternalsUMAHandler() {
@@ skipping 51 matching lines @@
                                &player_info[event.id].audio_codec_name);
       }
       if (event.params.HasKey("video_codec_name")) {
         event.params.GetString("video_codec_name",
                                &player_info[event.id].video_codec_name);
       }
       if (event.params.HasKey("video_decoder")) {
         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);
+      }
       break;
     default:
       break;
   }
   return;
 }
 
+std::string MediaInternals::MediaInternalsUMAHandler::GetUMANameForAVStream(
+    const PipelineInfo& player_info) {

[xhwang, 2014/12/04 00:37:33]

ditto about pipelineinfo vs playerinfo

[prabhur1, 2014/12/04 01:49:02]

see earlier response.

+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  static const char kPipelineUmaPrefix[] = "Media.PipelineStatus.AudioVideo.";
+  std::string uma_name = kPipelineUmaPrefix;
+  if (player_info.video_codec_name == "vp8") {
+    uma_name += "VP8.";
+  } else if (player_info.video_codec_name == "vp9") {
+    uma_name += "VP9.";
+  } else if (player_info.video_codec_name == "h264") {
+    uma_name += "H264.";
+  } else {
+    return uma_name + "Other";
+  }

[xhwang, 2014/12/04 00:37:33]

nit: add empty line here.

[prabhur1, 2014/12/04 01:49:02]

Done.

+  if (player_info.video_decoder ==
+      media::DecryptingVideoDecoder::kDecoderName) {
+    uma_name += "DVD.";

[xhwang, 2014/12/04 00:37:33]

You can return here. DVD.SW is redundant. DVD would be sufficient.

[prabhur1, 2014/12/04 01:49:02]

Done.

+  } else if (player_info.video_dds) {

[xhwang, 2014/12/04 00:37:33]

If you return above, you don't need "else" here.

[prabhur1, 2014/12/04 01:49:02]

Done.

+    uma_name += "DDS.";
+  }

[xhwang, 2014/12/04 00:37:33]

nit: add empty line here.

[prabhur1, 2014/12/04 01:49:02]

Done.

+  if (player_info.video_decoder == media::GpuVideoDecoder::kDecoderName) {
+    uma_name += "HW";
+  } else {
+    uma_name += "SW";
+  }
+  return uma_name;
+}
+
 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(
+        GetUMANameForAVStream(player_info), 1, 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,
                               media::PIPELINE_STATUS_MAX + 1);
   } else if (player_info.has_video) {
     UMA_HISTOGRAM_ENUMERATION("Media.PipelineStatus.VideoOnly",
                               player_info.last_pipeline_status,
                               media::PIPELINE_STATUS_MAX + 1);
   } else {
     UMA_HISTOGRAM_ENUMERATION("Media.PipelineStatus.Unsupported",
@@ skipping 159 matching lines @@
     const std::string& function,
     const base::DictionaryValue* value) {
   SendUpdate(SerializeUpdate(function, value));
 
   base::AutoLock auto_lock(lock_);
   scoped_ptr<base::Value> out_value;
   CHECK(audio_streams_cached_data_.Remove(cache_key, &out_value));
 }
 
 }  // namespace content