Increase Windows XP hardware buffer size to 4096.

Increase Windows XP hardware buffer size to 4096.

In local testing with XP we noticed WebAudio and HTML5 suffer from
clicking with the "low latency" 2048 buffer size.

This CL also adds a --audio-buffer-size parameter which we can use
to have users tweak in the field and report results back.

BUG=161307
TEST=XP!
TBR=sky

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=168111

[DaleCurtis, 2012-10-27 01:16:42]

scherkus and I had time to do some testing on his XP laptop with the latest
canary and our various audio clients and found that the 2k buffer size we use on
XP for low latency clients led to glitching.

Switching this to 4k resolved the problem and doesn't seem to add any noticeable
overhead in the demos we've tested (html5, flash youtube, iwebdj, and webaudio).
 The WebRTC demo sadly wouldn't connect. Henrika, do you have another good demo
we can check out?

We're able to use 4k on XP w/ 2k coming from the renderer due to the blocking
shared memory on Windows.

What does everyone think?

[henrika (OOO until Aug 14), 2012-10-27 08:46:55]

Perhaps I am blind but I can't see that you are increasing the buffer size in
this CL.

If that is what it takes, then I am fine with it. It is possible to reduce the
delay on XP but it requires a totally new implementation and I don't think we
have resources for that now.

Other demos: try pc1.html at https://webrtc-demos.appspot.com/

[scherkus (not reviewing), 2012-10-29 18:23:02]

lgtm w/ nit

background: dale and I ran a variety of audio tests on my trusty 5 year old XP
laptop and found that we still had the odd click when running at 2k

https://codereview.chromium.org/11309015/diff/1/media/audio/audio_util.cc
File media/audio/audio_util.cc (right):

https://codereview.chromium.org/11309015/diff/1/media/audio/audio_util.cc#new...
media/audio/audio_util.cc:50: if (!buffer_size_str.empty() &&
nit: base::StringToInt() will return 0 on empty string

up to you if you want to roll everything into a single statement

https://codereview.chromium.org/11309015/diff/1/media/audio/audio_util.cc#new...
media/audio/audio_util.cc:259: static const int kFallbackBufferSize = 4096;
henrika: this is the buffer size change

[henrika (OOO until Aug 14), 2012-10-29 18:50:18]

Cool stuff ;-)

LGTM.

May I ask how you "work with XP". I mean how did you transfer the Chrome binary
to the XP machine. Build official and install?

[henrika (OOO until Aug 14), 2012-10-29 18:50:33]

https://codereview.chromium.org/11309015/diff/1/media/audio/audio_util.cc
File media/audio/audio_util.cc (right):

https://codereview.chromium.org/11309015/diff/1/media/audio/audio_util.cc#new...
media/audio/audio_util.cc:259: static const int kFallbackBufferSize = 4096;
Got it. Thanks.

[scherkus (not reviewing), 2012-10-29 18:51:38]

On 2012/10/29 18:50:18, henrika wrote:
> Cool stuff ;-)
> 
> LGTM.
> 
> May I ask how you "work with XP". I mean how did you transfer the Chrome
binary
> to the XP machine. Build official and install?

Release builds and a USB key.

[DaleCurtis, 2012-10-30 21:52:52]

Done.

Chris: Since we don't know the hardware sample rate at this point, we can't
adjust the buffer size based on it.

Chris, Viet-Trung: Any other concerns or comments?

http://codereview.chromium.org/11309015/diff/1/media/audio/audio_util.cc
File media/audio/audio_util.cc (right):

http://codereview.chromium.org/11309015/diff/1/media/audio/audio_util.cc#newc...
media/audio/audio_util.cc:50: if (!buffer_size_str.empty() &&
On 2012/10/29 18:23:02, scherkus wrote:
> nit: base::StringToInt() will return 0 on empty string
> 
> up to you if you want to roll everything into a single statement
> 

Done.

[Chris Rogers, 2012-10-30 22:03:50]

Isn't there a pretty good chance (>90%) that the sample-rate is 48KHz.  In that
case, can't we pick a multiple of 10ms based on that?  It's at least worth
testing the callback jitter by looking at the tracing to see if this helps or
not.  Tracing gives us a good idea of how regular or irregular OnMoreData() is
getting called, so we should at least check this in addition to simply listening
for glitches.

[Chris Rogers, 2012-10-30 22:10:10]

Sorry, I forgot that we're relying on wavOut to do the SRC for us, so I suppose
the hardware sample-rate might be less important here.  But still I recommend
looking at the tracing to get a feel if buffer-size choice might influence the
regularity...

[DaleCurtis, 2012-11-05 21:29:01]

On 2012/10/30 22:10:10, Chris Rogers wrote:
> Sorry, I forgot that we're relying on wavOut to do the SRC for us, so I
suppose
> the hardware sample-rate might be less important here.  But still I recommend
> looking at the tracing to get a feel if buffer-size choice might influence the
> regularity...

The trace shows some irregularity for Flash @ 4k sizes:

http://i.imgur.com/trVkq.png

Things sound and play correctly as far as I can tell though.  However even with
4k if the system is under load, I was able to detect some clicks and pops on all
clients (flash included).

[DaleCurtis, 2012-11-15 21:52:50]

Guys, I've uploaded a new patch set which adds an additional fix for a crash I
encountered during load testing.

While at it I was able to reproduce the issue our M23 XP users are seeing with
the 2k buffer size, so I believe we'll need to merge this patch into M23.

Viet-Trung: There is a theoretical worry that flash might not like being called
back so frequently.  In practice everything seems to work fine.  However if
there is a way we could up the Flash buffer size on XP to 4096 that would
alleviate these concerns.  Is there an easy way to do that or do we need a
Flapper change?

[Chris Rogers, 2012-11-15 22:49:29]

Seems ok, nice to have the command-line flag for debugging...

https://codereview.chromium.org/11309015/diff/17001/media/audio/win/waveout_o...
File media/audio/win/waveout_output_win.cc (right):

https://codereview.chromium.org/11309015/diff/17001/media/audio/win/waveout_o...
media/audio/win/waveout_output_win.cc:289: base::AutoLock auto_lock(lock_);
maybe this auto_lock should be moved up to just above line 281?

[DaleCurtis, 2012-11-15 22:58:22]

https://codereview.chromium.org/11309015/diff/17001/media/audio/win/waveout_o...
File media/audio/win/waveout_output_win.cc (right):

https://codereview.chromium.org/11309015/diff/17001/media/audio/win/waveout_o...
media/audio/win/waveout_output_win.cc:289: base::AutoLock auto_lock(lock_);
On 2012/11/15 22:49:30, Chris Rogers wrote:
> maybe this auto_lock should be moved up to just above line 281?

Done.

[scherkus (not reviewing), 2012-11-16 00:04:09]

still lgtm media code-wise -- I'll leave it up to you to coordinate w/ vtl et al

[commit-bot: I haz the power, 2012-11-16 00:58:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/11309015/24001

[commit-bot: I haz the power, 2012-11-16 00:59:03]

Presubmit check for 11309015-24001 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    content/browser/renderer_host

Presubmit checks took 1.4s to calculate.

[DaleCurtis, 2012-11-16 01:00:59]

tbr=sky for adding the flag to content/browser/renderer_host

[commit-bot: I haz the power, 2012-11-16 01:01:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/11309015/24001

[commit-bot: I haz the power, 2012-11-16 01:16:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/11309015/27002

[viettrungluu, 2012-11-16 20:27:47]

https://codereview.chromium.org/11309015/diff/27002/ppapi/shared_impl/ppb_aud...
File ppapi/shared_impl/ppb_audio_config_shared.cc (right):

https://codereview.chromium.org/11309015/diff/27002/ppapi/shared_impl/ppb_aud...
ppapi/shared_impl/ppb_audio_config_shared.cc:57: return 2048;
Did you really land this with an uncondition |return| inserted into the middle
of a function?!?

[scherkus (not reviewing), 2012-11-16 20:35:52]

https://codereview.chromium.org/11309015/diff/27002/ppapi/shared_impl/ppb_aud...
File ppapi/shared_impl/ppb_audio_config_shared.cc (right):

https://codereview.chromium.org/11309015/diff/27002/ppapi/shared_impl/ppb_aud...
ppapi/shared_impl/ppb_audio_config_shared.cc:57: return 2048;
On 2012/11/16 20:27:47, viettrungluu wrote:
> Did you really land this with an uncondition |return| inserted into the middle
> of a function?!?

Woah -- looks like this snuck in after lgtms.

Dale was this leftover test code / bad rebase?

[DaleCurtis, 2012-11-16 20:41:43]

https://codereview.chromium.org/11309015/diff/27002/ppapi/shared_impl/ppb_aud...
File ppapi/shared_impl/ppb_audio_config_shared.cc (right):

https://codereview.chromium.org/11309015/diff/27002/ppapi/shared_impl/ppb_aud...
ppapi/shared_impl/ppb_audio_config_shared.cc:57: return 2048;
On 2012/11/16 20:35:53, scherkus wrote:
> On 2012/11/16 20:27:47, viettrungluu wrote:
> > Did you really land this with an uncondition |return| inserted into the
middle
> > of a function?!?
> 
> Woah -- looks like this snuck in after lgtms.
> 
> Dale was this leftover test code / bad rebase?

Bad rebase after test code. Reverting this part. Sorry.