Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(1013)

Unified Diff: media/blink/skcanvas_video_renderer.cc

Issue 1144323003: media: Refactor SkCanvasVideoRenderer to use SkImages and not SkBitmaps. (Closed) Base URL: https://chromium.googlesource.com/chromium/src.git@master
Patch Set: Clear the cache after 3 seconds. Created 5 years, 6 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
« no previous file with comments | « media/blink/skcanvas_video_renderer.h ('k') | media/blink/skcanvas_video_renderer_unittest.cc » ('j') | no next file with comments »
Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
Index: media/blink/skcanvas_video_renderer.cc
diff --git a/media/blink/skcanvas_video_renderer.cc b/media/blink/skcanvas_video_renderer.cc
index a61ea3688f0c27e8c9ba92e7c218635e7682c9b2..0631a23c6a29e94220e2b3f05e45851ee8f93937 100644
--- a/media/blink/skcanvas_video_renderer.cc
+++ b/media/blink/skcanvas_video_renderer.cc
@@ -18,9 +18,6 @@
#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.
@@ -40,10 +37,8 @@ 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,
@@ -54,40 +49,6 @@ bool CheckColorSpace(const scoped_refptr<VideoFrame>& video_frame,
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) {}
@@ -103,8 +64,8 @@ class SyncPointClientImpl : public VideoFrame::SyncPointClient {
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());
@@ -151,34 +112,48 @@ scoped_ptr<SkImage> CreateSkImageFromVideoFrameYUVTextures(
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;
- 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;
+ 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
Daniele Castagna 2015/07/17 15:42:41 The texture will be deleted after both SkImage and
dshwang 2015/07/20 10:47:47 let me imagine one worse senario. There are two ac
Daniele Castagna 2015/07/30 19:55:00 GVD waits for release_sync_point_ in VideoFrame be
dshwang 2015/08/03 17:51:52 That's problem. While GVD waits for |release_sync_
Daniele Castagna 2015/08/03 20:52:21 This wouldn't be a problem since last_image_ is no
+ }
+ 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
@@ -186,16 +161,14 @@ bool CopyVideoFrameSingleTextureToSkBitmap(VideoFrame* video_frame,
// 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,
@@ -204,7 +177,7 @@ class VideoImageGenerator : public SkImageGenerator {
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(
@@ -216,7 +189,7 @@ class VideoImageGenerator : public SkImageGenerator {
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.
@@ -280,29 +253,22 @@ class VideoImageGenerator : public SkImageGenerator {
}
private:
- scoped_refptr<VideoFrame> frame_;
+ VideoFrame* frame_;
DISALLOW_IMPLICIT_CONSTRUCTORS(VideoImageGenerator);
};
SkCanvasVideoRenderer::SkCanvasVideoRenderer()
- : last_frame_timestamp_(media::kNoTimestamp()),
- frame_deleting_timer_(
- FROM_HERE,
- base::TimeDelta::FromSeconds(kTemporaryResourceDeletionDelay),
- this,
- &SkCanvasVideoRenderer::ResetLastFrame),
- accelerated_generator_(nullptr),
- accelerated_last_frame_timestamp_(media::kNoTimestamp()),
- accelerated_frame_deleting_timer_(
+ : last_image_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,
@@ -331,91 +297,37 @@ void SkCanvasVideoRenderer::Paint(const scoped_refptr<VideoFrame>& video_frame,
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.
Daniele Castagna 2015/07/30 19:54:59 Expensive compared to what? We used to reallocate
dshwang 2015/08/03 17:51:52 no, previously reuse cached SkBitmap without reall
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_genera
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 comm
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());
+ last_image_ = NewSkImageFromVideoFrameNative(video_frame, context_3d);
} else {
- accelerated_last_image_ = CreateSkImageFromVideoFrameYUVTextures(
- video_frame.get(), context_3d);
- DCHECK(accelerated_last_image_);
+ last_image_ =
+ NewSkImageFromVideoFrameYUVTextures(video_frame, context_3d);
}
- 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 NewSkImageFromVide
Daniele Castagna 2015/07/17 15:42:41 Moved UpdateReleaseSyncPoint after we draw. I left
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
reed1 2015/07/15 15:37:32 It is. If there is a mismatch (e.g. raster-image t
dshwang 2015/07/15 16:21:10 thx for answering. in this case, gpu or raster ima
Daniele Castagna 2015/07/15 16:27:46 Is that a use case we care about? What's a real ex
dshwang 2015/07/15 16:37:13 here's real example, ./blink/tools/run_layout_test
dshwang 2015/07/15 17:56:24 It sounds good to this change. I agree on |last_im
}
- 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);
@@ -452,23 +364,26 @@ void SkCanvasVideoRenderer::Paint(const scoped_refptr<VideoFrame>& video_frame,
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));
- }
- if (accelerated_last_image_) {
- canvas->drawImage(accelerated_last_image_.get(), 0, 0, &paint);
- } else {
- canvas->drawBitmap(*target_frame, 0, 0, &paint);
+ 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));
}
+ 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 sa
+
+ // 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,
@@ -629,7 +544,8 @@ void SkCanvasVideoRenderer::CopyVideoFrameSingleTextureToGLTexture(
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(
@@ -652,16 +568,11 @@ void SkCanvasVideoRenderer::CopyVideoFrameSingleTextureToGLTexture(
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
« no previous file with comments | « media/blink/skcanvas_video_renderer.h ('k') | media/blink/skcanvas_video_renderer_unittest.cc » ('j') | no next file with comments »

Powered by Google App Engine
This is Rietveld 408576698