568431 - Remove the requirement for mCallbackPeriod from the generic media backend

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Matthew Gregan [:kinetik]]

There are places in the generic media backend where we rely on mCallbackPeriod being accurate, for example:

NS_ASSERTION(video->mTime <= seekTime &&
            seekTime <= video->mTime + mReader->GetInfo().mCallbackPeriod,
            "Seek target should lie inside the first frame after seek");

We don't have a valid value for this in the WebM backend as WebM files do not (always) have a fixed framerate.  Each frame has a presentation time and it should be displayed until the next frame's presentation time.

We could cheat and work around this by reading the first two frames from a WebM file and use the difference between those frame presentation times as the callback period (and, thus, framerate) but this won't work for any WebM content that includes variable framerate video.

Another approach to fixing this may be to read an extra frame ahead, and store the start and end times of each frame in the VideoData instance.  The end time of the frame would be the start time of the next frame in the stream.  Then, most of the places that use mCallbackPeriod to compute the frame end time can use the frame end time directly.

Comment 1

[Matthew Gregan [:kinetik]]

mFramerate needs to die too.

Comment 2

[cajbir (:cajbir)]

Also see bug 560127 comment 4 about mFramerate.

Comment 3

[Matthew Gregan [:kinetik]]

     if (HasAudio() && mAudioStartTime == -1 && !mAudioCompleted) {
       // We've got audio (so we should sync off the audio clock), but we've
       // not played a sample on the audio thread, so we can't get a time
       // from the audio clock. Just wait and then return, to give the audio
       // clock time to tick.
       Wait(mReader->GetInfo().mCallbackPeriod);

This code seems strange.  Shouldn't it be waiting on a definite signal from the audio thread rather than sleeping for an arbitrary length of time and then checking again?

Comment 4

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to comment #3)
>      if (HasAudio() && mAudioStartTime == -1 && !mAudioCompleted) {
>        // We've got audio (so we should sync off the audio clock), but we've
>        // not played a sample on the audio thread, so we can't get a time
>        // from the audio clock. Just wait and then return, to give the audio
>        // clock time to tick.
>        Wait(mReader->GetInfo().mCallbackPeriod);
> 
> This code seems strange.  Shouldn't it be waiting on a definite signal from the
> audio thread rather than sleeping for an arbitrary length of time and then
> checking again?

Yeah, that would make sense. I think it would only be safe to wait in that case if in the block in AudioLoop() in which we set mAudioStartTime was merged with the block above it which does the NotifyAll(). Otherwise if an audio file has only one SoundData, all the threads could sleep waiting for each other to do something.

Comment 5

[Matthew Gregan [:kinetik]]

Attachment: patch v0

Proof of concept, not ready for review.  Doesn't address the side issue Chris P and I discussed above.  

I think the proper solution for:

-          PRInt64 videoTime = HasVideo() ? (mVideoFrameTime + mReader->GetInfo().mCallbackPeriod) : 0;
+          PRInt64 videoTime = HasVideo() ? mVideoFrameTime : 0;

Is to turn mVideoFrameTime into mVideoFrameEndTime and store the frame end time instead of frame start time.  The change the one place that relies on it being the start time to use the start time from the current video frame (which the current code has just used to set mVideoFrameTime anyway).

Comment 6

[Matthew Gregan [:kinetik]]

This needs to be rebased on top of Chris D's latest patch in bug 566245.  That'll allow me to get rid of Get{Audio,Video}Packet, as he implemented something similar (but better) as NextPacket.

Comment 7

[Matthew Gregan [:kinetik]]

Attachment: patch v1

Rebased on top of the latest WebM patch.  Uses NextPacket().  Handles the last video frame by treating the video duration as the end time of the last frame (this needs to be raised as a spec issue, I'll do that soon).  If there's no duration in the WebM file, we'll end up treating the second-to-last frame as the last frame.  Not sure what we can do about that, for now.  As far as I can tell, all of the places using mVideoFrameTime wanted the end time anyway, so I changed them all to use the end time.

Comment 8

[Matthew Gregan [:kinetik]]

Attachment: patch v2

Rebased.

Comment 9

[Matthew Gregan [:kinetik]]

Note that this applies on top of bug 568162 and effectively reverts it.

Comment 10

[Chris Pearce [:cpearce (Not reading bugmail)]]

http://hg.mozilla.org/mozilla-central/rev/1a6eec789968