Refactor net::MimeUtil media code to return probably for codecs the platform likely supports.

Refactor net::MimeUtil media code to return "probably" for codecs the platform likely supports.

This patch amends the behaviour set for mp4 mime types
Now browser returns "probably" for codecs which are confirmed to be shipped with Chrome. 

CodecPrameter| Before| Now 
-------------------------------- 
avc1.42E0xx -| maybe | probably 
avc1.4D40xx -| maybe | probably (non-Android) 
avc1.6400xx -| maybe | probably (non-Android) 
avc3.xxxxxx -| maybe | probably 
mp4a.6B -----| maybe | probably 
mp4a.69 -----| maybe | probably 
mp4a.67 -----| maybe | probably (non-Android) 
mp4a.40.2 ---| maybe | probably 
mp4a.40.5 ---| maybe | probably 

BUG=388317
TEST=MediaCanPlayTypeTest.*

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

[acolwell GONE FROM CHROMIUM, 2014-07-26 00:31:47]

Since https://codereview.chromium.org/401523002/ turned into a "move all the
mime things", I was wondering it would be ok for me to land this in net/ before
the move as an alternative to https://codereview.chromium.org/336213011/ . I
think this change makes the media code a little easier to follow and fixes Bug
388317 which is negatively affecting actual developers we are trying to
collaborate with.

I took all the test cases from https://codereview.chromium.org/336213011/ to
make sure that my changes have the same outcomes as the original patch.

I still feel like it is important to move the mimetype code out of net/, but I
fear that moving all that code will be tricky and I don't want to block a
critical fix on that effort.

[acolwell GONE FROM CHROMIUM, 2014-07-26 00:58:37]

Fix VP9 support on Android condition.

[Ryan Sleevi, 2014-07-26 02:15:55]

https://codereview.chromium.org/422573005/diff/60001/net/base/mime_util.cc
File net/base/mime_util.cc (right):

https://codereview.chromium.org/422573005/diff/60001/net/base/mime_util.cc#ne...
net/base/mime_util.cc:119: typedef std::pair<Codec, bool> StringToCodecEntry;
Unnamed tuples are hard to grok. What's the bool mean? is_ambiguous?

https://codereview.chromium.org/422573005/diff/60001/net/base/mime_util.cc#ne...
net/base/mime_util.cc:523: static const char kProprietaryCodecsExpression[] =
Is this really kProprietaryVideoCodecsExpression?

https://codereview.chromium.org/422573005/diff/60001/net/base/mime_util.cc#ne...
net/base/mime_util.cc:547: const char* codec_id;
const char* const? Not because it tangibly alters things, but it matches the
preferred idiom when you can't use const char foo[]

https://codereview.chromium.org/422573005/diff/60001/net/base/mime_util.cc#ne...
net/base/mime_util.cc:602: !IsCodecSupported(codec))
Do the IsCodecSupported() check before the find, since IsCodecSupported() is
just a constant-time jump-table?

https://codereview.chromium.org/422573005/diff/60001/net/base/mime_util.cc#ne...
net/base/mime_util.cc:657: StringToCodecEntry(kUnambiguousCodecIDs[i].codec,
false);
Tripped me up, because I thought this was a function call at first - especially
given the similarity to StringToCodec

This isn't really a "StringToCodecEntry", it's just a "CodecEntry" (the StringTo
is part of the map, aka the string_to_codec_map)

https://codereview.chromium.org/422573005/diff/60001/net/base/mime_util.cc#ne...
net/base/mime_util.cc:677: CHECK(!is_ambiguous);
Why CHECK? Isn't this really a DCHECK() situation? That is, it's only an issue
if someone munges the tables and does a custom compile, oui?

https://codereview.chromium.org/422573005/diff/60001/net/base/mime_util.cc#ne...
net/base/mime_util.cc:891: return IsDefaultCodecSupported(mime_type) ?
IsSupported : MayBeSupported;
Is this correct? I'm surprised that a mimetype that would "expect" to get a
codecs parameter would fall back to having a default if it was absent.

Or is that the 'bug' w/r/t MP3?

(It makes since on 884, but this surprised me here)

https://codereview.chromium.org/422573005/diff/60001/net/base/mime_util.cc#ne...
net/base/mime_util.cc:932: codec_id.size() != 11) {
Length check (constant) before the variable check (StartsWithASCII)

https://codereview.chromium.org/422573005/diff/60001/net/base/mime_util.cc#ne...
net/base/mime_util.cc:950: *is_ambiguous = true;
Seems redundant with line 937-938.

937-938 should be nuked entirely, right?

https://codereview.chromium.org/422573005/diff/60001/net/base/mime_util.cc#ne...
net/base/mime_util.cc:969: return ParseH264CodecID(codec_id, codec,
is_ambiguous);
Need a comment here explaining.

[acolwell GONE FROM CHROMIUM, 2014-07-28 17:31:43]

https://codereview.chromium.org/422573005/diff/60001/net/base/mime_util.cc
File net/base/mime_util.cc (right):

https://codereview.chromium.org/422573005/diff/60001/net/base/mime_util.cc#ne...
net/base/mime_util.cc:119: typedef std::pair<Codec, bool> StringToCodecEntry;
On 2014/07/26 02:15:54, Ryan Sleevi wrote:
> Unnamed tuples are hard to grok. What's the bool mean? is_ambiguous?

Done. Changed to a struct.

https://codereview.chromium.org/422573005/diff/60001/net/base/mime_util.cc#ne...
net/base/mime_util.cc:523: static const char kProprietaryCodecsExpression[] =
On 2014/07/26 02:15:55, Ryan Sleevi wrote:
> Is this really kProprietaryVideoCodecsExpression?
For all intents and purposes yes. Techincally these apply to application/ URLs
too but I don't think that is a big deal. I've added Video to the name. I've
also s/Proprietary/MP4/ since these strings are only valid for the MP4
mimetypes.

https://codereview.chromium.org/422573005/diff/60001/net/base/mime_util.cc#ne...
net/base/mime_util.cc:547: const char* codec_id;
On 2014/07/26 02:15:55, Ryan Sleevi wrote:
> const char* const? Not because it tangibly alters things, but it matches the
> preferred idiom when you can't use const char foo[]

Done.

https://codereview.chromium.org/422573005/diff/60001/net/base/mime_util.cc#ne...
net/base/mime_util.cc:602: !IsCodecSupported(codec))
On 2014/07/26 02:15:55, Ryan Sleevi wrote:
> Do the IsCodecSupported() check before the find, since IsCodecSupported() is
> just a constant-time jump-table?

Done.

https://codereview.chromium.org/422573005/diff/60001/net/base/mime_util.cc#ne...
net/base/mime_util.cc:657: StringToCodecEntry(kUnambiguousCodecIDs[i].codec,
false);
On 2014/07/26 02:15:54, Ryan Sleevi wrote:
> Tripped me up, because I thought this was a function call at first -
especially
> given the similarity to StringToCodec
> 
> This isn't really a "StringToCodecEntry", it's just a "CodecEntry" (the
StringTo
> is part of the map, aka the string_to_codec_map)
Renamed the class to CodecEntry.

https://codereview.chromium.org/422573005/diff/60001/net/base/mime_util.cc#ne...
net/base/mime_util.cc:677: CHECK(!is_ambiguous);
On 2014/07/26 02:15:55, Ryan Sleevi wrote:
> Why CHECK? Isn't this really a DCHECK() situation? That is, it's only an issue
> if someone munges the tables and does a custom compile, oui?
Converted to a DCHECK.

https://codereview.chromium.org/422573005/diff/60001/net/base/mime_util.cc#ne...
net/base/mime_util.cc:891: return IsDefaultCodecSupported(mime_type) ?
IsSupported : MayBeSupported;
On 2014/07/26 02:15:55, Ryan Sleevi wrote:
> Is this correct? I'm surprised that a mimetype that would "expect" to get a
> codecs parameter would fall back to having a default if it was absent.
> 
> Or is that the 'bug' w/r/t MP3?
> 
> (It makes since on 884, but this surprised me here)

Yes this is to deal with the MP3 situation where either audio/mpeg or
audio/mpeg;codecs="mp3" are allowed. I reworked the logic here to make things a
little easier to follow and actually fixes a bug where MayBeSupported would have
been returned even if MP3 was not actually supported.

https://codereview.chromium.org/422573005/diff/60001/net/base/mime_util.cc#ne...
net/base/mime_util.cc:932: codec_id.size() != 11) {
On 2014/07/26 02:15:54, Ryan Sleevi wrote:
> Length check (constant) before the variable check (StartsWithASCII)

Done.

https://codereview.chromium.org/422573005/diff/60001/net/base/mime_util.cc#ne...
net/base/mime_util.cc:950: *is_ambiguous = true;
On 2014/07/26 02:15:55, Ryan Sleevi wrote:
> Seems redundant with line 937-938.
> 
> 937-938 should be nuked entirely, right?
oops. Yes. I've removed those lines.

https://codereview.chromium.org/422573005/diff/60001/net/base/mime_util.cc#ne...
net/base/mime_util.cc:969: return ParseH264CodecID(codec_id, codec,
is_ambiguous);
On 2014/07/26 02:15:55, Ryan Sleevi wrote:
> Need a comment here explaining.

Done.

[Ryan Sleevi, 2014-07-29 01:14:48]

LGTM. Quality cleanup.

https://codereview.chromium.org/422573005/diff/80001/net/base/mime_util.cc
File net/base/mime_util.cc (right):

https://codereview.chromium.org/422573005/diff/80001/net/base/mime_util.cc#ne...
net/base/mime_util.cc:125: typedef  std::map<std::string, CodecEntry>
StringToCodecMappings;
nit: delete space

https://codereview.chromium.org/422573005/diff/80001/net/base/mime_util.cc#ne...
net/base/mime_util.cc:967: 
Could move line 955 down here. For SPEED! :D

[acolwell GONE FROM CHROMIUM, 2014-07-29 01:33:26]

Thanks for the review.

https://codereview.chromium.org/422573005/diff/80001/net/base/mime_util.cc
File net/base/mime_util.cc (right):

https://codereview.chromium.org/422573005/diff/80001/net/base/mime_util.cc#ne...
net/base/mime_util.cc:125: typedef  std::map<std::string, CodecEntry>
StringToCodecMappings;
On 2014/07/29 01:14:48, Ryan Sleevi wrote:
> nit: delete space

Done.

https://codereview.chromium.org/422573005/diff/80001/net/base/mime_util.cc#ne...
net/base/mime_util.cc:967: 
On 2014/07/29 01:14:48, Ryan Sleevi wrote:
> Could move line 955 down here. For SPEED! :D

Done.

[scherkus (not reviewing), 2014-07-29 02:05:10]

silly style nits

lgtm -- thanks for tackling this!

https://codereview.chromium.org/422573005/diff/100001/net/base/mime_util.cc
File net/base/mime_util.cc (right):

https://codereview.chromium.org/422573005/diff/100001/net/base/mime_util.cc#n...
net/base/mime_util.cc:156: // Returns true if |codec| is supported by the
platform.
nit: looks like rest of the code here keeps a blank line before // comment
blocks

https://codereview.chromium.org/422573005/diff/100001/net/base/mime_util.cc#n...
net/base/mime_util.cc:485: // H.264 Main and High profiles are not supported on
Android
period

https://codereview.chromium.org/422573005/diff/100001/net/base/mime_util.cc#n...
net/base/mime_util.cc:1041: return false;
de-indent by 2

[acolwell GONE FROM CHROMIUM, 2014-07-29 16:26:17]

https://codereview.chromium.org/422573005/diff/100001/net/base/mime_util.cc
File net/base/mime_util.cc (right):

https://codereview.chromium.org/422573005/diff/100001/net/base/mime_util.cc#n...
net/base/mime_util.cc:156: // Returns true if |codec| is supported by the
platform.
On 2014/07/29 02:05:09, scherkus wrote:
> nit: looks like rest of the code here keeps a blank line before // comment
> blocks

Done.

https://codereview.chromium.org/422573005/diff/100001/net/base/mime_util.cc#n...
net/base/mime_util.cc:485: // H.264 Main and High profiles are not supported on
Android
On 2014/07/29 02:05:09, scherkus wrote:
> period

Done.

https://codereview.chromium.org/422573005/diff/100001/net/base/mime_util.cc#n...
net/base/mime_util.cc:1041: return false;
On 2014/07/29 02:05:09, scherkus wrote:
> de-indent by 2

Done.

[acolwell GONE FROM CHROMIUM, 2014-07-29 16:26:21]

The CQ bit was checked by acolwell@chromium.org

[commit-bot: I haz the power, 2014-07-29 16:27:07]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/acolwell@chromium.org/422573005/120001

[commit-bot: I haz the power, 2014-07-29 23:35:59]

Change committed as 286333