Closed Bug 461286 Opened 11 years ago Closed 11 years ago

Small vorbis file does not play in <video> and <audio>

Categories

(Core :: Audio/Video, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9.2a1

People

(Reporter: cajbir, Assigned: cajbir)

References

()

Details

(Keywords: testcase, verified1.9.1)

Attachments

(5 files, 2 obsolete files)

Attached file testcase
The vorbis file in the URL plays no sound using <video> or <audio>. It plays fine in other vorbis players.

Steps to reproduce:

1) Create an html file with the following:
   <video src='http://www.bluishcoder.co.nz/js8080/sound/shot.ogg' controls></video>
2) Load html file and use the built in controls to play.

What happens:

No sound plays.

What should happen:

Sounds plays.

See testcase for html file.
Any audio data in the audio buffer was left unplayed when closing. Small audio samples would not play since they were open, written and closed quickly. Another issue was a tight loop that could occur if decoding was completed and frames were left to play.
Attachment #344438 - Flags: superreview?(roc)
Attachment #344438 - Flags: review?(roc)
+        // If the queue of decoded frame is full or we have completed decoding

"frames is full"

+        if (mPlaying) {
+          StopPlayback(PR_TRUE);
+        }

So here we block with the monitor held. That seems bad.

However, in general I don't see how to shutdown everything cleanly if the main thread wants to shut down the decoder while we're blocked draining the audio. Any ideas?
> Any ideas?

Instead of using the libsydneyaudio drain I'll write my own. When I first open the audio device I can query the available bytes to see the size of the buffer. Then when I want to drain I can loop, checking this, and Wait() for a period of time if it's not empty. That should resolve the issue I think.
Attached patch Don't use audio backend to drain (obsolete) — Splinter Review
This implements the manual draining of the audio buffer, and fixes the review comment. I'll test on other platforms before seeking review, and remove the aDrain option which is no longer used.
Attachment #344438 - Attachment is obsolete: true
Attachment #344438 - Flags: superreview?(roc)
Attachment #344438 - Flags: review?(roc)
Comment on attachment 344458 [details] [diff] [review]
Don't use audio backend to drain

The patch is out of date. The problem still exists and I'm looking into it. Small sound files will play once but do not replay correctly.
Attachment #344458 - Attachment is obsolete: true
Attached patch FixSplinter Review
The issue is that in the DECODING state if there are no frames in our queue we loop waiting for frames and checking for SHUTDOWN. If the step decode thread has exited during this loop it will never exit. This occurs when playing small audio only files quite regularly. 

It can be seen with the test file by playing it, then re-playing it. The 2nd and subsequent play's don't work due to being stuck in that loop.

The fix is to check if decoding is completed during the loop.
Attachment #378185 - Flags: superreview?(roc)
Attachment #378185 - Flags: review?(roc)
Flags: wanted1.9.1?
Attachment #378185 - Attachment is patch: true
Attachment #378185 - Attachment mime type: application/octet-stream → text/plain
Attachment #378185 - Flags: superreview?(roc)
Attachment #378185 - Flags: superreview+
Attachment #378185 - Flags: review?(roc)
Attachment #378185 - Flags: review+
Flags: wanted1.9.1? → wanted1.9.1+
Whiteboard: [needs landing]
Upgrading to blocking since this can leave threads spinning, really bad.

http://hg.mozilla.org/mozilla-central/rev/850c33bb7814

We should have a testcase here. 'ended' will never file on this stream, should be easy to test.
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: wanted1.9.1+
Flags: in-testsuite?
Flags: blocking1.9.1+
Resolution: --- → FIXED
Whiteboard: [needs landing] → [needs 191 landing]
Verified fix on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1pre) Gecko/20090522 Shiretoko/3.5pre
and Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090522 Minefield/3.6a1pre
Status: RESOLVED → VERIFIED
I'm trying to test this, and I never get a timeupdate after calling .play() .

Is it expected that audio files this small will never fire a timeupdate event?
No. Can you attach your testcase?
Attached file simple test case
Simple html test.
Should I file a new bug on this?
Yes please, and assign it to me.
I am a little confused here. 

With the aforementioned test file, (i.e., http://www.bluishcoder.co.nz/js8080/sound/shot.ogg), it will play fine on initial load. 

After playback, I am unable to play the file again (i.e, clicking the Play button). In addition, I get a spinning throbber and the seek scrubber does not reach the end of the timeline (from the short sound clip).

Tested this on 1.9.1 branch and 1.9.2 trunk

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1pre) Gecko/20090604 Shiretoko/3.5pre

and

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090604 Minefield/3.6a1pre

Can I get some clarification on this? Bug?
Marking as reopened until Comment #17 can be verified
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
I get same behavior described in Comment #17. Same version.
Also, it seems Roc indicated this was blocking because we were leaving threads spinning, Comment #8.  If that's not the case anymore, do we still want to block on this, Roc?
Well... any case new bug filed bug 496456
I was not getting this issue when I originally did this fix. Multiple playbacks worked fine. I think something landing after this has caused the problem.
Re-closing.
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
Chris, we have a neat mochitest now. Can we get it checked into the tree?
Keywords: testcase
Target Milestone: --- → mozilla1.9.2a1
Attached patch Adds testcaseSplinter Review
This adds a testcase for the small ogg shot file to see if it plays through and ended is set correctly. The test to see if timeupdate is fired already exists as test_bug495319.html.
Testcase in attachment 392427 [details] [diff] [review] pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/d70d123a365f
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.