463627 - <video> audio playback stops after some seconds with alsa backend playing via pulseaudio

Status: VERIFIED FIXED

(Core :: Audio/Video, defect)

Description

[Jan Gerber]

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.1b2pre) Gecko/20081104 Minefield/3.1b2pre
Build Identifier: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.1b2pre) Gecko/20081104 Minefield/3.1b2pre

right now the ogg backend just returns if the audio buffer is filled, instead it should wait and try again. 

Reproducible: Always

Steps to Reproduce:
1. open page with <video> on linux with pulseaudio
2. play video

Actual Results:  
3. audio stops

Expected Results:  
3. audio keeps playing

Comment 1

[Jan Gerber]

Attachment: patch to solve this

Comment 2

[maikmerten]

I can confirm this issue on Ubuntu 8.10. Here "some seconds" is actually more like half a second ;-)

Comment 3

[cajbir (:cajbir)]

Attachment: Refactor original patch

I reworked the original patch to not use PRSleep (we can't block, so I wait on the monitor instead).

j or maikmerten, can you test with this and see if it still fixes the issue? I can't reproduce the original problem.

Comment 4

[Jan Gerber]

the patch does not work, playback stops after 0.20000000298023224 seconds.

Comment 5

[Jan Gerber]

in http://trac.annodex.net/changeset/3808 i made a change to libsydneyaudio that fixed the problems with pulseaudio here.
the problem seams to be that the available buffer space does not get updated as long as no new samples are written to the buffer, so it is locked, the initial patch did not check again if the buffer would be free.

Comment 6

[cajbir (:cajbir)]

So the libsyndeyaudio change is the only change needed to fix this bug?

Comment 7

[cajbir (:cajbir)]

Attachment: libsydneyaudio fix for audio problem

Comment 8

[Jan Gerber]

i have the libsydneyaudio fix and https://bugzilla.mozilla.org/attachment.cgi?id=348885 applied can try without the second, but it seams like the right thing to do in case of a full buffer.

Comment 9

[cajbir (:cajbir)]

Attachment: Work in progress patch

This is a work in progress patch that changes the way the ogg backend handles writing audio in the case where not enough buffer space is available in the audio backendd to avoid blocking. It fixes the issue on the machine I have access to that exhibits the problem.

I haven't applied the libsydneyaudio fix, as with that applied some writes to sydney audio block for a short period of time. This results in very slight choppy video playback.

I still need to do more testing, and try on Windows/Mac, and test the wav backend.

Comment 10

[Jan Gerber]

confirming that your patch fixes the issues from me too.
reverted the change in libsydneyaudio trunk.

Comment 11

[cajbir (:cajbir)]

Requires libsydneyaudio fix in bug 468206 for Windows playback.

Comment 12

[cajbir (:cajbir)]

Make that bug 468260. Typo.

Comment 13

[cajbir (:cajbir)]

Attachment: Final patch

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The 'temp' buffer in nsAudioStream::Drain should not be needed.

Comment 15

[Matthew Gregan [:kinetik]]

-  mSamplesBuffered += aCount;
-
   if (!mAudioHandle)
     return;
...
+   mSamplesBuffered += count;

This will break the fake Drain() impl, since we won't ever count samples when we
don't have an audio handle, so the fake Drain() won't sleep for the correct
duration.



+      for (PRUint32 i=0; i < aCount; ++i) {
...
+          s_data[i+offset] = (scaled_value > 32767.0) ?

Übernit: whitespace is inconsistent in a couple of lines like these.



+      mSamplesBuffered += count;
+      PRUint32 drainTime = (float(available) / mRate / mChannels) * 1000.0;
+      PR_Sleep(PR_MillisecondsToInterval(drainTime));

No need to update mSamplesBuffered here, it should be the correct value when
Write() is called (assuming the comment above is fixed).  It's only used to track how much data is
"buffered" so we can fake a realistic drain time, so we don't need to treat it specially during the overflow buffer write out.

Comment 16

[cajbir (:cajbir)]

Attachment: Address review comments

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+    if (!sa_stream_write(static_cast<sa_stream_t*>(mAudioHandle),
+                         mBufferOverflow.Elements(),
+                         mBufferOverflow.Length()) == SA_SUCCESS)

Shouldn't this be "if (sa_stream_wraite(...) != SA_SUCCESS)"?

Comment 18

[Matthew Gregan [:kinetik]]

-    if (sa_stream_write(static_cast<sa_stream_t*>(mAudioHandle), s_data.get(), aCount * sizeof(short)) != SA_SUCCESS) {
+   mSamplesBuffered += count;

mSamplesBuffered is already updated at the top of Write, so this can be deleted.

Comment 19

[cajbir (:cajbir)]

Attachment: Address review comment

Comment 20

[cajbir (:cajbir)]

Attachment: Missed Matthews comment

Comment 21

[cajbir (:cajbir)]

Attachment: White space fixes

Whitespace fixes. Carry forward roc's sr.

Comment 22

[cajbir (:cajbir)]

Attachment: Fix write underrun in drain causing test failure

test_wave_8bit failed due to a write underrun in drain on linux machines. This patch update fixes that.

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 352973 [details] [diff] [review]
Fix write underrun in drain causing test failure

Good catch.

Comment 24

[cajbir (:cajbir)]

Pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/28f2413aca75

Comment 25

[cajbir (:cajbir)]

Pushed to mozilla-1.9.1:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/3d8040d7539e

Comment 26

[Dan Ho]

Audio lag after playing a couple of hours of flash10, H.264 live.tiwt.tv streaming, horrible audio lag via SeaMonkey memory hang - SeaMonkey wont unload until the buffer is played all the way out!

This is not resolved.

Comment 27

[cajbir (:cajbir)]

Dan, sounds like a different bug. This one is for the built in <audio> and <video> controls, not Flash 10.

Comment 28

[Tony Chung [:tchung]]

Hi, can someone with a video + pulseaudio testcase verify this is indeed fixed on trunk and 1.9.1 branch?   I would do it, if you provide a testcase.  Thanks.

Comment 29

[Aaron Train [:aaronmt]]

(In reply to comment #28)
> Hi, can someone with a video + pulseaudio testcase verify this is indeed fixed
> on trunk and 1.9.1 branch?   I would do it, if you provide a testcase.  Thanks.

Verified FIXED on trunk and 1.9.1 branch using Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1pre) Gecko/20090528 Shiretoko/3.5pre 

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2a1pre) Gecko/20090528 Minefield/3.6a1pre

Tested in Ubuntu 8.10 as per Comment #2 - environment, running an alsa backend playing pulseaudio