Assume pending buffered bytes is zero when ALSA has underrun.

Assume pending buffered bytes is zero when ALSA has underrun.

According to docs, snd_pcm_delay() may not reach zero when ALSA has underrun.  Furthermore it may return negative numbers when underrun, leading to an underflow when converted to uint32.  Previously none of this was an issue however as of r43546 we now wait for pending buffered bytes to reach zero before notifying that the audio stream has finished.

This doesn't completely fix the Linux ended event issue, but is a required fix regardless.

BUG=30452
TEST=media_unittests

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

[scherkus (not reviewing), 2010-04-07 07:35:22]

CAVEAT: I'm at home and haven't tested this patch on Windows, Mac, etc...

ajwong: ALSA changes and if you could also try on PulseAudio system that'd be
slick (I'm not sure if they implement snd_pcm_state() as expected...)

hclam: audio host / impl changes

We now always reply to pending request even if we have no data.  This could have
some implications but would require some testing.

[Alpha Left Google, 2010-04-07 19:07:47]

On 2010/04/07 07:35:22, scherkus wrote:
> CAVEAT: I'm at home and haven't tested this patch on Windows, Mac, etc...
> 
> ajwong: ALSA changes and if you could also try on PulseAudio system that'd be
> slick (I'm not sure if they implement snd_pcm_state() as expected...)
> 
> hclam: audio host / impl changes
> 
> We now always reply to pending request even if we have no data.  This could
have
> some implications but would require some testing.

LGTM if removing the filled > 0 check doesn't have a perf hit when stalled.

[awong, 2010-04-07 20:05:47]

http://codereview.chromium.org/1618006/diff/2001/3001
File chrome/renderer/media/audio_renderer_impl.cc (right):

http://codereview.chromium.org/1618006/diff/2001/3001#newcode348
chrome/renderer/media/audio_renderer_impl.cc:348: new
ViewHostMsg_NotifyAudioPacketReady(0, stream_id_, filled));
So we're sending this even if filled==0 now?  That's the main change right?

http://codereview.chromium.org/1618006/diff/2001/3002
File media/audio/linux/alsa_output.cc (right):

http://codereview.chromium.org/1618006/diff/2001/3002#newcode481
media/audio/linux/alsa_output.cc:481: if (wrapper_->PcmState(playback_handle_)
!= SND_PCM_STATE_XRUN) {
Isn't this possibly a race?

If it underruns between here and the PcmDelay, we could be negative already.

Should we guard against delay being < 0 anyways?

http://codereview.chromium.org/1618006/diff/2001/3002#newcode493
media/audio/linux/alsa_output.cc:493: delay = 0;
This should be in the error < 0 statement, no?

http://codereview.chromium.org/1618006/diff/2001/3002#newcode495
media/audio/linux/alsa_output.cc:495: delay *= bytes_per_output_frame_;
This shouldn't be in an else clause should it?

[scherkus (not reviewing), 2010-04-08 01:08:50]

Reverted audio_renderer_impl.cc change to keep this focused on ALSA bug.

Doesn't fix the ended event bug anymore, but we need this change anyway.

Also addressed comments.

http://codereview.chromium.org/1618006/diff/2001/3001
File chrome/renderer/media/audio_renderer_impl.cc (right):

http://codereview.chromium.org/1618006/diff/2001/3001#newcode348
chrome/renderer/media/audio_renderer_impl.cc:348: new
ViewHostMsg_NotifyAudioPacketReady(0, stream_id_, filled));
On 2010/04/07 20:05:47, awong wrote:
> So we're sending this even if filled==0 now?  That's the main change right?

main change but as discovered there's bugs associated by doing so

http://codereview.chromium.org/1618006/diff/2001/3002
File media/audio/linux/alsa_output.cc (right):

http://codereview.chromium.org/1618006/diff/2001/3002#newcode481
media/audio/linux/alsa_output.cc:481: if (wrapper_->PcmState(playback_handle_)
!= SND_PCM_STATE_XRUN) {
On 2010/04/07 20:05:47, awong wrote:
> Isn't this possibly a race?
> 
> If it underruns between here and the PcmDelay, we could be negative already.
> 
> Should we guard against delay being < 0 anyways?

not sure but might as well add it in

http://codereview.chromium.org/1618006/diff/2001/3002#newcode493
media/audio/linux/alsa_output.cc:493: delay = 0;
On 2010/04/07 20:05:47, awong wrote:
> This should be in the error < 0 statement, no?

it's confusing but according to alsa if snd_pcm_delay() failed then we're
screwed and can't get the delay anyway

http://codereview.chromium.org/1618006/diff/2001/3002#newcode495
media/audio/linux/alsa_output.cc:495: delay *= bytes_per_output_frame_;
On 2010/04/07 20:05:47, awong wrote:
> This shouldn't be in an else clause should it?

it's correct but looks confusing -- will correct it

[awong, 2010-04-08 02:01:06]

LGTM