Use 2 buffers on presumable good Windows boxes.

Change the code to use 2 buffers on presumable good Windows boxes.
I.e. running non-Vista and having more than single core.

Changed unit tests as well.

(Earlier that CL was part of bugger one, changing the way audio mixer is working,
but it causes problems on Mac).

BUG=132009
TEST=Startup of 2nd stream should become somewhat faster.
TEST=Run tests on Win7 and XP myself.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=142488

[enal1, 2012-06-06 21:26:17]

Andrew, can you please take a look?

[scherkus (not reviewing), 2012-06-07 17:25:41]

+vrk to also look at the mixer code to have another pair of eyes

https://chromiumcodereview.appspot.com/10540034/diff/1/media/audio/audio_util.cc
File media/audio/audio_util.cc (right):

https://chromiumcodereview.appspot.com/10540034/diff/1/media/audio/audio_util...
media/audio/audio_util.cc:525: // 3 otherwise.
"use 3 buffers [...], 3 otherwise" ?

do you mean 2?

I'd also like to see a small explanation / URL link that explains why vista gets
stuck w/ 3 buffers and not XP nor Win7

https://chromiumcodereview.appspot.com/10540034/diff/1/media/audio/audio_util...
media/audio/audio_util.cc:531: return num_buffers;
nit: how about dropping the num_buffers stuff and writing this as:

if (cpus < 2 || win == VISTA) {
  return 3;
}
return 2;

https://chromiumcodereview.appspot.com/10540034/diff/1/media/audio/audio_util.h
File media/audio/audio_util.h (right):

https://chromiumcodereview.appspot.com/10540034/diff/1/media/audio/audio_util...
media/audio/audio_util.h:136: // Used by both production and test code.
no need to mention the prod/test stuff

https://chromiumcodereview.appspot.com/10540034/diff/1/media/audio/win/audio_...
File media/audio/win/audio_output_win_unittest.cc (right):

https://chromiumcodereview.appspot.com/10540034/diff/1/media/audio/win/audio_...
media/audio/win/audio_output_win_unittest.cc:604: // |bytes_100_ms| because the
audio output stream has a triple buffer scheme.
nit: update comment

[enal1, 2012-06-07 20:56:22]

Addressed everything.

https://chromiumcodereview.appspot.com/10540034/diff/1/media/audio/audio_util.cc
File media/audio/audio_util.cc (right):

https://chromiumcodereview.appspot.com/10540034/diff/1/media/audio/audio_util...
media/audio/audio_util.cc:525: // 3 otherwise.
On 2012/06/07 17:25:41, scherkus wrote:
> "use 3 buffers [...], 3 otherwise" ?
> 
> do you mean 2?
> 
> I'd also like to see a small explanation / URL link that explains why vista
gets
> stuck w/ 3 buffers and not XP nor Win7

Done.

https://chromiumcodereview.appspot.com/10540034/diff/1/media/audio/audio_util...
media/audio/audio_util.cc:531: return num_buffers;
On 2012/06/07 17:25:41, scherkus wrote:
> nit: how about dropping the num_buffers stuff and writing this as:
> 
> if (cpus < 2 || win == VISTA) {
>   return 3;
> }
> return 2;

Done.

https://chromiumcodereview.appspot.com/10540034/diff/1/media/audio/audio_util.h
File media/audio/audio_util.h (right):

https://chromiumcodereview.appspot.com/10540034/diff/1/media/audio/audio_util...
media/audio/audio_util.h:136: // Used by both production and test code.
On 2012/06/07 17:25:41, scherkus wrote:
> no need to mention the prod/test stuff

Done.

https://chromiumcodereview.appspot.com/10540034/diff/1/media/audio/win/audio_...
File media/audio/win/audio_output_win_unittest.cc (right):

https://chromiumcodereview.appspot.com/10540034/diff/1/media/audio/win/audio_...
media/audio/win/audio_output_win_unittest.cc:604: // |bytes_100_ms| because the
audio output stream has a triple buffer scheme.
On 2012/06/07 17:25:41, scherkus wrote:
> nit: update comment

Done.

[scherkus (not reviewing), 2012-06-07 20:59:29]

lgtm but I'm not terribly familiar with the audio mixer

vrk have any input?

[enal1, 2012-06-07 21:58:25]

Changes in mixer itself are minimal -- I just moved Start() for physical
stream into Open(), and Stop() into Close(). It became simpler, if anything.

On Thu, Jun 7, 2012 at 1:59 PM, <scherkus@chromium.org> wrote:

> lgtm but I'm not terribly familiar with the audio mixer
>
> vrk have any input?
>
>
https://chromiumcodereview.**appspot.com/10540034/<https://chromiumcodereview...
>

[vrk (LEFT CHROMIUM), 2012-06-11 17:17:09]

The code LGTM, glad that it simplified the locking scenarios in the mixer!

Question about "That increased audio startup latency, especially on Windows,
because we are using 3 buffers on Windows." Why did that happen? I think you
mentioned this in standup before, but I don't remember. Is it because it takes
longer to stop the stream now, and a new stream has to wait for the last one to
stop? (so the startup latency for the very first stream had not increased, but
the startup latency for subsequent streams had increased?)

[enal1, 2012-06-11 17:39:09]

Earlier, when starting new stream, we buffered 3 buffers (typical latency
per buffer 2-3ms), and then started physical stream (latency 15-20ms).
Total latency ~25ms.

(All times for my Win7 machines, on other systems exact timing may differ.)

Now we have physical stream that is already playing. It already have 3
buffers "in flight", so earliest we can start playing is after those
buffers finish. Exact timing depends on buffer size.

We are seeing the same problems WebAudio and WebRTC are hitting -- they
need smaller buffer for better startup/stop latency, and larger buffer to
avoid glitches. When we had no mixer, our startup latency was limited by
how fast OS starts new stream, and we wanted larger buffers to avoid
playback glitches, too. There was no direct relationship between size of
buffer (number of buffers) and startup/stop latency. Now there is.

Thanks,
Eugene

On Mon, Jun 11, 2012 at 10:17 AM, <vrk@chromium.org> wrote:

> The code LGTM, glad that it simplified the locking scenarios in the mixer!
>
> Question about "That increased audio startup latency, especially on
> Windows,
> because we are using 3 buffers on Windows." Why did that happen? I think
> you
> mentioned this in standup before, but I don't remember. Is it because it
> takes
> longer to stop the stream now, and a new stream has to wait for the last
> one to
> stop? (so the startup latency for the very first stream had not increased,
> but
> the startup latency for subsequent streams had increased?)
>
>
http://codereview.chromium.**org/10540034/<http://codereview.chromium.org/105...
>

[commit-bot: I haz the power, 2012-06-11 18:55:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/enal@chromium.org/10540034/7003

[commit-bot: I haz the power, 2012-06-11 20:12:23]

Change committed as 141476

[sail, 2012-06-11 21:19:37]

I think this broke the Mac unit tests:
http://build.chromium.org/p/chromium/buildstatus?builder=Mac10.5%20Tests%20%2...

[enal1, 2012-06-11 21:29:51]

Thank you, you reverted before I did.

On Mon, Jun 11, 2012 at 2:19 PM, <sail@chromium.org> wrote:

> I think this broke the Mac unit tests:
> http://build.chromium.org/p/**chromium/buildstatus?builder=**
>
Mac10.5%20Tests%20%281%29&**number=20624<http://build.chromium.org/p/chromium/buildstatus?builder=Mac10.5%20Tests%20%281%29&number=20624>
>
>
>
https://chromiumcodereview.**appspot.com/10540034/<https://chromiumcodereview...
>

[commit-bot: I haz the power, 2012-06-12 20:09:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/enal@chromium.org/10540034/13004

[commit-bot: I haz the power, 2012-06-12 20:19:09]

Try job failure for 10540034-13004 (retry) on mac_rel for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-06-12 20:23:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/enal@chromium.org/10540034/10003

[commit-bot: I haz the power, 2012-06-12 20:36:24]

Try job failure for 10540034-10003 (retry) on mac_rel for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-06-12 21:04:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/enal@chromium.org/10540034/7015

[commit-bot: I haz the power, 2012-06-12 22:55:59]

Change committed as 141770

[commit-bot: I haz the power, 2012-06-15 16:51:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/enal@chromium.org/10540034/26002

[commit-bot: I haz the power, 2012-06-15 18:30:29]

Change committed as 142430

[commit-bot: I haz the power, 2012-06-15 20:36:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/enal@chromium.org/10540034/18004

[commit-bot: I haz the power, 2012-06-15 21:47:32]

Change committed as 142488