Add IsSupportedAudioConfig in MediaClient

Add IsSupportedAudioConfig in MediaClient

We already have the IsSupportedVideoConfig function in MediaClient,
this CL will add a very similar API for audio codec checks on various
platforms.

BUG=695264
Review-Url: https://codereview.chromium.org/2783963002
Cr-Commit-Position: refs/heads/master@{#460921}
Committed: https://chromium.googlesource.com/chromium/src/+/600bdb41533233777a5e852e1b5d08f6ec1bca5d

[servolk, 2017-03-29 17:45:20]

Description was changed from

==========
Add IsSupportedAudioConfig in MediaClient

We already have the IsSupportedVideoConfig function in MediaClient,
this CL will add a very similar API for audio codec checks on various
platforms.

BUG=695264
==========

to

==========
Add IsSupportedAudioConfig in MediaClient

We already have the IsSupportedVideoConfig function in MediaClient,
this CL will add a very similar API for audio codec checks on various
platforms.

BUG=695264
==========

[servolk, 2017-03-29 17:45:21]

servolk@chromium.org changed reviewers:
+ chcunningham@chromium.org, erickung@chromium.org, gfhuang@chromium.org,
halliwell@chromium.org

[gfhuang, 2017-03-29 20:54:59]

https://codereview.chromium.org/2783963002/diff/1/chromecast/common/media/cas...
File chromecast/common/media/cast_media_client.cc (right):

https://codereview.chromium.org/2783963002/diff/1/chromecast/common/media/cas...
chromecast/common/media/cast_media_client.cc:78: // TODO(sanfin): Implement this
for Android.
Open a bug to sanfin@ so that this doesn't fall through the crack.


BTW, in case you don't know.
TODO label means "who knows what to do", not "who is going to do".

https://codereview.chromium.org/2783963002/diff/1/chromecast/common/media/cas...
chromecast/common/media/cast_media_client.cc:89: if (codec == kCodecEAC3 &&
MediaCapabilities::HdmiSinkSupportsEAC3())
is this defaults to true for non-HDMI TV device?

[servolk, 2017-03-29 21:51:52]

https://codereview.chromium.org/2783963002/diff/1/chromecast/common/media/cas...
File chromecast/common/media/cast_media_client.cc (right):

https://codereview.chromium.org/2783963002/diff/1/chromecast/common/media/cas...
chromecast/common/media/cast_media_client.cc:78: // TODO(sanfin): Implement this
for Android.
On 2017/03/29 20:54:59, gfhuang wrote:
> Open a bug to sanfin@ so that this doesn't fall through the crack.
> 
> 
> BTW, in case you don't know.
> TODO label means "who knows what to do", not "who is going to do".

Done (b/36734043).

I think it's pretty obvious in this case what needs to be done, so Simeon
hopefully know what to do in this case too :)

https://codereview.chromium.org/2783963002/diff/1/chromecast/common/media/cas...
chromecast/common/media/cast_media_client.cc:89: if (codec == kCodecEAC3 &&
MediaCapabilities::HdmiSinkSupportsEAC3())
On 2017/03/29 20:54:58, gfhuang wrote:
> is this defaults to true for non-HDMI TV device?

No, this is solely for HDMI devices. TVs that support AC3/EAC3 internally should
handle the kCodecAC3 / kCodecEAC3 explicitly in their
MediaCapShlib::IsSupportedAudioConfig implementations.

[gfhuang, 2017-03-29 21:57:28]

lgtm

https://codereview.chromium.org/2783963002/diff/1/chromecast/common/media/cas...
File chromecast/common/media/cast_media_client.cc (right):

https://codereview.chromium.org/2783963002/diff/1/chromecast/common/media/cas...
chromecast/common/media/cast_media_client.cc:89: if (codec == kCodecEAC3 &&
MediaCapabilities::HdmiSinkSupportsEAC3())
On 2017/03/29 21:51:52, servolk wrote:
> On 2017/03/29 20:54:58, gfhuang wrote:
> > is this defaults to true for non-HDMI TV device?
> 
> No, this is solely for HDMI devices. TVs that support AC3/EAC3 internally
should
> handle the kCodecAC3 / kCodecEAC3 explicitly in their
> MediaCapShlib::IsSupportedAudioConfig implementations.

I see. then please double-check
MediaCapabilities::HdmiSinkSupportsEAC3() is return false by default in CastTVs.
so that the code will keep going to L96

[chcunningham, 2017-03-29 22:15:17]

Looks great - just one question

https://codereview.chromium.org/2783963002/diff/1/media/base/decode_capabilit...
File media/base/decode_capabilities.cc (right):

https://codereview.chromium.org/2783963002/diff/1/media/base/decode_capabilit...
media/base/decode_capabilities.cc:107: case media::kCodecAAC:
Thanks a ton for adding this. I'm not familiar with some of these supported
codecs. How did you build the list of supported vs not?

[servolk, 2017-03-29 22:27:58]

https://codereview.chromium.org/2783963002/diff/1/media/base/decode_capabilit...
File media/base/decode_capabilities.cc (right):

https://codereview.chromium.org/2783963002/diff/1/media/base/decode_capabilit...
media/base/decode_capabilities.cc:107: case media::kCodecAAC:
On 2017/03/29 22:15:17, chcunningham wrote:
> Thanks a ton for adding this. I'm not familiar with some of these supported
> codecs. How did you build the list of supported vs not?

The supported codecs in this list are the the ones that are explicitly enabled
in ffmpeg config scripts for Chromium, e.g.
https://cs.chromium.org/chromium/src/third_party/ffmpeg/chromium/scripts/buil...
(see also lines 591, 606 and 608 in there).

I know that some of those might not be supported on all platforms, but the code
I'm adding here is new and I assume there's something else that's currently
blocking unsupported codecs for each platform (e.g.
MimeUtil::IsCodecSupportedOnAndroid for Android). IIUC this function is
currently shared between Chrome, ChromeOS and Android, right?

[servolk, 2017-03-29 22:30:46]

https://codereview.chromium.org/2783963002/diff/1/media/base/decode_capabilit...
File media/base/decode_capabilities.cc (right):

https://codereview.chromium.org/2783963002/diff/1/media/base/decode_capabilit...
media/base/decode_capabilities.cc:107: case media::kCodecAAC:
On 2017/03/29 22:27:58, servolk wrote:
> On 2017/03/29 22:15:17, chcunningham wrote:
> > Thanks a ton for adding this. I'm not familiar with some of these supported
> > codecs. How did you build the list of supported vs not?
> 
> The supported codecs in this list are the the ones that are explicitly enabled
> in ffmpeg config scripts for Chromium, e.g.
>
https://cs.chromium.org/chromium/src/third_party/ffmpeg/chromium/scripts/buil...
> (see also lines 591, 606 and 608 in there).
> 
> I know that some of those might not be supported on all platforms, but the
code
> I'm adding here is new and I assume there's something else that's currently
> blocking unsupported codecs for each platform (e.g.
> MimeUtil::IsCodecSupportedOnAndroid for Android). IIUC this function is
> currently shared between Chrome, ChromeOS and Android, right?

Oh, yes, and I know for sure that AC3 / EAC3 are currently only supported by
Cast platform, but it uses a separate MediaClient and doesn't call this. As for
ALAC, AFAIR it was added back in the day for Ensemble project, but we never
supported by any other platform, so I've marked it as unsupported here too.

[halliwell, 2017-03-29 23:46:16]

On 2017/03/29 22:30:46, servolk wrote:
>
https://codereview.chromium.org/2783963002/diff/1/media/base/decode_capabilit...
> File media/base/decode_capabilities.cc (right):
> 
>
https://codereview.chromium.org/2783963002/diff/1/media/base/decode_capabilit...
> media/base/decode_capabilities.cc:107: case media::kCodecAAC:
> On 2017/03/29 22:27:58, servolk wrote:
> > On 2017/03/29 22:15:17, chcunningham wrote:
> > > Thanks a ton for adding this. I'm not familiar with some of these
supported
> > > codecs. How did you build the list of supported vs not?
> > 
> > The supported codecs in this list are the the ones that are explicitly
enabled
> > in ffmpeg config scripts for Chromium, e.g.
> >
>
https://cs.chromium.org/chromium/src/third_party/ffmpeg/chromium/scripts/buil...
> > (see also lines 591, 606 and 608 in there).
> > 
> > I know that some of those might not be supported on all platforms, but the
> code
> > I'm adding here is new and I assume there's something else that's currently
> > blocking unsupported codecs for each platform (e.g.
> > MimeUtil::IsCodecSupportedOnAndroid for Android). IIUC this function is
> > currently shared between Chrome, ChromeOS and Android, right?
> 
> Oh, yes, and I know for sure that AC3 / EAC3 are currently only supported by
> Cast platform, but it uses a separate MediaClient and doesn't call this. As
for
> ALAC, AFAIR it was added back in the day for Ensemble project, but we never
> supported by any other platform, so I've marked it as unsupported here too.

lgtm

[halliwell, 2017-03-29 23:46:29]

On 2017/03/29 22:30:46, servolk wrote:
>
https://codereview.chromium.org/2783963002/diff/1/media/base/decode_capabilit...
> File media/base/decode_capabilities.cc (right):
> 
>
https://codereview.chromium.org/2783963002/diff/1/media/base/decode_capabilit...
> media/base/decode_capabilities.cc:107: case media::kCodecAAC:
> On 2017/03/29 22:27:58, servolk wrote:
> > On 2017/03/29 22:15:17, chcunningham wrote:
> > > Thanks a ton for adding this. I'm not familiar with some of these
supported
> > > codecs. How did you build the list of supported vs not?
> > 
> > The supported codecs in this list are the the ones that are explicitly
enabled
> > in ffmpeg config scripts for Chromium, e.g.
> >
>
https://cs.chromium.org/chromium/src/third_party/ffmpeg/chromium/scripts/buil...
> > (see also lines 591, 606 and 608 in there).
> > 
> > I know that some of those might not be supported on all platforms, but the
> code
> > I'm adding here is new and I assume there's something else that's currently
> > blocking unsupported codecs for each platform (e.g.
> > MimeUtil::IsCodecSupportedOnAndroid for Android). IIUC this function is
> > currently shared between Chrome, ChromeOS and Android, right?
> 
> Oh, yes, and I know for sure that AC3 / EAC3 are currently only supported by
> Cast platform, but it uses a separate MediaClient and doesn't call this. As
for
> ALAC, AFAIR it was added back in the day for Ensemble project, but we never
> supported by any other platform, so I've marked it as unsupported here too.

lgtm

[servolk, 2017-03-30 00:22:50]

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

[commit-bot: I haz the power, 2017-03-30 00:23:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-30 01:00:12]

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

[commit-bot: I haz the power, 2017-03-30 01:00:13]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[servolk, 2017-03-30 01:38:47]

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

[commit-bot: I haz the power, 2017-03-30 01:40:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-30 03:33:25]

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

[commit-bot: I haz the power, 2017-03-30 03:33:26]

Dry run: This issue passed the CQ dry run.

[chcunningham, 2017-03-30 18:09:04]

LGTM % "Can you" requests below.

https://codereview.chromium.org/2783963002/diff/20001/media/base/decode_capab...
File media/base/decode_capabilities.cc (right):

https://codereview.chromium.org/2783963002/diff/20001/media/base/decode_capab...
media/base/decode_capabilities.cc:105: bool IsSupportedAudioConfig(const
AudioConfig& config) {
This is fine for now, but I'll note some things for future planning.

1. AudioConfig will also need the mime_type string. Down the road the android
specific logic will move from MimeUtilInternal into an android specific version
of this class.

2. For now this class reports support for proprietary codecs. In the future we
will move the codec guards from MimeUtil into this class.

https://codereview.chromium.org/2783963002/diff/20001/media/base/decode_capab...
media/base/decode_capabilities.cc:114: case media::kCodecPCM_MULAW:
Can you group all the PCM codecs together?

https://codereview.chromium.org/2783963002/diff/20001/media/base/decode_capab...
media/base/decode_capabilities.cc:115: case media::kCodecGSM_MS:
Can you move the GSM and AMR codecs into a separate block and only return true
#if defined(OS_CHROMEOS)? These aren't part of the common ffmpeg config.

[servolk, 2017-03-30 18:16:36]

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

[servolk, 2017-03-30 18:16:43]

https://codereview.chromium.org/2783963002/diff/20001/media/base/decode_capab...
File media/base/decode_capabilities.cc (right):

https://codereview.chromium.org/2783963002/diff/20001/media/base/decode_capab...
media/base/decode_capabilities.cc:105: bool IsSupportedAudioConfig(const
AudioConfig& config) {
On 2017/03/30 18:09:04, chcunningham wrote:
> This is fine for now, but I'll note some things for future planning.
> 
> 1. AudioConfig will also need the mime_type string. Down the road the android
> specific logic will move from MimeUtilInternal into an android specific
version
> of this class.
> 
> 2. For now this class reports support for proprietary codecs. In the future we
> will move the codec guards from MimeUtil into this class.

Acknowledged.

https://codereview.chromium.org/2783963002/diff/20001/media/base/decode_capab...
media/base/decode_capabilities.cc:114: case media::kCodecPCM_MULAW:
On 2017/03/30 18:09:04, chcunningham wrote:
> Can you group all the PCM codecs together?

Done.

https://codereview.chromium.org/2783963002/diff/20001/media/base/decode_capab...
media/base/decode_capabilities.cc:115: case media::kCodecGSM_MS:
On 2017/03/30 18:09:04, chcunningham wrote:
> Can you move the GSM and AMR codecs into a separate block and only return true
> #if defined(OS_CHROMEOS)? These aren't part of the common ffmpeg config.

Done.

[commit-bot: I haz the power, 2017-03-30 18:17:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[servolk, 2017-03-30 18:56:58]

The CQ bit was unchecked by servolk@chromium.org

[servolk, 2017-03-30 18:56:59]

The CQ bit was checked by servolk@chromium.org

[servolk, 2017-03-30 18:56:59]

The patchset sent to the CQ was uploaded after l-g-t-m from
gfhuang@chromium.org, halliwell@chromium.org, chcunningham@chromium.org
Link to the patchset: https://codereview.chromium.org/2783963002/#ps40001
(title: "CR feedback")

[commit-bot: I haz the power, 2017-03-30 18:57:40]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-30 20:51:46]

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

[commit-bot: I haz the power, 2017-03-30 20:51:47]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[servolk, 2017-03-30 20:54:24]

The CQ bit was checked by servolk@chromium.org

[commit-bot: I haz the power, 2017-03-30 20:55:28]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-30 23:18:28]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1490907264708530,
"parent_rev": "c2d8b871f3810ec01394eee986b02bc400b36db7", "commit_rev":
"600bdb41533233777a5e852e1b5d08f6ec1bca5d"}

[commit-bot: I haz the power, 2017-03-30 23:19:28]

Description was changed from

==========
Add IsSupportedAudioConfig in MediaClient

We already have the IsSupportedVideoConfig function in MediaClient,
this CL will add a very similar API for audio codec checks on various
platforms.

BUG=695264
==========

to

==========
Add IsSupportedAudioConfig in MediaClient

We already have the IsSupportedVideoConfig function in MediaClient,
this CL will add a very similar API for audio codec checks on various
platforms.

BUG=695264

Review-Url: https://codereview.chromium.org/2783963002
Cr-Commit-Position: refs/heads/master@{#460921}
Committed:
https://chromium.googlesource.com/chromium/src/+/600bdb41533233777a5e852e1b5d...
==========

[commit-bot: I haz the power, 2017-03-30 23:19:29]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/600bdb41533233777a5e852e1b5d...