Always wait for DataReady() on Windows WaveOut.

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

[DaleCurtis, 2012-11-20 20:43:30]

If we could ensure Pepper clients always use GetAudioHardwareBufferSize() we
could might be able to get rid of this, but that will have to wait for another
day.

For now we can just make it slightly more efficient now that all clients use the
shared memory marker.

[scherkus (not reviewing), 2012-11-20 21:08:46]

q

https://codereview.chromium.org/11348166/diff/2001/content/browser/renderer_h...
File content/browser/renderer_host/media/audio_sync_reader.cc (right):

https://codereview.chromium.org/11348166/diff/2001/content/browser/renderer_h...
content/browser/renderer_host/media/audio_sync_reader.cc:66: //
http://crbug.com/161307 and http://crbug.com/61022
does it make sense to add a run-time check for XP?

[DaleCurtis, 2012-11-20 21:33:47]

https://codereview.chromium.org/11348166/diff/2001/content/browser/renderer_h...
File content/browser/renderer_host/media/audio_sync_reader.cc (right):

https://codereview.chromium.org/11348166/diff/2001/content/browser/renderer_h...
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:
> does it make sense to add a run-time check for XP?

I thought about that, but there are cases where we fall back to WaveOut even on
Vista+.  Ideally this check would be something like !low_latency_path ||
!DataReady() ...

On WASAPI based platforms this should be called in regular intervals large
enough that DataReady() is always true, so it becomes a semi-nop.

[scherkus (not reviewing), 2012-11-20 21:50:23]

lgtm but my knowledge of this area is rusty -- perhaps ping crogers@ to take a
quick look?

FWIW doing w/o the artificial delay seems like a good thing

[DaleCurtis, 2012-11-20 22:59:34]

Per discussion with Chris, I'll land this now and get some feedback from canary
users.  Long term we should see about getting PPAPI to return a recommended
buffer size of:

std::min(request_sample_rate / hw_sample_rate, 1) * hw_buffer_size *
(request_buffer_size + hw_buffer_size - 1) / hw_buffer_size;

Which ensures the requested buffer size is always a multiple of the
hw_buffer_size, doing so should allow us to avoid back to back shared memory
reads and remove this hack entirely.

The caveat is if WaveOut is under-performing enough that we're hitting back to
back reads like this mid-playback we won't be able to remove this hack without
reworking the WaveOut back end (perhaps using a DirectSound version instead).

[commit-bot: I haz the power, 2012-11-20 23:01:03]

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

[Chris Rogers, 2012-11-20 23:11:26]

As per our offline discussion, I'd hold off on changing this

[Chris Rogers, 2012-11-20 23:36:50]

Ok, Dale says he's going to be checking/testing and monitoring this closely, so
ok with me

[Chris Rogers, 2012-11-21 07:25:27]

On 2012/11/20 23:36:50, Chris Rogers wrote:
> Ok, Dale says he's going to be checking/testing and monitoring this closely,
so
> ok with me

The more I think about this, I'm a bit worried about this.  I didn't fully
realize that this is also touching the WASAPI path!  I can see that Andrew added
a comment in here that we should limit the yielding to waveOut, which I agree
with.  I think it's a really bad idea to be yielding at all on the high-priority
WASAPI thread, and now we're increasing our chance of doing so.

[DaleCurtis, 2012-11-21 18:50:30]

I actually had a better idea for this last night:

Move the DataReady() check into WaveOut::QueueNextPacket() and update
AudioOutputController::WaitTillDataReady() to do what we're currently doing in
AudioSyncReader today.

[DaleCurtis, 2012-11-21 19:31:06]

PTAL.

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

https://codereview.chromium.org/11348166/diff/12001/media/audio/win/waveout_o...
media/audio/win/waveout_output_win.cc:350: callback_->WaitTillDataReady();
I'm digging through the WaveOut docs to see if we can just do the equivalent of
"return 0" instead of waiting.

[Chris Rogers, 2012-11-21 19:33:36]

Thanks Dale, this looks much better.

https://codereview.chromium.org/11348166/diff/12001/media/audio/audio_output_...
File media/audio/audio_output_controller.cc (right):

https://codereview.chromium.org/11348166/diff/12001/media/audio/audio_output_...
media/audio/audio_output_controller.cc:323:
base::PlatformThread::YieldCurrentThread();
We should probably check that this while is not spinning for an unreasonable
amount of time to avoid potential infinite-loop / hang.
In other words, some kind of timeout to handle the case when for any reason (bug
or otherwise), the renderer gets into a condition where it's unable to *ever*
have data ready.

[scherkus (not reviewing), 2012-11-21 21:37:57]

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#ne...
media/audio/audio_io.h:78: // by Window's WaveOut clients which may be extremely
laggy.  Will yield the
nit: Windows'

https://codereview.chromium.org/11348166/diff/12001/media/audio/audio_output_...
File media/audio/audio_output_controller.cc (right):

https://codereview.chromium.org/11348166/diff/12001/media/audio/audio_output_...
media/audio/audio_output_controller.cc:322: while (!sync_reader_->DataReady())
how about backing up your comments w/ something like

#if defined(OS_WIN)
...
#else
  CHECK(false) << "You didn't read the comment, did you?"
#endif

https://codereview.chromium.org/11348166/diff/12001/media/audio/audio_output_...
media/audio/audio_output_controller.cc:323:
base::PlatformThread::YieldCurrentThread();
On 2012/11/21 19:33:37, Chris Rogers wrote:
> We should probably check that this while is not spinning for an unreasonable
> amount of time to avoid potential infinite-loop / hang.
> In other words, some kind of timeout to handle the case when for any reason
(bug
> or otherwise), the renderer gets into a condition where it's unable to *ever*
> have data ready.

perhaps a CHECK()-timeout and monitor for any reports?

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

https://codereview.chromium.org/11348166/diff/12001/media/audio/win/waveout_o...
media/audio/win/waveout_output_win.cc:346: // still using the WaveOut back end,
we run into synchronization issues where
s/back end/backend/

[DaleCurtis, 2012-11-21 22:42:10]

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#ne...
media/audio/audio_io.h:78: // by Window's WaveOut clients which may be extremely
laggy.  Will yield the
On 2012/11/21 21:37:57, scherkus wrote:
> nit: Windows'

Done.

https://codereview.chromium.org/11348166/diff/12001/media/audio/audio_output_...
File media/audio/audio_output_controller.cc (right):

https://codereview.chromium.org/11348166/diff/12001/media/audio/audio_output_...
media/audio/audio_output_controller.cc:322: while (!sync_reader_->DataReady())
On 2012/11/21 21:37:57, scherkus wrote:
> how about backing up your comments w/ something like
> 
> #if defined(OS_WIN)
> ...
> #else
>   CHECK(false) << "You didn't read the comment, did you?"
> #endif

We'll need OS_MAC in there too for now since the high latency path uses this to
spool audio output buffers.  I left it out of the comment since we're planning
to remove that path in the future.

https://codereview.chromium.org/11348166/diff/12001/media/audio/audio_output_...
media/audio/audio_output_controller.cc:323:
base::PlatformThread::YieldCurrentThread();
On 2012/11/21 21:37:57, scherkus wrote:
> On 2012/11/21 19:33:37, Chris Rogers wrote:
> > We should probably check that this while is not spinning for an unreasonable
> > amount of time to avoid potential infinite-loop / hang.
> > In other words, some kind of timeout to handle the case when for any reason
> (bug
> > or otherwise), the renderer gets into a condition where it's unable to
*ever*
> > have data ready.
> 
> perhaps a CHECK()-timeout and monitor for any reports?

CHECK() worries me, I've added a 1.5 second timeout for now.

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

https://codereview.chromium.org/11348166/diff/12001/media/audio/win/waveout_o...
media/audio/win/waveout_output_win.cc:346: // still using the WaveOut back end,
we run into synchronization issues where
On 2012/11/21 21:37:57, scherkus wrote:
> s/back end/backend/

Done.

[scherkus (not reviewing), 2012-11-21 22:46:55]

lgtm code-wise w/ nits

leaving it up to crogers for final look over and what he discovers from msdn
docs :)

https://codereview.chromium.org/11348166/diff/13002/media/audio/audio_output_...
File media/audio/audio_output_controller.cc (right):

https://codereview.chromium.org/11348166/diff/13002/media/audio/audio_output_...
media/audio/audio_output_controller.cc:329: const base::TimeDelta max_wait =
base::TimeDelta::FromMilliseconds(1500);
s/max_wait/kMaxWait/

https://codereview.chromium.org/11348166/diff/13002/media/audio/audio_output_...
media/audio/audio_output_controller.cc:335: // WaitTillDataReady() is deprecated
and should not be used.
make this a string?

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

https://codereview.chromium.org/11348166/diff/13002/media/audio/audio_output_...
File media/audio/audio_output_controller.cc (right):

https://codereview.chromium.org/11348166/diff/13002/media/audio/audio_output_...
media/audio/audio_output_controller.cc:329: const base::TimeDelta max_wait =
base::TimeDelta::FromMilliseconds(1500);
On 2012/11/21 22:46:55, scherkus wrote:
> s/max_wait/kMaxWait/

Done.

https://codereview.chromium.org/11348166/diff/13002/media/audio/audio_output_...
media/audio/audio_output_controller.cc:335: // WaitTillDataReady() is deprecated
and should not be used.
On 2012/11/21 22:46:55, scherkus wrote:
> make this a string?

I think it's unncessary and just adds string bloat.  Every CHECK() comes with a
line number and this one is pretty clear.

[Chris Rogers, 2012-11-21 23:43:17]

lgtm

[DaleCurtis, 2012-11-22 00:24:46]

Bah! This fails w/ AudioOutputResampler under load since 4k buffer requires 2
callbacks to fill, the first waits in WaveOut, but the second doesn't in
AudioOutputResampler.

I've patched AudioOutputResampler to add a waveout_wait_hack_ variable which is
uses to call WaitTillDataReady().

In load testing, things work fine with flash, but HTML5 media played in a
background tab stalls out for long periods of time when the system is under 100%
cpu usage.  Perhaps unavoidable, but worth noting.

WDYT?

[Chris Rogers, 2012-11-22 00:39:45]

https://codereview.chromium.org/11348166/diff/6012/media/audio/audio_output_r...
File media/audio/audio_output_resampler.cc (right):

https://codereview.chromium.org/11348166/diff/6012/media/audio/audio_output_r...
media/audio/audio_output_resampler.cc:360:
source_callback_->WaitTillDataReady();
I'm really not too crazy about this, although I understand the problem you're
seeing.  If I'm not mistaken, it's that the client buffer size is 2048, but the
hardware buffer size is larger (4096), and is calling back twice?  If so, can we
at least have a TODO that we should be able to fix this if we FIFO on the
renderer-side and prevent the client buffer-size from being smaller than the
hardware buffer-size.  This seems like a better long-term solution because that
would handle the case for small client buffer sizes for all platforms.

[DaleCurtis, 2012-11-22 00:53:58]

https://codereview.chromium.org/11348166/diff/6012/media/audio/audio_output_r...
File media/audio/audio_output_resampler.cc (right):

https://codereview.chromium.org/11348166/diff/6012/media/audio/audio_output_r...
media/audio/audio_output_resampler.cc:360:
source_callback_->WaitTillDataReady();
On 2012/11/22 00:39:46, Chris Rogers wrote:
> I'm really not too crazy about this, although I understand the problem you're
> seeing.  If I'm not mistaken, it's that the client buffer size is 2048, but
the
> hardware buffer size is larger (4096), and is calling back twice?  If so, can
we
> at least have a TODO that we should be able to fix this if we FIFO on the
> renderer-side and prevent the client buffer-size from being smaller than the
> hardware buffer-size.  This seems like a better long-term solution because
that
> would handle the case for small client buffer sizes for all platforms.
> 
> 

Yes that's the problem.  A FIFO won't help w/o accounting for resampling and
would be difficult to implement for all of our render clients.

Instead we just need to ensure that every client provide a multiple of the
hw_buffer_size scaled by input sample rate / hw sample rate.  That way every hw
stream request results in at most one renderer side request.  I've added a
comment and linked to a bug for this.

Making that change should be easier than trying to implement a FIFO as well.  We
can discuss more on the bug.

[Chris Rogers, 2012-11-26 20:59:46]

lgtm - unfortunate that we need the hack, but improvements to AudioSyncReader
look good

[scherkus (not reviewing), 2012-11-26 23:19:45]

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#ne...
media/audio/audio_io.h:81: virtual void WaitTillDataReady() {}
considering this has a single purpose... should we just rename this
WaitForWaveOutHack()?

[DaleCurtis, 2012-11-26 23:23:26]

That seems reasonable after we've removed the Mac caller. I'll rename it at that
time.

[commit-bot: I haz the power, 2012-11-26 23:27:58]

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