Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2677143003:
[Chromecast] Introduce a MediaCapabilitiesShlib::IsSupportedAudioConfig (Closed)
|
Created:
3 years, 10 months ago by servolk Modified:
3 years, 8 months ago CC:
chromium-reviews, alokp+watch_chromium.org, lcwu+watch_chromium.org, gfhuang+watch_chromium.org, halliwell+watch_chromium.org, feature-media-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[Chromecast] Introduce a MediaCapabilitiesShlib::IsSupportedAudioConfig
This new API is similar to IsSupportedVideoConfig and is strictly-typed
unlike the old MediaCodecSupportShlib::IsSupported, which is going to
be deprecated.
Review-Url: https://codereview.chromium.org/2677143003
Cr-Commit-Position: refs/heads/master@{#460576}
Committed: https://chromium.googlesource.com/chromium/src/+/824a4fafaea59bede0f34603abfd0f550db4992e
Patch Set 1 #
Total comments: 3
Patch Set 2 : rebase #
Total comments: 2
Patch Set 3 : Fixed cast_media_android.cc impl #
Dependent Patchsets: Messages
Total messages: 32 (18 generated)
The CQ bit was checked by servolk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [Chromecast] Introduce a MediaCapabilitiesShlib::IsSupportedAudioConfig This new API is similar to IsSupportedVideoConfig and is strictly-typed unlike the old MediaCodecSupportShlib::IsSupported, which is going to be deprecated. ========== to ========== [Chromecast] Introduce a MediaCapabilitiesShlib::IsSupportedAudioConfig This new API is similar to IsSupportedVideoConfig and is strictly-typed unlike the old MediaCodecSupportShlib::IsSupported, which is going to be deprecated. ==========
servolk@chromium.org changed reviewers: + erickung@chromium.org, gfhuang@chromium.org, halliwell@chromium.org
On 2017/02/06 19:25:39, servolk wrote: > mailto:servolk@chromium.org changed reviewers: > + mailto:erickung@chromium.org, mailto:gfhuang@chromium.org, mailto:halliwell@chromium.org Since we have an opportunity to change partner API in 1.23, let's add IsSupportedAudioConfig function, that is going to be called from MediaClient in a manner similar to IsSupportedVideoConfig. This will allow us to deprecate the old IsSupported function that takes just a codec id string.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm% nits https://codereview.chromium.org/2677143003/diff/1/chromecast/media/cma/backen... File chromecast/media/cma/backend/alsa/media_codec_support_cast_audio.cc (right): https://codereview.chromium.org/2677143003/diff/1/chromecast/media/cma/backen... chromecast/media/cma/backend/alsa/media_codec_support_cast_audio.cc:44: return config.codec == kCodecAAC || config.codec == kCodecMP3 || nit: not sure which one is chromium style return (a || b) return a || b it should be consistent between MediaCapabilitiesShlib::IsSupportedVideoConfig() and MediaCapabilitiesShlib::IsSupportedAudioConfig() https://codereview.chromium.org/2677143003/diff/1/chromecast/media/cma/backen... File chromecast/media/cma/backend/cast_media_default.cc (right): https://codereview.chromium.org/2677143003/diff/1/chromecast/media/cma/backen... chromecast/media/cma/backend/cast_media_default.cc:116: config.codec == kCodecPCM; ditto https://codereview.chromium.org/2677143003/diff/1/chromecast/media/cma/backen... File chromecast/media/cma/backend/cast_media_dummy.cc (right): https://codereview.chromium.org/2677143003/diff/1/chromecast/media/cma/backen... chromecast/media/cma/backend/cast_media_dummy.cc:56: return config.codec == kCodecAAC || config.codec == kCodecMP3 || ditto
On 2017/02/06 19:27:03, servolk wrote: > On 2017/02/06 19:25:39, servolk wrote: > > mailto:servolk@chromium.org changed reviewers: > > + mailto:erickung@chromium.org, mailto:gfhuang@chromium.org, > mailto:halliwell@chromium.org > > Since we have an opportunity to change partner API in 1.23, let's add > IsSupportedAudioConfig function, that is going to be called from MediaClient in > a manner similar to IsSupportedVideoConfig. This will allow us to deprecate the > old IsSupported function that takes just a codec id string. Are you able to deprecate IsSupported and remove in the same CL? that would make it more clear.
On 2017/02/06 21:19:01, gfhuang wrote: > On 2017/02/06 19:27:03, servolk wrote: > > On 2017/02/06 19:25:39, servolk wrote: > > > mailto:servolk@chromium.org changed reviewers: > > > + mailto:erickung@chromium.org, mailto:gfhuang@chromium.org, > > mailto:halliwell@chromium.org > > > > Since we have an opportunity to change partner API in 1.23, let's add > > IsSupportedAudioConfig function, that is going to be called from MediaClient > in > > a manner similar to IsSupportedVideoConfig. This will allow us to deprecate > the > > old IsSupported function that takes just a codec id string. > > Are you able to deprecate IsSupported and remove in the same CL? > that would make it more clear. Fully switching to this new API will take some time. I think we should keep both this new API and the old IsSupported for now. In 1.23 we will use both and will ensure that their outputs match, then later in 1.24 we will remove the old IsSupported API. We are still going to do system update in 1.24 right?
On 2017/02/07 22:08:11, servolk wrote: > On 2017/02/06 21:19:01, gfhuang wrote: > > On 2017/02/06 19:27:03, servolk wrote: > > > On 2017/02/06 19:25:39, servolk wrote: > > > > mailto:servolk@chromium.org changed reviewers: > > > > + mailto:erickung@chromium.org, mailto:gfhuang@chromium.org, > > > mailto:halliwell@chromium.org > > > > > > Since we have an opportunity to change partner API in 1.23, let's add > > > IsSupportedAudioConfig function, that is going to be called from MediaClient > > in > > > a manner similar to IsSupportedVideoConfig. This will allow us to deprecate > > the > > > old IsSupported function that takes just a codec id string. > > > > Are you able to deprecate IsSupported and remove in the same CL? > > that would make it more clear. > > Fully switching to this new API will take some time. I think we should keep both > this new API and the old IsSupported for now. In 1.23 we will use both and will > ensure that their outputs match, then later in 1.24 we will remove the old > IsSupported API. We are still going to do system update in 1.24 right? that's the concern. we didn't plan to do system update in 1.24
On 2017/02/07 22:09:33, gfhuang wrote: > On 2017/02/07 22:08:11, servolk wrote: > > On 2017/02/06 21:19:01, gfhuang wrote: > > > On 2017/02/06 19:27:03, servolk wrote: > > > > On 2017/02/06 19:25:39, servolk wrote: > > > > > mailto:servolk@chromium.org changed reviewers: > > > > > + mailto:erickung@chromium.org, mailto:gfhuang@chromium.org, > > > > mailto:halliwell@chromium.org > > > > > > > > Since we have an opportunity to change partner API in 1.23, let's add > > > > IsSupportedAudioConfig function, that is going to be called from > MediaClient > > > in > > > > a manner similar to IsSupportedVideoConfig. This will allow us to > deprecate > > > the > > > > old IsSupported function that takes just a codec id string. > > > > > > Are you able to deprecate IsSupported and remove in the same CL? > > > that would make it more clear. > > > > Fully switching to this new API will take some time. I think we should keep > both > > this new API and the old IsSupported for now. In 1.23 we will use both and > will > > ensure that their outputs match, then later in 1.24 we will remove the old > > IsSupported API. We are still going to do system update in 1.24 right? > > that's the concern. > we didn't plan to do system update in 1.24 Wait, wasn't the plan actually to do system updates in even-numbered releases? I thought this system update in 1.23 was a one-off thing due to glibc bug.
On 2017/02/07 22:11:15, servolk wrote: > On 2017/02/07 22:09:33, gfhuang wrote: > > On 2017/02/07 22:08:11, servolk wrote: > > > On 2017/02/06 21:19:01, gfhuang wrote: > > > > On 2017/02/06 19:27:03, servolk wrote: > > > > > On 2017/02/06 19:25:39, servolk wrote: > > > > > > mailto:servolk@chromium.org changed reviewers: > > > > > > + mailto:erickung@chromium.org, mailto:gfhuang@chromium.org, > > > > > mailto:halliwell@chromium.org > > > > > > > > > > Since we have an opportunity to change partner API in 1.23, let's add > > > > > IsSupportedAudioConfig function, that is going to be called from > > MediaClient > > > > in > > > > > a manner similar to IsSupportedVideoConfig. This will allow us to > > deprecate > > > > the > > > > > old IsSupported function that takes just a codec id string. > > > > > > > > Are you able to deprecate IsSupported and remove in the same CL? > > > > that would make it more clear. > > > > > > Fully switching to this new API will take some time. I think we should keep > > both > > > this new API and the old IsSupported for now. In 1.23 we will use both and > > will > > > ensure that their outputs match, then later in 1.24 we will remove the old > > > IsSupported API. We are still going to do system update in 1.24 right? > > > > that's the concern. > > we didn't plan to do system update in 1.24 > > Wait, wasn't the plan actually to do system updates in even-numbered releases? I > thought this system update in 1.23 was a one-off thing due to glibc bug. No. system updates is scheduled per-need and no predefined pattern. If we have needs to do that 1.24, we could schedule and plan. It's not planned yet.
On 2017/02/07 22:13:36, gfhuang wrote: > On 2017/02/07 22:11:15, servolk wrote: > > On 2017/02/07 22:09:33, gfhuang wrote: > > > On 2017/02/07 22:08:11, servolk wrote: > > > > On 2017/02/06 21:19:01, gfhuang wrote: > > > > > On 2017/02/06 19:27:03, servolk wrote: > > > > > > On 2017/02/06 19:25:39, servolk wrote: > > > > > > > mailto:servolk@chromium.org changed reviewers: > > > > > > > + mailto:erickung@chromium.org, mailto:gfhuang@chromium.org, > > > > > > mailto:halliwell@chromium.org > > > > > > > > > > > > Since we have an opportunity to change partner API in 1.23, let's add > > > > > > IsSupportedAudioConfig function, that is going to be called from > > > MediaClient > > > > > in > > > > > > a manner similar to IsSupportedVideoConfig. This will allow us to > > > deprecate > > > > > the > > > > > > old IsSupported function that takes just a codec id string. > > > > > > > > > > Are you able to deprecate IsSupported and remove in the same CL? > > > > > that would make it more clear. > > > > > > > > Fully switching to this new API will take some time. I think we should > keep > > > both > > > > this new API and the old IsSupported for now. In 1.23 we will use both and > > > will > > > > ensure that their outputs match, then later in 1.24 we will remove the old > > > > IsSupported API. We are still going to do system update in 1.24 right? > > > > > > that's the concern. > > > we didn't plan to do system update in 1.24 > > > > Wait, wasn't the plan actually to do system updates in even-numbered releases? > I > > thought this system update in 1.23 was a one-off thing due to glibc bug. > > No. system updates is scheduled per-need and no predefined pattern. > If we have needs to do that 1.24, we could schedule and plan. > It's not planned yet. Getting back to this. Now that we know that 1.24 is going to be a system update again (due to switching to clang). Let's land this change. I'll rebase and will re-run CQ shortly. Here is the plan: 1. Land this change and cherry-pick it into chromecast master. 2. Add vendor implementations for the new API added in media_capabilities_shlib.h 3. Add a new IsSupportedAudioConfig API in CastMediaClient and use the new function in media_capabilities_shlib.h for that. 4. Switch all audio codec checks to the CastMediaClient::IsSupportedAudioConfig and get rid of the local downstream patches. In theory 1 and 3 can be done in a single CL, but it would be hard to test without any actual vendor implementations. Plus chcunningham@ is currently doing some refactoring of the media clients (see https://codereview.chromium.org/2712983004/), which will affect CastMediaClient as well and might cause conflicts for step #3. So let's land this change as is for now, this will allow us to complete steps #1 and #2. And in the meantime I'll start looking into what exactly needs to be done for #3 and #4.
The CQ bit was checked by servolk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/03/22 18:54:17, servolk wrote: > On 2017/02/07 22:13:36, gfhuang wrote: > > On 2017/02/07 22:11:15, servolk wrote: > > > On 2017/02/07 22:09:33, gfhuang wrote: > > > > On 2017/02/07 22:08:11, servolk wrote: > > > > > On 2017/02/06 21:19:01, gfhuang wrote: > > > > > > On 2017/02/06 19:27:03, servolk wrote: > > > > > > > On 2017/02/06 19:25:39, servolk wrote: > > > > > > > > mailto:servolk@chromium.org changed reviewers: > > > > > > > > + mailto:erickung@chromium.org, mailto:gfhuang@chromium.org, > > > > > > > mailto:halliwell@chromium.org > > > > > > > > > > > > > > Since we have an opportunity to change partner API in 1.23, let's > add > > > > > > > IsSupportedAudioConfig function, that is going to be called from > > > > MediaClient > > > > > > in > > > > > > > a manner similar to IsSupportedVideoConfig. This will allow us to > > > > deprecate > > > > > > the > > > > > > > old IsSupported function that takes just a codec id string. > > > > > > > > > > > > Are you able to deprecate IsSupported and remove in the same CL? > > > > > > that would make it more clear. > > > > > > > > > > Fully switching to this new API will take some time. I think we should > > keep > > > > both > > > > > this new API and the old IsSupported for now. In 1.23 we will use both > and > > > > will > > > > > ensure that their outputs match, then later in 1.24 we will remove the > old > > > > > IsSupported API. We are still going to do system update in 1.24 right? > > > > > > > > that's the concern. > > > > we didn't plan to do system update in 1.24 > > > > > > Wait, wasn't the plan actually to do system updates in even-numbered > releases? > > I > > > thought this system update in 1.23 was a one-off thing due to glibc bug. > > > > No. system updates is scheduled per-need and no predefined pattern. > > If we have needs to do that 1.24, we could schedule and plan. > > It's not planned yet. > > Getting back to this. Now that we know that 1.24 is going to be a system update > again (due to switching to clang). Let's land this change. I'll rebase and will > re-run CQ shortly. Here is the plan: > 1. Land this change and cherry-pick it into chromecast master. > 2. Add vendor implementations for the new API added in > media_capabilities_shlib.h > 3. Add a new IsSupportedAudioConfig API in CastMediaClient and use the new > function in media_capabilities_shlib.h for that. > 4. Switch all audio codec checks to the CastMediaClient::IsSupportedAudioConfig > and get rid of the local downstream patches. > > In theory 1 and 3 can be done in a single CL, but it would be hard to test > without any actual vendor implementations. Plus chcunningham@ is currently doing > some refactoring of the media clients (see > https://codereview.chromium.org/2712983004/), which will affect CastMediaClient > as well and might cause conflicts for step #3. So let's land this change as is > for now, this will allow us to complete steps #1 and #2. And in the meantime > I'll start looking into what exactly needs to be done for #3 and #4. lgtm my only concern is about the OEM confusing of when old API and new API co-exists. they don't know which one to work with.
https://codereview.chromium.org/2677143003/diff/20001/chromecast/media/cma/ba... File chromecast/media/cma/backend/cast_media_android.cc (right): https://codereview.chromium.org/2677143003/diff/20001/chromecast/media/cma/ba... chromecast/media/cma/backend/cast_media_android.cc:63: return true; Should this part be NOTREACHED, i.e. work consistently with IsSupportedVideoConfig case for android?
The CQ bit was checked by servolk@chromium.org to run a CQ dry run
https://codereview.chromium.org/2677143003/diff/20001/chromecast/media/cma/ba... File chromecast/media/cma/backend/cast_media_android.cc (right): https://codereview.chromium.org/2677143003/diff/20001/chromecast/media/cma/ba... chromecast/media/cma/backend/cast_media_android.cc:63: return true; On 2017/03/24 14:34:37, halliwell wrote: > Should this part be NOTREACHED, i.e. work consistently with > IsSupportedVideoConfig case for android? Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/03/29 17:35:45, servolk wrote: > https://codereview.chromium.org/2677143003/diff/20001/chromecast/media/cma/ba... > File chromecast/media/cma/backend/cast_media_android.cc (right): > > https://codereview.chromium.org/2677143003/diff/20001/chromecast/media/cma/ba... > chromecast/media/cma/backend/cast_media_android.cc:63: return true; > On 2017/03/24 14:34:37, halliwell wrote: > > Should this part be NOTREACHED, i.e. work consistently with > > IsSupportedVideoConfig case for android? > > Done. lgtm
The CQ bit was checked by servolk@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from erickung@chromium.org, gfhuang@chromium.org Link to the patchset: https://codereview.chromium.org/2677143003/#ps40001 (title: "Fixed cast_media_android.cc impl")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1490828924630500,
"parent_rev": "94161a5070865ff5d84dc45417786758ed372fa6", "commit_rev":
"824a4fafaea59bede0f34603abfd0f550db4992e"}
Message was sent while issue was closed.
Description was changed from ========== [Chromecast] Introduce a MediaCapabilitiesShlib::IsSupportedAudioConfig This new API is similar to IsSupportedVideoConfig and is strictly-typed unlike the old MediaCodecSupportShlib::IsSupported, which is going to be deprecated. ========== to ========== [Chromecast] Introduce a MediaCapabilitiesShlib::IsSupportedAudioConfig This new API is similar to IsSupportedVideoConfig and is strictly-typed unlike the old MediaCodecSupportShlib::IsSupported, which is going to be deprecated. Review-Url: https://codereview.chromium.org/2677143003 Cr-Commit-Position: refs/heads/master@{#460576} Committed: https://chromium.googlesource.com/chromium/src/+/824a4fafaea59bede0f34603abfd... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/824a4fafaea59bede0f34603abfd... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
