media: Support VP9 in media source.

media: Support VP9 in media source.


Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=192672

[Tom Finegan, 2013-04-04 22:09:57]

Super tiny CL, with a test (yay!) that's disabled (boo!). PTAL, thanks!

https://codereview.chromium.org/13652011/diff/1/media/filters/pipeline_integr...
File media/filters/pipeline_integration_test.cc (right):

https://codereview.chromium.org/13652011/diff/1/media/filters/pipeline_integr...
media/filters/pipeline_integration_test.cc:24: static const char kWebMVp9[] =
"video/webm; codecs=\"vp9\"";
Should I use VP9 here (to be consistent with test names), Vp9 in both places, or
leave this as-is?

https://codereview.chromium.org/13652011/diff/1/media/filters/pipeline_integr...
media/filters/pipeline_integration_test.cc:55: static const int
kVp9WebMFileDurationMs = 2736;
Ditto.

https://codereview.chromium.org/13652011/diff/1/media/filters/pipeline_integr...
media/filters/pipeline_integration_test.cc:460: // TODO(tomfinegan): Enable this
test when the VP9 bitstream is finalized.
This is really about crbug.com/173333

Should I update the comment, or is this fine?

[acolwell GONE FROM CHROMIUM, 2013-04-04 22:49:28]

Looks pretty good. Just a few questions.

Did the keyframe start code get changed for VP9? If so then you'll need to
update WebMClusterParser::Track::IsKeyframe() as well to properly handle VP9
keyframe detection in a BlockGroup.

https://codereview.chromium.org/13652011/diff/1/media/filters/pipeline_integr...
File media/filters/pipeline_integration_test.cc (right):

https://codereview.chromium.org/13652011/diff/1/media/filters/pipeline_integr...
media/filters/pipeline_integration_test.cc:24: static const char kWebMVp9[] =
"video/webm; codecs=\"vp9\"";
On 2013/04/04 22:09:57, Tom Finegan wrote:
> Should I use VP9 here (to be consistent with test names), Vp9 in both places,
or
> leave this as-is?
I'd prefer VP9 here and below. I feel it's equivalant to MP4 below.

https://codereview.chromium.org/13652011/diff/1/media/filters/pipeline_integr...
media/filters/pipeline_integration_test.cc:460: // TODO(tomfinegan): Enable this
test when the VP9 bitstream is finalized.
On 2013/04/04 22:09:57, Tom Finegan wrote:
> This is really about crbug.com/173333
> 
> Should I update the comment, or is this fine?
Please update the comment to point to the bug.

https://codereview.chromium.org/13652011/diff/1/media/filters/stream_parser_f...
File media/filters/stream_parser_factory.cc (right):

https://codereview.chromium.org/13652011/diff/1/media/filters/stream_parser_f...
media/filters/stream_parser_factory.cc:50: &kVP9CodecInfo,
Is VP9 decoding on by default now? If not then this will lead applications to
believe that they can actually play VP9 content when we actually can't.

[fgalligan1, 2013-04-04 23:05:07]

On 2013/04/04 22:49:28, acolwell wrote:
> Looks pretty good. Just a few questions.
> 
> Did the keyframe start code get changed for VP9? If so then you'll need to
> update WebMClusterParser::Track::IsKeyframe() as well to properly handle VP9
> keyframe detection in a BlockGroup.
>
It is the same for now.

[Tom Finegan, 2013-04-05 03:35:05]

PTAL... still works!

https://codereview.chromium.org/13652011/diff/1/media/filters/pipeline_integr...
File media/filters/pipeline_integration_test.cc (right):

https://codereview.chromium.org/13652011/diff/1/media/filters/pipeline_integr...
media/filters/pipeline_integration_test.cc:24: static const char kWebMVp9[] =
"video/webm; codecs=\"vp9\"";
On 2013/04/04 22:49:28, acolwell wrote:
> On 2013/04/04 22:09:57, Tom Finegan wrote:
> > Should I use VP9 here (to be consistent with test names), Vp9 in both
places,
> or
> > leave this as-is?
> I'd prefer VP9 here and below. I feel it's equivalant to MP4 below.

Done.

https://codereview.chromium.org/13652011/diff/1/media/filters/pipeline_integr...
media/filters/pipeline_integration_test.cc:55: static const int
kVp9WebMFileDurationMs = 2736;
On 2013/04/04 22:09:57, Tom Finegan wrote:
> Ditto.

Done.

https://codereview.chromium.org/13652011/diff/1/media/filters/pipeline_integr...
media/filters/pipeline_integration_test.cc:460: // TODO(tomfinegan): Enable this
test when the VP9 bitstream is finalized.
On 2013/04/04 22:49:28, acolwell wrote:
> On 2013/04/04 22:09:57, Tom Finegan wrote:
> > This is really about crbug.com/173333
> > 
> > Should I update the comment, or is this fine?
> Please update the comment to point to the bug.

Done.

https://codereview.chromium.org/13652011/diff/1/media/filters/stream_parser_f...
File media/filters/stream_parser_factory.cc (right):

https://codereview.chromium.org/13652011/diff/1/media/filters/stream_parser_f...
media/filters/stream_parser_factory.cc:50: &kVP9CodecInfo,
On 2013/04/04 22:49:28, acolwell wrote:
> Is VP9 decoding on by default now? If not then this will lead applications to
> believe that they can actually play VP9 content when we actually can't.

Added a hackish fix for this, PTAL.

https://codereview.chromium.org/13652011/diff/8001/media/filters/stream_parse...
File media/filters/stream_parser_factory.cc (right):

https://codereview.chromium.org/13652011/diff/8001/media/filters/stream_parse...
media/filters/stream_parser_factory.cc:222: return false;
Should I add an informative log message for this? (i.e. VP9 isn't supported
because the flag is required). Otherwise, maybe this isn't that bad.

[acolwell GONE FROM CHROMIUM, 2013-04-05 14:52:11]

LGTM

https://codereview.chromium.org/13652011/diff/8001/media/filters/stream_parse...
File media/filters/stream_parser_factory.cc (right):

https://codereview.chromium.org/13652011/diff/8001/media/filters/stream_parse...
media/filters/stream_parser_factory.cc:221:
!cmd_line->HasSwitch(switches::kEnableVp9Playback))
nit: Add {} since this is a multi-line conditional.

https://codereview.chromium.org/13652011/diff/8001/media/filters/stream_parse...
media/filters/stream_parser_factory.cc:222: return false;
On 2013/04/05 03:35:06, Tom Finegan wrote:
> Should I add an informative log message for this? (i.e. VP9 isn't supported
> because the flag is required). Otherwise, maybe this isn't that bad.
I think this is fine for now.

[scherkus (not reviewing), 2013-04-05 18:11:13]

lgtm

[commit-bot: I haz the power, 2013-04-05 18:13:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tomfinegan@chromium.org/13652011/15001

[commit-bot: I haz the power, 2013-04-06 00:35:36]

Change committed as 192672