Add VideoImageGenerator.

Add VideoImageGenerator.

This allows Skia to do YUV conversion on the GPU in the case of a
hardware-accelerated canvas.

Note that this only works for JPEG color space video frames, as that's
the only format Skia currently supports.

BUG=91208
Committed: https://crrev.com/54b8c174c8420abc0bc519eb032b1289029fcc48
Cr-Commit-Position: refs/heads/master@{#294981}

[rileya (GONE FROM CHROMIUM), 2014-09-03 02:42:54]

rileya@chromium.org changed reviewers:
+ bsalomon@google.com, danakj@chromium.org, reed@google.com,
robertphillips@google.com, scherkus@chromium.org

[rileya (GONE FROM CHROMIUM), 2014-09-03 02:42:55]

This speeds up video->accelerated canvas copies significantly.

Currently only for JPEG color range, but with
https://codereview.chromium.org/516463005/ and some plumbing to expose the YUV
format through SkImageGenerator it'll work for other YUV flavors as well.

With some Blink changes, this can also speed up the video->WebGL path.

scherkus: take a peek at media/
danakj: have a look at the tiny change in cc/
reed/bsalomon/robertphillips: thoughts on skia usage? Particularly, as we
discussed the other day, I think this may flood the Ganesh texture cache with
video frames...

https://codereview.chromium.org/531353002/diff/40001/media/filters/skcanvas_v...
File media/filters/skcanvas_video_renderer.h (right):

https://codereview.chromium.org/531353002/diff/40001/media/filters/skcanvas_v...
media/filters/skcanvas_video_renderer.h:8: #include "base/memory/ref_counted.h"
SkCanvasVideoRenderer now takes a scoped_refptr<VideoFrame> rather than a raw
pointer.

The VideoImageGenerator won't get called into by Skia until it actually needs
the pixels, so there may be some cases where the VideoFrame needs to outlive the
scope of the Paint() call in SkCanvasVideoRenderer.

Currently I don't think we run into this, but down the road I'd like to be able
to, say, hand the software compositor a SkBitmap backed by the
VideoImageGenerator and do the YUV conversion lazily.

[reed1, 2014-09-03 12:59:12]

skia part lgtm

[scherkus (not reviewing), 2014-09-03 22:51:50]

mostly nits

CL description needs to be wrapped at 72 and contain a BUG= as well

do you think we have any existing performance tests that would track this?

https://codereview.chromium.org/531353002/diff/40001/media/filters/skcanvas_v...
File media/filters/skcanvas_video_renderer.cc (right):

https://codereview.chromium.org/531353002/diff/40001/media/filters/skcanvas_v...
media/filters/skcanvas_video_renderer.cc:180:
VideoImageGenerator(scoped_refptr<VideoFrame> frame) : frame_(frame) {}
const-ref

https://codereview.chromium.org/531353002/diff/40001/media/filters/skcanvas_v...
media/filters/skcanvas_video_renderer.cc:181: protected:
don't forget to declare a virtual dtor

https://codereview.chromium.org/531353002/diff/40001/media/filters/skcanvas_v...
media/filters/skcanvas_video_renderer.cc:222: VideoFrame* frame_;
should we be holding onto a ref?

https://codereview.chromium.org/531353002/diff/40001/media/filters/skcanvas_v...
media/filters/skcanvas_video_renderer.cc:263: // FIXME: Perform this rotation on
the canvas, rather than allocating a new
s/FIXME/TODO(username)/

https://codereview.chromium.org/531353002/diff/40001/media/filters/skcanvas_v...
File media/filters/skcanvas_video_renderer.h (right):

https://codereview.chromium.org/531353002/diff/40001/media/filters/skcanvas_v...
media/filters/skcanvas_video_renderer.h:33: void Paint(scoped_refptr<VideoFrame>
video_frame,
const ref

https://codereview.chromium.org/531353002/diff/40001/media/filters/skcanvas_v...
media/filters/skcanvas_video_renderer.h:41: void Copy(scoped_refptr<VideoFrame>
video_frame, SkCanvas* canvas);
const ref

[danakj, 2014-09-03 22:57:34]

https://codereview.chromium.org/531353002/diff/40001/cc/resources/video_resou...
File cc/resources/video_resource_updater.cc (right):

https://codereview.chromium.org/531353002/diff/40001/cc/resources/video_resou...
cc/resources/video_resource_updater.cc:252: video_renderer_->Copy(video_frame,
lock.sk_canvas());
cc LGTM if the media people think it makes sense for Copy() to hold a reference
to the VideoFrame

it seems weird to me since we're:
a) using software compositing so this is not a GL resource
b) expecting the copy to complete into the canvas by the time the function
returns

[rileya (GONE FROM CHROMIUM), 2014-09-05 20:03:08]

> if the media people think it makes sense for Copy() to hold a reference
> to the VideoFrame

I agree that this is kinda silly, I've removed the refcounting for the moment...
my concern is that by using an SkImageGenerator we're basically promising skia
that we can always conjure up YUV planes or RGB pixels for the lifetime of the
SkBitmap it's installed on (so it can discard the memory, or, say, ask for the
pixels to be copied directly into a mapped pixel buffer object, which might be
deferred due to command buffering in Ganesh).

If skia ever decided to defer things for whatever reason, we might have a
situation where the frame gets released and skia asks the VideoImageGenerator
for pixels afterwards. I'm thinking this isn't actually possible with our
current usage, but I'll take a closer look.

https://codereview.chromium.org/531353002/diff/40001/media/filters/skcanvas_v...
File media/filters/skcanvas_video_renderer.cc (right):

https://codereview.chromium.org/531353002/diff/40001/media/filters/skcanvas_v...
media/filters/skcanvas_video_renderer.cc:263: // FIXME: Perform this rotation on
the canvas, rather than allocating a new
On 2014/09/03 22:51:50, scherkus wrote:
> s/FIXME/TODO(username)/

done.

[rileya (GONE FROM CHROMIUM), 2014-09-08 19:51:20]

rileya@chromium.org changed reviewers:
+ aelias@chromium.org

[rileya (GONE FROM CHROMIUM), 2014-09-08 19:51:21]

After consulting with Skia folks, I think the earlier approach of making the
VideoImageGenerator hold a ref to the frame is the best choice here. It is a
little weird making a "Paint" or "Copy" call (potentially) hold onto a ref, but
giving Skia more room to be clever with optimization behind the scenes is worth
it, IMO. Apart from the ref this doesn't change the publicly visible behavior,
anything going on behind the scenes in skia will be opaque to the caller.

scherkus: I think I addressed all of your nits from the first round of review.
+aelias: PTAL at render_widget_host_view_browsertest.cc

[aelias_OOO_until_Jul13, 2014-09-08 20:13:35]

content/browser/renderer_host/render_widget_host_view_browsertest.cc lgtm

[scherkus (not reviewing), 2014-09-09 00:27:06]

https://codereview.chromium.org/531353002/diff/160001/media/filters/skcanvas_...
File media/filters/skcanvas_video_renderer.cc (right):

https://codereview.chromium.org/531353002/diff/160001/media/filters/skcanvas_...
media/filters/skcanvas_video_renderer.cc:193: size_t rowBytes,
nit: I think skia has a different naming style, but in  chromium this should be
unix_hacker (i.e., row_bytes)

https://codereview.chromium.org/531353002/diff/160001/media/filters/skcanvas_...
media/filters/skcanvas_video_renderer.cc:203: size_t rowBytes[3]) OVERRIDE {
ditto for row_bytes

https://codereview.chromium.org/531353002/diff/160001/media/filters/skcanvas_...
media/filters/skcanvas_video_renderer.cc:205: if (frame_->format() !=
VideoFrame::YV12J)
if our Media.VideoPixelFormat UMA data is to believed, we hit YV12J content
~0.02% of the time

are there plans to support other YUV formats? is there a bug, etc?

https://codereview.chromium.org/531353002/diff/160001/media/filters/skcanvas_...
media/filters/skcanvas_video_renderer.cc:206: return false;
what does returning false entail?

https://codereview.chromium.org/531353002/diff/160001/media/filters/skcanvas_...
File media/filters/skcanvas_video_renderer.h (right):

https://codereview.chromium.org/531353002/diff/160001/media/filters/skcanvas_...
media/filters/skcanvas_video_renderer.h:33: void Paint(const
scoped_refptr<VideoFrame> video_frame,
nit: missing a & to make these const-ref

https://codereview.chromium.org/531353002/diff/160001/media/filters/skcanvas_...
media/filters/skcanvas_video_renderer.h:41: void Copy(const
scoped_refptr<VideoFrame>, SkCanvas* canvas);
ditto

https://codereview.chromium.org/531353002/diff/160001/media/filters/skcanvas_...
File media/filters/skcanvas_video_renderer_unittest.cc (right):

https://codereview.chromium.org/531353002/diff/160001/media/filters/skcanvas_...
media/filters/skcanvas_video_renderer_unittest.cc:55: void Paint(const
scoped_refptr<VideoFrame> video_frame,
needs &

https://codereview.chromium.org/531353002/diff/160001/media/filters/skcanvas_...
media/filters/skcanvas_video_renderer_unittest.cc:58: void PaintRotated(const
scoped_refptr<VideoFrame> video_frame,
ditto

https://codereview.chromium.org/531353002/diff/160001/media/filters/skcanvas_...
media/filters/skcanvas_video_renderer_unittest.cc:64: void
Copy(scoped_refptr<VideoFrame> video_frame, SkCanvas* canvas);
ditto

[DaleCurtis, 2014-09-09 00:32:53]

drive-by!

https://codereview.chromium.org/531353002/diff/160001/media/filters/skcanvas_...
File media/filters/skcanvas_video_renderer.cc (right):

https://codereview.chromium.org/531353002/diff/160001/media/filters/skcanvas_...
media/filters/skcanvas_video_renderer.cc:72: SkBitmap tmp;
Drive by, you can avoid putting this way up here if you wrap your case with {}

https://codereview.chromium.org/531353002/diff/160001/media/filters/skcanvas_...
media/filters/skcanvas_video_renderer.cc:209: gfx::Size size;
Move inside if block.

[rileya (GONE FROM CHROMIUM), 2014-09-09 00:50:14]

https://codereview.chromium.org/531353002/diff/160001/media/filters/skcanvas_...
File media/filters/skcanvas_video_renderer.cc (right):

https://codereview.chromium.org/531353002/diff/160001/media/filters/skcanvas_...
media/filters/skcanvas_video_renderer.cc:72: SkBitmap tmp;
On 2014/09/09 00:32:53, DaleCurtis wrote:
> Drive by, you can avoid putting this way up here if you wrap your case with {}

Ah, didn't know that, thanks!

https://codereview.chromium.org/531353002/diff/160001/media/filters/skcanvas_...
media/filters/skcanvas_video_renderer.cc:193: size_t rowBytes,
On 2014/09/09 00:27:05, scherkus wrote:
> nit: I think skia has a different naming style, but in  chromium this should
be
> unix_hacker (i.e., row_bytes)

Fixed, I'd wrongly assumed maintaining parity with the naming of the params in
the third party interface took precedence.

https://codereview.chromium.org/531353002/diff/160001/media/filters/skcanvas_...
media/filters/skcanvas_video_renderer.cc:205: if (frame_->format() !=
VideoFrame::YV12J)
On 2014/09/09 00:27:05, scherkus wrote:
> if our Media.VideoPixelFormat UMA data is to believed, we hit YV12J content
> ~0.02% of the time
> 
> are there plans to support other YUV formats? is there a bug, etc?

Yep, skia changes in progress here: https://codereview.chromium.org/516463005/

https://codereview.chromium.org/531353002/diff/160001/media/filters/skcanvas_...
media/filters/skcanvas_video_renderer.cc:206: return false;
On 2014/09/09 00:27:05, scherkus wrote:
> what does returning false entail?

It means we couldn't provide the YUV planes, and so it'll fall back on asking us
for RGB in onGetPixels().

https://codereview.chromium.org/531353002/diff/160001/media/filters/skcanvas_...
media/filters/skcanvas_video_renderer.cc:209: gfx::Size size;
On 2014/09/09 00:32:53, DaleCurtis wrote:
> Move inside if block.

Done.

https://codereview.chromium.org/531353002/diff/160001/media/filters/skcanvas_...
File media/filters/skcanvas_video_renderer.h (right):

https://codereview.chromium.org/531353002/diff/160001/media/filters/skcanvas_...
media/filters/skcanvas_video_renderer.h:33: void Paint(const
scoped_refptr<VideoFrame> video_frame,
On 2014/09/09 00:27:05, scherkus wrote:
> nit: missing a & to make these const-ref

Whoops, think I fixed all these now.

https://codereview.chromium.org/531353002/diff/180001/media/filters/skcanvas_...
File media/filters/skcanvas_video_renderer.cc (right):

https://codereview.chromium.org/531353002/diff/180001/media/filters/skcanvas_...
media/filters/skcanvas_video_renderer.cc:169: default:
Off-topic: Should this be here?

[scherkus (not reviewing), 2014-09-09 00:55:35]

lgtm

https://codereview.chromium.org/531353002/diff/180001/media/filters/skcanvas_...
File media/filters/skcanvas_video_renderer.cc (right):

https://codereview.chromium.org/531353002/diff/180001/media/filters/skcanvas_...
media/filters/skcanvas_video_renderer.cc:169: default:
On 2014/09/09 00:50:14, rileya wrote:
> Off-topic: Should this be here?

no -- being explicit would be preferable

[rileya (GONE FROM CHROMIUM), 2014-09-09 23:53:47]

The CQ bit was checked by rileya@chromium.org

[commit-bot: I haz the power, 2014-09-10 00:07:04]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rileya@chromium.org/531353002/200001

[rileya (GONE FROM CHROMIUM), 2014-09-10 00:15:34]

The CQ bit was unchecked by rileya@chromium.org

[rileya (GONE FROM CHROMIUM), 2014-09-11 18:58:53]

The CQ bit was checked by rileya@chromium.org

[commit-bot: I haz the power, 2014-09-11 19:05:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/531353002/220001

[rileya (GONE FROM CHROMIUM), 2014-09-11 19:11:29]

The CQ bit was checked by rileya@chromium.org

[commit-bot: I haz the power, 2014-09-11 19:12:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/531353002/240001

[commit-bot: I haz the power, 2014-09-11 19:35:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-11 19:35:08]

Try jobs failed on following builders:
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[rileya (GONE FROM CHROMIUM), 2014-09-11 20:47:44]

The CQ bit was checked by rileya@chromium.org

[commit-bot: I haz the power, 2014-09-11 20:58:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/531353002/260001

[commit-bot: I haz the power, 2014-09-11 23:25:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-11 23:25:11]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[rileya (GONE FROM CHROMIUM), 2014-09-13 00:49:48]

The CQ bit was checked by rileya@chromium.org

[commit-bot: I haz the power, 2014-09-13 00:52:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/531353002/280001

[commit-bot: I haz the power, 2014-09-13 02:12:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-13 02:12:30]

Try jobs failed on following builders:
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[rileya1, 2014-09-14 03:02:40]

Patchset #18 (id:340001) has been deleted

[rileya (GONE FROM CHROMIUM), 2014-09-15 20:10:18]

The CQ bit was checked by rileya@chromium.org

[commit-bot: I haz the power, 2014-09-15 20:12:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/531353002/320001

[commit-bot: I haz the power, 2014-09-15 21:29:05]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[rileya (GONE FROM CHROMIUM), 2014-09-15 23:54:21]

The CQ bit was checked by rileya@chromium.org

[commit-bot: I haz the power, 2014-09-16 00:00:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/531353002/380001

[commit-bot: I haz the power, 2014-09-16 02:21:51]

Committed patchset #19 (id:380001) as 843d3750b821694b47476a5a69f7e8c42ac565c1

[commit-bot: I haz the power, 2014-09-16 02:23:25]

Patchset 19 (id:??) landed as
https://crrev.com/54b8c174c8420abc0bc519eb032b1289029fcc48
Cr-Commit-Position: refs/heads/master@{#294981}