Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

VIDEO: Fix QuickTimeDecoder bugs #3512

Merged
merged 3 commits into from Nov 12, 2021
Merged

VIDEO: Fix QuickTimeDecoder bugs #3512

merged 3 commits into from Nov 12, 2021

Conversation

@sluicebox
Copy link
Member

@sluicebox sluicebox commented Nov 10, 2021

The KQ6 Mac QuickTime opening movie doesn't play correctly in ScummVM. There are frozen frames and other problems at transitions between tracks and edits. https://bugs.scummvm.org/ticket/11085

There are two bugs preventing correct playback:

  1. A workaround from three years ago for a video in a Spanish version of Riven. The workaround has several problems and it breaks valid movies. (Added in: b8abe40)
  2. QuickTimeDecoder doesn't correctly handle tracks where the first edit is empty (a delay) and the second has a mediaTime offset. mediaTime is ignored in this case, causing the first frame of the media to be played instead of the intended later frame.

I've limited the Riven workaround to just Riven through a flag, this mitigates that regression. It's important to keep that workaround, even if buggy, since it prevents a crash. (Hopefully it can be fixed someday by someone with that video)

The decoder bug is an initialization/workflow problem. All tracks are constructed at the start where they each enter their first edit. Entering an edit calculates the next frame of the edit and next frame start time. But first, enterNewEditList() skips empty edits. That's a problem because the track constructor then resets the first frame, undoing the calculation. The edit is never re-initialized since it's already been entered. In other words, there are accidentally two paths to real edit initialization: the one during playback which correctly handles everything, and the one during track construction where the caller is unaware of skipping past edits or the need to buffer frames in case the first edit has a mediaTime.

The fix is to no longer skip empty edits, which are just delays until the next edit starts. Instead, a track's current edit can now happily be in an empty edit state during an empty edit. This is a more accurate representation of the movie playback model. This causes subsequent edits to now always be initialized during playback just like all other edits. The trade-off is to just add a check to decodeNextFrame() and getRateAdjustedFrameTime() so that they take empty edits into account.

Also we now buffer frames during track initialization, which fixes the case where the first edit in a track has mediaTime; otherwise the frame's keyframe wouldn't get processed.

I'm not going to admit how long I spent working on this, but...

image

sluicebox added 3 commits Nov 11, 2021
QuickTimeDecoder has a workaround for a video in a Spanish version of
Riven, but this workaround breaks valid QuickTime videos such as the
KQ6 Macintosh opening movie. (Bug #11085)

Until the original Riven video bug can be debugged to improve the
workaround, it is now disabled unless an engine enables it.

Workaround added in: b8abe40
QuickTimeDecoder has a bug which causes the mediaTime offset to be
ignored when a track begins with an empty edit and is followed by an
edit with a non-zero mediaTime. This causes the KQ6 Mac opening movie
to start several tracks at unintended frames (they're never supposed to
be displayed) and the intended frames at the end of the edit to never
be displayed. (Bug #11085)
@bluegr bluegr self-assigned this Nov 12, 2021
@bluegr
Copy link
Member

@bluegr bluegr commented Nov 12, 2021

Excellent! This reduces the scope of the QT workaround/hack. Since this is not an applicable workaround for all QT movies, the approach with the quirk flag is the best that can be done for this.

Ideally, this should be applied at the engine level and more specifically, only for that specific QT movie, but that's an extra improvement that can be done in the future.

Thanks for your work! Merging

Loading

@bluegr bluegr merged commit dcd5373 into scummvm:master Nov 12, 2021
8 checks passed
Loading
@sluicebox sluicebox deleted the quirktime branch Nov 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants