Media Remoting: Update remoting interstitial when status changes.

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 5f0d37eacdbc7a5939d49d5b0941e7077d07da8f..277db0e16ecefee3dd9c2cf69963358c55906bfb 100644
--- a/media/remoting/remoting_renderer_controller.cc
+++ b/media/remoting/remoting_renderer_controller.cc
@@ -7,10 +7,21 @@
 #include "base/bind.h"
 #include "base/logging.h"
 #include "base/threading/thread_checker.h"
+#include "media/base/video_util.h"
 #include "media/remoting/remoting_cdm_context.h"
 
 namespace media {
 
+namespace {
+
+gfx::Size GetRotatedVideoSize(VideoRotation rotation, gfx::Size natural_size) {
+  if (rotation == VIDEO_ROTATION_90 || rotation == VIDEO_ROTATION_270)
+    return gfx::Size(natural_size.height(), natural_size.width());
+  return natural_size;
+}
+
+}  // namespace
+
 RemotingRendererController::RemotingRendererController(
     scoped_refptr<RemotingSourceImpl> remoting_source)
     : remoting_source_(remoting_source), weak_factory_(this) {
@@ -43,6 +54,8 @@ void RemotingRendererController::OnSessionStateChanged() {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   VLOG(1) << "OnSessionStateChanged: " << remoting_source_->state();
+
+  UpdateInterstitial();
   UpdateAndMaybeSwitch();
 }
 
@@ -103,17 +116,22 @@ void RemotingRendererController::OnMetadataChanged(
     const PipelineMetadata& metadata) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
+  const gfx::Size old_size = pipeline_metadata_.natural_size;
   pipeline_metadata_ = metadata;
 
   is_encrypted_ = false;
   if (has_video()) {
-    video_decoder_config_ = metadata.video_decoder_config;
-    is_encrypted_ |= video_decoder_config_.is_encrypted();
+    is_encrypted_ |= metadata.video_decoder_config.is_encrypted();
+    pipeline_metadata_.natural_size = GetRotatedVideoSize(
+        pipeline_metadata_.video_rotation, pipeline_metadata_.natural_size);
   }
   if (has_audio()) {
-    audio_decoder_config_ = metadata.audio_decoder_config;
-    is_encrypted_ |= audio_decoder_config_.is_encrypted();
+    is_encrypted_ |= metadata.audio_decoder_config.is_encrypted();
   }
+
+  if (pipeline_metadata_.natural_size != old_size)
+    UpdateInterstitial();
+
   UpdateAndMaybeSwitch();
 }
 
@@ -121,13 +139,13 @@ bool RemotingRendererController::IsVideoCodecSupported() {
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(has_video());
 
-  switch (video_decoder_config_.codec()) {
+  switch (pipeline_metadata_.video_decoder_config.codec()) {
     case VideoCodec::kCodecH264:
     case VideoCodec::kCodecVP8:
       return true;
     default:
       VLOG(2) << "Remoting does not support video codec: "
-              << video_decoder_config_.codec();
+              << pipeline_metadata_.video_decoder_config.codec();
       return false;
   }
 }
@@ -136,7 +154,7 @@ bool RemotingRendererController::IsAudioCodecSupported() {
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(has_audio());
 
-  switch (audio_decoder_config_.codec()) {
+  switch (pipeline_metadata_.audio_decoder_config.codec()) {
     case AudioCodec::kCodecAAC:
     case AudioCodec::kCodecMP3:
     case AudioCodec::kCodecPCM:
@@ -156,7 +174,7 @@ bool RemotingRendererController::IsAudioCodecSupported() {
       return true;
     default:
       VLOG(2) << "Remoting does not support audio codec: "
-              << audio_decoder_config_.codec();
+              << pipeline_metadata_.audio_decoder_config.codec();
       return false;
   }
 }
@@ -243,4 +261,36 @@ void RemotingRendererController::UpdateAndMaybeSwitch() {
   }
 }
 
+void RemotingRendererController::SetShowInterstitialCallback(
+    const ShowInterstitialCallback& cb) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  show_interstitial_cb_ = cb;
+  UpdateInterstitial();
+}
+
+void RemotingRendererController::UpdateInterstitial() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  if (!remote_rendering_started_ || show_interstitial_cb_.is_null() ||
+      pipeline_metadata_.natural_size.IsEmpty())
+    return;
+
+  RemotingInterstitialType type = RemotingInterstitialType::BETWEEN_SESSIONS;
+  switch (remoting_source_->state()) {
+    case SESSION_STARTED:
+      type = RemotingInterstitialType::IN_SESSION;
+      break;
+    case SESSION_PERMANENTLY_STOPPED:
+      type = RemotingInterstitialType::ENCRYPTED_MEDIA_FATAL_ERROR;
+      break;
+    case SESSION_UNAVAILABLE:
+    case SESSION_CAN_START:
+    case SESSION_STARTING:

[miu, 2016/12/20 21:21:04]

IMHO, we should show the "IN_SESSION" interstitial when in the SESSION_STARTING
state.

[xjz, 2016/12/20 22:15:37]

Why? We only need (and can only) show interstitial when RemoteRendererImpl is
created, which can not be in "SESSION_STARTING" state.

[miu, 2016/12/20 23:15:49]

tl;dr: Don't bake assumptions into your code when you don't have to.

We should try not to write code that makes assumptions about what code elsewhere
will or will not do. Code outside of this method will change, and so we should
try to make sure this method always does what it was intended to do. As I see
it, this method's purpose is to decide whether the interstitial *should* be
updated, and then which one should be showing, and then run the callback. It
does not know anything about why it was called, nor what the effects of running
the callback will be. It should do the same thing no matter what those
reasons/effects are.

At the most narrow scoping, we are looking at just this switch statement: Its
purpose is to map one or more RemotingSource states into a
RemotingInterstitialType. The mapping from "SESSION_STARTING" to "show the
BETWEEN_SESSIONS interstitial" is incorrect if you consider that and nothing
else.

None of this seems to matter now, but it is important in the long-run in order
to effectively manage large software systems. No one can hold perfectly detailed
knowledge of the entire codebase in their head at once; and so any given
developer's effectiveness is inversely proportional to the number of baked-in
assumptions in the existing code.

For example, what if in the future somebody (maybe not either of us) comes along
and re-works the whole interstitial UI? They want to make it so that
interstitials are rendered a bit earlier to give users a "snappier" response
when remoting is engaged. It would be nice if they could depend on the
UpdateInterstitial() method to work correctly regardless of the fact that the
conditions under which it is used have changed. With the code as you have it
now, they would encounter a bug and have to search through the codebase to
eventually find out that UpdateInterstitial() was choosing "BETWEEN_SESSIONS."
Then, they will assume the last developer had a good reason to map
"SESSION_STARTING" to "BETWEEN_SESSIONS." They really want to change it, but
will worry about whether changing it will break something else; and it will take
them time to convince themselves that it is actually safe to change it to
"IN_SESSION".

Make sense? ;-)

[xjz, 2016/12/20 23:54:54]

Thanks for the explanation. Learned a lot from your comments. :)  
As chatted face to face, still map the "SESSION_STARTING" to the
"BETWEEN_SESSIONS" interstitial, as it is the transient state between two
sessions. 

+    case SESSION_STOPPING:
+      return;

[miu, 2016/12/20 21:21:04]

I think you meant "break" instead of "return" here, so that we would paint the
"BETWEEN_SESSIONS" interstitial for these cases.

[xjz, 2016/12/20 22:15:37]

I mean "return" because we don't need to paint interstitial on other cases. The
"BETWEEN_SESSIONS" interstitial will only be painted by RemoteRendererImpl when
it is destructed. Is there any case that RemoteRendererImpl destructor is not
called whe n remoting session stops?

[miu, 2016/12/20 23:15:49]

This is another flavor of the prior comment: Don't assume what the callback will
or won't do. Just call it with the right value for the current situation. It may
have no effect, but it's not up to UpdateInterstitial() to have to know that.
Why should it matter to the code in UpdateInterstitial() what some other class's
destructor does? ;-)

[xjz, 2016/12/20 23:54:54]

Done. Thanks for the explanation. 
I see. Thanks. :)

+  }
+
+  // TODO(xjz): Download poster image when available.
+  show_interstitial_cb_.Run(SkBitmap(), pipeline_metadata_.natural_size, type);
+}
+
 }  // namespace media