Media Remoting: Add remoting control logic for encrypted contents.

media/remoting/remoting_renderer_controller.cc

Index: media/remoting/remoting_renderer_controller.cc
diff --git a/media/remoting/remoting_controller.cc b/media/remoting/remoting_renderer_controller.cc
similarity index 34%
rename from media/remoting/remoting_controller.cc
rename to media/remoting/remoting_renderer_controller.cc
index dd38d70ff78f4760dd6b49a91de4aa2ca8c67d67..550f0194ae5e6cce8bc4dcb6cc048725e069f6e8 100644
--- a/media/remoting/remoting_controller.cc
+++ b/media/remoting/remoting_renderer_controller.cc
@@ -2,110 +2,110 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "media/remoting/remoting_controller.h"
+#include "media/remoting/remoting_renderer_controller.h"
 
 #include "base/bind.h"
 #include "base/logging.h"
-#include "base/single_thread_task_runner.h"
+#include "base/threading/thread_checker.h"
+#include "media/remoting/remoting_cdm.h"
 
 namespace media {
 
-RemotingController::RemotingController(
-    mojom::RemotingSourceRequest source_request,
-    mojom::RemoterPtr remoter)
-    : binding_(this, std::move(source_request)),
-      remoter_(std::move(remoter)),
-      task_runner_(base::ThreadTaskRunnerHandle::Get()),
-      weak_factory_(this) {}
-
-RemotingController::~RemotingController() {}
-
-void RemotingController::OnSinkAvailable() {
-  DCHECK(task_runner_->BelongsToCurrentThread());
-
-  is_sink_available_ = true;
-  UpdateAndMaybeSwitch();
-}
-
-void RemotingController::OnSinkGone() {
-  DCHECK(task_runner_->BelongsToCurrentThread());
-
-  is_sink_available_ = false;
-  UpdateAndMaybeSwitch();
+RemotingRendererController::RemotingRendererController(
+    scoped_refptr<RemotingSourceImpl> remoting_source)
+    : remoting_source_(remoting_source), weak_factory_(this) {
+  remoting_source_->AddClient(this);
 }
 
-void RemotingController::OnStarted() {
-  DCHECK(task_runner_->BelongsToCurrentThread());
-
-  VLOG(1) << "Remoting started successively.";
-  if (is_remoting_)
-    switch_renderer_cb_.Run();
-  else
-    remoter_->Stop(mojom::RemotingStopReason::LOCAL_PLAYBACK);
-}
-
-void RemotingController::OnStartFailed(mojom::RemotingStartFailReason reason) {
-  DCHECK(task_runner_->BelongsToCurrentThread());
-
-  VLOG(1) << "Failed to start remoting:" << reason;
-  is_remoting_ = false;
+RemotingRendererController::~RemotingRendererController() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  remoting_source_->RemoveClient(this);
 }
 
-void RemotingController::OnMessageFromSink(
-    const std::vector<uint8_t>& message) {
-  DCHECK(task_runner_->BelongsToCurrentThread());
-
-  // TODO(xjz): Merge with Eric's CL to handle the RPC messages here.
-  NOTIMPLEMENTED();
+void RemotingRendererController::OnStarted(bool success) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+
+  if (success) {
+    VLOG(1) << "Remoting started successively.";
+    if (remote_rendering_started_) {
+      DCHECK(!switch_renderer_cb_.is_null());
+      switch_renderer_cb_.Run();
+    } else {
+      remoting_source_->StopRemoting(this);
+    }
+  } else {
+    VLOG(1) << "Failed to start remoting.";
+    remote_rendering_started_ = false;
+  }
 }
 
-void RemotingController::OnStopped(mojom::RemotingStopReason reason) {
-  DCHECK(task_runner_->BelongsToCurrentThread());
-
-  VLOG(1) << "Remoting stopped: " << reason;
-  is_remoting_ = false;
+void RemotingRendererController::OnSessionStateChanged() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+
+  VLOG(1) << "OnSessionStateChanged: " << remoting_source_->state();
+  bool session_could_remote = session_can_remote_;
+  switch (remoting_source_->state()) {

[miu, 2016/10/29 02:22:16]

Looking at this code again, I think you can simultaneously get rid of the
|session_can_remote_| bool and put this logic in ShouldBeRemoting() where it
will be clearer why we "can remote." So, the whole OnSessionStateChanged()
method could be:

void RemotingRendererController::OnSessionStateChanged() {
  DCHECK(thread_checker_.CalledOnValidThread());
  VLOG(1) << "OnSessionStateChanged: " << remoting_source_->state();
  UpdateAndMaybeSwitch();
}

[xjz, 2016/10/31 17:24:00]

Done.

+    case SESSION_CAN_START:
+    case SESSION_STARTING:
+    case SESSION_STARTED:
+      session_can_remote_ = true;
+      break;
+    case SESSION_STOPPING:
+    case SESSION_UNAVAILABLE:
+    case SESSION_PERMANENTLY_STOPPED:
+      session_can_remote_ = false;
+      break;
+  }
+  if (session_can_remote_ != session_could_remote)
+    UpdateAndMaybeSwitch();
 }
 
-void RemotingController::OnEnteredFullscreen() {
-  DCHECK(task_runner_->BelongsToCurrentThread());
+void RemotingRendererController::OnEnteredFullscreen() {
+  DCHECK(thread_checker_.CalledOnValidThread());
 
   is_fullscreen_ = true;
   UpdateAndMaybeSwitch();
 }
 
-void RemotingController::OnExitedFullscreen() {
-  DCHECK(task_runner_->BelongsToCurrentThread());
+void RemotingRendererController::OnExitedFullscreen() {
+  DCHECK(thread_checker_.CalledOnValidThread());
 
   is_fullscreen_ = false;
   UpdateAndMaybeSwitch();
 }
 
-void RemotingController::OnSetCdm(CdmContext* cdm_context) {
-  DCHECK(task_runner_->BelongsToCurrentThread());
+void RemotingRendererController::OnSetCdm(CdmContext* cdm_context) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+
+  auto* cdm = RemotingCdm::From(cdm_context);
+  if (!cdm)
+    return;
 
-  // TODO(xjz): Not implemented. Will add in up-coming change.
-  NOTIMPLEMENTED();
+  remoting_source_->RemoveClient(this);
+  remoting_source_ = cdm->GetRemotingSource();
+  remoting_source_->AddClient(this);  // Calls OnSessionStateChanged().
+  UpdateAndMaybeSwitch();
 }
 
-void RemotingController::SetSwitchRendererCallback(
-    const SwitchRendererCallback& cb) {
-  DCHECK(task_runner_->BelongsToCurrentThread());
+void RemotingRendererController::SetSwitchRendererCallback(
+    const base::Closure& cb) {
+  DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(!cb.is_null());
 
   switch_renderer_cb_ = cb;
+  UpdateAndMaybeSwitch();
 }
 
-void RemotingController::OnMetadataChanged(const PipelineMetadata& metadata) {
-  DCHECK(task_runner_->BelongsToCurrentThread());
+void RemotingRendererController::OnMetadataChanged(
+    const PipelineMetadata& metadata) {
+  DCHECK(thread_checker_.CalledOnValidThread());
 
-  has_video_ = metadata.has_video;
-  has_audio_ = metadata.has_audio;
-  if (!has_video_ && !has_audio_)
-    return;
+  has_video_ =
+      metadata.has_video && metadata.video_decoder_config.IsValidConfig();
+  has_audio_ =
+      metadata.has_audio && metadata.audio_decoder_config.IsValidConfig();
 
-  // On Android, when using the MediaPlayerRenderer, |has_video_| and
-  // |has_audio_| will be true, but the respective configs will be empty.
-  // We cannot make any assumptions on the validity of configs.
+  is_encrypted_ = false;
   if (has_video_) {
     video_decoder_config_ = metadata.video_decoder_config;
     is_encrypted_ |= video_decoder_config_.is_encrypted();
@@ -117,8 +117,8 @@ void RemotingController::OnMetadataChanged(const PipelineMetadata& metadata) {
   UpdateAndMaybeSwitch();
 }
 
-bool RemotingController::IsVideoCodecSupported() {
-  DCHECK(task_runner_->BelongsToCurrentThread());
+bool RemotingRendererController::IsVideoCodecSupported() {
+  DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(has_video_);
 
   switch (video_decoder_config_.codec()) {
@@ -132,8 +132,8 @@ bool RemotingController::IsVideoCodecSupported() {
   }
 }
 
-bool RemotingController::IsAudioCodecSupported() {
-  DCHECK(task_runner_->BelongsToCurrentThread());
+bool RemotingRendererController::IsAudioCodecSupported() {
+  DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(has_audio_);
 
   switch (audio_decoder_config_.codec()) {
@@ -161,49 +161,59 @@ bool RemotingController::IsAudioCodecSupported() {
   }
 }
 
-bool RemotingController::ShouldBeRemoting() {
-  DCHECK(task_runner_->BelongsToCurrentThread());
+bool RemotingRendererController::ShouldBeRemoting() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+
+  // Explicitly ignoring |is_fullscreen_| since all EME content should trigger
+  // the start of remoting immediately, and not later after switching into
+  // fullscreen. This is required by current technical limitations.
+  // Note: For encrypted contents, once the CDM is created remotely, we will let
+  // RemotingRendererImpl handle all failures, e.g., video/audio codec is not
+  // supported, or session is permanently terminated.
+  if (is_encrypted_) {
+    return remoting_source_->state() == RemotingSessionState::SESSION_STARTED ||

[miu, 2016/10/29 02:22:16]

Hmm...Things moved around and now I'm confused again:

1. I could be wrong, but shouldn't this also include SESSION_STARTING and
SESSION_STOPPING?

2. I'm also not sure we should be unconditionally skipping the checking of the
other dependencies (the ones you moved below this).


I think we were discussing two completely different things the other day. ;)
IIUC, you were talking about: CDM fails (and ends session) before remote
rendering is started. Instead, I was talking about: Ecrypted content was
remotely rendering, and now is shutting down.

In both cases, we want to be using the RemotingRendererImpl because it will be
used to show interstitials to the user to notify them to reload the page to
continue with local rendering.


Just to make sure we're on the same page, here are the transitions I think we
need to consider in this logic:

1. The non-EME case: If the session can start (or is already running), all codec
dependencies are met, and is_fullscreen_ == true; we want to be using the
remoting renderer. If any of these changes, we return false to go back to using
the local renderer.

2. EME: We were never before remotely rendering, and we are in the CAN_START or
STARTING state: Return false to keep the local renderer because the CDM has not
started remoting yet.

3. EME: We were never before remotely rendering, and CDM has shut down the
session: Always return true so we'll be using the RemotingRendererImpl to show
the "error" interstitial.

4. EME: We were never before remotely rendering, and the CDM has started running
(the session is now in the STARTED state to indicate this): Return true since we
are now able to remotely render.

5. EME: We were successfully remotely rendering, and the session is stopping or
has shut down: Return true so we'll be using the RemotingRendererImpl to show
the "reload to continue" interstitial.


I was trying to think of how to describe suggested changes to the code, but I
ended up having to work it all out in an editor. So, taking all of the above
into account, here's what I came up with:

bool RemotingRendererController::ShouldBeRemoting() {
  DCHECK(thread_checker_.CalledOnValidThread());

  if (!switch_renderer_cb_.is_null())
    return false;  // No way to switch to a remoting Renderer impl.

  switch (remoting_source_->state()) {
    case SESSION_UNAVAILABLE:
      return false;  // Cannot remote media without a remote sink.
    case SESSION_CAN_START:
    case SESSION_STARTING:
      if (is_encrypted_)
        return false;  // Prerequisite: Remoting CDM not running yet.
      break;  // Non-encrypted media remoting is possible.
    case SESSION_STARTED:
      break;  // Both encrypted and non-encrypted media remoting are possible.
    case SESSION_STOPPING:
    case SESSION_PERMANENTLY_STOPPED:
      // If content is encrypted, and the session is being stopped (or has
      // stopped), return true to indicate that RemotingRendererImpl should be
      // used to display an interstitial since local rendering cannot be
      // resumed. Otherwise, return false to use local media rendering.
      return is_encrypted_;
  }

  // TODO(xjz): Remoting CDM should not start until supported codecs are
detected.
  if ((!has_audio_ && !has_video_) ||
      (has_video_ && !IsVideoCodecSupported()) ||
      (has_audio_ && !IsAudioCodecSupported())) {
    return false;
  }

  // Normally, entering fullscreen is the signal that starts remote rendering.
  // However, current technical limitations require encrypted content be remoted
  // without waiting for a user signal.
  return is_fullscreen_ || is_encrytped_;
}

[xjz, 2016/10/31 17:24:00]

Yes, I should include SESSION_STOPPING here. But the session should never be in
SESSION_STARTING state when remoting cdm was created. So always return false for
encrypted content when session is starting.
As discussed offline, we will let RemotingRendererImpl handle the failure for
codec support for encrypted contents. Moved other two checks back (callback and
whether we have video or audio).
Yes, I agree. 
Modified the logic as we discussed offline. PTAL.

+           remoting_source_->state() ==
+               RemotingSessionState::SESSION_PERMANENTLY_STOPPED;
+  }
 
-  // TODO(xjz): The control logic for EME will be added in a later CL.
-  if (is_encrypted_)
+  if (!session_can_remote_)
     return false;
-
-  if (!is_sink_available_)
+  if (switch_renderer_cb_.is_null())
     return false;
-  if (!is_fullscreen_)
+  if (!has_audio_ && !has_video_)
     return false;
   if (has_video_ && !IsVideoCodecSupported())
     return false;
   if (has_audio_ && !IsAudioCodecSupported())
     return false;
-  return true;
+  return is_fullscreen_;
 }
 
-void RemotingController::UpdateAndMaybeSwitch() {
-  DCHECK(task_runner_->BelongsToCurrentThread());
-
-  // TODO(xjz): The switching logic for encrypted content will be added in a
-  // later CL.
-
-  // Demuxer is not initialized yet.
-  if (!has_audio_ && !has_video_)
-    return;
-
-  DCHECK(!switch_renderer_cb_.is_null());
+void RemotingRendererController::UpdateAndMaybeSwitch() {
+  DCHECK(thread_checker_.CalledOnValidThread());
 
   bool should_be_remoting = ShouldBeRemoting();
-  if (is_remoting_ == should_be_remoting)
+
+  if (remote_rendering_started_ == should_be_remoting)
     return;
 
-  // Switch between local and remoting.
-  is_remoting_ = should_be_remoting;
-  if (is_remoting_) {
-    // |swithc_renderer_cb_.Run()| will be called after remoting is started
+  // Switch between local renderer and remoting renderer.
+  remote_rendering_started_ = should_be_remoting;
+
+  if (remote_rendering_started_) {
+    DCHECK(!switch_renderer_cb_.is_null());
+    if (remoting_source_->state() ==
+        RemotingSessionState::SESSION_PERMANENTLY_STOPPED) {
+      switch_renderer_cb_.Run();
+      return;
+    }
+    // |switch_renderer_cb_.Run()| will be called after remoting is started
     // successfully.
-    remoter_->Start();
+    remoting_source_->StartRemoting(this);
   } else {
+    DCHECK(!is_encrypted_);

[miu, 2016/10/29 02:22:16]

Please add comment to explain this DCHECK (something like: "For encrypted
content, it's only valid to switch to the remoting renderer, and never back to
the local renderer.").

[xjz, 2016/10/31 17:24:00]

Done.

     switch_renderer_cb_.Run();
-    remoter_->Stop(mojom::RemotingStopReason::LOCAL_PLAYBACK);
+    remoting_source_->StopRemoting(this);
   }
 }