audio: CrasAudioHandler::SetActiveDevices does not always properly disable active nodes |
||||
Issue descriptionAfter the following sequence of set active devices operations, Chrome (primarily chrome.audio apps API) and cras would end up seeing different set of active nodes: SetActiveNodes([A]); - sets A as primary active device SetActiveNodes([A, B]); SetActiveNodes([B]); At this point only B device should be active, but in reality both A and B are active - Chrome does think that only B is active, though. When the primary active device is not present in new active device set, CrasAudioHandler will first call ChangeActiveDevice in order to update primary active device. This does the following: - resets CrasAudioHandlers internal state of all devices so only the new primary device is marcked active - makes a request to cras to select new primary active device (using SetActive*Node dbus method) At this point set of active devices as seen by Chrome will not reflect the actual state in cras - Chrome will assume that all but B device were deactivated, while cras does not actually deactivate any active nodes in when new active device is already active (done in cras_iodev_list_select_node). As a result, A will remain active, even though Chrome thinks that is not the case. For graphics-related bugs, please copy/paste the contents of the about:gpu page at the end of this report.
,
Jan 11 2017
OK, I think the problem is that SetActive*Node does not deactivate all nodes in this case. cras_iodev_list_select_node bails out early if new node is already active (which is the case here), before any node is deactivated (https://cs.corp.google.com/chromeos_public/src/third_party/adhd/cras/src/server/cras_iodev_list.c?rcl=bf1f4c514711b3b33f82006f8937c61039a07c71&l=1057).
,
Jan 11 2017
Ah you're right. We should fix that so the behavior is consistent.
,
Jan 12 2017
,
Jan 13 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/third_party/adhd/+/2a31c92503421def21d141b16acc788d89c5b9cf commit 2a31c92503421def21d141b16acc788d89c5b9cf Author: Toni Barzic <tbarzic@google.com> Date: Wed Jan 11 19:23:32 2017 CRAS: When setting new active node, disable all other nodes Previously, new node selection would bail out early if the new node was already active - this did not take into account that this might not be the only active node, so other nodes remained active. To be consistent with the case the new node was not active previously, when all nodes but the new one are disabled, make sure that the new node becomes the only enabled node when it's enabled using SetActive*Node method. BUG=chromium:672665, chromium:679852 TEST=Using chrome.audio apps API, (where A, B are ID of distinct existing audio nodes): setActiveDevices([A]); setActiveDevices([A, B]); setActiveDevices([B]); Verify that only audio node B is enabled. Change-Id: Ic602d31183286caa36dd16e2ae8ec6ef03791f68 Reviewed-on: https://chromium-review.googlesource.com/427043 Commit-Ready: Toni Barzic <tbarzic@chromium.org> Tested-by: Toni Barzic <tbarzic@chromium.org> Reviewed-by: Hsinyu Chao <hychao@chromium.org> [modify] https://crrev.com/2a31c92503421def21d141b16acc788d89c5b9cf/cras/src/server/cras_iodev_list.c [modify] https://crrev.com/2a31c92503421def21d141b16acc788d89c5b9cf/cras/src/tests/iodev_list_unittest.cc
,
Jan 18 2017
Fix verified. Good job!
,
Jan 23 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/third_party/adhd/+/00910cb931e5900709497f4b19fb9a5b7bd3affa commit 00910cb931e5900709497f4b19fb9a5b7bd3affa Author: Toni Barzic <tbarzic@google.com> Date: Wed Jan 11 19:23:32 2017 CRAS: When setting new active node, disable all other nodes Previously, new node selection would bail out early if the new node was already active - this did not take into account that this might not be the only active node, so other nodes remained active. To be consistent with the case the new node was not active previously, when all nodes but the new one are disabled, make sure that the new node becomes the only enabled node when it's enabled using SetActive*Node method. BUG=chromium:672665, chromium:679852 TEST=Using chrome.audio apps API, (where A, B are ID of distinct existing audio nodes): setActiveDevices([A]); setActiveDevices([A, B]); setActiveDevices([B]); Verify that only audio node B is enabled. Change-Id: Ic602d31183286caa36dd16e2ae8ec6ef03791f68 Reviewed-on: https://chromium-review.googlesource.com/427043 Commit-Ready: Toni Barzic <tbarzic@chromium.org> Tested-by: Toni Barzic <tbarzic@chromium.org> Reviewed-by: Hsinyu Chao <hychao@chromium.org> (cherry picked from commit 2a31c92503421def21d141b16acc788d89c5b9cf) Reviewed-on: https://chromium-review.googlesource.com/431295 Reviewed-by: Toni Barzic <tbarzic@chromium.org> Commit-Queue: Toni Barzic <tbarzic@chromium.org> [modify] https://crrev.com/00910cb931e5900709497f4b19fb9a5b7bd3affa/cras/src/server/cras_iodev_list.c [modify] https://crrev.com/00910cb931e5900709497f4b19fb9a5b7bd3affa/cras/src/tests/iodev_list_unittest.cc
,
Jan 23 2017
|
||||
►
Sign in to add a comment |
||||
Comment 1 by hychao@chromium.org
, Jan 11 2017