Media Remoting: UMAs to track session events and measurements.

media/remoting/remoting_renderer_controller.cc

Index: media/remoting/remoting_renderer_controller.cc
diff --git a/media/remoting/remoting_renderer_controller.cc b/media/remoting/remoting_renderer_controller.cc
index ab938b98a7b9f08c3e7a9cdb6da2fa791824643e..efe675de83fd7278d313cd8c7f7511c81965599e 100644
--- a/media/remoting/remoting_renderer_controller.cc
+++ b/media/remoting/remoting_renderer_controller.cc
@@ -7,6 +7,7 @@
 #include "base/bind.h"
 #include "base/logging.h"
 #include "base/threading/thread_checker.h"
+#include "base/time/time.h"
 #include "media/remoting/remoting_cdm_context.h"
 
 namespace media {
@@ -19,6 +20,7 @@ RemotingRendererController::RemotingRendererController(
 
 RemotingRendererController::~RemotingRendererController() {
   DCHECK(thread_checker_.CalledOnValidThread());
+  metrics_recorder_.WillStopSession(remoting::MEDIA_ELEMENT_DESTROYED);
   remoting_source_->RemoveClient(this);
 }
 
@@ -28,6 +30,7 @@ void RemotingRendererController::OnStarted(bool success) {
   if (success) {
     VLOG(1) << "Remoting started successively.";
     if (remote_rendering_started_) {
+      metrics_recorder_.DidStartSession();
       DCHECK(!switch_renderer_cb_.is_null());
       switch_renderer_cb_.Run();
     } else {
@@ -36,6 +39,7 @@ void RemotingRendererController::OnStarted(bool success) {
   } else {
     VLOG(1) << "Failed to start remoting.";
     remote_rendering_started_ = false;
+    metrics_recorder_.WillStopSession(remoting::START_RACE);
   }
 }
 
@@ -47,7 +51,7 @@ void RemotingRendererController::OnSessionStateChanged() {
     sink_available_changed_cb_.Run(IsRemoteSinkAvailable());
 
   UpdateInterstitial(base::nullopt);
-  UpdateAndMaybeSwitch();
+  UpdateAndMaybeSwitch(remoting::SINK_AVAILABLE, remoting::ROUTE_TERMINATED);

[xjz, 2017/01/17 05:36:25]

For EME case, this call will start the remoting session when status is STARTED
(please see the comment in OnSetCdm() below). So maybe:
UpdateAndMaybeSwitch(is_encrypted_ ? remoting::CDM_READY :
remoting::SINK_AVAILABLE,
                                          is_encrypted_ ? 
remoting::DECRYPTION_ERROR : remoting::ROUTE_TERMINATED);

[miu, 2017/01/17 21:09:18]

Hmm...But, there are cases where the route was terminated for a remoting session
with encrypted media. So, I came up with an alternate solution, where the
Start/StopTrigger values depend on how this LOC was reached.

[xjz, 2017/01/17 21:52:04]

This lgtm, though for encrypted media, ROUTE_TERMINATED will never be a trigger
to stop remoting. In that case, we'll just show error interstitial and wait for
user to reload the page.

[miu, 2017/01/17 22:46:31]

Added a TODO comment to revisit the logic in the future, once more of the
encrypted-remoting impl is in-place.

 }
 
 bool RemotingRendererController::IsRemoteSinkAvailable() {
@@ -71,21 +75,45 @@ void RemotingRendererController::OnEnteredFullscreen() {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   is_fullscreen_ = true;
-  UpdateAndMaybeSwitch();
+  // See notes in OnBecameDominantVisibleContent() for why this is forced:
+  is_dominant_content_ = true;
+  UpdateAndMaybeSwitch(remoting::ENTERED_FULLSCREEN,
+                       remoting::UNKNOWN_STOP_TRIGGER);
 }
 
 void RemotingRendererController::OnExitedFullscreen() {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   is_fullscreen_ = false;
-  UpdateAndMaybeSwitch();
+  // See notes in OnBecameDominantVisibleContent() for why this is forced:
+  is_dominant_content_ = false;
+  UpdateAndMaybeSwitch(remoting::UNKNOWN_START_TRIGGER,
+                       remoting::EXITED_FULLSCREEN);
 }
 
 void RemotingRendererController::OnBecameDominantVisibleContent(
     bool is_dominant) {
   DCHECK(thread_checker_.CalledOnValidThread());
+
+  // Two scenarios where "dominance" status mixes with fullscreen transitions:
+  //
+  //   1. Just before/after entering fullscreen, the element will, of course,
+  //      become the dominant on-screen content via automatic page layout.
+  //   2. Just before/after exiting fullscreen, the element may or may not
+  //      shrink in size enough to become non-dominant. However, exiting
+  //      fullscreen was caused by a user action that explicitly indicates a
+  //      desire to exit remoting, so even if the element is still dominant,
+  //      remoting should be shut down.
+  //
+  // Thus, to achieve the desired behaviors, |is_dominant_content_| is force-set
+  // in OnEnteredFullscreen() and OnExitedFullscreen(), and changes to it here
+  // are ignored while in fullscreen.
+  if (is_fullscreen_)
+    return;
+
   is_dominant_content_ = is_dominant;
-  UpdateAndMaybeSwitch();
+  UpdateAndMaybeSwitch(remoting::BECAME_DOMINANT_CONTENT,
+                       remoting::BECAME_AUXILIARY_CONTENT);
 }
 
 void RemotingRendererController::OnSetCdm(CdmContext* cdm_context) {
@@ -98,14 +126,15 @@ void RemotingRendererController::OnSetCdm(CdmContext* cdm_context) {
   remoting_source_->RemoveClient(this);
   remoting_source_ = remoting_cdm_context->GetRemotingSource();
   remoting_source_->AddClient(this);  // Calls OnSessionStateChanged().
-  UpdateAndMaybeSwitch();
+  UpdateAndMaybeSwitch(remoting::CDM_READY, remoting::DECRYPTION_ERROR);

[xjz, 2017/01/17 05:36:25]

I just noticed that this UpdateAndMaybeSwith() call is duplicate and should be
removed here, as it will be called in OnSessionStateChanged() when AddClient()
is called above.

[miu, 2017/01/17 21:09:18]

Changed as discussed in prior comment. Here, notice that I took out the
automatic call to OnSessionStateChanged() from RemotingSourceImpl; so I make an
"update" call anyway.

 }
 
 void RemotingRendererController::OnRemotePlaybackDisabled(bool disabled) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   is_remote_playback_disabled_ = disabled;
-  UpdateAndMaybeSwitch();
+  metrics_recorder_.OnRemotePlaybackDisabled(disabled);
+  UpdateAndMaybeSwitch(remoting::ENABLED_BY_PAGE, remoting::DISABLED_BY_PAGE);
 }
 
 void RemotingRendererController::OnSetPoster(const GURL& poster_url) {
@@ -126,7 +155,11 @@ void RemotingRendererController::SetSwitchRendererCallback(
   DCHECK(!cb.is_null());
 
   switch_renderer_cb_ = cb;
-  UpdateAndMaybeSwitch();
+  // Note: Passing "UNKNOWN" triggers here, since this method should be called
+  // as part of the initialization of this RemotingRendererController, and
+  // definitely before a whole lot of other things that would cause a switch.
+  UpdateAndMaybeSwitch(remoting::UNKNOWN_START_TRIGGER,
+                       remoting::UNKNOWN_STOP_TRIGGER);

[xjz, 2017/01/17 05:36:25]

As you explained here, this will never cause a switch. Maybe just remove this
UpdateAndMaybeSwitch() call?

[miu, 2017/01/17 21:09:18]

Done.

 }
 
 void RemotingRendererController::SetRemoteSinkAvailableChangedCallback(
@@ -160,7 +193,12 @@ void RemotingRendererController::OnMetadataChanged(
   DCHECK(thread_checker_.CalledOnValidThread());
 
   const gfx::Size old_size = pipeline_metadata_.natural_size;
+  const bool was_audio_codec_supported = has_audio() && IsAudioCodecSupported();
+  const bool was_video_codec_supported = has_video() && IsVideoCodecSupported();
   pipeline_metadata_ = metadata;
+  const bool is_audio_codec_supported = has_audio() && IsAudioCodecSupported();
+  const bool is_video_codec_supported = has_video() && IsVideoCodecSupported();
+  metrics_recorder_.OnPipelineMetadataChanged(metadata);
 
   is_encrypted_ = false;
   if (has_video())
@@ -171,7 +209,17 @@ void RemotingRendererController::OnMetadataChanged(
   if (pipeline_metadata_.natural_size != old_size)
     UpdateInterstitial(base::nullopt);
 
-  UpdateAndMaybeSwitch();
+  remoting::StartTrigger start_trigger = remoting::UNKNOWN_START_TRIGGER;
+  if (!was_audio_codec_supported && is_audio_codec_supported)
+    start_trigger = remoting::SUPPORTED_AUDIO_CODEC;
+  if (!was_video_codec_supported && is_video_codec_supported)
+    start_trigger = remoting::SUPPORTED_VIDEO_CODEC;

[xjz, 2017/01/17 05:36:25]

Does the SUPPORTED/UNSUPPORTED_VIDEO_CODEC trigger means to start/stop remoting
when either video codec or both audio and video codec become
supported/unsupported?

[miu, 2017/01/17 21:09:18]

I was doing this, but I suppose it's easy to have more granularity here. Fixed.

+  remoting::StopTrigger stop_trigger = remoting::UNKNOWN_STOP_TRIGGER;
+  if (was_audio_codec_supported && !is_audio_codec_supported)
+    stop_trigger = remoting::UNSUPPORTED_AUDIO_CODEC;
+  if (was_video_codec_supported && !is_video_codec_supported)
+    stop_trigger = remoting::UNSUPPORTED_VIDEO_CODEC;
+  UpdateAndMaybeSwitch(start_trigger, stop_trigger);
 }
 
 bool RemotingRendererController::IsVideoCodecSupported() {
@@ -222,7 +270,7 @@ void RemotingRendererController::OnPlaying() {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   is_paused_ = false;
-  UpdateAndMaybeSwitch();
+  UpdateAndMaybeSwitch(remoting::PLAY_COMMAND, remoting::UNKNOWN_STOP_TRIGGER);
 }
 
 void RemotingRendererController::OnPaused() {
@@ -251,7 +299,7 @@ bool RemotingRendererController::ShouldBeRemoting() {
            state == RemotingSessionState::SESSION_PERMANENTLY_STOPPED;
   }
 
-  if (irregular_playback_detected_)
+  if (encountered_renderer_fatal_error_)
     return false;
 
   switch (state) {
@@ -289,7 +337,9 @@ bool RemotingRendererController::ShouldBeRemoting() {
   return is_fullscreen_ || is_dominant_content_;
 }
 
-void RemotingRendererController::UpdateAndMaybeSwitch() {
+void RemotingRendererController::UpdateAndMaybeSwitch(
+    remoting::StartTrigger start_trigger,
+    remoting::StopTrigger stop_trigger) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   bool should_be_remoting = ShouldBeRemoting();
@@ -300,7 +350,8 @@ void RemotingRendererController::UpdateAndMaybeSwitch() {
   // Only switch to remoting when media is playing. Since the renderer is
   // created when video starts loading/playing, receiver will display a black
   // screen before video starts playing if switching to remoting when paused.
-  // Keep mirroring the video in this case is good for the user experience.
+  // Thus, the user experience is improved by not starting remoting until
+  // playback resumes.
   if (should_be_remoting && is_paused_)
     return;
 
@@ -314,6 +365,8 @@ void RemotingRendererController::UpdateAndMaybeSwitch() {
       switch_renderer_cb_.Run();
       return;
     }
+    DCHECK_NE(start_trigger, remoting::UNKNOWN_START_TRIGGER);
+    metrics_recorder_.WillStartSession(start_trigger);
     // |switch_renderer_cb_.Run()| will be called after remoting is started
     // successfully.
     remoting_source_->StartRemoting(this);
@@ -323,6 +376,8 @@ void RemotingRendererController::UpdateAndMaybeSwitch() {
     // force-stop the session when remoting has ended; so no need to call
     // StopRemoting() from here.
     DCHECK(!is_encrypted_);
+    DCHECK_NE(stop_trigger, remoting::UNKNOWN_STOP_TRIGGER);
+    metrics_recorder_.WillStopSession(stop_trigger);
     switch_renderer_cb_.Run();
     remoting_source_->StopRemoting(this);
   }
@@ -376,29 +431,37 @@ void RemotingRendererController::DownloadPosterImage() {
     return;
   DCHECK(!poster_url_.is_empty());
 
+  const base::TimeTicks download_start_time = base::TimeTicks::Now();
   download_poster_cb_.Run(
       poster_url_,
       base::Bind(&RemotingRendererController::OnPosterImageDownloaded,
-                 weak_factory_.GetWeakPtr(), poster_url_));
+                 weak_factory_.GetWeakPtr(), poster_url_, download_start_time));
 }
 
 void RemotingRendererController::OnPosterImageDownloaded(
     const GURL& download_url,
+    base::TimeTicks download_start_time,
     const SkBitmap& image) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
+  metrics_recorder_.OnPosterImageDownloaded(
+      base::TimeTicks::Now() - download_start_time, !image.drawsNothing());
   if (download_url != poster_url_)
     return;  // The poster image URL has changed during the download.
   UpdateInterstitial(image);
 }
 
-void RemotingRendererController::OnIrregularPlaybackDetected() {
+void RemotingRendererController::OnRendererFatalError(
+    remoting::StopTrigger stop_trigger) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
-  if (irregular_playback_detected_)
+  // Do not act on errors caused by things like Mojo pipes being closed during
+  // shutdown.
+  if (!remote_rendering_started_)
     return;
-  irregular_playback_detected_ = true;
-  UpdateAndMaybeSwitch();
+
+  encountered_renderer_fatal_error_ = true;
+  UpdateAndMaybeSwitch(remoting::UNKNOWN_START_TRIGGER, stop_trigger);
 }
 
 }  // namespace media