Media Remoting: Draw remoting interstitial on poster image.

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..b35f0c540b7816acadb097e9cf0b2d1f6d7f18e7 100644
--- a/media/remoting/remoting_renderer_controller.cc
+++ b/media/remoting/remoting_renderer_controller.cc
@@ -73,6 +73,15 @@ void RemotingRendererController::OnSetCdm(CdmContext* cdm_context) {
   UpdateAndMaybeSwitch();
 }
 
+void RemotingRendererController::OnSetPoster(const GURL& poster_url) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+
+  if (poster_url != poster_url_) {
+    poster_url_ = poster_url;
+    DownloadPosterImage();

[miu, 2016/12/22 02:04:02]

What if the URL is set to empty (i.e., the web page has cleared the property)?
IIUC, we should interpret that to mean that the page is removing the poster
image. Granted, this is an edge-case, but it could happen, so we should probably
account for that with something like:

  if (poster_url != poster_url_) {
    poster_url_ = poster_url;
    if (poster_url.is_empty()) {
      UpdateInterstitial(SkBitmap());
    } else {
      DownloadPosterImage();
    }
  }

[xjz, 2016/12/22 20:13:08]

This case is really an edge case as the poster image is mainly to be shown while
video is downloading or before user clicks the play button. It seems no use to
clear it after. I think it makes sense to use the previously downloaded image as
the background for the interstitial in this edge case. WDYT?

The current design is that we will only download every poster image once in each
remoting session, and RemoteRendererImpl will keep a copy, and use it to draw
remoting interstitial whenever update is needed. So except calling from
OnPosterImageDownloaded(), we just simply call UpdateInterstitial(SkBitmap()) to
request updating the interstitial. Please see the comment for
UpdateInterstitial() in the header file. If we want to handle the above edge
case and clear the background image when an empty url is set, we could do either
of the following:
1. Keep a copy of the downloaded image in both RemoteRendererImpl and here.
2. Download the image again whenever need to update the interstitial and send it
to RemoteRendererImpl.
3. Add another callback (UpdateBackgroudImageCallback) or keep a reference to
RemoteRendererImpl (or using an interface) here to update (clear) the background
image whenever needed.

WDYT?

[xjz, 2016/12/22 23:21:12]

Done. As chatted face to face, change UpdateInterstitial() argument to
base::<Optional>.

+  }
+}
+
 void RemotingRendererController::SetSwitchRendererCallback(
     const base::Closure& cb) {
   DCHECK(thread_checker_.CalledOnValidThread());
@@ -243,4 +252,42 @@ void RemotingRendererController::UpdateAndMaybeSwitch() {
   }
 }
 
+void RemotingRendererController::SetDownloadPosterCallback(
+    const DownloadPosterCallback& cb) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(download_poster_cb_.is_null());
+  download_poster_cb_ = cb;
+  DownloadPosterImage();

[miu, 2016/12/22 02:04:02]

Along the lines of my previous comment:

  if (!poster_url_.is_empty())
    DownloadPosterImage();

[xjz, 2016/12/22 23:21:12]

Done.

+}
+
+void RemotingRendererController::UpdateInterstitial(const SkBitmap& image) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  if (pipeline_metadata_.natural_size.IsEmpty() || !remote_rendering_started_)
+    return;
+
+  // TODO(xjz): Update interstitial.
+  // https://codereview.chromium.org/2566223005/
+}
+
+void RemotingRendererController::DownloadPosterImage() {
+  // TODO(xjz): early return if show interstitial callback is not set.
+  if (download_poster_cb_.is_null() || poster_url_.is_empty())

[miu, 2016/12/22 02:04:02]

nit: Suggest taking out the "|| poster_url_.is_empty()" part since the decision
is made by the caller of this method.

[xjz, 2016/12/22 23:21:11]

Done.

+    return;
+
+  download_poster_cb_.Run(
+      poster_url_,
+      base::Bind(&RemotingRendererController::OnPosterImageDownloaded,
+                 weak_factory_.GetWeakPtr(), poster_url_));
+}
+
+void RemotingRendererController::OnPosterImageDownloaded(
+    const GURL& download_url,
+    const SkBitmap& image) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+
+  if (download_url != poster_url_)
+    return;  // The poster image URL has changed during the download.
+  UpdateInterstitial(image);
+}
+
 }  // namespace media