Fix: Changing canPlayType behaviour for MP4 containers

Fix: Changing canPlayType behaviour for MP4 containers

This patch amends the behaviour set for mp4 mime types in r277386.
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
mp4a.40.29 --|  ""    | probably

BUG=388317, 53193

[amogh.bihani, 2014-06-30 11:17:07]

Apologies for the delay. I misunderstood the comment in the bug. I was waiting
for the patch to be reverted. After going through the comment again, I
understood that it will be reverted only in M37 branch.

PTAL.

[acolwell GONE FROM CHROMIUM, 2014-06-30 20:00:07]

https://codereview.chromium.org/336213011/diff/30001/content/browser/media/me...
File content/browser/media/media_canplaytype_browsertest.cc (right):

https://codereview.chromium.org/336213011/diff/30001/content/browser/media/me...
content/browser/media/media_canplaytype_browsertest.cc:903:
EXPECT_EQ(maybeCanPlayHLS, CanPlay("'application/vnd.apple.mpegurl'"));
There appears to be a fair amount of duplication in this method. It seems like
you could at least halve the amount of code in this function by using a helper
function or loop.

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

https://codereview.chromium.org/336213011/diff/30001/net/base/mime_util.cc#ne...
net/base/mime_util.cc:124: // A map of MP4 mime_types which expect codecs with
profile parameter and
nit: I think the comment needs updating since the name doesn't appear to be mp4
specific now.

https://codereview.chromium.org/336213011/diff/30001/net/base/mime_util.cc#ne...
net/base/mime_util.cc:478: //   mp4a.40.29  - MPEG-4 HE-AACv2
This should be added in a different CL. This is a new feature and requires other
changes as well.

https://codereview.chromium.org/336213011/diff/30001/net/base/mime_util.cc#ne...
net/base/mime_util.cc:485: // only them, just like avc1.
avc3 is the same as avc1. The only difference is that SPS & PPS are inline. I
would expect avc3 support to be the same as avc1 support.

https://codereview.chromium.org/336213011/diff/30001/net/base/mime_util.cc#ne...
net/base/mime_util.cc:487:
"mp4a.6B,mp4a.69,mp4a.67,mp4a.40.2,mp4a.40.5,mp4a.40.29";
remove mp4a.40.29 here and below. It should be added in a separate CL.

https://codereview.chromium.org/336213011/diff/30001/net/base/mime_util.cc#ne...
net/base/mime_util.cc:527: "mp4a.6?,mp4a.40";
Why do we want mp4a.6? ? 66,68 are the only 2 other valid audio values aside
from the ones we us above. Everything else is video. I think we should just
explicitly mention them and reject the other values since they would be invalid.

https://codereview.chromium.org/336213011/diff/30001/net/base/mime_util.cc#ne...
net/base/mime_util.cc:672: codecs.insert(codecs.end(), it->second.begin(),
it->second.end());
Don't you need to protect this with a (it != strict_format_map_.end()) check or
at least have a DCHECK(it != strict_format_map_.end())?

https://codereview.chromium.org/336213011/diff/30001/net/base/mime_util.cc#ne...
net/base/mime_util.cc:889: if (codecs_map_.size() ==
This looks REALLY brittle. Why can't you just stick with the original code?

[amogh.bihani, 2014-07-01 12:53:57]

PTAL.
Comments are in PS#3

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

https://codereview.chromium.org/336213011/diff/30001/net/base/mime_util.cc#ne...
net/base/mime_util.cc:478: //   mp4a.40.29  - MPEG-4 HE-AACv2
On 2014/06/30 20:00:07, acolwell wrote:
> This should be added in a different CL. This is a new feature and requires
other
> changes as well.

Oh Ok! Becuase of what was reported in Bug 387604, I thought that this codec
just needs an entry here.
I'll remove it from this CL.

https://codereview.chromium.org/336213011/diff/30001/net/base/mime_util.cc#ne...
net/base/mime_util.cc:889: if (codecs_map_.size() ==
On 2014/06/30 20:00:06, acolwell wrote:
> This looks REALLY brittle. Why can't you just stick with the original code?

Now we do not have exact strings, we have expressions with wifdcards('?') so now
codecs_map_ is a vector and not a hash set. Vector does not erase by values. :(

I have changed this and used std::remove for this purpose.

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

lgtm % comments

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

https://codereview.chromium.org/336213011/diff/30001/net/base/mime_util.cc#ne...
net/base/mime_util.cc:478: //   mp4a.40.29  - MPEG-4 HE-AACv2
On 2014/07/01 12:53:57, amogh.bihani wrote:
> On 2014/06/30 20:00:07, acolwell wrote:
> > This should be added in a different CL. This is a new feature and requires
> other
> > changes as well.
> 
> Oh Ok! Becuase of what was reported in Bug 387604, I thought that this codec
> just needs an entry here.
> I'll remove it from this CL.

It will eventually, but that should be as part of a fix for Bug 315566.

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

https://codereview.chromium.org/336213011/diff/110001/net/base/mime_util.cc#n...
net/base/mime_util.cc:124: // A map of mime_types, which expect codecs which are
RFC compliant but are
nit: s/but are/but/

https://codereview.chromium.org/336213011/diff/110001/net/base/mime_util.cc#n...
net/base/mime_util.cc:330: #if !defined(OS_ANDROID)  // Android doesn't support
Ogg Theora.
I think you can drop this #if since IsCodecSupportedOnAndroid() prevents ths
codec from being included.

[amogh.bihani, 2014-07-02 03:13:25]

+r: Ryan

PTAL.

[amogh.bihani, 2014-07-04 01:29:54]

Ryan: Ping!

[amogh.bihani, 2014-07-08 03:32:49]

On 2014/07/04 01:29:54, amogh.bihani wrote:
> Ryan: Ping!

Ryan: Please review :)

[Ryan Sleevi, 2014-07-09 23:56:13]

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

https://codereview.chromium.org/336213011/diff/170001/net/base/mime_util.cc#n...
net/base/mime_util.cc:288: "video/ogg",
This is no longer consistent with the comment on line 283, as evidenced by line
455.

https://codereview.chromium.org/336213011/diff/170001/net/base/mime_util.cc#n...
net/base/mime_util.cc:320: // This set of codecs is supported by all variations
of Chromium.
This comment is no longer true for line 328.

https://codereview.chromium.org/336213011/diff/170001/net/base/mime_util.cc#n...
net/base/mime_util.cc:512: // cannot be sure whether the platform supports them
or not
grammar rewrite:

// The following is the list of codecs that are RFC 6381 compliant, but
// which support for varies by platform and cannot be determined
// beforehand.

(Context: "browser" concepts should not exist in //net)

https://codereview.chromium.org/336213011/diff/170001/net/base/mime_util.cc#n...
net/base/mime_util.cc:664: // string in reply.
This comment needs rewording, but unfortunately, it's confusing enough that I'm
not sure what to suggest here, as I'm not clear why you merge the two.

[amogh.bihani, 2014-07-10 04:02:49]

Thanks for the review :)
Comments in PS#10

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

https://codereview.chromium.org/336213011/diff/170001/net/base/mime_util.cc#n...
net/base/mime_util.cc:664: // string in reply.
On 2014/07/09 23:56:13, Ryan Sleevi wrote:
> This comment needs rewording, but unfortunately, it's confusing enough that
I'm
> not sure what to suggest here, as I'm not clear why you merge the two.

Here is the scenario... "type=video/mp4; codecs= avc1.64001F, mp4a.40"
in this case, avc1.64001F is in the conclusive list but mp4a.40 is in
inconclusive list, so if we do not merge the two list, we will give "" in reply
instead of "maybe".

Another aproach could have been that I rewrite all the entries of conclusive
list in inconclusive list... but then to make it robust we need to have a check
that - if in the future any new entry is added in conclusive list, then it has
to be added in both.

[Ryan Sleevi, 2014-07-10 20:58:44]

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

https://codereview.chromium.org/336213011/diff/170001/net/base/mime_util.cc#n...
net/base/mime_util.cc:664: // string in reply.
On 2014/07/10 04:02:49, amogh.bihani wrote:
> On 2014/07/09 23:56:13, Ryan Sleevi wrote:
> > This comment needs rewording, but unfortunately, it's confusing enough that
> I'm
> > not sure what to suggest here, as I'm not clear why you merge the two.
> 
> Here is the scenario... "type=video/mp4; codecs= avc1.64001F, mp4a.40"
> in this case, avc1.64001F is in the conclusive list but mp4a.40 is in
> inconclusive list, so if we do not merge the two list, we will give "" in
reply
> instead of "maybe".
> 
> Another aproach could have been that I rewrite all the entries of conclusive
> list in inconclusive list... but then to make it robust we need to have a
check
> that - if in the future any new entry is added in conclusive list, then it has
> to be added in both.

Both of the solutions you've describe involve significant copying.

What's not clear is why you can't break apart the codecs parameter, and test if
it's in either list, and make an appropriate decision from that?

This accomplishes the same thing (AFAICT), but without having to copy these
strings around and reparse them. Have I missed something?

[amogh.bihani, 2014-07-11 09:03:54]

Thanks for the review :)

I have some doubts. Comment is in PS#14. PTAL.

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

https://codereview.chromium.org/336213011/diff/250001/net/base/mime_util.cc#n...
net/base/mime_util.cc:866: MimeExpressionMappings all_supported_codecs =
it_expression_map->second;
Now the coping part has moved here :(. I am sorry if I have misunderstood the
comment, from what I could get I can say that at some point we will have to
merge two vectors as AreSupportedCodecs() accepts vectors. If we change that
method to accept string instead of vector then we will also have to make changes
at other places (like line 822-825, line 859 etc) and call it in a loop.

Frankly putting it I am not pretty happy with what I have done here as I have
introduced another vector which stores the same things again. Earlier we had to
store conclusive and (conclusive + inconclusive), now we store conclusive,
inconclusive and (conclusive + inconclusive).However, if the compiler optimizes
the heap memory allocation, then this might be better (but I am not sure on that
part).

[Ryan Sleevi, 2014-07-11 18:51:17]

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

https://codereview.chromium.org/336213011/diff/250001/net/base/mime_util.cc#n...
net/base/mime_util.cc:866: MimeExpressionMappings all_supported_codecs =
it_expression_map->second;
On 2014/07/11 09:03:53, amogh.bihani wrote:
> Now the coping part has moved here :(. I am sorry if I have misunderstood the
> comment, from what I could get I can say that at some point we will have to
> merge two vectors as AreSupportedCodecs() accepts vectors. If we change that
> method to accept string instead of vector then we will also have to make
changes
> at other places (like line 822-825, line 859 etc) and call it in a loop.
> 
> Frankly putting it I am not pretty happy with what I have done here as I have
> introduced another vector which stores the same things again. Earlier we had
to
> store conclusive and (conclusive + inconclusive), now we store conclusive,
> inconclusive and (conclusive + inconclusive).However, if the compiler
optimizes
> the heap memory allocation, then this might be better (but I am not sure on
that
> part).

I'm sorry, I'm still not clear why you need to store (conclusive + inconclusive)

My request was that you have (conclusive) and (inconclusive). If you need to
check for the set of those two, you do so within the function, not by making a
copy.

Why can you not make a version of AreSupportedCodecs take two factors -
conclusive and inconclusive? It's an internal function.

[amogh.bihani, 2014-07-14 09:10:35]

Thanks!
I have made the changes. PTAL.

[Ryan Sleevi, 2014-07-14 18:25:10]

We don't do function overloading like this. See
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...

I don't think what's being asked is unreasonable, just please pay close
attention to code correctness.

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

https://codereview.chromium.org/336213011/diff/270001/net/base/mime_util.cc#n...
net/base/mime_util.cc:895: AreSupportedCodecs(it_strict_map->second,
BUG: it_strict_map->second can == strict_format_map_.end() here.

[amogh.bihani, 2014-07-15 08:35:32]

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

https://codereview.chromium.org/336213011/diff/270001/net/base/mime_util.cc#n...
net/base/mime_util.cc:895: AreSupportedCodecs(it_strict_map->second,
On 2014/07/14 18:25:09, Ryan Sleevi wrote:
> BUG: it_strict_map->second can == strict_format_map_.end() here. 

Done.
Ah! My bad. Sorry.

[amogh.bihani, 2014-07-18 03:51:24]

Ping!

[Ryan Sleevi, 2014-07-18 20:08:19]

This is pending the move of this code to media/

[acolwell GONE FROM CHROMIUM, 2014-07-18 20:58:00]

On 2014/07/18 20:08:19, Ryan Sleevi wrote:
> This is pending the move of this code to media/

Yes. I'm also experimenting with significantly simplifying the code once the
move lands. Amogh, I'll CC you when I post the CL.

[amogh.bihani, 2014-07-30 03:15:59]

Closing this as the bug has been solved by
https://codereview.chromium.org/422573005/