Media Remoting: Draw remoting interstitial on poster image.

media/remoting/remoting_renderer_controller.cc

 // Copyright 2016 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 "media/remoting/remoting_renderer_controller.h"
 
 #include "base/bind.h"
 #include "base/logging.h"
 #include "base/threading/thread_checker.h"
 #include "media/remoting/remoting_cdm_context.h"
@@ skipping 55 matching lines @@
   auto* remoting_cdm_context = RemotingCdmContext::From(cdm_context);
   if (!remoting_cdm_context)
     return;
 
   remoting_source_->RemoveClient(this);
   remoting_source_ = remoting_cdm_context->GetRemotingSource();
   remoting_source_->AddClient(this);  // Calls OnSessionStateChanged().
   UpdateAndMaybeSwitch();
 }
 
+void RemotingRendererController::OnSetPoster(const GURL& poster_url) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+
+  if (poster_url != poster_url_) {
+    poster_url_ = poster_url;
+    UpdateAndMaybeShowInterstitial();

[miu, 2016/12/20 00:48:06]

Instead of calling UpdateInterstitial() here, I would suggest you just run the
download callback here, and then let PosterImageDownloaded() decide whether to
call UpdateInterstitial() later.

[xjz, 2016/12/20 21:46:25]

Done. The poster image will be downloaded here only when url changes and
interstitial callback is set (after merging with another CL). After merge, it
will be also downloaded when the interstitial callback is set (if the url was
set before).

+  }
+}
+
 void RemotingRendererController::SetSwitchRendererCallback(
     const base::Closure& cb) {
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(!cb.is_null());
 
   switch_renderer_cb_ = cb;
   UpdateAndMaybeSwitch();
 }
 
 base::WeakPtr<remoting::RpcBroker> RemotingRendererController::GetRpcBroker()
@@ skipping 150 matching lines @@
     // For encrypted content, it's only valid to switch to remoting renderer,
     // and never back to the local renderer. The RemotingCdmController will
     // force-stop the session when remoting has ended; so no need to call
     // StopRemoting() from here.
     DCHECK(!is_encrypted_);
     switch_renderer_cb_.Run();
     remoting_source_->StopRemoting(this);
   }
 }
 
+void RemotingRendererController::SetDownloadPosterCallback(
+    const DownloadPosterCallback& cb) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(download_poster_cb_.is_null());
+  download_poster_cb_ = cb;
+}
+
+void RemotingRendererController::UpdateAndMaybeShowInterstitial() {

[miu, 2016/12/20 00:48:06]

naming nit: It's fine to just call this UpdateInterstitial() (This'll be a merge
task after your other CL lands.)

[xjz, 2016/12/20 21:46:25]

Done.

+  DCHECK(thread_checker_.CalledOnValidThread());
+  if (pipeline_metadata_.natural_size.IsEmpty())
+    return;
+
+  DCHECK(remote_rendering_started_);
+  if (poster_url_.is_empty() || download_poster_cb_.is_null()) {
+    // TODO(xjz): Draw interstitial on a black image and show it.
+    // https://codereview.chromium.org/2566223005/
+  } else {
+    download_poster_cb_.Run(
+        poster_url_,
+        base::Bind(&RemotingRendererController::PosterImageDownloaded,
+                   weak_factory_.GetWeakPtr()));

[miu, 2016/12/20 00:48:06]

Note: Recommend you pass |poster_url_| as an argument here. (See comment below
for why.)

[xjz, 2016/12/20 21:46:25]

Done.

+  }
+}
+
+void RemotingRendererController::PosterImageDownloaded(const SkBitmap& image) {

[miu, 2016/12/20 00:48:06]

naming nit: OnPosterImageDownloaded().

[miu, 2016/12/20 00:48:06]

Recommend you double-check that the Poster URL has not changed in the meantime.
If it has, just discard the result. Something like:

void RemotingRendererController::PosterImageDownloaded(const GURL& download_url,
const SkBitmap& image) {
  DCHECK(thread_checker_.CalledOnValidThread());
  if (download_url != poster_url_) {
    // The poster image URL has changed during the download.
    return;
  }
  poster_image_ = image;
  UpdateInterstitial();
}

[xjz, 2016/12/20 21:46:25]

Done.

[xjz, 2016/12/20 21:46:25]

Done.

+  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(remote_rendering_started_);

[miu, 2016/12/20 00:48:06]

Since the downloading can take an unbounded amount of time, it's easily possible
that remote rendering was stopped during the download. That shouldn't crash the
process, so I suggest removing this DCHECK(). Just let the call to
UpdateInterstitial() decide what to do.

[xjz, 2016/12/20 21:46:25]

Done.

+  // TODO(xjz): Draw interstitial on |image| and show it.
+  // https://codereview.chromium.org/2566223005/
+}
+
 }  // namespace media