Closed Bug 568431 Opened 11 years ago Closed 11 years ago

Remove the requirement for mCallbackPeriod from the generic media backend

Categories

(Core :: Audio/Video, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: kinetik, Assigned: kinetik)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
Blocks: 568432
mFramerate needs to die too.
Also see bug 560127 comment 4 about mFramerate.
Assignee: nobody → kinetik
Status: NEW → ASSIGNED
     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?
(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.
Attached patch patch v0 (obsolete) — Splinter Review
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).
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.
Blocks: 560127
Attached patch patch v1 (obsolete) — Splinter Review
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.
Attachment #447935 - Attachment is obsolete: true
Attached patch patch v2Splinter Review
Rebased.
Attachment #448327 - Attachment is obsolete: true
Attachment #448330 - Flags: review?(chris.double)
Note that this applies on top of bug 568162 and effectively reverts it.
Attachment #448330 - Flags: review?(chris.double) → review+
Keywords: checkin-needed
Whiteboard: [needs landing]
http://hg.mozilla.org/mozilla-central/rev/1a6eec789968
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [needs landing]
Depends on: 571208
Depends on: 579812
You need to log in before you can comment on or make changes to this bug.