Supports multiple active input/output audio nodes in CrasAudioHandler.

Supports multiple active input/output audio nodes in CrasAudioHandler.

BUG=402072
TBR=xiyuan,jamescook

Committed: https://crrev.com/b47947f8151421e8501032dc0b6449f4fecb85e2
Cr-Commit-Position: refs/heads/master@{#296593}

[jennyz, 2014-09-22 23:48:03]

jennyz@chromium.org changed reviewers:
+ hychao@chromium.org, rkc@chromium.org

[rkc, 2014-09-24 01:22:44]

Mostly looks good, just need a few clarifications.

https://codereview.chromium.org/596603002/diff/40001/chromeos/audio/cras_audi...
File chromeos/audio/cras_audio_handler.cc (right):

https://codereview.chromium.org/596603002/diff/40001/chromeos/audio/cras_audi...
chromeos/audio/cras_audio_handler.cc:235: // the only active node.
Why?

https://codereview.chromium.org/596603002/diff/40001/chromeos/audio/cras_audi...
chromeos/audio/cras_audio_handler.cc:531: // devices. For input devices, we
don't restore their gain value so far.
I don't recall the reason exactly, but I believe the reason we weren't
storing/restoring input gain was as a hack to fix something, correct? Did we
ever decide if this was the correct behavior we wanted long term?

https://codereview.chromium.org/596603002/diff/40001/chromeos/audio/cras_audi...
chromeos/audio/cras_audio_handler.cc:646: // If GetNodes is not triggered by
NodesChanged signal,
This code seems unused. Everywhere that GetNodes is being called from is with
parameter set to true. If it isn't being used, we should check it in once it is
being used.

[hychao, 2014-09-24 04:25:47]

lgtm

Overall looks good to me, thanks for the work!

https://codereview.chromium.org/596603002/diff/40001/chromeos/audio/cras_audi...
File chromeos/audio/cras_audio_handler.cc (right):

https://codereview.chromium.org/596603002/diff/40001/chromeos/audio/cras_audi...
chromeos/audio/cras_audio_handler.cc:237: HasAdditionalActiveInputNode()) {
would this be simpler if we just don't allow the primary active node be removed?

https://codereview.chromium.org/596603002/diff/40001/chromeos/audio/cras_audi...
File chromeos/audio/cras_audio_handler.h (right):

https://codereview.chromium.org/596603002/diff/40001/chromeos/audio/cras_audi...
chromeos/audio/cras_audio_handler.h:251: // Calling dbus to get nodes data.
|nodes_changed| is true if we are caling
nit: s/caling/calling/

https://codereview.chromium.org/596603002/diff/40001/chromeos/audio/cras_audi...
File chromeos/audio/cras_audio_handler_unittest.cc (right):

https://codereview.chromium.org/596603002/diff/40001/chromeos/audio/cras_audi...
chromeos/audio/cras_audio_handler_unittest.cc:388: TEST_F(CrasAudioHandlerTest,
SetKeyboardMicActive) {
Thanks for adding the test for this!

[jennyz, 2014-09-24 18:01:20]

https://codereview.chromium.org/596603002/diff/40001/chromeos/audio/cras_audi...
File chromeos/audio/cras_audio_handler.cc (right):

https://codereview.chromium.org/596603002/diff/40001/chromeos/audio/cras_audi...
chromeos/audio/cras_audio_handler.cc:235: // the only active node.
On 2014/09/24 01:22:43, Rahul Chaturvedi wrote:
> Why?
CrasAudioHandler and Cras need to consistently maintain the primary active node
at both sides, which is set by SetActive* API. we do assume there will be a
primary active node at all times if there are any active nodes. If we remove the
primary active node while still keeping other active nodes, then
CrasAudioHandler has no way to predicate which active node will be selected by
Cras as the new primary active node. It will mess up the state of
CrasAudioHandler. From the use case of cras extension, it  will be much neat
just to remove all active nodes, then add back a new set of active nodes, like
what we did in SetActiveDevices. Until we have a perfect solution to deprecated
the primary active node concept, this might be easier way to keep the state
clean.Based on hychao's suggestion, we just do not allow remove the primary
active node, this is even simpler.

https://codereview.chromium.org/596603002/diff/40001/chromeos/audio/cras_audi...
chromeos/audio/cras_audio_handler.cc:237: HasAdditionalActiveInputNode()) {
On 2014/09/24 04:25:47, hychao wrote:
> would this be simpler if we just don't allow the primary active node be
removed?

Yes, that's even better.

https://codereview.chromium.org/596603002/diff/40001/chromeos/audio/cras_audi...
chromeos/audio/cras_audio_handler.cc:531: // devices. For input devices, we
don't restore their gain value so far.
On 2014/09/24 01:22:43, Rahul Chaturvedi wrote:
> I don't recall the reason exactly, but I believe the reason we weren't
> storing/restoring input gain was as a hack to fix something, correct? Did we
> ever decide if this was the correct behavior we wanted long term?

I also remembered this might be a hack to fix some input device problem, but I
don't remember exactly what the problem is. One thing I could think about is we
don't have UI for user to adjust the input capture gain, if the input capture
gain was set to a very low number, user could run into problem with input
device. For the same reason, we don't persist the mute setting for input device
as well. I would rather keep this as IS for this cl, majorly not introduce
another issue for this cl. We can fix this problem in a different cl if we want
to.

https://codereview.chromium.org/596603002/diff/40001/chromeos/audio/cras_audi...
chromeos/audio/cras_audio_handler.cc:646: // If GetNodes is not triggered by
NodesChanged signal,
On 2014/09/24 01:22:43, Rahul Chaturvedi wrote:
> This code seems unused. Everywhere that GetNodes is being called from is with
> parameter set to true. If it isn't being used, we should check it in once it
is
> being used.
Yes, this is not used in the current code, it could be useful for diagnostic
purpose if we just want to check the nodes data from Cras. I could remove it if
you really don't like it.

https://codereview.chromium.org/596603002/diff/40001/chromeos/audio/cras_audi...
File chromeos/audio/cras_audio_handler.h (right):

https://codereview.chromium.org/596603002/diff/40001/chromeos/audio/cras_audi...
chromeos/audio/cras_audio_handler.h:251: // Calling dbus to get nodes data.
|nodes_changed| is true if we are caling
On 2014/09/24 04:25:47, hychao wrote:
> nit: s/caling/calling/

Done.

[rkc, 2014-09-24 19:45:30]

https://codereview.chromium.org/596603002/diff/40001/chromeos/audio/cras_audi...
File chromeos/audio/cras_audio_handler.cc (right):

https://codereview.chromium.org/596603002/diff/40001/chromeos/audio/cras_audi...
chromeos/audio/cras_audio_handler.cc:235: // the only active node.
I agree - we should not allow this particular method to remove the primary
active node at all.
Though it should be possible to remove all nodes via the audio API - which
looking at the code will still function fine since the audio API doesn't
actually remove nodes individually but calls RemoveAllActiveNodes instead.

On 2014/09/24 18:01:20, jennyz wrote:
> On 2014/09/24 01:22:43, Rahul Chaturvedi wrote:
> > Why?
> CrasAudioHandler and Cras need to consistently maintain the primary active
node
> at both sides, which is set by SetActive* API. we do assume there will be a
> primary active node at all times if there are any active nodes. If we remove
the
> primary active node while still keeping other active nodes, then
> CrasAudioHandler has no way to predicate which active node will be selected by
> Cras as the new primary active node. It will mess up the state of
> CrasAudioHandler. From the use case of cras extension, it  will be much neat
> just to remove all active nodes, then add back a new set of active nodes, like
> what we did in SetActiveDevices. Until we have a perfect solution to
deprecated
> the primary active node concept, this might be easier way to keep the state
> clean.Based on hychao's suggestion, we just do not allow remove the primary
> active node, this is even simpler.

https://codereview.chromium.org/596603002/diff/40001/chromeos/audio/cras_audi...
chromeos/audio/cras_audio_handler.cc:531: // devices. For input devices, we
don't restore their gain value so far.
That sounds fine. Please do create a bug to track the issue and link it here
though.

On 2014/09/24 18:01:20, jennyz wrote:
> On 2014/09/24 01:22:43, Rahul Chaturvedi wrote:
> > I don't recall the reason exactly, but I believe the reason we weren't
> > storing/restoring input gain was as a hack to fix something, correct? Did we
> > ever decide if this was the correct behavior we wanted long term?
> 
> I also remembered this might be a hack to fix some input device problem, but I
> don't remember exactly what the problem is. One thing I could think about is
we
> don't have UI for user to adjust the input capture gain, if the input capture
> gain was set to a very low number, user could run into problem with input
> device. For the same reason, we don't persist the mute setting for input
device
> as well. I would rather keep this as IS for this cl, majorly not introduce
> another issue for this cl. We can fix this problem in a different cl if we
want
> to.

https://codereview.chromium.org/596603002/diff/40001/chromeos/audio/cras_audi...
chromeos/audio/cras_audio_handler.cc:646: // If GetNodes is not triggered by
NodesChanged signal,
I am really uncomfortable checking in unused code. I'm a big fan of YAGNI :)

On 2014/09/24 18:01:20, jennyz wrote:
> On 2014/09/24 01:22:43, Rahul Chaturvedi wrote:
> > This code seems unused. Everywhere that GetNodes is being called from is
with
> > parameter set to true. If it isn't being used, we should check it in once it
> is
> > being used.
> Yes, this is not used in the current code, it could be useful for diagnostic
> purpose if we just want to check the nodes data from Cras. I could remove it
if
> you really don't like it.

https://codereview.chromium.org/596603002/diff/60001/chromeos/audio/cras_audi...
File chromeos/audio/cras_audio_handler.cc (right):

https://codereview.chromium.org/596603002/diff/60001/chromeos/audio/cras_audi...
chromeos/audio/cras_audio_handler.cc:742: bool
CrasAudioHandler::HasAdditionalActiveOutputNode() const {
No longer used.

https://codereview.chromium.org/596603002/diff/60001/chromeos/audio/cras_audi...
chromeos/audio/cras_audio_handler.cc:754: bool
CrasAudioHandler::HasAdditionalActiveInputNode() const {
Ditto.

[jennyz, 2014-09-24 20:47:18]

https://codereview.chromium.org/596603002/diff/40001/chromeos/audio/cras_audi...
File chromeos/audio/cras_audio_handler.cc (right):

https://codereview.chromium.org/596603002/diff/40001/chromeos/audio/cras_audi...
chromeos/audio/cras_audio_handler.cc:531: // devices. For input devices, we
don't restore their gain value so far.
On 2014/09/24 19:45:30, Rahul Chaturvedi wrote:
> That sounds fine. Please do create a bug to track the issue and link it here
> though.
> 
> On 2014/09/24 18:01:20, jennyz wrote:
> > On 2014/09/24 01:22:43, Rahul Chaturvedi wrote:
> > > I don't recall the reason exactly, but I believe the reason we weren't
> > > storing/restoring input gain was as a hack to fix something, correct? Did
we
> > > ever decide if this was the correct behavior we wanted long term?
> > 
> > I also remembered this might be a hack to fix some input device problem, but
I
> > don't remember exactly what the problem is. One thing I could think about is
> we
> > don't have UI for user to adjust the input capture gain, if the input
capture
> > gain was set to a very low number, user could run into problem with input
> > device. For the same reason, we don't persist the mute setting for input
> device
> > as well. I would rather keep this as IS for this cl, majorly not introduce
> > another issue for this cl. We can fix this problem in a different cl if we
> want
> > to.
> 

Done.

https://codereview.chromium.org/596603002/diff/40001/chromeos/audio/cras_audi...
chromeos/audio/cras_audio_handler.cc:646: // If GetNodes is not triggered by
NodesChanged signal,
On 2014/09/24 19:45:29, Rahul Chaturvedi wrote:
> I am really uncomfortable checking in unused code. I'm a big fan of YAGNI :)
> 
> On 2014/09/24 18:01:20, jennyz wrote:
> > On 2014/09/24 01:22:43, Rahul Chaturvedi wrote:
> > > This code seems unused. Everywhere that GetNodes is being called from is
> with
> > > parameter set to true. If it isn't being used, we should check it in once
it
> > is
> > > being used.
> > Yes, this is not used in the current code, it could be useful for diagnostic
> > purpose if we just want to check the nodes data from Cras. I could remove it
> if
> > you really don't like it.
> 

Done.

https://codereview.chromium.org/596603002/diff/60001/chromeos/audio/cras_audi...
File chromeos/audio/cras_audio_handler.cc (right):

https://codereview.chromium.org/596603002/diff/60001/chromeos/audio/cras_audi...
chromeos/audio/cras_audio_handler.cc:742: bool
CrasAudioHandler::HasAdditionalActiveOutputNode() const {
On 2014/09/24 19:45:30, Rahul Chaturvedi wrote:
> No longer used.

Done.

https://codereview.chromium.org/596603002/diff/60001/chromeos/audio/cras_audi...
chromeos/audio/cras_audio_handler.cc:754: bool
CrasAudioHandler::HasAdditionalActiveInputNode() const {
On 2014/09/24 19:45:30, Rahul Chaturvedi wrote:
> Ditto.

Done.

[rkc, 2014-09-24 22:10:16]

lgtm
Thanks for doing this :)

[jennyz, 2014-09-24 23:26:52]

The CQ bit was checked by jennyz@chromium.org

[commit-bot: I haz the power, 2014-09-24 23:30:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/596603002/80001

[commit-bot: I haz the power, 2014-09-24 23:45:15]

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

[commit-bot: I haz the power, 2014-09-24 23:45:16]

Try jobs failed on following builders:
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jennyz, 2014-09-25 00:11:46]

The CQ bit was checked by jennyz@chromium.org

[commit-bot: I haz the power, 2014-09-25 00:13:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/596603002/80001

[commit-bot: I haz the power, 2014-09-25 00:42:13]

Committed patchset #5 (id:80001) as 80bbe2036e18b9b2ca6833987104c4f6ff29253c

[commit-bot: I haz the power, 2014-09-25 00:42:51]

Patchset 5 (id:??) landed as
https://crrev.com/b47947f8151421e8501032dc0b6449f4fecb85e2
Cr-Commit-Position: refs/heads/master@{#296593}