Clean up video frame sizes, types in Video Decode API

Remove redundant size and type information from VideoDecode PPAPI

The visible size of the video frame is determined at Create stage. 
The type of buffer (from system memory or texture-backed via GLES) 
is decided by the plugin when it calls AssignGLESBuffers or 
AssignSysmemBuffers in response to ProvidePictureBuffers. 
The decoded size will be the same as the size requested in
ProvidePictureBuffers.

Thus these values specified anywhere else is redundant, and introduces
a place to have inconsistency.

BUG=NONE
TEST=ovda unittest passes; gles2 runs w/o crashing

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

[vrk (LEFT CHROMIUM), 2011-05-16 20:07:19]

A small clean-up patch as I rework the gpu_video_decode_accelerator_tester.

[vrk (LEFT CHROMIUM), 2011-05-16 20:08:23]

On 2011/05/16 20:07:19, Victoria Kirst wrote:
> A small clean-up patch as I rework the gpu_video_decode_accelerator_tester.

Bleh, got some of the names wrong in the description. Fixing now...

[vrk (LEFT CHROMIUM), 2011-05-16 20:10:58]

On 2011/05/16 20:08:23, Victoria Kirst wrote:
> On 2011/05/16 20:07:19, Victoria Kirst wrote:
> > A small clean-up patch as I rework the gpu_video_decode_accelerator_tester.
> 
> Bleh, got some of the names wrong in the description. Fixing now...

... aaaand fixed. Sorry for spam there :)

[Ami GONE FROM CHROMIUM, 2011-05-16 20:17:49]

Nice.  There's no code like no code!

LGTM modulo the one question.

http://codereview.chromium.org/7021020/diff/5001/ppapi/c/dev/ppp_video_decode...
File ppapi/c/dev/ppp_video_decoder_dev.h (left):

http://codereview.chromium.org/7021020/diff/5001/ppapi/c/dev/ppp_video_decode...
ppapi/c/dev/ppp_video_decoder_dev.h:32: enum PP_PictureBufferType_Dev type);
Previously, the API allowed the decoder to specify which type of buffers it
wants to render into.  This change makes it look like the plugin gets to decide
what to provide to the decoder.  Intentional?

[vrk (LEFT CHROMIUM), 2011-05-16 20:28:37]

+brettw OWNERS review

http://codereview.chromium.org/7021020/diff/5001/ppapi/c/dev/ppp_video_decode...
File ppapi/c/dev/ppp_video_decoder_dev.h (left):

http://codereview.chromium.org/7021020/diff/5001/ppapi/c/dev/ppp_video_decode...
ppapi/c/dev/ppp_video_decoder_dev.h:32: enum PP_PictureBufferType_Dev type);
On 2011/05/16 20:17:50, Ami Fischman wrote:
> Previously, the API allowed the decoder to specify which type of buffers it
> wants to render into.  This change makes it look like the plugin gets to
decide
> what to provide to the decoder.  Intentional?

Yes, intentional. (I think we talked about it on Friday that it seems to make
more sense this way; decoder should never have "preference" for output buffer
type, but the plugin might.)

[brettw, 2011-05-16 20:51:12]

Rubber stamp LGTM (didn't check the details).

[Ville-Mikko Rautio, 2011-05-17 08:58:48]

Hi Victoria and others,

I gave a pass on the interfaces which I believe were the most interesting part
of this CL.

Ville-Mikko

http://codereview.chromium.org/7021020/diff/5001/media/video/picture.h
File media/video/picture.h (left):

http://codereview.chromium.org/7021020/diff/5001/media/video/picture.h#oldcod...
media/video/picture.h:125: gfx::Size decoded_size_;
After removing these how do we plan to handle situation where the picture size
changes due to in-band signalling in video bitstream? This means a case where
application does not see the change in picture parameters, only the decoder gets
to know about it through decoding the input video bitstream with new parameters.

This is valid use case in most modern video coding standards, including H.264
and VP8.

http://codereview.chromium.org/7021020/diff/5001/media/video/video_decode_acc...
File media/video/video_decode_accelerator.h (left):

http://codereview.chromium.org/7021020/diff/5001/media/video/video_decode_acc...
media/video/video_decode_accelerator.h:178: };
See the comment in pp_video_dev.h

http://codereview.chromium.org/7021020/diff/5001/ppapi/c/dev/pp_video_dev.h
File ppapi/c/dev/pp_video_dev.h (left):

http://codereview.chromium.org/7021020/diff/5001/ppapi/c/dev/pp_video_dev.h#o...
ppapi/c/dev/pp_video_dev.h:165: 
How do we signal in the configuration the case where decoder supports only
system memory buffers or GLES textures, i.e. not both?

http://codereview.chromium.org/7021020/diff/5001/ppapi/c/dev/ppp_video_decode...
File ppapi/c/dev/ppp_video_decoder_dev.h (left):

http://codereview.chromium.org/7021020/diff/5001/ppapi/c/dev/ppp_video_decode...
ppapi/c/dev/ppp_video_decoder_dev.h:32: enum PP_PictureBufferType_Dev type);
On 2011/05/16 20:28:37, Victoria Kirst wrote:
> On 2011/05/16 20:17:50, Ami Fischman wrote:
> > Previously, the API allowed the decoder to specify which type of buffers it
> > wants to render into.  This change makes it look like the plugin gets to
> decide
> > what to provide to the decoder.  Intentional?
> 
> Yes, intentional. (I think we talked about it on Friday that it seems to make
> more sense this way; decoder should never have "preference" for output buffer
> type, but the plugin might.)

But it is possible that the decoder implementation might support only one of the
types. See previous comment.

[scherkus (not reviewing), 2011-05-23 05:46:57]

does this still need to be reviewed?

[vrk (LEFT CHROMIUM), 2011-05-23 18:20:28]

Forgot about this patch; distracted with the gpu tester! Added responses to
vmr's concerns.

http://codereview.chromium.org/7021020/diff/5001/media/video/picture.h
File media/video/picture.h (left):

http://codereview.chromium.org/7021020/diff/5001/media/video/picture.h#oldcod...
media/video/picture.h:125: gfx::Size decoded_size_;
On 2011/05/17 08:58:49, vmr1 wrote:
> After removing these how do we plan to handle situation where the picture size
> changes due to in-band signalling in video bitstream? This means a case where
> application does not see the change in picture parameters, only the decoder
gets
> to know about it through decoding the input video bitstream with new
parameters.
> 
> This is valid use case in most modern video coding standards, including H.264
> and VP8.

If the decoded picture size changes, the decoder will dismiss the currently
assigned picture buffers and will request new picture buffers of the proper
size.

The output size is still always determined by the client.

http://codereview.chromium.org/7021020/diff/5001/ppapi/c/dev/pp_video_dev.h
File ppapi/c/dev/pp_video_dev.h (left):

http://codereview.chromium.org/7021020/diff/5001/ppapi/c/dev/pp_video_dev.h#o...
ppapi/c/dev/pp_video_dev.h:165: 
On 2011/05/17 08:58:49, vmr1 wrote:
> How do we signal in the configuration the case where decoder supports only
> system memory buffers or GLES textures, i.e. not both?

I don't think this is a valid case. To the decoder, system memory versus GLES
texture is really just a difference between where to write bytes. A GPU decoder
will always be able to decode into textures, and it will also be able to decode
into shared memory.

http://codereview.chromium.org/7021020/diff/5001/ppapi/c/dev/ppp_video_decode...
File ppapi/c/dev/ppp_video_decoder_dev.h (left):

http://codereview.chromium.org/7021020/diff/5001/ppapi/c/dev/ppp_video_decode...
ppapi/c/dev/ppp_video_decoder_dev.h:32: enum PP_PictureBufferType_Dev type);
On 2011/05/17 08:58:49, vmr1 wrote:
> On 2011/05/16 20:28:37, Victoria Kirst wrote:
> > On 2011/05/16 20:17:50, Ami Fischman wrote:
> > > Previously, the API allowed the decoder to specify which type of buffers
it
> > > wants to render into.  This change makes it look like the plugin gets to
> > decide
> > > what to provide to the decoder.  Intentional?
> > 
> > Yes, intentional. (I think we talked about it on Friday that it seems to
make
> > more sense this way; decoder should never have "preference" for output
buffer
> > type, but the plugin might.)
> 
> But it is possible that the decoder implementation might support only one of
the
> types. See previous comment.

I don't think this is true; see response.

[vmr, 2011-05-24 07:11:11]

http://codereview.chromium.org/7021020/diff/5001/ppapi/c/dev/pp_video_dev.h
File ppapi/c/dev/pp_video_dev.h (left):

http://codereview.chromium.org/7021020/diff/5001/ppapi/c/dev/pp_video_dev.h#o...
ppapi/c/dev/pp_video_dev.h:165: 
On 2011/05/23 18:20:29, Victoria Kirst wrote:
> On 2011/05/17 08:58:49, vmr1 wrote:
> > How do we signal in the configuration the case where decoder supports only
> > system memory buffers or GLES textures, i.e. not both?
> 
> I don't think this is a valid case. To the decoder, system memory versus GLES
> texture is really just a difference between where to write bytes. A GPU
decoder
> will always be able to decode into textures, and it will also be able to
decode
> into shared memory.

Even though theoretically these differences are just different decoding targets
I do think there is difference between the cases in practice. For example, in
case of textures OMX decoder goes into EGLImage decoding path whereas when
decoding to system memory there is separate decoding path used. In this case the
functions I am talking about are OMX_UseEGLImage and OMX_UseBuffer. I know for
fact that there are number of OMX implementations out there in the world that
support only the latter. I know our first target has the OMX_UseEGLImage but it
will be a problem for other vendors who may try to create Chrome OS devices.

[Ami GONE FROM CHROMIUM, 2011-05-24 07:29:06]

>
> I know for fact that there are number of OMX implementations out there in
> the world that support only the latter.


Let's worry about that when we need to.  APIs are much easier to widen than
to narrow.

[piman, 2011-05-24 16:14:53]

On Tue, May 24, 2011 at 12:11 AM, <vmr@google.com> wrote:

>
> http://codereview.chromium.org/7021020/diff/5001/ppapi/c/dev/pp_video_dev.h
> File ppapi/c/dev/pp_video_dev.h (left):
>
>
>
http://codereview.chromium.org/7021020/diff/5001/ppapi/c/dev/pp_video_dev.h#o...
> ppapi/c/dev/pp_video_dev.h:165:
> On 2011/05/23 18:20:29, Victoria Kirst wrote:
>
>> On 2011/05/17 08:58:49, vmr1 wrote:
>> > How do we signal in the configuration the case where decoder
>>
> supports only
>
>> > system memory buffers or GLES textures, i.e. not both?
>>
>
>  I don't think this is a valid case. To the decoder, system memory
>>
> versus GLES
>
>> texture is really just a difference between where to write bytes. A
>>
> GPU decoder
>
>> will always be able to decode into textures, and it will also be able
>>
> to decode
>
>> into shared memory.
>>
>
> Even though theoretically these differences are just different decoding
> targets I do think there is difference between the cases in practice.
> For example, in case of textures OMX decoder goes into EGLImage decoding
> path whereas when decoding to system memory there is separate decoding
> path used. In this case the functions I am talking about are
> OMX_UseEGLImage and OMX_UseBuffer. I know for fact that there are number
> of OMX implementations out there in the world that support only the
> latter. I know our first target has the OMX_UseEGLImage but it will be a
> problem for other vendors who may try to create Chrome OS devices.


For Chrome OS, we can, and will require the vendor to support decoding into
a texture.
In any case, even if it doesn't do it in the driver, we can always do it in
the GPU process (upload to the texture after each decode), completely hiding
it from the plugin.
If anything, the difficult case is supporting decoding to a memory buffer if
the driver only decodes to a texture (because a readback may be very
impractical), but I'm 100% happy dropping support for decoding to a memory
buffer in the Pepper API.



>
>
> http://codereview.chromium.org/7021020/
>

[Ami GONE FROM CHROMIUM, 2011-07-08 20:54:33]

Time to rebase & submit this?

[vrk (LEFT CHROMIUM), 2011-07-18 18:19:42]

On 2011/07/08 20:54:33, Ami Fischman wrote:
> Time to rebase & submit this?

Yes indeed!

Rebased and resubmitted. It's largely identical; there are just a few extra
files to modify. PTAL?

[Ami GONE FROM CHROMIUM, 2011-07-18 18:32:59]

Yay!

http://codereview.chromium.org/7021020/diff/20001/media/video/video_decode_ac...
File media/video/video_decode_accelerator.h (left):

http://codereview.chromium.org/7021020/diff/20001/media/video/video_decode_ac...
media/video/video_decode_accelerator.h:185: // Represents the type of data
buffer to be used by the decoder.
Do you want to DO the TODO from picture.h now, too?

http://codereview.chromium.org/7021020/diff/20001/ppapi/c/dev/pp_video_dev.h
File ppapi/c/dev/pp_video_dev.h (left):

http://codereview.chromium.org/7021020/diff/20001/ppapi/c/dev/pp_video_dev.h#...
ppapi/c/dev/pp_video_dev.h:169: enum PP_PictureBufferType_Dev {
Technically requires bumping the PPB_ version number, too?

http://codereview.chromium.org/7021020/diff/20001/ppapi/c/dev/ppp_video_decod...
File ppapi/c/dev/ppp_video_decoder_dev.h (right):

http://codereview.chromium.org/7021020/diff/20001/ppapi/c/dev/ppp_video_decod...
ppapi/c/dev/ppp_video_decoder_dev.h:12: #define PPP_VIDEODECODER_DEV_INTERFACE
"PPP_VideoDecoder(Dev);0.5"
bump

http://codereview.chromium.org/7021020/diff/20001/webkit/plugins/ppapi/ppb_vi...
File webkit/plugins/ppapi/ppb_video_decoder_impl.cc (left):

http://codereview.chromium.org/7021020/diff/20001/webkit/plugins/ppapi/ppb_vi...
webkit/plugins/ppapi/ppb_video_decoder_impl.cc:189: // a blind cast.
Does this TODO not apply elsewhere?

[vrk (LEFT CHROMIUM), 2011-07-19 01:26:08]

GLESBuffer -> PictureBuffer

Added more (tediousness) to the CL, though!

http://codereview.chromium.org/7021020/diff/20001/media/video/video_decode_ac...
File media/video/video_decode_accelerator.h (left):

http://codereview.chromium.org/7021020/diff/20001/media/video/video_decode_ac...
media/video/video_decode_accelerator.h:185: // Represents the type of data
buffer to be used by the decoder.
On 2011/07/18 18:33:00, Ami Fischman wrote:
> Do you want to DO the TODO from picture.h now, too?

Done.

http://codereview.chromium.org/7021020/diff/20001/ppapi/c/dev/pp_video_dev.h
File ppapi/c/dev/pp_video_dev.h (left):

http://codereview.chromium.org/7021020/diff/20001/ppapi/c/dev/pp_video_dev.h#...
ppapi/c/dev/pp_video_dev.h:169: enum PP_PictureBufferType_Dev {
On 2011/07/18 18:33:00, Ami Fischman wrote:
> Technically requires bumping the PPB_ version number, too?

Makes sense; done.

http://codereview.chromium.org/7021020/diff/20001/ppapi/c/dev/ppp_video_decod...
File ppapi/c/dev/ppp_video_decoder_dev.h (right):

http://codereview.chromium.org/7021020/diff/20001/ppapi/c/dev/ppp_video_decod...
ppapi/c/dev/ppp_video_decoder_dev.h:12: #define PPP_VIDEODECODER_DEV_INTERFACE
"PPP_VideoDecoder(Dev);0.5"
On 2011/07/18 18:33:00, Ami Fischman wrote:
> bump

Done.

http://codereview.chromium.org/7021020/diff/20001/webkit/plugins/ppapi/ppb_vi...
File webkit/plugins/ppapi/ppb_video_decoder_impl.cc (left):

http://codereview.chromium.org/7021020/diff/20001/webkit/plugins/ppapi/ppb_vi...
webkit/plugins/ppapi/ppb_video_decoder_impl.cc:189: // a blind cast.
On 2011/07/18 18:33:00, Ami Fischman wrote:
> Does this TODO not apply elsewhere?

Looks like there are TODOs written in all the other places where we do something
like this in this file.

[Ami GONE FROM CHROMIUM, 2011-07-19 02:55:08]

LGTM
Ship it!