WebMediaPlayerMsCompositor: use correct ARGB/ABGR, use SkSurface/SkPixmap ISO SkBitmap

content/renderer/media/webmediaplayer_ms_compositor.cc

 // Copyright 2015 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 "content/renderer/media/webmediaplayer_ms_compositor.h"
 
 #include <stdint.h>
 #include <string>
 
 #include "base/command_line.h"
 #include "base/hash.h"
 #include "base/single_thread_task_runner.h"
 #include "base/values.h"
 #include "content/common/gpu/client/context_provider_command_buffer.h"
 #include "content/renderer/media/webmediaplayer_ms.h"
 #include "content/renderer/render_thread_impl.h"
 #include "media/base/media_switches.h"
 #include "media/base/video_frame.h"
 #include "media/base/video_util.h"
 #include "media/filters/video_renderer_algorithm.h"
 #include "media/renderers/skcanvas_video_renderer.h"
 #include "skia/ext/platform_canvas.h"
 #include "third_party/WebKit/public/platform/WebMediaStream.h"
 #include "third_party/WebKit/public/platform/WebMediaStreamSource.h"
 #include "third_party/WebKit/public/platform/WebMediaStreamTrack.h"
 #include "third_party/libyuv/include/libyuv/convert.h"
 #include "third_party/libyuv/include/libyuv/planar_functions.h"
 #include "third_party/libyuv/include/libyuv/video_common.h"
+#include "third_party/skia/include/core/SkSurface.h"
 
 namespace content {
 
 namespace {
 
 // This function copies |frame| to a new I420 or YV12A media::VideoFrame.
 scoped_refptr<media::VideoFrame> CopyFrame(
     const scoped_refptr<media::VideoFrame>& frame,
     media::SkCanvasVideoRenderer* video_renderer) {
   scoped_refptr<media::VideoFrame> new_frame;
   if (frame->HasTextures()) {
     DCHECK(frame->format() == media::PIXEL_FORMAT_ARGB ||
            frame->format() == media::PIXEL_FORMAT_XRGB ||
            frame->format() == media::PIXEL_FORMAT_I420 ||
            frame->format() == media::PIXEL_FORMAT_UYVY ||
            frame->format() == media::PIXEL_FORMAT_NV12);
     new_frame = media::VideoFrame::CreateFrame(
         media::PIXEL_FORMAT_I420, frame->coded_size(), frame->visible_rect(),
         frame->natural_size(), frame->timestamp());
-    SkBitmap bitmap;
-    bitmap.allocN32Pixels(frame->visible_rect().width(),
-                          frame->visible_rect().height());
-    SkCanvas canvas(bitmap);
 
-    auto* provider =
+    // TODO(mcasas): Reuse |surface| (as long as frame->visible_rect() doesn't

[Daniele Castagna, 2016/07/18 16:51:23]

Why? Is it expensive to create one?

[mcasas, 2016/07/18 17:40:42]

It boils down to creating a Raster N32 memory, which is
actually not needed on a per-frame basis, since we want 
to use it here as an intermediate whiteboard between |frame| 
and |new_frame|. I think it can be quite expensive to create,
the question would be if SkSurfaces are recycled under 
the hood.

[emircan, 2016/07/18 18:24:25]

Since the use-case here is for pausing media stream, which clearly wouldn't
happen per frame, I think it would be OK allocate and deallocate buffers. I dont
think we need to hold onto surface buffers when it isn't paused.

[mcasas, 2016/07/18 18:33:14]

Oh gotcha, removing the TODO then.

+    // change) by moving it and CopyFrame() to WebMediaPlayerMSCompositor.
+    sk_sp<SkSurface> surface = SkSurface::MakeRasterN32Premul(
+        frame->visible_rect().width(), frame->visible_rect().height());
+
+    ContextProviderCommandBuffer* const provider =
         RenderThreadImpl::current()->SharedMainThreadContextProvider().get();
-    if (provider) {
-      const media::Context3D context_3d =
-          media::Context3D(provider->ContextGL(), provider->GrContext());
-      DCHECK(context_3d.gl);
-      video_renderer->Copy(frame.get(), &canvas, context_3d);
+    if (surface && provider) {

[Daniele Castagna, 2016/07/18 16:51:23]

It seems like surface can be null only if we can't allocate SkPixelRef, should
we prefer DCHECKing that?

[mcasas, 2016/07/18 17:40:43]

It could also be null due to incorrect width/height. I
followed the pattern of e.g. [1,2] of testing |surface|.

[1]
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/Ima...
[2]
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/imageb...

+      DCHECK(provider->ContextGL());
+      video_renderer->Copy(
+          frame.get(), surface->getCanvas(),
+          media::Context3D(provider->ContextGL(), provider->GrContext()));
     } else {
-      // GPU Process crashed.
-      bitmap.eraseColor(SK_ColorTRANSPARENT);
+      // Return a black frame (yuv = {0, 0x80, 0x80}).
+      return media::VideoFrame::CreateColorFrame(
+          frame->visible_rect().size(), 0u, 0x80, 0x80, frame->timestamp());
     }
-    libyuv::ARGBToI420(reinterpret_cast<uint8_t*>(bitmap.getPixels()),
-                       bitmap.rowBytes(),
-                       new_frame->visible_data(media::VideoFrame::kYPlane),
-                       new_frame->stride(media::VideoFrame::kYPlane),
-                       new_frame->visible_data(media::VideoFrame::kUPlane),
-                       new_frame->stride(media::VideoFrame::kUPlane),
-                       new_frame->visible_data(media::VideoFrame::kVPlane),
-                       new_frame->stride(media::VideoFrame::kVPlane),
-                       bitmap.width(), bitmap.height());
+
+    SkPixmap pixmap;
+    const bool result = surface->getCanvas()->peekPixels(&pixmap);
+    DCHECK(result) << "Error trying to access SkSurface's pixels";
+
+    const uint32 source_pixel_format =
+        (kN32_SkColorType == kRGBA_8888_SkColorType) ? libyuv::FOURCC_ABGR
+                                                     : libyuv::FOURCC_ARGB;
+    libyuv::ConvertToI420(
+        static_cast<uint8*>(pixmap.writable_addr(0, 0)),

[Daniele Castagna, 2016/07/18 16:51:23]

nit: can you use addr instead of writable_addr?

[mcasas, 2016/07/18 17:40:43]

Good catch, done 

+        pixmap.getSize64(),

[Daniele Castagna, 2016/07/18 16:51:23]

nit: getSafeSize64 seems to be <= than size64, should we use that instead?

[mcasas, 2016/07/18 17:40:43]

Done.

+        new_frame->data(media::VideoFrame::kYPlane),

[emircan, 2016/07/18 18:24:25]

Consider the case for coded_size() is different than visible_rect() -it can only
be bigger-. We allocate SkSurface with visible_rect() on l.53. We need to copy
into into visible_data() on l.78, 80, 82. Also, we should change to
visible_rect() on l.85 because coded_size() can be bigger than visible_Rect().

[mcasas, 2016/07/18 18:33:14]

Done.

+        new_frame->stride(media::VideoFrame::kYPlane),
+        new_frame->data(media::VideoFrame::kUPlane),
+        new_frame->stride(media::VideoFrame::kUPlane),
+        new_frame->data(media::VideoFrame::kVPlane),
+        new_frame->stride(media::VideoFrame::kVPlane), 0 /* crop_x */,
+        0 /* crop_y */, pixmap.width(), pixmap.height(),
+        new_frame->coded_size().width(), new_frame->coded_size().height(),
+        libyuv::kRotate0, source_pixel_format);
   } else {
     DCHECK(frame->IsMappable());
     DCHECK(frame->format() == media::PIXEL_FORMAT_YV12 ||
            frame->format() == media::PIXEL_FORMAT_YV12A ||
            frame->format() == media::PIXEL_FORMAT_I420);
     const gfx::Size& coded_size = frame->coded_size();
     new_frame = media::VideoFrame::CreateFrame(
         media::IsOpaque(frame->format()) ? media::PIXEL_FORMAT_I420
                                          : media::PIXEL_FORMAT_YV12A,
         coded_size, frame->visible_rect(), frame->natural_size(),
@@ skipping 344 matching lines @@
   }
 
   if (!rendering_frame_buffer_) {
     rendering_frame_buffer_.reset(new media::VideoRendererAlgorithm(
         base::Bind(&WebMediaPlayerMSCompositor::MapTimestampsToRenderTimeTicks,
                    base::Unretained(this))));
   }
 }
 
 }  // namespace content