470639 - skip video frames to ensure smooth audio

Status: RESOLVED FIXED

(Core :: Audio/Video, enhancement)

Description

[Biju]

OGG performance has improved a lot!!!

And now a days, in my old favorite computer I can play 
http://lachy.id.au/dev/markup/tests/html5/video/002.html
http://www.double.co.nz/video_test/test4.html
http://www.double.co.nz/video_test/test5.html

But audio has some problem.

This is due to all resource are consumed by VIDEO.
How do I know?
I can do a Ctrl+T (new tab)
And when the video is in the background the AUDIO is clear.

So can We SKIP some VIDEO FRAMES to save resources for the audio

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre)
Gecko/20081220 Minefield/3.2a1pre

Comment 1

[cajbir (:cajbir)]

Attachment: Fix

Matthew, I put you as a reviewer as I know you're looking at improving the framedata stuff.

While fixing bug 466699 I changed things so frames could be skipped. I've taken this patch from there and put it here since this is the bug for it.

When it's time to display a frame I go through the frame queue and look for the latest frame that should play for this time. I play the audio for the skipped frames however.

This will work even better when Matthews experimental yuv2rgb fix mentioned in bug 474748 comment 8 is done. That will mean skipped frames don't have the yuv2rgb routine called which saves a bit of processing time.

All this still will cause audio to skip on some videos (the ghostbusters HD video on tinyvid for example). This is because we skip all the frames in the framequeue but from then on we can only decode one frame at a time since even that takes too long (when combined with the yuv2rgb for that frame and the display of it). Since there is only one frame in the queue we can't skip any. 

For that to be fixed we need a way of telling liboggplay to skip video decoding up until frame 'X'. I'll discuss this with the liboggplay maintainer and see what they think. Improvements to the yuv2rgb code that are coming may also help resolve this.

Comment 2

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

Comment on attachment 367569 [details] [diff] [review]
Fix

       double time = (PR_IntervalToMilliseconds(PR_IntervalNow()-mPlayStartTime-mPauseDuration)/1000.0);
-      if (time >= frame->mTime) {
-        // Audio for the current frame is played, but video for the next frame
-        // is displayed, to account for lag from the time the audio is written
-        // to when it is played. This will go away when we move to a/v sync
-        // using the audio hardware clock.
-        PlayAudio(frame);
+
+      if (mDecodedFrames.IsFull() && time < frame->mTime) {
+        mon.Wait(PR_MillisecondsToInterval(PRInt64((frame->mTime - time)*1000)));
+        if (mState == DECODER_STATE_SHUTDOWN)
+          return;
+        time = (PR_IntervalToMilliseconds(PR_IntervalNow()-mPlayStartTime-mPauseDuration)/1000.0);
+      }

How about
double time;
for (;;) {
  time = (PR_IntervalToMilliseconds(PR_IntervalNow()-mPlayStartTime-mPauseDuration)/1000.0);
  if (mDecodedFrames.IsFull() && time < frame->mTime) {
    mon.Wait(PR_MillisecondsToInterval(PRInt64((frame->mTime - time)*1000)));
    if (mState == DECODER_STATE_SHUTDOWN)
      return;
    continue;
  }
  break;
}

so we don't have to duplicate the code to compute 'time'...

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

Missing space before (

Comment 3

[cajbir (:cajbir)]

Attachment: Address review comments

Addressed review comments from Robert, carried forward sr+

Comment 4

[cajbir (:cajbir)]

Comment on attachment 367673 [details] [diff] [review]
Address review comments

Oops, included a bunch of things from a recent pull.

Comment 5

[cajbir (:cajbir)]

Attachment: Address review comments

The real patch

Comment 6

[Biju]

Chris, when is this going to land?

Comment 7

[cajbir (:cajbir)]

I'm looking into some audio/video sync issues and it'll land once I'm happy this isn't affect that.

Comment 8

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

This is obsolete, right?

Comment 9

[cajbir (:cajbir)]

Fixed in bug 466699.