Support multiple HW video decoders per context.

Support multiple HW video decoders per context.

Enhanced gles2.cc to display two decoders in the same context to demo this
capability.

BUG=92598
TEST=ovdatest passes, gles2 works, trybots

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

[Ami GONE FROM CHROMIUM, 2011-08-15 23:50:55]

apatrick: please OWNERS-review and sanity check my changes to expose routing a
little more than it was before.  The up-side to this approach is that I don't
re-invent message routing outside the IPC system (i.e. add a decoder_id field to
all AcceleratedVideoDecoderHostMsg_* messages, keep the decoders on the same
route_id as the gpucommandbufferstub, and dispatch to the correct decoder based
on inspecting the unpacked message).  The (small, IMO) cost is that CBP and GCBS
now know they're held by a GpuChannelHost and GpuChannel, resp. instead of the
more abstract interfaces.

piman: FYI since you asked for this functionality
vrk: please review everything

[apatrick_chromium, 2011-08-16 00:08:55]

LGTM for GPU stuff.

[vrk (LEFT CHROMIUM), 2011-08-16 22:06:21]

As we discussed in person: Try making struct DecoderState -> class
DecoderClient, but if that's too annoying then abort mission!

http://codereview.chromium.org/7659001/diff/2001/content/renderer/gpu/gpu_vid...
File content/renderer/gpu/gpu_video_decode_accelerator_host.h (right):

http://codereview.chromium.org/7659001/diff/2001/content/renderer/gpu/gpu_vid...
content/renderer/gpu/gpu_video_decode_accelerator_host.h:23: // |router| is used
to dispatch IPC messages to this object.
nit: router is not a parameter; update comment

http://codereview.chromium.org/7659001/diff/2001/ppapi/examples/gles2/gles2.cc
File ppapi/examples/gles2/gles2.cc (right):

http://codereview.chromium.org/7659001/diff/2001/ppapi/examples/gles2/gles2.c...
ppapi/examples/gles2/gles2.cc:141: pp::Size position_size_;
nit: rename position_size_ to viewport_size_? (If that's accurate)

http://codereview.chromium.org/7659001/diff/2001/ppapi/examples/gles2/gles2.c...
ppapi/examples/gles2/gles2.cc:212:
it->second->DeleteOutstandingBitstreamBuffers();
Any reason not to push these DeleteOutstandingBitstreamBuffers() and
DeleteOutstandingTextures() into the DecoderState destructor?

http://codereview.chromium.org/7659001/diff/2001/ppapi/examples/gles2/gles2.c...
ppapi/examples/gles2/gles2.cc:240: for (int i = 0; i < 2; ++i) {
Instead of 2, make into an enum constant? (like kNumConcurrentDecodes)

http://codereview.chromium.org/7659001/diff/2001/ppapi/examples/gles2/gles2.c...
ppapi/examples/gles2/gles2.cc:243: assert(!state->decoder_->is_null());
If you're going the DecoderClient route, could probably have an accessor for
state->decoder_

http://codereview.chromium.org/7659001/diff/2001/ppapi/examples/gles2/gles2.c...
ppapi/examples/gles2/gles2.cc:331: for (uint32_t i = 0; i < req_num_of_bufs;
++i) {
Put this behavior in DecoderState object if not too annoying.

http://codereview.chromium.org/7659001/diff/2001/ppapi/examples/gles2/gles2.c...
ppapi/examples/gles2/gles2.cc:346: DecoderState::PictureBufferMap::iterator it =
Put this behavior in DecoderState object if not too annoying.

http://codereview.chromium.org/7659001/diff/2001/ppapi/examples/gles2/gles2.c...
ppapi/examples/gles2/gles2.cc:363: DecoderState::PictureBufferMap::iterator it =
If you're going the DecoderClient route, smush this into a
GetPictureBufferByID() method.

http://codereview.chromium.org/7659001/diff/2001/ppapi/examples/gles2/gles2.c...
ppapi/examples/gles2/gles2.cc:450: if (state && state->decoder_)
If you're going the DecoderClient route, here too you could just use an accesor
for state->decoder_

[Ami GONE FROM CHROMIUM, 2011-08-16 22:56:26]

http://codereview.chromium.org/7659001/diff/2001/content/renderer/gpu/gpu_vid...
File content/renderer/gpu/gpu_video_decode_accelerator_host.h (right):

http://codereview.chromium.org/7659001/diff/2001/content/renderer/gpu/gpu_vid...
content/renderer/gpu/gpu_video_decode_accelerator_host.h:23: // |router| is used
to dispatch IPC messages to this object.
On 2011/08/16 22:06:21, Victoria Kirst wrote:
> nit: router is not a parameter; update comment

Done.

http://codereview.chromium.org/7659001/diff/2001/ppapi/examples/gles2/gles2.cc
File ppapi/examples/gles2/gles2.cc (right):

http://codereview.chromium.org/7659001/diff/2001/ppapi/examples/gles2/gles2.c...
ppapi/examples/gles2/gles2.cc:141: pp::Size position_size_;
On 2011/08/16 22:06:21, Victoria Kirst wrote:
> nit: rename position_size_ to viewport_size_? (If that's accurate)

It's not.
The size is that of the position argument to DidChangeView(), so in that sense
is a "view".  But in another (more relevant :)) sense, viewport is a quarter of
this. 

Renamed to plugin_size_.

http://codereview.chromium.org/7659001/diff/2001/ppapi/examples/gles2/gles2.c...
ppapi/examples/gles2/gles2.cc:212:
it->second->DeleteOutstandingBitstreamBuffers();
On 2011/08/16 22:06:21, Victoria Kirst wrote:
> Any reason not to push these DeleteOutstandingBitstreamBuffers() and
> DeleteOutstandingTextures() into the DecoderState destructor?

Done (and in-lined).

http://codereview.chromium.org/7659001/diff/2001/ppapi/examples/gles2/gles2.c...
ppapi/examples/gles2/gles2.cc:240: for (int i = 0; i < 2; ++i) {
On 2011/08/16 22:06:21, Victoria Kirst wrote:
> Instead of 2, make into an enum constant? (like kNumConcurrentDecodes)

Done.
(I previously hadn't b/c the /2 was still hard-coded into the rendering code,
but I was being silly; is fine to use the constant there too)

http://codereview.chromium.org/7659001/diff/2001/ppapi/examples/gles2/gles2.c...
ppapi/examples/gles2/gles2.cc:243: assert(!state->decoder_->is_null());
On 2011/08/16 22:06:21, Victoria Kirst wrote:
> If you're going the DecoderClient route, could probably have an accessor for
> state->decoder_

Done.

http://codereview.chromium.org/7659001/diff/2001/ppapi/examples/gles2/gles2.c...
ppapi/examples/gles2/gles2.cc:331: for (uint32_t i = 0; i < req_num_of_bufs;
++i) {
On 2011/08/16 22:06:21, Victoria Kirst wrote:
> Put this behavior in DecoderState object if not too annoying.

Done.

http://codereview.chromium.org/7659001/diff/2001/ppapi/examples/gles2/gles2.c...
ppapi/examples/gles2/gles2.cc:346: DecoderState::PictureBufferMap::iterator it =
On 2011/08/16 22:06:21, Victoria Kirst wrote:
> Put this behavior in DecoderState object if not too annoying.

Done.

http://codereview.chromium.org/7659001/diff/2001/ppapi/examples/gles2/gles2.c...
ppapi/examples/gles2/gles2.cc:363: DecoderState::PictureBufferMap::iterator it =
On 2011/08/16 22:06:21, Victoria Kirst wrote:
> If you're going the DecoderClient route, smush this into a
> GetPictureBufferByID() method.

Done.

http://codereview.chromium.org/7659001/diff/2001/ppapi/examples/gles2/gles2.c...
ppapi/examples/gles2/gles2.cc:450: if (state && state->decoder_)
On 2011/08/16 22:06:21, Victoria Kirst wrote:
> If you're going the DecoderClient route, here too you could just use an
accesor
> for state->decoder_

Done.

[vrk (LEFT CHROMIUM), 2011-08-16 23:53:11]

LGTM with nits

http://codereview.chromium.org/7659001/diff/8001/ppapi/examples/gles2/gles2.cc
File ppapi/examples/gles2/gles2.cc (right):

http://codereview.chromium.org/7659001/diff/8001/ppapi/examples/gles2/gles2.c...
ppapi/examples/gles2/gles2.cc:78: class DecoderState {
nit: change class name + comment? Feels like there's too much behavior for this
to be "State"; I defer to you though!

http://codereview.chromium.org/7659001/diff/8001/ppapi/examples/gles2/gles2.c...
ppapi/examples/gles2/gles2.cc:85: void DecodeNextNALU();
I think this and the next 3 methods can be private.

http://codereview.chromium.org/7659001/diff/8001/ppapi/examples/gles2/gles2.c...
ppapi/examples/gles2/gles2.cc:92: PP_Size dimensions);
nit: indentation

http://codereview.chromium.org/7659001/diff/8001/ppapi/examples/gles2/gles2.c...
ppapi/examples/gles2/gles2.cc:352: int id) {
nit: indentation (put on line above)

[Ami GONE FROM CHROMIUM, 2011-08-17 00:01:58]

http://codereview.chromium.org/7659001/diff/8001/ppapi/examples/gles2/gles2.cc
File ppapi/examples/gles2/gles2.cc (right):

http://codereview.chromium.org/7659001/diff/8001/ppapi/examples/gles2/gles2.c...
ppapi/examples/gles2/gles2.cc:78: class DecoderState {
On 2011/08/16 23:53:11, Victoria Kirst wrote:
> nit: change class name + comment? Feels like there's too much behavior for
this
> to be "State"; I defer to you though!

I'm not crazy about it, but s/state/done/g

http://codereview.chromium.org/7659001/diff/8001/ppapi/examples/gles2/gles2.c...
ppapi/examples/gles2/gles2.cc:85: void DecodeNextNALU();
On 2011/08/16 23:53:11, Victoria Kirst wrote:
> I think this and the next 3 methods can be private.

Done.

http://codereview.chromium.org/7659001/diff/8001/ppapi/examples/gles2/gles2.c...
ppapi/examples/gles2/gles2.cc:92: PP_Size dimensions);
On 2011/08/16 23:53:11, Victoria Kirst wrote:
> nit: indentation

Done.