Support reading pixels from HW-decoded video textures into canvas/webgl.

Support reading pixels from HW-decoded video textures into canvas/webgl.

BUG=153094
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=159571

[Ami GONE FROM CHROMIUM, 2012-10-01 17:35:01]

kbr/scherkus: please review

[Ken Russell (switch to Gerrit), 2012-10-01 17:56:16]

LGTM

https://chromiumcodereview.appspot.com/11016006/diff/5001/content/renderer/me...
File content/renderer/media/renderer_gpu_video_decoder_factories.cc (right):

https://chromiumcodereview.appspot.com/11016006/diff/5001/content/renderer/me...
content/renderer/media/renderer_gpu_video_decoder_factories.cc:190:
gles2->ReadPixels(0, 0, size.width(), size.height(), GL_RGBA,
Depending on who uses this context (just HW video decode? the compositor as
well?) it may be worth calling glPixelStorei(GL_PACK_ALIGNMENT, 4); before doing
the glReadPixels, to make sure that someone else didn't set it to 8-byte
alignment.

https://chromiumcodereview.appspot.com/11016006/diff/5001/media/base/video_fr...
File media/base/video_frame.h (right):

https://chromiumcodereview.appspot.com/11016006/diff/5001/media/base/video_fr...
media/base/video_frame.h:79: // them to |*pixels|.
Document the format of the pixels (i.e., RGBA, in that order, rows tightly
packed). May be worth having caller pass in the size of the storage and
returning false if it isn't large enough.

https://chromiumcodereview.appspot.com/11016006/diff/5001/media/filters/gpu_v...
File media/filters/gpu_video_decoder.h (right):

https://chromiumcodereview.appspot.com/11016006/diff/5001/media/filters/gpu_v...
media/filters/gpu_video_decoder.h:50: // Read pixels from a native texture.
Again, document format of pixels.

[scherkus (not reviewing), 2012-10-01 18:15:04]

lgtm w/ nits

https://chromiumcodereview.appspot.com/11016006/diff/5001/media/base/video_fr...
File media/base/video_frame.h (right):

https://chromiumcodereview.appspot.com/11016006/diff/5001/media/base/video_fr...
media/base/video_frame.h:79: // them to |*pixels|.
On 2012/10/01 17:56:16, kbr wrote:
> Document the format of the pixels (i.e., RGBA, in that order, rows tightly
> packed). May be worth having caller pass in the size of the storage and
> returning false if it isn't large enough.

+1,000,000

I don't know if our hardware decode path supports frame size changes, but it
seems that we could make a contract about |pixels| pointing to memory that
matches the dimensions of the video frame

For example, in SkCVR we adjust our allocation of |bitmap| in case dimensions
have changed.

https://chromiumcodereview.appspot.com/11016006/diff/5001/webkit/media/skcanv...
File webkit/media/skcanvas_video_renderer.cc (right):

https://chromiumcodereview.appspot.com/11016006/diff/5001/webkit/media/skcanv...
webkit/media/skcanvas_video_renderer.cc:21: format ==
media::VideoFrame::NATIVE_TEXTURE;
indent

https://chromiumcodereview.appspot.com/11016006/diff/5001/webkit/media/skcanv...
webkit/media/skcanvas_video_renderer.cc:176:
DCHECK(video_frame->stride(media::VideoFrame::kUPlane) ==
no _EQ love?

https://chromiumcodereview.appspot.com/11016006/diff/5001/webkit/media/skcanv...
webkit/media/skcanvas_video_renderer.cc:194: (video_frame->format() ==
media::VideoFrame::YV12) ?
fix indent

[Ami GONE FROM CHROMIUM, 2012-10-01 18:16:12]

https://chromiumcodereview.appspot.com/11016006/diff/5001/content/renderer/me...
File content/renderer/media/renderer_gpu_video_decoder_factories.cc (right):

https://chromiumcodereview.appspot.com/11016006/diff/5001/content/renderer/me...
content/renderer/media/renderer_gpu_video_decoder_factories.cc:190:
gles2->ReadPixels(0, 0, size.width(), size.height(), GL_RGBA,
On 2012/10/01 17:56:16, kbr wrote:
> Depending on who uses this context (just HW video decode? the compositor as
> well?)

Only HW decode uses it.

> it may be worth calling glPixelStorei(GL_PACK_ALIGNMENT, 4); before doing
> the glReadPixels, to make sure that someone else didn't set it to 8-byte
> alignment.

Done.

https://chromiumcodereview.appspot.com/11016006/diff/5001/media/base/video_fr...
File media/base/video_frame.h (right):

https://chromiumcodereview.appspot.com/11016006/diff/5001/media/base/video_fr...
media/base/video_frame.h:79: // them to |*pixels|.
On 2012/10/01 17:56:16, kbr wrote:
> Document the format of the pixels (i.e., RGBA, in that order, rows tightly
> packed).

Done.

> May be worth having caller pass in the size of the storage and
> returning false if it isn't large enough.

I don't think that's worthwhile, since the number of ways this can go wrong is
so large.

https://chromiumcodereview.appspot.com/11016006/diff/5001/media/filters/gpu_v...
File media/filters/gpu_video_decoder.h (right):

https://chromiumcodereview.appspot.com/11016006/diff/5001/media/filters/gpu_v...
media/filters/gpu_video_decoder.h:50: // Read pixels from a native texture.
On 2012/10/01 17:56:16, kbr wrote:
> Again, document format of pixels.

Done.

[Ami GONE FROM CHROMIUM, 2012-10-01 18:40:26]

https://chromiumcodereview.appspot.com/11016006/diff/5001/media/base/video_fr...
File media/base/video_frame.h (right):

https://chromiumcodereview.appspot.com/11016006/diff/5001/media/base/video_fr...
media/base/video_frame.h:79: // them to |*pixels|.
On 2012/10/01 18:15:04, scherkus wrote:
> On 2012/10/01 17:56:16, kbr wrote:
> > Document the format of the pixels (i.e., RGBA, in that order, rows tightly
> > packed). May be worth having caller pass in the size of the storage and
> > returning false if it isn't large enough.
> 
> +1,000,000
> 
> I don't know if our hardware decode path supports frame size changes

It does not.

> but it
> seems that we could make a contract about |pixels| pointing to memory that
> matches the dimensions of the video frame

At some point you have to hand the pointer to GL, and at that point you'd better
have not made a mistake in sizing its allocation.

Since there is a multitude of layers between the allocating code and the using
code, I see two options:
1) Pass the size data alongside the pointer everywhere it goes and DCHECK at
every layer.
2) Have *none* of the intermediate layers worry about the size, since they can't
do anything about it.

> For example, in SkCVR we adjust our allocation of |bitmap| in case dimensions
> have changed.

Perhaps your +1e6 of worry is allayed by the knowledge that we malloc the pixels
for every frame?

(basically, VideoFrame is the only piece that knows the correct sizing for
pixels; none of the other layers can assert anything else)

https://chromiumcodereview.appspot.com/11016006/diff/5001/webkit/media/skcanv...
File webkit/media/skcanvas_video_renderer.cc (right):

https://chromiumcodereview.appspot.com/11016006/diff/5001/webkit/media/skcanv...
webkit/media/skcanvas_video_renderer.cc:21: format ==
media::VideoFrame::NATIVE_TEXTURE;
On 2012/10/01 18:15:04, scherkus wrote:
> indent

Done.

https://chromiumcodereview.appspot.com/11016006/diff/5001/webkit/media/skcanv...
webkit/media/skcanvas_video_renderer.cc:176:
DCHECK(video_frame->stride(media::VideoFrame::kUPlane) ==
On 2012/10/01 18:15:04, scherkus wrote:
> no _EQ love?

Done.

https://chromiumcodereview.appspot.com/11016006/diff/5001/webkit/media/skcanv...
webkit/media/skcanvas_video_renderer.cc:194: (video_frame->format() ==
media::VideoFrame::YV12) ?
On 2012/10/01 18:15:04, scherkus wrote:
> fix indent

Done.

[commit-bot: I haz the power, 2012-10-01 18:40:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fischman@chromium.org/11016006/10001

[scherkus (not reviewing), 2012-10-01 18:52:06]

https://chromiumcodereview.appspot.com/11016006/diff/5001/media/base/video_fr...
File media/base/video_frame.h (right):

https://chromiumcodereview.appspot.com/11016006/diff/5001/media/base/video_fr...
media/base/video_frame.h:79: // them to |*pixels|.
On 2012/10/01 18:40:26, Ami Fischman wrote:
> On 2012/10/01 18:15:04, scherkus wrote:
> > On 2012/10/01 17:56:16, kbr wrote:
> > > Document the format of the pixels (i.e., RGBA, in that order, rows tightly
> > > packed). May be worth having caller pass in the size of the storage and
> > > returning false if it isn't large enough.
> > 
> > +1,000,000
> > 
> > I don't know if our hardware decode path supports frame size changes
> 
> It does not.
> 
> > but it
> > seems that we could make a contract about |pixels| pointing to memory that
> > matches the dimensions of the video frame
> 
> At some point you have to hand the pointer to GL, and at that point you'd
better
> have not made a mistake in sizing its allocation.
> 
> Since there is a multitude of layers between the allocating code and the using
> code, I see two options:
> 1) Pass the size data alongside the pointer everywhere it goes and DCHECK at
> every layer.
> 2) Have *none* of the intermediate layers worry about the size, since they
can't
> do anything about it.
> 
> > For example, in SkCVR we adjust our allocation of |bitmap| in case
dimensions
> > have changed.
> 
> Perhaps your +1e6 of worry is allayed by the knowledge that we malloc the
pixels
> for every frame?

Where "we" == SkCVR?

So the best practice for using this function is documented as code in SkCVR as
opposed to here?

I'm not advocating plumbing but a mention of, say, assumptions on dimensions
might be nice.

[Ami GONE FROM CHROMIUM, 2012-10-01 19:26:39]

https://chromiumcodereview.appspot.com/11016006/diff/5001/media/base/video_fr...
File media/base/video_frame.h (right):

https://chromiumcodereview.appspot.com/11016006/diff/5001/media/base/video_fr...
media/base/video_frame.h:79: // them to |*pixels|.
On 2012/10/01 18:52:06, scherkus wrote:
> On 2012/10/01 18:40:26, Ami Fischman wrote:
> > On 2012/10/01 18:15:04, scherkus wrote:
> > > On 2012/10/01 17:56:16, kbr wrote:
> > > > Document the format of the pixels (i.e., RGBA, in that order, rows
tightly
> > > > packed). May be worth having caller pass in the size of the storage and
> > > > returning false if it isn't large enough.
> > > 
> > > +1,000,000
> > > 
> > > I don't know if our hardware decode path supports frame size changes
> > 
> > It does not.
> > 
> > > but it
> > > seems that we could make a contract about |pixels| pointing to memory that
> > > matches the dimensions of the video frame
> > 
> > At some point you have to hand the pointer to GL, and at that point you'd
> better
> > have not made a mistake in sizing its allocation.
> > 
> > Since there is a multitude of layers between the allocating code and the
using
> > code, I see two options:
> > 1) Pass the size data alongside the pointer everywhere it goes and DCHECK at
> > every layer.
> > 2) Have *none* of the intermediate layers worry about the size, since they
> can't
> > do anything about it.
> > 
> > > For example, in SkCVR we adjust our allocation of |bitmap| in case
> dimensions
> > > have changed.
> > 
> > Perhaps your +1e6 of worry is allayed by the knowledge that we malloc the
> pixels
> > for every frame?
> 
> Where "we" == SkCVR?
> 
> So the best practice for using this function is documented as code in SkCVR as
> opposed to here?
> 
> I'm not advocating plumbing but a mention of, say, assumptions on dimensions
> might be nice.

OIC; done.

[commit-bot: I haz the power, 2012-10-01 19:26:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fischman@chromium.org/11016006/11010

[commit-bot: I haz the power, 2012-10-01 22:09:43]

Change committed as 159571