Closed Bug 474540 Opened 15 years ago Closed 15 years ago

High Definition Theora videos do not play smoothly using <video> element

Categories

(Core :: Audio/Video, defect)

x86
All
defect
Not set
normal

Tracking

()

VERIFIED FIXED

People

(Reporter: cajbir, Assigned: cajbir)

References

()

Details

(Keywords: verified1.9.1)

Attachments

(1 file, 4 obsolete files)

Large Theora video's, in particular 720p HD and above, do not play back smoothly in Firefox. The example in the URL skips audio and plays the video frames about half speed on my dual core laptop.

The liboggplay example player also exhibits this problem. Interestingly the libtheora example player does not. It plays this file very smoothly at about 25% cpu usage.

Steps to Reproduce:

1) Visit URL
2) Let a decent portion of the video load to ensure the problem is not due to network buffering.
3) Press play.

Result:

Playback is very poor

Expected Result:

Smooth playback, somewhere in the vicinity of the quality of the libtheora example.
I raised a ticket with the liboggplay developers:

http://trac.annodex.net/ticket/448

Any fix they apply to get the liboggplay example playback improved will be part of the fix to this bug,
I noticed vlad filed bug 474749 and wondered if that might be (part of) the cause of this bug.
Also bug 474748. Talking to the liboggplay developers on irc they also seem to think it's the yuv2rgb conversion.
Blocks: 476397
Attached patch fix for liboggplay hang (obsolete) — Splinter Review
The patch to improve playback requires this liboggplay patch from trac ticket 466 to prevent an infinite loop.
Assignee: nobody → chris.double
Status: NEW → ASSIGNED
Moves the oggplay_step_decode loop into a thread of its own so that the time spent decoding keyframes doesn't impact the playback. With this patch applied I can play 720p content without issue and some 1080p content.
Attachment #373257 - Attachment is obsolete: true
Attachment #373258 - Flags: superreview?(roc)
Attachment #373258 - Flags: review?(kinetik)
Summary: High Definition Theora video's do not play smoothly using <video> element → High Definition Theora videos do not play smoothly using <video> element
Attachment #373257 - Attachment is obsolete: false
+  // Return true if we are in a state where the decoder should not be running.
+  PRBool InStopDecodingState() {

Should this assert that we're in the decoder's monitor?

+        time = (TimeStamp::Now() - mPlayStartTime -mPauseDuration).ToSeconds();

space between - and mPauseDuration

       if (time >= frame->mTime) {

Why do we need to check this here? Seems to me it must be true because of the new for(;;) loop above.

+        while(!mDecodedFrames.IsEmpty() && time >= mDecodedFrames.Peek()->mTime) {

Space after "while"

+        // Skip frames up to the one we should be showing.
+        // We need to play the audio for these frames however.
+        while(!mDecodedFrames.IsEmpty() && time >= mDecodedFrames.Peek()->mTime) {
+          LOG(PR_LOG_DEBUG, ("Skipping frame time %f with audio at time %f", mDecodedFrames.Peek()->mTime, time));
+          delete frame;
+          frame = mDecodedFrames.Peek();
+          PlayAudio(frame);
+          mDecodedFrames.Pop();

Seems to me that playing the audio for skipped frames will make audio fall behind. For example if the browser process hangs for one second, audio will fall behind the displayed video frame by about one second. I think we should just drop the audio, for now, and fix it properly in your audio-clock-sync patch.

+        delete NextFrame();
+
+        if (mStepDecodeThread) {
+          mDecodingCompleted = PR_TRUE;
+          mBufferExhausted = PR_FALSE;
+          mon.NotifyAll();
+
+          // Flush liboggplay frame buffers so that we break out of
+          // of the decoding loop - this ensures the shutdown call
+          // below doesn't block.
+          delete NextFrame();

Why do we need the first "delete NextFrame()"? I presume oggplay_seek flushes oggplay's buffers?

Why do we keep the decode thread around while buffering? Would it be simpler to just destroy it and recreate when buffering ends?
In nsOggDecodeStateMachine::Run, DECODING state:

+        if (!mDecodingCompleted) {
+          if (!mStepDecodeThread) {
+            nsresult rv = NS_NewThread(getter_AddRefs(mStepDecodeThread));
+            if (NS_SUCCEEDED(rv)) {

Might be nicer with less nesting and an early continue:

+        if (!mDecodingCompleted && !mStepDecodeThread) {
+          nsresult rv = NS_NewThread(getter_AddRefs(mStepDecodeThread));
+          if (NS_FAILED(rv)) {

In nsOggDecodeStateMachine::Shutdown:

   mon.NotifyAll();
+
+  if (mPlayer) {
+    // This will unblock the step decode loop in the
+    // StepDecode thread. The thread can then be safely
+    // shutdown.
+    mBufferExhausted = PR_FALSE;
+    mon.NotifyAll();

The second NotifyAll() seems unnecessary.  The other thread sleeping in Wait() can't return until it owns the monitor, and the monitor isn't dropped until Shutdown() is called (and then only if mStepDecodeThread is non-null).
What happens when you fall behind?  Just wondering what the user experience will be here.
Here is a patten we use a lot in data processing

+        FrameData* frame;
+        do {
+          frame = NextFrame();
+          if (frame) {
+            mDecodedFrames.Push(frame);
+          }
+        } while (frame);


converted to ==>


       FrameData* frame = NextFrame();  //First time fetch
       while (frame) {
         // do all the processing with the fetched item
         mDecodedFrames.Push(frame);

         // do subsequent fetch
         frame = NextFrame();
       }


or again reduced to

       FrameData* frame;
       while (frame = NextFrame())
            mDecodedFrames.Push(frame);
Attached patch address review comments (obsolete) — Splinter Review
I changed the decoder loop so it continues decoding during buffering. It's playback that should be stopped rather than the decoding of the data.
Attachment #373258 - Attachment is obsolete: true
Attachment #373969 - Flags: superreview?(roc)
Attachment #373969 - Flags: review?(kinetik)
Attachment #373258 - Flags: superreview?(roc)
Attachment #373258 - Flags: review?(kinetik)
+      // We need to play the audio for these frames however.

Take this comment out now that we're skipping audio for those frames...

Let's go back to your previous approach of having the decoder thread wait while the buffer is exhausted. It'll be simpler overall since with the latest patch, the decoder thread can set mBufferExhausted incorrectly if we download a lot of data while blocked in oggplay_step_decoding.
Attached patch Revert buffering strategy (obsolete) — Splinter Review
Revert the buffering stragegy back to that of the previous patch and fix comment.
Attachment #373969 - Attachment is obsolete: true
Attachment #373991 - Flags: superreview?(roc)
Attachment #373991 - Flags: review?(kinetik)
Attachment #373969 - Flags: superreview?(roc)
Attachment #373969 - Flags: review?(kinetik)
I attached the wrong patch in my last comment, sorry.
Attachment #373991 - Attachment is obsolete: true
Attachment #373994 - Flags: superreview?(roc)
Attachment #373994 - Flags: review?(kinetik)
Attachment #373991 - Flags: superreview?(roc)
Attachment #373991 - Flags: review?(kinetik)
Attachment #373994 - Flags: superreview?(roc) → superreview+
Attachment #373994 - Flags: review?(kinetik) → review+
Comment on attachment 373257 [details] [diff] [review]
fix for liboggplay hang

this will be picked up in the refresh to latest liboggplay git in bug 480063.
Attachment #373257 - Attachment is obsolete: true
Depends on: 480063
Pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/397dee5a493a
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Whiteboard: [baking for 1.9.1]
Flags: wanted1.9.1?
Flags: wanted1.9.1? → wanted1.9.1+
the URL looked smooth to me on Shiretoko and Minefield:

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

and

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1pre) Gecko/20090527 Shiretoko/3.5pre ID:20090527031214
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.