Re-add AC3/EAC3 audio demuxing support

Re-add AC3/EAC3 audio demuxing support

Chrome used to have EAC3/DD+ support (crbug.com/215773),
but it was removed last year (crbug.com/334126). This CL
partially restores the old code and adds AC3 support.
This will allow AC3/EAC3 audio demuxing and can be used
for AC3/EAC3 pass-through support.

BUG=448878
Committed: https://crrev.com/063ce09474bc035a743aa565f87b7f506db8ff86
Cr-Commit-Position: refs/heads/master@{#368476}

[servolk, 2015-01-14 20:43:08]

servolk@chromium.org changed reviewers:
+ lcwu@chromium.org

[servolk, 2015-01-14 22:31:11]

servolk@chromium.org changed reviewers:
+ dalecurtis@chromium.org, gunsch@chromium.org, thakis@chromium.org,
wolenetz@chromium.org

[servolk, 2015-01-14 23:29:42]

servolk@chromium.org changed reviewers:
+ sherkus@chromium.org

[servolk, 2015-01-14 23:33:56]

servolk@chromium.org changed reviewers:
+ scherkus@chromium.org
- sherkus@chromium.org

[DaleCurtis, 2015-01-14 23:36:33]

https://codereview.chromium.org/812643005/diff/40001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/812643005/diff/40001/build/common.gypi#newcod...
build/common.gypi:1471: # Set to 1 to enable AC3/EAC3 audio demuxing
Do we need this here? I don't see any non media/ code using this.

https://codereview.chromium.org/812643005/diff/40001/media/filters/stream_par...
File media/filters/stream_parser_factory.cc (right):

https://codereview.chromium.org/812643005/diff/40001/media/filters/stream_par...
media/filters/stream_parser_factory.cc:212: || MatchPattern(codec_id,
kAC3CodecInfo2.pattern)) {
|| on above line.

https://codereview.chromium.org/812643005/diff/40001/media/filters/stream_par...
media/filters/stream_parser_factory.cc:215: || MatchPattern(codec_id,
kEAC3CodecInfo2.pattern)) {
Ditto.

[servolk, 2015-01-14 23:44:12]

https://codereview.chromium.org/812643005/diff/40001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/812643005/diff/40001/build/common.gypi#newcod...
build/common.gypi:1471: # Set to 1 to enable AC3/EAC3 audio demuxing
On 2015/01/14 23:36:33, DaleCurtis wrote:
> Do we need this here? I don't see any non media/ code using this.

Two reasons:
1. I was trying to keep gyp variables consistent, enable_ac3_eac3_audio_demuxing
is very similar in its role and scope to enable_mpeg2ts_stream_parser and that
one is defined here.
2. We will actually needs this in net/base/mime_types.cc when adding ac3/eac3 to
the list of supported mime types.
This CL is not complete yet - I still need to finish the net/base/mime_types.cc
part and trying to figure out if I can add more unit tests (the current version
only has unit tests for FFmpeg demuxer, would be nice to add some unit tests for
MSE code path). But I've decided to sent it out a bit earlier to give you guys
time to take a look at this, I didn't expect such a quick reply :)

[DaleCurtis, 2015-01-14 23:50:07]

https://codereview.chromium.org/812643005/diff/40001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/812643005/diff/40001/build/common.gypi#newcod...
build/common.gypi:1471: # Set to 1 to enable AC3/EAC3 audio demuxing
On 2015/01/14 23:44:11, servolk wrote:
> On 2015/01/14 23:36:33, DaleCurtis wrote:
> > Do we need this here? I don't see any non media/ code using this.
> 
> Two reasons:
> 1. I was trying to keep gyp variables consistent,
enable_ac3_eac3_audio_demuxing
> is very similar in its role and scope to enable_mpeg2ts_stream_parser and that
> one is defined here.
> 2. We will actually needs this in net/base/mime_types.cc when adding ac3/eac3
to
> the list of supported mime types.
> This CL is not complete yet - I still need to finish the
net/base/mime_types.cc
> part and trying to figure out if I can add more unit tests (the current
version
> only has unit tests for FFmpeg demuxer, would be nice to add some unit tests
for
> MSE code path). But I've decided to sent it out a bit earlier to give you guys
> time to take a look at this, I didn't expect such a quick reply :)

New values of this type are effectively banned, so you'll face an uphill battle
trying to add this. Each define added here is applied to every Chrome build
file. You're also missing the GN changes, both of which will require brettw@'s
approval (who's very likely going to say what I just said above).

In this case it's only for Chromecast, so you might be able to get away with it,
but I'd look closely at how you can avoid this being a global value.

I believe for the net/ stuff we want to move that into media/ (certainly the
net/ team wants us to).

[servolk, 2015-01-15 00:06:45]

https://codereview.chromium.org/812643005/diff/40001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/812643005/diff/40001/build/common.gypi#newcod...
build/common.gypi:1471: # Set to 1 to enable AC3/EAC3 audio demuxing
On 2015/01/14 23:50:07, DaleCurtis wrote:
> On 2015/01/14 23:44:11, servolk wrote:
> > On 2015/01/14 23:36:33, DaleCurtis wrote:
> > > Do we need this here? I don't see any non media/ code using this.
> > 
> > Two reasons:
> > 1. I was trying to keep gyp variables consistent,
> enable_ac3_eac3_audio_demuxing
> > is very similar in its role and scope to enable_mpeg2ts_stream_parser and
that
> > one is defined here.
> > 2. We will actually needs this in net/base/mime_types.cc when adding
ac3/eac3
> to
> > the list of supported mime types.
> > This CL is not complete yet - I still need to finish the
> net/base/mime_types.cc
> > part and trying to figure out if I can add more unit tests (the current
> version
> > only has unit tests for FFmpeg demuxer, would be nice to add some unit tests
> for
> > MSE code path). But I've decided to sent it out a bit earlier to give you
guys
> > time to take a look at this, I didn't expect such a quick reply :)
> 
> New values of this type are effectively banned, so you'll face an uphill
battle
> trying to add this. Each define added here is applied to every Chrome build
> file. You're also missing the GN changes, both of which will require brettw@'s
> approval (who's very likely going to say what I just said above).

Added the same option to media/media_options.gni for now. But I don't see where
the mpeg2ts option declared in the .gni file is being used.

> In this case it's only for Chromecast, so you might be able to get away with
it,
> but I'd look closely at how you can avoid this being a global value.

What would be a good alternative way to achieve this (ac3/eac3 demuxing behind a
gyp/gn variable, so that we could only enable it when necessary, e.g.
Chromecast)?
 
> I believe for the net/ stuff we want to move that into media/ (certainly the
> net/ team wants us to).

Yes, I understand and completely agree here, it seemed strange to me that net/
has such a hard dependency on media, I agree that this stuff has to move to
media. But at the moment the first level of mime type checks is still done
there, we have to add AC3/EAC3 mime types in there to make them usable, and then
it would be totally fine to move all this code to media/ in a separate CL,
together with all the other media mime types.

https://codereview.chromium.org/812643005/diff/40001/media/filters/stream_par...
File media/filters/stream_parser_factory.cc (right):

https://codereview.chromium.org/812643005/diff/40001/media/filters/stream_par...
media/filters/stream_parser_factory.cc:212: || MatchPattern(codec_id,
kAC3CodecInfo2.pattern)) {
On 2015/01/14 23:36:33, DaleCurtis wrote:
> || on above line. 

Done.

https://codereview.chromium.org/812643005/diff/40001/media/filters/stream_par...
media/filters/stream_parser_factory.cc:215: || MatchPattern(codec_id,
kEAC3CodecInfo2.pattern)) {
On 2015/01/14 23:36:33, DaleCurtis wrote:
> Ditto.

Done.

[servolk, 2015-01-15 00:28:34]

servolk@chromium.org changed reviewers:
+ damienv@chromium.org

[DaleCurtis, 2015-01-15 01:51:01]

https://codereview.chromium.org/812643005/diff/40001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/812643005/diff/40001/build/common.gypi#newcod...
build/common.gypi:1471: # Set to 1 to enable AC3/EAC3 audio demuxing
On 2015/01/15 00:06:45, servolk wrote:
> On 2015/01/14 23:50:07, DaleCurtis wrote:
> > On 2015/01/14 23:44:11, servolk wrote:
> > > On 2015/01/14 23:36:33, DaleCurtis wrote:
> > > > Do we need this here? I don't see any non media/ code using this.
> > > 
> > > Two reasons:
> > > 1. I was trying to keep gyp variables consistent,
> > enable_ac3_eac3_audio_demuxing
> > > is very similar in its role and scope to enable_mpeg2ts_stream_parser and
> that
> > > one is defined here.
> > > 2. We will actually needs this in net/base/mime_types.cc when adding
> ac3/eac3
> > to
> > > the list of supported mime types.
> > > This CL is not complete yet - I still need to finish the
> > net/base/mime_types.cc
> > > part and trying to figure out if I can add more unit tests (the current
> > version
> > > only has unit tests for FFmpeg demuxer, would be nice to add some unit
tests
> > for
> > > MSE code path). But I've decided to sent it out a bit earlier to give you
> guys
> > > time to take a look at this, I didn't expect such a quick reply :)
> > 
> > New values of this type are effectively banned, so you'll face an uphill
> battle
> > trying to add this. Each define added here is applied to every Chrome build
> > file. You're also missing the GN changes, both of which will require
brettw@'s
> > approval (who's very likely going to say what I just said above).
> 
> Added the same option to media/media_options.gni for now. But I don't see
where
> the mpeg2ts option declared in the .gni file is being used.
> 
> > In this case it's only for Chromecast, so you might be able to get away with
> it,
> > but I'd look closely at how you can avoid this being a global value.
> 
> What would be a good alternative way to achieve this (ac3/eac3 demuxing behind
a
> gyp/gn variable, so that we could only enable it when necessary, e.g.
> Chromecast)?

If it's only necessary in media/ and net/ I'd just duplicate the logic in both
net/ and media/ .gyp/.gn files temporarily until everything can be moved into
media/. That said, I bet net/ folk will say we should just move the code first,
they've been asking forever.

>  
> > I believe for the net/ stuff we want to move that into media/ (certainly the
> > net/ team wants us to).
> 
> Yes, I understand and completely agree here, it seemed strange to me that net/
> has such a hard dependency on media, I agree that this stuff has to move to
> media. But at the moment the first level of mime type checks is still done
> there, we have to add AC3/EAC3 mime types in there to make them usable, and
then
> it would be totally fine to move all this code to media/ in a separate CL,
> together with all the other media mime types.

[servolk, 2015-01-15 02:37:20]

https://codereview.chromium.org/812643005/diff/40001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/812643005/diff/40001/build/common.gypi#newcod...
build/common.gypi:1471: # Set to 1 to enable AC3/EAC3 audio demuxing
On 2015/01/15 01:51:01, DaleCurtis wrote:
> On 2015/01/15 00:06:45, servolk wrote:
> > On 2015/01/14 23:50:07, DaleCurtis wrote:
> > > On 2015/01/14 23:44:11, servolk wrote:
> > > > On 2015/01/14 23:36:33, DaleCurtis wrote:
> > > > > Do we need this here? I don't see any non media/ code using this.
> > > > 
> > > > Two reasons:
> > > > 1. I was trying to keep gyp variables consistent,
> > > enable_ac3_eac3_audio_demuxing
> > > > is very similar in its role and scope to enable_mpeg2ts_stream_parser
and
> > that
> > > > one is defined here.
> > > > 2. We will actually needs this in net/base/mime_types.cc when adding
> > ac3/eac3
> > > to
> > > > the list of supported mime types.
> > > > This CL is not complete yet - I still need to finish the
> > > net/base/mime_types.cc
> > > > part and trying to figure out if I can add more unit tests (the current
> > > version
> > > > only has unit tests for FFmpeg demuxer, would be nice to add some unit
> tests
> > > for
> > > > MSE code path). But I've decided to sent it out a bit earlier to give
you
> > guys
> > > > time to take a look at this, I didn't expect such a quick reply :)
> > > 
> > > New values of this type are effectively banned, so you'll face an uphill
> > battle
> > > trying to add this. Each define added here is applied to every Chrome
build
> > > file. You're also missing the GN changes, both of which will require
> brettw@'s
> > > approval (who's very likely going to say what I just said above).
> > 
> > Added the same option to media/media_options.gni for now. But I don't see
> where
> > the mpeg2ts option declared in the .gni file is being used.
> > 
> > > In this case it's only for Chromecast, so you might be able to get away
with
> > it,
> > > but I'd look closely at how you can avoid this being a global value.
> > 
> > What would be a good alternative way to achieve this (ac3/eac3 demuxing
behind
> a
> > gyp/gn variable, so that we could only enable it when necessary, e.g.
> > Chromecast)?
> 
> If it's only necessary in media/ and net/ I'd just duplicate the logic in both
> net/ and media/ .gyp/.gn files temporarily until everything can be moved into
> media/. That said, I bet net/ folk will say we should just move the code
first,
> they've been asking forever.
> 
> >  
> > > I believe for the net/ stuff we want to move that into media/ (certainly
the
> > > net/ team wants us to).
> > 
> > Yes, I understand and completely agree here, it seemed strange to me that
net/
> > has such a hard dependency on media, I agree that this stuff has to move to
> > media. But at the moment the first level of mime type checks is still done
> > there, we have to add AC3/EAC3 mime types in there to make them usable, and
> then
> > it would be totally fine to move all this code to media/ in a separate CL,
> > together with all the other media mime types.
> 

Ok, I can try to move the media mime checks from net/ to media/, but could you
give me a bit more context?
I guess I could always just try a brute-force approach, e.g. just extract all
media-related code into a separate class, name it MediaMimeUtil, move it
somewhere under media/ and use MediaMimeUtil in the net/'s
MimeUtil::IsSupportedMediaMimeType and MimeUtil::IsSupportedNonImageMimeType.
But is that what you and net/ folks had in mind?
If there has been a discussion on that matter already, could you tell me the bug
# or loop me in on that mail thread.

[lcwu1, 2015-01-15 02:41:48]

On 2015/01/15 02:37:20, servolk wrote:
> https://codereview.chromium.org/812643005/diff/40001/build/common.gypi
> File build/common.gypi (right):
> 
>
https://codereview.chromium.org/812643005/diff/40001/build/common.gypi#newcod...
> build/common.gypi:1471: # Set to 1 to enable AC3/EAC3 audio demuxing
> On 2015/01/15 01:51:01, DaleCurtis wrote:
> > On 2015/01/15 00:06:45, servolk wrote:
> > > On 2015/01/14 23:50:07, DaleCurtis wrote:
> > > > On 2015/01/14 23:44:11, servolk wrote:
> > > > > On 2015/01/14 23:36:33, DaleCurtis wrote:
> > > > > > Do we need this here? I don't see any non media/ code using this.
> > > > > 
> > > > > Two reasons:
> > > > > 1. I was trying to keep gyp variables consistent,
> > > > enable_ac3_eac3_audio_demuxing
> > > > > is very similar in its role and scope to enable_mpeg2ts_stream_parser
> and
> > > that
> > > > > one is defined here.
> > > > > 2. We will actually needs this in net/base/mime_types.cc when adding
> > > ac3/eac3
> > > > to
> > > > > the list of supported mime types.
> > > > > This CL is not complete yet - I still need to finish the
> > > > net/base/mime_types.cc
> > > > > part and trying to figure out if I can add more unit tests (the
current
> > > > version
> > > > > only has unit tests for FFmpeg demuxer, would be nice to add some unit
> > tests
> > > > for
> > > > > MSE code path). But I've decided to sent it out a bit earlier to give
> you
> > > guys
> > > > > time to take a look at this, I didn't expect such a quick reply :)
> > > > 
> > > > New values of this type are effectively banned, so you'll face an uphill
> > > battle
> > > > trying to add this. Each define added here is applied to every Chrome
> build
> > > > file. You're also missing the GN changes, both of which will require
> > brettw@'s
> > > > approval (who's very likely going to say what I just said above).
> > > 
> > > Added the same option to media/media_options.gni for now. But I don't see
> > where
> > > the mpeg2ts option declared in the .gni file is being used.
> > > 
> > > > In this case it's only for Chromecast, so you might be able to get away
> with
> > > it,
> > > > but I'd look closely at how you can avoid this being a global value.
> > > 
> > > What would be a good alternative way to achieve this (ac3/eac3 demuxing
> behind
> > a
> > > gyp/gn variable, so that we could only enable it when necessary, e.g.
> > > Chromecast)?
> > 
> > If it's only necessary in media/ and net/ I'd just duplicate the logic in
both
> > net/ and media/ .gyp/.gn files temporarily until everything can be moved
into
> > media/. That said, I bet net/ folk will say we should just move the code
> first,
> > they've been asking forever.
> > 
> > >  
> > > > I believe for the net/ stuff we want to move that into media/ (certainly
> the
> > > > net/ team wants us to).
> > > 
> > > Yes, I understand and completely agree here, it seemed strange to me that
> net/
> > > has such a hard dependency on media, I agree that this stuff has to move
to
> > > media. But at the moment the first level of mime type checks is still done
> > > there, we have to add AC3/EAC3 mime types in there to make them usable,
and
> > then
> > > it would be totally fine to move all this code to media/ in a separate CL,
> > > together with all the other media mime types.
> > 
> 
> Ok, I can try to move the media mime checks from net/ to media/, but could you
> give me a bit more context?
> I guess I could always just try a brute-force approach, e.g. just extract all
> media-related code into a separate class, name it MediaMimeUtil, move it
> somewhere under media/ and use MediaMimeUtil in the net/'s
> MimeUtil::IsSupportedMediaMimeType and MimeUtil::IsSupportedNonImageMimeType.
> But is that what you and net/ folks had in mind?
> If there has been a discussion on that matter already, could you tell me the
bug
> # or loop me in on that mail thread.

I think what Dale suggested is to, instead of modifying build/common.gypi,
introducing the new gyp variable and C define in both media.gyp and net.gyp so
that the new gyp variable won't pollute the whole Chromium source.

[servolk, 2015-01-15 02:45:26]

On 2015/01/15 02:41:48, lcwu1 wrote:
> On 2015/01/15 02:37:20, servolk wrote:
> > https://codereview.chromium.org/812643005/diff/40001/build/common.gypi
> > File build/common.gypi (right):
> > 
> >
>
https://codereview.chromium.org/812643005/diff/40001/build/common.gypi#newcod...
> > build/common.gypi:1471: # Set to 1 to enable AC3/EAC3 audio demuxing
> > On 2015/01/15 01:51:01, DaleCurtis wrote:
> > > On 2015/01/15 00:06:45, servolk wrote:
> > > > On 2015/01/14 23:50:07, DaleCurtis wrote:
> > > > > On 2015/01/14 23:44:11, servolk wrote:
> > > > > > On 2015/01/14 23:36:33, DaleCurtis wrote:
> > > > > > > Do we need this here? I don't see any non media/ code using this.
> > > > > > 
> > > > > > Two reasons:
> > > > > > 1. I was trying to keep gyp variables consistent,
> > > > > enable_ac3_eac3_audio_demuxing
> > > > > > is very similar in its role and scope to
enable_mpeg2ts_stream_parser
> > and
> > > > that
> > > > > > one is defined here.
> > > > > > 2. We will actually needs this in net/base/mime_types.cc when adding
> > > > ac3/eac3
> > > > > to
> > > > > > the list of supported mime types.
> > > > > > This CL is not complete yet - I still need to finish the
> > > > > net/base/mime_types.cc
> > > > > > part and trying to figure out if I can add more unit tests (the
> current
> > > > > version
> > > > > > only has unit tests for FFmpeg demuxer, would be nice to add some
unit
> > > tests
> > > > > for
> > > > > > MSE code path). But I've decided to sent it out a bit earlier to
give
> > you
> > > > guys
> > > > > > time to take a look at this, I didn't expect such a quick reply :)
> > > > > 
> > > > > New values of this type are effectively banned, so you'll face an
uphill
> > > > battle
> > > > > trying to add this. Each define added here is applied to every Chrome
> > build
> > > > > file. You're also missing the GN changes, both of which will require
> > > brettw@'s
> > > > > approval (who's very likely going to say what I just said above).
> > > > 
> > > > Added the same option to media/media_options.gni for now. But I don't
see
> > > where
> > > > the mpeg2ts option declared in the .gni file is being used.
> > > > 
> > > > > In this case it's only for Chromecast, so you might be able to get
away
> > with
> > > > it,
> > > > > but I'd look closely at how you can avoid this being a global value.
> > > > 
> > > > What would be a good alternative way to achieve this (ac3/eac3 demuxing
> > behind
> > > a
> > > > gyp/gn variable, so that we could only enable it when necessary, e.g.
> > > > Chromecast)?
> > > 
> > > If it's only necessary in media/ and net/ I'd just duplicate the logic in
> both
> > > net/ and media/ .gyp/.gn files temporarily until everything can be moved
> into
> > > media/. That said, I bet net/ folk will say we should just move the code
> > first,
> > > they've been asking forever.
> > > 
> > > >  
> > > > > I believe for the net/ stuff we want to move that into media/
(certainly
> > the
> > > > > net/ team wants us to).
> > > > 
> > > > Yes, I understand and completely agree here, it seemed strange to me
that
> > net/
> > > > has such a hard dependency on media, I agree that this stuff has to move
> to
> > > > media. But at the moment the first level of mime type checks is still
done
> > > > there, we have to add AC3/EAC3 mime types in there to make them usable,
> and
> > > then
> > > > it would be totally fine to move all this code to media/ in a separate
CL,
> > > > together with all the other media mime types.
> > > 
> > 
> > Ok, I can try to move the media mime checks from net/ to media/, but could
you
> > give me a bit more context?
> > I guess I could always just try a brute-force approach, e.g. just extract
all
> > media-related code into a separate class, name it MediaMimeUtil, move it
> > somewhere under media/ and use MediaMimeUtil in the net/'s
> > MimeUtil::IsSupportedMediaMimeType and
MimeUtil::IsSupportedNonImageMimeType.
> > But is that what you and net/ folks had in mind?
> > If there has been a discussion on that matter already, could you tell me the
> bug
> > # or loop me in on that mail thread.
> 
> I think what Dale suggested is to, instead of modifying build/common.gypi,
> introducing the new gyp variable and C define in both media.gyp and net.gyp so
> that the new gyp variable won't pollute the whole Chromium source.

Yeah, I understand that that was his first suggestion. But see the second part
of his reply. And I also agree that we should move the media mime type checks
from net/ to media/ and if it's not too much work, why not do it now?

[lcwu1, 2015-01-15 02:57:41]

On 2015/01/15 02:45:26, servolk wrote:
> On 2015/01/15 02:41:48, lcwu1 wrote:
> > On 2015/01/15 02:37:20, servolk wrote:
> > > https://codereview.chromium.org/812643005/diff/40001/build/common.gypi
> > > File build/common.gypi (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/812643005/diff/40001/build/common.gypi#newcod...
> > > build/common.gypi:1471: # Set to 1 to enable AC3/EAC3 audio demuxing
> > > On 2015/01/15 01:51:01, DaleCurtis wrote:
> > > > On 2015/01/15 00:06:45, servolk wrote:
> > > > > On 2015/01/14 23:50:07, DaleCurtis wrote:
> > > > > > On 2015/01/14 23:44:11, servolk wrote:
> > > > > > > On 2015/01/14 23:36:33, DaleCurtis wrote:
> > > > > > > > Do we need this here? I don't see any non media/ code using
this.
> > > > > > > 
> > > > > > > Two reasons:
> > > > > > > 1. I was trying to keep gyp variables consistent,
> > > > > > enable_ac3_eac3_audio_demuxing
> > > > > > > is very similar in its role and scope to
> enable_mpeg2ts_stream_parser
> > > and
> > > > > that
> > > > > > > one is defined here.
> > > > > > > 2. We will actually needs this in net/base/mime_types.cc when
adding
> > > > > ac3/eac3
> > > > > > to
> > > > > > > the list of supported mime types.
> > > > > > > This CL is not complete yet - I still need to finish the
> > > > > > net/base/mime_types.cc
> > > > > > > part and trying to figure out if I can add more unit tests (the
> > current
> > > > > > version
> > > > > > > only has unit tests for FFmpeg demuxer, would be nice to add some
> unit
> > > > tests
> > > > > > for
> > > > > > > MSE code path). But I've decided to sent it out a bit earlier to
> give
> > > you
> > > > > guys
> > > > > > > time to take a look at this, I didn't expect such a quick reply :)
> > > > > > 
> > > > > > New values of this type are effectively banned, so you'll face an
> uphill
> > > > > battle
> > > > > > trying to add this. Each define added here is applied to every
Chrome
> > > build
> > > > > > file. You're also missing the GN changes, both of which will require
> > > > brettw@'s
> > > > > > approval (who's very likely going to say what I just said above).
> > > > > 
> > > > > Added the same option to media/media_options.gni for now. But I don't
> see
> > > > where
> > > > > the mpeg2ts option declared in the .gni file is being used.
> > > > > 
> > > > > > In this case it's only for Chromecast, so you might be able to get
> away
> > > with
> > > > > it,
> > > > > > but I'd look closely at how you can avoid this being a global value.
> > > > > 
> > > > > What would be a good alternative way to achieve this (ac3/eac3
demuxing
> > > behind
> > > > a
> > > > > gyp/gn variable, so that we could only enable it when necessary, e.g.
> > > > > Chromecast)?
> > > > 
> > > > If it's only necessary in media/ and net/ I'd just duplicate the logic
in
> > both
> > > > net/ and media/ .gyp/.gn files temporarily until everything can be moved
> > into
> > > > media/. That said, I bet net/ folk will say we should just move the code
> > > first,
> > > > they've been asking forever.
> > > > 
> > > > >  
> > > > > > I believe for the net/ stuff we want to move that into media/
> (certainly
> > > the
> > > > > > net/ team wants us to).
> > > > > 
> > > > > Yes, I understand and completely agree here, it seemed strange to me
> that
> > > net/
> > > > > has such a hard dependency on media, I agree that this stuff has to
move
> > to
> > > > > media. But at the moment the first level of mime type checks is still
> done
> > > > > there, we have to add AC3/EAC3 mime types in there to make them
usable,
> > and
> > > > then
> > > > > it would be totally fine to move all this code to media/ in a separate
> CL,
> > > > > together with all the other media mime types.
> > > > 
> > > 
> > > Ok, I can try to move the media mime checks from net/ to media/, but could
> you
> > > give me a bit more context?
> > > I guess I could always just try a brute-force approach, e.g. just extract
> all
> > > media-related code into a separate class, name it MediaMimeUtil, move it
> > > somewhere under media/ and use MediaMimeUtil in the net/'s
> > > MimeUtil::IsSupportedMediaMimeType and
> MimeUtil::IsSupportedNonImageMimeType.
> > > But is that what you and net/ folks had in mind?
> > > If there has been a discussion on that matter already, could you tell me
the
> > bug
> > > # or loop me in on that mail thread.
> > 
> > I think what Dale suggested is to, instead of modifying build/common.gypi,
> > introducing the new gyp variable and C define in both media.gyp and net.gyp
so
> > that the new gyp variable won't pollute the whole Chromium source.
> 
> Yeah, I understand that that was his first suggestion. But see the second part
> of his reply. And I also agree that we should move the media mime type checks
> from net/ to media/ and if it's not too much work, why not do it now?

Yes, definitely. I mistakenly thought you missed Dale's first point.

[DaleCurtis, 2015-01-15 17:56:13]

https://codereview.chromium.org/812643005/diff/40001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/812643005/diff/40001/build/common.gypi#newcod...
build/common.gypi:1471: # Set to 1 to enable AC3/EAC3 audio demuxing
On 2015/01/15 02:37:20, servolk wrote:
> On 2015/01/15 01:51:01, DaleCurtis wrote:
> > On 2015/01/15 00:06:45, servolk wrote:
> > > On 2015/01/14 23:50:07, DaleCurtis wrote:
> > > > On 2015/01/14 23:44:11, servolk wrote:
> > > > > On 2015/01/14 23:36:33, DaleCurtis wrote:
> > > > > > Do we need this here? I don't see any non media/ code using this.
> > > > > 
> > > > > Two reasons:
> > > > > 1. I was trying to keep gyp variables consistent,
> > > > enable_ac3_eac3_audio_demuxing
> > > > > is very similar in its role and scope to enable_mpeg2ts_stream_parser
> and
> > > that
> > > > > one is defined here.
> > > > > 2. We will actually needs this in net/base/mime_types.cc when adding
> > > ac3/eac3
> > > > to
> > > > > the list of supported mime types.
> > > > > This CL is not complete yet - I still need to finish the
> > > > net/base/mime_types.cc
> > > > > part and trying to figure out if I can add more unit tests (the
current
> > > > version
> > > > > only has unit tests for FFmpeg demuxer, would be nice to add some unit
> > tests
> > > > for
> > > > > MSE code path). But I've decided to sent it out a bit earlier to give
> you
> > > guys
> > > > > time to take a look at this, I didn't expect such a quick reply :)
> > > > 
> > > > New values of this type are effectively banned, so you'll face an uphill
> > > battle
> > > > trying to add this. Each define added here is applied to every Chrome
> build
> > > > file. You're also missing the GN changes, both of which will require
> > brettw@'s
> > > > approval (who's very likely going to say what I just said above).
> > > 
> > > Added the same option to media/media_options.gni for now. But I don't see
> > where
> > > the mpeg2ts option declared in the .gni file is being used.
> > > 
> > > > In this case it's only for Chromecast, so you might be able to get away
> with
> > > it,
> > > > but I'd look closely at how you can avoid this being a global value.
> > > 
> > > What would be a good alternative way to achieve this (ac3/eac3 demuxing
> behind
> > a
> > > gyp/gn variable, so that we could only enable it when necessary, e.g.
> > > Chromecast)?
> > 
> > If it's only necessary in media/ and net/ I'd just duplicate the logic in
both
> > net/ and media/ .gyp/.gn files temporarily until everything can be moved
into
> > media/. That said, I bet net/ folk will say we should just move the code
> first,
> > they've been asking forever.
> > 
> > >  
> > > > I believe for the net/ stuff we want to move that into media/ (certainly
> the
> > > > net/ team wants us to).
> > > 
> > > Yes, I understand and completely agree here, it seemed strange to me that
> net/
> > > has such a hard dependency on media, I agree that this stuff has to move
to
> > > media. But at the moment the first level of mime type checks is still done
> > > there, we have to add AC3/EAC3 mime types in there to make them usable,
and
> > then
> > > it would be totally fine to move all this code to media/ in a separate CL,
> > > together with all the other media mime types.
> > 
> 
> Ok, I can try to move the media mime checks from net/ to media/, but could you
> give me a bit more context?
> I guess I could always just try a brute-force approach, e.g. just extract all
> media-related code into a separate class, name it MediaMimeUtil, move it
> somewhere under media/ and use MediaMimeUtil in the net/'s
> MimeUtil::IsSupportedMediaMimeType and MimeUtil::IsSupportedNonImageMimeType.
> But is that what you and net/ folks had in mind?
> If there has been a discussion on that matter already, could you tell me the
bug
> # or loop me in on that mail thread.

+rsleevi,wolenetz who are more familiar with the historical context here. I
think it's mostly a straightforward move, but it may be necessary to move it
into a minimal media/ subtarget that net/ could depend upon if any functionality
is still necessary there.  I thought there was a bug for this, but I can't find
one now.

[Ryan Sleevi, 2015-01-16 02:40:19]

On 2015/01/15 17:56:13, DaleCurtis wrote:
> +rsleevi,wolenetz who are more familiar with the historical context here. I
> think it's mostly a straightforward move, but it may be necessary to move it
> into a minimal media/ subtarget that net/ could depend upon if any
functionality
> is still necessary there.  I thought there was a bug for this, but I can't
find
> one now.

See https://codereview.chromium.org/401523002/

[Ryan Sleevi, 2015-01-16 02:43:06]

On 2015/01/16 02:40:19, Ryan Sleevi wrote:
> On 2015/01/15 17:56:13, DaleCurtis wrote:
> > +rsleevi,wolenetz who are more familiar with the historical context here. I
> > think it's mostly a straightforward move, but it may be necessary to move it
> > into a minimal media/ subtarget that net/ could depend upon if any
> functionality
> > is still necessary there.  I thought there was a bug for this, but I can't
> find
> > one now.
> 
> See https://codereview.chromium.org/401523002/

I should add that, if someone wants to take that up, I suspect rdsmith@ is best
in line to help remove the single usage from //net.

[servolk, 2015-01-16 18:10:43]

On 2015/01/16 02:40:19, Ryan Sleevi wrote:
> On 2015/01/15 17:56:13, DaleCurtis wrote:
> > +rsleevi,wolenetz who are more familiar with the historical context here. I
> > think it's mostly a straightforward move, but it may be necessary to move it
> > into a minimal media/ subtarget that net/ could depend upon if any
> functionality
> > is still necessary there.  I thought there was a bug for this, but I can't
> find
> > one now.
> 
> See https://codereview.chromium.org/401523002/

Ok, thanks Ryan, that's very helpful! Looks like that CL never landed for some
reason, and unfortunately I can't see anyone mentioning in the comments why. The
first few attempts apparently failed because of unit test failures, but then
Aaron manually unchecked the CQ bit and never updated/commented on the CL after
that. It's ~6 months old, so probably going to be a bit hard to rebase this to
the current code, but I'll give it a try.

[ddorwin, 2015-02-26 20:49:31]

ddorwin@chromium.org changed reviewers:
+ ddorwin@chromium.org

[ddorwin, 2015-02-26 20:49:32]

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

https://codereview.chromium.org/812643005/diff/100001/net/base/mime_util.cc#n...
net/base/mime_util.cc:556: "ac-3,ec-3,mp4a.a5,mp4a.a6,"
You only need one each for AC3 and EAC3.

https://codereview.chromium.org/812643005/diff/100001/net/base/mime_util.cc#n...
net/base/mime_util.cc:606: {"ac-3", MimeUtil::AC3},
Please add browser tests.

https://codereview.chromium.org/812643005/diff/100001/net/base/mime_util.cc#n...
net/base/mime_util.cc:911: // We need to know the codec subtype to properly
support AC3/EAC3.
Why is stripping okay? I don't think that is the intent of strip. Instead, you
should check whether the entire string is valid as we do for AVC levels.

[servolk, 2015-02-26 22:00:59]

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

https://codereview.chromium.org/812643005/diff/100001/net/base/mime_util.cc#n...
net/base/mime_util.cc:556: "ac-3,ec-3,mp4a.a5,mp4a.a6,"
On 2015/02/26 20:49:32, ddorwin wrote:
> You only need one each for AC3 and EAC3.

Why? I believe these are just two different ways to indicate the same codec. For
example for EAC3, the ec-3 corresponds to the standard ETSI TS 102 366 v1.2.1,
while mp4a.a6 corresponds to the preferred way to specify codec according to RFC
6381. I think we should support both. I know for sure that, for example, Netflix
is using mp4a.a6, whereas I've seen other content providers using ec-3.

https://codereview.chromium.org/812643005/diff/100001/net/base/mime_util.cc#n...
net/base/mime_util.cc:911: // We need to know the codec subtype to properly
support AC3/EAC3.
On 2015/02/26 20:49:32, ddorwin wrote:
> Why is stripping okay? I don't think that is the intent of strip. Instead, you
> should check whether the entire string is valid as we do for AVC levels.

Actually I'm not sure about that myself, that just a piece of code that we had
in downstream (Chromecast) code, that was added by somebody else, so I assumed
it was needed. I'll look into this some more.
Btw, note that this actually prevents the stripping.

[ddorwin, 2015-02-26 22:06:18]

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

https://codereview.chromium.org/812643005/diff/100001/net/base/mime_util.cc#n...
net/base/mime_util.cc:556: "ac-3,ec-3,mp4a.a5,mp4a.a6,"
On 2015/02/26 22:00:59, servolk wrote:
> On 2015/02/26 20:49:32, ddorwin wrote:
> > You only need one each for AC3 and EAC3.
> 
> Why? I believe these are just two different ways to indicate the same codec.
For
> example for EAC3, the ec-3 corresponds to the standard ETSI TS 102 366 v1.2.1,
> while mp4a.a6 corresponds to the preferred way to specify codec according to
RFC
> 6381. I think we should support both. I know for sure that, for example,
Netflix
> is using mp4a.a6, whereas I've seen other content providers using ec-3.

Because this doesn't do what you think it does. (That code is at line 606.) See
the comment in this area in tip of tree.

https://codereview.chromium.org/812643005/diff/100001/net/base/mime_util.cc#n...
net/base/mime_util.cc:561:
"mp4a.66,mp4a.67,mp4a.68,mp4a.69,mp4a.6B,mp4a.40.2,mp4a.40.02,mp4a.40.5,"
You probably need to add those strings here too if you want "video/mp4" to
support them.

[servolk, 2015-02-26 22:29:21]

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

https://codereview.chromium.org/812643005/diff/100001/net/base/mime_util.cc#n...
net/base/mime_util.cc:556: "ac-3,ec-3,mp4a.a5,mp4a.a6,"
On 2015/02/26 22:06:18, ddorwin wrote:
> On 2015/02/26 22:00:59, servolk wrote:
> > On 2015/02/26 20:49:32, ddorwin wrote:
> > > You only need one each for AC3 and EAC3.
> > 
> > Why? I believe these are just two different ways to indicate the same codec.
> For
> > example for EAC3, the ec-3 corresponds to the standard ETSI TS 102 366
v1.2.1,
> > while mp4a.a6 corresponds to the preferred way to specify codec according to
> RFC
> > 6381. I think we should support both. I know for sure that, for example,
> Netflix
> > is using mp4a.a6, whereas I've seen other content providers using ec-3.
> 
> Because this doesn't do what you think it does. (That code is at line 606.)
See
> the comment in this area in tip of tree.

Could you elaborate? As far as I can see this adds those codecs to
format_codec_mappings for audio/mp4 and audio/x-m4a types, which leads to those
codecs being added to strict_format_map_ and consequently properly recognized in
IsSupportedStrictMediaMimeType, which is probably something that we want to do
in order to fully support those codecs on all code paths. Am I missing something
here?

https://codereview.chromium.org/812643005/diff/100001/net/base/mime_util.cc#n...
net/base/mime_util.cc:561:
"mp4a.66,mp4a.67,mp4a.68,mp4a.69,mp4a.6B,mp4a.40.2,mp4a.40.02,mp4a.40.5,"
On 2015/02/26 22:06:18, ddorwin wrote:
> You probably need to add those strings here too if you want "video/mp4" to
> support them.

Yes, that's probably a good idea, I'll do this. But it's still not completely
clear to me - should we add all 5 strings here (i.e. including ec-3/mp4a.a6
variants) or just some subset of that (e.g. only aac51,ec-3,ac-3)?

[ddorwin, 2015-02-26 22:34:15]

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

https://codereview.chromium.org/812643005/diff/100001/net/base/mime_util.cc#n...
net/base/mime_util.cc:556: "ac-3,ec-3,mp4a.a5,mp4a.a6,"
On 2015/02/26 22:29:20, servolk wrote:
> On 2015/02/26 22:06:18, ddorwin wrote:
> > On 2015/02/26 22:00:59, servolk wrote:
> > > On 2015/02/26 20:49:32, ddorwin wrote:
> > > > You only need one each for AC3 and EAC3.
> > > 
> > > Why? I believe these are just two different ways to indicate the same
codec.
> > For
> > > example for EAC3, the ec-3 corresponds to the standard ETSI TS 102 366
> v1.2.1,
> > > while mp4a.a6 corresponds to the preferred way to specify codec according
to
> > RFC
> > > 6381. I think we should support both. I know for sure that, for example,
> > Netflix
> > > is using mp4a.a6, whereas I've seen other content providers using ec-3.
> > 
> > Because this doesn't do what you think it does. (That code is at line 606.)
> See
> > the comment in this area in tip of tree.
> 
> Could you elaborate? As far as I can see this adds those codecs to
> format_codec_mappings for audio/mp4 and audio/x-m4a types, which leads to
those
> codecs being added to strict_format_map_ and consequently properly recognized
in
> IsSupportedStrictMediaMimeType, which is probably something that we want to do
> in order to fully support those codecs on all code paths. Am I missing
something
> here?

Yes, but format_codec_mappings is converted to enums during initialization. See
https://codereview.chromium.org/952673002/diff/1/net/base/mime_util.cc#newcod...

https://codereview.chromium.org/812643005/diff/100001/net/base/mime_util.cc#n...
net/base/mime_util.cc:561:
"mp4a.66,mp4a.67,mp4a.68,mp4a.69,mp4a.6B,mp4a.40.2,mp4a.40.02,mp4a.40.5,"
On 2015/02/26 22:29:20, servolk wrote:
> On 2015/02/26 22:06:18, ddorwin wrote:
> > You probably need to add those strings here too if you want "video/mp4" to
> > support them.
> 
> Yes, that's probably a good idea, I'll do this. But it's still not completely
> clear to me - should we add all 5 strings here (i.e. including ec-3/mp4a.a6
> variants) or just some subset of that (e.g. only aac51,ec-3,ac-3)?

Same as above: 2,
BTW, aac51 is not part of this CL (nor standardized I believe).

[servolk, 2015-02-26 22:43:54]

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

https://codereview.chromium.org/812643005/diff/100001/net/base/mime_util.cc#n...
net/base/mime_util.cc:556: "ac-3,ec-3,mp4a.a5,mp4a.a6,"
On 2015/02/26 22:34:15, ddorwin wrote:
> On 2015/02/26 22:29:20, servolk wrote:
> > On 2015/02/26 22:06:18, ddorwin wrote:
> > > On 2015/02/26 22:00:59, servolk wrote:
> > > > On 2015/02/26 20:49:32, ddorwin wrote:
> > > > > You only need one each for AC3 and EAC3.
> > > > 
> > > > Why? I believe these are just two different ways to indicate the same
> codec.
> > > For
> > > > example for EAC3, the ec-3 corresponds to the standard ETSI TS 102 366
> > v1.2.1,
> > > > while mp4a.a6 corresponds to the preferred way to specify codec
according
> to
> > > RFC
> > > > 6381. I think we should support both. I know for sure that, for example,
> > > Netflix
> > > > is using mp4a.a6, whereas I've seen other content providers using ec-3.
> > > 
> > > Because this doesn't do what you think it does. (That code is at line
606.)
> > See
> > > the comment in this area in tip of tree.
> > 
> > Could you elaborate? As far as I can see this adds those codecs to
> > format_codec_mappings for audio/mp4 and audio/x-m4a types, which leads to
> those
> > codecs being added to strict_format_map_ and consequently properly
recognized
> in
> > IsSupportedStrictMediaMimeType, which is probably something that we want to
do
> > in order to fully support those codecs on all code paths. Am I missing
> something
> > here?
> 
> Yes, but format_codec_mappings is converted to enums during initialization.
See
>
https://codereview.chromium.org/952673002/diff/1/net/base/mime_util.cc#newcod...

Ah, ok, you are right, I missed the fact that they are converted into enums
instead of being registered as strings. So we should be good with adding just
one of the variants here, as long as we have all the variations in
format_codec_mappings. Working on it.

[servolk, 2015-02-27 01:21:14]

Added unit tests and various fixes to address earlier feedback. Note that one
thing that still needs to be done is figuring out the proper way for adding a
gyp flag and gating this code in Chrome, I'm going to update this CL once we
find a solution (this is currently being discussed at
https://codereview.chromium.org/877323009/)

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

https://codereview.chromium.org/812643005/diff/100001/net/base/mime_util.cc#n...
net/base/mime_util.cc:606: {"ac-3", MimeUtil::AC3},
On 2015/02/26 20:49:32, ddorwin wrote:
> Please add browser tests.

Done.

[ddorwin, 2015-02-27 01:44:13]

https://codereview.chromium.org/812643005/diff/160001/content/browser/media/m...
File content/browser/media/media_canplaytype_browsertest.cc (right):

https://codereview.chromium.org/812643005/diff/160001/content/browser/media/m...
content/browser/media/media_canplaytype_browsertest.cc:1044: #if
defined(ENABLE_AC3_EAC3_DEMUXING)
We should always run the tests, but the expected results should be based on this
define.

https://codereview.chromium.org/812643005/diff/160001/content/browser/media/m...
content/browser/media/media_canplaytype_browsertest.cc:1047:
EXPECT_EQ(kPropProbably, CanPlay("'audio/mp4; codecs=\"ac-3\"'"));
Please also test invalid variants, such as:
ac-2
ac-4
mp4a.a5.
mp4a.a5.1
mp4a.a7

If you put these in TestMPEGUnacceptableCombinations(), they'll run for all
relevant containers.

https://codereview.chromium.org/812643005/diff/160001/content/browser/media/m...
content/browser/media/media_canplaytype_browsertest.cc:1055:
EXPECT_EQ(kPropProbably, CanPlay("'video/mp4; codecs=\"mp4a.a6\"'"));
There are many more test scenarios. For example, x-m4a, HLS, etc. Please inline
these like the others. (By ifdef'ing the result, you won't need ifdefs
everywhere.)

[servolk, 2015-02-27 02:19:08]

https://codereview.chromium.org/812643005/diff/160001/content/browser/media/m...
File content/browser/media/media_canplaytype_browsertest.cc (right):

https://codereview.chromium.org/812643005/diff/160001/content/browser/media/m...
content/browser/media/media_canplaytype_browsertest.cc:1044: #if
defined(ENABLE_AC3_EAC3_DEMUXING)
On 2015/02/27 01:44:13, ddorwin wrote:
> We should always run the tests, but the expected results should be based on
this
> define.

Done.

https://codereview.chromium.org/812643005/diff/160001/content/browser/media/m...
content/browser/media/media_canplaytype_browsertest.cc:1047:
EXPECT_EQ(kPropProbably, CanPlay("'audio/mp4; codecs=\"ac-3\"'"));
On 2015/02/27 01:44:13, ddorwin wrote:
> Please also test invalid variants, such as:
> ac-2
> ac-4
> mp4a.a5.
> mp4a.a5.1
> mp4a.a7
> 
> If you put these in TestMPEGUnacceptableCombinations(), they'll run for all
> relevant containers.

Ok, I've added a few variants, should be enough for basic coverage.

https://codereview.chromium.org/812643005/diff/160001/content/browser/media/m...
content/browser/media/media_canplaytype_browsertest.cc:1055:
EXPECT_EQ(kPropProbably, CanPlay("'video/mp4; codecs=\"mp4a.a6\"'"));
On 2015/02/27 01:44:13, ddorwin wrote:
> There are many more test scenarios. For example, x-m4a, HLS, etc. Please
inline
> these like the others. (By ifdef'ing the result, you won't need ifdefs
> everywhere.)

What do you mean? I've basically taken one of the existing tests and changed it
to test ac3/eac3 codec variants.

[ddorwin, 2015-03-02 19:35:39]

https://codereview.chromium.org/812643005/diff/160001/content/browser/media/m...
File content/browser/media/media_canplaytype_browsertest.cc (right):

https://codereview.chromium.org/812643005/diff/160001/content/browser/media/m...
content/browser/media/media_canplaytype_browsertest.cc:1055:
EXPECT_EQ(kPropProbably, CanPlay("'video/mp4; codecs=\"mp4a.a6\"'"));
On 2015/02/27 02:19:07, servolk wrote:
> On 2015/02/27 01:44:13, ddorwin wrote:
> > There are many more test scenarios. For example, x-m4a, HLS, etc. Please
> inline
> > these like the others. (By ifdef'ing the result, you won't need ifdefs
> > everywhere.)
> 
> What do you mean? I've basically taken one of the existing tests and changed
it
> to test ac3/eac3 codec variants.

The tests are broken up by container, not codec. If you add these ~10 lines to
each of the BMFF containers (video/mp4, x-m4a, HLS, etc.) you'll get more
complete coverage (and possibly see that you are enabling things you didn't
intend to). We also need to check that WebM does not support these codecs.

https://codereview.chromium.org/812643005/diff/180001/content/browser/media/m...
File content/browser/media/media_canplaytype_browsertest.cc (right):

https://codereview.chromium.org/812643005/diff/180001/content/browser/media/m...
content/browser/media/media_canplaytype_browsertest.cc:177: EXPECT_EQ(kNot,
CanPlay("'" + mime + "; codecs=\"ac-4\"'"));
nit: add ec-4 for completeness/consistency

https://codereview.chromium.org/812643005/diff/180001/content/browser/media/m...
content/browser/media/media_canplaytype_browsertest.cc:180: EXPECT_EQ(kNot,
CanPlay("'" + mime + "; codecs=\"mp4a.a5.1\"'"));
Add: mp4a.a5. (ends in period)

https://codereview.chromium.org/812643005/diff/180001/content/browser/media/m...
content/browser/media/media_canplaytype_browsertest.cc:1052: const char*
expected_result = kNot;
You'll probably need to move this up to line 20.

[servolk, 2015-11-21 00:06:12]

servolk@chromium.org changed reviewers:
- damienv@chromium.org, gunsch@chromium.org, scherkus@chromium.org

[servolk, 2015-11-24 19:39:14]

On 2015/03/02 19:35:39, ddorwin wrote:
>
https://codereview.chromium.org/812643005/diff/160001/content/browser/media/m...
> File content/browser/media/media_canplaytype_browsertest.cc (right):
> 
>
https://codereview.chromium.org/812643005/diff/160001/content/browser/media/m...
> content/browser/media/media_canplaytype_browsertest.cc:1055:
> EXPECT_EQ(kPropProbably, CanPlay("'video/mp4; codecs=\"mp4a.a6\"'"));
> On 2015/02/27 02:19:07, servolk wrote:
> > On 2015/02/27 01:44:13, ddorwin wrote:
> > > There are many more test scenarios. For example, x-m4a, HLS, etc. Please
> > inline
> > > these like the others. (By ifdef'ing the result, you won't need ifdefs
> > > everywhere.)
> > 
> > What do you mean? I've basically taken one of the existing tests and changed
> it
> > to test ac3/eac3 codec variants.
> 
> The tests are broken up by container, not codec. If you add these ~10 lines to
> each of the BMFF containers (video/mp4, x-m4a, HLS, etc.) you'll get more
> complete coverage (and possibly see that you are enabling things you didn't
> intend to). We also need to check that WebM does not support these codecs.
> 
>
https://codereview.chromium.org/812643005/diff/180001/content/browser/media/m...
> File content/browser/media/media_canplaytype_browsertest.cc (right):
> 
>
https://codereview.chromium.org/812643005/diff/180001/content/browser/media/m...
> content/browser/media/media_canplaytype_browsertest.cc:177: EXPECT_EQ(kNot,
> CanPlay("'" + mime + "; codecs=\"ac-4\"'"));
> nit: add ec-4 for completeness/consistency
> 
>
https://codereview.chromium.org/812643005/diff/180001/content/browser/media/m...
> content/browser/media/media_canplaytype_browsertest.cc:180: EXPECT_EQ(kNot,
> CanPlay("'" + mime + "; codecs=\"mp4a.a5.1\"'"));
> Add: mp4a.a5. (ends in period)
> 
>
https://codereview.chromium.org/812643005/diff/180001/content/browser/media/m...
> content/browser/media/media_canplaytype_browsertest.cc:1052: const char*
> expected_result = kNot;
> You'll probably need to move this up to line 20.

Please take another look. I have re-worked how the AC3/EAC3 demuxing
configuration is handled (similar to how it was eventually done in HEVC CL). And
fixed the unit tests as suggested.

[servolk, 2015-11-24 19:45:17]

https://codereview.chromium.org/812643005/diff/180001/content/browser/media/m...
File content/browser/media/media_canplaytype_browsertest.cc (right):

https://codereview.chromium.org/812643005/diff/180001/content/browser/media/m...
content/browser/media/media_canplaytype_browsertest.cc:177: EXPECT_EQ(kNot,
CanPlay("'" + mime + "; codecs=\"ac-4\"'"));
On 2015/03/02 19:35:38, ddorwin wrote:
> nit: add ec-4 for completeness/consistency

Done.

https://codereview.chromium.org/812643005/diff/180001/content/browser/media/m...
content/browser/media/media_canplaytype_browsertest.cc:180: EXPECT_EQ(kNot,
CanPlay("'" + mime + "; codecs=\"mp4a.a5.1\"'"));
On 2015/03/02 19:35:38, ddorwin wrote:
> Add: mp4a.a5. (ends in period)

Done.

https://codereview.chromium.org/812643005/diff/180001/content/browser/media/m...
content/browser/media/media_canplaytype_browsertest.cc:1052: const char*
expected_result = kNot;
On 2015/03/02 19:35:38, ddorwin wrote:
> You'll probably need to move this up to line 20.

Done.

[ddorwin, 2015-11-25 22:24:08]

I reviewed through media/base/mime_util.cc. Please have someone else review the
details. Mostly minor comments.

https://codereview.chromium.org/812643005/diff/400001/content/browser/media/m...
File content/browser/media/media_canplaytype_browsertest.cc (right):

https://codereview.chromium.org/812643005/diff/400001/content/browser/media/m...
content/browser/media/media_canplaytype_browsertest.cc:226: EXPECT_EQ(kNot,
CanPlay("'" + mime + "; codecs=\"ac-\"'"));
ac
ec
ac3
ec3

https://codereview.chromium.org/812643005/diff/400001/content/browser/media/m...
content/browser/media/media_canplaytype_browsertest.cc:311: EXPECT_EQ(kNot,
CanPlay("'" + mime + "; codecs=\"hvc1.1.6.L93.B0,opus\"'"));
Add valid ac3/eac3 strings to all the non-mpeg test helpers.

https://codereview.chromium.org/812643005/diff/400001/content/browser/media/m...
content/browser/media/media_canplaytype_browsertest.cc:620: // types being
supported, so they should be allowed for now.
bug reference?

https://codereview.chromium.org/812643005/diff/400001/content/browser/media/m...
content/browser/media/media_canplaytype_browsertest.cc:712:
CanPlay("'video/x-m4v; codecs=\"hvc1.1.6.L93.B0, mp4a.40.5\"'"));
Test the x- variants for audio and video.

https://codereview.chromium.org/812643005/diff/400001/content/browser/media/m...
content/browser/media/media_canplaytype_browsertest.cc:1093: // HLS are
supported only on Android IceCreamSandwich and above (API level 14)
Separate CL: We can remove this check. We might search for similar checks here
and in mime_util and remove any support for < 16 as ICS is no longer supported.

https://codereview.chromium.org/812643005/diff/400001/content/browser/media/m...
content/browser/media/media_canplaytype_browsertest.cc:1233: EXPECT_EQ(kNot,
I don't think these are correct when HEVC is supported. HLS uses the mp4
constant. Is HEVC removed from the HLS list somehow?

https://codereview.chromium.org/812643005/diff/400001/content/browser/media/m...
content/browser/media/media_canplaytype_browsertest.cc:1238:
"codecs=\"hvc1.1.6.L93.B0,mp4a.40.5\"'"));
Add test for ac3/eac3.

https://codereview.chromium.org/812643005/diff/400001/content/browser/media/m...
content/browser/media/media_canplaytype_browsertest.cc:1268:
CanPlay("'video/mp2t; codecs=\"avc1.640028,mp4a.40.2\"'"));
ditto

HEVC too

https://codereview.chromium.org/812643005/diff/400001/media/base/audio_decode...
File media/base/audio_decoder_config.cc (right):

https://codereview.chromium.org/812643005/diff/400001/media/base/audio_decode...
media/base/audio_decoder_config.cc:103: case kCodecAC3:
Keep the cases in the same order as the enum.

https://codereview.chromium.org/812643005/diff/400001/media/base/mime_util.cc
File media/base/mime_util.cc (right):

https://codereview.chromium.org/812643005/diff/400001/media/base/mime_util.cc...
media/base/mime_util.cc:43: #if defined(ENABLE_AC3_EAC3_AUDIO_DEMUXING)
Do we need to ifdef this? There are other proprietary codecs, including HEVC,
that are not and the previous files did not.

Yes, it is dead code on most platforms, but this also hurts readability.

https://codereview.chromium.org/812643005/diff/400001/media/base/mime_util.cc...
media/base/mime_util.cc:45: AC3,
Regardless, keep similar types (at least audio) together. There is no indication
that this list cannot be modified.

https://codereview.chromium.org/812643005/diff/400001/media/base/mime_util.cc...
media/base/mime_util.cc:272: "ac-3,ec-3,"  // Note: only one variant of eac
codec string is sufficient
Same as the next comment.

https://codereview.chromium.org/812643005/diff/400001/media/base/mime_util.cc...
media/base/mime_util.cc:292: "ac-3,ec-3,"  // Note: only one variant of eac
codec string is sufficient
Here and above, be consistent with existing text and start the comment above the
string. Also, perhaps use text more similar to the others?

https://codereview.chromium.org/812643005/diff/400001/media/base/mime_util.cc...
media/base/mime_util.cc:326: {"application/x-mpegurl",
kMP4VideoCodecsExpression},
Do you want to support this in HLS? The current code does.

https://codereview.chromium.org/812643005/diff/400001/media/base/mime_util.cc...
media/base/mime_util.cc:340: // avc1/avc3.XXXXXX may be unambiguous; handled by
ParseH264CodecID().
Please add a similar comment for HEVC.

https://codereview.chromium.org/812643005/diff/400001/media/base/mime_util.cc...
media/base/mime_util.cc:354: {"ec-3", MimeUtil::EAC3},
group the codecs together

https://codereview.chromium.org/812643005/diff/400001/media/base/mime_util.cc...
media/base/mime_util.cc:356: {"mp4a.A5", MimeUtil::AC3},
Why both cases? Is one not "correct?" With everything else case sensitive, this
seems odd.

https://codereview.chromium.org/812643005/diff/400001/media/base/mime_util.cc...
media/base/mime_util.cc:729: #if defined(ENABLE_AC3_EAC3_AUDIO_DEMUXING)
ditto and match order of enum.

[servolk, 2015-12-01 18:03:50]

servolk@chromium.org changed reviewers:
+ erickung@chromium.org

[servolk, 2015-12-03 00:56:15]

https://codereview.chromium.org/812643005/diff/400001/content/browser/media/m...
File content/browser/media/media_canplaytype_browsertest.cc (right):

https://codereview.chromium.org/812643005/diff/400001/content/browser/media/m...
content/browser/media/media_canplaytype_browsertest.cc:226: EXPECT_EQ(kNot,
CanPlay("'" + mime + "; codecs=\"ac-\"'"));
On 2015/11/25 22:24:07, ddorwin wrote:
> ac
> ec
> ac3
> ec3

Done.

https://codereview.chromium.org/812643005/diff/400001/content/browser/media/m...
content/browser/media/media_canplaytype_browsertest.cc:311: EXPECT_EQ(kNot,
CanPlay("'" + mime + "; codecs=\"hvc1.1.6.L93.B0,opus\"'"));
On 2015/11/25 22:24:07, ddorwin wrote:
> Add valid ac3/eac3 strings to all the non-mpeg test helpers.

Done.

https://codereview.chromium.org/812643005/diff/400001/content/browser/media/m...
content/browser/media/media_canplaytype_browsertest.cc:620: // types being
supported, so they should be allowed for now.
On 2015/11/25 22:24:07, ddorwin wrote:
> bug reference?

Done.

https://codereview.chromium.org/812643005/diff/400001/content/browser/media/m...
content/browser/media/media_canplaytype_browsertest.cc:712:
CanPlay("'video/x-m4v; codecs=\"hvc1.1.6.L93.B0, mp4a.40.5\"'"));
On 2015/11/25 22:24:07, ddorwin wrote:
> Test the x- variants for audio and video.

Done.

https://codereview.chromium.org/812643005/diff/400001/content/browser/media/m...
content/browser/media/media_canplaytype_browsertest.cc:1093: // HLS are
supported only on Android IceCreamSandwich and above (API level 14)
On 2015/11/25 22:24:07, ddorwin wrote:
> Separate CL: We can remove this check. We might search for similar checks here
> and in mime_util and remove any support for < 16 as ICS is no longer
supported.

Acknowledged.

https://codereview.chromium.org/812643005/diff/400001/content/browser/media/m...
content/browser/media/media_canplaytype_browsertest.cc:1233: EXPECT_EQ(kNot,
On 2015/11/25 22:24:07, ddorwin wrote:
> I don't think these are correct when HEVC is supported. HLS uses the mp4
> constant. Is HEVC removed from the HLS list somehow?

No, but HLS mime types are only enabled for Android (see
https://code.google.com/p/chromium/codesearch#chromium/src/media/base/mime_ut...)
and HEVC support is not enabled for Clank yet.

https://codereview.chromium.org/812643005/diff/400001/content/browser/media/m...
content/browser/media/media_canplaytype_browsertest.cc:1238:
"codecs=\"hvc1.1.6.L93.B0,mp4a.40.5\"'"));
On 2015/11/25 22:24:07, ddorwin wrote:
> Add test for ac3/eac3.

Done.

https://codereview.chromium.org/812643005/diff/400001/content/browser/media/m...
content/browser/media/media_canplaytype_browsertest.cc:1268:
CanPlay("'video/mp2t; codecs=\"avc1.640028,mp4a.40.2\"'"));
On 2015/11/25 22:24:07, ddorwin wrote:
> ditto
> 
> HEVC too

Done. (added AC3/EAC3 in mpeg2ts container, let's leave HEVC for a separate CL).

https://codereview.chromium.org/812643005/diff/400001/media/base/audio_decode...
File media/base/audio_decoder_config.cc (right):

https://codereview.chromium.org/812643005/diff/400001/media/base/audio_decode...
media/base/audio_decoder_config.cc:103: case kCodecAC3:
On 2015/11/25 22:24:07, ddorwin wrote:
> Keep the cases in the same order as the enum.

Done.

https://codereview.chromium.org/812643005/diff/400001/media/base/mime_util.cc
File media/base/mime_util.cc (right):

https://codereview.chromium.org/812643005/diff/400001/media/base/mime_util.cc...
media/base/mime_util.cc:43: #if defined(ENABLE_AC3_EAC3_AUDIO_DEMUXING)
On 2015/11/25 22:24:08, ddorwin wrote:
> Do we need to ifdef this? There are other proprietary codecs, including HEVC,
> that are not and the previous files did not.
> 
> Yes, it is dead code on most platforms, but this also hurts readability.

Done.

https://codereview.chromium.org/812643005/diff/400001/media/base/mime_util.cc...
media/base/mime_util.cc:45: AC3,
On 2015/11/25 22:24:08, ddorwin wrote:
> Regardless, keep similar types (at least audio) together. There is no
indication
> that this list cannot be modified.

Done.

https://codereview.chromium.org/812643005/diff/400001/media/base/mime_util.cc...
media/base/mime_util.cc:272: "ac-3,ec-3,"  // Note: only one variant of eac
codec string is sufficient
On 2015/11/25 22:24:08, ddorwin wrote:
> Same as the next comment.

Done.

https://codereview.chromium.org/812643005/diff/400001/media/base/mime_util.cc...
media/base/mime_util.cc:292: "ac-3,ec-3,"  // Note: only one variant of eac
codec string is sufficient
On 2015/11/25 22:24:08, ddorwin wrote:
> Here and above, be consistent with existing text and start the comment above
the
> string. Also, perhaps use text more similar to the others?

Done.

https://codereview.chromium.org/812643005/diff/400001/media/base/mime_util.cc...
media/base/mime_util.cc:326: {"application/x-mpegurl",
kMP4VideoCodecsExpression},
On 2015/11/25 22:24:08, ddorwin wrote:
> Do you want to support this in HLS? The current code does.

HLS is only enabled on Clank/Android, where AC3/EAC3 is not currently supported,
AFAIK. But once AC3_EAC3_DEMUXING is enabled on Android, I don't see any reasons
to block AC3/EAC3 in HLS.

https://codereview.chromium.org/812643005/diff/400001/media/base/mime_util.cc...
media/base/mime_util.cc:354: {"ec-3", MimeUtil::EAC3},
On 2015/11/25 22:24:07, ddorwin wrote:
> group the codecs together

Done.

https://codereview.chromium.org/812643005/diff/400001/media/base/mime_util.cc...
media/base/mime_util.cc:356: {"mp4a.A5", MimeUtil::AC3},
On 2015/11/25 22:24:08, ddorwin wrote:
> Why both cases? Is one not "correct?" With everything else case sensitive,
this
> seems odd.

I've copied the comment from the unit test that references crbug.com/564960 to
explain why we need both.

https://codereview.chromium.org/812643005/diff/400001/media/base/mime_util.cc...
media/base/mime_util.cc:729: #if defined(ENABLE_AC3_EAC3_AUDIO_DEMUXING)
On 2015/11/25 22:24:08, ddorwin wrote:
> ditto and match order of enum.

Done.

[ddorwin, 2015-12-03 23:46:53]

See
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/5c4ySpPsV14
for the new way to do build flags.

https://codereview.chromium.org/812643005/diff/440001/media/base/audio_decode...
File media/base/audio_decoder_config.cc (right):

https://codereview.chromium.org/812643005/diff/440001/media/base/audio_decode...
media/base/audio_decoder_config.cc:119: case kCodecPCM_ALAW:
It looks like this one is out of order. Hmm. I wonder if others are as well.

https://codereview.chromium.org/812643005/diff/440001/media/base/mime_util.cc
File media/base/mime_util.cc (right):

https://codereview.chromium.org/812643005/diff/440001/media/base/mime_util.cc...
media/base/mime_util.cc:27: AC3,
nit: It seems these should come after MP3.

https://codereview.chromium.org/812643005/diff/440001/media/base/mime_util.cc...
media/base/mime_util.cc:152: case MimeUtil::HEVC_MAIN:
This should have been added at the end.

https://codereview.chromium.org/812643005/diff/440001/media/base/mime_util.cc...
media/base/mime_util.cc:167: case MimeUtil::AC3:
Should be before 161.

https://codereview.chromium.org/812643005/diff/440001/media/base/mime_util.cc...
media/base/mime_util.cc:216: // Note: only one variant of ac3/eac3 codec string
is sufficient here, since
variant *each* of ac3 *&* eac3

https://codereview.chromium.org/812643005/diff/440001/media/base/mime_util.cc...
media/base/mime_util.cc:236: // Note: only one variant of ac3/eac3 codec string
is sufficient here, since
ditto

https://codereview.chromium.org/812643005/diff/440001/media/base/mime_util.cc...
media/base/mime_util.cc:293: // avc1/avc3.XXXXXX may be unambiguous; handled by
ParseH264CodecID().
Please add a similar comment for HEVC. (It's missing from this list without
explanation.)

https://codereview.chromium.org/812643005/diff/440001/media/base/mime_util.cc...
media/base/mime_util.cc:311: // (crbug.com/564960)
nit: missing period

[servolk, 2015-12-21 17:50:57]

I've created a separate CL to try out converting an existing global flag
(enable_mpeg2ts) into a build flag. I'll convert this CL to use the same scheme
once we work out what it should look like:
https://codereview.chromium.org/1539983004/

https://codereview.chromium.org/812643005/diff/440001/media/base/audio_decode...
File media/base/audio_decoder_config.cc (right):

https://codereview.chromium.org/812643005/diff/440001/media/base/audio_decode...
media/base/audio_decoder_config.cc:119: case kCodecPCM_ALAW:
On 2015/12/03 23:46:52, ddorwin wrote:
> It looks like this one is out of order. Hmm. I wonder if others are as well.

Yep, some of these are out of order, but I'd rather keep this CL limited to
AC3/EAC3 changes, so not changing it for now.

https://codereview.chromium.org/812643005/diff/440001/media/base/mime_util.cc
File media/base/mime_util.cc (right):

https://codereview.chromium.org/812643005/diff/440001/media/base/mime_util.cc...
media/base/mime_util.cc:27: AC3,
On 2015/12/03 23:46:52, ddorwin wrote:
> nit: It seems these should come after MP3.

Done.

https://codereview.chromium.org/812643005/diff/440001/media/base/mime_util.cc...
media/base/mime_util.cc:152: case MimeUtil::HEVC_MAIN:
On 2015/12/03 23:46:52, ddorwin wrote:
> This should have been added at the end.

Will do in a separate CL.

https://codereview.chromium.org/812643005/diff/440001/media/base/mime_util.cc...
media/base/mime_util.cc:167: case MimeUtil::AC3:
On 2015/12/03 23:46:52, ddorwin wrote:
> Should be before 161.

Done.

https://codereview.chromium.org/812643005/diff/440001/media/base/mime_util.cc...
media/base/mime_util.cc:216: // Note: only one variant of ac3/eac3 codec string
is sufficient here, since
On 2015/12/03 23:46:52, ddorwin wrote:
> variant *each* of ac3 *&* eac3

Done.

https://codereview.chromium.org/812643005/diff/440001/media/base/mime_util.cc...
media/base/mime_util.cc:236: // Note: only one variant of ac3/eac3 codec string
is sufficient here, since
On 2015/12/03 23:46:52, ddorwin wrote:
> ditto

Done.

https://codereview.chromium.org/812643005/diff/440001/media/base/mime_util.cc...
media/base/mime_util.cc:293: // avc1/avc3.XXXXXX may be unambiguous; handled by
ParseH264CodecID().
On 2015/12/03 23:46:52, ddorwin wrote:
> Please add a similar comment for HEVC. (It's missing from this list without
> explanation.)

Will do in a separate CL. I'd prefer this CL to have only AC3/EAC3 changes, to
make it easier for us to cherry-pick it into Chromecast.

https://codereview.chromium.org/812643005/diff/440001/media/base/mime_util.cc...
media/base/mime_util.cc:311: // (crbug.com/564960)
On 2015/12/03 23:46:52, ddorwin wrote:
> nit: missing period

Done.

[ddorwin, 2016-01-04 23:22:18]

Through mime_util.cc LG but pending the build flag change. I also reviewed the
rest of the files this time.

https://codereview.chromium.org/812643005/diff/480001/media/ffmpeg/ffmpeg_com...
File media/ffmpeg/ffmpeg_common.cc (right):

https://codereview.chromium.org/812643005/diff/480001/media/ffmpeg/ffmpeg_com...
media/ffmpeg/ffmpeg_common.cc:310: // For AC3/EAC3, FFmpeg does not fill
|sample_fmt|.
Is this a bug that should be fixed upstream or because the decoder is not
enabled (see 322)?

https://codereview.chromium.org/812643005/diff/480001/media/ffmpeg/ffmpeg_com...
media/ffmpeg/ffmpeg_common.cc:313: ? kSampleFormatS16
How do we know S16 is always correct?

https://codereview.chromium.org/812643005/diff/480001/media/ffmpeg/ffmpeg_com...
media/ffmpeg/ffmpeg_common.cc:321: if (codec == kCodecOpus) {
It looks like we handle exceptions here. I suggest doing the same for your new
exception and possibly converting this to a switch statement.

https://codereview.chromium.org/812643005/diff/480001/media/ffmpeg/ffmpeg_com...
media/ffmpeg/ffmpeg_common.cc:364: if (codec != kCodecOpus && codec != kCodecAC3
&& codec != kCodecEAC3) {
I assume this is for the same reasons. Perhaps reference this in a comment.

A switch statement might be better too.

https://codereview.chromium.org/812643005/diff/480001/media/filters/ffmpeg_de...
File media/filters/ffmpeg_demuxer_unittest.cc (right):

https://codereview.chromium.org/812643005/diff/480001/media/filters/ffmpeg_de...
media/filters/ffmpeg_demuxer_unittest.cc:1131: #if
defined(ENABLE_AC3_EAC3_AUDIO_DEMUXING)
Instead, run the tests and ensure they fail gracefully when not defined.

https://codereview.chromium.org/812643005/diff/480001/media/filters/stream_pa...
File media/filters/stream_parser_factory.cc (right):

https://codereview.chromium.org/812643005/diff/480001/media/filters/stream_pa...
media/filters/stream_parser_factory.cc:232: MatchPattern(codec_id,
kAC3CodecInfo2.pattern) ||
We could do direct string comparisons, but that moves some detailed knowledge
here. Since we're pattern matching, can we indicate that the 'A' is
case-insensitive and eliminate an extra codec info for each?

https://codereview.chromium.org/812643005/diff/480001/media/formats/mp4/es_de...
File media/formats/mp4/es_descriptor.h (right):

https://codereview.chromium.org/812643005/diff/480001/media/formats/mp4/es_de...
media/formats/mp4/es_descriptor.h:24: kISO_14496_3 = 0x40,         // MPEG4 AAC
Are these complex names the best option? I wonder why this one doesn't contain
"AAC" like the next one.

https://codereview.chromium.org/812643005/diff/480001/media/formats/mp4/es_de...
media/formats/mp4/es_descriptor.h:26: kAC3 = 0xa5,                 // AC3
Why do the names above have complex names while these have simple names? Is
there no ISO for them?

https://codereview.chromium.org/812643005/diff/480001/media/formats/mp4/fourc...
File media/formats/mp4/fourccs.h (right):

https://codereview.chromium.org/812643005/diff/480001/media/formats/mp4/fourc...
media/formats/mp4/fourccs.h:21: FOURCC_A52b = 0x61353262,  // "a52b"
"eac3" is never used? Is there a reference for these?

https://codereview.chromium.org/812643005/diff/480001/media/formats/mp4/fourc...
media/formats/mp4/fourccs.h:22: FOURCC_EAC3 = 0x65632d33,  // "ec-3"
For consistency, this would be EAC_3.

https://codereview.chromium.org/812643005/diff/480001/media/formats/mp4/mp4_s...
File media/formats/mp4/mp4_stream_parser.cc (right):

https://codereview.chromium.org/812643005/diff/480001/media/formats/mp4/mp4_s...
media/formats/mp4/mp4_stream_parser.cc:220: FourCC audio_format = (entry.format
== FOURCC_ENCA)
If you can provide some context on this (basically, why ENCA is different), that
would be great.

https://codereview.chromium.org/812643005/diff/480001/media/formats/mp4/mp4_s...
media/formats/mp4/mp4_stream_parser.cc:222: : entry.format;
nit: Empty line after since this is it's own chunk of logic and separate from
the following block.

https://codereview.chromium.org/812643005/diff/480001/media/formats/mp4/mp4_s...
media/formats/mp4/mp4_stream_parser.cc:224: if (audio_format != FOURCC_MP4A &&
audio_format != FOURCC_A52 &&
I think a switch statement will be cleaner and avoid code duplication.

https://codereview.chromium.org/812643005/diff/480001/media/formats/mp4/mp4_s...
media/formats/mp4/mp4_stream_parser.cc:225: audio_format != FOURCC_A52b &&
audio_format != FOURCC_AC3 &&
I see AC3 and EAC3 values are mixed here. Should they be kept together?

https://codereview.chromium.org/812643005/diff/480001/media/formats/mp4/mp4_s...
media/formats/mp4/mp4_stream_parser.cc:238: if (audio_type == kForbidden &&
No need to duplicate the kForbidden check. Put everything else inside the if
block. Can you explain what kFobidden means in this case?

https://codereview.chromium.org/812643005/diff/480001/media/formats/mp4/mp4_s...
media/formats/mp4/mp4_stream_parser.cc:239: (audio_format == FOURCC_EAC3 ||
audio_format == FOURCC_A52b)) {
With the change above, a switch statement probably makes sense.

https://codereview.chromium.org/812643005/diff/480001/media/formats/mp4/mp4_s...
media/formats/mp4/mp4_stream_parser.cc:243: (audio_format == FOURCC_A52 ||
audio_format == FOURCC_AC3 ||
AC3 has been before EAC3 except here.

https://codereview.chromium.org/812643005/diff/480001/media/formats/mp4/mp4_s...
File media/formats/mp4/mp4_stream_parser_unittest.cc (right):

https://codereview.chromium.org/812643005/diff/480001/media/formats/mp4/mp4_s...
media/formats/mp4/mp4_stream_parser_unittest.cc:299: #if
defined(ENABLE_HEVC_DEMUXING)
Ditto (below).

https://codereview.chromium.org/812643005/diff/480001/media/formats/mp4/mp4_s...
media/formats/mp4/mp4_stream_parser_unittest.cc:341: #if
defined(ENABLE_AC3_EAC3_AUDIO_DEMUXING)
As before, can we expect a graceful failure when not enabled?

https://codereview.chromium.org/812643005/diff/480001/media/formats/mp4/mp4_s...
media/formats/mp4/mp4_stream_parser_unittest.cc:346:
ParseMP4File("bear-ac3-frag.mp4", 512);
Comments on test files:

* Do these files have video? If so, include the video codec, if not, consider
adding "-only" like bear-mpeg2-aac-only_frag.mp4.
* Please update the README with how they were created.

https://codereview.chromium.org/812643005/diff/480001/media/media.gyp
File media/media.gyp (right):

https://codereview.chromium.org/812643005/diff/480001/media/media.gyp#newcode40
media/media.gyp:40: # Enable AC3/EAC3 audio demuxing. Actual decoding must be
provided by the
80 chars

[ddorwin, 2016-01-04 23:23:08]

P.S. Please don't forget about the four or so items in PS 23 comments to be done
in another CL.

[servolk, 2016-01-07 02:30:04]

https://codereview.chromium.org/812643005/diff/440001/media/base/audio_decode...
File media/base/audio_decoder_config.cc (right):

https://codereview.chromium.org/812643005/diff/440001/media/base/audio_decode...
media/base/audio_decoder_config.cc:119: case kCodecPCM_ALAW:
On 2015/12/21 17:50:57, servolk wrote:
> On 2015/12/03 23:46:52, ddorwin wrote:
> > It looks like this one is out of order. Hmm. I wonder if others are as well.
> 
> Yep, some of these are out of order, but I'd rather keep this CL limited to
> AC3/EAC3 changes, so not changing it for now.

https://codereview.chromium.org/1563893003

https://codereview.chromium.org/812643005/diff/440001/media/base/mime_util.cc
File media/base/mime_util.cc (right):

https://codereview.chromium.org/812643005/diff/440001/media/base/mime_util.cc...
media/base/mime_util.cc:152: case MimeUtil::HEVC_MAIN:
On 2015/12/21 17:50:57, servolk wrote:
> On 2015/12/03 23:46:52, ddorwin wrote:
> > This should have been added at the end.
> 
> Will do in a separate CL.

https://codereview.chromium.org/1563893003

https://codereview.chromium.org/812643005/diff/440001/media/base/mime_util.cc...
media/base/mime_util.cc:293: // avc1/avc3.XXXXXX may be unambiguous; handled by
ParseH264CodecID().
On 2015/12/21 17:50:57, servolk wrote:
> On 2015/12/03 23:46:52, ddorwin wrote:
> > Please add a similar comment for HEVC. (It's missing from this list without
> > explanation.)
> 
> Will do in a separate CL. I'd prefer this CL to have only AC3/EAC3 changes, to
> make it easier for us to cherry-pick it into Chromecast.

https://codereview.chromium.org/1563893003

https://codereview.chromium.org/812643005/diff/480001/media/ffmpeg/ffmpeg_com...
File media/ffmpeg/ffmpeg_common.cc (right):

https://codereview.chromium.org/812643005/diff/480001/media/ffmpeg/ffmpeg_com...
media/ffmpeg/ffmpeg_common.cc:310: // For AC3/EAC3, FFmpeg does not fill
|sample_fmt|.
On 2016/01/04 23:22:17, ddorwin wrote:
> Is this a bug that should be fixed upstream or because the decoder is not
> enabled (see 322)?

That's because we don't enable decoder, to avoid any potential licensing
issues/fee. We enable only demuxing and pass demuxed data to underlying platform
and eventually to HDMI sink, the actual decoding/playback happens there.
Adjusted the comment to explain that.

https://codereview.chromium.org/812643005/diff/480001/media/ffmpeg/ffmpeg_com...
media/ffmpeg/ffmpeg_common.cc:313: ? kSampleFormatS16
On 2016/01/04 23:22:17, ddorwin wrote:
> How do we know S16 is always correct?

The spec for AC3/EAC3 audio is ETSI TS 102 366:
http://www.etsi.org/deliver/etsi_ts/102300_102399/102366/01.03.01_60/ts_10236...
Sections F.3.1 and F.5.1 there indicate that SampleSize should be 16.

https://codereview.chromium.org/812643005/diff/480001/media/ffmpeg/ffmpeg_com...
media/ffmpeg/ffmpeg_common.cc:321: if (codec == kCodecOpus) {
On 2016/01/04 23:22:17, ddorwin wrote:
> It looks like we handle exceptions here. I suggest doing the same for your new
> exception and possibly converting this to a switch statement.

Done.

https://codereview.chromium.org/812643005/diff/480001/media/ffmpeg/ffmpeg_com...
media/ffmpeg/ffmpeg_common.cc:364: if (codec != kCodecOpus && codec != kCodecAC3
&& codec != kCodecEAC3) {
On 2016/01/04 23:22:17, ddorwin wrote:
> I assume this is for the same reasons. Perhaps reference this in a comment.
> 
> A switch statement might be better too.

I think a switch would be awkward here, it would have to look like:
switch(codec) {
case kOpus:
case kCodecAC3:
case kCodecEAC3:
break;

default:
DCHECK...
}
}

Much more verbose than this if.

https://codereview.chromium.org/812643005/diff/480001/media/filters/stream_pa...
File media/filters/stream_parser_factory.cc (right):

https://codereview.chromium.org/812643005/diff/480001/media/filters/stream_pa...
media/filters/stream_parser_factory.cc:232: MatchPattern(codec_id,
kAC3CodecInfo2.pattern) ||
On 2016/01/04 23:22:17, ddorwin wrote:
> We could do direct string comparisons, but that moves some detailed knowledge
> here. Since we're pattern matching, can we indicate that the 'A' is
> case-insensitive and eliminate an extra codec info for each?

Looking at
https://code.google.com/p/chromium/codesearch#chromium/src/base/strings/patte...
I can see that MatchPattern supports wildcards, but not regexps, so I'm not sure
how to implement a case-insensitive check for just one letter here using
MatchPattern.

https://codereview.chromium.org/812643005/diff/480001/media/formats/mp4/es_de...
File media/formats/mp4/es_descriptor.h (right):

https://codereview.chromium.org/812643005/diff/480001/media/formats/mp4/es_de...
media/formats/mp4/es_descriptor.h:24: kISO_14496_3 = 0x40,         // MPEG4 AAC
On 2016/01/04 23:22:17, ddorwin wrote:
> Are these complex names the best option? I wonder why this one doesn't contain
> "AAC" like the next one.

I don't know. Git blame tells me these were added by acolwell@ and ycheo@ a
while ago. My only change in these lines is in adjusting the indentation of the
comments (and that was done automatically by  'git cl format'). I can change
these, if you insist, but in a separate CL, to keep this CL purely about
AC3/EAC3.

https://codereview.chromium.org/812643005/diff/480001/media/formats/mp4/es_de...
media/formats/mp4/es_descriptor.h:26: kAC3 = 0xa5,                 // AC3
On 2016/01/04 23:22:17, ddorwin wrote:
> Why do the names above have complex names while these have simple names? Is
> there no ISO for them?

I believe the standard that describes AC3/EAC3 audio is ETSI TS 102 366, so we
could name those kETSI_TS_102_366_AC3 and kETSI_TS_102_366_EAC3, if you prefer.
Those shorted names were chosen by damienv@, I'm just upstreaming this from
Chromecast tree.

https://codereview.chromium.org/812643005/diff/480001/media/formats/mp4/fourc...
File media/formats/mp4/fourccs.h (right):

https://codereview.chromium.org/812643005/diff/480001/media/formats/mp4/fourc...
media/formats/mp4/fourccs.h:21: FOURCC_A52b = 0x61353262,  // "a52b"
On 2016/01/04 23:22:17, ddorwin wrote:
> "eac3" is never used? Is there a reference for these?

AFAIK, no "eac3" is never used. I've found out that ETSI TS 102 366 specifies
"ac-3" for AC3 and "ec-3" for EAC3 (also confirmed at
http://www.mp4ra.org/codecs.html). I believe a52/a52b are older legacy names for
AC3/EAC3 respectively. As for "ac3 " - I believe this is also unnecessary, since
we support "ac-3". So I'll leave only "ac-3" and "ec-3" here.

https://codereview.chromium.org/812643005/diff/480001/media/formats/mp4/fourc...
media/formats/mp4/fourccs.h:22: FOURCC_EAC3 = 0x65632d33,  // "ec-3"
On 2016/01/04 23:22:17, ddorwin wrote:
> For consistency, this would be EAC_3.

Done.

https://codereview.chromium.org/812643005/diff/480001/media/formats/mp4/mp4_s...
File media/formats/mp4/mp4_stream_parser.cc (right):

https://codereview.chromium.org/812643005/diff/480001/media/formats/mp4/mp4_s...
media/formats/mp4/mp4_stream_parser.cc:220: FourCC audio_format = (entry.format
== FOURCC_ENCA)
On 2016/01/04 23:22:17, ddorwin wrote:
> If you can provide some context on this (basically, why ENCA is different),
that
> would be great.

FOURCC_ENCA just means that audio stream is encrypted and actual format is in
entry.sinf.format.format. Added a comment.

https://codereview.chromium.org/812643005/diff/480001/media/formats/mp4/mp4_s...
media/formats/mp4/mp4_stream_parser.cc:222: : entry.format;
On 2016/01/04 23:22:17, ddorwin wrote:
> nit: Empty line after since this is it's own chunk of logic and separate from
> the following block.

Done.

https://codereview.chromium.org/812643005/diff/480001/media/formats/mp4/mp4_s...
media/formats/mp4/mp4_stream_parser.cc:224: if (audio_format != FOURCC_MP4A &&
audio_format != FOURCC_A52 &&
On 2016/01/04 23:22:17, ddorwin wrote:
> I think a switch statement will be cleaner and avoid code duplication.

Now that I've removed a52/a52b the ifs are only four lines and fairly readable,
switch would be more verbose than that.

https://codereview.chromium.org/812643005/diff/480001/media/formats/mp4/mp4_s...
media/formats/mp4/mp4_stream_parser.cc:225: audio_format != FOURCC_A52b &&
audio_format != FOURCC_AC3 &&
On 2016/01/04 23:22:17, ddorwin wrote:
> I see AC3 and EAC3 values are mixed here. Should they be kept together?

Now, with only one value for AC3/EAC3 each, this looks cleaner.

https://codereview.chromium.org/812643005/diff/480001/media/formats/mp4/mp4_s...
media/formats/mp4/mp4_stream_parser.cc:238: if (audio_type == kForbidden &&
On 2016/01/04 23:22:17, ddorwin wrote:
> No need to duplicate the kForbidden check. Put everything else inside the if
> block. Can you explain what kFobidden means in this case?

kForbidden means the mp4 parser used by MSE doesn't recognize this format and
that's fine, since we only need to be able to demux it to implement audio
pass-through.

https://codereview.chromium.org/812643005/diff/480001/media/formats/mp4/mp4_s...
media/formats/mp4/mp4_stream_parser.cc:239: (audio_format == FOURCC_EAC3 ||
audio_format == FOURCC_A52b)) {
On 2016/01/04 23:22:17, ddorwin wrote:
> With the change above, a switch statement probably makes sense.

Now that we have only two values, I believe if should be cleaner.

https://codereview.chromium.org/812643005/diff/480001/media/formats/mp4/mp4_s...
media/formats/mp4/mp4_stream_parser.cc:243: (audio_format == FOURCC_A52 ||
audio_format == FOURCC_AC3 ||
On 2016/01/04 23:22:17, ddorwin wrote:
> AC3 has been before EAC3 except here.

Fixed the ordering above. AC3 should be before EAC3.

https://codereview.chromium.org/812643005/diff/480001/media/formats/mp4/mp4_s...
File media/formats/mp4/mp4_stream_parser_unittest.cc (right):

https://codereview.chromium.org/812643005/diff/480001/media/formats/mp4/mp4_s...
media/formats/mp4/mp4_stream_parser_unittest.cc:299: #if
defined(ENABLE_HEVC_DEMUXING)
On 2016/01/04 23:22:18, ddorwin wrote:
> Ditto (below).

Acknowledged.

https://codereview.chromium.org/812643005/diff/480001/media/formats/mp4/mp4_s...
media/formats/mp4/mp4_stream_parser_unittest.cc:341: #if
defined(ENABLE_AC3_EAC3_AUDIO_DEMUXING)
On 2016/01/04 23:22:18, ddorwin wrote:
> As before, can we expect a graceful failure when not enabled?

No other tests do that, so currently the helper classes are structured in a way
that don't make it easy. Besides - how do we define 'graceful failure'? Failure
anywhere in the code, but not a crash? Or failure at a specific place in the
media stack with a specific error code?
I'd prefer to make that a separate change.

https://codereview.chromium.org/812643005/diff/480001/media/formats/mp4/mp4_s...
media/formats/mp4/mp4_stream_parser_unittest.cc:346:
ParseMP4File("bear-ac3-frag.mp4", 512);
On 2016/01/04 23:22:18, ddorwin wrote:
> Comments on test files:
> 
> * Do these files have video? If so, include the video codec, if not, consider
> adding "-only" like bear-mpeg2-aac-only_frag.mp4.
> * Please update the README with how they were created.

No, these are audio-only files, renamed. Also updated README with ffmpeg command
lines.

https://codereview.chromium.org/812643005/diff/480001/media/media.gyp
File media/media.gyp (right):

https://codereview.chromium.org/812643005/diff/480001/media/media.gyp#newcode40
media/media.gyp:40: # Enable AC3/EAC3 audio demuxing. Actual decoding must be
provided by the
On 2016/01/04 23:22:18, ddorwin wrote:
> 80 chars

Do gyp files also have 80-char limit? I thought that was only for source code.
There's a lot of other lines much longer than 80 chars in this file. e.g. take a
look at lines 16, 30, 841, etc.

[ddorwin, 2016-01-07 19:12:43]

LG. Are you going to convert the build flags before landing?

https://codereview.chromium.org/812643005/diff/480001/media/filters/stream_pa...
File media/filters/stream_parser_factory.cc (right):

https://codereview.chromium.org/812643005/diff/480001/media/filters/stream_pa...
media/filters/stream_parser_factory.cc:232: MatchPattern(codec_id,
kAC3CodecInfo2.pattern) ||
On 2016/01/07 02:30:04, servolk wrote:
> On 2016/01/04 23:22:17, ddorwin wrote:
> > We could do direct string comparisons, but that moves some detailed
knowledge
> > here. Since we're pattern matching, can we indicate that the 'A' is
> > case-insensitive and eliminate an extra codec info for each?
> 
> Looking at
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/strings/patte...
> I can see that MatchPattern supports wildcards, but not regexps, so I'm not
sure
> how to implement a case-insensitive check for just one letter here using
> MatchPattern.

Acknowledged.

https://codereview.chromium.org/812643005/diff/480001/media/formats/mp4/es_de...
File media/formats/mp4/es_descriptor.h (right):

https://codereview.chromium.org/812643005/diff/480001/media/formats/mp4/es_de...
media/formats/mp4/es_descriptor.h:26: kAC3 = 0xa5,                 // AC3
On 2016/01/07 02:30:04, servolk wrote:
> On 2016/01/04 23:22:17, ddorwin wrote:
> > Why do the names above have complex names while these have simple names? Is
> > there no ISO for them?
> 
> I believe the standard that describes AC3/EAC3 audio is ETSI TS 102 366, so we
> could name those kETSI_TS_102_366_AC3 and kETSI_TS_102_366_EAC3, if you
prefer.
> Those shorted names were chosen by damienv@, I'm just upstreaming this from
> Chromecast tree.

I think we should be consistent, though I'm not wild about the existing names.
dalecurtis@, WDYT?

https://codereview.chromium.org/812643005/diff/480001/media/formats/mp4/mp4_s...
File media/formats/mp4/mp4_stream_parser_unittest.cc (right):

https://codereview.chromium.org/812643005/diff/480001/media/formats/mp4/mp4_s...
media/formats/mp4/mp4_stream_parser_unittest.cc:341: #if
defined(ENABLE_AC3_EAC3_AUDIO_DEMUXING)
On 2016/01/07 02:30:04, servolk wrote:
> On 2016/01/04 23:22:18, ddorwin wrote:
> > As before, can we expect a graceful failure when not enabled?
> 
> No other tests do that, so currently the helper classes are structured in a
way
> that don't make it easy. Besides - how do we define 'graceful failure'?
Failure
> anywhere in the code, but not a crash? Or failure at a specific place in the
> media stack with a specific error code?
> I'd prefer to make that a separate change.

Failure without crashing or hanging (timeout).

https://codereview.chromium.org/812643005/diff/580001/media/ffmpeg/ffmpeg_com...
File media/ffmpeg/ffmpeg_common.cc (right):

https://codereview.chromium.org/812643005/diff/580001/media/ffmpeg/ffmpeg_com...
media/ffmpeg/ffmpeg_common.cc:332: // For AC3/EAC3 we enable only demuxing, but
not decoding (due to licensing
For a variety of reasons, including that we don't have a complex matrix of
"ffmpeg_branding" options. I think you can just drop the ().

https://codereview.chromium.org/812643005/diff/580001/media/ffmpeg/ffmpeg_com...
media/ffmpeg/ffmpeg_common.cc:334: if (codec == kCodecAC3 || codec ==
kCodecEAC3) {
This should be else if.

https://codereview.chromium.org/812643005/diff/580001/media/ffmpeg/ffmpeg_com...
media/ffmpeg/ffmpeg_common.cc:338: }
Should this code be ifdef'd or should we have this "dead" code in all builds?

How is this different from line 70-75?

https://codereview.chromium.org/812643005/diff/580001/media/ffmpeg/ffmpeg_com...
media/ffmpeg/ffmpeg_common.cc:369: if (codec != kCodecOpus && codec != kCodecAC3
&& codec != kCodecEAC3) {
Suggest:
// Verify XXX for codecs for which FFmpeg provides a sample_fmt.

https://codereview.chromium.org/812643005/diff/580001/media/formats/mp4/fourc...
File media/formats/mp4/fourccs.h (right):

https://codereview.chromium.org/812643005/diff/580001/media/formats/mp4/fourc...
media/formats/mp4/fourccs.h:16: FOURCC_AC_3 = 0x61632d33,   // "ac-3"
You can drop the second '_' now. It was only necessary when there were multiple
variants for each.

https://codereview.chromium.org/812643005/diff/580001/media/formats/mp4/mp4_s...
File media/formats/mp4/mp4_stream_parser.cc (right):

https://codereview.chromium.org/812643005/diff/580001/media/formats/mp4/mp4_s...
media/formats/mp4/mp4_stream_parser.cc:243: if (audio_type == kForbidden) {
Does "forbidden" have a special meaning or would "unsupported" or similar be
better?

https://codereview.chromium.org/812643005/diff/580001/media/media.gyp
File media/media.gyp (right):

https://codereview.chromium.org/812643005/diff/580001/media/media.gyp#newcode...
media/media.gyp:1115: 'ENABLE_AC3_EAC3_AUDIO_DEMUXING'
Should we try to keep the variable and the define names consistent?

[servolk, 2016-01-07 21:17:36]

Conversion to buildflag is done in patchset 32 (I had to wait before the mpeg2ts
flag conversion CL lands, to avoid conflicts).

https://codereview.chromium.org/812643005/diff/480001/media/formats/mp4/mp4_s...
File media/formats/mp4/mp4_stream_parser_unittest.cc (right):

https://codereview.chromium.org/812643005/diff/480001/media/formats/mp4/mp4_s...
media/formats/mp4/mp4_stream_parser_unittest.cc:341: #if
defined(ENABLE_AC3_EAC3_AUDIO_DEMUXING)
On 2016/01/07 19:12:43, ddorwin wrote:
> On 2016/01/07 02:30:04, servolk wrote:
> > On 2016/01/04 23:22:18, ddorwin wrote:
> > > As before, can we expect a graceful failure when not enabled?
> > 
> > No other tests do that, so currently the helper classes are structured in a
> way
> > that don't make it easy. Besides - how do we define 'graceful failure'?
> Failure
> > anywhere in the code, but not a crash? Or failure at a specific place in the
> > media stack with a specific error code?
> > I'd prefer to make that a separate change.
> 
> Failure without crashing or hanging (timeout).

Ok, I'll see if we can do this easily here.

https://codereview.chromium.org/812643005/diff/580001/media/ffmpeg/ffmpeg_com...
File media/ffmpeg/ffmpeg_common.cc (right):

https://codereview.chromium.org/812643005/diff/580001/media/ffmpeg/ffmpeg_com...
media/ffmpeg/ffmpeg_common.cc:332: // For AC3/EAC3 we enable only demuxing, but
not decoding (due to licensing
On 2016/01/07 19:12:43, ddorwin wrote:
> For a variety of reasons, including that we don't have a complex matrix of
> "ffmpeg_branding" options. I think you can just drop the ().

Done.

https://codereview.chromium.org/812643005/diff/580001/media/ffmpeg/ffmpeg_com...
media/ffmpeg/ffmpeg_common.cc:334: if (codec == kCodecAC3 || codec ==
kCodecEAC3) {
On 2016/01/07 19:12:43, ddorwin wrote:
> This should be else if.

Even if we wrap it into #if as suggested below? I think the difference between
if/'else if' in this case is really minor and I'd rather have a stand-alone
piece of code inside #if, rather than relying on another if to be above it.

https://codereview.chromium.org/812643005/diff/580001/media/ffmpeg/ffmpeg_com...
media/ffmpeg/ffmpeg_common.cc:338: }
On 2016/01/07 19:12:43, ddorwin wrote:
> Should this code be ifdef'd or should we have this "dead" code in all builds?
> 
> How is this different from line 70-75?

The difference is really minor, I think lines 70-75 were wrapped in #if to
guarantee CodecIDToAudioCodec never returns kCodecAC3/EAC3 for builds where
AC3/EAC3 are disabled, whereas this code would simply be dead, but harmless even
if somehow it got executed on the builds with AC3/EAC3 disabled. But I agree we
can wrap this into #if as well, it shouldn't make any difference.

https://codereview.chromium.org/812643005/diff/580001/media/ffmpeg/ffmpeg_com...
media/ffmpeg/ffmpeg_common.cc:369: if (codec != kCodecOpus && codec != kCodecAC3
&& codec != kCodecEAC3) {
On 2016/01/07 19:12:43, ddorwin wrote:
> Suggest:
> // Verify XXX for codecs for which FFmpeg provides a sample_fmt.

Done.

https://codereview.chromium.org/812643005/diff/580001/media/formats/mp4/fourc...
File media/formats/mp4/fourccs.h (right):

https://codereview.chromium.org/812643005/diff/580001/media/formats/mp4/fourc...
media/formats/mp4/fourccs.h:16: FOURCC_AC_3 = 0x61632d33,   // "ac-3"
On 2016/01/07 19:12:43, ddorwin wrote:
> You can drop the second '_' now. It was only necessary when there were
multiple
> variants for each.

Done.

https://codereview.chromium.org/812643005/diff/580001/media/formats/mp4/mp4_s...
File media/formats/mp4/mp4_stream_parser.cc (right):

https://codereview.chromium.org/812643005/diff/580001/media/formats/mp4/mp4_s...
media/formats/mp4/mp4_stream_parser.cc:243: if (audio_type == kForbidden) {
On 2016/01/07 19:12:43, ddorwin wrote:
> Does "forbidden" have a special meaning or would "unsupported" or similar be
> better?

AFAIK no special meaning, it was just a name chosen for ESDS.object_type enum.
We can rename into kUnsupported or kUnknown in a separate CL.

https://codereview.chromium.org/812643005/diff/580001/media/media.gyp
File media/media.gyp (right):

https://codereview.chromium.org/812643005/diff/580001/media/media.gyp#newcode...
media/media.gyp:1115: 'ENABLE_AC3_EAC3_AUDIO_DEMUXING'
On 2016/01/07 19:12:43, ddorwin wrote:
> Should we try to keep the variable and the define names consistent?

I've fixed this in the patchset 32 with conversion to buildflag.

[servolk, 2016-01-08 00:53:34]

https://codereview.chromium.org/812643005/diff/480001/media/formats/mp4/mp4_s...
File media/formats/mp4/mp4_stream_parser_unittest.cc (right):

https://codereview.chromium.org/812643005/diff/480001/media/formats/mp4/mp4_s...
media/formats/mp4/mp4_stream_parser_unittest.cc:341: #if
defined(ENABLE_AC3_EAC3_AUDIO_DEMUXING)
On 2016/01/07 21:17:36, servolk wrote:
> On 2016/01/07 19:12:43, ddorwin wrote:
> > On 2016/01/07 02:30:04, servolk wrote:
> > > On 2016/01/04 23:22:18, ddorwin wrote:
> > > > As before, can we expect a graceful failure when not enabled?
> > > 
> > > No other tests do that, so currently the helper classes are structured in
a
> > way
> > > that don't make it easy. Besides - how do we define 'graceful failure'?
> > Failure
> > > anywhere in the code, but not a crash? Or failure at a specific place in
the
> > > media stack with a specific error code?
> > > I'd prefer to make that a separate change.
> > 
> > Failure without crashing or hanging (timeout).
> 
> Ok, I'll see if we can do this easily here.

I guess we could do something like this
https://codereview.chromium.org/1567783005/
And something similar for HEVC. But I'd still prefer this to be a separate CL

[ddorwin, 2016-01-08 18:11:02]

LGTM with a few comments. Thanks.

https://codereview.chromium.org/812643005/diff/680001/media/ffmpeg/ffmpeg_com...
File media/ffmpeg/ffmpeg_common.cc (right):

https://codereview.chromium.org/812643005/diff/680001/media/ffmpeg/ffmpeg_com...
media/ffmpeg/ffmpeg_common.cc:374: if (codec != kCodecOpus && codec != kCodecAC3
&& codec != kCodecEAC3) {
That leaves this as the only code in this file not protected. We should be
consistent either way, though at least this would be optimized out of release
builds.

https://codereview.chromium.org/812643005/diff/680001/media/formats/mp4/es_de...
File media/formats/mp4/es_descriptor.h (right):

https://codereview.chromium.org/812643005/diff/680001/media/formats/mp4/es_de...
media/formats/mp4/es_descriptor.h:24: kISO_14496_3 = 0x40,         // MPEG4 AAC
dalecurtis agreed that just calling it AAC sounds better. We could move the spec
to the comment if it is useful.

This doesn't need to be done in this CL, but we should be consistent.

https://codereview.chromium.org/812643005/diff/680001/media/media.gyp
File media/media.gyp (right):

https://codereview.chromium.org/812643005/diff/680001/media/media.gyp#newcode37
media/media.gyp:37: # Enable AC3/EAC3 audio demuxing. Actual decoding must be
provided by the
nit: Regarding line lengths: If it can be done and you are already breaking up
the line, I think it's better to try. Being one character off looks wrong. In
this case, I think "the " will fit below.

https://codereview.chromium.org/812643005/diff/680001/media/media_options.gni
File media/media_options.gni (right):

https://codereview.chromium.org/812643005/diff/680001/media/media_options.gni...
media/media_options.gni:54: enable_ac3_eac3_audio_demuxing = is_chromecast
I wonder whether we should add "proprietary_codecs &&" to these. This is always
the case for Chromecast, but it would make it clear that there is a dependency
in the build files and code.

[servolk, 2016-01-08 20:02:33]

https://codereview.chromium.org/812643005/diff/680001/media/ffmpeg/ffmpeg_com...
File media/ffmpeg/ffmpeg_common.cc (right):

https://codereview.chromium.org/812643005/diff/680001/media/ffmpeg/ffmpeg_com...
media/ffmpeg/ffmpeg_common.cc:374: if (codec != kCodecOpus && codec != kCodecAC3
&& codec != kCodecEAC3) {
On 2016/01/08 18:11:02, ddorwin wrote:
> That leaves this as the only code in this file not protected. We should be
> consistent either way, though at least this would be optimized out of release
> builds.

Done (and converted to switch to avoid splitting the if condition with #if
buildflag(ac3) )

https://codereview.chromium.org/812643005/diff/680001/media/formats/mp4/es_de...
File media/formats/mp4/es_descriptor.h (right):

https://codereview.chromium.org/812643005/diff/680001/media/formats/mp4/es_de...
media/formats/mp4/es_descriptor.h:24: kISO_14496_3 = 0x40,         // MPEG4 AAC
On 2016/01/08 18:11:02, ddorwin wrote:
> dalecurtis agreed that just calling it AAC sounds better. We could move the
spec
> to the comment if it is useful.
> 
> This doesn't need to be done in this CL, but we should be consistent.

Ok, will do in a separate CL.

https://codereview.chromium.org/812643005/diff/680001/media/media.gyp
File media/media.gyp (right):

https://codereview.chromium.org/812643005/diff/680001/media/media.gyp#newcode37
media/media.gyp:37: # Enable AC3/EAC3 audio demuxing. Actual decoding must be
provided by the
On 2016/01/08 18:11:02, ddorwin wrote:
> nit: Regarding line lengths: If it can be done and you are already breaking up
> the line, I think it's better to try. Being one character off looks wrong. In
> this case, I think "the " will fit below.

Done.

https://codereview.chromium.org/812643005/diff/680001/media/media_options.gni
File media/media_options.gni (right):

https://codereview.chromium.org/812643005/diff/680001/media/media_options.gni...
media/media_options.gni:54: enable_ac3_eac3_audio_demuxing = is_chromecast
On 2016/01/08 18:11:02, ddorwin wrote:
> I wonder whether we should add "proprietary_codecs &&" to these. This is
always
> the case for Chromecast, but it would make it clear that there is a dependency
> in the build files and code.

Done (I've placed is_chromecast first to make it more prominent)

[servolk, 2016-01-08 20:06:23]

servolk@chromium.org changed reviewers:
+ asvitkine@chromium.org

[servolk, 2016-01-08 20:06:43]

On 2016/01/08 20:02:33, servolk wrote:
>
https://codereview.chromium.org/812643005/diff/680001/media/ffmpeg/ffmpeg_com...
> File media/ffmpeg/ffmpeg_common.cc (right):
> 
>
https://codereview.chromium.org/812643005/diff/680001/media/ffmpeg/ffmpeg_com...
> media/ffmpeg/ffmpeg_common.cc:374: if (codec != kCodecOpus && codec !=
kCodecAC3
> && codec != kCodecEAC3) {
> On 2016/01/08 18:11:02, ddorwin wrote:
> > That leaves this as the only code in this file not protected. We should be
> > consistent either way, though at least this would be optimized out of
release
> > builds.
> 
> Done (and converted to switch to avoid splitting the if condition with #if
> buildflag(ac3) )
> 
>
https://codereview.chromium.org/812643005/diff/680001/media/formats/mp4/es_de...
> File media/formats/mp4/es_descriptor.h (right):
> 
>
https://codereview.chromium.org/812643005/diff/680001/media/formats/mp4/es_de...
> media/formats/mp4/es_descriptor.h:24: kISO_14496_3 = 0x40,         // MPEG4
AAC
> On 2016/01/08 18:11:02, ddorwin wrote:
> > dalecurtis agreed that just calling it AAC sounds better. We could move the
> spec
> > to the comment if it is useful.
> > 
> > This doesn't need to be done in this CL, but we should be consistent.
> 
> Ok, will do in a separate CL.
> 
> https://codereview.chromium.org/812643005/diff/680001/media/media.gyp
> File media/media.gyp (right):
> 
>
https://codereview.chromium.org/812643005/diff/680001/media/media.gyp#newcode37
> media/media.gyp:37: # Enable AC3/EAC3 audio demuxing. Actual decoding must be
> provided by the
> On 2016/01/08 18:11:02, ddorwin wrote:
> > nit: Regarding line lengths: If it can be done and you are already breaking
up
> > the line, I think it's better to try. Being one character off looks wrong.
In
> > this case, I think "the " will fit below.
> 
> Done.
> 
> https://codereview.chromium.org/812643005/diff/680001/media/media_options.gni
> File media/media_options.gni (right):
> 
>
https://codereview.chromium.org/812643005/diff/680001/media/media_options.gni...
> media/media_options.gni:54: enable_ac3_eac3_audio_demuxing = is_chromecast
> On 2016/01/08 18:11:02, ddorwin wrote:
> > I wonder whether we should add "proprietary_codecs &&" to these. This is
> always
> > the case for Chromecast, but it would make it clear that there is a
dependency
> > in the build files and code.
> 
> Done (I've placed is_chromecast first to make it more prominent)

+asvitkine@ for tools/metrics/histograms/histograms.xml approval

[Alexei Svitkine (slow), 2016-01-08 20:11:37]

histograms.xml lgtm - didn't look elsewhere

[ddorwin, 2016-01-08 20:15:42]

Thanks.

https://codereview.chromium.org/812643005/diff/700001/media/ffmpeg/ffmpeg_com...
File media/ffmpeg/ffmpeg_common.cc (right):

https://codereview.chromium.org/812643005/diff/700001/media/ffmpeg/ffmpeg_com...
media/ffmpeg/ffmpeg_common.cc:319: if (codec == kCodecOpus) {
I think a switch statement would make sense here too. Even though it's longer,
it would be consistent with 374 and you could use NOTREACHED() if you wanted.

You might also move the default behaviors above into the default: case.

https://codereview.chromium.org/812643005/diff/700001/media/ffmpeg/ffmpeg_com...
media/ffmpeg/ffmpeg_common.cc:383: // Verify that AudioConfig.bits_per_channel
was calculated correctly for
nit: Since we're not doing anything else, it might make sense to move this
comment above 375.

https://codereview.chromium.org/812643005/diff/700001/media/media.gyp
File media/media.gyp (right):

https://codereview.chromium.org/812643005/diff/700001/media/media.gyp#newcode36
media/media.gyp:36: ['chromecast == 1', {
We should keep the conditions consistent with GN.

https://codereview.chromium.org/812643005/diff/700001/media/media_options.gni
File media/media_options.gni (right):

https://codereview.chromium.org/812643005/diff/700001/media/media_options.gni...
media/media_options.gni:54: enable_ac3_eac3_audio_demuxing = is_chromecast &&
proprietary_codecs
The reason I proposed the other way in PS35 is that proprietary_codecs is the
common prerequisite. For example, if another platform was added, it would be
"proprietary_codecs && (is_chromecast || is_foo)". Written the other way,
someone might add "is_foo || ...", which would be wrong.

https://codereview.chromium.org/812643005/diff/700001/media/media_options.gni...
media/media_options.gni:56: enable_mse_mpeg2ts_stream_parser = is_chromecast
I think it's fine to make the same change here in this CL.

[servolk, 2016-01-08 21:55:48]

https://codereview.chromium.org/812643005/diff/700001/media/ffmpeg/ffmpeg_com...
File media/ffmpeg/ffmpeg_common.cc (right):

https://codereview.chromium.org/812643005/diff/700001/media/ffmpeg/ffmpeg_com...
media/ffmpeg/ffmpeg_common.cc:319: if (codec == kCodecOpus) {
On 2016/01/08 20:15:42, ddorwin wrote:
> I think a switch statement would make sense here too. Even though it's longer,
> it would be consistent with 374 and you could use NOTREACHED() if you wanted.
> 
> You might also move the default behaviors above into the default: case.

Done (converted to switch, but I think it's better to leave default behaviors
outside of switch to make it more readable).

https://codereview.chromium.org/812643005/diff/700001/media/ffmpeg/ffmpeg_com...
media/ffmpeg/ffmpeg_common.cc:383: // Verify that AudioConfig.bits_per_channel
was calculated correctly for
On 2016/01/08 20:15:42, ddorwin wrote:
> nit: Since we're not doing anything else, it might make sense to move this
> comment above 375.

Done.

https://codereview.chromium.org/812643005/diff/700001/media/media.gyp
File media/media.gyp (right):

https://codereview.chromium.org/812643005/diff/700001/media/media.gyp#newcode36
media/media.gyp:36: ['chromecast == 1', {
On 2016/01/08 20:15:42, ddorwin wrote:
> We should keep the conditions consistent with GN.

Done.

https://codereview.chromium.org/812643005/diff/700001/media/media_options.gni
File media/media_options.gni (right):

https://codereview.chromium.org/812643005/diff/700001/media/media_options.gni...
media/media_options.gni:54: enable_ac3_eac3_audio_demuxing = is_chromecast &&
proprietary_codecs
On 2016/01/08 20:15:42, ddorwin wrote:
> The reason I proposed the other way in PS35 is that proprietary_codecs is the
> common prerequisite. For example, if another platform was added, it would be
> "proprietary_codecs && (is_chromecast || is_foo)". Written the other way,
> someone might add "is_foo || ...", which would be wrong.

Done.

https://codereview.chromium.org/812643005/diff/700001/media/media_options.gni...
media/media_options.gni:56: enable_mse_mpeg2ts_stream_parser = is_chromecast
On 2016/01/08 20:15:42, ddorwin wrote:
> I think it's fine to make the same change here in this CL.

Done.

[servolk, 2016-01-08 21:56:03]

The CQ bit was checked by servolk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-08 21:58:19]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/812643005/720001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/812643005/720001

[ddorwin, 2016-01-08 22:00:15]

LGTM. Thank you.

[commit-bot: I haz the power, 2016-01-08 22:13:36]

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

[commit-bot: I haz the power, 2016-01-08 22:13:37]

Dry run: Try jobs failed on following builders:
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

[servolk, 2016-01-08 22:24:24]

The CQ bit was checked by servolk@chromium.org

[servolk, 2016-01-08 22:24:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org, ddorwin@chromium.org
Link to the patchset: https://codereview.chromium.org/812643005/#ps740001
(title: "Import //build/config/features.gni in media_options.gni")

[commit-bot: I haz the power, 2016-01-08 22:24:52]

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

[commit-bot: I haz the power, 2016-01-08 22:44:01]

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

[commit-bot: I haz the power, 2016-01-08 22:44:02]

Try jobs failed on following builders:
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

[servolk, 2016-01-08 22:48:51]

The CQ bit was checked by servolk@chromium.org

[servolk, 2016-01-08 22:48:51]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org, ddorwin@chromium.org
Link to the patchset: https://codereview.chromium.org/812643005/#ps760001
(title: "buildfix")

[commit-bot: I haz the power, 2016-01-08 22:51:06]

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

[commit-bot: I haz the power, 2016-01-09 00:39:59]

Description was changed from

==========
Re-add AC3/EAC3 audio demuxing support

Chrome used to have EAC3/DD+ support (crbug.com/215773),
but it was removed last year (crbug.com/334126). This CL
partially restores the old code and adds AC3 support.
This will allow AC3/EAC3 audio demuxing and can be used
for AC3/EAC3 pass-through support.

BUG=448878
==========

to

==========
Re-add AC3/EAC3 audio demuxing support

Chrome used to have EAC3/DD+ support (crbug.com/215773),
but it was removed last year (crbug.com/334126). This CL
partially restores the old code and adds AC3 support.
This will allow AC3/EAC3 audio demuxing and can be used
for AC3/EAC3 pass-through support.

BUG=448878
==========

[commit-bot: I haz the power, 2016-01-09 00:40:00]

Committed patchset #39 (id:760001)

[commit-bot: I haz the power, 2016-01-09 00:40:50]

Description was changed from

==========
Re-add AC3/EAC3 audio demuxing support

Chrome used to have EAC3/DD+ support (crbug.com/215773),
but it was removed last year (crbug.com/334126). This CL
partially restores the old code and adds AC3 support.
This will allow AC3/EAC3 audio demuxing and can be used
for AC3/EAC3 pass-through support.

BUG=448878
==========

to

==========
Re-add AC3/EAC3 audio demuxing support

Chrome used to have EAC3/DD+ support (crbug.com/215773),
but it was removed last year (crbug.com/334126). This CL
partially restores the old code and adds AC3 support.
This will allow AC3/EAC3 audio demuxing and can be used
for AC3/EAC3 pass-through support.

BUG=448878

Committed: https://crrev.com/063ce09474bc035a743aa565f87b7f506db8ff86
Cr-Commit-Position: refs/heads/master@{#368476}
==========

[commit-bot: I haz the power, 2016-01-09 00:40:51]

Patchset 39 (id:??) landed as
https://crrev.com/063ce09474bc035a743aa565f87b7f506db8ff86
Cr-Commit-Position: refs/heads/master@{#368476}