Closed Bug 463627 Opened 13 years ago Closed 13 years ago

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

Categories

(Core :: Audio/Video, defect)

x86
Linux
defect
Not set
normal

Tracking

()

VERIFIED FIXED

People

(Reporter: j, Assigned: cajbir)

References

Details

(Keywords: fixed1.9.1)

Attachments

(1 file, 9 obsolete files)

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
Attached patch patch to solve this (obsolete) — Splinter Review
Component: General → Video/Audio
Product: Firefox → Core
Version: unspecified → Trunk
I can confirm this issue on Ubuntu 8.10. Here "some seconds" is actually more like half a second ;-)
Attached patch Refactor original patch (obsolete) — Splinter Review
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.
Attachment #346881 - Attachment is obsolete: true
the patch does not work, playback stops after 0.20000000298023224 seconds.
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.
So the libsyndeyaudio change is the only change needed to fix this bug?
Assignee: nobody → chris.double
Attachment #348885 - Attachment is obsolete: true
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: blocking1.9.1?
Blocks: 464457
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.
Flags: blocking1.9.1? → blocking1.9.1+
Attached patch Work in progress patch (obsolete) — Splinter Review
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.
Attachment #351305 - Attachment is obsolete: true
confirming that your patch fixes the issues from me too.
reverted the change in libsydneyaudio trunk.
Requires libsydneyaudio fix in bug 468206 for Windows playback.
Depends on: 468206
Make that bug 468260. Typo.
Depends on: 468260
No longer depends on: 468206
Attached patch Final patch (obsolete) — Splinter Review
Attachment #352047 - Attachment is obsolete: true
Attachment #352953 - Flags: superreview?(roc)
Attachment #352953 - Flags: review?(kinetik)
The 'temp' buffer in nsAudioStream::Drain should not be needed.
-  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.
Attached patch Address review comments (obsolete) — Splinter Review
Attachment #352953 - Attachment is obsolete: true
Attachment #352961 - Flags: superreview?(roc)
Attachment #352961 - Flags: review?(kinetik)
Attachment #352953 - Flags: superreview?(roc)
Attachment #352953 - Flags: review?(kinetik)
+    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)"?
-    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.
Attached patch Address review comment (obsolete) — Splinter Review
Attachment #352961 - Attachment is obsolete: true
Attachment #352964 - Flags: superreview?(roc)
Attachment #352964 - Flags: review?(kinetik)
Attachment #352961 - Flags: superreview?(roc)
Attachment #352961 - Flags: review?(kinetik)
Attached patch Missed Matthews comment (obsolete) — Splinter Review
Attachment #352964 - Attachment is obsolete: true
Attachment #352966 - Flags: superreview?(roc)
Attachment #352966 - Flags: review?(kinetik)
Attachment #352964 - Flags: superreview?(roc)
Attachment #352964 - Flags: review?(kinetik)
Attachment #352966 - Flags: superreview?(roc) → superreview+
Attached patch White space fixes (obsolete) — Splinter Review
Whitespace fixes. Carry forward roc's sr.
Attachment #352966 - Attachment is obsolete: true
Attachment #352967 - Flags: superreview+
Attachment #352967 - Flags: review?(kinetik)
Attachment #352966 - Flags: review?(kinetik)
Attachment #352967 - Flags: review?(kinetik) → review+
test_wave_8bit failed due to a write underrun in drain on linux machines. This patch update fixes that.
Attachment #352967 - Attachment is obsolete: true
Attachment #352973 - Flags: superreview?(roc)
Attachment #352973 - Flags: review?(kinetik)
Attachment #352973 - Flags: review?(kinetik) → review+
Comment on attachment 352973 [details] [diff] [review]
Fix write underrun in drain causing test failure

Good catch.
Attachment #352973 - Flags: superreview?(roc) → superreview+
Pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/28f2413aca75
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [baking for 1.9.1]
Pushed to mozilla-1.9.1:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/3d8040d7539e
Keywords: fixed1.9.1
Whiteboard: [baking for 1.9.1]
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.
Dan, sounds like a different bug. This one is for the built in <audio> and <video> controls, not Flash 10.
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.
(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
Status: RESOLVED → VERIFIED
QA Contact: general → video.audio
You need to log in before you can comment on or make changes to this bug.