Encrypted Media: Add VP9 support to IsTypeSupported().

Encrypted Media: Add VP9 support to IsTypeSupported().

BUG=361318
TEST=Add VP9 to unittests.

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

[xhwang, 2014-04-24 22:47:17]

PTAL

[ddorwin, 2014-04-24 23:00:22]

https://codereview.chromium.org/252513005/diff/1/chrome/browser/media/encrypt...
File chrome/browser/media/encrypted_media_istypesupported_browsertest.cc
(right):

https://codereview.chromium.org/252513005/diff/1/chrome/browser/media/encrypt...
chrome/browser/media/encrypted_media_istypesupported_browsertest.cc:25: #error
This file needs to be updated to run on Android.
Is it not even compiled currently?

https://codereview.chromium.org/252513005/diff/1/chrome/browser/media/encrypt...
chrome/browser/media/encrypted_media_istypesupported_browsertest.cc:101:
vp9_codec_.push_back("vp9");
We should really simplify this test sometime. :)

https://codereview.chromium.org/252513005/diff/1/chrome/browser/media/encrypt...
chrome/browser/media/encrypted_media_istypesupported_browsertest.cc:630:
"video/webm", vp9_codec(), kExternalClearKey));
Does ECK actually support VP9 yet?

https://codereview.chromium.org/252513005/diff/1/content/renderer/media/crypt...
File content/renderer/media/crypto/key_systems.cc (right):

https://codereview.chromium.org/252513005/diff/1/content/renderer/media/crypt...
content/renderer/media/crypto/key_systems.cc:53: {"vp9", EME_CODEC_WEBM_VP9},
I thought the .x was going to be required for VP9. I see that mime_util.cc
doesn't do this, but we should check with fgalligan.

https://codereview.chromium.org/252513005/diff/1/content/renderer/media/crypt...
content/renderer/media/crypto/key_systems.cc:68: // On Android, all WebM codecs
(Vorbis, VP8, AAC and AVC1) except VP9 are
I think you need to reword this "all" statement.

https://codereview.chromium.org/252513005/diff/1/content/renderer/media/crypt...
content/renderer/media/crypto/key_systems.cc:72: // TODO(xhwang): Query
MediaCodec for VP9 support on Android.
Or just mime_util? Can content/renderer access net/?

https://codereview.chromium.org/252513005/diff/1/content/renderer/media/crypt...
File content/renderer/media/crypto/key_systems_unittest.cc (right):

https://codereview.chromium.org/252513005/diff/1/content/renderer/media/crypt...
content/renderer/media/crypto/key_systems_unittest.cc:511:
EXPECT_TRUE(IsSupportedKeySystemWithMediaMimeType(
indentation

https://codereview.chromium.org/252513005/diff/1/third_party/widevine/cdm/wid...
File third_party/widevine/cdm/widevine_cdm_common.h (right):

https://codereview.chromium.org/252513005/diff/1/third_party/widevine/cdm/wid...
third_party/widevine/cdm/widevine_cdm_common.h:54: const char
kCdmSupportedCodecVp9[] = "vp9";
I think we should use vp9.0 in case we need vp9.1 or something later.

[xhwang, 2014-04-25 16:49:08]

Thanks for the comments. PTAnotherL!

https://codereview.chromium.org/252513005/diff/1/chrome/browser/media/encrypt...
File chrome/browser/media/encrypted_media_istypesupported_browsertest.cc
(right):

https://codereview.chromium.org/252513005/diff/1/chrome/browser/media/encrypt...
chrome/browser/media/encrypted_media_istypesupported_browsertest.cc:25: #error
This file needs to be updated to run on Android.
On 2014/04/24 23:00:23, ddorwin wrote:
> Is it not even compiled currently?

I think no browser tests are running on android right now. I may be wrong, then
this #error will catch it.

https://codereview.chromium.org/252513005/diff/1/chrome/browser/media/encrypt...
chrome/browser/media/encrypted_media_istypesupported_browsertest.cc:101:
vp9_codec_.push_back("vp9");
On 2014/04/24 23:00:23, ddorwin wrote:
> We should really simplify this test sometime. :)

Can't agree more! Filed bug and added TODO.

https://codereview.chromium.org/252513005/diff/1/chrome/browser/media/encrypt...
chrome/browser/media/encrypted_media_istypesupported_browsertest.cc:630:
"video/webm", vp9_codec(), kExternalClearKey));
On 2014/04/24 23:00:23, ddorwin wrote:
> Does ECK actually support VP9 yet?

Yes, there's a browser test added to cover this!

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/med...

https://codereview.chromium.org/252513005/diff/1/content/renderer/media/crypt...
File content/renderer/media/crypto/key_systems.cc (right):

https://codereview.chromium.org/252513005/diff/1/content/renderer/media/crypt...
content/renderer/media/crypto/key_systems.cc:53: {"vp9", EME_CODEC_WEBM_VP9},
On 2014/04/24 23:00:23, ddorwin wrote:
> I thought the .x was going to be required for VP9. I see that mime_util.cc
> doesn't do this, but we should check with fgalligan.

Checked and we don't need to support x > 0.

http://www.webmproject.org/docs/container/

https://codereview.chromium.org/252513005/diff/1/content/renderer/media/crypt...
content/renderer/media/crypto/key_systems.cc:68: // On Android, all WebM codecs
(Vorbis, VP8, AAC and AVC1) except VP9 are
On 2014/04/24 23:00:23, ddorwin wrote:
> I think you need to reword this "all" statement.

Done.

https://codereview.chromium.org/252513005/diff/1/content/renderer/media/crypt...
content/renderer/media/crypto/key_systems.cc:72: // TODO(xhwang): Query
MediaCodec for VP9 support on Android.
On 2014/04/24 23:00:23, ddorwin wrote:
> Or just mime_util? Can content/renderer access net/?

It seems so:
https://code.google.com/p/chromium/codesearch#chromium/src/content/DEPS&l=43

Updated comment.

https://codereview.chromium.org/252513005/diff/1/content/renderer/media/crypt...
File content/renderer/media/crypto/key_systems_unittest.cc (right):

https://codereview.chromium.org/252513005/diff/1/content/renderer/media/crypt...
content/renderer/media/crypto/key_systems_unittest.cc:511:
EXPECT_TRUE(IsSupportedKeySystemWithMediaMimeType(
On 2014/04/24 23:00:23, ddorwin wrote:
> indentation

Done.

https://codereview.chromium.org/252513005/diff/1/third_party/widevine/cdm/wid...
File third_party/widevine/cdm/widevine_cdm_common.h (right):

https://codereview.chromium.org/252513005/diff/1/third_party/widevine/cdm/wid...
third_party/widevine/cdm/widevine_cdm_common.h:54: const char
kCdmSupportedCodecVp9[] = "vp9";
On 2014/04/24 23:00:23, ddorwin wrote:
> I think we should use vp9.0 in case we need vp9.1 or something later.

Done.

[xhwang, 2014-04-25 16:51:28]

rebase only

[xhwang, 2014-04-28 18:25:40]

kindly ping

[ddorwin, 2014-04-28 18:30:51]

Oops, I thought I had published these comments.

FYI, PS2 is not (no longer?) "rebase only" It's the changes from the previous
comments.

https://codereview.chromium.org/252513005/diff/1/chrome/browser/media/encrypt...
File chrome/browser/media/encrypted_media_istypesupported_browsertest.cc
(right):

https://codereview.chromium.org/252513005/diff/1/chrome/browser/media/encrypt...
chrome/browser/media/encrypted_media_istypesupported_browsertest.cc:25: #error
This file needs to be updated to run on Android.
On 2014/04/25 16:49:08, xhwang wrote:
> On 2014/04/24 23:00:23, ddorwin wrote:
> > Is it not even compiled currently?
> 
> I think no browser tests are running on android right now. I may be wrong,
then
> this #error will catch it.

My point was this might fail the build if the tests are compiled but not run.

https://codereview.chromium.org/252513005/diff/1/content/renderer/media/crypt...
File content/renderer/media/crypto/key_systems.cc (right):

https://codereview.chromium.org/252513005/diff/1/content/renderer/media/crypt...
content/renderer/media/crypto/key_systems.cc:53: {"vp9", EME_CODEC_WEBM_VP9},
On 2014/04/25 16:49:08, xhwang wrote:
> On 2014/04/24 23:00:23, ddorwin wrote:
> > I thought the .x was going to be required for VP9. I see that mime_util.cc
> > doesn't do this, but we should check with fgalligan.
> 
> Checked and we don't need to support x > 0.
> 
> http://www.webmproject.org/docs/container/

That doesn't say anything about VP9.

[ddorwin, 2014-04-28 19:53:02]

LGTM if Android builds.

[xhwang, 2014-04-28 22:41:56]

On 2014/04/28 19:53:02, ddorwin wrote:
> LGTM if Android builds.

Target browser_tests_apk doesn't exist now. Target browser_tests does exist but
when I try to compile it I got a bunch of errors (not related to my CL), which
make me think nobody is building browser_tests on Android. I'll let the commit
queue to further check that for me.

[xhwang, 2014-04-28 22:57:36]

jam@: Please OWNERS review the following two files:

- content/public/common/eme_codec.h
- chrome/common/chrome_content_client.cc

It's a simple change and should take <1min to review.

[jam, 2014-04-29 00:35:21]

lgtm

[xhwang, 2014-04-29 00:43:50]

The CQ bit was checked by xhwang@chromium.org

[commit-bot: I haz the power, 2014-04-29 00:45:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xhwang@chromium.org/252513005/20001

[commit-bot: I haz the power, 2014-04-29 01:41:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-29 01:41:31]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_rel

[xhwang, 2014-04-29 16:39:02]

The CQ bit was checked by xhwang@chromium.org

[commit-bot: I haz the power, 2014-04-29 16:39:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xhwang@chromium.org/252513005/20001

[commit-bot: I haz the power, 2014-04-30 02:22:38]

Change committed as 267047