Implement new audio handler which talks to the new audio dbus apis.

Implement new audio handler which talks to the new audio dbus apis, and adds a new AudioClientRestarted signal in CrasAudioClient.

BUG=160311, 232032
TBR=sky
TBR=battre

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

[stevenjb, 2013-04-17 01:25:11]

https://codereview.chromium.org/14314002/diff/1/chromeos/audio/audio_pref_nam...
File chromeos/audio/audio_pref_names.cc (right):

https://codereview.chromium.org/14314002/diff/1/chromeos/audio/audio_pref_nam...
chromeos/audio/audio_pref_names.cc:12: // chrome/common/pref_names.cc, once we
retire the old AudioHandler code.
Does it make sense to change the old code to include this file instead?

https://codereview.chromium.org/14314002/diff/1/chromeos/audio/cras_audio_han...
File chromeos/audio/cras_audio_handler.cc (right):

https://codereview.chromium.org/14314002/diff/1/chromeos/audio/cras_audio_han...
chromeos/audio/cras_audio_handler.cc:178: : weak_ptr_factory_(this),
ALLOW_THIS...

https://codereview.chromium.org/14314002/diff/1/chromeos/audio/cras_audio_han...
chromeos/audio/cras_audio_handler.cc:201: if (output_volume_ != volume) {
nit: early exit instead

https://codereview.chromium.org/14314002/diff/1/chromeos/audio/cras_audio_han...
chromeos/audio/cras_audio_handler.cc:209: if (output_mute_on_ != mute_on) {
nit: early exit

https://codereview.chromium.org/14314002/diff/1/chromeos/audio/cras_audio_han...
chromeos/audio/cras_audio_handler.cc:218: void
CrasAudioHandler::InputMuteChanged(bool mute_on) {
nit: early exit

https://codereview.chromium.org/14314002/diff/1/chromeos/audio/cras_audio_han...
File chromeos/audio/cras_audio_handler.h (right):

https://codereview.chromium.org/14314002/diff/1/chromeos/audio/cras_audio_han...
chromeos/audio/cras_audio_handler.h:109: // Overriden from
CrasAuduiHandler::Observer.
s/Audui/Audio

https://codereview.chromium.org/14314002/diff/1/chromeos/audio/cras_audio_han...
chromeos/audio/cras_audio_handler.h:140: base::WeakPtrFactory<CrasAudioHandler>
weak_ptr_factory_;
This should be after pref_change_registrar_ so that pref_change_registrar_ can't
make any callbacks to this from its destructor (which happens after this is
mostly destroyed).
Or inherit from SupportsWeakPtr.

https://codereview.chromium.org/14314002/diff/1/chromeos/dbus/cras_audio_clie...
File chromeos/dbus/cras_audio_client.cc (right):

https://codereview.chromium.org/14314002/diff/1/chromeos/dbus/cras_audio_clie...
chromeos/dbus/cras_audio_client.cc:448: LOG(ERROR) <<
"CrasAudioClientStubImpl::SetOutputVolume volume=" << volume;
->VLOG here and below

[jennyz, 2013-04-18 01:21:19]

https://codereview.chromium.org/14314002/diff/1/chromeos/audio/audio_pref_nam...
File chromeos/audio/audio_pref_names.cc (right):

https://codereview.chromium.org/14314002/diff/1/chromeos/audio/audio_pref_nam...
chromeos/audio/audio_pref_names.cc:12: // chrome/common/pref_names.cc, once we
retire the old AudioHandler code.
On 2013/04/17 01:25:12, stevenjb (chromium) wrote:
> Does it make sense to change the old code to include this file instead?
It turns out that one of the audio pref, ie, kAudioCaptureAllowed is not
chromeos only, which is used by chrome in other places too. I end up with
refactoring all the pref related code to AudioPrefHandler and pass this object
into both old and new audio handlers. Please take a look.

https://codereview.chromium.org/14314002/diff/1/chromeos/audio/cras_audio_han...
File chromeos/audio/cras_audio_handler.cc (right):

https://codereview.chromium.org/14314002/diff/1/chromeos/audio/cras_audio_han...
chromeos/audio/cras_audio_handler.cc:178: : weak_ptr_factory_(this),
On 2013/04/17 01:25:12, stevenjb (chromium) wrote:
> ALLOW_THIS...

Done.

https://codereview.chromium.org/14314002/diff/1/chromeos/audio/cras_audio_han...
chromeos/audio/cras_audio_handler.cc:201: if (output_volume_ != volume) {
On 2013/04/17 01:25:12, stevenjb (chromium) wrote:
> nit: early exit instead

Done.

https://codereview.chromium.org/14314002/diff/1/chromeos/audio/cras_audio_han...
chromeos/audio/cras_audio_handler.cc:209: if (output_mute_on_ != mute_on) {
On 2013/04/17 01:25:12, stevenjb (chromium) wrote:
> nit: early exit

Done.

https://codereview.chromium.org/14314002/diff/1/chromeos/audio/cras_audio_han...
chromeos/audio/cras_audio_handler.cc:218: void
CrasAudioHandler::InputMuteChanged(bool mute_on) {
On 2013/04/17 01:25:12, stevenjb (chromium) wrote:
> nit: early exit

Done.

https://codereview.chromium.org/14314002/diff/1/chromeos/audio/cras_audio_han...
File chromeos/audio/cras_audio_handler.h (right):

https://codereview.chromium.org/14314002/diff/1/chromeos/audio/cras_audio_han...
chromeos/audio/cras_audio_handler.h:109: // Overriden from
CrasAuduiHandler::Observer.
On 2013/04/17 01:25:12, stevenjb (chromium) wrote:
> s/Audui/Audio

Done.

https://codereview.chromium.org/14314002/diff/1/chromeos/audio/cras_audio_han...
chromeos/audio/cras_audio_handler.h:140: base::WeakPtrFactory<CrasAudioHandler>
weak_ptr_factory_;
On 2013/04/17 01:25:12, stevenjb (chromium) wrote:
> This should be after pref_change_registrar_ so that pref_change_registrar_
can't
> make any callbacks to this from its destructor (which happens after this is
> mostly destroyed).
> Or inherit from SupportsWeakPtr.

Done.

https://codereview.chromium.org/14314002/diff/1/chromeos/dbus/cras_audio_clie...
File chromeos/dbus/cras_audio_client.cc (right):

https://codereview.chromium.org/14314002/diff/1/chromeos/dbus/cras_audio_clie...
chromeos/dbus/cras_audio_client.cc:448: LOG(ERROR) <<
"CrasAudioClientStubImpl::SetOutputVolume volume=" << volume;
On 2013/04/17 01:25:12, stevenjb (chromium) wrote:
> ->VLOG here and below

Done.

[jennyz, 2013-04-18 15:48:28]

I fixed all the little glitches with clang and tests. Ready for another look,
thanks!

[stevenjb, 2013-04-18 17:10:26]

Much nicer, thanks.
LGTM with nits.

https://codereview.chromium.org/14314002/diff/38001/chromeos/audio/audio_pref...
File chromeos/audio/audio_pref_observer.h (right):

https://codereview.chromium.org/14314002/diff/38001/chromeos/audio/audio_pref...
chromeos/audio/audio_pref_observer.h:18: virtual ~AudioPrefObserver() {}
nit: protected destructor

https://codereview.chromium.org/14314002/diff/38001/chromeos/audio/cras_audio...
File chromeos/audio/cras_audio_handler.cc (right):

https://codereview.chromium.org/14314002/diff/38001/chromeos/audio/cras_audio...
chromeos/audio/cras_audio_handler.cc:281: VLOG(1) << "Failed to retrieve audio
nodes data";
We may want this to be LOG(ERROR), or at least WARNING?

https://codereview.chromium.org/14314002/diff/38001/chromeos/dbus/cras_audio_...
File chromeos/dbus/cras_audio_client.cc (right):

https://codereview.chromium.org/14314002/diff/38001/chromeos/dbus/cras_audio_...
chromeos/dbus/cras_audio_client.cc:215: VLOG(1) << "Error reading signal from
cras:"
These should probably also be LOG(ERROR) or LOG(WARNING).

[jennyz, 2013-04-18 17:44:21]

https://codereview.chromium.org/14314002/diff/38001/chromeos/audio/audio_pref...
File chromeos/audio/audio_pref_observer.h (right):

https://codereview.chromium.org/14314002/diff/38001/chromeos/audio/audio_pref...
chromeos/audio/audio_pref_observer.h:18: virtual ~AudioPrefObserver() {}
On 2013/04/18 17:10:26, stevenjb (chromium) wrote:
> nit: protected destructor

Done.

https://codereview.chromium.org/14314002/diff/38001/chromeos/audio/cras_audio...
File chromeos/audio/cras_audio_handler.cc (right):

https://codereview.chromium.org/14314002/diff/38001/chromeos/audio/cras_audio...
chromeos/audio/cras_audio_handler.cc:281: VLOG(1) << "Failed to retrieve audio
nodes data";
On 2013/04/18 17:10:26, stevenjb (chromium) wrote:
> We may want this to be LOG(ERROR), or at least WARNING?

Done.

https://codereview.chromium.org/14314002/diff/38001/chromeos/dbus/cras_audio_...
File chromeos/dbus/cras_audio_client.cc (right):

https://codereview.chromium.org/14314002/diff/38001/chromeos/dbus/cras_audio_...
chromeos/dbus/cras_audio_client.cc:215: VLOG(1) << "Error reading signal from
cras:"
On 2013/04/18 17:10:26, stevenjb (chromium) wrote:
> These should probably also be LOG(ERROR) or LOG(WARNING).

Done.

[commit-bot: I haz the power, 2013-04-18 17:44:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jennyz@chromium.org/14314002/51001

[commit-bot: I haz the power, 2013-04-18 17:44:44]

Presubmit check for 14314002-51001 failed and returned exit status 1.

INFO:root:Found 15 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py

** Presubmit Messages **
Your #include order seems to be broken. Send mail to
marja@chromium.org if this is not the case.
      chrome/browser/ui/ash/volume_controller_browsertest_chromeos.cc:7

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    chrome/browser/ui/ash/volume_controller_browsertest_chromeos.cc
    chrome/browser/prefs/browser_prefs.cc
    chrome/chrome_browser_chromeos.gypi

Presubmit checks took 1.6s to calculate.

[jennyz, 2013-04-18 18:00:58]

derat@: would you please review:
hrome/browser/ui/ash/volume_controller_browsertest_chromeos.cc?
battre: Would you please review:
chrome/browser/prefs/browser_prefs.cc

These are small changes.

[jennyz, 2013-04-18 18:32:43]

jamescook@: would you please review:
hrome/browser/ui/ash/volume_controller_browsertest_chromeos.cc?

[James Cook, 2013-04-18 18:37:11]

On 2013/04/18 18:32:43, jennyz wrote:
> jamescook@: would you please review:
> hrome/browser/ui/ash/volume_controller_browsertest_chromeos.cc?

chrome/browser/ui/ash LGTM but...

I see a couple of new classes added here but no new tests. Is all this new code
covered by existing tests?  If not, what's the plan for testing it?

[Daniel Erat, 2013-04-18 19:25:30]

I'm finding the big picture confusing here.  We used to have a single
AudioHandler implementation that loaded/saved prefs and used an AudioMixer to
interact with hardware.  AudioMixer had two implementations, one for ALSA and
one for cras.

This change adds more classes (AudioPrefHandler, CrasAudioHandler).  Do you have
additional changes planned after this one to clean things up?  Is there a
document describing how everything fits together?

[Daniel Erat, 2013-04-18 19:26:49]

At the very least, every class should have a comment describing what it is and
what it does (as required by the style guide:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Class_Comments).

[jennyz, 2013-04-18 20:07:41]

AudioHandler is the old audio handler, and I am wring a new audio handler,
ie, CrasAudioHandler, which talks the new audio dbus apis. The plan is to
retire the old AudioHandler and audio library code, for M28, we will enable
the new audio handler code behind a flag, which is in my next cl, and
eventually clean up the old audio code. For the new audio handler code, i
put them at chromeos/audio. Since some the audio pref, is not Chromeos
only, eg. kAudioCaptureAllowed, is used by chrome media code as well, I
wrapped the audio pref related code in AudioPrefHandler and pass the
reference to the new audio handler under chromeos/audio. I will confirm
with dgred@ and chihchung@ again on the plan.

I will add description in the comment for each new class.


On Thu, Apr 18, 2013 at 12:25 PM, <derat@chromium.org> wrote:

> I'm finding the big picture confusing here.  We used to have a single
> AudioHandler implementation that loaded/saved prefs and used an AudioMixer
> to
> interact with hardware.  AudioMixer had two implementations, one for ALSA
> and
> one for cras.
>
> This change adds more classes (AudioPrefHandler, CrasAudioHandler).  Do
> you have
> additional changes planned after this one to clean things up?  Is there a
> document describing how everything fits together?
>
>
https://codereview.chromium.**org/14314002/<https://codereview.chromium.org/1...
>

[Daniel Erat, 2013-04-18 20:26:18]

Thanks.  Please also add TODOs at the tops of the class(es) that you plan
to retire.


On Thu, Apr 18, 2013 at 1:07 PM, Jenny Zhang(章瑛) <jennyz@chromium.org>wrote:

> AudioHandler is the old audio handler, and I am wring a new audio handler,
> ie, CrasAudioHandler, which talks the new audio dbus apis. The plan is to
> retire the old AudioHandler and audio library code, for M28, we will enable
> the new audio handler code behind a flag, which is in my next cl, and
> eventually clean up the old audio code. For the new audio handler code, i
> put them at chromeos/audio. Since some the audio pref, is not Chromeos
> only, eg. kAudioCaptureAllowed, is used by chrome media code as well, I
> wrapped the audio pref related code in AudioPrefHandler and pass the
> reference to the new audio handler under chromeos/audio. I will confirm
> with dgred@ and chihchung@ again on the plan.
>
> I will add description in the comment for each new class.
>
>
> On Thu, Apr 18, 2013 at 12:25 PM, <derat@chromium.org> wrote:
>
>> I'm finding the big picture confusing here.  We used to have a single
>> AudioHandler implementation that loaded/saved prefs and used an
>> AudioMixer to
>> interact with hardware.  AudioMixer had two implementations, one for ALSA
>> and
>> one for cras.
>>
>> This change adds more classes (AudioPrefHandler, CrasAudioHandler).  Do
>> you have
>> additional changes planned after this one to clean things up?  Is there a
>> document describing how everything fits together?
>>
>>
https://codereview.chromium.**org/14314002/<https://codereview.chromium.org/1...
>>
>
>

[jennyz, 2013-04-18 20:45:03]

On 2013/04/18 20:26:18, Daniel Erat wrote:
> Thanks.  Please also add TODOs at the tops of the class(es) that you plan
> to retire.
> 
> 
> On Thu, Apr 18, 2013 at 1:07 PM, Jenny Zhang(章瑛) <jennyz@chromium.org>wrote:
> 
> > AudioHandler is the old audio handler, and I am wring a new audio handler,
> > ie, CrasAudioHandler, which talks the new audio dbus apis. The plan is to
> > retire the old AudioHandler and audio library code, for M28, we will enable
> > the new audio handler code behind a flag, which is in my next cl, and
> > eventually clean up the old audio code. For the new audio handler code, i
> > put them at chromeos/audio. Since some the audio pref, is not Chromeos
> > only, eg. kAudioCaptureAllowed, is used by chrome media code as well, I
> > wrapped the audio pref related code in AudioPrefHandler and pass the
> > reference to the new audio handler under chromeos/audio. I will confirm
> > with dgred@ and chihchung@ again on the plan.
> >
> > I will add description in the comment for each new class.
> >
> >
> > On Thu, Apr 18, 2013 at 12:25 PM, <mailto:derat@chromium.org> wrote:
> >
> >> I'm finding the big picture confusing here.  We used to have a single
> >> AudioHandler implementation that loaded/saved prefs and used an
> >> AudioMixer to
> >> interact with hardware.  AudioMixer had two implementations, one for ALSA
> >> and
> >> one for cras.
> >>
> >> This change adds more classes (AudioPrefHandler, CrasAudioHandler).  Do
> >> you have
> >> additional changes planned after this one to clean things up?  Is there a
> >> document describing how everything fits together?
> >>
> >>
>
https://codereview.chromium.**org/14314002/%3Chttps://codereview.chromium.org...>
> >>
> >
> >

Done. Added comments for new classes and TODO and crbug.com/233310 for cleaning
up legacy code.

[commit-bot: I haz the power, 2013-04-18 20:45:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jennyz@chromium.org/14314002/72001

[Daniel Erat, 2013-04-18 20:55:22]

https://codereview.chromium.org/14314002/diff/72001/chrome/browser/chromeos/a...
File chrome/browser/chromeos/audio/audio_pref_handler_impl.h (right):

https://codereview.chromium.org/14314002/diff/72001/chrome/browser/chromeos/a...
chrome/browser/chromeos/audio/audio_pref_handler_impl.h:17: // Class which
implements AudioPrefHandler interface and register audio
nit: s/register/registers/

saying "which implements AudioPrefHandler interface" here isn't very useful,
since the "class AudioPrefHandlerImpl : public AudioPrefHandler" line says the
same thing.

[jennyz, 2013-04-18 21:18:38]

On 2013/04/18 20:55:22, Daniel Erat wrote:
>
https://codereview.chromium.org/14314002/diff/72001/chrome/browser/chromeos/a...
> File chrome/browser/chromeos/audio/audio_pref_handler_impl.h (right):
> 
>
https://codereview.chromium.org/14314002/diff/72001/chrome/browser/chromeos/a...
> chrome/browser/chromeos/audio/audio_pref_handler_impl.h:17: // Class which
> implements AudioPrefHandler interface and register audio
> nit: s/register/registers/
> 
> saying "which implements AudioPrefHandler interface" here isn't very useful,
> since the "class AudioPrefHandlerImpl : public AudioPrefHandler" line says the
> same thing.

Will fix in next cl.

[jennyz, 2013-04-19 03:28:30]

Committed patchset #8 manually as r195067 (presubmit successful).

[dgreid, 2013-04-19 04:20:18]

https://codereview.chromium.org/14314002/diff/72001/chromeos/audio/cras_audio...
File chromeos/audio/cras_audio_handler.cc (right):

https://codereview.chromium.org/14314002/diff/72001/chromeos/audio/cras_audio...
chromeos/audio/cras_audio_handler.cc:120:
SetOutputVolumeInternal(volume_percent);
Should these be reversed?  Is it possible that some amount of audio creeps out
at the previous volume level after the unmute and before the set volume?

https://codereview.chromium.org/14314002/diff/72001/chromeos/audio/cras_audio...
chromeos/audio/cras_audio_handler.cc:137: if (output_volume_ <=
kMuteThresholdPercent) {
If SetOutputVolumePercent(50) is called when currently at zero or one, will this
set the volume to 4 before setting it to 50?

https://codereview.chromium.org/14314002/diff/72001/chromeos/audio/cras_audio...
chromeos/audio/cras_audio_handler.cc:245:
SetOutputVolumeInternal(output_volume_);
similar comment to above about the order of this.  In general assert mute first
and unassert mute last.

[jennyz, 2013-04-19 16:48:18]

https://codereview.chromium.org/14314002/diff/72001/chromeos/audio/cras_audio...
File chromeos/audio/cras_audio_handler.cc (right):

https://codereview.chromium.org/14314002/diff/72001/chromeos/audio/cras_audio...
chromeos/audio/cras_audio_handler.cc:120:
SetOutputVolumeInternal(volume_percent);
On 2013/04/19 04:20:18, dgreid wrote:
> Should these be reversed?  Is it possible that some amount of audio creeps out
> at the previous volume level after the unmute and before the set volume?

This clones the exact logic we used in AudioHandler::SetVolumePercent, which is
the current behavior of audio UI. I tested it on lumpy, the audio works just
like the old AudioHandler. It should be pretty subtle behavior. I am not sure
user can tell the difference. For better order, do you suggest I move line 120
before line 116?

https://codereview.chromium.org/14314002/diff/72001/chromeos/audio/cras_audio...
chromeos/audio/cras_audio_handler.cc:137: if (output_volume_ <=
kMuteThresholdPercent) {
On 2013/04/19 04:20:18, dgreid wrote:
> If SetOutputVolumePercent(50) is called when currently at zero or one, will
this
> set the volume to 4 before setting it to 50?
It could if the audio is muted when user sets it to 50, but it will be set to 50
just a little later, user should not be able to tell the difference. This is
what AudioHander::SetMuted() does.

https://codereview.chromium.org/14314002/diff/72001/chromeos/audio/cras_audio...
chromeos/audio/cras_audio_handler.cc:245:
SetOutputVolumeInternal(output_volume_);
On 2013/04/19 04:20:18, dgreid wrote:
> similar comment to above about the order of this.  In general assert mute
first
> and unassert mute last.
Same as we discussed in previous comment.

[dgreid, 2013-04-22 16:23:26]

https://codereview.chromium.org/14314002/diff/72001/chromeos/audio/cras_audio...
File chromeos/audio/cras_audio_handler.cc (right):

https://codereview.chromium.org/14314002/diff/72001/chromeos/audio/cras_audio...
chromeos/audio/cras_audio_handler.cc:120:
SetOutputVolumeInternal(volume_percent);
On 2013/04/19 16:48:18, jennyz wrote:
> On 2013/04/19 04:20:18, dgreid wrote:
> > Should these be reversed?  Is it possible that some amount of audio creeps
out
> > at the previous volume level after the unmute and before the set volume?
> 
> This clones the exact logic we used in AudioHandler::SetVolumePercent, which
is
> the current behavior of audio UI. I tested it on lumpy, the audio works just
> like the old AudioHandler. It should be pretty subtle behavior. I am not sure
> user can tell the difference. For better order, do you suggest I move line 120
> before line 116?

I think that is safer.  It will most likely not happen often (i'd worry more
about mario and daisy) but might as well avoid it if possible.  Especially now
that we switched to using dbus witch adds and extra few process hops where
scheduling latency could be encountered.

https://codereview.chromium.org/14314002/diff/72001/chromeos/audio/cras_audio...
chromeos/audio/cras_audio_handler.cc:137: if (output_volume_ <=
kMuteThresholdPercent) {
On 2013/04/19 16:48:18, jennyz wrote:
> On 2013/04/19 04:20:18, dgreid wrote:
> > If SetOutputVolumePercent(50) is called when currently at zero or one, will
> this
> > set the volume to 4 before setting it to 50?
> It could if the audio is muted when user sets it to 50, but it will be set to
50
> just a little later, user should not be able to tell the difference. This is
> what AudioHander::SetMuted() does.

I was less worried about the user noticing and more about wasting cycles setting
the volume twice for no reason.

[jennyz, 2013-04-23 17:22:55]

https://codereview.chromium.org/14314002/diff/72001/chromeos/audio/cras_audio...
File chromeos/audio/cras_audio_handler.cc (right):

https://codereview.chromium.org/14314002/diff/72001/chromeos/audio/cras_audio...
chromeos/audio/cras_audio_handler.cc:120:
SetOutputVolumeInternal(volume_percent);
On 2013/04/22 16:23:26, dgreid wrote:
> On 2013/04/19 16:48:18, jennyz wrote:
> > On 2013/04/19 04:20:18, dgreid wrote:
> > > Should these be reversed?  Is it possible that some amount of audio creeps
> out
> > > at the previous volume level after the unmute and before the set volume?
> > 
> > This clones the exact logic we used in AudioHandler::SetVolumePercent, which
> is
> > the current behavior of audio UI. I tested it on lumpy, the audio works just
> > like the old AudioHandler. It should be pretty subtle behavior. I am not
sure
> > user can tell the difference. For better order, do you suggest I move line
120
> > before line 116?
> 
> I think that is safer.  It will most likely not happen often (i'd worry more
> about mario and daisy) but might as well avoid it if possible.  Especially now
> that we switched to using dbus witch adds and extra few process hops where
> scheduling latency could be encountered.
> 
Will changed in the next cl.

https://codereview.chromium.org/14314002/diff/72001/chromeos/audio/cras_audio...
chromeos/audio/cras_audio_handler.cc:137: if (output_volume_ <=
kMuteThresholdPercent) {
On 2013/04/22 16:23:26, dgreid wrote:
> On 2013/04/19 16:48:18, jennyz wrote:
> > On 2013/04/19 04:20:18, dgreid wrote:
> > > If SetOutputVolumePercent(50) is called when currently at zero or one,
will
> > this
> > > set the volume to 4 before setting it to 50?
> > It could if the audio is muted when user sets it to 50, but it will be set
to
> 50
> > just a little later, user should not be able to tell the difference. This is
> > what AudioHander::SetMuted() does.
> 
> I was less worried about the user noticing and more about wasting cycles
setting
> the volume twice for no reason.
> 
Shouldn't happen after the change made in SetOutputVolumePercent.

https://codereview.chromium.org/14314002/diff/72001/chromeos/audio/cras_audio...
chromeos/audio/cras_audio_handler.cc:245:
SetOutputVolumeInternal(output_volume_);
On 2013/04/19 16:48:18, jennyz wrote:
> On 2013/04/19 04:20:18, dgreid wrote:
> > similar comment to above about the order of this.  In general assert mute
> first
> > and unassert mute last.
> Same as we discussed in previous comment.
Changed in the next cl.