CRAS: pinned stream to enabled device is broken |
||||||||||
Issue descriptionCreates recording stream and pinned to an enabled device. Found out there's no stream added at all.
,
Aug 6
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/third_party/adhd/+/6e5451907b24258ee2308eca17b631b8a29abea7 commit 6e5451907b24258ee2308eca17b631b8a29abea7 Author: Hsin-Yu Chao <hychao@chromium.org> Date: Mon Aug 06 07:34:52 2018 CRAS: iodev_list - Fix pinned stream on already enabled dev An earlier commit 5e1e698 checks iodev's enable state to avoid multiple threads accessing the same iodev. This check breaks pinned stream usage, because when a pinned stream is connected, the pinned device doesn't have to be added to the enabled dev list. In other words, 'pinned' state and 'enabled' state of iodev are independent. Fix the problem by removing the check. The potential issue of multiple threads accessing pinned device will be addressed in future commits. Updated unittest AddRemovePinnedStream to cover this regression case. BUG= chromium:870648 TEST=unittest cras_test_client --capture_file /tmp/1 --pin_device <id> where <id> is an enabled iodev. Then use cras_test_client --dump_a to verify stream is added. Change-Id: Ie2b32b3478e92ac737a5d283c50f0bfbbd4e89fa Reviewed-on: https://chromium-review.googlesource.com/1161860 Commit-Ready: Hsinyu Chao <hychao@chromium.org> Tested-by: Hsinyu Chao <hychao@chromium.org> Reviewed-by: Dylan Reid <dgreid@chromium.org> [modify] https://crrev.com/6e5451907b24258ee2308eca17b631b8a29abea7/cras/src/server/cras_iodev_list.c [modify] https://crrev.com/6e5451907b24258ee2308eca17b631b8a29abea7/cras/src/tests/iodev_list_unittest.cc
,
Aug 7
Requesting merge to M69. Note that this eventually need merge to M68 to fix pinned stream breakage.
,
Aug 7
This bug requires manual review: M69 has already been promoted to the beta branch, so this requires manual review Please contact the milestone owner if you have questions. Owners: amineer@(Android), kariahda@(iOS), cindyb@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Aug 9
Merge approved, M69. Contact bhthompson@ for M68.
,
Aug 9
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/third_party/adhd/+/07db6801f9fd493be33727ba8ecb093282e5d7ac commit 07db6801f9fd493be33727ba8ecb093282e5d7ac Author: Hsin-Yu Chao <hychao@chromium.org> Date: Thu Aug 09 15:24:21 2018 CRAS: iodev_list - Fix pinned stream on already enabled dev An earlier commit 5e1e698 checks iodev's enable state to avoid multiple threads accessing the same iodev. This check breaks pinned stream usage, because when a pinned stream is connected, the pinned device doesn't have to be added to the enabled dev list. In other words, 'pinned' state and 'enabled' state of iodev are independent. Fix the problem by removing the check. The potential issue of multiple threads accessing pinned device will be addressed in future commits. Updated unittest AddRemovePinnedStream to cover this regression case. BUG= chromium:870648 TEST=unittest cras_test_client --capture_file /tmp/1 --pin_device <id> where <id> is an enabled iodev. Then use cras_test_client --dump_a to verify stream is added. Change-Id: Ie2b32b3478e92ac737a5d283c50f0bfbbd4e89fa Reviewed-on: https://chromium-review.googlesource.com/1161860 Commit-Ready: Hsinyu Chao <hychao@chromium.org> Tested-by: Hsinyu Chao <hychao@chromium.org> Reviewed-by: Dylan Reid <dgreid@chromium.org> (cherry picked from commit 6e5451907b24258ee2308eca17b631b8a29abea7) Reviewed-on: https://chromium-review.googlesource.com/1169483 Reviewed-by: Hsinyu Chao <hychao@chromium.org> Commit-Queue: Hsinyu Chao <hychao@chromium.org> [modify] https://crrev.com/07db6801f9fd493be33727ba8ecb093282e5d7ac/cras/src/server/cras_iodev_list.c [modify] https://crrev.com/07db6801f9fd493be33727ba8ecb093282e5d7ac/cras/src/tests/iodev_list_unittest.cc
,
Aug 13
,
Aug 14
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/third_party/adhd/+/89cee57095ff5a6d7cfa0ed8f29206f6fad22516 commit 89cee57095ff5a6d7cfa0ed8f29206f6fad22516 Author: Hsin-Yu Chao <hychao@chromium.org> Date: Tue Aug 14 05:35:23 2018 CRAS: iodev_list - Fix pinned stream on already enabled dev An earlier commit 5e1e698 checks iodev's enable state to avoid multiple threads accessing the same iodev. This check breaks pinned stream usage, because when a pinned stream is connected, the pinned device doesn't have to be added to the enabled dev list. In other words, 'pinned' state and 'enabled' state of iodev are independent. Fix the problem by removing the check. The potential issue of multiple threads accessing pinned device will be addressed in future commits. Updated unittest AddRemovePinnedStream to cover this regression case. BUG= chromium:870648 TEST=unittest cras_test_client --capture_file /tmp/1 --pin_device <id> where <id> is an enabled iodev. Then use cras_test_client --dump_a to verify stream is added. Change-Id: Ie2b32b3478e92ac737a5d283c50f0bfbbd4e89fa Reviewed-on: https://chromium-review.googlesource.com/1161860 Commit-Ready: Hsinyu Chao <hychao@chromium.org> Tested-by: Hsinyu Chao <hychao@chromium.org> Reviewed-by: Dylan Reid <dgreid@chromium.org> (cherry picked from commit 6e5451907b24258ee2308eca17b631b8a29abea7) Reviewed-on: https://chromium-review.googlesource.com/1173948 Reviewed-by: Hsinyu Chao <hychao@chromium.org> Commit-Queue: Hsinyu Chao <hychao@chromium.org> [modify] https://crrev.com/89cee57095ff5a6d7cfa0ed8f29206f6fad22516/cras/src/server/cras_iodev_list.c [modify] https://crrev.com/89cee57095ff5a6d7cfa0ed8f29206f6fad22516/cras/src/tests/iodev_list_unittest.cc
,
Aug 16
This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible! If all merges have been completed, please remove any remaining Merge-Approved labels from this issue. Thanks for your time! To disable nags, add the Disable-Nags label. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Aug 17
,
Sep 3
Leaving one note and mark fixed. web.skype.com seems to be using pinned stream API and hit this bug in earlier M68. |
||||||||||
►
Sign in to add a comment |
||||||||||
Comment 1 by hychao@chromium.org
, Aug 3