media: Remove VP9 flag, and enable VP9 support by default.

media: Remove VP9 flag, and enable VP9 support by default.

BUG=180280
TEST=VP9 pipeline integration tests pass

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

[Tom Finegan, 2013-06-12 19:46:13]

Hello reviewers,

This CL, while small, touches a few different places in the tree.

acolwell: Please review MSE related changes.
jochen: Please review content/ and chrome/ changes.
scherkus: Please review media changes.
wolenetz: Please check the change in stream_parser_factory (nothing especially
interesting, it's just that the HISTOGRAM codec tag stuff is new to me).

Thanks!

[scherkus (not reviewing), 2013-06-12 20:06:31]

https://codereview.chromium.org/16830004/diff/1/webkit/renderer/media/webmedi...
File webkit/renderer/media/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/16830004/diff/1/webkit/renderer/media/webmedi...
webkit/renderer/media/webmediaplayer_impl.cc:1081: video_decoders.push_back(new
media::VpxVideoDecoder(media_loop_));
OOC this will still prefer FFmpeg's ffvp8 because you VpxVideoDecoder only does
VP8A?

[acolwell GONE FROM CHROMIUM, 2013-06-12 20:24:38]

LGTM % scherkus' comment

[wolenetz, 2013-06-12 20:32:28]

LGTM (stream_parser_factory), though I've added jrummell@ for closer review of
stream_parser_factory changes.

[Tom Finegan, 2013-06-12 20:35:49]

Oops, forgot thought that you did the histogram stuff, wolenetz. Thanks! 

PTAL, jrummel. :)

https://codereview.chromium.org/16830004/diff/1/webkit/renderer/media/webmedi...
File webkit/renderer/media/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/16830004/diff/1/webkit/renderer/media/webmedi...
webkit/renderer/media/webmediaplayer_impl.cc:1081: video_decoders.push_back(new
media::VpxVideoDecoder(media_loop_));
On 2013/06/12 20:06:31, scherkus wrote:
> OOC this will still prefer FFmpeg's ffvp8 because you VpxVideoDecoder only
does
> VP8A?

Right, VpxVideoDecoder only accepts VP8 input when:
- flag is enabled
- output video format is YV12A

FFmpeg still gets all non-alpha VP8.

[scherkus (not reviewing), 2013-06-12 21:48:49]

lgtm!

[jrummell, 2013-06-12 22:05:53]

Minor problem with deleting too much code.

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

https://codereview.chromium.org/16830004/diff/1/media/filters/stream_parser_f...
media/filters/stream_parser_factory.cc:203: const CommandLine* cmd_line =
CommandLine::ForCurrentProcess();
This is also needed for EAC3 playback, if configured. It can be moved inside the
#if defined below.

[Tom Finegan, 2013-06-12 23:17:48]

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

https://codereview.chromium.org/16830004/diff/1/media/filters/stream_parser_f...
media/filters/stream_parser_factory.cc:203: const CommandLine* cmd_line =
CommandLine::ForCurrentProcess();
On 2013/06/12 22:05:53, jrummell wrote:
> This is also needed for EAC3 playback, if configured. It can be moved inside
the
> #if defined below.

Fixed, though slightly reworked the checks because clang doesn't like having
cmd_line unused w/EAC3 disabled, and also didn't like defining cmd_line inside
the case statement w/o an inner scope. Thanks!

[jochen (gone - plz use gerrit), 2013-06-13 07:49:10]

lgtm

would you mind sending a PSA about this to blink-dev@chromium.org (as it's a web
exposed change)? Thanks!

[jrummell, 2013-06-13 16:24:39]

lgtm

[Tom Finegan, 2013-06-13 17:26:39]

On 2013/06/13 07:49:10, jochen wrote:
> lgtm
> 
> would you mind sending a PSA about this to mailto:blink-dev@chromium.org (as
it's a web
> exposed change)? Thanks!

No problem, writing it now.

[Tom Finegan, 2013-06-13 17:27:48]

Oops! I forgot to add the canPlayType() support.

willchan: Please do an OWNERS review of net/base/mime_util.cc changes. Thanks!

[wolenetz, 2013-06-13 17:49:02]

Sorry, I just spotted this nit. My previous lgtm stands % canPlayType() review
and this nit.

https://codereview.chromium.org/16830004/diff/19001/chrome/app/generated_reso...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/16830004/diff/19001/chrome/app/generated_reso...
chrome/app/generated_resources.grd:6747: +     <message
name="IDS_FLAGS_ENABLE_VP8_ALPHA_PLAYBACK_NAME" desc="Title for the flag to
enable VP8 Alpha playback in &lt;video&gt; elements.">
nit: missing 1 column of indentation?

[Tom Finegan, 2013-06-13 17:53:41]

https://codereview.chromium.org/16830004/diff/19001/chrome/app/generated_reso...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/16830004/diff/19001/chrome/app/generated_reso...
chrome/app/generated_resources.grd:6747: +     <message
name="IDS_FLAGS_ENABLE_VP8_ALPHA_PLAYBACK_NAME" desc="Title for the flag to
enable VP8 Alpha playback in &lt;video&gt; elements.">
On 2013/06/13 17:49:03, wolenetz wrote:
> nit: missing 1 column of indentation?

Done.

[willchan no longer on Chromium, 2013-06-13 20:30:47]

net/ LGTM. I'm still disappointed with the layering violation, but it's no worse
than what's already there.

[scherkus (not reviewing), 2013-06-13 21:36:34]

On 2013/06/13 20:30:47, willchan wrote:
> net/ LGTM. I'm still disappointed with the layering violation, but it's no
worse
> than what's already there.

willchan: is it time to reopen http://crbug.com/163800 ?

[commit-bot: I haz the power, 2013-06-14 17:23:49]

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

[commit-bot: I haz the power, 2013-06-14 22:52:47]

Retried try job too often on win7_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...

[commit-bot: I haz the power, 2013-06-14 22:54:19]

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

[commit-bot: I haz the power, 2013-06-15 00:50:57]

Change committed as 206530