Opened 15 years ago

Closed 15 years ago

Last modified 15 years ago

#1399 closed defect (fixed)

BS2: Bad audio/video sync in cutscenes

Reported by: eriktorbjorn Owned by: eriktorbjorn
Priority: normal Component: Engine: Sword2
Keywords: Cc:
Game: Broken Sword 2

Description

For some reason, when I run BS2 on my Linux box
cutscene playback is smooth and well synced. However,
when I try it on a Windows box, the playback is jerky
(but well synced).

The jerkiness does not come from dropped frames or
anything like that. The Windows box is much faster than
my Linux box anyway. Instead, it appears to be because
the getSamplesPlayed() method of finding out how much
of the sound has played is simply too coarse in this case.

Each time readBuffer() is called, it requests 11024 new
samples, which means the returned value from
getSamplesPlayed() is increased by 5512. (When I play
MI1CD each call to readBuffer() requests 512 samples. I
don't know why the difference is so great.)

From what I understand, this means that the
approximation for number video frames to play increases
in steps of three.

I can make the playback smoother by having
getSamplesPlayed() return an approximation based on
when readBuffer() was last called. I haven't checked
how that impacts syncing - the playback code makes
assumptions about the returned value being inaccurate -
but that hack is probably too ugly to live anyway...

Ticket imported from: #880484. Ticket imported from: bugs/1399.

Attachments (4)

vorbis.diff (1.0 KB) - added by eriktorbjorn 15 years ago.
Ugly hack against a January 20 CVS snapshot
sword2-animation.diff (3.8 KB) - added by eriktorbjorn 15 years ago.
Patch against a January 21 CVS snapshot
sword2-animation2.diff (3.6 KB) - added by eriktorbjorn 15 years ago.
Slightly cleaned up version of the previous patch
sword-animation.diff (8.0 KB) - added by eriktorbjorn 15 years ago.
Patch against a January 22 CVS snapshot

Download all attachments as: .zip

Change History (25)

Changed 15 years ago by eriktorbjorn

Attachment: vorbis.diff added

Ugly hack against a January 20 CVS snapshot

comment:1 Changed 15 years ago by eriktorbjorn

Owner: set to SF/ender

comment:2 Changed 15 years ago by eriktorbjorn

I'd better point out that the patch/hack is wrong anyway. It
will not notice if the user calls read() instead of
readBuffer() (then again, I thought read() was obsolete),
and the approximation should subtract, not add, since the
most recently added samples haven't yet been played.

It was just what I used to test my theory.

comment:3 Changed 15 years ago by eriktorbjorn

When I run the cutscenes on my Linux box, readBuffer() is
asked for 4096 samples instead of 11024. At this rate, the
jerkiness is not very noticeable.

In the MI1CD case it asked for 512, just as the Windows
version did.

I have absolutely no idea what causes this difference.

comment:4 Changed 15 years ago by SF/ender

Max, I don't really know much about the streaming code - do you
have any ideas on this one?

comment:5 Changed 15 years ago by SF/ender

Owner: changed from SF/ender to fingolfin

comment:6 Changed 15 years ago by fingolfin

Owner: fingolfin deleted

comment:7 Changed 15 years ago by fingolfin

The number of bytes passed to readBuffer essentially depends on
the backend, and on the sampling rate of the data.
First off, if the sampling rate differs from the backend's output
rate, a rate converter is used, which fetches 512 samples at a
time. This is why in one case you see 512 on both systems.
If sample rate and audio rate are equal, a special "copy rate"
converter is used, which reads all the data it needs at once. How
much it reads depends on how much the backend requests from
the mixer, i.e. this is will differe between different OSes.

However, this is IMO *not* a flaw in the mixer. Yes, in the first
case you are seeing read's of 512 bytes, but they are all not
actually being outputed yet - first, it'll read enough 512byte blocks
to fill up the 4096 / 11024 samples requested by the backend; and
only then those are passed to the backend and played back. So the
higher granularity is purely an illusion.

To me this sounds as if the animation sync logic is poor; maybe it
always assumes that at most one frame of audio gets played, and
then doesn't cope properly with the case when multiple frames get
played at once...

comment:8 Changed 15 years ago by eriktorbjorn

Having thought about this some more, the whole
getSamplesPlayed() approach seems pretty hopeless. How about
simply measuring the amount of time since the call to
playInputStream() instead? That should be almost as reliable
(unless the sound starts stuttering, in which case syncing
is the least of our troubles), immensely more fine-grained
and not as susceptible to the whims of the backend and mixer.

Of course, it will break if we ever allow pausing through
cutscenes. So let's not do that, then.

I'm going to see if I can come up with a patch for the BS2
case, and someone else can fix up the BS1 case afterwards.
Someone with access to the re-encoded BS1 cutscene files,
preferably.

Changed 15 years ago by eriktorbjorn

Attachment: sword2-animation.diff added

Patch against a January 21 CVS snapshot

comment:9 Changed 15 years ago by eriktorbjorn

I haven't checked how well this actually works compared to
the old one. That is, I know the playback is smoother under
Windows, naturally, but I haven't really compared how well
the audio is synced.

But it's at least something to experiment with.

comment:10 Changed 15 years ago by eriktorbjorn

This is a slightly cleaned up version of the previous patch.
I've verified that it runs in both 8 and 16-bit mode.
(There's usually an ugly flash in 8-bit mode when the
palette changes, but perhaps there isn't much to be done
about that.)

Changed 15 years ago by eriktorbjorn

Attachment: sword2-animation2.diff added

Slightly cleaned up version of the previous patch

comment:11 Changed 15 years ago by SF/khalek

Component: Engine: Sword2
Game: Broken Sword 2

comment:12 Changed 15 years ago by eriktorbjorn

I had to update the patch anyway to deal with some
whitespace changes, so I figured I might as well update the
BS1 animation code while I was at it.

The BS1 code will need some more work, but that's true
regardless of whether or not this patch is accepted.

Changed 15 years ago by eriktorbjorn

Attachment: sword-animation.diff added

Patch against a January 22 CVS snapshot

comment:13 Changed 15 years ago by eriktorbjorn

Owner: set to SF/ender

comment:14 Changed 15 years ago by eriktorbjorn

Assigning to Ender again, since the proposed patch doesn't
use the sound code for syncing. :-)

comment:15 Changed 15 years ago by eriktorbjorn

Fingolfin suggested adding a AudioStream::getTimeElapsed()
function that would return basically the same information as
getSamplesPlayed(), but with interpolation to make it linear.

Which means we're getting close to my original hack again. :-)

In other words, don't apply the current patch right now.

comment:16 Changed 15 years ago by SF/roever

The problem with this is that the audio device doesn't have to use the
same clock as the system. This wont hurt in small animations as the audio
time and the system time usually differ by only a small amount but in the
long intro I get a lager difference. The difference is quite visible in the
intro, when George knocks on the door and later when he is knocked
down.

I know that the current situation is not nice. Video players use a feature
provided by the sound driver that returns the exact number of samples
sent to the sound card, which is normally exact enough.

But as we don't have this, I'd suggest we add some guesswork to the
getSamplesPlayed() function. Something like time since last read call times
samplerate

comment:17 Changed 15 years ago by eriktorbjorn

I've submitted a patch that adds a getChannelElapsedTime()
function to the mixer that returns an approximation of how
long a sound has been playing. It's not perfect by any
means, and I don't know if it'll be accepted anyway, but if
it is it should be trivial to change the cutscenes to use that.

I've only tried it on my Linux box though, where even the
current method works well.

comment:18 Changed 15 years ago by eriktorbjorn

I've tried it on the Windows box that has trouble with the
current version. Seems to work fine.

comment:19 Changed 15 years ago by eriktorbjorn

Resolution: fixed
Status: newclosed

comment:20 Changed 15 years ago by eriktorbjorn

I've applied the patch, and updated the BS2 cutscene code.
(I'm working on the BS1 code now.) Closing.

comment:21 Changed 15 years ago by eriktorbjorn

Owner: changed from SF/ender to eriktorbjorn
Note: See TracTickets for help on using tickets.