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

Issue 11348166: Always wait for DataReady() on Windows WaveOut. (Closed)

Created:
6 years, 5 months ago by DaleCurtis
Modified:
6 years, 4 months ago
CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, feature-media-reviews_chromium.org
Visibility:
Public.

Description

Always wait for DataReady() on Windows WaveOut. Only WaveOut still needs this hack and since all renderer clients now use the shared memory marker we can always wait for it. I suspect on low end machines this timeout is leading to pops and clicks for some users on XP. BUG=161307 TEST=audio still works. Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=169553

Patch Set 1 : Typo. #

Total comments: 2

Patch Set 2 : Only wait on WaveOut. #

Total comments: 10

Patch Set 3 : The comment is law. Timeout. #

Total comments: 4

Patch Set 4 : kMaxWait. #

Patch Set 5 : AudioOutputResampler change. #

Total comments: 2

Patch Set 6 : Comment. #

Total comments: 1

Patch Set 7 : Fix typo! #

Unified diffs Side-by-side diffs Delta from patch set Stats (+52 lines, -47 lines) Patch
M content/browser/renderer_host/media/audio_sync_reader.h View 1 chunk +0 lines, -1 line 0 comments Download
M content/browser/renderer_host/media/audio_sync_reader.cc View 1 2 chunks +0 lines, -21 lines 0 comments Download
M media/audio/audio_io.h View 1 2 1 chunk +4 lines, -8 lines 0 comments Download
M media/audio/audio_output_controller.h View 1 1 chunk +1 line, -5 lines 0 comments Download
M media/audio/audio_output_controller.cc View 1 2 3 4 5 6 2 chunks +15 lines, -10 lines 0 comments Download
M media/audio/audio_output_resampler.cc View 1 2 3 4 5 5 chunks +25 lines, -1 line 0 comments Download
M media/audio/win/waveout_output_win.cc View 1 2 2 chunks +7 lines, -1 line 0 comments Download

Messages

Total messages: 24 (0 generated)
DaleCurtis
If we could ensure Pepper clients always use GetAudioHardwareBufferSize() we could might be able to ...
6 years, 5 months ago (2012-11-20 20:43:30 UTC) #1
scherkus (not reviewing)
q https://codereview.chromium.org/11348166/diff/2001/content/browser/renderer_host/media/audio_sync_reader.cc File content/browser/renderer_host/media/audio_sync_reader.cc (right): https://codereview.chromium.org/11348166/diff/2001/content/browser/renderer_host/media/audio_sync_reader.cc#newcode66 content/browser/renderer_host/media/audio_sync_reader.cc:66: // http://crbug.com/161307 and http://crbug.com/61022 does it make sense ...
6 years, 5 months ago (2012-11-20 21:08:46 UTC) #2
DaleCurtis
https://codereview.chromium.org/11348166/diff/2001/content/browser/renderer_host/media/audio_sync_reader.cc File content/browser/renderer_host/media/audio_sync_reader.cc (right): https://codereview.chromium.org/11348166/diff/2001/content/browser/renderer_host/media/audio_sync_reader.cc#newcode66 content/browser/renderer_host/media/audio_sync_reader.cc:66: // http://crbug.com/161307 and http://crbug.com/61022 On 2012/11/20 21:08:46, scherkus wrote: ...
6 years, 5 months ago (2012-11-20 21:33:47 UTC) #3
scherkus (not reviewing)
lgtm but my knowledge of this area is rusty -- perhaps ping crogers@ to take ...
6 years, 5 months ago (2012-11-20 21:50:23 UTC) #4
DaleCurtis
Per discussion with Chris, I'll land this now and get some feedback from canary users. ...
6 years, 5 months ago (2012-11-20 22:59:34 UTC) #5
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/11348166/2001
6 years, 5 months ago (2012-11-20 23:01:03 UTC) #6
Chris Rogers
As per our offline discussion, I'd hold off on changing this
6 years, 5 months ago (2012-11-20 23:11:26 UTC) #7
Chris Rogers
Ok, Dale says he's going to be checking/testing and monitoring this closely, so ok with ...
6 years, 5 months ago (2012-11-20 23:36:50 UTC) #8
Chris Rogers
On 2012/11/20 23:36:50, Chris Rogers wrote: > Ok, Dale says he's going to be checking/testing ...
6 years, 5 months ago (2012-11-21 07:25:27 UTC) #9
DaleCurtis
I actually had a better idea for this last night: Move the DataReady() check into ...
6 years, 5 months ago (2012-11-21 18:50:30 UTC) #10
DaleCurtis
PTAL. https://codereview.chromium.org/11348166/diff/12001/media/audio/win/waveout_output_win.cc File media/audio/win/waveout_output_win.cc (right): https://codereview.chromium.org/11348166/diff/12001/media/audio/win/waveout_output_win.cc#newcode350 media/audio/win/waveout_output_win.cc:350: callback_->WaitTillDataReady(); I'm digging through the WaveOut docs to ...
6 years, 5 months ago (2012-11-21 19:31:06 UTC) #11
Chris Rogers
Thanks Dale, this looks much better. https://codereview.chromium.org/11348166/diff/12001/media/audio/audio_output_controller.cc File media/audio/audio_output_controller.cc (right): https://codereview.chromium.org/11348166/diff/12001/media/audio/audio_output_controller.cc#newcode323 media/audio/audio_output_controller.cc:323: base::PlatformThread::YieldCurrentThread(); We should ...
6 years, 5 months ago (2012-11-21 19:33:36 UTC) #12
scherkus (not reviewing)
https://codereview.chromium.org/11348166/diff/12001/media/audio/audio_io.h File media/audio/audio_io.h (right): https://codereview.chromium.org/11348166/diff/12001/media/audio/audio_io.h#newcode78 media/audio/audio_io.h:78: // by Window's WaveOut clients which may be extremely ...
6 years, 5 months ago (2012-11-21 21:37:57 UTC) #13
DaleCurtis
https://codereview.chromium.org/11348166/diff/12001/media/audio/audio_io.h File media/audio/audio_io.h (right): https://codereview.chromium.org/11348166/diff/12001/media/audio/audio_io.h#newcode78 media/audio/audio_io.h:78: // by Window's WaveOut clients which may be extremely ...
6 years, 5 months ago (2012-11-21 22:42:10 UTC) #14
scherkus (not reviewing)
lgtm code-wise w/ nits leaving it up to crogers for final look over and what ...
6 years, 5 months ago (2012-11-21 22:46:55 UTC) #15
DaleCurtis
https://codereview.chromium.org/11348166/diff/13002/media/audio/audio_output_controller.cc File media/audio/audio_output_controller.cc (right): https://codereview.chromium.org/11348166/diff/13002/media/audio/audio_output_controller.cc#newcode329 media/audio/audio_output_controller.cc:329: const base::TimeDelta max_wait = base::TimeDelta::FromMilliseconds(1500); On 2012/11/21 22:46:55, scherkus ...
6 years, 5 months ago (2012-11-21 22:50:15 UTC) #16
Chris Rogers
lgtm
6 years, 5 months ago (2012-11-21 23:43:17 UTC) #17
DaleCurtis
Bah! This fails w/ AudioOutputResampler under load since 4k buffer requires 2 callbacks to fill, ...
6 years, 5 months ago (2012-11-22 00:24:46 UTC) #18
Chris Rogers
https://codereview.chromium.org/11348166/diff/6012/media/audio/audio_output_resampler.cc File media/audio/audio_output_resampler.cc (right): https://codereview.chromium.org/11348166/diff/6012/media/audio/audio_output_resampler.cc#newcode360 media/audio/audio_output_resampler.cc:360: source_callback_->WaitTillDataReady(); I'm really not too crazy about this, although ...
6 years, 5 months ago (2012-11-22 00:39:45 UTC) #19
DaleCurtis
https://codereview.chromium.org/11348166/diff/6012/media/audio/audio_output_resampler.cc File media/audio/audio_output_resampler.cc (right): https://codereview.chromium.org/11348166/diff/6012/media/audio/audio_output_resampler.cc#newcode360 media/audio/audio_output_resampler.cc:360: source_callback_->WaitTillDataReady(); On 2012/11/22 00:39:46, Chris Rogers wrote: > I'm ...
6 years, 5 months ago (2012-11-22 00:53:58 UTC) #20
Chris Rogers
lgtm - unfortunate that we need the hack, but improvements to AudioSyncReader look good
6 years, 4 months ago (2012-11-26 20:59:46 UTC) #21
scherkus (not reviewing)
lgtm w/ random suggestion https://codereview.chromium.org/11348166/diff/12003/media/audio/audio_io.h File media/audio/audio_io.h (right): https://codereview.chromium.org/11348166/diff/12003/media/audio/audio_io.h#newcode81 media/audio/audio_io.h:81: virtual void WaitTillDataReady() {} considering ...
6 years, 4 months ago (2012-11-26 23:19:45 UTC) #22
DaleCurtis
That seems reasonable after we've removed the Mac caller. I'll rename it at that time.
6 years, 4 months ago (2012-11-26 23:23:26 UTC) #23
commit-bot: I haz the power
6 years, 4 months ago (2012-11-26 23:27:58 UTC) #24

Powered by Google App Engine
This is Rietveld 408576698