AUDIO: Fix incorrect reading of DK3 ADPCM audio data

Previously, _topNibble was not reset at the beginning of a new
audio block, and the alignment byte at the end of odd blocks was
being read as audio data, which caused audible clicks and
out-of-bounds sample generation. There may have also been read
errors related to the use of continue/break keywords inside of a
macro wrapped with do-while(0).

The introduction of partial block reads in this code when it was
converted from ffmpeg to a ReadStream interface was also confusing
and somewhat inefficient (calling SeekableReadStream::pos
frequently), so this code has been refactored for clarity and to
improve efficiency by reducing the number of virtual calls. Error
detection has also been improved somewhat by ensuring that there
are enough bytes to read a block header, and that the step indexes
in the header are within the valid range.

2 files changed, 51 insertions, 37 deletions

diff --git a/audio/decoders/adpcm.cpp b/audio/decoders/adpcm.cpp
index ffb61a49c0..7f578ddbae 100644
--- a/audio/decoders/adpcm.cpp
+++ b/audio/decoders/adpcm.cpp
@@ -334,67 +334,89 @@ int MS_ADPCMStream::readBuffer(int16 *buffer, const int numSamples) {
 
 #pragma mark -
 
-
-#define DK3_READ_NIBBLE() \
+#define DK3_READ_NIBBLE(channelNo) \
 do { \
 	if (_topNibble) { \
 		_nibble = _lastByte >> 4; \
 		_topNibble = false; \
 	} else { \
-		if (_stream->pos() >= _endpos) \
-			break; \
-		if ((_stream->pos() % _blockAlign) == 0) \
-			continue; \
 		_lastByte = _stream->readByte(); \
 		_nibble = _lastByte & 0xf; \
 		_topNibble = true; \
+		--blockBytesLeft; \
+		--audioBytesLeft; \
 	} \
-} while (0)
-
+	decodeIMA(_nibble, channelNo); \
+} while(0)
 
 int DK3_ADPCMStream::readBuffer(int16 *buffer, const int numSamples) {
+	assert((numSamples % 4) == 0);
+
+	const uint startOffset = _stream->pos() % _blockAlign;
+	uint audioBytesLeft = _endpos - _stream->pos();
+	uint blockBytesLeft;
+	if (startOffset != 0) {
+		blockBytesLeft = _blockAlign - startOffset;
+	} else {
+		blockBytesLeft = 0;
+	}
+
 	int samples = 0;
+	while (samples < numSamples && audioBytesLeft) {
+		if (blockBytesLeft == 0) {
+			blockBytesLeft = MIN(_blockAlign, audioBytesLeft);
+			_topNibble = false;
 
-	assert((numSamples % 4) == 0);
+			if (blockBytesLeft < 16) {
+				warning("Truncated DK3 ADPCM block header");
+				break;
+			}
+
+			_stream->skip(2);
+			const uint16 rate = _stream->readUint16LE();
+			assert(rate == getRate());
+			_stream->skip(6);
 
-	while (samples < numSamples && !_stream->eos() && _stream->pos() < _endpos) {
-		if ((_stream->pos() % _blockAlign) == 0) {
-			_stream->readUint16LE(); // Unknown
-			uint16 rate = _stream->readUint16LE(); // Copy of rate
-			_stream->skip(6); // Unknown
 			// Get predictor for both sum/diff channels
 			_status.ima_ch[0].last = _stream->readSint16LE();
 			_status.ima_ch[1].last = _stream->readSint16LE();
+
 			// Get index for both sum/diff channels
 			_status.ima_ch[0].stepIndex = _stream->readByte();
 			_status.ima_ch[1].stepIndex = _stream->readByte();
+			assert(_status.ima_ch[0].stepIndex < ARRAYSIZE(_imaTable));
+			assert(_status.ima_ch[1].stepIndex < ARRAYSIZE(_imaTable));
 
-			if (_stream->eos())
-				break;
-
-			// Sanity check
-			assert(rate == getRate());
+			blockBytesLeft -= 16;
+			audioBytesLeft -= 16;
 		}
 
-		DK3_READ_NIBBLE();
-		decodeIMA(_nibble, 0);
+		DK3_READ_NIBBLE(0);
+		DK3_READ_NIBBLE(1);
 
-		DK3_READ_NIBBLE();
-		decodeIMA(_nibble, 1);
+		*buffer++ = _status.ima_ch[0].last + _status.ima_ch[1].last;
+		*buffer++ = _status.ima_ch[0].last - _status.ima_ch[1].last;
 
-		buffer[samples++] = _status.ima_ch[0].last + _status.ima_ch[1].last;
-		buffer[samples++] = _status.ima_ch[0].last - _status.ima_ch[1].last;
+		DK3_READ_NIBBLE(0);
 
-		DK3_READ_NIBBLE();
-		decodeIMA(_nibble, 0);
+		*buffer++ = _status.ima_ch[0].last + _status.ima_ch[1].last;
+		*buffer++ = _status.ima_ch[0].last - _status.ima_ch[1].last;
 
-		buffer[samples++] = _status.ima_ch[0].last + _status.ima_ch[1].last;
-		buffer[samples++] = _status.ima_ch[0].last - _status.ima_ch[1].last;
+		samples += 4;
+
+		// if the last sample of a block ends on an odd byte, the encoder adds
+		// an extra alignment byte
+		if (!_topNibble && blockBytesLeft == 1) {
+			_stream->skip(1);
+			--blockBytesLeft;
+			--audioBytesLeft;
+		}
 	}
 
 	return samples;
 }
 
+#undef DK3_READ_NIBBLE
 
 #pragma mark -
 
diff --git a/audio/decoders/adpcm_intern.h b/audio/decoders/adpcm_intern.h
index f4a708c3fc..4b7b3481f4 100644
--- a/audio/decoders/adpcm_intern.h
+++ b/audio/decoders/adpcm_intern.h
@@ -229,20 +229,12 @@ private:
 // Based on FFmpeg's decoder and http://wiki.multimedia.cx/index.php?title=Duck_DK3_IMA_ADPCM
 
 class DK3_ADPCMStream : public Ima_ADPCMStream {
-protected:
-
-	void reset() {
-		Ima_ADPCMStream::reset();
-		_topNibble = false;
-	}
-
 public:
 	DK3_ADPCMStream(Common::SeekableReadStream *stream, DisposeAfterUse::Flag disposeAfterUse, uint32 size, int rate, int channels, uint32 blockAlign)
 		: Ima_ADPCMStream(stream, disposeAfterUse, size, rate, channels, blockAlign) {
 
 		// DK3 only works as a stereo stream
 		assert(channels == 2);
-		_topNibble = false;
 	}
 
 	virtual int readBuffer(int16 *buffer, const int numSamples);