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

Issue 912457 link

Starred by 1 user

Issue metadata

Status: Started
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Bug



Sign in to add a comment

audio_CrasOutputStress failure

Project Member Reported by yuhsuan@chromium.org, Dec 6

Issue description

Cc: dgreid@chromium.org
For input_only, I found the easy way to reproduce it.
1. Add stream A
2. Add stream B and remove stream A
3. Add stream C and remove stream B
4. Add stream D and remove stream C
5. Repeat...
And then we can get the very high hw_level of input device.

The reason is 
1. When the first stream A is added, the hw_level is between 0 ~ 1024. (Support block size is 1024.)
2. And then we add new stream B and the hw_level now is 512. (512 is an example, it can be any number.)
3. Suppose we can remove the stream A at the same time. The current hw_level is 512.
4. The stream B will read captured samples until its next_cb_ts(1024 frames). At that time, the hw_level is 1536.
5. The new hw_level is between 512 ~ 1536.

The attachment is a picture for details.
overrun.png
61.2 KB View Download
For output_only, here are steps to reproduce it.
1. Add stream A
2. Remove stream A and add stream B
3. Remove stream B and add stream C
4. Remove stream C and add stream D
5. Repeat...

It is because there are still the remaining samples in the device after removing streams. If we add new stream immediately, the new samples will append to old samples. Here is an example to make it easier to understand.
1. When the first stream A is added, the hw_level is between 1024 ~ 2048. (Support block size is 1024.)
2. And then we remove stream A and the hw_level now is 1500. (1500 is an example, it can be any number.)
3. Suppose we can add new stream B at the same time. The current hw_level is 1500.
4. The new samples are written into device. The hw_level becomes 1500 + 1024 = 2524.
5. The new hw_level is between 2524 ~ 3548

Actually, audio_thread enters no_stream state when we remove all streams. And resets hw_ptr to number of remaining samples after adding new stream. Since it does not affect result, I omit it above.

The attachment is a picture for details.
output.png
71.7 KB View Download
For input problem, I think we can ignore first next_cb_ts to fix it. So the stream will wake up when it gets enough samples to capture. Here is a CL on crrev.com/c/1369344

For output problem, if we set appl_ptr to min_cb_level instead of number of samples remaining, the problem can easily be fixed (Just remove adjust_appl_ptr_samples_remaining function from source code). But I'm not sure whether it is a correct behavior. Maybe users don't want CRAS to cut the audio. If CRAS doesn't cut it, mixing new streams and old ones is also weird. 

By the way, this problem only happens when the time between removing and adding streams is less than 40ms. (For 48000 rate and 1024 block_size, 1024 * 2 / 48000 = 0.043)
I'm not sure whether it can be triggered under normal usage. (Youtube spends about 1s to switch audios and build-in audio player spends about 40ms to switch. But I can not observe this issue on build-in audio player.)

Jimmy and Dylan, do you have any suggestions on this problem? Thanks.
Labels: M-72
We have seen CFM feedback showing unreasonable high input level. Let's targeting fix this in M72 to see if that helps.
Project Member

Comment 5 by bugdroid1@chromium.org, Dec 14

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/adhd/+/a2021649d242b7dd022d93093dc970381c7aa1c4

commit a2021649d242b7dd022d93093dc970381c7aa1c4
Author: Yu-Hsuan Hsu <yuhsuan@chromium.org>
Date: Fri Dec 14 03:28:19 2018

CRAS: audio_thread - Modify init_cb_ts to avoid hw_level rise

If we set the init_cb_ts of the new input stream to now +
sleep_interval_ts, the hw_level of input device will rise. It is because
the stream need to wait until next_cb_ts to read no matter it already
has enough samples or not.

We can set init_cb_ts to the time when stream gets enough samples to
post. It can resolve this issue.

BUG=chromium:912457
TEST=Can pass audio_CrasOutputStress.input_only test on peppy relibly.

Change-Id: I4bf6598438647cdbbe64c91c89999a033d05e109
Reviewed-on: https://chromium-review.googlesource.com/1369344
Commit-Ready: ChromeOS CL Exonerator Bot <chromiumos-cl-exonerator@appspot.gserviceaccount.com>
Tested-by: Yu-Hsuan Hsu <yuhsuan@chromium.org>
Reviewed-by: Hsinyu Chao <hychao@chromium.org>

[modify] https://crrev.com/a2021649d242b7dd022d93093dc970381c7aa1c4/cras/src/server/dev_stream.c
[modify] https://crrev.com/a2021649d242b7dd022d93093dc970381c7aa1c4/cras/src/server/audio_thread.c
[modify] https://crrev.com/a2021649d242b7dd022d93093dc970381c7aa1c4/cras/src/tests/audio_thread_unittest.cc

Labels: Merge-Request-72
Status: Started (was: Assigned)
Request merge the CL from comment #5 to R72. Hoping to help the delay issue in b/119388318
Project Member

Comment 7 by sheriffbot@chromium.org, Jan 10

Labels: -Merge-Request-72 Merge-Review-72 Hotlist-Merge-Review
This bug requires manual review: M72 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: govind@(Android), kariahda@(iOS), djmm@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Cc: djmm@chromium.org
Labels: -Merge-Review-72 Merge-Approved-72
Project Member

Comment 10 by bugdroid1@chromium.org, Jan 14

Labels: merge-merged-release-R72-11316.B
The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/adhd/+/7975105c91e3639c49ef5761633f2cb4880f3591

commit 7975105c91e3639c49ef5761633f2cb4880f3591
Author: Yu-Hsuan Hsu <yuhsuan@chromium.org>
Date: Mon Jan 14 04:07:12 2019

CRAS: audio_thread - Modify init_cb_ts to avoid hw_level rise

If we set the init_cb_ts of the new input stream to now +
sleep_interval_ts, the hw_level of input device will rise. It is because
the stream need to wait until next_cb_ts to read no matter it already
has enough samples or not.

We can set init_cb_ts to the time when stream gets enough samples to
post. It can resolve this issue.

BUG=chromium:912457
TEST=Can pass audio_CrasOutputStress.input_only test on peppy relibly.

Change-Id: I4bf6598438647cdbbe64c91c89999a033d05e109
Reviewed-on: https://chromium-review.googlesource.com/1369344
Commit-Ready: ChromeOS CL Exonerator Bot <chromiumos-cl-exonerator@appspot.gserviceaccount.com>
Tested-by: Yu-Hsuan Hsu <yuhsuan@chromium.org>
Reviewed-by: Hsinyu Chao <hychao@chromium.org>
(cherry picked from commit a2021649d242b7dd022d93093dc970381c7aa1c4)
Reviewed-on: https://chromium-review.googlesource.com/c/1408671
Commit-Queue: Yu-Hsuan Hsu <yuhsuan@chromium.org>

[modify] https://crrev.com/7975105c91e3639c49ef5761633f2cb4880f3591/cras/src/server/dev_stream.c
[modify] https://crrev.com/7975105c91e3639c49ef5761633f2cb4880f3591/cras/src/server/audio_thread.c
[modify] https://crrev.com/7975105c91e3639c49ef5761633f2cb4880f3591/cras/src/tests/audio_thread_unittest.cc

Project Member

Comment 11 by bugdroid1@chromium.org, Jan 14

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/adhd/+/4bd24adff6e91ca6b387dacbe9dba37f8c26b55d

commit 4bd24adff6e91ca6b387dacbe9dba37f8c26b55d
Author: Yu-Hsuan Hsu <yuhsuan@chromium.org>
Date: Mon Jan 14 04:50:23 2019

CRAS: audio_thread - Fix the incomplete cherry-pick

The CL:1408671 was cherry-picked from CL:1369344.
But there was something not cherry-picked successfully.
This change is to fix these errors.

BUG=chromium:912457
TEST=Can pass audio_CrasOutputStress.input_only test on peppy relibly.

Change-Id: I5a4abc005d1d085231543c2b95607e7e575471f5
Reviewed-on: https://chromium-review.googlesource.com/c/1408674
Reviewed-by: Hsinyu Chao <hychao@chromium.org>
Commit-Queue: Yu-Hsuan Hsu <yuhsuan@chromium.org>
Tested-by: Yu-Hsuan Hsu <yuhsuan@chromium.org>

[modify] https://crrev.com/4bd24adff6e91ca6b387dacbe9dba37f8c26b55d/cras/src/server/audio_thread.c

Project Member

Comment 12 by sheriffbot@chromium.org, Jan 14

Cc: hychao@chromium.org djmm@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
Project Member

Comment 13 by sheriffbot@chromium.org, Jan 18 (4 days ago)

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

Comment 14 by hychao@chromium.org, Jan 19 (4 days ago)

Labels: -Merge-Approved-72 Merge-Merged
Cherry-pick done to fix the "input" part of this problem in M72.

Still need to address "output" stress failure.

Sign in to add a comment