I think we can fix this problem by these method.

1. Follow the stream wake up time even if the stream can not be fetched.
It should not worsen the result because audio thread can wake up at any time. 
But if we don't skip the stream wake up time, we can fill frames into device earlier to avoid underrun happening.

2. If there are no space in the share memory, we can just skip this fetch and move it to the next callback time.
It is because we already have enough data. We can easily skip one fetch. (It usually happen when the rate of audio device is not accurate.)

By adding these changes, the hw_level looks better now.

Deleted: figure2.png 32.8 KB

	figure2.png 32.8 KB View Download

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

commit fd8747c7e7ee2bef1a83c204987a47ccad1be4e0
Author: Yu-Hsuan Hsu <yuhsuan@chromium.org>
Date: Tue Oct 16 17:30:55 2018

CRAS: Do not ignore next_cb_ts of stream when share memory is full

If device buffer size is not large enough, we might leave some frames in
share memory. If we ignore next callback time of stream and use hardware
level in device to compute next wake up time, it might cause we wake up
too late and trigger underruns.

The solution to continue using the stream wake up time when shared memory
is full. Andi then we can skip one fetch if there are enough frames in
the share memory.

BUG= chromium:894694 
TEST=The underruns do not happen even if there is unaccurate rate
on device. (grunt)

Change-Id: I48294466ef8f79617ffaf390a58f18f4841345b0
Reviewed-on: https://chromium-review.googlesource.com/1278611
Commit-Ready: Yu-Hsuan Hsu <yuhsuan@chromium.org>
Tested-by: Yu-Hsuan Hsu <yuhsuan@chromium.org>
Reviewed-by: Cheng-Yi Chiang <cychiang@chromium.org>

[modify] https://crrev.com/fd8747c7e7ee2bef1a83c204987a47ccad1be4e0/cras/src/tests/dev_stream_unittest.cc
[modify] https://crrev.com/fd8747c7e7ee2bef1a83c204987a47ccad1be4e0/cras/src/common/cras_types.h
[modify] https://crrev.com/fd8747c7e7ee2bef1a83c204987a47ccad1be4e0/cras/src/tests/cras_test_client.c
[modify] https://crrev.com/fd8747c7e7ee2bef1a83c204987a47ccad1be4e0/cras/src/server/dev_io.c
[modify] https://crrev.com/fd8747c7e7ee2bef1a83c204987a47ccad1be4e0/cras/src/server/dev_stream.c
[modify] https://crrev.com/fd8747c7e7ee2bef1a83c204987a47ccad1be4e0/cras/src/server/audio_thread.c
[modify] https://crrev.com/fd8747c7e7ee2bef1a83c204987a47ccad1be4e0/cras/src/tests/audio_thread_unittest.cc
[modify] https://crrev.com/fd8747c7e7ee2bef1a83c204987a47ccad1be4e0/cras/src/server/dev_stream.h

Request merge to M71. It is to fix the bug in CRAS. Thanks.

This bug requires manual review: M71 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: benmason@(Android), kariahda@(iOS), kbleicher@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

This change impacts a number of areas.  Can you add context regarding testing on ToT/M72 or otherwise prior to a merge?  I'd like to understand the merge risk.  Thanks.

We don't find regression in our test. (This change landed on R72-11165.0.0)
https://stainless.corp.google.com/search?view=matrix&row=test&col=build&first_date=2018-10-02&last_date=2018-10-29&suite=^chameleon\_audio\_perbuild%24&exclude_cts=true&exclude_not_run=false&exclude_non_release=true&exclude_au=true&exclude_acts=true&exclude_retried=true&exclude_non_production=false

We can test it by running any audio after suspending on the grunt. It should no longer trigger underruns (You can hear some noise when underrun happens). Basically, it only affects grunt because it has small buffer size on audio device. It will not affect other boards. Thanks.

Thanks for the detail.  This is still a fairly large change and only a P2.  I also don't see that this was a M71 regression (not mentioned in this bug).  I'd rather wait for M72... Compelling reason not to?

Here is the relative issue. https://b.corp.google.com/issues/117302276 . This change can fix the noise problem on grunt. I think we should merge it into M71 because the bug affects experience of users.

Is the CL limited to grunt then?  Appears to be per the associated bug (thanks).  Possible to test / verify prior to the merge?

In M71, we can hear obvious noise after suspending on grunt. After I cherry pick this CL, the problem is fixed. The change is not limited to grunt. It fixes the bug which only happens on device with small buffer, such as grunt. I also test it on other device(eve, peppy) and they all work fine.

Approving merge to M71 Chrome OS.
Thanks for the details.

R-71 CL: https://chromium-review.googlesource.com/c/1310233

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

commit bf585c80f4f6b2d07f987381a66980087a6d9fe0
Author: Yu-Hsuan Hsu <yuhsuan@chromium.org>
Date: Thu Nov 01 02:49:07 2018

CRAS: Do not ignore next_cb_ts of stream when share memory is full

If device buffer size is not large enough, we might leave some frames in
share memory. If we ignore next callback time of stream and use hardware
level in device to compute next wake up time, it might cause we wake up
too late and trigger underruns.

The solution to continue using the stream wake up time when shared memory
is full. Andi then we can skip one fetch if there are enough frames in
the share memory.

BUG= chromium:894694 
TEST=The underruns do not happen even if there is unaccurate rate
on device. (grunt)

Change-Id: I48294466ef8f79617ffaf390a58f18f4841345b0
Reviewed-on: https://chromium-review.googlesource.com/1278611
Commit-Ready: Yu-Hsuan Hsu <yuhsuan@chromium.org>
Tested-by: Yu-Hsuan Hsu <yuhsuan@chromium.org>
Reviewed-by: Cheng-Yi Chiang <cychiang@chromium.org>
(cherry picked from commit fd8747c7e7ee2bef1a83c204987a47ccad1be4e0)
Reviewed-on: https://chromium-review.googlesource.com/c/1310233
Reviewed-by: Yu-Hsuan Hsu <yuhsuan@chromium.org>
Commit-Queue: Daniel Kurtz <djkurtz@chromium.org>
Tested-by: Daniel Kurtz <djkurtz@chromium.org>

[modify] https://crrev.com/bf585c80f4f6b2d07f987381a66980087a6d9fe0/cras/src/tests/dev_stream_unittest.cc
[modify] https://crrev.com/bf585c80f4f6b2d07f987381a66980087a6d9fe0/cras/src/common/cras_types.h
[modify] https://crrev.com/bf585c80f4f6b2d07f987381a66980087a6d9fe0/cras/src/tests/cras_test_client.c
[modify] https://crrev.com/bf585c80f4f6b2d07f987381a66980087a6d9fe0/cras/src/server/dev_io.c
[modify] https://crrev.com/bf585c80f4f6b2d07f987381a66980087a6d9fe0/cras/src/server/dev_stream.c
[modify] https://crrev.com/bf585c80f4f6b2d07f987381a66980087a6d9fe0/cras/src/server/audio_thread.c
[modify] https://crrev.com/bf585c80f4f6b2d07f987381a66980087a6d9fe0/cras/src/tests/audio_thread_unittest.cc
[modify] https://crrev.com/bf585c80f4f6b2d07f987381a66980087a6d9fe0/cras/src/server/dev_stream.h

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