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;
+    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());
   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;
+  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