Closed Bug 1654091 Opened 2 years ago Closed 2 years ago

ImageComposite's "biasing" machinery doesn't handle framerate drift in a satisfactory way

Categories

(Core :: Audio/Video: Playback, defect)

defect

Tracking

()

RESOLVED FIXED
81 Branch
Tracking Status
firefox81 --- fixed

People

(Reporter: mstange, Assigned: mstange)

References

Details

Attachments

(5 files)

When playing a 60fps video on a 60fps screen, clock drift can put our video frame timestamp biasing code into an awkward situation where we stay in "negative" bias mode for too long before switching to "none" or "positive" bias mode.
Example profile: https://share.firefox.dev/3fOpYSH

What happens here, I think, is that the display refreshes at a slightly faster rate than the video, which is driven by the audio clock. In the beginning, the video frames fall just before the composition time, and at the end, the two clocks have drifted far enough that the video frames fall just after the composition time.

Ideally, what we'd see in this case is the following: For the most part, we should display exactly one video frame per display refresh. Except at one point in the middle, where we "switch modes": At this point we should display the same video frame on two consecutive display refreshes.

  • First, video frames fall squarely between composition times. No bias is needed.
  • After enough drift, the video frames come dangerously close to the composition times. "Negative" bias is needed to consistently place the video frame before the composition time.
  • After more drift, even the biased video frame timestamp doesn't fall before the composition time any more. This is the time to switch modes: We should switch to "positive" bias, and the current video frame should be displayed for two consecutive display frames.
  • From now on, the biased video frames fall just after the composition times. Video frames continue to drift "to the right" compared to composition times.
  • Once there's enough distance, we're back in the initial state (just offset by one frame) and we should switch to "none" bias again, because no bias is needed to pick video frames consistently.

(With WebM videos, the whole thing is a little more complicated to get right because WebM video frame timestamps advance in integer milliseconds, so the offset between the video frame timestamp and the composition time varies by up to 0.67ms even when there's no drift at all.)

What happens instead is that we don't get out of the "negative" bias fast enough; we fluctuate back and forth between "negative" and "none" bias for a while, and drop a number of frames in the process.

The challenge here is the fact that the "mode switch" needs to happen in a frame where we display the same video frame as in the previous composite. At the moment, we do not call UpdateBias for those composites at all! (That's because it would be counter-productive for 30fps videos, see bug 1652181.)

This reproduces fairly consistently for me on https://www.youtube.com/watch?v=KaCQ8SQ6ZHQ but not on other 60 fps videos such as https://www.youtube.com/watch?v=7TmcXYp8xu4 . Maybe the Avengers trailer is actually in a 59.something frame rate rather than 60 fps.

Edit: Yes, indeed, with the full patch set, the Avengers trailer has one frame repeat roughly every 16 seconds. This is in line with what you'd expect from a 59.94 video: 16 seconds * 59.94 fps = 959.04 frames, 16 seconds * 60 fps = 960 frames.
Or, alternatively: 60 fps - 59.94 fps = 0.06 frames per second or 16.67 seconds per 1 frame.

Assignee: nobody → mstange.moz
Status: NEW → ASSIGNED

However, don't reset the bias to NONE in those cases.

Depends on D84282

We still keep the actual biasing amount (BIAS_TIME_MS) at 1.0ms.

This makes it possible to switch from negative bias to positive bias in an
instant, at the frame that is repeated. Here's how:

While the bias is negative, it means that we're allowed to pick frames up to
1.0ms in the future. But we've drifted far enough that the next frame's
timestamp is more than 1.0ms in the future, for example 1.3ms, we cannot pick
it, so we're at a composite that repeats a frame. This is the point where we
want to switch to positive bias. In order to switch to positive bias, the next
frame's timestamp must be within the biasing threshold of the current
composition time. But if that threshold is at 1.0ms, then the 1.3ms falls
outside of that and we keep the negative bias, which is bad.

There would be no harm in keeping the negative bias if the video frames were
going to be consistently further than 1.0ms away from the composition time
from now on, because then even a negative bias would not make them eligible to
be picked. But whenever we have small fluctuations, which is the case when we
actually need the bias for consistent playback, it means that the next frame
might again be within 1.0ms of the composition time, and the negative bias would
cause a frame to be skipped.

One source of such fluctuations is the fact that WebM video frames only have
integer millisecond precision on their timestamps. That means they're off by
up to 0.5ms from their desired timestamp.

So the frame that's 1.3ms in the future could be one that was supposed to be at
composition time + 0.9ms, but happened to be off by 0.4ms from its desired
timestamp due to the integer millisecond restriction. This frame could then be
followed by another frame that also wanted to be at +0.9ms (compared to the next
frame's composition time), but ended up being at +0.6ms. So at that point it is
important to no longer have the negative bias, because otherwise a frame would
be skipped.

Profile before: https://share.firefox.dev/3hnOOtk
Profile after: https://share.firefox.dev/3fIfEf4

Depends on D84284

Pushed by mstange@themasta.com:
https://hg.mozilla.org/integration/autoland/rev/ba8c767fb3eb
Make these two fields private rather than protected. r=nical
https://hg.mozilla.org/integration/autoland/rev/6f453aacc2a1
Pick the frame in the first call to ChooseImageIndex and then don't look at timestamps again for the remainder of the composition. r=nical
https://hg.mozilla.org/integration/autoland/rev/0ea136f89f8f
Update ImageComposite bias even on frames where the video frame doesn't update. r=nical
https://hg.mozilla.org/integration/autoland/rev/e35fce52004b
Add a profiler marker for the current and next video frame timestamp offsets from the composition time. r=nical
https://hg.mozilla.org/integration/autoland/rev/fe04d92f83ca
Widen the biasing threshold to 1.5ms. r=nical
You need to log in before you can comment on or make changes to this bug.