Support EAC3 (Dolby Digital Plus) codec

Support EAC3 (Dolby Digital Plus) codec

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

[ycheo (away), 2013-04-30 13:37:23]

PTAL.

The paired CL in the clank side: https://gerrit-int.chromium.org/#/c/37032/

acolwell: for media/*
sky: for content/browser/*

[sky, 2013-04-30 13:54:44]

LGTM

[acolwell GONE FROM CHROMIUM, 2013-04-30 18:19:02]

Looks pretty good. Just a few comments.

https://codereview.chromium.org/14641006/diff/2001/media/filters/stream_parse...
File media/filters/stream_parser_factory.cc (right):

https://codereview.chromium.org/14641006/diff/2001/media/filters/stream_parse...
media/filters/stream_parser_factory.cc:138: static bool enable_eac3 =
CommandLine::ForCurrentProcess()->HasSwitch(
nit: drop static.

https://codereview.chromium.org/14641006/diff/2001/media/mp4/mp4_stream_parse...
File media/mp4/mp4_stream_parser.cc (right):

https://codereview.chromium.org/14641006/diff/2001/media/mp4/mp4_stream_parse...
media/mp4/mp4_stream_parser.cc:256: entry.channelcount > 0) {
Should this logic be moved above where 'codec' is getting set? It seems a little
odd to query the aac object in the EAC3 case.

https://codereview.chromium.org/14641006/diff/2001/media/mp4/mp4_stream_parse...
media/mp4/mp4_stream_parser.cc:485: if
(runs_->audio_description().esds.object_type != kForbidden &&
nit: How about adding a isAAC() helper method to esds since that is really what
we want to check. It could be used here and above the line that sets codec =
kCodecAAC;

[acolwell GONE FROM CHROMIUM, 2013-04-30 18:43:43]

https://codereview.chromium.org/14641006/diff/2001/media/mp4/mp4_stream_parse...
File media/mp4/mp4_stream_parser.cc (right):

https://codereview.chromium.org/14641006/diff/2001/media/mp4/mp4_stream_parse...
media/mp4/mp4_stream_parser.cc:203: #if defined(ENABLE_EAC3_PLAYBACK)
I think we should just drop the #if's in this file. audio_object_types_ won't
contain kEAC3 if this define isn't enabled because of the #if blocks in
stream_parser_factory. This should cause playback to fail if we happen to
encounter content w/ this object type. This code is pretty small so I don't
think we need to worry about bloat here.

[ycheo (away), 2013-05-01 00:05:45]

Thanks for the comments.

Addressed Aaron's comments.

https://codereview.chromium.org/14641006/diff/2001/media/filters/stream_parse...
File media/filters/stream_parser_factory.cc (right):

https://codereview.chromium.org/14641006/diff/2001/media/filters/stream_parse...
media/filters/stream_parser_factory.cc:138: static bool enable_eac3 =
CommandLine::ForCurrentProcess()->HasSwitch(
On 2013/04/30 18:19:02, acolwell wrote:
> nit: drop static.

Done.

https://codereview.chromium.org/14641006/diff/2001/media/mp4/mp4_stream_parse...
File media/mp4/mp4_stream_parser.cc (right):

https://codereview.chromium.org/14641006/diff/2001/media/mp4/mp4_stream_parse...
media/mp4/mp4_stream_parser.cc:203: #if defined(ENABLE_EAC3_PLAYBACK)
On 2013/04/30 18:43:43, acolwell wrote:
> I think we should just drop the #if's in this file. audio_object_types_ won't
> contain kEAC3 if this define isn't enabled because of the #if blocks in
> stream_parser_factory. This should cause playback to fail if we happen to
> encounter content w/ this object type. This code is pretty small so I don't
> think we need to worry about bloat here.

Done.

https://codereview.chromium.org/14641006/diff/2001/media/mp4/mp4_stream_parse...
media/mp4/mp4_stream_parser.cc:256: entry.channelcount > 0) {
On 2013/04/30 18:19:02, acolwell wrote:
> Should this logic be moved above where 'codec' is getting set? It seems a
little
> odd to query the aac object in the EAC3 case.

Done.

https://codereview.chromium.org/14641006/diff/2001/media/mp4/mp4_stream_parse...
media/mp4/mp4_stream_parser.cc:485: if
(runs_->audio_description().esds.object_type != kForbidden &&
On 2013/04/30 18:19:02, acolwell wrote:
> nit: How about adding a isAAC() helper method to esds since that is really
what
> we want to check. It could be used here and above the line that sets codec =
> kCodecAAC;

Done.

[acolwell GONE FROM CHROMIUM, 2013-05-01 00:31:00]

LGTM

https://codereview.chromium.org/14641006/diff/8001/media/mp4/mp4_stream_parse...
File media/mp4/mp4_stream_parser.cc (right):

https://codereview.chromium.org/14641006/diff/8001/media/mp4/mp4_stream_parse...
media/mp4/mp4_stream_parser.cc:211: audio_object_types_.find(kEAC3) !=
audio_object_types_.end()) {
nit: Replace 'audio_object_types_.find(kEAC3) != audio_object_types_.end()' with
'entry.format == FOURCC_EAC3'? It seems like that is more strict and more
accurately identifies the EAC3 content. Right?

[qinmin, 2013-05-01 03:59:15]

lgtm

[ycheo (away), 2013-05-01 06:01:10]

Thanks for the review.

https://codereview.chromium.org/14641006/diff/8001/media/mp4/mp4_stream_parse...
File media/mp4/mp4_stream_parser.cc (right):

https://codereview.chromium.org/14641006/diff/8001/media/mp4/mp4_stream_parse...
media/mp4/mp4_stream_parser.cc:211: audio_object_types_.find(kEAC3) !=
audio_object_types_.end()) {
On 2013/05/01 00:31:00, acolwell wrote:
> nit: Replace 'audio_object_types_.find(kEAC3) != audio_object_types_.end()'
with
> 'entry.format == FOURCC_EAC3'? It seems like that is more strict and more
> accurately identifies the EAC3 content. Right?

Done.

[commit-bot: I haz the power, 2013-05-01 06:02:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ycheo@chromium.org/14641006/24001

[commit-bot: I haz the power, 2013-05-01 06:53:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ycheo@chromium.org/14641006/30001

[commit-bot: I haz the power, 2013-05-01 12:42:22]

Change committed as 197602

[sky, 2014-01-15 14:25:29]

LGTM