media: Refactor SkCanvasVideoRenderer to use SkImages and not SkBitmaps.

media/blink/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/blink/skcanvas_video_renderer.h"
 
 #include "gpu/GLES2/gl2extchromium.h"
 #include "gpu/command_buffer/client/gles2_interface.h"
 #include "gpu/command_buffer/common/mailbox_holder.h"
 #include "media/base/video_frame.h"
 #include "media/base/yuv_convert.h"
 #include "skia/ext/refptr.h"
 #include "third_party/libyuv/include/libyuv.h"
 #include "third_party/skia/include/core/SkCanvas.h"
 #include "third_party/skia/include/core/SkImage.h"
 #include "third_party/skia/include/core/SkImageGenerator.h"
 #include "third_party/skia/include/gpu/GrContext.h"
 #include "third_party/skia/include/gpu/GrPaint.h"
 #include "third_party/skia/include/gpu/GrTexture.h"
 #include "third_party/skia/include/gpu/GrTextureProvider.h"
-#include "third_party/skia/include/gpu/SkGr.h"
-#include "third_party/skia/include/gpu/SkGrPixelRef.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 {
 
 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.
+// This class keeps the last image drawn.
+// We delete the temporary resource if it is not used for 3 seconds.
 const int kTemporaryResourceDeletionDelay = 3;  // Seconds;
 
 bool CheckColorSpace(const scoped_refptr<VideoFrame>& video_frame,
                      VideoFrame::ColorSpace color_space) {
   int result;
   return video_frame->metadata()->GetInteger(
              VideoFrameMetadata::COLOR_SPACE, &result) &&
          result == color_space;
 }
 
-bool IsSkBitmapProperlySizedTexture(const SkBitmap* bitmap,
-                                    const gfx::Size& size) {
-  return bitmap->getTexture() && bitmap->width() == size.width() &&
-         bitmap->height() == size.height();
-}
-
-bool AllocateSkBitmapTexture(GrContext* gr,
-                             SkBitmap* bitmap,
-                             const gfx::Size& size) {
-  DCHECK(gr);
-  GrTextureDesc desc;
-  // Use kRGBA_8888_GrPixelConfig, not kSkia8888_GrPixelConfig, to avoid
-  // RGBA to BGRA conversion.
-  desc.fConfig = kRGBA_8888_GrPixelConfig;
-  desc.fFlags = kRenderTarget_GrSurfaceFlag;
-  desc.fSampleCnt = 0;
-  desc.fOrigin = kTopLeft_GrSurfaceOrigin;
-  desc.fWidth = size.width();
-  desc.fHeight = size.height();
-  skia::RefPtr<GrTexture> texture = skia::AdoptRef(
-      gr->textureProvider()->refScratchTexture(
-          desc, GrTextureProvider::kExact_ScratchTexMatch));
-  if (!texture.get())
-    return false;
-
-  SkImageInfo info = SkImageInfo::MakeN32Premul(desc.fWidth, desc.fHeight);
-  SkGrPixelRef* pixel_ref = SkNEW_ARGS(SkGrPixelRef, (info, texture.get()));
-  if (!pixel_ref)
-    return false;
-  bitmap->setInfo(info);
-  bitmap->setPixelRef(pixel_ref)->unref();
-  return true;
-}
-
 class SyncPointClientImpl : public VideoFrame::SyncPointClient {
  public:
   explicit SyncPointClientImpl(gpu::gles2::GLES2Interface* gl) : gl_(gl) {}
   ~SyncPointClientImpl() override {}
   uint32 InsertSyncPoint() override { return gl_->InsertSyncPointCHROMIUM(); }
   void WaitSyncPoint(uint32 sync_point) override {
     gl_->WaitSyncPointCHROMIUM(sync_point);
   }
 
  private:
   gpu::gles2::GLES2Interface* gl_;
 
   DISALLOW_IMPLICIT_CONSTRUCTORS(SyncPointClientImpl);
 };
 
-scoped_ptr<SkImage> CreateSkImageFromVideoFrameYUVTextures(
-    VideoFrame* video_frame,
+skia::RefPtr<SkImage> NewSkImageFromVideoFrameYUVTextures(
+    const scoped_refptr<VideoFrame>& video_frame,
     const Context3D& context_3d) {
   // Support only TEXTURE_YUV_420.
   DCHECK(video_frame->HasTextures());
   DCHECK_EQ(media::VideoFrame::I420, video_frame->format());
   DCHECK_EQ(3u, media::VideoFrame::NumPlanes(video_frame->format()));
 
   gpu::gles2::GLES2Interface* gl = context_3d.gl;
   DCHECK(gl);
   gfx::Size ya_tex_size = video_frame->coded_size();
   gfx::Size uv_tex_size((ya_tex_size.width() + 1) / 2,
@@ skipping 26 matching lines @@
   if (CheckColorSpace(video_frame, VideoFrame::COLOR_SPACE_JPEG))
     color_space = kJPEG_SkYUVColorSpace;
 
   SkImage* img = SkImage::NewFromYUVTexturesCopy(context_3d.gr_context,
                                                  color_space, handles, yuvSizes,
                                                  kTopLeft_GrSurfaceOrigin);
   DCHECK(img);
   gl->DeleteTextures(3, source_textures);
   SyncPointClientImpl client(gl);
   video_frame->UpdateReleaseSyncPoint(&client);
-  return make_scoped_ptr(img);
+  return skia::AdoptRef(img);
 }
 
-bool CopyVideoFrameSingleTextureToSkBitmap(VideoFrame* video_frame,
-                                           SkBitmap* bitmap,
-                                           const Context3D& context_3d) {
-  // Check if we could reuse existing texture based bitmap.
-  // Otherwise, release existing texture based bitmap and allocate
-  // a new one based on video size.
-  if (!IsSkBitmapProperlySizedTexture(bitmap,
-                                      video_frame->visible_rect().size())) {
-    if (!AllocateSkBitmapTexture(context_3d.gr_context, bitmap,
-                                 video_frame->visible_rect().size())) {
-      return false;
-    }
+// Creates a SkImage from a |video_frame| backed by native resources.
+// The SkImage will take ownership of the underlying resource.
+skia::RefPtr<SkImage> NewSkImageFromVideoFrameNative(
+    const scoped_refptr<VideoFrame>& video_frame,
+    const Context3D& context_3d) {
+  DCHECK_EQ(VideoFrame::ARGB, video_frame->format());
+
+  const gpu::MailboxHolder& mailbox_holder = video_frame->mailbox_holder(0);
+  DCHECK(mailbox_holder.texture_target == GL_TEXTURE_2D ||
+         mailbox_holder.texture_target == GL_TEXTURE_RECTANGLE_ARB ||
+         mailbox_holder.texture_target == GL_TEXTURE_EXTERNAL_OES)
+      << mailbox_holder.texture_target;
+
+  gpu::gles2::GLES2Interface* gl = context_3d.gl;
+  unsigned source_texture = 0;
+  if (mailbox_holder.texture_target != GL_TEXTURE_2D) {
+    // TODO(dcastagna): At the moment Skia doesn't support targets different
+    // than GL_TEXTURE_2D.  Avoid this copy once
+    // https://code.google.com/p/skia/issues/detail?id=3868 is addressed.
+    gl->GenTextures(1, &source_texture);
+    DCHECK(source_texture);
+    gl->BindTexture(GL_TEXTURE_2D, source_texture);
+    SkCanvasVideoRenderer::CopyVideoFrameSingleTextureToGLTexture(
+        gl, video_frame.get(), source_texture, GL_RGBA, GL_UNSIGNED_BYTE, true,
+        false);
+  } else {
+    gl->WaitSyncPointCHROMIUM(mailbox_holder.sync_point);
+    source_texture = gl->CreateAndConsumeTextureCHROMIUM(
+        mailbox_holder.texture_target, mailbox_holder.mailbox.name);

[dshwang, 2015/07/15 15:10:52]

This code bind the texture of the mailbox and then make SkImage keep the
texture. After this code, VideoFrame cannot manage the life cycle of this
texture.

CreateAndConsumeTextureCHROMIUM() is not expensive operation. In this case, I
think we should not cache and create |source_texture_| every time.

[Daniele Castagna, 2015/07/17 15:42:41]

The texture will be deleted after both SkImage and VideoFrame will call
glDeleteTextures on their texture names. What problem do you see here?
I agree it's cheap, but the code/logic is simpler without having to avoid
caching for this specific case and there isn't any major drawback.

[dshwang, 2015/07/20 10:47:47]

let me imagine one worse senario.
There are two actors; GVD (gpu video decoder) and painter (this code)
GVD: create mailbox
painter: refer to this texture
GVD: go to recycle process because of the refcount of VideoFrame reach zero.
painter: during recycling the texture, it draws the texture on screen.

It happens because GVD handle the texture via VideoFrame, but this code make
painter escape the texture from VideoFrame mechanism.

[Daniele Castagna, 2015/07/30 19:55:00]

GVD waits for release_sync_point_ in VideoFrame before recycling the texture.
Look at GpuVideoDecoder::ReleaseMailbox.

[dshwang, 2015/08/03 17:51:52]

That's problem. While GVD waits for |release_sync_point_|, |last_image_| holds
the texture. GVD cannot know the time in which |last_image_| releases the
texture.
Before this change, there is not any code to hold VideoFrame or the mailbox
texture for undetermined period.

I think the most practical solution is to not cache the |source_texture| in this
case, although code would be bloating.

[Daniele Castagna, 2015/08/03 20:52:21]

This wouldn't be a problem since last_image_ is not used unless we receive that
same VideoFrame.

Said so, I changed the code to drop the cache as you suggested since you don't
like the idea of keeping around a texture even if we won't use it.

   }
-
-  unsigned texture_id =
-      static_cast<unsigned>((bitmap->getTexture())->getTextureHandle());
-  // If CopyVideoFrameSingleTextureToGLTexture() changes the state of the
-  // |texture_id|, it's needed to invalidate the state cached in skia,
-  // but currently the state isn't changed.
-
-  SkCanvasVideoRenderer::CopyVideoFrameSingleTextureToGLTexture(
-      context_3d.gl, video_frame, texture_id, GL_RGBA, GL_UNSIGNED_BYTE, true,
-      false);
-  bitmap->notifyPixelsChanged();
-  return true;
+  GrBackendTextureDesc desc;
+  desc.fFlags = kRenderTarget_GrBackendTextureFlag;
+  desc.fOrigin = kTopLeft_GrSurfaceOrigin;
+  desc.fWidth = video_frame->coded_size().width();
+  desc.fHeight = video_frame->coded_size().height();
+  desc.fConfig = kRGBA_8888_GrPixelConfig;
+  desc.fTextureHandle = source_texture;
+  return skia::AdoptRef(
+      SkImage::NewFromAdoptedTexture(context_3d.gr_context, desc));
 }
 
 }  // anonymous namespace
 
 // Generates an RGB image from a VideoFrame. Convert YUV to RGB plain on GPU.
 class VideoImageGenerator : public SkImageGenerator {
  public:
-  VideoImageGenerator(const scoped_refptr<VideoFrame>& frame)
+  VideoImageGenerator(VideoFrame* frame)
       : SkImageGenerator(
             SkImageInfo::MakeN32Premul(frame->visible_rect().width(),
-                                       frame->visible_rect().height()))
-      , frame_(frame) {
-    DCHECK(frame_.get());
-  }
+                                       frame->visible_rect().height())),
+        frame_(frame) {}
   ~VideoImageGenerator() override {}
 
-  void set_frame(const scoped_refptr<VideoFrame>& frame) { frame_ = frame; }
+  void set_frame(VideoFrame* frame) { frame_ = frame; }
 
  protected:
   Result onGetPixels(const SkImageInfo& info,
                      void* pixels,
                      size_t row_bytes,
                      const Options&,
                      SkPMColor ctable[],
                      int* ctable_count) override {
-    if (!frame_.get())
+    if (!frame_)
       return kInvalidInput;
     // If skia couldn't do the YUV conversion on GPU, we will on CPU.
     SkCanvasVideoRenderer::ConvertVideoFrameToRGBPixels(
         frame_, pixels, row_bytes);
     return kSuccess;
   }
 
   bool onGetYUV8Planes(SkISize sizes[3],
                        void* planes[3],
                        size_t row_bytes[3],
                        SkYUVColorSpace* color_space) override {
-    if (!frame_.get() || !VideoFrame::IsYuvPlanar(frame_->format()) ||
+    if (!frame_ || !media::VideoFrame::IsYuvPlanar(frame_->format()) ||
         // TODO(rileya): Skia currently doesn't support Rec709 YUV conversion,
         // or YUVA conversion. Remove this case once it does. As-is we will
         // fall back on the pure-software path in this case.
         CheckColorSpace(frame_, VideoFrame::COLOR_SPACE_HD_REC709) ||
         frame_->format() == VideoFrame::YV12A) {
       return false;
     }
 
     if (color_space) {
       if (CheckColorSpace(frame_, VideoFrame::COLOR_SPACE_JPEG))
@@ skipping 43 matching lines @@
             in_line += in_line_stride;
             out_line += out_line_stride;
           }
         }
       }
     }
     return true;
   }
 
  private:
-  scoped_refptr<VideoFrame> frame_;
+  VideoFrame* frame_;
 
   DISALLOW_IMPLICIT_CONSTRUCTORS(VideoImageGenerator);
 };
 
 SkCanvasVideoRenderer::SkCanvasVideoRenderer()
-    : last_frame_timestamp_(media::kNoTimestamp()),
-      frame_deleting_timer_(
+    : last_image_deleting_timer_(
           FROM_HERE,
           base::TimeDelta::FromSeconds(kTemporaryResourceDeletionDelay),
           this,
-          &SkCanvasVideoRenderer::ResetLastFrame),
-      accelerated_generator_(nullptr),
-      accelerated_last_frame_timestamp_(media::kNoTimestamp()),
-      accelerated_frame_deleting_timer_(
-          FROM_HERE,
-          base::TimeDelta::FromSeconds(kTemporaryResourceDeletionDelay),
-          this,
-          &SkCanvasVideoRenderer::ResetAcceleratedLastFrame) {
-  last_frame_.setIsVolatile(true);
+          &SkCanvasVideoRenderer::ResetCache) {
 }
 
-SkCanvasVideoRenderer::~SkCanvasVideoRenderer() {}
+SkCanvasVideoRenderer::~SkCanvasVideoRenderer() {
+  ResetCache();
+}
 
 void SkCanvasVideoRenderer::Paint(const scoped_refptr<VideoFrame>& video_frame,
                                   SkCanvas* canvas,
                                   const gfx::RectF& dest_rect,
                                   uint8 alpha,
                                   SkXfermode::Mode mode,
                                   VideoRotation video_rotation,
                                   const Context3D& context_3d) {
   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() || video_frame->natural_size().IsEmpty() ||
       !(VideoFrame::IsYuvPlanar(video_frame->format()) ||
         video_frame->HasTextures())) {
     canvas->drawRect(dest, paint);
     canvas->flush();
     return;
   }
 
-  SkBitmap* target_frame = nullptr;
+  gpu::gles2::GLES2Interface* gl = context_3d.gl;
 
-  if (video_frame->HasTextures()) {
-    // Draw HW Video on both SW and HW Canvas.
-    // In SW Canvas case, rely on skia drawing Ganesh SkBitmap on SW SkCanvas.
-    if (accelerated_last_frame_.isNull() ||
-        video_frame->timestamp() != accelerated_last_frame_timestamp_) {
-      DCHECK(context_3d.gl);
+  // This might be wrong, two different videos could be drawn to the same
+  // canvas and the two different videoframes could have the same timestamp.
+  if (last_image_ && video_frame->timestamp() == last_timestamp_) {
+    // |last_image_| can be reused.
+    // |video_generator_| will be set only if the last cache videoframe was
+    // a software videoframe.
+    if (video_generator_)
+      video_generator_->set_frame(video_frame.get());
+  } else {
+    ResetCache();

[dshwang, 2015/07/20 10:47:47]

allocating texture or system memory is expensive. we want to reuse allocated
texture or system memory.

[Daniele Castagna, 2015/07/30 19:54:59]

Expensive compared to what?
We used to reallocate VideoImageGenerator before this patch too, so that is not
getting worse.

SkImages are immutables, so if we want to switch to that interface we have to
reallocate a new SkImage for every new VideoFrame. Internally SkImage tries to
avoid reallocating resources.

[dshwang, 2015/08/03 17:51:52]

no, previously reuse cached SkBitmap without reallocation. I'm not sure this
perf regression is acceptable.

In sw path, tcmalloc might reallocate same sized memory without overhead.
However, in hw path, it's not acceptable to delete texture and then create new
texture.

[Daniele Castagna, 2015/08/03 20:52:21]

Please refer to Brian's answer.

+    // Generate a new image.
+    if (video_frame->HasTextures()) {
       DCHECK(context_3d.gr_context);
-      if (accelerated_generator_) {
-        // Reset SkBitmap used in SWVideo-to-HWCanvas path.
-        accelerated_last_frame_.reset();
-        accelerated_generator_ = nullptr;
+      DCHECK(gl);

[dshwang, 2015/07/15 15:10:52]

if |video_generator_| exists, delete |video_generator_| here.
it's because one video can have both software frame and hardware frame
interleavingly.

[Daniele Castagna, 2015/07/17 15:42:41]

ResetCache() above takes care of that.

[dshwang, 2015/07/20 10:47:47]

I have question about ResetCache(). see above comment.

+      if (media::VideoFrame::NumPlanes(video_frame->format()) == 1) {
+        last_image_ = NewSkImageFromVideoFrameNative(video_frame, context_3d);
+      } else {
+        last_image_ =
+            NewSkImageFromVideoFrameYUVTextures(video_frame, context_3d);
       }
-
-      if (media::VideoFrame::NumPlanes(video_frame->format()) == 1) {
-        accelerated_last_image_.reset();
-        if (!CopyVideoFrameSingleTextureToSkBitmap(
-                video_frame.get(), &accelerated_last_frame_, context_3d)) {
-          NOTREACHED();
-          return;
-        }
-        DCHECK(video_frame->visible_rect().width() ==
-                   accelerated_last_frame_.width() &&
-               video_frame->visible_rect().height() ==
-                   accelerated_last_frame_.height());
-      } else {
-        accelerated_last_image_ = CreateSkImageFromVideoFrameYUVTextures(
-            video_frame.get(), context_3d);
-        DCHECK(accelerated_last_image_);
-      }
-      accelerated_last_frame_timestamp_ = video_frame->timestamp();
+      SyncPointClientImpl client(gl);
+      video_frame->UpdateReleaseSyncPoint(&client);

[dshwang, 2015/07/15 15:10:52]

it's duplicated to same code in NewSkImageFromVideoFrameYUVTextures() and
SkCanvasVideoRenderer::CopyVideoFrameSingleTextureToGLTexture() in
NewSkImageFromVideoFrameNative().
The rule of thumb is that we should update the sync point after completing to
use the texture. However in GL_TEXTURE_2D and single plain case, we cannot
determine when the texture access is completed because of SkImage cache.

[Daniele Castagna, 2015/07/17 15:42:41]

Moved UpdateReleaseSyncPoint after we draw. I left the one in
CopyVideoFrameSingleTextureToGLTexture since that method is used outside
SkCanvasVideoRenderer.

[dshwang, 2015/07/20 10:47:47]

Acknowledged.

+    } else {
+      video_generator_ = new VideoImageGenerator(video_frame.get());
+      last_image_ = skia::AdoptRef(SkImage::NewFromGenerator(video_generator_));

[dshwang, 2015/07/15 15:10:52]

now |last_image_| deal with both software SkCanvas and Ganesh SkCanvas.
bsalomon, reed, is it safe to use one |last_image_| on both canvases? 

[reed1, 2015/07/15 15:37:32]

It is. If there is a mismatch (e.g. raster-image to gpu-canvas or the reverse)
will handle it and cache the conversion.

[dshwang, 2015/07/15 16:21:10]

thx for answering. in this case, gpu or raster image can be drawn on both
sw-canvas and gpu-canvas at same time. So gpu or raster image should keep both
gpu and raster cache. Is it possible?

[Daniele Castagna, 2015/07/15 16:27:46]

Is that a use case we care about? What's a real example of that happening?

[dshwang, 2015/07/15 16:37:13]

here's real example,
./blink/tools/run_layout_tests.py
virtual/gpu/fast/canvas/yuv-video-on-accelerated-canvas.html

it's rare in real world, but possible.

[dshwang, 2015/07/15 17:56:24]

It sounds good to this change. I agree on |last_image_| handling both sw and gpu
canvas.

     }
-    target_frame = &accelerated_last_frame_;
-    accelerated_frame_deleting_timer_.Reset();
-  } else if (canvas->getGrContext()) {
-    if (accelerated_last_frame_.isNull() ||
-        video_frame->timestamp() != accelerated_last_frame_timestamp_) {
-      // Draw SW Video on HW Canvas.
-      if (!accelerated_generator_ && !accelerated_last_frame_.isNull()) {
-        // Reset SkBitmap used in HWVideo-to-HWCanvas path.
-        accelerated_last_frame_.reset();
-      }
-      accelerated_generator_ = new VideoImageGenerator(video_frame);
-
-      // Note: This takes ownership of |accelerated_generator_|.
-      if (!SkInstallDiscardablePixelRef(accelerated_generator_,
-                                        &accelerated_last_frame_)) {
-        NOTREACHED();
-        return;
-      }
-      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 if (accelerated_generator_) {
-      accelerated_generator_->set_frame(video_frame);
-    }
-    target_frame = &accelerated_last_frame_;
-    accelerated_frame_deleting_timer_.Reset();
-  } else {
-    // Draw SW Video on SW Canvas.
-    DCHECK(video_frame->IsMappable());
-    if (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);
-      }
-      last_frame_.lockPixels();
-      ConvertVideoFrameToRGBPixels(
-          video_frame, last_frame_.getPixels(), last_frame_.rowBytes());
-      last_frame_.notifyPixelsChanged();
-      last_frame_.unlockPixels();
-      last_frame_timestamp_ = video_frame->timestamp();
-    }
-    target_frame = &last_frame_;
-    frame_deleting_timer_.Reset();
+    last_timestamp_ = video_frame->timestamp();
   }
+  last_image_deleting_timer_.Reset();
 
   paint.setXfermodeMode(mode);
   paint.setFilterQuality(kLow_SkFilterQuality);
 
   const 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();
@@ skipping 16 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() / 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));
+        SkFloatToScalar(rotated_dest_size.width() / last_image_->width()),
+        SkFloatToScalar(rotated_dest_size.height() / last_image_->height()));
+    canvas->translate(-SkFloatToScalar(last_image_->width() * 0.5f),
+                      -SkFloatToScalar(last_image_->height() * 0.5f));
   }
-  if (accelerated_last_image_) {
-    canvas->drawImage(accelerated_last_image_.get(), 0, 0, &paint);
-  } else {
-    canvas->drawBitmap(*target_frame, 0, 0, &paint);
-  }
+  canvas->drawImage(last_image_.get(), 0, 0, &paint);
+
   if (need_transform)
     canvas->restore();
+  // Make sure to flush so we can remove the videoframe from the generator.
   canvas->flush();
-  // SkCanvas::flush() causes the generator to generate SkImage, so delete
-  // |video_frame| not to be outlived.
-  if (canvas->getGrContext() && accelerated_generator_)
-    accelerated_generator_->set_frame(nullptr);
+  if (gl)
+    gl->Flush();

[dshwang, 2015/07/15 15:10:52]

Why is this glFlush needed?

[Daniele Castagna, 2015/07/17 15:42:41]

Gone.
I initially put it there to behave in the same way that
CopyVideoFrameSingleTextureToGLTexture does.

+
+  // TODO(dcastagna): here we're assuming |video_generator_| will still be valid
+  // after SkImage::NewFromGenerator took the ownership.
+  // Fix this once https://code.google.com/p/skia/issues/detail?id=3870 is
+  // addressed.
+  if (video_generator_)
+    video_generator_->set_frame(nullptr);
 }
 
 void SkCanvasVideoRenderer::Copy(const scoped_refptr<VideoFrame>& video_frame,
                                  SkCanvas* canvas,
                                  const Context3D& context_3d) {
   Paint(video_frame, canvas, video_frame->visible_rect(), 0xff,
         SkXfermode::kSrc_Mode, media::VIDEO_ROTATION_0, context_3d);
 }
 
 // static
@@ skipping 140 matching lines @@
     unsigned int type,
     bool premultiply_alpha,
     bool flip_y) {
   DCHECK(video_frame);
   DCHECK(video_frame->HasTextures());
   DCHECK_EQ(1u, VideoFrame::NumPlanes(video_frame->format()));
 
   const gpu::MailboxHolder& mailbox_holder = video_frame->mailbox_holder(0);
   DCHECK(mailbox_holder.texture_target == GL_TEXTURE_2D ||
          mailbox_holder.texture_target == GL_TEXTURE_RECTANGLE_ARB ||
-         mailbox_holder.texture_target == GL_TEXTURE_EXTERNAL_OES);
+         mailbox_holder.texture_target == GL_TEXTURE_EXTERNAL_OES)
+      << mailbox_holder.texture_target;
 
   gl->WaitSyncPointCHROMIUM(mailbox_holder.sync_point);
   uint32 source_texture = gl->CreateAndConsumeTextureCHROMIUM(
       mailbox_holder.texture_target, mailbox_holder.mailbox.name);
 
   // The video is stored in a unmultiplied format, so premultiply
   // if necessary.
   // Application itself needs to take care of setting the right |flip_y|
   // value down to get the expected result.
   // "flip_y == true" means to reverse the video orientation while
   // "flip_y == false" means to keep the intrinsic orientation.
   gl->CopyTextureCHROMIUM(GL_TEXTURE_2D, source_texture, texture,
                           internal_format, type,
                           flip_y, premultiply_alpha, false);
 
   gl->DeleteTextures(1, &source_texture);
   gl->Flush();
 
   SyncPointClientImpl client(gl);
   video_frame->UpdateReleaseSyncPoint(&client);
 }
 
-void SkCanvasVideoRenderer::ResetLastFrame() {
-  last_frame_.reset();
-  last_frame_timestamp_ = media::kNoTimestamp();
-}
-
-void SkCanvasVideoRenderer::ResetAcceleratedLastFrame() {
-  accelerated_last_image_.reset();
-  accelerated_last_frame_.reset();
-  accelerated_generator_ = nullptr;
-  accelerated_last_frame_timestamp_ = media::kNoTimestamp();
+void SkCanvasVideoRenderer::ResetCache() {
+  // Clear cached values.
+  video_generator_ = nullptr;
+  last_image_ = nullptr;
+  last_timestamp_ = kNoTimestamp();
 }
 
 }  // namespace media