Store SupportedCodecs in KeySystemInfo and KeySystems.

Store SupportedCodecs in KeySystemInfo and KeySystems.

Previously we convert SupportedCodecs (a uint32 bit mask indicating which
codec is supported) to ContainerCodecMap (a map from a container type to a set
of supported codecs in that container). Then we pass ContainerCodecMap to
KeySystems and store it there for easy look up.

This results in a lot of duplicate converting code. This CL stores
SupportedCodecs in KeySystmeInfo and KeySystems so that we don't need to convert
anything. Then when IsTypeSupported() is called, we find the bit masks for
queried container and codec types and compare it with the SupportedCodecs stored
in KeySystems.

BUG=362769
TEST=All existing tests pass.

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

[xhwang, 2014-04-21 23:12:19]

PTAL

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

https://codereview.chromium.org/246033002/diff/1/chrome/browser/media/encrypt...
chrome/browser/media/encrypted_media_istypesupported_browsertest.cc:868:
EXPECT_WVAAC(IsSupportedKeySystemWithMediaMimeType(
+ddorwin:

When AAC is not supported for Widevine, there are no MP4 audio codecs supported.
Therefore IsSupportedKeySystemWithMediaMimeType("audio/mp4", no_codecs(),
kWidevine) returns false with this CL.

Previously we declare "audio/mp4" support even if we only support MP4 video
codecs.

Let me know which way is preferred.

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

https://codereview.chromium.org/246033002/diff/1/content/renderer/media/crypt...
content/renderer/media/crypto/key_systems.cc:153: MaskMap codec_masks_;
Note: Use hash maps for quick look up.

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

https://codereview.chromium.org/246033002/diff/1/content/renderer/media/crypt...
content/renderer/media/crypto/key_systems.h:58: CONTENT_EXPORT void
AddCodecMask(const std::string& codec, uint32 mask);
ddorwin: This is currently used by the test. But it can also be used to add
platform specific container/codecs.

https://codereview.chromium.org/246033002/diff/1/media/cdm/encrypted_media_co...
File media/cdm/encrypted_media_codecs.h (right):

https://codereview.chromium.org/246033002/diff/1/media/cdm/encrypted_media_co...
media/cdm/encrypted_media_codecs.h:1: // Copyright 2014 The Chromium Authors.
All rights reserved.
This is need by:

chrome/common/encrypted_media_messages_android.h
android_webview/renderer/aw_key_systems.cc
content/public/renderer/key_system_info.h
content/renderer/media/crypto/key_systems.cc

I am having a hard time to find a good path for it without changing the existing
DEPS rules.

[xhwang, 2014-04-22 00:12:22]

rebase only

[xhwang, 2014-04-22 00:42:54]

FYI, I need to fix Android build. You are welcome to review the rest of the
change though.

[ddorwin, 2014-04-22 21:24:41]

https://codereview.chromium.org/246033002/diff/40001/android_webview/renderer...
File android_webview/renderer/DEPS (right):

https://codereview.chromium.org/246033002/diff/40001/android_webview/renderer...
android_webview/renderer/DEPS:1: include_rules = [
Interesting: The OWNERS inherits DEPS=* from src/. That's probably a bug in the
OWNERS file.

https://codereview.chromium.org/246033002/diff/40001/chrome/browser/media/enc...
File chrome/browser/media/encrypted_media_istypesupported_browsertest.cc
(right):

https://codereview.chromium.org/246033002/diff/40001/chrome/browser/media/enc...
chrome/browser/media/encrypted_media_istypesupported_browsertest.cc:862:
EXPECT_WVAAC(IsSupportedKeySystemWithMediaMimeType(
This must not change. It will negatively affect production websites.

https://codereview.chromium.org/246033002/diff/40001/chrome/browser/media/enc...
chrome/browser/media/encrypted_media_istypesupported_browsertest.cc:868:
EXPECT_WVAAC(IsSupportedKeySystemWithMediaMimeType(
ditto

https://codereview.chromium.org/246033002/diff/40001/chrome/browser/media/enc...
File chrome/browser/media/encrypted_media_message_filter_android.cc (right):

https://codereview.chromium.org/246033002/diff/40001/chrome/browser/media/enc...
chrome/browser/media/encrypted_media_message_filter_android.cc:106:
DCHECK(!(request.codecs & media::INVALID_CODECS)) << "unrecognized codec";
DCHECK(request.codecs & media::ALL_CODECS) seems easier to read. Do we even need
INVALID_CODECS?

https://codereview.chromium.org/246033002/diff/40001/chrome/renderer/media/ch...
File chrome/renderer/media/chrome_key_systems.cc (right):

https://codereview.chromium.org/246033002/diff/40001/chrome/renderer/media/ch...
chrome/renderer/media/chrome_key_systems.cc:153: info.supported_codecs =
supported_codecs;
The comment in the base revision is still valid and needs to be addressed
somewhere to fix the browser test.

https://codereview.chromium.org/246033002/diff/40001/chrome/renderer/media/ch...
chrome/renderer/media/chrome_key_systems.cc:244:
DCHECK(!(response.compositing_codecs & media::INVALID_CODECS))
ditto here and below.

https://codereview.chromium.org/246033002/diff/40001/content/public/renderer/...
File content/public/renderer/DEPS (right):

https://codereview.chromium.org/246033002/diff/40001/content/public/renderer/...
content/public/renderer/DEPS:2: "+media/cdm",
Can and should we put encrypted_media_codecs.h in content/public/common/
instead? Or does that break down on the Android browser side?

If not, I feel like there should be public/ directories for things like this.
WDYT?

https://codereview.chromium.org/246033002/diff/40001/content/public/renderer/...
File content/public/renderer/key_system_info.h (right):

https://codereview.chromium.org/246033002/diff/40001/content/public/renderer/...
content/public/renderer/key_system_info.h:41: // Specifies codecs supported by
|key_system|.
Will we eventually need a separate list of InitData types?

https://codereview.chromium.org/246033002/diff/40001/content/renderer/media/c...
File content/renderer/media/crypto/key_systems.cc (right):

https://codereview.chromium.org/246033002/diff/40001/content/renderer/media/c...
content/renderer/media/crypto/key_systems.cc:40: // A container is supported iif
a codec that belongs to it is supported.
This is the cause of the problem I mentioned earlier.

https://codereview.chromium.org/246033002/diff/40001/content/renderer/media/c...
content/renderer/media/crypto/key_systems.cc:45: {"video/webm",
media::WEBM_CODECS},
I mentioned this in the .h file later, but see how AUDIO and CODECS don't look
right together?

https://codereview.chromium.org/246033002/diff/40001/content/renderer/media/c...
content/renderer/media/crypto/key_systems.cc:75: info.supported_codecs =
media::WEBM_CODECS;
OOC, what will this look like when we selectively enable VP9? We'll either be
building it from the base codecs or subtracting VP9. The same may eventually
apply to Opus.

https://codereview.chromium.org/246033002/diff/40001/content/renderer/media/c...
content/renderer/media/crypto/key_systems.cc:115: const SupportedCodecs&
supported_codecs,
an integer, so can remove '&'

https://codereview.chromium.org/246033002/diff/40001/content/renderer/media/c...
content/renderer/media/crypto/key_systems.cc:119: friend class KeySystemsTest;
I don't think this is right. This class is local.

https://codereview.chromium.org/246033002/diff/40001/content/renderer/media/c...
content/renderer/media/crypto/key_systems.cc:141: const std::string&
key_system);
Should be const method?

https://codereview.chromium.org/246033002/diff/40001/content/renderer/media/c...
content/renderer/media/crypto/key_systems.cc:169:
container_masks_[kContainerMasks[i].type] = kContainerMasks[i].mask;
DCHECK(!find())?

https://codereview.chromium.org/246033002/diff/40001/content/renderer/media/c...
content/renderer/media/crypto/key_systems.cc:241: const std::string& codec,
We query each codec? Maybe we should pass multiple now (or a future CL) since
we're using a mask anyway.

https://codereview.chromium.org/246033002/diff/40001/content/renderer/media/c...
content/renderer/media/crypto/key_systems.cc:258: const SupportedCodecs&
supported_codecs =
ditto: '&'.

https://codereview.chromium.org/246033002/diff/40001/content/renderer/media/c...
content/renderer/media/crypto/key_systems.cc:260: 
This code is not obvious. It might help to either use variable names or
comments.

I assume this is looking up supported codecs for a key system then using that to
determine the associated mime type and whether it's supported by the key system.
It's hard to reason about. Then we check actual codecs below.

This also assumes that the inferred initDataType is supported. I guess we strip
the codecs if the MIME type is not supported elsewhere? That's also not obvious.

https://codereview.chromium.org/246033002/diff/40001/content/renderer/media/c...
content/renderer/media/crypto/key_systems.cc:274: !(codec_iter->second &
supported_codecs)) {
Even this is a bit confusing.

https://codereview.chromium.org/246033002/diff/40001/content/renderer/media/c...
File content/renderer/media/crypto/key_systems.h (right):

https://codereview.chromium.org/246033002/diff/40001/content/renderer/media/c...
content/renderer/media/crypto/key_systems.h:54: // Helper functions to add extra
container/codec masks. This can be used to add
Is this only for testing? Is the only reason we need this because of the foo and
bar types in the tests?
If so, can we use #ifdef UNIT_TEST so this is only visible for tests?

https://codereview.chromium.org/246033002/diff/40001/content/renderer/media/c...
File content/renderer/media/crypto/key_systems_unittest.cc (right):

https://codereview.chromium.org/246033002/diff/40001/content/renderer/media/c...
content/renderer/media/crypto/key_systems_unittest.cc:56: FOO_AUDIO_CODEC = 1 <<
10,
Is there significance to 10?

https://codereview.chromium.org/246033002/diff/40001/content/renderer/media/c...
content/renderer/media/crypto/key_systems_unittest.cc:63:
COMPILE_ASSERT((FOO_CODECS & media::ALL_CODECS) == media::NO_CODECS,
Maybe we should just remove some PROPRIETARY_CODECS checks in the enums but
still prevent them from being used externally. Then we wouldn't need foo, the
additional methods, etc. WDYT?

https://codereview.chromium.org/246033002/diff/40001/media/cdm/encrypted_medi...
File media/cdm/encrypted_media_codecs.h (right):

https://codereview.chromium.org/246033002/diff/40001/media/cdm/encrypted_medi...
media/cdm/encrypted_media_codecs.h:13: typedef uint32 SupportedCodecs;
Move this after the enum? The comment describes the enum but is immediately
followed by the typedef.

https://codereview.chromium.org/246033002/diff/40001/media/cdm/encrypted_medi...
media/cdm/encrypted_media_codecs.h:18: WEBM_AUDIO = WEBM_VORBIS,
Should we add _CODECS to the end of this? AUDIO and VIDEO are sort of strange,
especially when used next to WEBM_CODECS.

Maybe WEBM_CODECS should be WEBM_ALL_CODECS

https://codereview.chromium.org/246033002/diff/40001/media/cdm/encrypted_medi...
media/cdm/encrypted_media_codecs.h:32: INVALID_CODECS = ~ALL_CODECS
Do we really need this?

[xhwang, 2014-04-23 17:29:14]

Thanks for the valuable comments. I think I addressed most of the comments. PTAL
again.

https://codereview.chromium.org/246033002/diff/40001/android_webview/renderer...
File android_webview/renderer/DEPS (right):

https://codereview.chromium.org/246033002/diff/40001/android_webview/renderer...
android_webview/renderer/DEPS:1: include_rules = [
On 2014/04/22 21:24:41, ddorwin wrote:
> Interesting: The OWNERS inherits DEPS=* from src/. That's probably a bug in
the
> OWNERS file.

ok

https://codereview.chromium.org/246033002/diff/40001/chrome/browser/media/enc...
File chrome/browser/media/encrypted_media_istypesupported_browsertest.cc
(right):

https://codereview.chromium.org/246033002/diff/40001/chrome/browser/media/enc...
chrome/browser/media/encrypted_media_istypesupported_browsertest.cc:862:
EXPECT_WVAAC(IsSupportedKeySystemWithMediaMimeType(
On 2014/04/22 21:24:41, ddorwin wrote:
> This must not change. It will negatively affect production websites.

Done.

https://codereview.chromium.org/246033002/diff/40001/chrome/browser/media/enc...
chrome/browser/media/encrypted_media_istypesupported_browsertest.cc:868:
EXPECT_WVAAC(IsSupportedKeySystemWithMediaMimeType(
On 2014/04/22 21:24:41, ddorwin wrote:
> ditto

Done.

https://codereview.chromium.org/246033002/diff/40001/chrome/browser/media/enc...
File chrome/browser/media/encrypted_media_message_filter_android.cc (right):

https://codereview.chromium.org/246033002/diff/40001/chrome/browser/media/enc...
chrome/browser/media/encrypted_media_message_filter_android.cc:106:
DCHECK(!(request.codecs & media::INVALID_CODECS)) << "unrecognized codec";
On 2014/04/22 21:24:41, ddorwin wrote:
> DCHECK(request.codecs & media::ALL_CODECS) seems easier to read. Do we even
need
> INVALID_CODECS?

Done.

https://codereview.chromium.org/246033002/diff/40001/chrome/renderer/media/ch...
File chrome/renderer/media/chrome_key_systems.cc (right):

https://codereview.chromium.org/246033002/diff/40001/chrome/renderer/media/ch...
chrome/renderer/media/chrome_key_systems.cc:153: info.supported_codecs =
supported_codecs;
On 2014/04/22 21:24:41, ddorwin wrote:
> The comment in the base revision is still valid and needs to be addressed
> somewhere to fix the browser test.

Done.

https://codereview.chromium.org/246033002/diff/40001/chrome/renderer/media/ch...
chrome/renderer/media/chrome_key_systems.cc:244:
DCHECK(!(response.compositing_codecs & media::INVALID_CODECS))
On 2014/04/22 21:24:41, ddorwin wrote:
> ditto here and below.

Done.

https://codereview.chromium.org/246033002/diff/40001/content/public/renderer/...
File content/public/renderer/DEPS (right):

https://codereview.chromium.org/246033002/diff/40001/content/public/renderer/...
content/public/renderer/DEPS:2: "+media/cdm",
On 2014/04/22 21:24:41, ddorwin wrote:
> Can and should we put encrypted_media_codecs.h in content/public/common/
> instead? Or does that break down on the Android browser side?
> 
> If not, I feel like there should be public/ directories for things like this.
> WDYT?

Done.

https://codereview.chromium.org/246033002/diff/40001/content/public/renderer/...
File content/public/renderer/key_system_info.h (right):

https://codereview.chromium.org/246033002/diff/40001/content/public/renderer/...
content/public/renderer/key_system_info.h:41: // Specifies codecs supported by
|key_system|.
On 2014/04/22 21:24:41, ddorwin wrote:
> Will we eventually need a separate list of InitData types?

I think so. From the CDMs perspective, codecs and initdata types should be
orthogonal.

https://codereview.chromium.org/246033002/diff/40001/content/renderer/media/c...
File content/renderer/media/crypto/key_systems.cc (right):

https://codereview.chromium.org/246033002/diff/40001/content/renderer/media/c...
content/renderer/media/crypto/key_systems.cc:40: // A container is supported iif
a codec that belongs to it is supported.
On 2014/04/22 21:24:41, ddorwin wrote:
> This is the cause of the problem I mentioned earlier.

Done.

https://codereview.chromium.org/246033002/diff/40001/content/renderer/media/c...
content/renderer/media/crypto/key_systems.cc:45: {"video/webm",
media::WEBM_CODECS},
On 2014/04/22 21:24:41, ddorwin wrote:
> I mentioned this in the .h file later, but see how AUDIO and CODECS don't look
> right together?

Done.

https://codereview.chromium.org/246033002/diff/40001/content/renderer/media/c...
content/renderer/media/crypto/key_systems.cc:75: info.supported_codecs =
media::WEBM_CODECS;
On 2014/04/22 21:24:41, ddorwin wrote:
> OOC, what will this look like when we selectively enable VP9? We'll either be
> building it from the base codecs or subtracting VP9. The same may eventually
> apply to Opus.

VP9 will be device dependent on Android. So we really can't build the
information statically here. 

Short term plan: Enable VP9 everywhere except here (clearkey + android).

Long term plan: We'll need to do something similar to AddAndroidWidevine() to
query that information from the browser process. WDYT?

https://codereview.chromium.org/246033002/diff/40001/content/renderer/media/c...
content/renderer/media/crypto/key_systems.cc:115: const SupportedCodecs&
supported_codecs,
On 2014/04/22 21:24:41, ddorwin wrote:
> an integer, so can remove '&'

Done.

https://codereview.chromium.org/246033002/diff/40001/content/renderer/media/c...
content/renderer/media/crypto/key_systems.cc:119: friend class KeySystemsTest;
On 2014/04/22 21:24:41, ddorwin wrote:
> I don't think this is right. This class is local.

I forgot to remove this. Removed.

https://codereview.chromium.org/246033002/diff/40001/content/renderer/media/c...
content/renderer/media/crypto/key_systems.cc:141: const std::string&
key_system);
On 2014/04/22 21:24:41, ddorwin wrote:
> Should be const method?

This function has been refactored. The new function is const.

https://codereview.chromium.org/246033002/diff/40001/content/renderer/media/c...
content/renderer/media/crypto/key_systems.cc:169:
container_masks_[kContainerMasks[i].type] = kContainerMasks[i].mask;
On 2014/04/22 21:24:41, ddorwin wrote:
> DCHECK(!find())?

Done.

https://codereview.chromium.org/246033002/diff/40001/content/renderer/media/c...
content/renderer/media/crypto/key_systems.cc:241: const std::string& codec,
On 2014/04/22 21:24:41, ddorwin wrote:
> We query each codec? Maybe we should pass multiple now (or a future CL) since
> we're using a mask anyway.

Done.

https://codereview.chromium.org/246033002/diff/40001/content/renderer/media/c...
content/renderer/media/crypto/key_systems.cc:258: const SupportedCodecs&
supported_codecs =
On 2014/04/22 21:24:41, ddorwin wrote:
> ditto: '&'.

Done.

https://codereview.chromium.org/246033002/diff/40001/content/renderer/media/c...
content/renderer/media/crypto/key_systems.cc:260: 
On 2014/04/22 21:24:41, ddorwin wrote:
> This code is not obvious. It might help to either use variable names or
> comments.
> 
> I assume this is looking up supported codecs for a key system then using that
to
> determine the associated mime type and whether it's supported by the key
system.
> It's hard to reason about. Then we check actual codecs below.
> 
> This also assumes that the inferred initDataType is supported. I guess we
strip
> the codecs if the MIME type is not supported elsewhere? That's also not
obvious.

Done.

https://codereview.chromium.org/246033002/diff/40001/content/renderer/media/c...
content/renderer/media/crypto/key_systems.cc:274: !(codec_iter->second &
supported_codecs)) {
On 2014/04/22 21:24:41, ddorwin wrote:
> Even this is a bit confusing.

Done.

https://codereview.chromium.org/246033002/diff/40001/content/renderer/media/c...
File content/renderer/media/crypto/key_systems.h (right):

https://codereview.chromium.org/246033002/diff/40001/content/renderer/media/c...
content/renderer/media/crypto/key_systems.h:54: // Helper functions to add extra
container/codec masks. This can be used to add
On 2014/04/22 21:24:41, ddorwin wrote:
> Is this only for testing? Is the only reason we need this because of the foo
and
> bar types in the tests?
> If so, can we use #ifdef UNIT_TEST so this is only visible for tests?

Done.

https://codereview.chromium.org/246033002/diff/40001/content/renderer/media/c...
File content/renderer/media/crypto/key_systems_unittest.cc (right):

https://codereview.chromium.org/246033002/diff/40001/content/renderer/media/c...
content/renderer/media/crypto/key_systems_unittest.cc:56: FOO_AUDIO_CODEC = 1 <<
10,
On 2014/04/22 21:24:41, ddorwin wrote:
> Is there significance to 10?

Added a comment.

https://codereview.chromium.org/246033002/diff/40001/content/renderer/media/c...
content/renderer/media/crypto/key_systems_unittest.cc:63:
COMPILE_ASSERT((FOO_CODECS & media::ALL_CODECS) == media::NO_CODECS,
On 2014/04/22 21:24:41, ddorwin wrote:
> Maybe we should just remove some PROPRIETARY_CODECS checks in the enums but
> still prevent them from being used externally. Then we wouldn't need foo, the
> additional methods, etc. WDYT?

I am not sure what you exactly mean...

https://codereview.chromium.org/246033002/diff/40001/media/cdm/encrypted_medi...
File media/cdm/encrypted_media_codecs.h (right):

https://codereview.chromium.org/246033002/diff/40001/media/cdm/encrypted_medi...
media/cdm/encrypted_media_codecs.h:13: typedef uint32 SupportedCodecs;
On 2014/04/22 21:24:41, ddorwin wrote:
> Move this after the enum? The comment describes the enum but is immediately
> followed by the typedef.

Done.

https://codereview.chromium.org/246033002/diff/40001/media/cdm/encrypted_medi...
media/cdm/encrypted_media_codecs.h:18: WEBM_AUDIO = WEBM_VORBIS,
On 2014/04/22 21:24:41, ddorwin wrote:
> Should we add _CODECS to the end of this? AUDIO and VIDEO are sort of strange,
> especially when used next to WEBM_CODECS.
> 
> Maybe WEBM_CODECS should be WEBM_ALL_CODECS

Done.

https://codereview.chromium.org/246033002/diff/40001/media/cdm/encrypted_medi...
media/cdm/encrypted_media_codecs.h:32: INVALID_CODECS = ~ALL_CODECS
On 2014/04/22 21:24:41, ddorwin wrote:
> Do we really need this?

Removed now.

[ddorwin, 2014-04-23 21:07:20]

lgtm % comments

https://codereview.chromium.org/246033002/diff/140001/content/public/common/e...
File content/public/common/encrypted_media_codecs.h (right):

https://codereview.chromium.org/246033002/diff/140001/content/public/common/e...
content/public/common/encrypted_media_codecs.h:29: // Defines bitmask values
used to specify supported codecs.
The comment should be at line 10, right?

https://codereview.chromium.org/246033002/diff/140001/content/renderer/media/...
File content/renderer/media/crypto/key_systems.cc (right):

https://codereview.chromium.org/246033002/diff/140001/content/renderer/media/...
content/renderer/media/crypto/key_systems.cc:251: // same as "video/foo".
Convert the |container| to achieve this.
Should we have a TODO to replace this with real checks against supported
initDataTypes combined with what the Chrome demuxers support?

https://codereview.chromium.org/246033002/diff/140001/content/renderer/media/...
content/renderer/media/crypto/key_systems.cc:253: if (container.find("audio/")
== 0)
This would break down for audio/mpeg, but we don't need to worry about that.

https://codereview.chromium.org/246033002/diff/140001/content/renderer/media/...
content/renderer/media/crypto/key_systems.cc:270: SupportedCodecs
supported_codecs) const {
nit: This might be easier to follow if this was key_system_supported_codecs.
It's long, but clearer.

https://codereview.chromium.org/246033002/diff/140001/content/renderer/media/...
content/renderer/media/crypto/key_systems.cc:276: SupportedCodecMask
container_mask = container_iter->second;
container_CODEC_mask?

[xhwang, 2014-04-23 23:30:18]

comments addressed

[xhwang, 2014-04-23 23:30:37]

https://codereview.chromium.org/246033002/diff/140001/content/public/common/e...
File content/public/common/encrypted_media_codecs.h (right):

https://codereview.chromium.org/246033002/diff/140001/content/public/common/e...
content/public/common/encrypted_media_codecs.h:29: // Defines bitmask values
used to specify supported codecs.
On 2014/04/23 21:07:20, ddorwin wrote:
> The comment should be at line 10, right?

Done.

https://codereview.chromium.org/246033002/diff/140001/content/renderer/media/...
File content/renderer/media/crypto/key_systems.cc (right):

https://codereview.chromium.org/246033002/diff/140001/content/renderer/media/...
content/renderer/media/crypto/key_systems.cc:251: // same as "video/foo".
Convert the |container| to achieve this.
On 2014/04/23 21:07:20, ddorwin wrote:
> Should we have a TODO to replace this with real checks against supported
> initDataTypes combined with what the Chrome demuxers support?

Done.

https://codereview.chromium.org/246033002/diff/140001/content/renderer/media/...
content/renderer/media/crypto/key_systems.cc:253: if (container.find("audio/")
== 0)
On 2014/04/23 21:07:20, ddorwin wrote:
> This would break down for audio/mpeg, but we don't need to worry about that.

yeah, this is a hack :)

https://codereview.chromium.org/246033002/diff/140001/content/renderer/media/...
content/renderer/media/crypto/key_systems.cc:270: SupportedCodecs
supported_codecs) const {
On 2014/04/23 21:07:20, ddorwin wrote:
> nit: This might be easier to follow if this was key_system_supported_codecs.
> It's long, but clearer.

Done.

https://codereview.chromium.org/246033002/diff/140001/content/renderer/media/...
content/renderer/media/crypto/key_systems.cc:276: SupportedCodecMask
container_mask = container_iter->second;
On 2014/04/23 21:07:20, ddorwin wrote:
> container_CODEC_mask?

Done.

[xhwang, 2014-04-23 23:34:08]

boliu/jam/dcheng:

This CL further simplifies the key system related code. Please do OWNERS review:

boliu@: android_webview/renderer/aw_key_systems.cc
jam@: content/public/common/encrypted_media_codecs.h
dcheng@: chrome/common/encrypted_media_messages_android.h

Thanks,
xhwang

[xhwang, 2014-04-23 23:39:30]

On 2014/04/23 23:34:08, xhwang wrote:
> boliu/jam/dcheng:
> 
> This CL further simplifies the key system related code. Please do OWNERS
review:
> 
> boliu@: android_webview/renderer/aw_key_systems.cc
> jam@: content/public/common/encrypted_media_codecs.h
> dcheng@: chrome/common/encrypted_media_messages_android.h
> 
> Thanks,
> xhwang

jam@: Please OWNERS review content/public/renderer/key_system_info.h as well.

[xhwang, 2014-04-23 23:49:43]

rebase only

[boliu, 2014-04-24 00:45:22]

android_webview lgtm

[jam, 2014-04-24 16:21:06]

lgtm with comment

https://codereview.chromium.org/246033002/diff/190001/content/public/common/e...
File content/public/common/encrypted_media_codecs.h (right):

https://codereview.chromium.org/246033002/diff/190001/content/public/common/e...
content/public/common/encrypted_media_codecs.h:13: enum SupportedCodecMask {
see the convention for enums in the content api
http://www.chromium.org/developers/content-module/content-api

this should be
enum EncryptedMediaCodec {
  ENCRYPTED_MEDIA_CODEC_NONE,
  ENCRYPTED_MEDIA_CODE_WEBM_VORBIS,
...

[dcheng, 2014-04-24 16:28:36]

lgtm

[xhwang, 2014-04-24 17:08:20]

ddorwin: Please take another look at the new enum names. Thanks!

https://codereview.chromium.org/246033002/diff/190001/content/public/common/e...
File content/public/common/encrypted_media_codecs.h (right):

https://codereview.chromium.org/246033002/diff/190001/content/public/common/e...
content/public/common/encrypted_media_codecs.h:13: enum SupportedCodecMask {
On 2014/04/24 16:21:06, jam wrote:
> see the convention for enums in the content api
> http://www.chromium.org/developers/content-module/content-api
> 
> this should be
> enum EncryptedMediaCodec {
>   ENCRYPTED_MEDIA_CODEC_NONE,
>   ENCRYPTED_MEDIA_CODE_WEBM_VORBIS,
> ...

Done.

[ddorwin, 2014-04-24 18:27:25]

encrypted_media_codecs.h changes LGTM

[xhwang, 2014-04-24 18:50:15]

The CQ bit was checked by xhwang@chromium.org

[jam, 2014-04-24 19:34:12]

https://codereview.chromium.org/246033002/diff/230001/content/public/common/e...
File content/public/common/encrypted_media_codecs.h (right):

https://codereview.chromium.org/246033002/diff/230001/content/public/common/e...
content/public/common/encrypted_media_codecs.h:1: // Copyright 2014 The Chromium
Authors. All rights reserved.
if you rename the enum to EmeCode, then this file should be eme_codec.h

[jam, 2014-04-24 19:34:20]

The CQ bit was unchecked by jam@chromium.org

[xhwang, 2014-04-24 20:04:29]

https://codereview.chromium.org/246033002/diff/230001/content/public/common/e...
File content/public/common/encrypted_media_codecs.h (right):

https://codereview.chromium.org/246033002/diff/230001/content/public/common/e...
content/public/common/encrypted_media_codecs.h:1: // Copyright 2014 The Chromium
Authors. All rights reserved.
On 2014/04/24 19:34:15, jam wrote:
> if you rename the enum to EmeCode, then this file should be eme_codec.h

Got it. Thanks for catching that. Done.

[xhwang, 2014-04-24 20:04:40]

The CQ bit was checked by xhwang@chromium.org

[commit-bot: I haz the power, 2014-04-24 21:46:40]

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

[commit-bot: I haz the power, 2014-04-24 23:09:31]

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

[commit-bot: I haz the power, 2014-04-24 23:09:32]

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

[xhwang, 2014-04-24 23:11:29]

The CQ bit was checked by xhwang@chromium.org

[commit-bot: I haz the power, 2014-04-24 23:18:32]

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

[commit-bot: I haz the power, 2014-04-24 23:53:00]

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

[commit-bot: I haz the power, 2014-04-24 23:53:01]

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

[xhwang, 2014-04-25 00:36:42]

The CQ bit was checked by xhwang@chromium.org

[commit-bot: I haz the power, 2014-04-25 00:37:10]

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

[commit-bot: I haz the power, 2014-04-25 07:34:48]

Change committed as 266148