Updated video decoder IPC to use PPB_Buffer_Dev's new support for SharedMemory.

Updated video decoder IPC to use PPB_Buffer_Dev's new support for SharedMemory. 
Plumb through from renderer to GPU process' OMX decoder.

BUG=none
TEST=trybots


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

[Ami GONE FROM CHROMIUM, 2011-05-18 23:05:21]

This looks right to me but isn't actually tested past the "chrome still builds"
line.

[vrk (LEFT CHROMIUM), 2011-05-19 00:37:09]

LGTM if it works! :)

(That is, as we discussed in person: it would be good to hack together something
to verify that data's getting passed from plugin to gpu process like we hope!)

Thanks for figuring this out, fischman!

http://codereview.chromium.org/7034040/diff/1/content/gpu/omx_video_decode_ac...
File content/gpu/omx_video_decode_accelerator.cc (right):

http://codereview.chromium.org/7034040/diff/1/content/gpu/omx_video_decode_ac...
content/gpu/omx_video_decode_accelerator.cc:263: // setup |omx_buffer|.
nit: capitalize first letter.

http://codereview.chromium.org/7034040/diff/1/content/gpu/omx_video_decode_ac...
content/gpu/omx_video_decode_accelerator.cc:265: new
base::SharedMemory(bitstream_buffer.handle(), true));
Isn't true -> readonly? I think this should be false.

http://codereview.chromium.org/7034040/diff/1/content/gpu/omx_video_decode_ac...
File content/gpu/omx_video_decode_accelerator.h (right):

http://codereview.chromium.org/7034040/diff/1/content/gpu/omx_video_decode_ac...
content/gpu/omx_video_decode_accelerator.h:135: std::pair<base::SharedMemory*,
An FYI that I think you already know:

In the ToT media::VideoDecodeAccelerator, the Decode method does not take a
callback parameter. Instead, one is expected to call
"NotifyEndOfBitstreamBuffer" with the bitstream buffer ID. (The
PPB_VideoDecode_Dev interface _does_ accept a plugin-defined callback, so a
small-but-intentional discrepancy there.)

Anyway that means that in the future we'll probably change this map to include
bitstream buffer ID rather than callback, but let's do that in another CL!
(presumably one that updates Decode to match the ToT interface.)

http://codereview.chromium.org/7034040/diff/1/content/renderer/gpu_video_deco...
File content/renderer/gpu_video_decode_accelerator_host.cc (right):

http://codereview.chromium.org/7034040/diff/1/content/renderer/gpu_video_deco...
content/renderer/gpu_video_decode_accelerator_host.cc:93: DLOG(ERROR) <<
"Send(AcceleratedVideoDecoderMsg_Decode) failed";
Any particular reason to use DLOG instead of LOG or vice versa?

http://codereview.chromium.org/7034040/diff/1/media/base/bitstream_buffer.h
File media/base/bitstream_buffer.h (right):

http://codereview.chromium.org/7034040/diff/1/media/base/bitstream_buffer.h#n...
media/base/bitstream_buffer.h:32: // Allow compiler-generated copy & assign
constructors.
Why are we allowing this now? Couldn't find where in the patch you'd want this.

(Perhaps at one point you had OMXBufferCallbackMap map to
media::BitstreamBuffers, then changed it when you realized the Omx stuff isn't
up to date with ToT?)

[vrk (LEFT CHROMIUM), 2011-05-19 01:10:55]

http://codereview.chromium.org/7034040/diff/1/content/renderer/gpu_video_deco...
File content/renderer/gpu_video_decode_accelerator_host.cc (right):

http://codereview.chromium.org/7034040/diff/1/content/renderer/gpu_video_deco...
content/renderer/gpu_video_decode_accelerator_host.cc:93: DLOG(ERROR) <<
"Send(AcceleratedVideoDecoderMsg_Decode) failed";
On 2011/05/19 00:37:09, Victoria Kirst wrote:
> Any particular reason to use DLOG instead of LOG or vice versa?

Whoop just saw darin's protip in your other CL; carry on! :) We've got LOGs
everywhere so we should probably change that eventually!

[Ami GONE FROM CHROMIUM, 2011-05-19 19:33:37]

+brettw for OWNERS approval.

http://codereview.chromium.org/7034040/diff/1/content/gpu/omx_video_decode_ac...
File content/gpu/omx_video_decode_accelerator.cc (right):

http://codereview.chromium.org/7034040/diff/1/content/gpu/omx_video_decode_ac...
content/gpu/omx_video_decode_accelerator.cc:263: // setup |omx_buffer|.
On 2011/05/19 00:37:09, Victoria Kirst wrote:
> nit: capitalize first letter.

Done.

http://codereview.chromium.org/7034040/diff/1/content/gpu/omx_video_decode_ac...
content/gpu/omx_video_decode_accelerator.cc:265: new
base::SharedMemory(bitstream_buffer.handle(), true));
On 2011/05/19 00:37:09, Victoria Kirst wrote:
> Isn't true -> readonly? I think this should be false.

Yes, true -> readonly.  Why do you think the decoder needs to write to the
shared memory?

http://codereview.chromium.org/7034040/diff/1/content/gpu/omx_video_decode_ac...
File content/gpu/omx_video_decode_accelerator.h (right):

http://codereview.chromium.org/7034040/diff/1/content/gpu/omx_video_decode_ac...
content/gpu/omx_video_decode_accelerator.h:135: std::pair<base::SharedMemory*,
On 2011/05/19 00:37:09, Victoria Kirst wrote:
> An FYI that I think you already know:
> 
> In the ToT media::VideoDecodeAccelerator, the Decode method does not take a
> callback parameter. Instead, one is expected to call
> "NotifyEndOfBitstreamBuffer" with the bitstream buffer ID. (The
> PPB_VideoDecode_Dev interface _does_ accept a plugin-defined callback, so a
> small-but-intentional discrepancy there.)
> 
> Anyway that means that in the future we'll probably change this map to include
> bitstream buffer ID rather than callback, but let's do that in another CL!
> (presumably one that updates Decode to match the ToT interface.)

SGTM.

http://codereview.chromium.org/7034040/diff/1/content/renderer/gpu_video_deco...
File content/renderer/gpu_video_decode_accelerator_host.cc (right):

http://codereview.chromium.org/7034040/diff/1/content/renderer/gpu_video_deco...
content/renderer/gpu_video_decode_accelerator_host.cc:93: DLOG(ERROR) <<
"Send(AcceleratedVideoDecoderMsg_Decode) failed";
On 2011/05/19 01:10:55, Victoria Kirst wrote:
> On 2011/05/19 00:37:09, Victoria Kirst wrote:
> > Any particular reason to use DLOG instead of LOG or vice versa?
> 
> Whoop just saw darin's protip in your other CL; carry on! :) We've got LOGs
> everywhere so we should probably change that eventually!

Done.

http://codereview.chromium.org/7034040/diff/1/content/renderer/gpu_video_deco...
content/renderer/gpu_video_decode_accelerator_host.cc:93: DLOG(ERROR) <<
"Send(AcceleratedVideoDecoderMsg_Decode) failed";
On 2011/05/19 00:37:09, Victoria Kirst wrote:
> Any particular reason to use DLOG instead of LOG or vice versa?

Done.

http://codereview.chromium.org/7034040/diff/1/media/base/bitstream_buffer.h
File media/base/bitstream_buffer.h (right):

http://codereview.chromium.org/7034040/diff/1/media/base/bitstream_buffer.h#n...
media/base/bitstream_buffer.h:32: // Allow compiler-generated copy & assign
constructors.
On 2011/05/19 00:37:09, Victoria Kirst wrote:
> Why are we allowing this now? Couldn't find where in the patch you'd want
this.
> 
> (Perhaps at one point you had OMXBufferCallbackMap map to
> media::BitstreamBuffers, then changed it when you realized the Omx stuff isn't
> up to date with ToT?)

We use media::BitstreamBuffer as a dumb wrapper, so the code looks prettier if
you allow this, for example so that the temporary can be passed by const-ref in
content/common/gpu/gpu_video_decode_accelerator.cc:122

It's correct to use compiler-generated ctors b/c this type is basically POD.

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

LGTM!

http://codereview.chromium.org/7034040/diff/1/content/gpu/omx_video_decode_ac...
File content/gpu/omx_video_decode_accelerator.cc (right):

http://codereview.chromium.org/7034040/diff/1/content/gpu/omx_video_decode_ac...
content/gpu/omx_video_decode_accelerator.cc:265: new
base::SharedMemory(bitstream_buffer.handle(), true));
On 2011/05/19 19:33:37, Ami Fischman wrote:
> On 2011/05/19 00:37:09, Victoria Kirst wrote:
> > Isn't true -> readonly? I think this should be false.
> 
> Yes, true -> readonly.  Why do you think the decoder needs to write to the
> shared memory?

You're right; my mistake! I was mixing this up with the PictureBuffers.

http://codereview.chromium.org/7034040/diff/1/media/base/bitstream_buffer.h
File media/base/bitstream_buffer.h (right):

http://codereview.chromium.org/7034040/diff/1/media/base/bitstream_buffer.h#n...
media/base/bitstream_buffer.h:32: // Allow compiler-generated copy & assign
constructors.
On 2011/05/19 19:33:37, Ami Fischman wrote:
> On 2011/05/19 00:37:09, Victoria Kirst wrote:
> > Why are we allowing this now? Couldn't find where in the patch you'd want
> this.
> > 
> > (Perhaps at one point you had OMXBufferCallbackMap map to
> > media::BitstreamBuffers, then changed it when you realized the Omx stuff
isn't
> > up to date with ToT?)
> 
> We use media::BitstreamBuffer as a dumb wrapper, so the code looks prettier if
> you allow this, for example so that the temporary can be passed by const-ref
in
> content/common/gpu/gpu_video_decode_accelerator.cc:122
> 
> It's correct to use compiler-generated ctors b/c this type is basically POD.

OK sounds good to me! Thanks for explaining!

[Ami GONE FROM CHROMIUM, 2011-05-20 02:41:55]

+jam for OWNERS approval
-brettw since his calendar makes it look like he's OOO until next tuesday.

[jam, 2011-05-23 16:58:38]

content lgtm

sorry for the delay, I missed this cl.  in the future please IM me if I don't
respond in an hour or two.

[commit-bot: I haz the power, 2011-05-23 18:05:53]

Change committed as 86302