Opened 14 years ago

Closed 14 years ago

Last modified 21 months ago

#2883 closed defect (fixed)

SMUSH: Too dependent on accurate timers

Reported by: eriktorbjorn Owned by: eriktorbjorn
Priority: normal Component: Engine: SCUMM
Keywords: Cc:


The SMUSH player currently uses a timer to trigger the decoding of each frame. This makes it very dependent on accurate timers, not in the sense that each timer call has to be accurate but in that the timer must not be allowed to drift out of sync over time.

The problem is that in The Curse of Monkey Island, the opening cutscene is supposed to run at 12 fps. That means that the timer interval should be approximately 83.33 milliseconds. This was the reason timer accuracy was changed from milliseconds to microseconds in the first place.

In retrospect, perhaps this wasn't such a great idea. The current default timer handler appears to run on millisecond accuracy - it starts by dividing the timer interval by 1000...

It should be fairly simple to remove the dependency on timers from the SMUSH player, though the ability to pause does add some complications compared to the cutscene players in Broken Sword.

Ticket imported from: #1584888. Ticket imported from: bugs/2883.

Attachments (4)

smush.diff (4.5 KB ) - added by eriktorbjorn 14 years ago.
Patch against current SVN
smush2.diff (4.5 KB ) - added by eriktorbjorn 14 years ago.
Updated (and hopefully working) patch.
smush3.diff (7.7 KB ) - added by eriktorbjorn 14 years ago.
Another updated patch
smush4.diff (8.4 KB ) - added by eriktorbjorn 14 years ago.
Another updated patch

Download all attachments as: .zip

Change History (19)

comment:1 by fingolfin, 14 years ago

I changed the timer resolution back, but it's still indeed a bad way to run a movie.

It wouldn't be too hard to do a resync from time to time, though, would it? At least if we knew for each frame at which time it is supposed to play. For a given frame speed and frame number, that could be computed using a simple multiplication. Then, if we are lagging, skip frames until we get to the correct one... or os...

Another complication to keep in mind is the pause mode, any logic / code for this has to deal properly with that.

comment:2 by fingolfin, 14 years ago

Further note: Probably would use Mixer::getSoundElapsedTime(), to sync the video with the sound. Oh, and I am not sure whether we really have to use a timer for the playback, or not, though syncing should be possible no matter whether we use it or not.

comment:3 by eriktorbjorn, 14 years ago

Yes, getSoundElapsedTime() works fine for Broken Sword, Feeble Files and Kyra 3 cutscenes, so it should work fine for SMUSH too. Though at the moment, it doesn't handle pausing, so we're just moving that problem from one place to another. :-)

Aquadran would know more about whether or not a timer is needed, but at least this morning he seemed quite open to the idea of removing it. (See channel logs.)

comment:4 by fingolfin, 14 years ago

handling pause shouldn't be too hard. You only have to record the getTime() value when the pause starts, and then, when it is ended, calculate the difference and use it to adjust the "elapsed" value. So, essentially we need to modify the Channel::paused() method. Really only needs a couple lines of code, I'd think...

by eriktorbjorn, 14 years ago

Attachment: smush.diff added

Patch against current SVN

comment:5 by eriktorbjorn, 14 years ago

Just to get the ball rolling on this, I'm attaching a minimal patch that gets rid of the timer. Thigs to do that I'm aware of.

1) Currently, it syncs only to elapsed time, not elapsed sound. (But it does handle pausing! :-) Syncing to elapsed time may very well be a necessary fallback if there are silent cutscenes, but syncing to elapsed sound is of course preferable.

2) Frame skipping is not implemented. We probably need to decode every frame, but we can skip the actual drawing. As in the Broken Sword games, we should probably force occasional redraws even if we're lagging behind.

3) Cleanups. Some code looks like it's there only to deal with threading issues.

comment:6 by fingolfin, 14 years ago

This patch breaks the COMI intro movie for me (it just doesn't show anything but blackness).

by eriktorbjorn, 14 years ago

Attachment: smush2.diff added

Updated (and hopefully working) patch.

comment:7 by eriktorbjorn, 14 years ago

Oops, I guess I must have made some last-minute clean-up and forgot to test. Try this updated version instead.

by eriktorbjorn, 14 years ago

Attachment: smush3.diff added

Another updated patch

comment:8 by eriktorbjorn, 14 years ago

Another updated patch. Fixed a syntax error (Symbian only - untested), removed a mutex that doesn't look necessary any more, and added experimental frame-skipping. I'm not sure how well that last thing really works.

by eriktorbjorn, 14 years ago

Attachment: smush4.diff added

Another updated patch

comment:9 by eriktorbjorn, 14 years ago

Another updated version of the patch. This time, I've added experimental syncing to elapsed sound time, though only for compressed SMUSH movies, and for CoMI SMUSH movies. For The Dig and Full Throttle, apparently there is no guarantee that we have one channel that plays through the entire thing. That makes syncing to elapsed sound time much harder.

comment:10 by eriktorbjorn, 14 years ago

I wonder if this experimental patch has any effect on bugs #1303367 and #1327891.

comment:11 by fingolfin, 14 years ago

The patch works fine for me. I'd say we should commit it. If we encounter regressions, we can fix 'em as we go...

Unless aquadran has anything to say against this?

comment:12 by fingolfin, 14 years ago

Owner: set to aquadran

comment:13 by fingolfin, 14 years ago

Well, I added the patch to SVN. Further improvements can follow now.

comment:14 by fingolfin, 14 years ago

Owner: changed from aquadran to eriktorbjorn
Resolution: fixed
Status: newclosed

comment:15 by digitall, 21 months ago

Component: --Unset--Engine: SCUMM
Note: See TracTickets for help on using tickets.