[Scummvm-git-logs] scummvm master -> 832cd25ef1a5cd2dc9cb8062f043fb402dab6ed7

csnover csnover at users.noreply.github.com
Sat Jun 17 21:37:48 CEST 2017


This automated email contains information about 5 new commits which have been
pushed to the 'scummvm' repo located at https://github.com/scummvm/scummvm .

Summary:
7334f33a3d SCI: State SCI version in error if kernel subop detection fails
e69507cc28 SCI32: Implement engine-accurate DPCM overflow behaviour
12afcaec49 SCI32: Support old-format 8-bit DPCM coding for SCI2
0d63d2a7ad VIDEO: Wrap out-of-range VMD audio samples instead of clipping
832cd25ef1 SCI32: Avoid out-of-bounds read of pixel data in kIsOnMe


Commit: 7334f33a3d1fdb172d8987093bd677aeb9382571
    https://github.com/scummvm/scummvm/commit/7334f33a3d1fdb172d8987093bd677aeb9382571
Author: Colin Snover (github.com at zetafleet.com)
Date: 2017-06-17T13:09:27-05:00

Commit Message:
SCI: State SCI version in error if kernel subop detection fails

Changed paths:
    engines/sci/engine/kernel.cpp


diff --git a/engines/sci/engine/kernel.cpp b/engines/sci/engine/kernel.cpp
index 539475c..72c074f 100644
--- a/engines/sci/engine/kernel.cpp
+++ b/engines/sci/engine/kernel.cpp
@@ -656,7 +656,7 @@ void Kernel::mapFunctions() {
 					kernelSubMap++;
 				}
 				if (!subFunctionCount)
-					error("k%s[%x]: no subfunctions found for requested version", kernelName.c_str(), id);
+					error("k%s[%x]: no subfunctions found for requested version %s", kernelName.c_str(), id, getSciVersionDesc(mySubVersion));
 				// Now allocate required memory and go through it again
 				_kernelFuncs[id].subFunctionCount = subFunctionCount;
 				KernelSubFunction *subFunctions = new KernelSubFunction[subFunctionCount];


Commit: e69507cc28ad5de480f12d9116624b7795738ece
    https://github.com/scummvm/scummvm/commit/e69507cc28ad5de480f12d9116624b7795738ece
Author: Colin Snover (github.com at zetafleet.com)
Date: 2017-06-17T13:09:27-05:00

Commit Message:
SCI32: Implement engine-accurate DPCM overflow behaviour

DPCM decompression algorithms in SSCI operate directly on 8- and
16-bit registers, so any sample that ends up being out-of-range
during decompression gets wrapped by the CPU, not clipped.

This does not fix any known problem with AUD files, but there are
some VMDs (e.g. GK2 5280.VMD) which are known to contain OOR
samples. Making this code more accurate should prevent trouble
with any other similar files.

Changed paths:
    engines/sci/sound/decoders/sol.cpp


diff --git a/engines/sci/sound/decoders/sol.cpp b/engines/sci/sound/decoders/sol.cpp
index 121ffe4..4b366ce 100644
--- a/engines/sci/sound/decoders/sol.cpp
+++ b/engines/sci/sound/decoders/sol.cpp
@@ -54,13 +54,21 @@ static const byte tableDPCM8[8] = { 0, 1, 2, 3, 6, 10, 15, 21 };
  * Decompresses one channel of 16-bit DPCM compressed audio.
  */
 static void deDPCM16Channel(int16 *out, int16 &sample, uint8 delta) {
+	int32 nextSample = sample;
 	if (delta & 0x80) {
-		sample -= tableDPCM16[delta & 0x7f];
+		nextSample -= tableDPCM16[delta & 0x7f];
 	} else {
-		sample += tableDPCM16[delta];
+		nextSample += tableDPCM16[delta];
 	}
-	sample = CLIP<int16>(sample, -32768, 32767);
-	*out = sample;
+
+	// Emulating x86 16-bit signed register overflow
+	if (nextSample > 32767) {
+		nextSample -= 65536;
+	} else if (nextSample < -32768) {
+		nextSample += 65536;
+	}
+
+	*out = sample = nextSample;
 }
 
 /**
@@ -101,7 +109,6 @@ static void deDPCM8Nibble(int16 *out, uint8 &sample, uint8 delta) {
 	} else {
 		sample += tableDPCM8[delta & 7];
 	}
-	sample = CLIP<byte>(sample, 0, 255);
 	*out = ((lastSample + sample) << 7) ^ 0x8000;
 }
 


Commit: 12afcaec49caf4dad5af9fc46af607044bd4b82f
    https://github.com/scummvm/scummvm/commit/12afcaec49caf4dad5af9fc46af607044bd4b82f
Author: Colin Snover (github.com at zetafleet.com)
Date: 2017-06-17T13:09:27-05:00

Commit Message:
SCI32: Support old-format 8-bit DPCM coding for SCI2

Changed paths:
    engines/sci/sound/decoders/sol.cpp
    engines/sci/sound/decoders/sol.h


diff --git a/engines/sci/sound/decoders/sol.cpp b/engines/sci/sound/decoders/sol.cpp
index 4b366ce..290898f 100644
--- a/engines/sci/sound/decoders/sol.cpp
+++ b/engines/sci/sound/decoders/sol.cpp
@@ -102,10 +102,11 @@ static void deDPCM16Stereo(int16 *out, Common::ReadStream &audioStream, const ui
  * Decompresses one half of an 8-bit DPCM compressed audio
  * byte.
  */
+template <bool OLD>
 static void deDPCM8Nibble(int16 *out, uint8 &sample, uint8 delta) {
 	const uint8 lastSample = sample;
 	if (delta & 8) {
-		sample -= tableDPCM8[delta & 7];
+		sample -= tableDPCM8[OLD ? (7 - (delta & 7)) : (delta & 7)];
 	} else {
 		sample += tableDPCM8[delta & 7];
 	}
@@ -116,26 +117,27 @@ static void deDPCM8Nibble(int16 *out, uint8 &sample, uint8 delta) {
  * Decompresses 8-bit DPCM compressed audio. Each byte read
  * outputs two samples into the decompression buffer.
  */
+template <bool OLD>
 static void deDPCM8Mono(int16 *out, Common::ReadStream &audioStream, uint32 numBytes, uint8 &sample) {
 	for (uint32 i = 0; i < numBytes; ++i) {
 		const uint8 delta = audioStream.readByte();
-		deDPCM8Nibble(out++, sample, delta >> 4);
-		deDPCM8Nibble(out++, sample, delta & 0xf);
+		deDPCM8Nibble<OLD>(out++, sample, delta >> 4);
+		deDPCM8Nibble<OLD>(out++, sample, delta & 0xf);
 	}
 }
 
 static void deDPCM8Stereo(int16 *out, Common::ReadStream &audioStream, uint32 numBytes, uint8 &sampleL, uint8 &sampleR) {
 	for (uint32 i = 0; i < numBytes; ++i) {
 		const uint8 delta = audioStream.readByte();
-		deDPCM8Nibble(out++, sampleL, delta >> 4);
-		deDPCM8Nibble(out++, sampleR, delta & 0xf);
+		deDPCM8Nibble<false>(out++, sampleL, delta >> 4);
+		deDPCM8Nibble<false>(out++, sampleR, delta & 0xf);
 	}
 }
 
 # pragma mark -
 
-template<bool STEREO, bool S16BIT>
-SOLStream<STEREO, S16BIT>::SOLStream(Common::SeekableReadStream *stream, const DisposeAfterUse::Flag disposeAfterUse, const uint16 sampleRate, const int32 rawDataSize) :
+template<bool STEREO, bool S16BIT, bool OLDDPCM8>
+SOLStream<STEREO, S16BIT, OLDDPCM8>::SOLStream(Common::SeekableReadStream *stream, const DisposeAfterUse::Flag disposeAfterUse, const uint16 sampleRate, const int32 rawDataSize) :
 	_stream(stream, disposeAfterUse),
 	_sampleRate(sampleRate),
 	// SSCI aligns the size of SOL data to 32 bits
@@ -152,8 +154,8 @@ SOLStream<STEREO, S16BIT>::SOLStream(Common::SeekableReadStream *stream, const D
 		_length = Audio::Timestamp((_rawDataSize * compressionRatio * 1000) / (_sampleRate * numChannels * bytesPerSample), 60);
 	}
 
-template <bool STEREO, bool S16BIT>
-bool SOLStream<STEREO, S16BIT>::seek(const Audio::Timestamp &where) {
+template <bool STEREO, bool S16BIT, bool OLDDPCM8>
+bool SOLStream<STEREO, S16BIT, OLDDPCM8>::seek(const Audio::Timestamp &where) {
 	if (where != 0) {
 		// In order to seek in compressed SOL files, all
 		// previous bytes must be known since it uses
@@ -172,13 +174,13 @@ bool SOLStream<STEREO, S16BIT>::seek(const Audio::Timestamp &where) {
 	return _stream->seek(0, SEEK_SET);
 }
 
-template <bool STEREO, bool S16BIT>
-Audio::Timestamp SOLStream<STEREO, S16BIT>::getLength() const {
+template <bool STEREO, bool S16BIT, bool OLDDPCM8>
+Audio::Timestamp SOLStream<STEREO, S16BIT, OLDDPCM8>::getLength() const {
 	return _length;
 }
 
-template <bool STEREO, bool S16BIT>
-int SOLStream<STEREO, S16BIT>::readBuffer(int16 *buffer, const int numSamples) {
+template <bool STEREO, bool S16BIT, bool OLDDPCM8>
+int SOLStream<STEREO, S16BIT, OLDDPCM8>::readBuffer(int16 *buffer, const int numSamples) {
 	// Reading an odd number of 8-bit samples will result in a loss of samples
 	// since one byte represents two samples and we do not store the second
 	// nibble in this case; it should never happen in reality
@@ -200,8 +202,10 @@ int SOLStream<STEREO, S16BIT>::readBuffer(int16 *buffer, const int numSamples) {
 	} else {
 		if (STEREO) {
 			deDPCM8Stereo(buffer, *_stream, bytesToRead, _dpcmCarry8.l, _dpcmCarry8.r);
+		} else if (OLDDPCM8) {
+			deDPCM8Mono<true>(buffer, *_stream, bytesToRead, _dpcmCarry8.l);
 		} else {
-			deDPCM8Mono(buffer, *_stream, bytesToRead, _dpcmCarry8.l);
+			deDPCM8Mono<false>(buffer, *_stream, bytesToRead, _dpcmCarry8.l);
 		}
 	}
 
@@ -209,23 +213,23 @@ int SOLStream<STEREO, S16BIT>::readBuffer(int16 *buffer, const int numSamples) {
 	return samplesRead;
 }
 
-template <bool STEREO, bool S16BIT>
-bool SOLStream<STEREO, S16BIT>::isStereo() const {
+template <bool STEREO, bool S16BIT, bool OLDDPCM8>
+bool SOLStream<STEREO, S16BIT, OLDDPCM8>::isStereo() const {
 	return STEREO;
 }
 
-template <bool STEREO, bool S16BIT>
-int SOLStream<STEREO, S16BIT>::getRate() const {
+template <bool STEREO, bool S16BIT, bool OLDDPCM8>
+int SOLStream<STEREO, S16BIT, OLDDPCM8>::getRate() const {
 	return _sampleRate;
 }
 
-template <bool STEREO, bool S16BIT>
-bool SOLStream<STEREO, S16BIT>::endOfData() const {
+template <bool STEREO, bool S16BIT, bool OLDDPCM8>
+bool SOLStream<STEREO, S16BIT, OLDDPCM8>::endOfData() const {
 	return _stream->eos() || _stream->pos() >= _rawDataSize;
 }
 
-template <bool STEREO, bool S16BIT>
-bool SOLStream<STEREO, S16BIT>::rewind() {
+template <bool STEREO, bool S16BIT, bool OLDDPCM8>
+bool SOLStream<STEREO, S16BIT, OLDDPCM8>::rewind() {
 	return seek(0);
 }
 
@@ -243,7 +247,7 @@ Audio::SeekableAudioStream *makeSOLStream(Common::SeekableReadStream *stream, Di
 		return nullptr;
 	}
 
-	const uint8 headerSize = header[1] + /* resource header */ 2;
+	const uint8 headerSize = header[1] + kResourceHeaderSize;
 	const uint16 sampleRate = stream->readUint16LE();
 	const byte flags = stream->readByte();
 	const uint32 dataSize = stream->readUint32LE();
@@ -252,13 +256,21 @@ Audio::SeekableAudioStream *makeSOLStream(Common::SeekableReadStream *stream, Di
 
 	if (flags & kCompressed) {
 		if (flags & kStereo && flags & k16Bit) {
-			return new SOLStream<true, true>(new Common::SeekableSubReadStream(stream, initialPosition, initialPosition + dataSize, disposeAfterUse), disposeAfterUse, sampleRate, dataSize);
+			return new SOLStream<true, true, false>(new Common::SeekableSubReadStream(stream, initialPosition, initialPosition + dataSize, disposeAfterUse), disposeAfterUse, sampleRate, dataSize);
 		} else if (flags & kStereo) {
-			return new SOLStream<true, false>(new Common::SeekableSubReadStream(stream, initialPosition, initialPosition + dataSize, disposeAfterUse), disposeAfterUse, sampleRate, dataSize);
+			if (getSciVersion() < SCI_VERSION_2_1_EARLY) {
+				error("SCI2 and earlier did not support stereo SOL audio");
+			}
+
+			return new SOLStream<true, false, false>(new Common::SeekableSubReadStream(stream, initialPosition, initialPosition + dataSize, disposeAfterUse), disposeAfterUse, sampleRate, dataSize);
 		} else if (flags & k16Bit) {
-			return new SOLStream<false, true>(new Common::SeekableSubReadStream(stream, initialPosition, initialPosition + dataSize, disposeAfterUse), disposeAfterUse, sampleRate, dataSize);
+			return new SOLStream<false, true, false>(new Common::SeekableSubReadStream(stream, initialPosition, initialPosition + dataSize, disposeAfterUse), disposeAfterUse, sampleRate, dataSize);
 		} else {
-			return new SOLStream<false, false>(new Common::SeekableSubReadStream(stream, initialPosition, initialPosition + dataSize, disposeAfterUse), disposeAfterUse, sampleRate, dataSize);
+			if (getSciVersion() < SCI_VERSION_2_1_EARLY) {
+				return new SOLStream<false, false, true>(new Common::SeekableSubReadStream(stream, initialPosition, initialPosition + dataSize, disposeAfterUse), disposeAfterUse, sampleRate, dataSize);
+			} else {
+				return new SOLStream<false, false, false>(new Common::SeekableSubReadStream(stream, initialPosition, initialPosition + dataSize, disposeAfterUse), disposeAfterUse, sampleRate, dataSize);
+			}
 		}
 	}
 
diff --git a/engines/sci/sound/decoders/sol.h b/engines/sci/sound/decoders/sol.h
index 80a2181..1141132 100644
--- a/engines/sci/sound/decoders/sol.h
+++ b/engines/sci/sound/decoders/sol.h
@@ -33,7 +33,7 @@ enum SOLFlags {
 	kStereo     = 16
 };
 
-template <bool STEREO, bool S16BIT>
+template <bool STEREO, bool S16BIT, bool OLDDPCM8>
 class SOLStream : public Audio::SeekableAudioStream {
 private:
 	/**


Commit: 0d63d2a7adbe4381d945228a9da4471c9409a8f1
    https://github.com/scummvm/scummvm/commit/0d63d2a7adbe4381d945228a9da4471c9409a8f1
Author: Colin Snover (github.com at zetafleet.com)
Date: 2017-06-17T13:09:27-05:00

Commit Message:
VIDEO: Wrap out-of-range VMD audio samples instead of clipping

The 16-bit DPCM decompressors in SSCI and Urban Runner use a 16-bit
register to store sample data, without any special handling of
overflow. As such, out-of-range samples simply wrap around, rather
than getting clipped.

It is not totally clear if the wrapping behaviour was intentionally
exploited to handle extreme transients, but in any case, videos
like GK2 5280.VMD that generate samples outside the signed 16-bit
range cause a loud pop when using clipping, but play back correctly
when wrapping.

Changed paths:
    video/coktel_decoder.cpp


diff --git a/video/coktel_decoder.cpp b/video/coktel_decoder.cpp
index 905ffd6..daebbd3 100644
--- a/video/coktel_decoder.cpp
+++ b/video/coktel_decoder.cpp
@@ -2617,7 +2617,14 @@ int DPCMStream::readBuffer(int16 *buffer, const int numSamples) {
 			else
 				_buffer[i] += tableDPCM[data];
 
-			*buffer++ = _buffer[i] = CLIP<int32>(_buffer[i], -32768, 32767);
+			// Emulating x86 16-bit signed register overflow
+			if (_buffer[i] > 32767) {
+				_buffer[i] -= 65536;
+			} else if (_buffer[i] < -32768) {
+				_buffer[i] += 65536;
+			}
+
+			*buffer++ = _buffer[i];
 		}
 
 		samples += _channels;


Commit: 832cd25ef1a5cd2dc9cb8062f043fb402dab6ed7
    https://github.com/scummvm/scummvm/commit/832cd25ef1a5cd2dc9cb8062f043fb402dab6ed7
Author: Colin Snover (github.com at zetafleet.com)
Date: 2017-06-17T14:35:42-05:00

Commit Message:
SCI32: Avoid out-of-bounds read of pixel data in kIsOnMe

Fixes Trac#9761, Trac#9844, Trac#9850, Trac#9851.

Changed paths:
    engines/sci/graphics/frameout.cpp


diff --git a/engines/sci/graphics/frameout.cpp b/engines/sci/graphics/frameout.cpp
index b4c842d..c969f91 100644
--- a/engines/sci/graphics/frameout.cpp
+++ b/engines/sci/graphics/frameout.cpp
@@ -1229,6 +1229,24 @@ bool GfxFrameout::isOnMe(const ScreenItem &screenItem, const Plane &plane, const
 			scaledPosition.y = scaledPosition.y * 128 / screenItem._scale.y;
 		}
 
+		// TODO/HACK: When clicking at the very bottom edge of a scaled cel, it
+		// is possible that the calculated `scaledPosition` ends up one pixel
+		// outside of the bounds of the cel. It is not known yet whether this is
+		// a bug that also existed in SSCI (and so garbage memory would be read
+		// there), or if there is actually an error in our scaling of
+		// `ScreenItem::_screenRect` and/or `scaledPosition`. For now, just do
+		// an extra bounds check and return so games don't crash when a user
+		// clicks an unlucky point. Later, double-check the disassembly and
+		// either confirm this is a suitable fix (because SSCI just read bad
+		// memory) or fix the actual broken thing and remove this workaround.
+		if (scaledPosition.x < 0 ||
+			scaledPosition.y < 0 ||
+			scaledPosition.x >= celObj._width ||
+			scaledPosition.y >= celObj._height) {
+
+			return false;
+		}
+
 		uint8 pixel = celObj.readPixel(scaledPosition.x, scaledPosition.y, mirrorX);
 		return pixel != celObj._skipColor;
 	}





More information about the Scummvm-git-logs mailing list