New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 870648 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Sep 3
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Bug



Sign in to add a comment

CRAS: pinned stream to enabled device is broken

Project Member Reported by hychao@chromium.org, Aug 3

Issue description

Creates recording stream and pinned to an enabled device.
Found out there's no stream added at all.
 
More background:
willhui@ reported this issue for rtanalytics service. This is a cras client using pinned recording stream.

The bug was in my earlier commit to fix potential crash (  issue 863823  ), and the commit was cherry-picked to R68.

I have just uploaded fix to https://chromium-review.googlesource.com/c/chromiumos/third_party/adhd/+/1161860

and we should merge it to 70, 69, 68.
Project Member

Comment 2 by bugdroid1@chromium.org, 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

Cc: cindyb@chromium.org
Labels: Merge-Request-69
Requesting merge to M69.

Note that this eventually need merge to M68 to fix pinned stream breakage.
Project Member

Comment 4 by sheriffbot@chromium.org, Aug 7

Labels: -Merge-Request-69 Merge-Review-69 Hotlist-Merge-Review
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
Labels: -Merge-Review-69 Merge-Approved-69
Merge approved, M69. Contact bhthompson@ for M68.
Project Member

Comment 6 by bugdroid1@chromium.org, Aug 9

Labels: merge-merged-release-R69-10895.B
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

Labels: -Hotlist-Merge-Review Merge-Approved-68
Project Member

Comment 8 by bugdroid1@chromium.org, Aug 14

Labels: merge-merged-release-R68-10718.B
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

Project Member

Comment 9 by sheriffbot@chromium.org, Aug 16

Cc: bhthompson@google.com cindyb@google.com
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
Labels: -Merge-Approved-68 -Merge-Approved-69
Status: Fixed (was: Started)
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