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.

The patch to improve playback requires this liboggplay patch from trac ticket 466 to prevent an infinite loop.

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.

+ // 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);

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.

+ // 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.

Revert the buffering stragegy back to that of the previous patch and fix comment.

I attached the wrong patch in my last comment, sorry.

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.

Pushed to mozilla-central: http://hg.mozilla.org/mozilla-central/rev/397dee5a493a

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