[Media Remoting] Add function to create interstitial.

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
new file mode 100644
index 0000000000000000000000000000000000000000..2db21ee8cbff952e8b32a80ab7354bb6c9e97877
--- /dev/null
+++ b/media/remoting/remoting_interstitial_ui.cc
@@ -0,0 +1,100 @@
+// 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_interstitial_ui.h"
+
+#include "media/base/media_resources.h"
+#include "media/base/video_util.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/paint_vector_icon.h"
+#include "ui/gfx/skbitmap_operations.h"
+#include "ui/gfx/vector_icons_public.h"
+
+namespace media {
+
+scoped_refptr<VideoFrame> GetInterstitial(gfx::Size canvas_size,

[miu, 2016/11/04 22:02:30]

Is |canvas_size| ever different from existing_frame_canvas->getBaseLayerSize()?
If not, then it would seem we don't need the |canvas_size| argument. If so, then
we might have to include a scaling transform (and some stretching or cropping
logic).

[apacible, 2016/11/05 01:12:52]

Updated to use the |existing_frame_canvas| size.

+                                          SkCanvas* existing_frame_canvas,
+                                          bool isRemotingSuccessful) {

[miu, 2016/11/04 22:02:30]

ditto: (underscore_naming)

[apacible, 2016/11/05 01:12:51]

Done.

+  if (!existing_frame_canvas)
+    return nullptr;
+
+  // Initial bitmap to grab the existing frame image.
+  SkBitmap bitmap;
+  bitmap.setInfo(existing_frame_canvas->imageInfo());
+
+  // Second bitmap for manipulation.
+  SkBitmap modified_bitmap;
+
+  // Reads the pixels from the current frame into |modified_bitmap|. In the
+  // case this failed, create and use a blank bitmap.
+  if (existing_frame_canvas->readPixels(&bitmap, 0, 0)) {
+    // Make monochromatic.
+    color_utils::HSL shift = {-1, 0, 0.2};
+    modified_bitmap = SkBitmapOperations::CreateHSLShiftedBitmap(bitmap, shift);
+  } else {
+    modified_bitmap.allocN32Pixels(canvas_size.width(), canvas_size.height());
+  }
+
+  SkCanvas canvas(modified_bitmap);
+
+  // Blur the background image.
+  SkScalar sigma = SkDoubleToScalar(10);
+  SkPaint paint_blur;
+  paint_blur.setImageFilter(SkBlurImageFilter::Make(sigma, sigma, nullptr));
+  canvas.saveLayer(0, &paint_blur);
+  canvas.drawBitmap(modified_bitmap, SkIntToScalar(0), SkIntToScalar(0));

[miu, 2016/11/04 22:02:30]

I don't know for sure (maybe you already looked into this?): Is it legal to draw
|modified_bitmap| onto a SkCanvas backed by the same bitmap? In other words,
should the SkCanvas be backed by a different bitmap instance?

[apacible, 2016/11/05 01:12:51]

This line is actually a no-op, so removed.

+  canvas.restore();
+
+  // Write text over the image.
+  SkPaint paint;
+  int text_size = SkIntToScalar(30);
+  paint.setAntiAlias(true);
+  paint.setFilterQuality(kHigh_SkFilterQuality);
+  paint.setColor(SK_ColorLTGRAY);
+  paint.setTypeface(SkTypeface::MakeFromName(
+      "sans", SkFontStyle::FromOldStyle(SkTypeface::kNormal)));
+  paint.setTextSize(text_size);
+
+  const std::string remote_playback_message =
+      isRemotingSuccessful
+          ? GetLocalizedStringUTF8(MEDIA_REMOTING_CASTING_VIDEO_TEXT)
+          : GetLocalizedStringUTF8(MEDIA_REMOTING_CAST_ERROR_TEXT);
+
+  // Center the text horizontally in the canvas.

[miu, 2016/11/04 22:02:30]

The comment says "horizontally," but the next line of code is vertical. Should
the comment just say "center the text in the canvas" (since it looks like you're
centering both ways)?

Alternatively, maybe it'd be better to just include a comment further up in this
method that explains the layout of the text and icon in the result.

[xjz, 2016/11/04 22:10:24]

It is only centered horizontally, not vertically. Agree that this comment should
be moved to before Line 70. 

[apacible, 2016/11/05 01:12:52]

I removed this and added a general comment.

+  SkScalar sk_text_offset_y = (canvas_size.height() / 2.0) + text_size;
+  size_t display_text_width = paint.measureText(remote_playback_message.c_str(),

[miu, 2016/11/04 22:02:30]

Avoid using c_str() where possible. Since the length of the string is being
passed, no NUL terminator append is necessary, and so you can just call data()
instead.

[apacible, 2016/11/05 01:12:52]

Done.

+                                                remote_playback_message.size());
+  SkScalar sk_text_offset_x = (canvas_size.width() - display_text_width) / 2.0;
+  canvas.drawText(remote_playback_message.c_str(),

[miu, 2016/11/04 22:02:30]

s/c_str/data/

[apacible, 2016/11/05 01:12:52]

Done.

+                  remote_playback_message.size(), sk_text_offset_x,
+                  sk_text_offset_y, paint);
+
+  // Draw the appropriate Cast icon.
+  gfx::VectorIconId current_icon = isRemotingSuccessful
+                                       ? gfx::VectorIconId::MEDIA_ROUTER_ACTIVE
+                                       : gfx::VectorIconId::MEDIA_ROUTER_ERROR;
+  // gfx::ImageSkia and SkBitmap have the same width.

[miu, 2016/11/04 22:02:30]

Was this comment meant to be placed just before line 83 (where
|sk_image_offset_x_| is initialized)?

[apacible, 2016/11/05 01:12:52]

This was more a note for myself. Removed.

+  gfx::ImageSkia icon_image =
+      gfx::CreateVectorIcon(current_icon, 65, SK_ColorLTGRAY);
+  const SkBitmap* icon_bitmap = icon_image.bitmap();
+  SkScalar sk_image_offset_x = (canvas_size.width() - icon_image.width()) / 2.0;
+  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);
+
+  // Create a new VideoFrame, copy the bitmap, then return it.
+  const gfx::Rect region_in_frame =

[miu, 2016/11/04 22:02:30]

Is |region_in_frame| a different size than the canvas? Maybe we can just use
|canvas_size| everywhere here, and then we don't need this?

[apacible, 2016/11/05 01:12:52]

Done.

+      gfx::Rect(modified_bitmap.width(), modified_bitmap.height());
+  scoped_refptr<media::VideoFrame> video_frame = media::VideoFrame::CreateFrame(
+      media::PIXEL_FORMAT_I420, canvas_size, gfx::Rect(canvas_size),
+      canvas_size, base::TimeDelta());
+  media::CopyRGBToVideoFrame(

[miu, 2016/11/04 22:02:30]

I think you have to lock the pixels while reading from the bitmap:

  modified_bitmap.lockPixels();
  media::CopyRGBToVideoFrame(...);
  modified_bitmap.unlockPixels();

[apacible, 2016/11/05 01:12:52]

Done.

+      reinterpret_cast<uint8_t*>(modified_bitmap.getPixels()),
+      modified_bitmap.rowBytes(), region_in_frame, video_frame.get());
+  return video_frame;
+}
+
+}  // namespace media