search

Interrupt / systemshock

Shockolate - A minimalist and cross platform System Shock source port.
GNU General Public License v3.0
803 stars 62 forks source link

Stray pixels in intro video #309

Closed icosahedral-dragon closed 4 years ago

icosahedral-dragon commented 4 years ago

When playing the intro video, in the scene where SHODAN "reevaluates her priorities", some pixels are mis-coloured.

Screenshot from 2019-07-31 13-15-14

This happens on my computer (Ubuntu Linux 16.04) with the 32-bit and my experimental 64-bit build, but not with the original DOS CD-ROM version. I'm not able at present to verify whether the same is true with the "enhanced" Steam edition, but the video on the Steam store page looks correct with no bad pixels.

The source code for the video decoder was apparently lost somewhere along the line. The C code for a couple of key functions is clearly decompiled from the machine code. I then updated the decompiled versions to more portable C, apparently preserving whatever bug was there.

It might be worth rewriting the suspect functions entirely. The format is pretty well described in ss-specs.

icosahedral-dragon commented 4 years ago

It turns out that the stray pixels are actually marked transparent in the compressed data, although the frame in question should be an iframe. If I'd been paying attention when I checked ss-specs I'd have known this because it actually mentions this specific case:

In the intro video for instance, SHODAN's gaining of conscience scene has a few pixel left over from the previous scene.

The frame is preceded by the special palette entry that marks it as an iframe, but the game code ignores this and treats the frame as transparent anyway. I don't know why, given this, the DOS version displays the scene correctly. It's possible that the bug was fixed very late in DOS development and that the Mac source was forked from an earlier version of the code: the Mac team didn't bother patching the video code because they used the OS' built-in QuickTime support. (This is speculation on my part, but it would also help explain why the original low-level decode code went missing.)

I also discovered that the dotted red line separating the video from the subtitles isn't supposed to be there. A typo in the reverse-engineered video code causes the bottom 4 lines of the video never to be decoded.

donnierussellii commented 4 years ago

My PR #193 "Bitmap uncompress malloc'ed space too small" attempted to fix this bug. I suspected a buffer was being overrun, so increased its size. It should be safe to undo this; the increase was really absurdly large.

I noticed, though, that the corrupted pixels didn't appear as often after the increased buffer size. I couldn't get the corrupted pixels to appear in any seemingly consistent way.

icosahedral-dragon commented 4 years ago

Thanks @donnierussellii for your comment: it actually turned out to be a buffer underrun rather than an overrun. As for why your patch changed the observed behaviour, the real reason is probably buried in the internals of malloc, but if I were to guess I'd say that the increased buffer size forced the memory block into a different location whose contents were more likely to be zeroed to start with by coincidence. Just as well you didn't use calloc, or we'd never have spotted it!

To be honest, the original BM_PLENTY_SIZE seems absurd to me. I mean, it's pretty obviously a hack to work around a (real or suspected) bug in the decoder, but I'm unsure if there's any actual need for it. I may investigate further. (Although if an extra 180K of memory wasn't an issue back in 1994, it probably isn't today!)

icosahedral-dragon commented 4 years ago

No sign of buffer overruns in current code with either intro or ending video. I'm inclined to revert the buffer to a sensible size but leave the overrun detection code in just in case.

Interrupt commented 4 years ago

Merged this PR in for the fix, so I'm closing this issue.