Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(12)

Issue 11309015: Increase Windows XP hardware buffer size to 4096. (Closed)

Created:
6 years, 3 months ago by DaleCurtis
Modified:
6 years, 3 months ago
CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, feature-media-reviews_chromium.org
Base URL:
http://git.chromium.org/chromium/src.git@master
Visibility:
Public.

Description

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

Patch Set 1 #

Total comments: 4

Patch Set 2 : Comments. #

Patch Set 3 : Rebase. #

Total comments: 2

Patch Set 4 : Comments. #

Patch Set 5 : Fix includes. #

Total comments: 3
Unified diffs Side-by-side diffs Delta from patch set Stats (+37 lines, -3 lines) Patch
M content/browser/renderer_host/render_process_host_impl.cc View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M media/audio/audio_util.cc View 1 2 3 4 4 chunks +26 lines, -3 lines 0 comments Download
M media/audio/win/waveout_output_win.cc View 1 2 3 1 chunk +3 lines, -0 lines 0 comments Download
M media/base/media_switches.h View 1 chunk +2 lines, -0 lines 0 comments Download
M media/base/media_switches.cc View 1 chunk +3 lines, -0 lines 0 comments Download
M ppapi/shared_impl/ppb_audio_config_shared.cc View 1 2 3 4 1 chunk +2 lines, -0 lines 3 comments Download

Messages

Total messages: 22 (0 generated)
DaleCurtis
scherkus and I had time to do some testing on his XP laptop with the ...
6 years, 3 months ago (2012-10-27 01:16:42 UTC) #1
henrika (OOO until Aug 14)
Perhaps I am blind but I can't see that you are increasing the buffer size ...
6 years, 3 months ago (2012-10-27 08:46:55 UTC) #2
scherkus (not reviewing)
lgtm w/ nit background: dale and I ran a variety of audio tests on my ...
6 years, 3 months ago (2012-10-29 18:23:02 UTC) #3
henrika (OOO until Aug 14)
Cool stuff ;-) LGTM. May I ask how you "work with XP". I mean how ...
6 years, 3 months ago (2012-10-29 18:50:18 UTC) #4
henrika (OOO until Aug 14)
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#newcode259 media/audio/audio_util.cc:259: static const int kFallbackBufferSize = 4096; Got it. Thanks.
6 years, 3 months ago (2012-10-29 18:50:33 UTC) #5
scherkus (not reviewing)
On 2012/10/29 18:50:18, henrika wrote: > Cool stuff ;-) > > LGTM. > > May ...
6 years, 3 months ago (2012-10-29 18:51:38 UTC) #6
DaleCurtis
Done. Chris: Since we don't know the hardware sample rate at this point, we can't ...
6 years, 3 months ago (2012-10-30 21:52:52 UTC) #7
Chris Rogers
Isn't there a pretty good chance (>90%) that the sample-rate is 48KHz. In that case, ...
6 years, 3 months ago (2012-10-30 22:03:50 UTC) #8
Chris Rogers
Sorry, I forgot that we're relying on wavOut to do the SRC for us, so ...
6 years, 3 months ago (2012-10-30 22:10:10 UTC) #9
DaleCurtis
On 2012/10/30 22:10:10, Chris Rogers wrote: > Sorry, I forgot that we're relying on wavOut ...
6 years, 3 months ago (2012-11-05 21:29:01 UTC) #10
DaleCurtis
Guys, I've uploaded a new patch set which adds an additional fix for a crash ...
6 years, 3 months ago (2012-11-15 21:52:50 UTC) #11
Chris Rogers
Seems ok, nice to have the command-line flag for debugging... https://codereview.chromium.org/11309015/diff/17001/media/audio/win/waveout_output_win.cc File media/audio/win/waveout_output_win.cc (right): https://codereview.chromium.org/11309015/diff/17001/media/audio/win/waveout_output_win.cc#newcode289 ...
6 years, 3 months ago (2012-11-15 22:49:29 UTC) #12
DaleCurtis
https://codereview.chromium.org/11309015/diff/17001/media/audio/win/waveout_output_win.cc File media/audio/win/waveout_output_win.cc (right): https://codereview.chromium.org/11309015/diff/17001/media/audio/win/waveout_output_win.cc#newcode289 media/audio/win/waveout_output_win.cc:289: base::AutoLock auto_lock(lock_); On 2012/11/15 22:49:30, Chris Rogers wrote: > ...
6 years, 3 months ago (2012-11-15 22:58:22 UTC) #13
scherkus (not reviewing)
still lgtm media code-wise -- I'll leave it up to you to coordinate w/ vtl ...
6 years, 3 months ago (2012-11-16 00:04:09 UTC) #14
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/11309015/24001
6 years, 3 months ago (2012-11-16 00:58:56 UTC) #15
commit-bot: I haz the power
Presubmit check for 11309015-24001 failed and returned exit status 1. Running presubmit commit checks ...
6 years, 3 months ago (2012-11-16 00:59:03 UTC) #16
DaleCurtis
tbr=sky for adding the flag to content/browser/renderer_host
6 years, 3 months ago (2012-11-16 01:00:59 UTC) #17
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/11309015/24001
6 years, 3 months ago (2012-11-16 01:01:42 UTC) #18
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/11309015/27002
6 years, 3 months ago (2012-11-16 01:16:09 UTC) #19
viettrungluu
https://codereview.chromium.org/11309015/diff/27002/ppapi/shared_impl/ppb_audio_config_shared.cc File ppapi/shared_impl/ppb_audio_config_shared.cc (right): https://codereview.chromium.org/11309015/diff/27002/ppapi/shared_impl/ppb_audio_config_shared.cc#newcode57 ppapi/shared_impl/ppb_audio_config_shared.cc:57: return 2048; Did you really land this with an ...
6 years, 3 months ago (2012-11-16 20:27:47 UTC) #20
scherkus (not reviewing)
https://codereview.chromium.org/11309015/diff/27002/ppapi/shared_impl/ppb_audio_config_shared.cc File ppapi/shared_impl/ppb_audio_config_shared.cc (right): https://codereview.chromium.org/11309015/diff/27002/ppapi/shared_impl/ppb_audio_config_shared.cc#newcode57 ppapi/shared_impl/ppb_audio_config_shared.cc:57: return 2048; On 2012/11/16 20:27:47, viettrungluu wrote: > Did ...
6 years, 3 months ago (2012-11-16 20:35:52 UTC) #21
DaleCurtis
6 years, 3 months ago (2012-11-16 20:41:43 UTC) #22
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.

Powered by Google App Engine
This is Rietveld 408576698