Closed Bug 616800 Opened 11 years ago Closed 11 years ago

Audio playback is very choppy, pauses every second during playback

Categories

(Core :: Audio/Video, defect)

x86
Windows 7
defect
Not set
normal

Tracking

()

VERIFIED FIXED
Tracking Status
blocking2.0 --- final+

People

(Reporter: marek.raida, Assigned: cpearce)

References

()

Details

(Keywords: regression, testcase)

Attachments

(3 files, 2 obsolete files)

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
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
Blocks: 610570
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression, testcase
Version: unspecified → Trunk
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!
blocking2.0: --- → ?
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...
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.
Assignee: nobody → chris
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.
It makes more sense to me to fix sydneyaudio.
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.
Attached patch Patch v1 (obsolete) — Splinter Review
* 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.
Attachment #497536 - Flags: review?(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.
Attached patch Patch v2 (obsolete) — Splinter Review
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.
Attachment #497536 - Attachment is obsolete: true
Attachment #498372 - Flags: review?(kinetik)
Attachment #497536 - Flags: review?(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.
Attached patch Patch v3Splinter Review
Review comments addressed. Also made audioDuration in AudioLoop() signed to prevent signed/unsigned comparison compiler warning in gcc.
Attachment #498372 - Attachment is obsolete: true
Attachment #498436 - Flags: review?(kinetik)
Attachment #498372 - Flags: review?(kinetik)
Attachment #498436 - Flags: review?(kinetik) → review+
http://hg.mozilla.org/mozilla-central/rev/99b19e566bbf
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Blocks: 620331
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.