Media Remoting: Update remoting interstitial when status changes.

media/remoting/remoting_interstitial_ui.cc

Index: media/remoting/remoting_interstitial_ui.cc
diff --git a/media/remoting/remoting_interstitial_ui.cc b/media/remoting/remoting_interstitial_ui.cc
index 9e3f01d23ea1fe55d4f598ef01ebaa02fba3b65a..873684b8c79b6e3e2b1a65b142d3c05dadf6e458 100644
--- a/media/remoting/remoting_interstitial_ui.cc
+++ b/media/remoting/remoting_interstitial_ui.cc
@@ -8,11 +8,11 @@
 #include "media/base/video_frame.h"
 #include "media/base/video_renderer_sink.h"
 #include "media/base/video_util.h"
+#include "skia/ext/image_operations.h"
 #include "third_party/skia/include/core/SkCanvas.h"
 #include "third_party/skia/include/core/SkTypeface.h"
 #include "third_party/skia/include/effects/SkBlurImageFilter.h"
 #include "ui/gfx/color_palette.h"
-#include "ui/gfx/geometry/size.h"
 #include "ui/gfx/paint_vector_icon.h"
 #include "ui/gfx/skbitmap_operations.h"
 #include "ui/gfx/vector_icons_public.h"
@@ -21,39 +21,27 @@ 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
-
-RemotingInterstitialUI::RemotingInterstitialUI(
-    VideoRendererSink* video_renderer_sink,
-    const PipelineMetadata& pipeline_metadata)
-    : video_renderer_sink_(video_renderer_sink),
-      pipeline_metadata_(pipeline_metadata) {
-  DCHECK(pipeline_metadata_.has_video);
-  pipeline_metadata_.natural_size = GetRotatedVideoSize(
-      pipeline_metadata_.video_rotation, pipeline_metadata_.natural_size);
-}
+// Resizes |image| to fit in the center of |canvas_size| and preserves the
+// image's aspect ratio as close as possible.
+SkBitmap ResizeImage(const SkBitmap& image, const gfx::Size& canvas_size) {
+  DCHECK(!canvas_size.IsEmpty());
 
-RemotingInterstitialUI::~RemotingInterstitialUI() {}
+  const gfx::Size image_size = gfx::Size(image.width(), image.height());
+  if (image_size == canvas_size)
+    return image;
 
-scoped_refptr<VideoFrame> RemotingInterstitialUI::GetInterstitial(
-    const SkBitmap& background_image,
-    bool is_remoting_successful) {
-  const gfx::Size canvas_size =
-      gfx::Size(background_image.width(), background_image.height());
-  DCHECK(canvas_size == pipeline_metadata_.natural_size);
+  gfx::Size scaled_size = ScaleSizeToFitWithinTarget(image_size, canvas_size);

[miu, 2016/12/20 00:16:17]

Don't need this (see later comment).

[xjz, 2016/12/20 19:32:28]

Done.

+  if (scaled_size == image_size)
+    return image;
 
-  color_utils::HSL shift = {-1, 0, 0.2};  // Make monochromatic.
-  SkBitmap modified_bitmap =
-      SkBitmapOperations::CreateHSLShiftedBitmap(background_image, shift);
-
-  SkCanvas canvas(modified_bitmap);
+  return skia::ImageOperations::Resize(
+      image, skia::ImageOperations::RESIZE_BEST, scaled_size.width(),
+      scaled_size.height());
+}
 
+void DrawInterstitial(SkCanvas& canvas,

[miu, 2016/12/20 00:16:17]

style: Pass by pointer, not by reference. (and it should be the 3rd argument,
since it's an in-out argument)

[xjz, 2016/12/20 19:32:28]

Done.

+                      const gfx::Size& canvas_size,
+                      bool is_remoting_successful) {

[miu, 2016/12/20 00:16:16]

nit: Instead of the boolean, just pass the "type" enum.

[xjz, 2016/12/20 19:32:28]

Done.

   // Blur the background image.
   SkScalar sigma = SkDoubleToScalar(10);
   SkPaint paint_blur;
@@ -106,37 +94,74 @@ scoped_refptr<VideoFrame> RemotingInterstitialUI::GetInterstitial(
   SkScalar sk_image_offset_y =
       (canvas_size.height() / 2.0) - icon_image.height();
   canvas.drawBitmap(*icon_bitmap, sk_image_offset_x, sk_image_offset_y, &paint);
+}
+
+scoped_refptr<VideoFrame> GetInterstitial(const SkBitmap& image,

[miu, 2016/12/20 00:16:17]

naming nit: How about RenderInterstitialFrame()?

[xjz, 2016/12/20 19:32:28]

Done.

+                                          const gfx::Size& canvas_size,
+                                          RemotingInterstitialType type) {
+  SkBitmap black_background;

[miu, 2016/12/20 00:16:17]

naming nit: This isn't "black background" by the end of this function. So, how
about "canvas_bitmap" or "result_bitmap"?

[xjz, 2016/12/20 19:32:28]

Done. Renamed it as "canvas_bitmap".

+  black_background.allocN32Pixels(canvas_size.width(), canvas_size.height());
+  black_background.eraseColor(SK_ColorBLACK);
+  SkCanvas canvas(black_background);
+
+  SkBitmap background_image;
+  // Draw |background_image| on the canvas.
+  if (!image.drawsNothing()) {
+    background_image = ResizeImage(image, canvas_size);

[miu, 2016/12/20 00:16:16]

Looks like you should call ComputeLetterboxRegion() first, then pass
centered_rect.size() as the 2nd argument to ResizeImage() here. Then, in
ResizeImage(), you don't need to call ScaleSizeToFitWithinTarget().

[xjz, 2016/12/20 19:32:28]

Done.

+    SkBitmap processed_image;
+    if (type == RemotingInterstitialType::NONE) {
+      processed_image = background_image;
+    } else {
+      color_utils::HSL shift = {-1, 0, 0.2};  // Make monochromatic.
+      processed_image =
+          SkBitmapOperations::CreateHSLShiftedBitmap(background_image, shift);
+    }
+
+    const gfx::Size image_size =

[miu, 2016/12/20 00:16:16]

Given the suggestions above, this and the following 9 LOC can just become:

  canvas.writePixels(processed_image, centered_rect.x(), centered_rect.y());

[xjz, 2016/12/20 19:32:28]

Done.

+        gfx::Size(background_image.width(), background_image.height());
+    if (image_size != canvas_size) {
+      // Centered the background image.
+      gfx::Rect centered_rect =
+          ComputeLetterboxRegion(gfx::Rect(canvas_size), image_size);
+      canvas.writePixels(processed_image, centered_rect.x(), centered_rect.y());
+    } else {
+      canvas.writePixels(processed_image, 0, 0);
+    }
+  }
+
+  if (type != RemotingInterstitialType::NONE) {
+    DrawInterstitial(canvas, canvas_size,

[miu, 2016/12/20 00:16:17]

naming: Instead of DrawInterstitial(), how about RenderCastMessage()?

[xjz, 2016/12/20 19:32:28]

Done.

+                     type == RemotingInterstitialType::SUCCESS);
+  }
 
   // Create a new VideoFrame, copy the bitmap, then return it.
   scoped_refptr<media::VideoFrame> video_frame = media::VideoFrame::CreateFrame(
       media::PIXEL_FORMAT_I420, canvas_size, gfx::Rect(canvas_size),
       canvas_size, base::TimeDelta());
-  modified_bitmap.lockPixels();
+  black_background.lockPixels();
   media::CopyRGBToVideoFrame(
-      reinterpret_cast<uint8_t*>(modified_bitmap.getPixels()),
-      modified_bitmap.rowBytes(),
+      reinterpret_cast<uint8_t*>(black_background.getPixels()),
+      black_background.rowBytes(),
       gfx::Rect(canvas_size.width(), canvas_size.height()), video_frame.get());
-  modified_bitmap.unlockPixels();
+  black_background.unlockPixels();
   return video_frame;
 }
 
-void RemotingInterstitialUI::ShowInterstitial(bool is_remoting_successful) {
-  if (!video_renderer_sink_)
-    return;
+}  // namespace
 
-  // TODO(xjz): Provide poster image, if available, rather than a blank, black
-  // image.
-  SkBitmap background_image;
-  const gfx::Size size = pipeline_metadata_.natural_size;
-  background_image.allocN32Pixels(size.width(), size.height());
-  background_image.eraseColor(SK_ColorBLACK);
+void ShowRemotingInterstitial(VideoRendererSink* video_renderer_sink,

[miu, 2016/12/20 00:16:17]

naming nit: "Show" infers the interstitial can also be hidden, but that's not
how this stuff works. How about PaintRemotingInterstitial()?

[xjz, 2016/12/20 19:32:28]

Done.

+                              const SkBitmap& image,
+                              const gfx::Size& canvas_size,
+                              RemotingInterstitialType interstitial_type) {
+  if (canvas_size.IsEmpty())
+    return;
 
   const scoped_refptr<VideoFrame> interstitial =
-      GetInterstitial(background_image, is_remoting_successful);
+      GetInterstitial(image, canvas_size, interstitial_type);
+
   if (!interstitial)
     return;
-
-  video_renderer_sink_->PaintSingleFrame(interstitial);
+  video_renderer_sink->PaintSingleFrame(interstitial);
 }
 
 }  // namespace media