616800 - Audio playback is very choppy, pauses every second during playback

Status: VERIFIED FIXED

(Core :: Audio/Video, defect)

Description

[Marek Raida]

User-Agent:       Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b8pre) Gecko/20101204 Firefox/4.0b8pre
Build Identifier:               Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b8pre) Gecko/20101204 Firefox/4.0b8pre

I noticed it creating audio demo, that OGG playback is very very choppy, pauses for approx. 100ms every 1000ms, making it almost unusable.
Then tried the same with direct HTML and it is the same. On Windows XP it is better, on Windows 7 it is much worse. I tried turning on/off HW video acceleration but it has no effect, it is still the same. 

Reproducible: Always

Steps to Reproduce:
1. Open demo from link or simplified testcase from attachment

Actual Results:  
unexpected playback pauses happening every second

Expected Results:  
fluent playback

Comment 1

[Marek Raida]

Attachment: Simplified testcase with in plain html, behaving the same bad way...

Comment 2

[(mostly gone) XtC4UaLL [:xtc4uall]]

Last good nightly: 2010-11-28 First bad nightly: 2010-11-29
Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=9537e23f9708&tochange=5f9204fe5cd5 (on WinXP)
=> Candidate: Bug 610570

Comment 3

[Chris Pearce [:cpearce (Not reading bugmail)]]

Testcase uses a data uri, looks like data uri's aren't handled properly by the new buffering logic. This should definitely block 2.0! Thanks for filing!

Comment 4

[Marek Raida]

Attachment: Testcase with external audio file

Well, it seems data-uri is not to be blamed, because for me external audio file behaves exactly the same wrong way as embedded one, try the new testcase...

Comment 5

[Chris Pearce [:cpearce (Not reading bugmail)]]

Thanks for pointing that out Marek.

This seems to be another consequence of us dropping AMPLE_AUDIO_MS from 2000ms to 1000ms. Somehow that is causing us to underrun on that file, even when it's loaded locally from disk.

Comment 6

[Chris Pearce [:cpearce (Not reading bugmail)]]

The problem file is an 8Hz, 16bit, 1 channel file. This bug only occurs on Windows. The problem is that the libsyndeyaudio waveapi backend won't actually pass on audio data written to it to the waveapi until it's got a full "block", and its block size is 16KB.

So if we go to sleep without having written a full block, we won't actually play the data we think we should play while we sleep. This is what's causing the start/stop. We're writing 1s of data, and that's playing, but then we're sleeping for 0.5s, waking up and writing 0.5s of data (half a block's worth) and sleeping again for 0.5s, but our second write doesn't fill a block, so it's not being passed onto the waveapi, so doesn't play on time until we wake up again and write some more.

So one solution would be to instead of always having a "wait-threshold" value of AMPLE_AUDIO_MS, as used at http://mxr.mozilla.org/mozilla-central/source/content/media/nsBuiltinDecoderStateMachine.cpp#486, we instead ensure that this threshold is always at least the equivalent of 32KB worth of audio data. We can then guarantee that before we sleep in AudioLoop(), that we've written enough to ensure that at least 1 complete libsydneyaudio block gets pushed down to the waveapi.

I've tested this approach with files as low as 1Hz, and it seems to work fine, but I'm open to suggestions for better ways to fix this.

Comment 7

[Matthew Gregan [:kinetik]]

It makes more sense to me to fix sydneyaudio.

Comment 8

[Matthew Gregan [:kinetik]]

Specifically, change the buffer allocation in sydney_audio_waveapi to be time based, then expose the minimum write length via a new API.  Then, any time audio data is written, it must ensure that it writes at least the minimum write length before sleeping on the audio clock (or otherwise expecting the audio stream state to change).  This is same thing I mentioned in bug 615134 comment 4, but hoped we could avoid until the audio rewrite.

Comment 9

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v1

* Ensure we don't sleep if we've written less than the start threshold of audio data on Windows and Linux.
* If we drain on Linux and we've written less than the start threshold, push enough silence to reach the threshold to ensure playback starts.
* Includes testcase which is 99 samples (at 1000Hz), which is less than the start threshold.

Comment 10

[Matthew Gregan [:kinetik]]

+PRInt32 nsAudioStreamRemote::GetMinWriteSamples()
+{
+  return 1;
+}

This needs to be remoted.

+  size_t            start_threshold_samples;

Remove this everywhere and...

+sa_stream_get_min_write(sa_stream_t *s, size_t *nbytes) {

...use snd_pcm_sw_params_get_start_threshold here.

Also, let's remove the code in snd_pcm_open that tweaks that start_threshold since it's not necessary and larger is generally better.

+  min_write = snd_pcm_frames_to_bytes(s->output_unit, s->start_threshold_samples);
+  if (s->bytes_written < min_write) {

What about in the case where a long stream is near the end and we've just resumed from pause/underrun?  I think it'd be better to check snd_pcm_state for PREPARED and then write silence.

+UNSUPPORTED(int sa_stream_get_min_write(sa_stream_t *s, size_t *nbytes))

It might make sense to have the OS X code return BUF_SIZE, but it's not strictly necessary.

Everything else looks good.

Comment 11

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v2

With review comments addressed. I didn't implement GetMinWriteSamples() for the remote audio stream, I'll file a bug for that once this patch lands. I've not implemented sa_stream_get_min_write for OSX since I'm not sure it's necessary.

Comment 12

[Matthew Gregan [:kinetik]]

Should've noticed this one earlier, but:
+sa_stream_get_min_write(sa_stream_t *s, int32_t *samples) {

The samples argument should be unsigned.

+    if (s->bytes_written < min_bytes) {
+      /* Not written enough to begin playback, play silence to cross the gap. */

This means the logic only triggers if the total stream length is less than the start threshold.  It also makes sense to use this logic if the amount written since the last stream reset is less than the start threshold--for example, if there is an underrun and after recovery there's less than start_threshold samples left to play, we need this logic to finish playing the stream.  So just remove this test and always write start_threshold samples of silence if the stream is still PREPARED.

Comment 13

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v3

Review comments addressed. Also made audioDuration in AudioLoop() signed to prevent signed/unsigned comparison compiler warning in gcc.

Comment 14

[Chris Pearce [:cpearce (Not reading bugmail)]]

http://hg.mozilla.org/mozilla-central/rev/99b19e566bbf