Update VideoDecode PPAPI structs to be consistent with media structures, part 1

Update VideoDecode PPAPI structs to be consistent with media structures, part 1

This patch fixes some of the inconsistencies between the VideoDecode structs
in the ppapi namespace and their wrappers in the media namespace.
It also implements some of the hooks to pass messages to and from the browser
and plugin.

BUG=NONE
TEST=NONE

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

[vrk (LEFT CHROMIUM), 2011-04-26 20:29:12]

The data structures checked into Chromium right now are pretty broken. This
patch doesn't fix everything, but it addresses some of the most essential stuff.
Thought I would start from here and submit more patches soon!

One thing that I wasn't sure about was the conversions between the media C++
objects and the PPAPI C structs. For example, in
PPB_VideoDecoder_Impl::PictureReady, we are taking a PictureBuffer and need to
hand it to the plugin as a PP_PictureBuffer_Dev*. But I'm not sure why it needs
to be a pointer... since the object will need to exist after the function call
returns, right now I am mallocing a new PP_PictureBuffer_Dev and passing the raw
pointer along to the plugin. That seems scary and wrong.

Would definitely appreciate another pair of eyes on this!

[piman, 2011-04-26 20:48:07]

http://codereview.chromium.org/6901036/diff/1/ppapi/c/dev/ppb_video_decoder_d...
File ppapi/c/dev/ppb_video_decoder_dev.h (right):

http://codereview.chromium.org/6901036/diff/1/ppapi/c/dev/ppb_video_decoder_d...
ppapi/c/dev/ppb_video_decoder_dev.h:12: #define PPB_VIDEODECODER_DEV_INTERFACE
"PPB_VideoDecoder(Dev);0.5"
Drive-by: please bump the version if you change the interface.

http://codereview.chromium.org/6901036/diff/1/ppapi/c/dev/ppp_video_decoder_d...
File ppapi/c/dev/ppp_video_decoder_dev.h (right):

http://codereview.chromium.org/6901036/diff/1/ppapi/c/dev/ppp_video_decoder_d...
ppapi/c/dev/ppp_video_decoder_dev.h:10: #define PPP_VIDEODECODER_DEV_INTERFACE
"PPP_VideoDecoder(Dev);0.1"
Drive-by: please bump the version if you change the interface.

[scherkus (not reviewing), 2011-04-26 22:36:34]

cleanup patches are also welcome!

mostly curious about using ref as opposed to const-ref or pointer

http://codereview.chromium.org/6901036/diff/1/media/video/video_decode_accele...
File media/video/video_decode_accelerator.h (right):

http://codereview.chromium.org/6901036/diff/1/media/video/video_decode_accele...
media/video/video_decode_accelerator.h:333: virtual void PictureReady(Picture&
picture) = 0;
why ref versus pointer?

if ref we usually go with a const-ref if possible

http://codereview.chromium.org/6901036/diff/1/webkit/plugins/ppapi/ppb_video_...
File webkit/plugins/ppapi/ppb_video_decoder_impl.cc (right):

http://codereview.chromium.org/6901036/diff/1/webkit/plugins/ppapi/ppb_video_...
webkit/plugins/ppapi/ppb_video_decoder_impl.cc:18: #include
"webkit/plugins/ppapi/var.h"
nit: include order

http://codereview.chromium.org/6901036/diff/1/webkit/plugins/ppapi/ppb_video_...
webkit/plugins/ppapi/ppb_video_decoder_impl.cc:308: if (input.storage_type == 
PP_PICTUREBUFFERTYPE_SYSTEM) {
nit: two spaces should be one

http://codereview.chromium.org/6901036/diff/1/webkit/plugins/ppapi/ppb_video_...
webkit/plugins/ppapi/ppb_video_decoder_impl.cc:321: NOTREACHED();
warning: on release builds this will be optimized out (it's a DCHECK(false))

in which case data_plane might be uninitialized and get pushed onto
data_plan_handles etc etc

perhaps return NULL?

can we signal an error?

http://codereview.chromium.org/6901036/diff/1/webkit/plugins/ppapi/ppb_video_...
webkit/plugins/ppapi/ppb_video_decoder_impl.cc:344: malloc(sizeof(struct
PP_Picture_Dev));
not a pepper master but I'm not seeing any other ppb *.cc files use malloc+cast
but instead use new

I guess since this is C++ new should be OK as long as it's still C++ that still
de-allocates the pictures

http://codereview.chromium.org/6901036/diff/1/webkit/plugins/ppapi/ppb_video_...
webkit/plugins/ppapi/ppb_video_decoder_impl.cc:373: // TODO: Does this really
need to be a ptr?
todo for who?

http://codereview.chromium.org/6901036/diff/1/webkit/plugins/ppapi/ppb_video_...
File webkit/plugins/ppapi/ppb_video_decoder_impl.h (right):

http://codereview.chromium.org/6901036/diff/1/webkit/plugins/ppapi/ppb_video_...
webkit/plugins/ppapi/ppb_video_decoder_impl.h:62:
media::VideoDecodeAccelerator::Picture& picture) OVERRIDE;
why ref versus pointer?

if ref we usually go with a const-ref if possible

http://codereview.chromium.org/6901036/diff/1/webkit/plugins/ppapi/ppb_video_...
webkit/plugins/ppapi/ppb_video_decoder_impl.h:69:
CreateMediaPictureBuffer(struct PP_PictureBuffer_Dev& input);
nit: c++ doesn't need struct

also ref vs pointer vs const-ref

http://codereview.chromium.org/6901036/diff/1/webkit/plugins/ppapi/ppb_video_...
webkit/plugins/ppapi/ppb_video_decoder_impl.h:72: CreateMediaPicture(struct
PP_Picture_Dev& input);
nit: c++ doesn't need struct

also ref vs pointer vs const-ref

http://codereview.chromium.org/6901036/diff/1/webkit/plugins/ppapi/ppb_video_...
webkit/plugins/ppapi/ppb_video_decoder_impl.h:74: static struct PP_Picture_Dev*
CreatePictureDev(
nit: c++ doesn't need struct

also ref vs pointer vs const-ref

[scherkus (not reviewing), 2011-04-26 22:37:06]

On 2011/04/26 22:36:34, scherkus wrote:
> cleanup patches are also welcome!

blah typo

s/also/most/

[Ami GONE FROM CHROMIUM, 2011-04-26 23:01:56]

See next-to-last comment for my thoughts on your message.

http://codereview.chromium.org/6901036/diff/1/ppapi/c/dev/pp_video_dev.h
File ppapi/c/dev/pp_video_dev.h (right):

http://codereview.chromium.org/6901036/diff/1/ppapi/c/dev/pp_video_dev.h#newc...
ppapi/c/dev/pp_video_dev.h:298: uint32_t width;
WDYT, worth using PP_Size?

http://codereview.chromium.org/6901036/diff/1/ppapi/c/dev/ppb_video_decoder_d...
File ppapi/c/dev/ppb_video_decoder_dev.h (right):

http://codereview.chromium.org/6901036/diff/1/ppapi/c/dev/ppb_video_decoder_d...
ppapi/c/dev/ppb_video_decoder_dev.h:12: #define PPB_VIDEODECODER_DEV_INTERFACE
"PPB_VideoDecoder(Dev);0.5"
On 2011/04/26 20:48:07, piman wrote:
> Drive-by: please bump the version if you change the interface.

Is 0.10 greater or smaller than 0.9?  If smaller (as in math) then I propose the
version bump here be 0.500001, since it's feeling like a lot of these interfaces
are going to have a lot more CLs before they're ready for a 1.0 :)

http://codereview.chromium.org/6901036/diff/1/ppapi/tests/arch_dependent_size...
File ppapi/tests/arch_dependent_sizes_64.h (right):

http://codereview.chromium.org/6901036/diff/1/ppapi/tests/arch_dependent_size...
ppapi/tests/arch_dependent_sizes_64.h:20:
PP_COMPILE_ASSERT_STRUCT_SIZE_IN_BYTES(PP_Picture_Dev, 20);
I wonder what the value of these assertions is.

http://codereview.chromium.org/6901036/diff/1/webkit/plugins/ppapi/ppb_video_...
File webkit/plugins/ppapi/ppb_video_decoder_impl.cc (right):

http://codereview.chromium.org/6901036/diff/1/webkit/plugins/ppapi/ppb_video_...
webkit/plugins/ppapi/ppb_video_decoder_impl.cc:261: const PPP_VideoDecoder_Dev*
ppp_videodecoder =
worth doing this once in the ctor to avoid this 3-line stanza that repeats
multiple times below?

http://codereview.chromium.org/6901036/diff/1/webkit/plugins/ppapi/ppb_video_...
webkit/plugins/ppapi/ppb_video_decoder_impl.cc:265: struct PP_Picture_Dev*
out_pic = CreatePictureDev(picture);
I think this leaks all out_pics?
To make the current API work, you need to keep your own map from
picture_buffer_id to the out_pic that currently points to it, so in
{Dismiss,Reuse}PictureBuffer(id) you can free/reuse the out_pic.

http://codereview.chromium.org/6901036/diff/1/webkit/plugins/ppapi/ppb_video_...
webkit/plugins/ppapi/ppb_video_decoder_impl.cc:308: if (input.storage_type == 
PP_PICTUREBUFFERTYPE_SYSTEM) {
extra space in there, but maybe use a switch instead (since that gives you a
free compiler-provided compile-time assertion you're not missing cases).

[piman, 2011-04-26 23:06:57]

On Tue, Apr 26, 2011 at 4:01 PM, <fischman@chromium.org> wrote:

> See next-to-last comment for my thoughts on your message.
>
>
> http://codereview.chromium.org/6901036/diff/1/ppapi/c/dev/pp_video_dev.h
> File ppapi/c/dev/pp_video_dev.h (right):
>
>
>
http://codereview.chromium.org/6901036/diff/1/ppapi/c/dev/pp_video_dev.h#newc...
> ppapi/c/dev/pp_video_dev.h:298: uint32_t width;
> WDYT, worth using PP_Size?
>
>
>
>
http://codereview.chromium.org/6901036/diff/1/ppapi/c/dev/ppb_video_decoder_d...
> File ppapi/c/dev/ppb_video_decoder_dev.h (right):
>
>
>
http://codereview.chromium.org/6901036/diff/1/ppapi/c/dev/ppb_video_decoder_d...
> ppapi/c/dev/ppb_video_decoder_dev.h:12: #define
> PPB_VIDEODECODER_DEV_INTERFACE "PPB_VideoDecoder(Dev);0.5"
> On 2011/04/26 20:48:07, piman wrote:
>
>> Drive-by: please bump the version if you change the interface.
>>
>
> Is 0.10 greater or smaller than 0.9?  If smaller (as in math) then I
> propose the version bump here be 0.500001, since it's feeling like a lot
> of these interfaces are going to have a lot more CLs before they're
> ready for a 1.0 :)
>

Make it 0.6.

What matters is not if it's bigger or smaller, just that it changes whenever
the ABI changes. It's used by plugins so that they can check that the
interface that they used when they were compiled is the same (in the binary
sense, not the C/C++ sense) that the browser provides.
Any scheme that avoids collisions is fine. Keep it <1.0 until it's stable.
In all other places we use ..., 0.9, 0.10, 0.11, ... just fine.



>
>
>
http://codereview.chromium.org/6901036/diff/1/ppapi/tests/arch_dependent_size...
> File ppapi/tests/arch_dependent_sizes_64.h (right):
>
>
>
http://codereview.chromium.org/6901036/diff/1/ppapi/tests/arch_dependent_size...
> ppapi/tests/arch_dependent_sizes_64.h:20:
> PP_COMPILE_ASSERT_STRUCT_SIZE_IN_BYTES(PP_Picture_Dev, 20);
> I wonder what the value of these assertions is.
>
>
>
>
http://codereview.chromium.org/6901036/diff/1/webkit/plugins/ppapi/ppb_video_...
> File webkit/plugins/ppapi/ppb_video_decoder_impl.cc (right):
>
>
>
http://codereview.chromium.org/6901036/diff/1/webkit/plugins/ppapi/ppb_video_...
> webkit/plugins/ppapi/ppb_video_decoder_impl.cc:261: const
> PPP_VideoDecoder_Dev* ppp_videodecoder =
> worth doing this once in the ctor to avoid this 3-line stanza that
> repeats multiple times below?
>
>
>
http://codereview.chromium.org/6901036/diff/1/webkit/plugins/ppapi/ppb_video_...
> webkit/plugins/ppapi/ppb_video_decoder_impl.cc:265: struct
> PP_Picture_Dev* out_pic = CreatePictureDev(picture);
> I think this leaks all out_pics?
> To make the current API work, you need to keep your own map from
> picture_buffer_id to the out_pic that currently points to it, so in
> {Dismiss,Reuse}PictureBuffer(id) you can free/reuse the out_pic.
>
>
>
>
http://codereview.chromium.org/6901036/diff/1/webkit/plugins/ppapi/ppb_video_...
> webkit/plugins/ppapi/ppb_video_decoder_impl.cc:308: if
> (input.storage_type ==  PP_PICTUREBUFFERTYPE_SYSTEM) {
> extra space in there, but maybe use a switch instead (since that gives
> you a free compiler-provided compile-time assertion you're not missing
> cases).
>
>
> http://codereview.chromium.org/6901036/
>

[vrk (LEFT CHROMIUM), 2011-04-27 00:40:33]

Uploaded new patch with fixes to most of the code review comments and made a few
other minor style fixes.

I still haven't fixed the memory/ownership problems with regard to the type
conversions. I'll send mail or update the patch when I have more formulated
thoughts on the best fix for it!

I also added to this patch a fix for the inconsistency between media::GetConfigs
and ppapi::GetConfig. This involved getting rid of the PP_VideoDecoderConfig_Dev
struct in favor of PP_VideoConfigElement* vectors. 

My configs change suffers from the same memory problems as the other type
conversions, and I do some scary leap-of-faith casts from
PP_VideoAttributeDictionary to VideoAttributeKey and vice versa. This is bad and
we need to either compiler assert their equivalence or have
video_decode_accelerator.h include pp_video_dev.h. fischman is talking to darin
to see if we can do the latter.

http://codereview.chromium.org/6901036/diff/1/media/video/video_decode_accele...
File media/video/video_decode_accelerator.h (right):

http://codereview.chromium.org/6901036/diff/1/media/video/video_decode_accele...
media/video/video_decode_accelerator.h:333: virtual void PictureReady(Picture&
picture) = 0;
On 2011/04/26 22:36:34, scherkus wrote:
> why ref versus pointer?
I believe the only reason the parameter is a pointer is to avoid the copy of the
struct, so I changed it to a ref.

> if ref we usually go with a const-ref if possible

Yeah this can be a const-ref ptr. Changed!

http://codereview.chromium.org/6901036/diff/1/ppapi/c/dev/pp_video_dev.h
File ppapi/c/dev/pp_video_dev.h (right):

http://codereview.chromium.org/6901036/diff/1/ppapi/c/dev/pp_video_dev.h#newc...
ppapi/c/dev/pp_video_dev.h:298: uint32_t width;
On 2011/04/26 23:01:57, Ami Fischman wrote:
> WDYT, worth using PP_Size?

Good catch! Yes, I think so. I changed this and the sizes in PP_Picture_Dev.

http://codereview.chromium.org/6901036/diff/1/ppapi/c/dev/ppb_video_decoder_d...
File ppapi/c/dev/ppb_video_decoder_dev.h (right):

http://codereview.chromium.org/6901036/diff/1/ppapi/c/dev/ppb_video_decoder_d...
ppapi/c/dev/ppb_video_decoder_dev.h:12: #define PPB_VIDEODECODER_DEV_INTERFACE
"PPB_VideoDecoder(Dev);0.5"
On 2011/04/26 20:48:07, piman wrote:
> Drive-by: please bump the version if you change the interface.

Done.

http://codereview.chromium.org/6901036/diff/1/ppapi/c/dev/ppp_video_decoder_d...
File ppapi/c/dev/ppp_video_decoder_dev.h (right):

http://codereview.chromium.org/6901036/diff/1/ppapi/c/dev/ppp_video_decoder_d...
ppapi/c/dev/ppp_video_decoder_dev.h:10: #define PPP_VIDEODECODER_DEV_INTERFACE
"PPP_VideoDecoder(Dev);0.1"
On 2011/04/26 20:48:07, piman wrote:
> Drive-by: please bump the version if you change the interface.

Done.

http://codereview.chromium.org/6901036/diff/1/webkit/plugins/ppapi/ppb_video_...
File webkit/plugins/ppapi/ppb_video_decoder_impl.cc (right):

http://codereview.chromium.org/6901036/diff/1/webkit/plugins/ppapi/ppb_video_...
webkit/plugins/ppapi/ppb_video_decoder_impl.cc:18: #include
"webkit/plugins/ppapi/var.h"
On 2011/04/26 22:36:34, scherkus wrote:
> nit: include order

Done.

http://codereview.chromium.org/6901036/diff/1/webkit/plugins/ppapi/ppb_video_...
webkit/plugins/ppapi/ppb_video_decoder_impl.cc:261: const PPP_VideoDecoder_Dev*
ppp_videodecoder =
On 2011/04/26 23:01:57, Ami Fischman wrote:
> worth doing this once in the ctor to avoid this 3-line stanza that repeats
> multiple times below?

Done.

http://codereview.chromium.org/6901036/diff/1/webkit/plugins/ppapi/ppb_video_...
webkit/plugins/ppapi/ppb_video_decoder_impl.cc:265: struct PP_Picture_Dev*
out_pic = CreatePictureDev(picture);
On 2011/04/26 23:01:57, Ami Fischman wrote:
> I think this leaks all out_pics?
> To make the current API work, you need to keep your own map from
> picture_buffer_id to the out_pic that currently points to it, so in
> {Dismiss,Reuse}PictureBuffer(id) you can free/reuse the out_pic.

Yeah, that sounds right. Hmm... not quite sure how we should fix this yet (API
change or using a map to manage the memory ourselves).

http://codereview.chromium.org/6901036/diff/1/webkit/plugins/ppapi/ppb_video_...
webkit/plugins/ppapi/ppb_video_decoder_impl.cc:308: if (input.storage_type == 
PP_PICTUREBUFFERTYPE_SYSTEM) {
On 2011/04/26 23:01:57, Ami Fischman wrote:
> extra space in there, but maybe use a switch instead (since that gives you a
> free compiler-provided compile-time assertion you're not missing cases).

Done.

http://codereview.chromium.org/6901036/diff/1/webkit/plugins/ppapi/ppb_video_...
webkit/plugins/ppapi/ppb_video_decoder_impl.cc:321: NOTREACHED();
On 2011/04/26 22:36:34, scherkus wrote:
> warning: on release builds this will be optimized out (it's a DCHECK(false))
> 
> in which case data_plane might be uninitialized and get pushed onto
> data_plan_handles etc etc
> 
> perhaps return NULL?
> 
> can we signal an error?

For now, returning NULL and not adding it to the picture buffers, though I think
the current API says the I should call DismissPictureBuffer on the malformed
buffer. Not sure if that's the "proper" behavior. I'll fix this after I fix the
API+memory issues!

http://codereview.chromium.org/6901036/diff/1/webkit/plugins/ppapi/ppb_video_...
webkit/plugins/ppapi/ppb_video_decoder_impl.cc:344: malloc(sizeof(struct
PP_Picture_Dev));
On 2011/04/26 22:36:34, scherkus wrote:
> not a pepper master but I'm not seeing any other ppb *.cc files use
malloc+cast
> but instead use new
> 
> I guess since this is C++ new should be OK as long as it's still C++ that
still
> de-allocates the pictures

Yeah I wasn't sure on this case. Again, since I'm not sure if I want to be
allocating memory here in the first place, I'll fix this once I figure out the
API+mem stuff should work.

http://codereview.chromium.org/6901036/diff/1/webkit/plugins/ppapi/ppb_video_...
webkit/plugins/ppapi/ppb_video_decoder_impl.cc:373: // TODO: Does this really
need to be a ptr?
On 2011/04/26 22:36:34, scherkus wrote:
> todo for who?

Whoops, this whole block of added code doesn't make sense. Meant to delete this
before uploading patch. D'oh. Deleted!

http://codereview.chromium.org/6901036/diff/1/webkit/plugins/ppapi/ppb_video_...
File webkit/plugins/ppapi/ppb_video_decoder_impl.h (right):

http://codereview.chromium.org/6901036/diff/1/webkit/plugins/ppapi/ppb_video_...
webkit/plugins/ppapi/ppb_video_decoder_impl.h:62:
media::VideoDecodeAccelerator::Picture& picture) OVERRIDE;
On 2011/04/26 22:36:34, scherkus wrote:
> why ref versus pointer?
> 
> if ref we usually go with a const-ref if possible

Const-ref'ed!

http://codereview.chromium.org/6901036/diff/1/webkit/plugins/ppapi/ppb_video_...
webkit/plugins/ppapi/ppb_video_decoder_impl.h:69:
CreateMediaPictureBuffer(struct PP_PictureBuffer_Dev& input);
On 2011/04/26 22:36:34, scherkus wrote:
> nit: c++ doesn't need struct
> 
> also ref vs pointer vs const-ref

Done.

http://codereview.chromium.org/6901036/diff/1/webkit/plugins/ppapi/ppb_video_...
webkit/plugins/ppapi/ppb_video_decoder_impl.h:72: CreateMediaPicture(struct
PP_Picture_Dev& input);
On 2011/04/26 22:36:34, scherkus wrote:
> nit: c++ doesn't need struct
> 
> also ref vs pointer vs const-ref

Done.

http://codereview.chromium.org/6901036/diff/1/webkit/plugins/ppapi/ppb_video_...
webkit/plugins/ppapi/ppb_video_decoder_impl.h:74: static struct PP_Picture_Dev*
CreatePictureDev(
On 2011/04/26 22:36:34, scherkus wrote:
> nit: c++ doesn't need struct
> 
> also ref vs pointer vs const-ref

Done.

[vrk (LEFT CHROMIUM), 2011-04-28 06:20:39]

Please take another look!

I have made some fairly radical changes, most of which I've run by at least
fischman in person, but please don't hesitate to pipe up if you think this patch
is headed in the wrong direction. 

Overview of changes:

1. No pointers in PPP_VideoDecode_Dev parameters
The pointers in PPP_VideoDecode_Dev made me nervous: if the browser malloc'd
these structures handed to the plugin, then the plugin would have to free them.
If the browser stack allocated, then you'd have to trust the plugin to not save
the pointer but instead copy data. I've changed the parameters to all make
copies of the struct instead. (This is probably obvious but to clarify: the
*buffers* aren't being copied, just the small containers that held pointers to
the buffers.)

2. Don't heap allocate conversions between PP_* C structs and media::Picture*
classes
I stack allocate and pass by const ref when possible, e.g. see
PPB_VideoDecoder_Impl::Decode

3. Picture and PictureBuffer are now non-virtual, single-implementation, with
copy constructors
As part of (2), I changed VideoDecodeAccelerator::AssignPictureBuffer to take a
const vector<PictureBuffer>& instead of a vector<PictureBuffer*>. PictureBuffer
had to change accordingly. I could have also changed this to
vector<scoped_ptr<PictureBuffer> > if the copy constructors are too evil. I
really don't think it needs to be virtual, though.

4. Decode output color format specified by enum at GetConfigs/Initialize phase.
*This is a bit different from what I talked about with scherkus and fischman*
My proposal is to have the plugin query the decoder via GetConfigs for what
color formats the decoder can output. Then the plugin will choose the decoded
output color format in the call to Initialize, and from there, all buffers
passed to and from browser/plugin should be in that color format. I can explain
why I wanted to make this change if it's not clear; let me know.

[Ami GONE FROM CHROMIUM, 2011-04-29 18:40:34]

Looking pretty good.

http://codereview.chromium.org/6901036/diff/1/media/video/video_decode_accele...
File media/video/video_decode_accelerator.h (right):

http://codereview.chromium.org/6901036/diff/1/media/video/video_decode_accele...
media/video/video_decode_accelerator.h:250: // Interface expected from
PictureBuffers where pictures are stored.
Yay!

http://codereview.chromium.org/6901036/diff/1/ppapi/tests/arch_dependent_size...
File ppapi/tests/arch_dependent_sizes_64.h (right):

http://codereview.chromium.org/6901036/diff/1/ppapi/tests/arch_dependent_size...
ppapi/tests/arch_dependent_sizes_64.h:20:
PP_COMPILE_ASSERT_STRUCT_SIZE_IN_BYTES(PP_Picture_Dev, 20);
On 2011/04/26 23:01:57, Ami Fischman wrote:
> I wonder what the value of these assertions is.

I still wonder :)

http://codereview.chromium.org/6901036/diff/5064/media/video/picture.cc
File media/video/picture.cc (right):

http://codereview.chromium.org/6901036/diff/5064/media/video/picture.cc#newco...
media/video/picture.cc:11: PictureBuffer::Data::Data() {
Hopefully this can go away.

http://codereview.chromium.org/6901036/diff/5064/media/video/picture.cc#newco...
media/video/picture.cc:20: data_(Data()),
FWIW you can omit the explicit ctor name:
dimensions_(), data_(),
ditto for Picture

http://codereview.chromium.org/6901036/diff/5064/media/video/picture.cc#newco...
media/video/picture.cc:28: DCHECK(!initialized_);
Wait, what?  If you don't want to allow *re*-initialization, why can't these be
ctor args?
Ditto for Picture.

http://codereview.chromium.org/6901036/diff/5064/media/video/picture.h
File media/video/picture.h (right):

http://codereview.chromium.org/6901036/diff/5064/media/video/picture.h#newcode13
media/video/picture.h:13: // Interface expected from PictureBuffers where
pictures are stored.
Include a comment pointer to the equivalent PP structs here and below?

http://codereview.chromium.org/6901036/diff/5064/media/video/picture.h#newcode17
media/video/picture.h:17: PICTUREBUFFER_MEMORYTYPE_NONE = 0,
I wonder about the utility of NONE.  What does that mean?  Send decoded frames
to /dev/null?

http://codereview.chromium.org/6901036/diff/5064/media/video/picture.h#newcode28
media/video/picture.h:28: Data();
Unions can have constructors???  OMG.
Per offline convo this is probably unnecessary.

http://codereview.chromium.org/6901036/diff/5064/media/video/picture.h#newcode31
media/video/picture.h:31: PictureBuffer();
ctor goes at the top

http://codereview.chromium.org/6901036/diff/5064/media/video/picture.h#newcode35
media/video/picture.h:35: int32 GetId() const;
const getters can be lower_case()'d (ditto for Picture below).

http://codereview.chromium.org/6901036/diff/5064/media/video/picture.h#newcode50
media/video/picture.h:50: // Picture size related functions.
ISTM the same commentary belongs also in the PP structs, and this one needs to
point to that one anyway, so just move the comment block?

http://codereview.chromium.org/6901036/diff/5064/media/video/picture.h#newcode55
media/video/picture.h:55: //   - Picture buffer size.
You explain visible & decoded sizes, but what about picture buffer size?  Why
would it be different to the others?

http://codereview.chromium.org/6901036/diff/5064/media/video/picture.h#newcode68
media/video/picture.h:68: // and H.264 codec. Since the width (854 pixels) is
not multiple of the
s/not/not a/

http://codereview.chromium.org/6901036/diff/5064/media/video/picture.h#newcode70
media/video/picture.h:70: // contain garbage data which is notintended for
display.
s/noti/not i/

http://codereview.chromium.org/6901036/diff/5064/media/video/picture.h#newcode81
media/video/picture.h:81: int32 GetId() const;
picture_buffer_id() ?

http://codereview.chromium.org/6901036/diff/5064/media/video/picture.h#newcode98
media/video/picture.h:98: bool initialized_;
DISALLOW_COPY_AND_ASSIGN for this and PictureBuffer?

http://codereview.chromium.org/6901036/diff/5064/media/video/video_decode_acc...
File media/video/video_decode_accelerator.h (right):

http://codereview.chromium.org/6901036/diff/5064/media/video/video_decode_acc...
media/video/video_decode_accelerator.h:206: virtual const std::vector<uint32>
GetConfigs(
If you're not returning a reference why make the ret value const?
Better yet, why not make the function void and take a std::vector<uint32>*
configs
?

http://codereview.chromium.org/6901036/diff/5064/ppapi/c/dev/pp_video_dev.h
File ppapi/c/dev/pp_video_dev.h (right):

http://codereview.chromium.org/6901036/diff/5064/ppapi/c/dev/pp_video_dev.h#n...
ppapi/c/dev/pp_video_dev.h:74: PP_VIDEOATTR_COLORFORMAT_RGBA
Comment this is 32-bit, 8bit per each of RGBA?

http://codereview.chromium.org/6901036/diff/5064/ppapi/c/dev/pp_video_dev.h#n...
ppapi/c/dev/pp_video_dev.h:107: PP_H264PROFILE_HIGH444PREDICTIVEPROFILE= 1 << 7,
??

http://codereview.chromium.org/6901036/diff/5064/ppapi/c/dev/pp_video_dev.h#n...
ppapi/c/dev/pp_video_dev.h:199: enum PP_PictureBufferType_Dev storage_type;
IMO this goes better right before or after the union that it describes.

http://codereview.chromium.org/6901036/diff/5064/ppapi/c/dev/pp_video_dev.h#n...
ppapi/c/dev/pp_video_dev.h:213: struct PP_Size visible_size;
Should this be a PP_Rect to describe the offset/position of the visible rect
within the decoded rect?

http://codereview.chromium.org/6901036/diff/5064/ppapi/c/dev/ppb_video_decode...
File ppapi/c/dev/ppb_video_decoder_dev.h (right):

http://codereview.chromium.org/6901036/diff/5064/ppapi/c/dev/ppb_video_decode...
ppapi/c/dev/ppb_video_decoder_dev.h:13: #define PPB_VIDEODECODER_DEV_INTERFACE
PPB_VIDEODECODER_DEV_INTERFACE_0_6
??

http://codereview.chromium.org/6901036/diff/5064/ppapi/c/dev/ppp_video_decode...
File ppapi/c/dev/ppp_video_decoder_dev.h (right):

http://codereview.chromium.org/6901036/diff/5064/ppapi/c/dev/ppp_video_decode...
ppapi/c/dev/ppp_video_decoder_dev.h:31: struct PP_Size dimensions,
s/dimensions/size/ ?
(singular to emph that this isn't specifying a size per buffer)

http://codereview.chromium.org/6901036/diff/5064/ppapi/c/dev/ppp_video_decode...
ppapi/c/dev/ppp_video_decoder_dev.h:51: struct PP_Picture_Dev picture);
const&?

http://codereview.chromium.org/6901036/diff/5064/webkit/plugins/ppapi/ppb_vid...
File webkit/plugins/ppapi/ppb_video_decoder_impl.h (right):

http://codereview.chromium.org/6901036/diff/5064/webkit/plugins/ppapi/ppb_vid...
webkit/plugins/ppapi/ppb_video_decoder_impl.h:68: media::PictureBuffer& output);
per offline convo, you should be able to drop all these Copy methods (yay!)

[piman, 2011-04-29 18:56:17]

http://codereview.chromium.org/6901036/diff/1/ppapi/tests/arch_dependent_size...
File ppapi/tests/arch_dependent_sizes_64.h (right):

http://codereview.chromium.org/6901036/diff/1/ppapi/tests/arch_dependent_size...
ppapi/tests/arch_dependent_sizes_64.h:20:
PP_COMPILE_ASSERT_STRUCT_SIZE_IN_BYTES(PP_Picture_Dev, 20);
On 2011/04/29 18:40:34, Ami Fischman wrote:
> On 2011/04/26 23:01:57, Ami Fischman wrote:
> > I wonder what the value of these assertions is.
> 
> I still wonder :)

To protect against regressions, and for NaCl which is using a different compiler
on the target and host.

[Ami GONE FROM CHROMIUM, 2011-04-29 20:11:20]

>
> To protect against regressions,
>

What's a regression in this context?
Or do you mean to protect against people changing the struct size and
forgetting to change the version number?
Maybe the version number should just be a hash of the struct sizes :)

 and for NaCl which is using a different
> compiler on the target and host.


I'm not sure what you're getting at here, but if you mean you want to ensure
equal memory layout by different compilers, ISTM you need to do a deeper
assertion than merely size.  As an example, in the presence of
padding/alignment, the placement of fields within memory must be verified,
no?

[scherkus (not reviewing), 2011-04-29 20:33:01]

few nits

http://codereview.chromium.org/6901036/diff/5064/content/renderer/pepper_plat...
File content/renderer/pepper_platform_video_decoder_impl.cc (right):

http://codereview.chromium.org/6901036/diff/5064/content/renderer/pepper_plat...
content/renderer/pepper_platform_video_decoder_impl.cc:11: #include
"media/base/buffers.h"
I don't think this header file is needed

http://codereview.chromium.org/6901036/diff/5064/content/renderer/pepper_plat...
content/renderer/pepper_platform_video_decoder_impl.cc:12: #include
"media/base/video_frame.h"
I don't think this header file is needed

http://codereview.chromium.org/6901036/diff/5064/content/renderer/pepper_plat...
File content/renderer/pepper_platform_video_decoder_impl.h (right):

http://codereview.chromium.org/6901036/diff/5064/content/renderer/pepper_plat...
content/renderer/pepper_platform_video_decoder_impl.h:11: #include
"media/base/data_buffer.h"
I don't think this header file is used

http://codereview.chromium.org/6901036/diff/5064/media/video/picture.cc
File media/video/picture.cc (right):

http://codereview.chromium.org/6901036/diff/5064/media/video/picture.cc#newco...
media/video/picture.cc:13: memset(this, 0, sizeof(Data));
pedantic nit: use variables for sizeof() calculations

sizeof(*this)

http://codereview.chromium.org/6901036/diff/5064/media/video/picture.h
File media/video/picture.h (right):

http://codereview.chromium.org/6901036/diff/5064/media/video/picture.h#newcode46
media/video/picture.h:46: };
DISALLOW etc?

[piman, 2011-04-29 20:34:28]

On Fri, Apr 29, 2011 at 1:11 PM, Ami Fischman <fischman@chromium.org> wrote:

> To protect against regressions,
>>
>
> What's a regression in this context?
> Or do you mean to protect against people changing the struct size and
> forgetting to change the version number?
>

Yes. We need to guarantee the ABI.


> Maybe the version number should just be a hash of the struct sizes :)
>

And function parameters and return types and "implicit" behavior. Sounds
good. Waiting for your CL :)


>
>  and for NaCl which is using a different
>> compiler on the target and host.
>
>
> I'm not sure what you're getting at here, but if you mean you want to
> ensure equal memory layout by different compilers, ISTM you need to do a
> deeper assertion than merely size.  As an example, in the presence of
> padding/alignment, the placement of fields within memory must be verified,
> no?
>

Yes in theory you need to be more complete. In practice this will catch most
regressions as long as we stick with portable C types.

Antoine

[vrk (LEFT CHROMIUM), 2011-05-03 18:19:58]

Uploaded another round!

In this patch I have also gotten rid of the Data Type union and have updated the
API accordingly.

http://codereview.chromium.org/6901036/diff/5064/content/renderer/pepper_plat...
File content/renderer/pepper_platform_video_decoder_impl.cc (right):

http://codereview.chromium.org/6901036/diff/5064/content/renderer/pepper_plat...
content/renderer/pepper_platform_video_decoder_impl.cc:11: #include
"media/base/buffers.h"
On 2011/04/29 20:33:01, scherkus wrote:
> I don't think this header file is needed

Done.

http://codereview.chromium.org/6901036/diff/5064/content/renderer/pepper_plat...
content/renderer/pepper_platform_video_decoder_impl.cc:12: #include
"media/base/video_frame.h"
On 2011/04/29 20:33:01, scherkus wrote:
> I don't think this header file is needed

Done.

http://codereview.chromium.org/6901036/diff/5064/content/renderer/pepper_plat...
File content/renderer/pepper_platform_video_decoder_impl.h (right):

http://codereview.chromium.org/6901036/diff/5064/content/renderer/pepper_plat...
content/renderer/pepper_platform_video_decoder_impl.h:11: #include
"media/base/data_buffer.h"
On 2011/04/29 20:33:01, scherkus wrote:
> I don't think this header file is used

Correct! Removed.

http://codereview.chromium.org/6901036/diff/5064/media/video/picture.cc
File media/video/picture.cc (right):

http://codereview.chromium.org/6901036/diff/5064/media/video/picture.cc#newco...
media/video/picture.cc:11: PictureBuffer::Data::Data() {
On 2011/04/29 18:40:34, Ami Fischman wrote:
> Hopefully this can go away.

Yup! Gone.

http://codereview.chromium.org/6901036/diff/5064/media/video/picture.cc#newco...
media/video/picture.cc:13: memset(this, 0, sizeof(Data));
On 2011/04/29 20:33:01, scherkus wrote:
> pedantic nit: use variables for sizeof() calculations
> 
> sizeof(*this)

n/a now!

http://codereview.chromium.org/6901036/diff/5064/media/video/picture.cc#newco...
media/video/picture.cc:20: data_(Data()),
On 2011/04/29 18:40:34, Ami Fischman wrote:
> FWIW you can omit the explicit ctor name:
> dimensions_(), data_(),
> ditto for Picture

Totally forgot about that, thanks Ami!! N/A anymore here but fixed in a few
other places!

http://codereview.chromium.org/6901036/diff/5064/media/video/picture.cc#newco...
media/video/picture.cc:28: DCHECK(!initialized_);
On 2011/04/29 18:40:34, Ami Fischman wrote:
> Wait, what?  If you don't want to allow *re*-initialization, why can't these
be
> ctor args?
> Ditto for Picture.

Explained offline. Fixed now!

http://codereview.chromium.org/6901036/diff/5064/media/video/picture.h
File media/video/picture.h (right):

http://codereview.chromium.org/6901036/diff/5064/media/video/picture.h#newcode13
media/video/picture.h:13: // Interface expected from PictureBuffers where
pictures are stored.
On 2011/04/29 18:40:34, Ami Fischman wrote:
> Include a comment pointer to the equivalent PP structs here and below?

Done.

http://codereview.chromium.org/6901036/diff/5064/media/video/picture.h#newcode17
media/video/picture.h:17: PICTUREBUFFER_MEMORYTYPE_NONE = 0,
On 2011/04/29 18:40:34, Ami Fischman wrote:
> I wonder about the utility of NONE.  What does that mean?  Send decoded frames
> to /dev/null?  

I'm not sure either :/ Looks like most of the existing enums have this as well
(see video_decode_accelerator.h). My guess is it's to allow variables holding
enum values to have an uninitialized state? I can remove this if you want.

http://codereview.chromium.org/6901036/diff/5064/media/video/picture.h#newcode28
media/video/picture.h:28: Data();
On 2011/04/29 18:40:34, Ami Fischman wrote:
> Unions can have constructors???  OMG.
> Per offline convo this is probably unnecessary.

Done.

http://codereview.chromium.org/6901036/diff/5064/media/video/picture.h#newcode31
media/video/picture.h:31: PictureBuffer();
On 2011/04/29 18:40:34, Ami Fischman wrote:
> ctor goes at the top

Done.

http://codereview.chromium.org/6901036/diff/5064/media/video/picture.h#newcode35
media/video/picture.h:35: int32 GetId() const;
On 2011/04/29 18:40:34, Ami Fischman wrote:
> const getters can be lower_case()'d (ditto for Picture below).

Done.

http://codereview.chromium.org/6901036/diff/5064/media/video/picture.h#newcode46
media/video/picture.h:46: };
On 2011/04/29 20:33:01, scherkus wrote:
> DISALLOW etc?

Since I'm using std::vectors that contain instances of these classes (not
pointers to them), got to allow copy constructors... Let me know if you want me
to change the containers to hold smart pointers instead.

http://codereview.chromium.org/6901036/diff/5064/media/video/picture.h#newcode50
media/video/picture.h:50: // Picture size related functions.
On 2011/04/29 18:40:34, Ami Fischman wrote:
> ISTM the same commentary belongs also in the PP structs, and this one needs to
> point to that one anyway, so just move the comment block?

Done.

http://codereview.chromium.org/6901036/diff/5064/media/video/picture.h#newcode55
media/video/picture.h:55: //   - Picture buffer size.
On 2011/04/29 18:40:34, Ami Fischman wrote:
> You explain visible & decoded sizes, but what about picture buffer size?  Why
> would it be different to the others?

Ah whoops, didn't read this very carefully when I moved it. Reworked and moved
to pp_video_dev, let me know what you think!

http://codereview.chromium.org/6901036/diff/5064/media/video/picture.h#newcode68
media/video/picture.h:68: // and H.264 codec. Since the width (854 pixels) is
not multiple of the
On 2011/04/29 18:40:34, Ami Fischman wrote:
> s/not/not a/

n/a now

http://codereview.chromium.org/6901036/diff/5064/media/video/picture.h#newcode70
media/video/picture.h:70: // contain garbage data which is notintended for
display.
On 2011/04/29 18:40:34, Ami Fischman wrote:
> s/noti/not i/

n/a now

http://codereview.chromium.org/6901036/diff/5064/media/video/picture.h#newcode81
media/video/picture.h:81: int32 GetId() const;
On 2011/04/29 18:40:34, Ami Fischman wrote:
> picture_buffer_id() ?

Done.

http://codereview.chromium.org/6901036/diff/5064/media/video/picture.h#newcode98
media/video/picture.h:98: bool initialized_;
On 2011/04/29 18:40:34, Ami Fischman wrote:
> DISALLOW_COPY_AND_ASSIGN for this and PictureBuffer?

As I told Andrew above: since I'm using std::vectors that contain instances of
these classes (not pointers to them), got to allow copy constructors... Let me
know if you want me to change the containers to hold smart pointers instead.

http://codereview.chromium.org/6901036/diff/5064/media/video/video_decode_acc...
File media/video/video_decode_accelerator.h (right):

http://codereview.chromium.org/6901036/diff/5064/media/video/video_decode_acc...
media/video/video_decode_accelerator.h:206: virtual const std::vector<uint32>
GetConfigs(
On 2011/04/29 18:40:34, Ami Fischman wrote:
> If you're not returning a reference why make the ret value const?
> Better yet, why not make the function void and take a std::vector<uint32>*
> configs
> ?

Done.

http://codereview.chromium.org/6901036/diff/5064/ppapi/c/dev/pp_video_dev.h
File ppapi/c/dev/pp_video_dev.h (right):

http://codereview.chromium.org/6901036/diff/5064/ppapi/c/dev/pp_video_dev.h#n...
ppapi/c/dev/pp_video_dev.h:74: PP_VIDEOATTR_COLORFORMAT_RGBA
On 2011/04/29 18:40:34, Ami Fischman wrote:
> Comment this is 32-bit, 8bit per each of RGBA?

Done.

http://codereview.chromium.org/6901036/diff/5064/ppapi/c/dev/pp_video_dev.h#n...
ppapi/c/dev/pp_video_dev.h:107: PP_H264PROFILE_HIGH444PREDICTIVEPROFILE= 1 << 7,
On 2011/04/29 18:40:34, Ami Fischman wrote:
> ??

My bad! Reverted.

http://codereview.chromium.org/6901036/diff/5064/ppapi/c/dev/pp_video_dev.h#n...
ppapi/c/dev/pp_video_dev.h:199: enum PP_PictureBufferType_Dev storage_type;
On 2011/04/29 18:40:34, Ami Fischman wrote:
> IMO this goes better right before or after the union that it describes.

n/a now

http://codereview.chromium.org/6901036/diff/5064/ppapi/c/dev/pp_video_dev.h#n...
ppapi/c/dev/pp_video_dev.h:213: struct PP_Size visible_size;
On 2011/04/29 18:40:34, Ami Fischman wrote:
> Should this be a PP_Rect to describe the offset/position of the visible rect
> within the decoded rect?

I think the decoder will always decode flush to the top-right. I have put a
comment saying so at the top of the struct.

http://codereview.chromium.org/6901036/diff/5064/ppapi/c/dev/ppb_video_decode...
File ppapi/c/dev/ppb_video_decoder_dev.h (right):

http://codereview.chromium.org/6901036/diff/5064/ppapi/c/dev/ppb_video_decode...
ppapi/c/dev/ppb_video_decoder_dev.h:13: #define PPB_VIDEODECODER_DEV_INTERFACE
PPB_VIDEODECODER_DEV_INTERFACE_0_6
On 2011/04/29 18:40:34, Ami Fischman wrote:
> ??

Talked offline.

http://codereview.chromium.org/6901036/diff/5064/ppapi/c/dev/ppp_video_decode...
File ppapi/c/dev/ppp_video_decoder_dev.h (right):

http://codereview.chromium.org/6901036/diff/5064/ppapi/c/dev/ppp_video_decode...
ppapi/c/dev/ppp_video_decoder_dev.h:31: struct PP_Size dimensions,
On 2011/04/29 18:40:34, Ami Fischman wrote:
> s/dimensions/size/ ?
> (singular to emph that this isn't specifying a size per buffer)

Done.

http://codereview.chromium.org/6901036/diff/5064/ppapi/c/dev/ppp_video_decode...
ppapi/c/dev/ppp_video_decoder_dev.h:51: struct PP_Picture_Dev picture);
On 2011/04/29 18:40:34, Ami Fischman wrote:
> const&?

C API, can't use &.

http://codereview.chromium.org/6901036/diff/5064/webkit/plugins/ppapi/ppb_vid...
File webkit/plugins/ppapi/ppb_video_decoder_impl.h (right):

http://codereview.chromium.org/6901036/diff/5064/webkit/plugins/ppapi/ppb_vid...
webkit/plugins/ppapi/ppb_video_decoder_impl.h:68: media::PictureBuffer& output);
On 2011/04/29 18:40:34, Ami Fischman wrote:
> per offline convo, you should be able to drop all these Copy methods (yay!)

Ah, I actually kept the last two because they are not initializing objects. Let
me know if this is offensive; I could also inline these methods to the places
where they are called.

[Ami GONE FROM CHROMIUM, 2011-05-03 23:34:46]

nit nit nit
Pretty much LGTM.

http://codereview.chromium.org/6901036/diff/10004/media/video/picture.cc
File media/video/picture.cc (right):

http://codereview.chromium.org/6901036/diff/10004/media/video/picture.cc#newc...
media/video/picture.cc:8: 
Add a comment pointing to the far-away implementations?

http://codereview.chromium.org/6901036/diff/10004/media/video/picture.h
File media/video/picture.h (right):

http://codereview.chromium.org/6901036/diff/10004/media/video/picture.h#newco...
media/video/picture.h:88: BufferInfo info_;
I wonder if we're really going to end up wishing we had a 
uint32 data_length_ here instead.  Leave for now until we have reason to change.

http://codereview.chromium.org/6901036/diff/10004/media/video/picture.h#newco...
media/video/picture.h:113: // picture was decoded.
What if a picture spans more than one (or more than two) buffers?

http://codereview.chromium.org/6901036/diff/10004/media/video/video_decode_ac...
File media/video/video_decode_accelerator.h (right):

http://codereview.chromium.org/6901036/diff/10004/media/video/video_decode_ac...
media/video/video_decode_accelerator.h:215: std::vector<uint32>&
matched_configs) = 0;
style guide doesn't like your pass-by-non-const-ref.
Pass by address/pointer instead.

http://codereview.chromium.org/6901036/diff/10004/media/video/video_decode_ac...
media/video/video_decode_accelerator.h:235: // Assigns texture-backed picture
buffer to the video decoder.
This comment needs to change from talking about a singular buffer to plural
buffers.

http://codereview.chromium.org/6901036/diff/10004/media/video/video_decode_ac...
media/video/video_decode_accelerator.h:244: // Assigns system memory-backed
picture buffer to the video decoder.
Ditto singular->plural.

http://codereview.chromium.org/6901036/diff/10004/ppapi/c/dev/ppb_video_decod...
File ppapi/c/dev/ppb_video_decoder_dev.h (right):

http://codereview.chromium.org/6901036/diff/10004/ppapi/c/dev/ppb_video_decod...
ppapi/c/dev/ppb_video_decoder_dev.h:134: // to ProvidePictureBuffers.
...in which case they should just be callback parameters to
ProvidePictureBuffers instead of being part of the PPB API.

http://codereview.chromium.org/6901036/diff/10004/ppapi/c/dev/ppb_video_decod...
ppapi/c/dev/ppb_video_decoder_dev.h:145: // Provides the decoder with system
memory-backed picture buffers for video
I think this comment & the newline preceding it can be elided, and the comment
on the previous function modified slightly to cover both.

http://codereview.chromium.org/6901036/diff/10004/webkit/plugins/ppapi/ppb_vi...
File webkit/plugins/ppapi/ppb_video_decoder_impl.cc (right):

http://codereview.chromium.org/6901036/diff/10004/webkit/plugins/ppapi/ppb_vi...
webkit/plugins/ppapi/ppb_video_decoder_impl.cc:342: bool
PPB_VideoDecoder_Impl::CopyToPictureDev(
IMO this & the next function are better as file-static than as class-static
methods (since they have nothing to do with PPB_VideoDecoder_Impl).

http://codereview.chromium.org/6901036/diff/10004/webkit/plugins/ppapi/ppb_vi...
webkit/plugins/ppapi/ppb_video_decoder_impl.cc:423: data_ =
static_cast<void*>(pepper_buffer->mapped_buffer());
Is the cast necessary?

http://codereview.chromium.org/6901036/diff/10004/webkit/plugins/ppapi/ppb_vi...
webkit/plugins/ppapi/ppb_video_decoder_impl.cc:429: visible_size_ =
You can avoid the newlines and use initializers instead
Picture::Picture(const PP_Picture_Dev& picture)
    : picture_buffer_id_(picture.picture_buffer_id),
      user_handle_(picture.bitstream_user_handle),
      visible_size_(picture.visible_size),
      decoded_size_(picture.decoded_size) {
}

[scherkus (not reviewing), 2011-05-04 00:03:28]

http://codereview.chromium.org/6901036/diff/10004/media/video/picture.cc
File media/video/picture.cc (right):

http://codereview.chromium.org/6901036/diff/10004/media/video/picture.cc#newc...
media/video/picture.cc:8: 
On 2011/05/03 23:34:47, Ami Fischman wrote:
> Add a comment pointing to the far-away implementations?

woah there are far away implementations!?

http://codereview.chromium.org/6901036/diff/10004/media/video/picture.cc#newc...
media/video/picture.cc:10: : id_(id),
should be indented by 2 more spaces

http://codereview.chromium.org/6901036/diff/10004/media/video/picture.cc#newc...
media/video/picture.cc:22: : data_(data),
should be indented by 2 more spaces

http://codereview.chromium.org/6901036/diff/10004/media/video/picture.h
File media/video/picture.h (right):

http://codereview.chromium.org/6901036/diff/10004/media/video/picture.h#newco...
media/video/picture.h:34: private:
nit: add blank line before public/private/protected sections

http://codereview.chromium.org/6901036/diff/10004/media/video/video_decode_ac...
File media/video/video_decode_accelerator.h (right):

http://codereview.chromium.org/6901036/diff/10004/media/video/video_decode_ac...
media/video/video_decode_accelerator.h:173: PICTUREBUFFER_MEMORYTYPE_NONE = 0,
should we remove NONE here as well?

http://codereview.chromium.org/6901036/diff/10004/webkit/plugins/ppapi/ppb_vi...
File webkit/plugins/ppapi/ppb_video_decoder_impl.cc (right):

http://codereview.chromium.org/6901036/diff/10004/webkit/plugins/ppapi/ppb_vi...
webkit/plugins/ppapi/ppb_video_decoder_impl.cc:78: struct PP_GLESBuffer_Dev*
buffers) {
did we resolve needing struct keyword or not?

c++ doesn't require it however you seem to be inconsistent in this file (unless
I'm mistaken)

http://codereview.chromium.org/6901036/diff/10004/webkit/plugins/ppapi/ppb_vi...
webkit/plugins/ppapi/ppb_video_decoder_impl.cc:89: struct PP_SysmemBuffer_Dev*
buffers) {
ditto

http://codereview.chromium.org/6901036/diff/10004/webkit/plugins/ppapi/ppb_vi...
webkit/plugins/ppapi/ppb_video_decoder_impl.cc:405: : id_(info.id) {
indent two more spaces

http://codereview.chromium.org/6901036/diff/10004/webkit/plugins/ppapi/ppb_vi...
webkit/plugins/ppapi/ppb_video_decoder_impl.cc:412: :
texture_id_(buffer.texture_id),
indent two more spaces

http://codereview.chromium.org/6901036/diff/10004/webkit/plugins/ppapi/ppb_vi...
webkit/plugins/ppapi/ppb_video_decoder_impl.cc:418: : info_(buffer.info) {
indent two more spaces

http://codereview.chromium.org/6901036/diff/10004/webkit/plugins/ppapi/ppb_vi...
webkit/plugins/ppapi/ppb_video_decoder_impl.cc:427: :
picture_buffer_id_(picture.picture_buffer_id),
indent two more spaces

[vrk (LEFT CHROMIUM), 2011-05-05 05:57:12]

Thanks again for reviewing! Addressed comments and made a few other minor
clean-ups.

http://codereview.chromium.org/6901036/diff/10004/media/video/picture.cc
File media/video/picture.cc (right):

http://codereview.chromium.org/6901036/diff/10004/media/video/picture.cc#newc...
media/video/picture.cc:8: 
On 2011/05/03 23:34:47, Ami Fischman wrote:
> Add a comment pointing to the far-away implementations?

Done.

http://codereview.chromium.org/6901036/diff/10004/media/video/picture.cc#newc...
media/video/picture.cc:8: 
On 2011/05/04 00:03:28, scherkus wrote:
> On 2011/05/03 23:34:47, Ami Fischman wrote:
> > Add a comment pointing to the far-away implementations?
> 
> woah there are far away implementations!?

The other constructors (e.g. BufferInfo(const PP_BufferInfo_Dev& info)) are in
webkit/plugins/ppapi/ppb_video_decoder_impl.cc; they're defined there instead of
here because of dependency reasons. Looks like ami's suggestion is a good one!
:)

http://codereview.chromium.org/6901036/diff/10004/media/video/picture.cc#newc...
media/video/picture.cc:10: : id_(id),
On 2011/05/04 00:03:28, scherkus wrote:
> should be indented by 2 more spaces

Done.

http://codereview.chromium.org/6901036/diff/10004/media/video/picture.cc#newc...
media/video/picture.cc:22: : data_(data),
On 2011/05/04 00:03:28, scherkus wrote:
> should be indented by 2 more spaces

Done.

http://codereview.chromium.org/6901036/diff/10004/media/video/picture.h
File media/video/picture.h (right):

http://codereview.chromium.org/6901036/diff/10004/media/video/picture.h#newco...
media/video/picture.h:34: private:
On 2011/05/04 00:03:28, scherkus wrote:
> nit: add blank line before public/private/protected sections

Done.

http://codereview.chromium.org/6901036/diff/10004/media/video/picture.h#newco...
media/video/picture.h:88: BufferInfo info_;
On 2011/05/03 23:34:47, Ami Fischman wrote:
> I wonder if we're really going to end up wishing we had a 
> uint32 data_length_ here instead.  Leave for now until we have reason to
change.

I think it's likely too... every time I've used height/width, it's been
info_.width() * info_.height()

http://codereview.chromium.org/6901036/diff/10004/media/video/picture.h#newco...
media/video/picture.h:113: // picture was decoded.
On 2011/05/03 23:34:47, Ami Fischman wrote:
> What if a picture spans more than one (or more than two) buffers?

Ooh, not prepared for that! Mind if I just leave a TODO?

http://codereview.chromium.org/6901036/diff/10004/media/video/video_decode_ac...
File media/video/video_decode_accelerator.h (right):

http://codereview.chromium.org/6901036/diff/10004/media/video/video_decode_ac...
media/video/video_decode_accelerator.h:173: PICTUREBUFFER_MEMORYTYPE_NONE = 0,
On 2011/05/04 00:03:28, scherkus wrote:
> should we remove NONE here as well?

I haven't removed any of the NONEs... do you want me to?

http://codereview.chromium.org/6901036/diff/10004/media/video/video_decode_ac...
media/video/video_decode_accelerator.h:215: std::vector<uint32>&
matched_configs) = 0;
On 2011/05/03 23:34:47, Ami Fischman wrote:
> style guide doesn't like your pass-by-non-const-ref.
> Pass by address/pointer instead.

Done.

http://codereview.chromium.org/6901036/diff/10004/media/video/video_decode_ac...
media/video/video_decode_accelerator.h:235: // Assigns texture-backed picture
buffer to the video decoder.
On 2011/05/03 23:34:47, Ami Fischman wrote:
> This comment needs to change from talking about a singular buffer to plural
> buffers.

Done.

http://codereview.chromium.org/6901036/diff/10004/media/video/video_decode_ac...
media/video/video_decode_accelerator.h:244: // Assigns system memory-backed
picture buffer to the video decoder.
On 2011/05/03 23:34:47, Ami Fischman wrote:
> Ditto singular->plural.

Done.

http://codereview.chromium.org/6901036/diff/10004/ppapi/c/dev/ppb_video_decod...
File ppapi/c/dev/ppb_video_decoder_dev.h (right):

http://codereview.chromium.org/6901036/diff/10004/ppapi/c/dev/ppb_video_decod...
ppapi/c/dev/ppb_video_decoder_dev.h:134: // to ProvidePictureBuffers.
On 2011/05/03 23:34:47, Ami Fischman wrote:
> ...in which case they should just be callback parameters to
> ProvidePictureBuffers instead of being part of the PPB API.

Good point. Added this almost verbatim to the comment! :)

http://codereview.chromium.org/6901036/diff/10004/ppapi/c/dev/ppb_video_decod...
ppapi/c/dev/ppb_video_decoder_dev.h:145: // Provides the decoder with system
memory-backed picture buffers for video
On 2011/05/03 23:34:47, Ami Fischman wrote:
> I think this comment & the newline preceding it can be elided, and the comment
> on the previous function modified slightly to cover both.

Done. I did a similar thing with the comments for the respective methods in
video_decode_accelerator.h.

http://codereview.chromium.org/6901036/diff/10004/webkit/plugins/ppapi/ppb_vi...
File webkit/plugins/ppapi/ppb_video_decoder_impl.cc (right):

http://codereview.chromium.org/6901036/diff/10004/webkit/plugins/ppapi/ppb_vi...
webkit/plugins/ppapi/ppb_video_decoder_impl.cc:78: struct PP_GLESBuffer_Dev*
buffers) {
On 2011/05/04 00:03:28, scherkus wrote:
> did we resolve needing struct keyword or not?
> 
> c++ doesn't require it however you seem to be inconsistent in this file
(unless
> I'm mistaken)

You're right, no struct keyword necessary! Removed.

Argh I keep doing that because I am copying the headers directly from
ppb_video_decoder_dev.h (where we do need the "struct" because it's part of the
C API).

http://codereview.chromium.org/6901036/diff/10004/webkit/plugins/ppapi/ppb_vi...
webkit/plugins/ppapi/ppb_video_decoder_impl.cc:89: struct PP_SysmemBuffer_Dev*
buffers) {
On 2011/05/04 00:03:28, scherkus wrote:
> ditto

Done.

http://codereview.chromium.org/6901036/diff/10004/webkit/plugins/ppapi/ppb_vi...
webkit/plugins/ppapi/ppb_video_decoder_impl.cc:342: bool
PPB_VideoDecoder_Impl::CopyToPictureDev(
On 2011/05/03 23:34:47, Ami Fischman wrote:
> IMO this & the next function are better as file-static than as class-static
> methods (since they have nothing to do with PPB_VideoDecoder_Impl).

Done... well kind of :) I added the functions to the unnamed namespace above. Is
that what you mean, or did you actually want static?

http://codereview.chromium.org/6901036/diff/10004/webkit/plugins/ppapi/ppb_vi...
webkit/plugins/ppapi/ppb_video_decoder_impl.cc:405: : id_(info.id) {
On 2011/05/04 00:03:28, scherkus wrote:
> indent two more spaces

Done.

http://codereview.chromium.org/6901036/diff/10004/webkit/plugins/ppapi/ppb_vi...
webkit/plugins/ppapi/ppb_video_decoder_impl.cc:412: :
texture_id_(buffer.texture_id),
On 2011/05/04 00:03:28, scherkus wrote:
> indent two more spaces

Done.

http://codereview.chromium.org/6901036/diff/10004/webkit/plugins/ppapi/ppb_vi...
webkit/plugins/ppapi/ppb_video_decoder_impl.cc:418: : info_(buffer.info) {
On 2011/05/04 00:03:28, scherkus wrote:
> indent two more spaces

Done.

http://codereview.chromium.org/6901036/diff/10004/webkit/plugins/ppapi/ppb_vi...
webkit/plugins/ppapi/ppb_video_decoder_impl.cc:423: data_ =
static_cast<void*>(pepper_buffer->mapped_buffer());
On 2011/05/03 23:34:47, Ami Fischman wrote:
> Is the cast necessary?

Nope! removed.

http://codereview.chromium.org/6901036/diff/10004/webkit/plugins/ppapi/ppb_vi...
webkit/plugins/ppapi/ppb_video_decoder_impl.cc:427: :
picture_buffer_id_(picture.picture_buffer_id),
On 2011/05/04 00:03:28, scherkus wrote:
> indent two more spaces

Done.

http://codereview.chromium.org/6901036/diff/10004/webkit/plugins/ppapi/ppb_vi...
webkit/plugins/ppapi/ppb_video_decoder_impl.cc:429: visible_size_ =
On 2011/05/03 23:34:47, Ami Fischman wrote:
> You can avoid the newlines and use initializers instead
> Picture::Picture(const PP_Picture_Dev& picture)
>     : picture_buffer_id_(picture.picture_buffer_id),
>       user_handle_(picture.bitstream_user_handle),
>       visible_size_(picture.visible_size),
>       decoded_size_(picture.decoded_size) {
> }

Changed to use initializers, but not quite in the way you suggested because I
don't want to add a constructor to gfx::Size for PP_Size.

[Ami GONE FROM CHROMIUM, 2011-05-05 16:40:53]

LGTM

http://codereview.chromium.org/6901036/diff/10004/webkit/plugins/ppapi/ppb_vi...
File webkit/plugins/ppapi/ppb_video_decoder_impl.cc (right):

http://codereview.chromium.org/6901036/diff/10004/webkit/plugins/ppapi/ppb_vi...
webkit/plugins/ppapi/ppb_video_decoder_impl.cc:342: bool
PPB_VideoDecoder_Impl::CopyToPictureDev(
On 2011/05/05 05:57:12, Victoria Kirst wrote:
> On 2011/05/03 23:34:47, Ami Fischman wrote:
> > IMO this & the next function are better as file-static than as class-static
> > methods (since they have nothing to do with PPB_VideoDecoder_Impl).
> 
> Done... well kind of :) I added the functions to the unnamed namespace above.
Is
> that what you mean, or did you actually want static?

In media/ code we decided to use static instead of anonymous namespace (see the
reviewlog for 6628020).  But this is webkit/plugins/ppapi, not media, so
whatever.
(as long as there's an anon namespace in this file, it's fine for your new
functions to live in it).

http://codereview.chromium.org/6901036/diff/18002/webkit/plugins/ppapi/ppb_vi...
webkit/plugins/ppapi/ppb_video_decoder_impl.cc:138: 
s/its/their/

[brettw, 2011-05-10 20:05:45]

Renderer LGTM