Remove the requirement for mCallbackPeriod from the generic media backend

RESOLVED FIXED

Status

()

RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: kinetik, Assigned: kinetik)

Tracking

Trunk
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

8 years ago
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.
(Assignee)

Updated

8 years ago
Blocks: 568432
(Assignee)

Comment 1

8 years ago
mFramerate needs to die too.

Comment 2

8 years ago
Also see bug 560127 comment 4 about mFramerate.
(Assignee)

Updated

8 years ago
Assignee: nobody → kinetik
Status: NEW → ASSIGNED
(Assignee)

Comment 3

8 years ago
     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.
(Assignee)

Comment 5

8 years ago
Created attachment 447935 [details] [diff] [review]
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).
(Assignee)

Comment 6

8 years ago
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.

Updated

8 years ago
Blocks: 560127
(Assignee)

Comment 7

8 years ago
Created attachment 448327 [details] [diff] [review]
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.
Attachment #447935 - Attachment is obsolete: true
(Assignee)

Comment 8

8 years ago
Created attachment 448330 [details] [diff] [review]
patch v2

Rebased.
Attachment #448327 - Attachment is obsolete: true
Attachment #448330 - Flags: review?(chris.double)
(Assignee)

Comment 9

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

Updated

8 years ago
Attachment #448330 - Flags: review?(chris.double) → review+
(Assignee)

Updated

8 years ago
Keywords: checkin-needed
Whiteboard: [needs landing]
http://hg.mozilla.org/mozilla-central/rev/1a6eec789968
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [needs landing]
(Assignee)

Updated

8 years ago
Depends on: 571208
(Assignee)

Updated

8 years ago
Depends on: 579812
You need to log in before you can comment on or make changes to this bug.