EME: Ensure that registered codecs match the MIME type.

EME: Ensure that registered codecs match the MIME type.

Prefixed EME used a single content type string and thus needed to allow audio
codecs with 'video/' MIME types. This is not true for unprefixed EME, so we can
be more strict now that prefixed has been removed.

This also renames some things to reflect this, reorders some declarations and
calls to enable the enforcement, and removes some unreferenced funcitons.

BUG=249976
Committed: https://crrev.com/d09e12187dfb54c3c49d937b8fafc4df09f5227d
Cr-Commit-Position: refs/heads/master@{#377700}

[ddorwin, 2016-02-25 00:10:47]

ddorwin@chromium.org changed reviewers:
+ sandersd@chromium.org, xhwang@chromium.org

[ddorwin, 2016-02-25 00:10:48]

https://codereview.chromium.org/1730383004/diff/1/media/base/key_systems.cc
File media/base/key_systems.cc (right):

https://codereview.chromium.org/1730383004/diff/1/media/base/key_systems.cc#n...
media/base/key_systems.cc:38: static NamedCodec kMimeTypeToCodecMasks[] = {
sandersd: I think this CL may be what you meant in your TODO, but please let me
know if there was more to it.

https://codereview.chromium.org/1730383004/diff/1/media/base/key_systems.cc#n...
media/base/key_systems.cc:244: // TODO(sandersd): Separate container enum from
codec mask value.
sandersd: If you have more details on the intent of this TODO, I can add them.

https://codereview.chromium.org/1730383004/diff/1/media/base/key_systems_unit...
File media/base/key_systems_unittest.cc (right):

https://codereview.chromium.org/1730383004/diff/1/media/base/key_systems_unit...
media/base/key_systems_unittest.cc:98: AddMimeTypeCodecMask("video/foo",
TEST_CODEC_FOO_VIDEO_ALL);
Moved, renamed, and added "_VIDEO" to the parameter.

[xhwang, 2016-02-25 19:48:00]

lgtm % nits

https://codereview.chromium.org/1730383004/diff/1/media/base/key_systems.cc
File media/base/key_systems.cc (right):

https://codereview.chromium.org/1730383004/diff/1/media/base/key_systems.cc#n...
media/base/key_systems.cc:38: static NamedCodec kMimeTypeToCodecMasks[] = {
Mark these arrays const?

https://codereview.chromium.org/1730383004/diff/1/media/base/key_systems.cc#n...
media/base/key_systems.cc:239: typedef base::hash_map<std::string,
SupportedCodecs> ContainerCodecsMap;
Should this be renamed to MimeTypeCodecMap?

https://codereview.chromium.org/1730383004/diff/1/media/base/key_systems.cc#n...
media/base/key_systems.cc:453: SupportedCodecs codecs_mask) const {
check thread as well?

[ddorwin, 2016-02-25 20:35:50]

sandersd: Any comments or replies to my comments in PS1?

https://codereview.chromium.org/1730383004/diff/1/media/base/key_systems.cc
File media/base/key_systems.cc (right):

https://codereview.chromium.org/1730383004/diff/1/media/base/key_systems.cc#n...
media/base/key_systems.cc:38: static NamedCodec kMimeTypeToCodecMasks[] = {
On 2016/02/25 19:48:00, xhwang wrote:
> Mark these arrays const?

Done.

https://codereview.chromium.org/1730383004/diff/1/media/base/key_systems.cc#n...
media/base/key_systems.cc:239: typedef base::hash_map<std::string,
SupportedCodecs> ContainerCodecsMap;
On 2016/02/25 19:48:00, xhwang wrote:
> Should this be renamed to MimeTypeCodecMap?

Done but with Codec*s*.

https://codereview.chromium.org/1730383004/diff/1/media/base/key_systems.cc#n...
media/base/key_systems.cc:453: SupportedCodecs codecs_mask) const {
On 2016/02/25 19:48:00, xhwang wrote:
> check thread as well?

Done.

[sandersd (OOO until July 31), 2016-02-25 20:47:44]

https://codereview.chromium.org/1730383004/diff/1/media/base/key_systems.cc
File media/base/key_systems.cc (right):

https://codereview.chromium.org/1730383004/diff/1/media/base/key_systems.cc#n...
media/base/key_systems.cc:38: static NamedCodec kMimeTypeToCodecMasks[] = {
On 2016/02/25 00:10:48, ddorwin wrote:
> sandersd: I think this CL may be what you meant in your TODO, but please let
me
> know if there was more to it.

Yes, the TODO was about exactly this.

https://codereview.chromium.org/1730383004/diff/1/media/base/key_systems.cc#n...
media/base/key_systems.cc:244: // TODO(sandersd): Separate container enum from
codec mask value.
On 2016/02/25 00:10:48, ddorwin wrote:
> sandersd: If you have more details on the intent of this TODO, I can add them.

I believe the idea was to have a separate mime string -> enum value step, and
then enum value -> codec_mask.

I don't recall the specifics, but if there was anywhere we were handing codec
masks around, it would then make sense to pass the enum value instead (and look
up the mask whenever necessary).

[ddorwin, 2016-02-25 20:55:27]

https://codereview.chromium.org/1730383004/diff/1/media/base/key_systems.cc
File media/base/key_systems.cc (right):

https://codereview.chromium.org/1730383004/diff/1/media/base/key_systems.cc#n...
media/base/key_systems.cc:244: // TODO(sandersd): Separate container enum from
codec mask value.
On 2016/02/25 20:47:44, sandersd wrote:
> On 2016/02/25 00:10:48, ddorwin wrote:
> > sandersd: If you have more details on the intent of this TODO, I can add
them.
> 
> I believe the idea was to have a separate mime string -> enum value step, and
> then enum value -> codec_mask.
> 
> I don't recall the specifics, but if there was anywhere we were handing codec
> masks around, it would then make sense to pass the enum value instead (and
look
> up the mask whenever necessary).

Okay, I think we are only looking up the mask when necessary. The thing we could
change is to not pass a MIME type string here, which is what I suggest in line
246.

[ddorwin, 2016-02-25 20:55:39]

The CQ bit was checked by ddorwin@chromium.org

[ddorwin, 2016-02-25 20:55:39]

The patchset sent to the CQ was uploaded after l-g-t-m from xhwang@chromium.org
Link to the patchset: https://codereview.chromium.org/1730383004/#ps20001
(title: "feedback")

[commit-bot: I haz the power, 2016-02-25 20:58:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1730383004/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1730383004/20001

[ddorwin, 2016-02-25 21:28:49]

The CQ bit was checked by ddorwin@chromium.org

[ddorwin, 2016-02-25 21:28:49]

The patchset sent to the CQ was uploaded after l-g-t-m from xhwang@chromium.org
Link to the patchset: https://codereview.chromium.org/1730383004/#ps40001
(title: "Add comments to avoid the formatter combining lines.")

[commit-bot: I haz the power, 2016-02-25 21:33:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1730383004/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1730383004/40001

[sandersd (OOO until July 31), 2016-02-25 23:06:15]

Interaction with the rMKSA algorithm LGTM.

[commit-bot: I haz the power, 2016-02-25 23:20:34]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-02-25 23:22:10]

Description was changed from

==========
EME: Ensure that registered codecs match the MIME type.

Prefixed EME used a single content type string and thus needed to allow audio
codecs with 'video/' MIME types. This is not true for unprefixed EME, so we can
be more strict now that prefixed has been removed.

This also renames some things to reflect this, reorders some declarations and
calls to enable the enforcement, and removes some unreferenced funcitons.

BUG=249976
==========

to

==========
EME: Ensure that registered codecs match the MIME type.

Prefixed EME used a single content type string and thus needed to allow audio
codecs with 'video/' MIME types. This is not true for unprefixed EME, so we can
be more strict now that prefixed has been removed.

This also renames some things to reflect this, reorders some declarations and
calls to enable the enforcement, and removes some unreferenced funcitons.

BUG=249976

Committed: https://crrev.com/d09e12187dfb54c3c49d937b8fafc4df09f5227d
Cr-Commit-Position: refs/heads/master@{#377700}
==========

[commit-bot: I haz the power, 2016-02-25 23:22:11]

Patchset 3 (id:??) landed as
https://crrev.com/d09e12187dfb54c3c49d937b8fafc4df09f5227d
Cr-Commit-Position: refs/heads/master@{#377700}