[Chromecast] Add new volume control API to CastMediaShlib

[Chromecast] Add new volume control API to CastMediaShlib

BUG= internal b/34454613

Review-Url: https://codereview.chromium.org/2712883006
Cr-Commit-Position: refs/heads/master@{#456773}
Committed: https://chromium.googlesource.com/chromium/src/+/c87c5d279daf0657fcd941003d0d4b503410c725

[kmackay, 2017-02-23 23:24:25]

kmackay@chromium.org changed reviewers:
+ bshaya@google.com, gfhuang@chromium.org, halliwell@chromium.org,
slan@chromium.org, yilongyao@chromium.org

[gfhuang, 2017-02-23 23:25:41]

On 2017/02/23 23:24:26, kmackay wrote:

lgtm, reviewed in eureka-internal

[kmackay, 2017-02-24 06:49:36]

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

[commit-bot: I haz the power, 2017-02-24 06:49:46]

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-02-24 07:11:18]

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

[commit-bot: I haz the power, 2017-02-24 07:11:19]

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

[halliwell, 2017-02-24 14:23:25]

https://codereview.chromium.org/2712883006/diff/20001/chromecast/public/avset...
File chromecast/public/avsettings.h (right):

https://codereview.chromium.org/2712883006/diff/20001/chromecast/public/avset...
chromecast/public/avsettings.h:207: // DEPRECATED - Prefer to implement volume
control in CastMediaShlib.
Has this been fully thought through for TV case?  Default use case for
integrated TV would be to modify volume during media playback, and you want the
actual TV volume to change (volume bar UI will appear on TV).

Will we support TTS on TV?  Suppose we do.  Is it ducking media volume and
increasing TTS volume?  Presumably we don't want TV volume UI to pop up at this
point?

[gfhuang, 2017-02-24 15:55:29]

On 2017/02/24 14:23:25, halliwell wrote:
>
https://codereview.chromium.org/2712883006/diff/20001/chromecast/public/avset...
> File chromecast/public/avsettings.h (right):
> 
>
https://codereview.chromium.org/2712883006/diff/20001/chromecast/public/avset...
> chromecast/public/avsettings.h:207: // DEPRECATED - Prefer to implement volume
> control in CastMediaShlib.
> Has this been fully thought through for TV case?  Default use case for
> integrated TV would be to modify volume during media playback, and you want
the
> actual TV volume to change (volume bar UI will appear on TV).
> 
> Will we support TTS on TV?  Suppose we do.  Is it ducking media volume and
> increasing TTS volume?  Presumably we don't want TV volume UI to pop up at
this
> point?

Yes, we have discussed this.

The design is to use SetOutputLimit() API for ducking, so that TV volume UI
won't pop up.

[alokp, 2017-02-24 17:34:26]

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

[alokp, 2017-02-24 17:34:26]

https://codereview.chromium.org/2712883006/diff/20001/chromecast/public/cast_...
File chromecast/public/cast_media_shlib.h (right):

https://codereview.chromium.org/2712883006/diff/20001/chromecast/public/cast_...
chromecast/public/cast_media_shlib.h:25: kMedia,
Add comments about what they mean.

Should we rename kTts to kCommunication to make it more generic.

https://codereview.chromium.org/2712883006/diff/20001/chromecast/public/cast_...
chromecast/public/cast_media_shlib.h:57: // Volume control must run separately
since it needs to be functional even when
It seems clunky to have a separate Initialize/Finalize for volume specifically.
What is the concern with using the existing Initialize/Finalize? It is not clear
from the comment.

https://codereview.chromium.org/2712883006/diff/20001/chromecast/public/media...
File chromecast/public/media/media_pipeline_device_params.h (right):

https://codereview.chromium.org/2712883006/diff/20001/chromecast/public/media...
chromecast/public/media/media_pipeline_device_params.h:46: const std::string&
device_id_in)
Do we need both - content_type and device_id?

[kmackay, 2017-02-24 19:01:06]

https://codereview.chromium.org/2712883006/diff/20001/chromecast/public/cast_...
File chromecast/public/cast_media_shlib.h (right):

https://codereview.chromium.org/2712883006/diff/20001/chromecast/public/cast_...
chromecast/public/cast_media_shlib.h:25: kMedia,
On 2017/02/24 17:34:26, alokp wrote:
> Add comments about what they mean.
> 
> Should we rename kTts to kCommunication to make it more generic.

Done.

https://codereview.chromium.org/2712883006/diff/20001/chromecast/public/cast_...
chromecast/public/cast_media_shlib.h:57: // Volume control must run separately
since it needs to be functional even when
On 2017/02/24 17:34:26, alokp wrote:
> It seems clunky to have a separate Initialize/Finalize for volume
specifically.
> What is the concern with using the existing Initialize/Finalize? It is not
clear
> from the comment.

I added more detail to the comment.

https://codereview.chromium.org/2712883006/diff/20001/chromecast/public/media...
File chromecast/public/media/media_pipeline_device_params.h (right):

https://codereview.chromium.org/2712883006/diff/20001/chromecast/public/media...
chromecast/public/media/media_pipeline_device_params.h:46: const std::string&
device_id_in)
On 2017/02/24 17:34:26, alokp wrote:
> Do we need both - content_type and device_id?

Ben is planning to use device_id in the short term, to do separate filtering for
things that fall into the same volume control group (see his CL). The idea is
that it more closely identifies the actual stream source, but OEM's don't need
to care about it if they don't want to. Also it's nice to have in in the params
rather than passing it around as a second param everywhere.

[kmackay, 2017-02-24 19:01:15]

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

[commit-bot: I haz the power, 2017-02-24 19:01:45]

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-02-24 19:21:14]

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

[commit-bot: I haz the power, 2017-02-24 19:21:15]

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

[halliwell, 2017-02-24 21:37:21]

https://codereview.chromium.org/2712883006/diff/40001/chromecast/media/cma/ba...
File chromecast/media/cma/backend/media_pipeline_backend_manager.h (right):

https://codereview.chromium.org/2712883006/diff/40001/chromecast/media/cma/ba...
chromecast/media/cma/backend/media_pipeline_backend_manager.h:56: //
Adds/removes an observer for when folume feedback sounds are allowed.
"folume"

https://codereview.chromium.org/2712883006/diff/40001/chromecast/media/cma/ba...
File chromecast/media/cma/backend/media_pipeline_backend_wrapper.cc (right):

https://codereview.chromium.org/2712883006/diff/40001/chromecast/media/cma/ba...
chromecast/media/cma/backend/media_pipeline_backend_wrapper.cc:35: sfx_backend_
? DecoderType::SFX_DECODER : DecoderType::AUDIO_DECODER);
nit: {} for multiline

https://codereview.chromium.org/2712883006/diff/40001/chromecast/media/cma/ba...
chromecast/media/cma/backend/media_pipeline_backend_wrapper.cc:91:
SetPlaying(true);
um, false? :)

https://codereview.chromium.org/2712883006/diff/40001/chromecast/media/cma/ba...
chromecast/media/cma/backend/media_pipeline_backend_wrapper.cc:99:
SetPlaying(false);
true? :)

https://codereview.chromium.org/2712883006/diff/40001/chromecast/public/avset...
File chromecast/public/avsettings.h (right):

https://codereview.chromium.org/2712883006/diff/40001/chromecast/public/avset...
chromecast/public/avsettings.h:76: // DEPRECATED - Prefer to implement volume
control in CastMediaShlib.
What is OEM supposed to do with this info?  What is the deprecation plan?

https://codereview.chromium.org/2712883006/diff/40001/chromecast/public/cast_...
File chromecast/public/cast_media_shlib.h (right):

https://codereview.chromium.org/2712883006/diff/40001/chromecast/public/cast_...
chromecast/public/cast_media_shlib.h:66: // on the same thread that calls
InitializeVolume().
This is kind of clumsy.  Would it be clearer just to make a new class for the
volume functions with its own init/finalize?

[kmackay, 2017-02-24 23:53:29]

https://codereview.chromium.org/2712883006/diff/40001/chromecast/media/cma/ba...
File chromecast/media/cma/backend/media_pipeline_backend_manager.h (right):

https://codereview.chromium.org/2712883006/diff/40001/chromecast/media/cma/ba...
chromecast/media/cma/backend/media_pipeline_backend_manager.h:56: //
Adds/removes an observer for when folume feedback sounds are allowed.
On 2017/02/24 21:37:21, halliwell wrote:
> "folume"

Done.

https://codereview.chromium.org/2712883006/diff/40001/chromecast/media/cma/ba...
File chromecast/media/cma/backend/media_pipeline_backend_wrapper.cc (right):

https://codereview.chromium.org/2712883006/diff/40001/chromecast/media/cma/ba...
chromecast/media/cma/backend/media_pipeline_backend_wrapper.cc:35: sfx_backend_
? DecoderType::SFX_DECODER : DecoderType::AUDIO_DECODER);
On 2017/02/24 21:37:21, halliwell wrote:
> nit: {} for multiline

Done.

https://codereview.chromium.org/2712883006/diff/40001/chromecast/media/cma/ba...
chromecast/media/cma/backend/media_pipeline_backend_wrapper.cc:91:
SetPlaying(true);
On 2017/02/24 21:37:21, halliwell wrote:
> um, false? :)

Done.

https://codereview.chromium.org/2712883006/diff/40001/chromecast/media/cma/ba...
chromecast/media/cma/backend/media_pipeline_backend_wrapper.cc:99:
SetPlaying(false);
On 2017/02/24 21:37:21, halliwell wrote:
> true? :)

Done.

https://codereview.chromium.org/2712883006/diff/40001/chromecast/public/avset...
File chromecast/public/avsettings.h (right):

https://codereview.chromium.org/2712883006/diff/40001/chromecast/public/avset...
chromecast/public/avsettings.h:76: // DEPRECATED - Prefer to implement volume
control in CastMediaShlib.
On 2017/02/24 21:37:21, halliwell wrote:
> What is OEM supposed to do with this info?  What is the deprecation plan?

Well... we haven't really figured out what the plan is for TVs yet. Ideally they
would implement the new API (maybe TVs won't need to support all content types,
depending on how we do it). Once all OEMs have implemented the new API, we can
remove the old API and stop calling it internally.

https://codereview.chromium.org/2712883006/diff/40001/chromecast/public/cast_...
File chromecast/public/cast_media_shlib.h (right):

https://codereview.chromium.org/2712883006/diff/40001/chromecast/public/cast_...
chromecast/public/cast_media_shlib.h:66: // on the same thread that calls
InitializeVolume().
On 2017/02/24 21:37:21, halliwell wrote:
> This is kind of clumsy.  Would it be clearer just to make a new class for the
> volume functions with its own init/finalize?

Maybe? But the new class would probably ideally live in the same shlib, since
the volume control relates to the content_type passed in
MediaPipelineBackendParams - so it's a lot easier to implement if both are in
the same shlib.

[halliwell, 2017-02-25 00:43:46]

https://codereview.chromium.org/2712883006/diff/40001/chromecast/public/avset...
File chromecast/public/avsettings.h (right):

https://codereview.chromium.org/2712883006/diff/40001/chromecast/public/avset...
chromecast/public/avsettings.h:76: // DEPRECATED - Prefer to implement volume
control in CastMediaShlib.
On 2017/02/24 23:53:29, kmackay wrote:
> On 2017/02/24 21:37:21, halliwell wrote:
> > What is OEM supposed to do with this info?  What is the deprecation plan?
> 
> Well... we haven't really figured out what the plan is for TVs yet. Ideally
they
> would implement the new API (maybe TVs won't need to support all content
types,
> depending on how we do it). Once all OEMs have implemented the new API, we can
> remove the old API and stop calling it internally.

Ok.  So can an OEM assume that if they implement the new functions, we will stop
calling these ones?  It could be useful to explain that to them for when they
are doing the update work.

https://codereview.chromium.org/2712883006/diff/40001/chromecast/public/cast_...
File chromecast/public/cast_media_shlib.h (right):

https://codereview.chromium.org/2712883006/diff/40001/chromecast/public/cast_...
chromecast/public/cast_media_shlib.h:66: // on the same thread that calls
InitializeVolume().
On 2017/02/24 23:53:29, kmackay wrote:
> On 2017/02/24 21:37:21, halliwell wrote:
> > This is kind of clumsy.  Would it be clearer just to make a new class for
the
> > volume functions with its own init/finalize?
> 
> Maybe? But the new class would probably ideally live in the same shlib, since
> the volume control relates to the content_type passed in
> MediaPipelineBackendParams - so it's a lot easier to implement if both are in
> the same shlib.

Yep, agreed, keeping in same shared lib makes sense.  It doesn't necessarily
stop us having another class, e.g. CastMediaVolume, implemented in the same
shlib.  We already have other classes (e.g. VideoPlane) in the same lib.  What
do you think?

[kmackay, 2017-02-25 02:01:08]

https://codereview.chromium.org/2712883006/diff/40001/chromecast/public/avset...
File chromecast/public/avsettings.h (right):

https://codereview.chromium.org/2712883006/diff/40001/chromecast/public/avset...
chromecast/public/avsettings.h:76: // DEPRECATED - Prefer to implement volume
control in CastMediaShlib.
On 2017/02/25 00:43:46, halliwell wrote:
> On 2017/02/24 23:53:29, kmackay wrote:
> > On 2017/02/24 21:37:21, halliwell wrote:
> > > What is OEM supposed to do with this info?  What is the deprecation plan?
> > 
> > Well... we haven't really figured out what the plan is for TVs yet. Ideally
> they
> > would implement the new API (maybe TVs won't need to support all content
> types,
> > depending on how we do it). Once all OEMs have implemented the new API, we
can
> > remove the old API and stop calling it internally.
> 
> Ok.  So can an OEM assume that if they implement the new functions, we will
stop
> calling these ones?  It could be useful to explain that to them for when they
> are doing the update work.

Yes, if they implement the new API, the code will automatically stop calling the
old API.

https://codereview.chromium.org/2712883006/diff/40001/chromecast/public/cast_...
File chromecast/public/cast_media_shlib.h (right):

https://codereview.chromium.org/2712883006/diff/40001/chromecast/public/cast_...
chromecast/public/cast_media_shlib.h:66: // on the same thread that calls
InitializeVolume().
On 2017/02/25 00:43:46, halliwell wrote:
> On 2017/02/24 23:53:29, kmackay wrote:
> > On 2017/02/24 21:37:21, halliwell wrote:
> > > This is kind of clumsy.  Would it be clearer just to make a new class for
> the
> > > volume functions with its own init/finalize?
> > 
> > Maybe? But the new class would probably ideally live in the same shlib,
since
> > the volume control relates to the content_type passed in
> > MediaPipelineBackendParams - so it's a lot easier to implement if both are
in
> > the same shlib.
> 
> Yep, agreed, keeping in same shared lib makes sense.  It doesn't necessarily
> stop us having another class, e.g. CastMediaVolume, implemented in the same
> shlib.  We already have other classes (e.g. VideoPlane) in the same lib.  What
> do you think?

Sure, sounds fine to me

[kmackay, 2017-02-25 05:57:02]

https://codereview.chromium.org/2712883006/diff/40001/chromecast/public/cast_...
File chromecast/public/cast_media_shlib.h (right):

https://codereview.chromium.org/2712883006/diff/40001/chromecast/public/cast_...
chromecast/public/cast_media_shlib.h:66: // on the same thread that calls
InitializeVolume().
On 2017/02/25 02:01:08, kmackay wrote:
> On 2017/02/25 00:43:46, halliwell wrote:
> > On 2017/02/24 23:53:29, kmackay wrote:
> > > On 2017/02/24 21:37:21, halliwell wrote:
> > > > This is kind of clumsy.  Would it be clearer just to make a new class
for
> > the
> > > > volume functions with its own init/finalize?
> > > 
> > > Maybe? But the new class would probably ideally live in the same shlib,
> since
> > > the volume control relates to the content_type passed in
> > > MediaPipelineBackendParams - so it's a lot easier to implement if both are
> in
> > > the same shlib.
> > 
> > Yep, agreed, keeping in same shared lib makes sense.  It doesn't necessarily
> > stop us having another class, e.g. CastMediaVolume, implemented in the same
> > shlib.  We already have other classes (e.g. VideoPlane) in the same lib. 
What
> > do you think?
> 
> Sure, sounds fine to me

I wouldn't implement it like VideoPlane though; I think the "static weak method"
model is a lot better than the "abstract class" method for shlib APIs, since it
allows us to add new methods without requiring a system update. That means that
the new CastMediaVolume class won't have any obvious ties to the CastMediaShlib
class.

[kmackay, 2017-02-26 21:16:42]

https://codereview.chromium.org/2712883006/diff/40001/chromecast/public/cast_...
File chromecast/public/cast_media_shlib.h (right):

https://codereview.chromium.org/2712883006/diff/40001/chromecast/public/cast_...
chromecast/public/cast_media_shlib.h:66: // on the same thread that calls
InitializeVolume().
On 2017/02/24 21:37:21, halliwell wrote:
> This is kind of clumsy.  Would it be clearer just to make a new class for the
> volume functions with its own init/finalize?

Done.

[halliwell, 2017-02-27 16:58:06]

On 2017/02/26 21:16:42, kmackay wrote:
>
https://codereview.chromium.org/2712883006/diff/40001/chromecast/public/cast_...
> File chromecast/public/cast_media_shlib.h (right):
> 
>
https://codereview.chromium.org/2712883006/diff/40001/chromecast/public/cast_...
> chromecast/public/cast_media_shlib.h:66: // on the same thread that calls
> InitializeVolume().
> On 2017/02/24 21:37:21, halliwell wrote:
> > This is kind of clumsy.  Would it be clearer just to make a new class for
the
> > volume functions with its own init/finalize?
> 
> Done.

lgtm

[bshaya, 2017-02-27 16:59:10]

On 2017/02/24 17:34:26, alokp wrote:
>
https://codereview.chromium.org/2712883006/diff/20001/chromecast/public/cast_...
> File chromecast/public/cast_media_shlib.h (right):
> 
> Should we rename kTts to kCommunication to make it more generic.

My CL relies on tts and communication being distinct device_ids (same
content_type is fine).

[kmackay, 2017-03-08 05:24:53]

Hmm... apparently uploading multiple git commits just merges them all into the
first CL. :-(

[halliwell, 2017-03-08 17:59:37]

On 2017/03/08 05:24:53, kmackay wrote:
> Hmm... apparently uploading multiple git commits just merges them all into the
> first CL. :-(

Yep, the review is associated with the branch.

[kmackay, 2017-03-14 17:39:57]

The CQ bit was checked by kmackay@chromium.org

[kmackay, 2017-03-14 17:39:57]

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

[commit-bot: I haz the power, 2017-03-14 17:40:38]

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-14 18:48:26]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1489513197013680,
"parent_rev": "ad977926ecd213a1063fb7d429d6fd1d5ee7dce7", "commit_rev":
"c87c5d279daf0657fcd941003d0d4b503410c725"}

[commit-bot: I haz the power, 2017-03-14 18:49:12]

Description was changed from

==========
[Chromecast] Add new volume control API to CastMediaShlib

BUG= internal b/34454613
==========

to

==========
[Chromecast] Add new volume control API to CastMediaShlib

BUG= internal b/34454613

Review-Url: https://codereview.chromium.org/2712883006
Cr-Commit-Position: refs/heads/master@{#456773}
Committed:
https://chromium.googlesource.com/chromium/src/+/c87c5d279daf0657fcd941003d0d...
==========

[commit-bot: I haz the power, 2017-03-14 18:49:14]

Committed patchset #11 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/c87c5d279daf0657fcd941003d0d...