Add support for VP8 decode to OmxVideoDecodeAccelerator, for HW that supports it.

Add support for VP8 decode to OmxVideoDecodeAccelerator, for HW that supports it.

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

[Ami GONE FROM CHROMIUM, 2012-07-20 19:55:19]

brettw: please OWNERS-review the ppapi/ bits (esp. see my q about dropping the
enum value).  I regen'd the .h with: [./generator.py --cgen --wnone]; let me
know if that's not how we're supposed to do it nowadays.
scherkus: please super-star-committer review for CQ chromiumness.
posciak: please lowly-uncommitter review for OMX goodness.

Note: the new functionality doesn't actually work yet; it gets an
OMX_ErrorMFCInit fired b/c of MFC_DEC_INIT_FAIL inside secomx.  Otherwise than
that I think it's got all the pieces in place and wired up.
I suspect either a bug in secomx or secomx expects a different bitstream format.
 Questions are being asked of the secomx experts.
Since the CL doesn't break existing functionality I might land it as-is and do
any necessary fixups as a followup CL (assuming the problem isn't in secomx).

https://chromiumcodereview.appspot.com/10808058/diff/1/content/common/gpu/med...
File content/common/gpu/media/omx_video_decode_accelerator.cc (left):

https://chromiumcodereview.appspot.com/10808058/diff/1/content/common/gpu/med...
content/common/gpu/media/omx_video_decode_accelerator.cc:202:
component_name_is_nvidia_h264ext_ = !strcmp(
This is just some random cleanup.

https://chromiumcodereview.appspot.com/10808058/diff/1/media/base/video_decod...
File media/base/video_decoder_config.h (right):

https://chromiumcodereview.appspot.com/10808058/diff/1/media/base/video_decod...
media/base/video_decoder_config.h:45: H264PROFILE_MAIN = 1,
I got tired of manually counting these off so added explicit values.

https://chromiumcodereview.appspot.com/10808058/diff/1/ppapi/api/dev/pp_video...
File ppapi/api/dev/pp_video_dev.idl (left):

https://chromiumcodereview.appspot.com/10808058/diff/1/ppapi/api/dev/pp_video...
ppapi/api/dev/pp_video_dev.idl:34: PP_VIDEODECODER_PROFILE_MAX =
PP_VIDEODECODER_H264PROFILE_MULTIVIEWHIGH
There are no uses of PROFILE_MAX in the chromium codebase, and the fact that
this API still lives in dev/ made me think this is legit.  Please let me know if
that's wrong.

[scherkus (not reviewing), 2012-07-20 20:20:20]

slick!

lgtm w/ nits+q

http://codereview.chromium.org/10808058/diff/1/content/common/gpu/media/video...
File content/common/gpu/media/video_decode_accelerator_unittest.cc (right):

http://codereview.chromium.org/10808058/diff/1/content/common/gpu/media/video...
content/common/gpu/media/video_decode_accelerator_unittest.cc:537: if (1) {
what who!?

http://codereview.chromium.org/10808058/diff/1/media/ffmpeg/ffmpeg_common.cc
File media/ffmpeg/ffmpeg_common.cc (right):

http://codereview.chromium.org/10808058/diff/1/media/ffmpeg/ffmpeg_common.cc#...
media/ffmpeg/ffmpeg_common.cc:252:
ProfileIDToVideoCodecProfile(stream->codec->profile);
indent

http://codereview.chromium.org/10808058/diff/1/media/filters/gpu_video_decode...
File media/filters/gpu_video_decoder.cc (right):

http://codereview.chromium.org/10808058/diff/1/media/filters/gpu_video_decode...
media/filters/gpu_video_decoder.cc:155:
demuxer_stream_->EnableBitstreamConverter();
q: do you recall the reason for having the bitstream converter owned by the
demuxer vs owned by GpuVD?

I guess it cuts down on code duplication when multiple VDs require bitstream
conversion hm..

[Ami GONE FROM CHROMIUM, 2012-07-20 20:25:19]

Thanks for the fast review.

http://codereview.chromium.org/10808058/diff/1/content/common/gpu/media/video...
File content/common/gpu/media/video_decode_accelerator_unittest.cc (right):

http://codereview.chromium.org/10808058/diff/1/content/common/gpu/media/video...
content/common/gpu/media/video_decode_accelerator_unittest.cc:537: if (1) {
On 2012/07/20 20:20:20, scherkus wrote:
> what who!?

Just making sure you're paying attention!

http://codereview.chromium.org/10808058/diff/1/media/ffmpeg/ffmpeg_common.cc
File media/ffmpeg/ffmpeg_common.cc (right):

http://codereview.chromium.org/10808058/diff/1/media/ffmpeg/ffmpeg_common.cc#...
media/ffmpeg/ffmpeg_common.cc:252:
ProfileIDToVideoCodecProfile(stream->codec->profile);
On 2012/07/20 20:20:20, scherkus wrote:
> indent

Done.

http://codereview.chromium.org/10808058/diff/1/media/filters/gpu_video_decode...
File media/filters/gpu_video_decoder.cc (right):

http://codereview.chromium.org/10808058/diff/1/media/filters/gpu_video_decode...
media/filters/gpu_video_decoder.cc:155:
demuxer_stream_->EnableBitstreamConverter();
On 2012/07/20 20:20:20, scherkus wrote:
> q: do you recall the reason for having the bitstream converter owned by the
> demuxer vs owned by GpuVD?
> 
> I guess it cuts down on code duplication when multiple VDs require bitstream
> conversion hm..

The decision long predates my involvement with the team.
When xhwang@ reworked the BSF stuff I thought about this a bit but didn't come
up with an obviously-much-better approach, so stayed quiet.

[Pawel Osciak, 2012-07-22 15:08:17]

lgtm in general, 2 comments.

http://codereview.chromium.org/10808058/diff/6001/content/common/gpu/media/vi...
File content/common/gpu/media/video_decode_accelerator_unittest.cc (right):

http://codereview.chromium.org/10808058/diff/6001/content/common/gpu/media/vi...
content/common/gpu/media/video_decode_accelerator_unittest.cc:540: *end_pos =
32;
Do you think you should be sending the IVF header to the decoder? Perhaps that's
why it's not working now?

http://codereview.chromium.org/10808058/diff/6001/content/common/gpu/media/vi...
content/common/gpu/media/video_decode_accelerator_unittest.cc:542: uint32
frame_size = *reinterpret_cast<uint32*>(&encoded_data_[*end_pos]);
I'm assuming you are ok segfaulting here on erroneous streams.

[Ami GONE FROM CHROMIUM, 2012-07-22 23:18:29]

http://codereview.chromium.org/10808058/diff/6001/content/common/gpu/media/vi...
File content/common/gpu/media/video_decode_accelerator_unittest.cc (right):

http://codereview.chromium.org/10808058/diff/6001/content/common/gpu/media/vi...
content/common/gpu/media/video_decode_accelerator_unittest.cc:540: *end_pos =
32;
On 2012/07/22 15:08:17, posciak wrote:
> Do you think you should be sending the IVF header to the decoder? Perhaps
that's
> why it's not working now?

Merely omitting the 32-byte header doesn't change the symptoms of failure I'm
seeing (plus, that header specifies height/width, so I'd be shocked if that
worked).
Omitting the 12-byte per-frame header as well triggers other problems.

It'd be best to get a description of the expectations around the stream from the
mfr.

http://codereview.chromium.org/10808058/diff/6001/content/common/gpu/media/vi...
content/common/gpu/media/video_decode_accelerator_unittest.cc:542: uint32
frame_size = *reinterpret_cast<uint32*>(&encoded_data_[*end_pos]);
On 2012/07/22 15:08:17, posciak wrote:
> I'm assuming you are ok segfaulting here on erroneous streams.

Yes, being a test binary I'm fine with that.

[Pawel Osciak, 2012-07-23 01:12:30]

On 2012/07/22 23:18:29, Ami Fischman wrote:
>
http://codereview.chromium.org/10808058/diff/6001/content/common/gpu/media/vi...
> File content/common/gpu/media/video_decode_accelerator_unittest.cc (right):
> 
>
http://codereview.chromium.org/10808058/diff/6001/content/common/gpu/media/vi...
> content/common/gpu/media/video_decode_accelerator_unittest.cc:540: *end_pos =
> 32;
> On 2012/07/22 15:08:17, posciak wrote:
> > Do you think you should be sending the IVF header to the decoder? Perhaps
> that's
> > why it's not working now?
> 
> Merely omitting the 32-byte header doesn't change the symptoms of failure I'm
> seeing (plus, that header specifies height/width, so I'd be shocked if that
> worked).

I don't know the structure of a vp8 stream, but w/h info could've been in the
encoded stream as well, like in the case of h264. Well, was worth a try. 

> It'd be best to get a description of the expectations around the stream from
the
> mfr.
> 

Yes, will get it tomorrow.

[brettw, 2012-07-23 19:11:15]

ppapi lgtm

[commit-bot: I haz the power, 2012-07-23 19:22:42]

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

[commit-bot: I haz the power, 2012-07-23 19:45:25]

Try job failure for 10808058-6001 (retry) on mac_rel for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-07-23 19:51:53]

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

[commit-bot: I haz the power, 2012-07-23 21:10:28]

Change committed as 147944