#2883 closed defect (fixed)
SMUSH: Too dependent on accurate timers
| Reported by: | eriktorbjorn | Owned by: | eriktorbjorn |
|---|---|---|---|
| Priority: | normal | Component: | Engine: SCUMM |
| Version: | Keywords: | ||
| Cc: | Game: |
Description
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)
Change History (19)
comment:1 by , 17 years ago
comment:2 by , 17 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 , 17 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 , 17 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...
comment:5 by , 17 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 , 17 years ago
This patch breaks the COMI intro movie for me (it just doesn't show anything but blackness).
comment:7 by , 17 years ago
Oops, I guess I must have made some last-minute clean-up and forgot to test. Try this updated version instead.
comment:8 by , 17 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.
comment:9 by , 17 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 , 17 years ago
I wonder if this experimental patch has any effect on bugs #1303367 and #1327891.
comment:11 by , 17 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 , 17 years ago
| Owner: | set to |
|---|
comment:14 by , 17 years ago
| Owner: | changed from to |
|---|---|
| Resolution: | → fixed |
| Status: | new → closed |
comment:15 by , 5 years ago
| Component: | --Unset-- → Engine: SCUMM |
|---|
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.