Fix audio glitching on Vista due to unreliable OS callbacks.

Fix audio glitching on Vista due to unreliable OS callbacks.

This reverts part of the fix I made for Windows 10 audio, since it
inadvertently broke Vista audio.  Vista will frequently underflow
and ask for more than one buffer at a time. :(

While back-to-back reads _may_ lead to glitches with single buffer
clients like WebRTC and WebAudio, not allowing them _definitely_
leads to glitches for all clients (even <audio> which double buffers on
the browser side).

Further, API documentation does not guarantee we won't be asked for
more than one buffer regardless of the period size. In practice, outside
of Vista this does not seem to happen though.

TEST=vista audio glitch free, windows 10 audio glitch free.
BUG=524947
Committed: https://crrev.com/cae8b7ed33dcace93561a1e0bb8126cc020001cd
Cr-Commit-Position: refs/heads/master@{#347135}

[DaleCurtis, 2015-09-02 18:26:33]

dalecurtis@chromium.org changed reviewers:
+ henrika@chromium.org, tommi@chromium.org

[DaleCurtis, 2015-09-02 18:29:47]

Patchset #1 (id:1) has been deleted

[DaleCurtis, 2015-09-02 18:30:23]

Patchset #1 (id:20001) has been deleted

[DaleCurtis, 2015-09-02 18:31:56]

What do you think? Since the API doesn't preclude back to back callbacks, I
decided to re-enable them for all platforms. It's okay for media since we double
buffer, but WebAudio and WebRTC might hit glitches more often this way.

Please CQ if it lgty and I'll handle back merges after a canary soak.

[henrika (OOO until Aug 14), 2015-09-03 08:19:49]

Thanks Dale. Looks good but it would be nice with a more clear view of what now
differs between the current 
implementation and the one before the Win 10 change. I guess I can check that
out on my own once landed.

LGTM

[tommi (sloooow) - chröme, 2015-09-03 08:31:28]

The CQ bit was checked by tommi@chromium.org

[tommi (sloooow) - chröme, 2015-09-03 08:31:28]

lgtm

https://codereview.chromium.org/1318933003/diff/40001/media/audio/win/audio_l...
File media/audio/win/audio_low_latency_output_win.cc (right):

https://codereview.chromium.org/1318933003/diff/40001/media/audio/win/audio_l...
media/audio/win/audio_low_latency_output_win.cc:487: // that the render can
fulfill the read before we glitch anyways.
Can we record some sort of stats to see how often this happens out in the field?

[commit-bot: I haz the power, 2015-09-03 08:31:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1318933003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1318933003/40001

[commit-bot: I haz the power, 2015-09-03 11:15:34]

Committed patchset #1 (id:40001)

[commit-bot: I haz the power, 2015-09-03 11:16:22]

Patchset 1 (id:??) landed as
https://crrev.com/cae8b7ed33dcace93561a1e0bb8126cc020001cd
Cr-Commit-Position: refs/heads/master@{#347135}

[DaleCurtis, 2015-09-03 16:36:31]

Thanks for review, the only difference the Win10 patch made now is to not block
devices based on endpoint buffer size not being a multiple of the buffer size.

When working on the original patch, I was able to induce glitches under load on
Windows 10 from back to back callbacks w/ single-buffer clients, so I included
that "fix" w/ the previous patch, unfortunately for our Vista users :(

https://codereview.chromium.org/1318933003/diff/40001/media/audio/win/audio_l...
File media/audio/win/audio_low_latency_output_win.cc (right):

https://codereview.chromium.org/1318933003/diff/40001/media/audio/win/audio_l...
media/audio/win/audio_low_latency_output_win.cc:487: // that the render can
fulfill the read before we glitch anyways.
On 2015/09/03 08:31:28, tommi wrote:
> Can we record some sort of stats to see how often this happens out in the
field?

It's tracked by Media.AudioRendererMissedDeadline which today says that over all
flavors of Windows, ~2% of audio sessions miss >= 1% of all callbacks.

https://uma.googleplex.com/histograms/?dayCount=1&histograms=Media.AudioRende...

If we filter to just Vista, we see that number is closer to ~4.4%:

https://uma.googleplex.com/histograms/?dayCount=1&endDate=09-02-2015&version=...

[wdzierzanowski, 2015-11-25 11:57:42]

Hey Dale,

This CL breaks WASAPIAudioOutputStreamTest.ValidPacketSize in some cases
(Windows 8 in a VM) like so:

[ RUN      ] WASAPIAudioOutputStreamTest.ValidPacketSize
???\chromium\src\media\audio\win\audio_low_latency_output_win_unittest.cc(384):
error: Mock function called more times than expected - returning default value.
    Function call: OnMoreData(00C8ECF8, 3528)
          Returns: 0
         Expected: to be called once
           Actual: called twice - over-saturated and active
???\chromium\src\media\audio\win\audio_low_latency_output_win_unittest.cc(384):
error: Mock function called more times than expected - returning default value.
    Function call: OnMoreData(00C8ECF8, 5292)
          Returns: 0
         Expected: to be called once
           Actual: called 3 times - over-saturated and active
[  FAILED  ] WASAPIAudioOutputStreamTest.ValidPacketSize (219 ms)

Is this something that you'd worry about?

[tommi (sloooow) - chröme, 2015-11-25 12:59:20]

If OnMoreData is called more than expected, then my first instinct would be
to fix the test to add a WillRepeatedly() handler to ignore the >1 calls :)
That is, without looking at the test. ;)

On Wed, Nov 25, 2015 at 12:57 PM <wdzierzanowski@opera.com> wrote:

> Hey Dale,
>
> This CL breaks WASAPIAudioOutputStreamTest.ValidPacketSize in some cases
> (Windows 8 in a VM) like so:
>
> [ RUN      ] WASAPIAudioOutputStreamTest.ValidPacketSize
>
>
???\chromium\src\media\audio\win\audio_low_latency_output_win_unittest.cc(384):
> error: Mock function called more times than expected - returning default
> value.
>      Function call: OnMoreData(00C8ECF8, 3528)
>            Returns: 0
>           Expected: to be called once
>             Actual: called twice - over-saturated and active
>
>
???\chromium\src\media\audio\win\audio_low_latency_output_win_unittest.cc(384):
> error: Mock function called more times than expected - returning default
> value.
>      Function call: OnMoreData(00C8ECF8, 5292)
>            Returns: 0
>           Expected: to be called once
>             Actual: called 3 times - over-saturated and active
> [  FAILED  ] WASAPIAudioOutputStreamTest.ValidPacketSize (219 ms)
>
> Is this something that you'd worry about?
>
> https://codereview.chromium.org/1318933003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[DaleCurtis, 2015-11-25 17:08:31]

Yeah, it seems like your test case is showing that the glitch described in the
commit message can happen on Windows 8 too, which is unfortunate -- but kind of
expected since the docs don't say it can't.

tommi@ has the gist of it, we should change to WillRepeatedly().

[wdzierzanowski, 2015-11-30 08:43:07]

On 2015/11/25 17:08:31, DaleCurtis wrote:
> Yeah, it seems like your test case is showing that the glitch described in the
> commit message can happen on Windows 8 too, which is unfortunate -- but kind
of
> expected since the docs don't say it can't.
> 
> tommi@ has the gist of it, we should change to WillRepeatedly().

OK, I'll work on a CL to do that.

[wdzierzanowski, 2015-12-01 08:35:06]

Filed https://codereview.chromium.org/1487733003/.