Potential bug on drawing video on SkCanvas

media/filters/skcanvas_video_renderer.cc

 // Copyright (c) 2012 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/filters/skcanvas_video_renderer.h"
 
 #include "base/logging.h"
 #include "media/base/video_frame.h"
 #include "media/base/yuv_convert.h"
 #include "third_party/libyuv/include/libyuv.h"
 #include "third_party/skia/include/core/SkCanvas.h"
 #include "third_party/skia/include/core/SkImageGenerator.h"
+#include "third_party/skia/include/gpu/GrContext.h"
 #include "ui/gfx/skbitmap_operations.h"
 
 // Skia internal format depends on a platform. On Android it is ABGR, on others
 // it is ARGB.
 #if SK_B32_SHIFT == 0 && SK_G32_SHIFT == 8 && SK_R32_SHIFT == 16 && \
     SK_A32_SHIFT == 24
 #define LIBYUV_I420_TO_ARGB libyuv::I420ToARGB
 #define LIBYUV_I422_TO_ARGB libyuv::I422ToARGB
 #elif SK_R32_SHIFT == 0 && SK_G32_SHIFT == 8 && SK_B32_SHIFT == 16 && \
     SK_A32_SHIFT == 24
 #define LIBYUV_I420_TO_ARGB libyuv::I420ToABGR
 #define LIBYUV_I422_TO_ARGB libyuv::I422ToABGR
 #else
 #error Unexpected Skia ARGB_8888 layout!
 #endif
 
 namespace media {
 
-static bool IsYUV(media::VideoFrame::Format format) {
+namespace {
+
+// This class keeps two temporary resources; software bitmap, hardware bitmap.
+// If both bitmap are created and then only software bitmap is updated every
+// frame, hardware bitmap outlives until the media player dies. So we delete
+// a temporary resource if it is not used for 3 sec.
+const int kTemporaryResourceDeletionDelay = 3;  // Seconds;

[dshwang, 2014/10/17 15:23:50]

In crrev.com/445013002
This code exists only for memory optimization. It's brute-force optimization so
exact clock time is not needed.
Without this code, this class keeps both |last_frame_| and
|accelerated_generator_| until destruction of this class.

For example, imagine that there are one software canvas and one accelerated
canvas, and sw-canvas draws youtube video one time for thumbnail, and
accel-canvas draws the video every frame. |last_frame_| outlives until the
youtube video lives.

[scherkus (not reviewing), 2014/10/22 23:17:36]

Is this actually a problem in practice? I'm wary of optimizing ahead of time
without knowing whether this is something worth worrying about.

If we *do* think it's a problem worth solving, I'd still see something based on
message loops and wall clock time (e.g., using base::Timer) as it provides a
more deterministic behaviour regardless of what the incoming media timestamps
are and will also clear the cache if, say, the video is currently paused.

+
+bool IsYUV(media::VideoFrame::Format format) {
   switch (format) {
     case VideoFrame::YV12:
     case VideoFrame::YV16:
     case VideoFrame::I420:
     case VideoFrame::YV12A:
     case VideoFrame::YV12J:
     case VideoFrame::YV24:
     case VideoFrame::NV12:
       return true;
     case VideoFrame::UNKNOWN:
     case VideoFrame::NATIVE_TEXTURE:
 #if defined(VIDEO_HOLE)
     case VideoFrame::HOLE:
 #endif  // defined(VIDEO_HOLE)
       return false;
   }
   NOTREACHED() << "Invalid videoframe format provided: " << format;
   return false;
 }
 
-static bool IsJPEGColorSpace(media::VideoFrame::Format format) {
+bool IsJPEGColorSpace(media::VideoFrame::Format format) {
   switch (format) {
     case VideoFrame::YV12J:
       return true;
     case VideoFrame::YV12:
     case VideoFrame::YV16:
     case VideoFrame::I420:
     case VideoFrame::YV12A:
     case VideoFrame::YV24:
     case VideoFrame::NV12:
     case VideoFrame::UNKNOWN:
     case VideoFrame::NATIVE_TEXTURE:
 #if defined(VIDEO_HOLE)
     case VideoFrame::HOLE:
 #endif  // defined(VIDEO_HOLE)
       return false;
   }
   NOTREACHED() << "Invalid videoframe format provided: " << format;
   return false;
 }
 
-static bool IsYUVOrNative(media::VideoFrame::Format format) {
+bool IsYUVOrNative(media::VideoFrame::Format format) {
   return IsYUV(format) || format == media::VideoFrame::NATIVE_TEXTURE;
 }
 
 // Converts a |video_frame| to raw |rgb_pixels|.
-static void ConvertVideoFrameToRGBPixels(
+void ConvertVideoFrameToRGBPixels(
     const scoped_refptr<media::VideoFrame>& video_frame,
     void* rgb_pixels,
     size_t row_bytes) {
   DCHECK(IsYUVOrNative(video_frame->format()))
       << video_frame->format();
   if (IsYUV(video_frame->format())) {
     DCHECK_EQ(video_frame->stride(media::VideoFrame::kUPlane),
               video_frame->stride(media::VideoFrame::kVPlane));
   }
 
@@ skipping 109 matching lines @@
           row_bytes);
       video_frame->ReadPixelsFromNativeTexture(tmp);
       break;
     }
     default:
       NOTREACHED();
       break;
   }
 }
 
-// Generates an RGB image from a VideoFrame.
+}  // anonymous namespace
+
+// Generates an RGB image from a VideoFrame. Convert YUV to RGB plain on GPU.

[scherkus (not reviewing), 2014/10/22 23:17:36]

what's this new comment referring to?

it makes it sound as if it always converts on the GPU -- is that the case?

 class VideoImageGenerator : public SkImageGenerator {
  public:
   VideoImageGenerator(const scoped_refptr<VideoFrame>& frame) : frame_(frame) {
     DCHECK(frame_.get());
   }
   virtual ~VideoImageGenerator() {}
 
   void set_frame(const scoped_refptr<VideoFrame>& frame) { frame_ = frame; }
 
  protected:
   virtual bool onGetInfo(SkImageInfo* info) override {
     info->fWidth = frame_->visible_rect().width();
     info->fHeight = frame_->visible_rect().height();
     info->fColorType = kN32_SkColorType;
     info->fAlphaType = kPremul_SkAlphaType;
     return true;
   }
 
   virtual bool onGetPixels(const SkImageInfo& info,
                            void* pixels,
                            size_t row_bytes,
                            SkPMColor ctable[],
                            int* ctable_count) override {
     if (!frame_.get())
       return false;
     if (!pixels)
       return false;
-    // If skia couldn't do the YUV conversion, we will.
+    // If skia couldn't do the YUV conversion on GPU, we will on CPU.
     ConvertVideoFrameToRGBPixels(frame_, pixels, row_bytes);
     return true;
   }
 
   virtual bool onGetYUV8Planes(SkISize sizes[3],
                                void* planes[3],
                                size_t row_bytes[3],
                                SkYUVColorSpace* color_space) override {
     if (!frame_.get() || !IsYUV(frame_->format()))
       return false;
@@ skipping 35 matching lines @@
     return true;
   }
 
  private:
   scoped_refptr<VideoFrame> frame_;
 
   DISALLOW_IMPLICIT_CONSTRUCTORS(VideoImageGenerator);
 };
 
 SkCanvasVideoRenderer::SkCanvasVideoRenderer()
-    : generator_(NULL), last_frame_timestamp_(media::kNoTimestamp()) {
+    : last_frame_timestamp_(media::kNoTimestamp()),
+      accelerated_generator_(NULL),
+      accelerated_last_frame_timestamp_(media::kNoTimestamp()) {
   last_frame_.setIsVolatile(true);
 }
 
 SkCanvasVideoRenderer::~SkCanvasVideoRenderer() {}
 
 void SkCanvasVideoRenderer::Paint(const scoped_refptr<VideoFrame>& video_frame,
                                   SkCanvas* canvas,
                                   const gfx::RectF& dest_rect,
                                   uint8 alpha,
                                   SkXfermode::Mode mode,
                                   VideoRotation video_rotation) {
   if (alpha == 0) {
     return;
   }
 
   SkRect dest;
   dest.set(dest_rect.x(), dest_rect.y(), dest_rect.right(), dest_rect.bottom());
 
   SkPaint paint;
   paint.setAlpha(alpha);
 
   // Paint black rectangle if there isn't a frame available or the
   // frame has an unexpected format.
-  if (!video_frame.get() || !IsYUVOrNative(video_frame->format())) {
+  if (!video_frame.get() || video_frame->natural_size().IsEmpty() ||

[scherkus (not reviewing), 2014/10/22 23:17:36]

out of curiosity, is this change necessary?

+      !IsYUVOrNative(video_frame->format())) {
     canvas->drawRect(dest, paint);
     canvas->flush();
     return;
   }
 
+  bool accelerated = false;

[scherkus (not reviewing), 2014/10/22 23:17:36]

I would replace this with a SkBitmap* target_frame == NULL and change the
condition on line 360 to be an else and then insert the re-allocating+coverting
code inside a nested if

that way it's more clear that it's impossible to run both branches and that each
branch is responsible for setting |target_frame| prior to completing

+  if (canvas->getGrContext()) {
+    if (accelerated_last_frame_.isNull() ||
+        video_frame->timestamp() != accelerated_last_frame_timestamp_) {
+      accelerated_generator_ = new VideoImageGenerator(video_frame);
+
+      // Note: This takes ownership of |accelerated_generator_|.
+      if (!SkInstallDiscardablePixelRef(accelerated_generator_,
+                                        &accelerated_last_frame_)) {
+        NOTREACHED();
+      }
+      DCHECK(video_frame->visible_rect().width() ==
+                 accelerated_last_frame_.width() &&
+             video_frame->visible_rect().height() ==
+                 accelerated_last_frame_.height());
+
+      accelerated_last_frame_timestamp_ = video_frame->timestamp();
+    } else {
+      accelerated_generator_->set_frame(video_frame);
+    }
+    accelerated = true;
+  }
+
   // Check if we should convert and update |last_frame_|.
-  if (last_frame_.isNull() ||
-      video_frame->timestamp() != last_frame_timestamp_) {
-    generator_ = new VideoImageGenerator(video_frame);
-
-    // Note: This takes ownership of |generator_|.
-    if (!SkInstallDiscardablePixelRef(generator_, &last_frame_)) {
-      NOTREACHED();
+  if (!accelerated && (last_frame_.isNull() ||
+                       video_frame->timestamp() != last_frame_timestamp_)) {
+    // Check if |bitmap| needs to be (re)allocated.
+    if (last_frame_.isNull() ||
+        last_frame_.width() != video_frame->visible_rect().width() ||
+        last_frame_.height() != video_frame->visible_rect().height()) {
+      last_frame_.allocN32Pixels(video_frame->visible_rect().width(),
+                                 video_frame->visible_rect().height());
+      last_frame_.setIsVolatile(true);
     }
-    DCHECK(video_frame->visible_rect().width() == last_frame_.width() &&
-           video_frame->visible_rect().height() == last_frame_.height());
-
+    last_frame_.lockPixels();
+    ConvertVideoFrameToRGBPixels(
+        video_frame, last_frame_.getPixels(), last_frame_.rowBytes());
+    last_frame_.notifyPixelsChanged();
+    last_frame_.unlockPixels();
     last_frame_timestamp_ = video_frame->timestamp();
-  } else if (generator_) {
-    generator_->set_frame(video_frame);
   }
 
   paint.setXfermodeMode(mode);
   paint.setFilterLevel(SkPaint::kLow_FilterLevel);
 
+  SkBitmap& target_frame = accelerated ? accelerated_last_frame_ : last_frame_;
   bool need_transform =
       video_rotation != VIDEO_ROTATION_0 ||
       dest_rect.size() != video_frame->visible_rect().size() ||
       !dest_rect.origin().IsOrigin();
   if (need_transform) {
     canvas->save();
     canvas->translate(
         SkFloatToScalar(dest_rect.x() + (dest_rect.width() * 0.5f)),
         SkFloatToScalar(dest_rect.y() + (dest_rect.height() * 0.5f)));
     SkScalar angle = SkFloatToScalar(0.0f);
@@ skipping 12 matching lines @@
     }
     canvas->rotate(angle);
 
     gfx::SizeF rotated_dest_size = dest_rect.size();
     if (video_rotation == VIDEO_ROTATION_90 ||
         video_rotation == VIDEO_ROTATION_270) {
       rotated_dest_size =
           gfx::SizeF(rotated_dest_size.height(), rotated_dest_size.width());
     }
     canvas->scale(
-        SkFloatToScalar(rotated_dest_size.width() / last_frame_.width()),
-        SkFloatToScalar(rotated_dest_size.height() / last_frame_.height()));
-    canvas->translate(-SkFloatToScalar(last_frame_.width() * 0.5f),
-                      -SkFloatToScalar(last_frame_.height() * 0.5f));
+        SkFloatToScalar(rotated_dest_size.width() / target_frame.width()),
+        SkFloatToScalar(rotated_dest_size.height() / target_frame.height()));
+    canvas->translate(-SkFloatToScalar(target_frame.width() * 0.5f),
+                      -SkFloatToScalar(target_frame.height() * 0.5f));
   }
-  canvas->drawBitmap(last_frame_, 0, 0, &paint);
+  canvas->drawBitmap(target_frame, 0, 0, &paint);
   if (need_transform)
     canvas->restore();
   canvas->flush();
   // SkCanvas::flush() causes the generator to generate SkImage, so delete
   // |video_frame| not to be outlived.
-  if (generator_)
-    generator_->set_frame(NULL);
+  if (accelerated_generator_)
+    accelerated_generator_->set_frame(NULL);
+  CleanUpTemporaryBuffers();
 }
 
 void SkCanvasVideoRenderer::Copy(const scoped_refptr<VideoFrame>& video_frame,
                                  SkCanvas* canvas) {
   Paint(video_frame,
         canvas,
         video_frame->visible_rect(),
         0xff,
         SkXfermode::kSrc_Mode,
         media::VIDEO_ROTATION_0);
 }
 
+void SkCanvasVideoRenderer::CleanUpTemporaryBuffers() {
+  static const base::TimeDelta buffer_time =
+      base::TimeDelta::FromSeconds(kTemporaryResourceDeletionDelay);
+  // Find the latest time stamp.
+  base::TimeDelta last_timestamp =
+      accelerated_last_frame_timestamp_ > last_frame_timestamp_
+          ? accelerated_last_frame_timestamp_
+          : last_frame_timestamp_;
+  // Delete a too old resource.
+  if (last_timestamp > last_frame_timestamp_ + buffer_time &&
+      !last_frame_.isNull()) {
+    last_frame_.reset();
+  }
+  if (last_timestamp > accelerated_last_frame_timestamp_ + buffer_time &&
+      !accelerated_last_frame_.isNull()) {
+    accelerated_last_frame_.reset();
+  }
+}
+
 }  // namespace media