thirdparty/scummvm
2
0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

SCI: Fix SoundResource error handling, ctor, dtor

Browse source 
Fixes several problems with the SoundResource class:

- Constructor doesn't fully initialize object if resource doesn't exist
- Destructor crashes if object not fully initialized
- Constructor has no mechanism to report failure
- Callers believe failure is reported by constructor returning null
- SoundCommandParser::initSoundResource attempts to pre-detect failure
  insufficiently in the absence of a formal mechanism.

SoundResource now always fully initializes, the destructor no longer
accesses uninitialized memory, and an exists() method has been added
which callers now test.

SQ6 Mac can now progress past the main menu.
This commit is contained in:
sluicebox 2020-04-12 01:47:15 -07:00
parent 17611ac289
commit 23fc7f52e0
5 changed files with 43 additions and 40 deletions
Download patch file Download diff file Expand all files Collapse all files

8
engines/sci/console.cpp
View file

@ -2705,24 +2705,22 @@ bool Console::cmdIsSample(int argc, const char **argv) {
return true; return true;
} }
SoundResource *soundRes = new SoundResource(number, _engine->getResMan(), _engine->_features->detectDoSoundType()); SoundResource soundRes(number, _engine->getResMan(), _engine->_features->detectDoSoundType());
if (!soundRes) { if (!soundRes.exists()) {
debugPrintf("Not a sound resource!\n"); debugPrintf("Not a sound resource!\n");
return true; return true;
} }
SoundResource::Track *track = soundRes->getDigitalTrack(); SoundResource::Track *track = soundRes.getDigitalTrack();
if (!track || track->digitalChannelNr == -1) { if (!track || track->digitalChannelNr == -1) {
debugPrintf("Valid song, but not a sample.\n"); debugPrintf("Valid song, but not a sample.\n");
delete soundRes;
return true; return true;
} }
debugPrintf("Sample size: %d, sample rate: %d, channels: %d, digital channel number: %d\n", debugPrintf("Sample size: %d, sample rate: %d, channels: %d, digital channel number: %d\n",
track->digitalSampleSize, track->digitalSampleRate, track->channelCount, track->digitalChannelNr); track->digitalSampleSize, track->digitalSampleRate, track->channelCount, track->digitalChannelNr);
delete soundRes;
return true; return true;
} }

2
engines/sci/engine/features.cpp
View file

@ -718,7 +718,7 @@ bool GameFeatures::generalMidiOnly() {
} }
SoundResource sound(13, g_sci->getResMan(), detectDoSoundType()); SoundResource sound(13, g_sci->getResMan(), detectDoSoundType());
return (sound.getTrackByType(/* AdLib */ 0) == nullptr); return (sound.exists() && sound.getTrackByType(/* AdLib */ 0) == nullptr);
} }
default: default:
break; break;

3
engines/sci/resource.h
View file

@ -684,12 +684,13 @@ public:
int getChannelFilterMask(int hardwareMask, bool wantsRhythm); int getChannelFilterMask(int hardwareMask, bool wantsRhythm);
byte getInitialVoiceCount(byte channel); byte getInitialVoiceCount(byte channel);
byte getSoundPriority() const { return _soundPriority; } byte getSoundPriority() const { return _soundPriority; }
bool exists() const { return _resource != nullptr; }
private: private:
SciVersion _soundVersion; SciVersion _soundVersion;
int _trackCount; int _trackCount;
Track *_tracks; Track *_tracks;
Resource *_innerResource; Resource *_resource;
ResourceManager *_resMan; ResourceManager *_resMan;
byte _soundPriority; byte _soundPriority;
}; };

59
engines/sci/resource_audio.cpp
View file

@ -707,30 +707,25 @@ bool ResourceManager::isGMTrackIncluded() {
Common::List<ResourceId>::iterator itr = resources.begin(); Common::List<ResourceId>::iterator itr = resources.begin();
int firstSongId = itr->getNumber(); int firstSongId = itr->getNumber();
SoundResource *song1 = new SoundResource(firstSongId, this, soundVersion); SoundResource song1(firstSongId, this, soundVersion);
if (!song1) { if (!song1.exists()) {
warning("ResourceManager::isGMTrackIncluded: track 1 not found"); warning("ResourceManager::isGMTrackIncluded: track 1 not found");
return false; return false;
} }
SoundResource::Track *gmTrack = song1->getTrackByType(0x07); SoundResource::Track *gmTrack = song1.getTrackByType(0x07);
if (gmTrack) if (gmTrack)
result = true; result = true;
delete song1;
return result; return result;
} }
SoundResource::SoundResource(uint32 resourceNr, ResourceManager *resMan, SciVersion soundVersion) : _resMan(resMan), _soundVersion(soundVersion) { SoundResource::SoundResource(uint32 resourceNr, ResourceManager *resMan, SciVersion soundVersion) :
Resource *resource = _resMan->findResource(ResourceId(kResourceTypeSound, resourceNr), true); _resMan(resMan), _soundVersion(soundVersion), _trackCount(0), _tracks(nullptr), _soundPriority(0xFF) {
int trackNr, channelNr; _resource = _resMan->findResource(ResourceId(kResourceTypeSound, resourceNr), true);
if (!resource) if (!_resource)
return; return;
_innerResource = resource;
_soundPriority = 0xFF;
Channel *channel, *sampleChannel; Channel *channel, *sampleChannel;
if (_soundVersion <= SCI_VERSION_0_LATE) { if (_soundVersion <= SCI_VERSION_0_LATE) {
@ -741,17 +736,17 @@ SoundResource::SoundResource(uint32 resourceNr, ResourceManager *resMan, SciVers
_tracks->type = 0; // Not used for SCI0 _tracks->type = 0; // Not used for SCI0
_tracks->channelCount = 1; _tracks->channelCount = 1;
// Digital sample data included? -> Add an additional channel // Digital sample data included? -> Add an additional channel
if (resource->getUint8At(0) == 2) if (_resource->getUint8At(0) == 2)
_tracks->channelCount++; _tracks->channelCount++;
// header information that can be passed to the SCI0 sound driver // header information that can be passed to the SCI0 sound driver
_tracks->header = resource->subspan(0, _soundVersion == SCI_VERSION_0_EARLY ? 0x11 : 0x21); _tracks->header = _resource->subspan(0, _soundVersion == SCI_VERSION_0_EARLY ? 0x11 : 0x21);
_tracks->channels = new Channel[_tracks->channelCount]; _tracks->channels = new Channel[_tracks->channelCount];
channel = &_tracks->channels[0]; channel = &_tracks->channels[0];
channel->flags |= 2; // don't remap (SCI0 doesn't have remapping) channel->flags |= 2; // don't remap (SCI0 doesn't have remapping)
if (_soundVersion == SCI_VERSION_0_EARLY) { if (_soundVersion == SCI_VERSION_0_EARLY) {
channel->data = resource->subspan(0x11); channel->data = _resource->subspan(0x11);
} else { } else {
channel->data = resource->subspan(0x21); channel->data = _resource->subspan(0x21);
} }
if (_tracks->channelCount == 2) { if (_tracks->channelCount == 2) {
// Digital sample data included // Digital sample data included
@ -776,7 +771,7 @@ SoundResource::SoundResource(uint32 resourceNr, ResourceManager *resMan, SciVers
sampleChannel->data += 44; // Skip over header sampleChannel->data += 44; // Skip over header
} }
} else if (_soundVersion >= SCI_VERSION_1_EARLY && _soundVersion <= SCI_VERSION_2_1_MIDDLE) { } else if (_soundVersion >= SCI_VERSION_1_EARLY && _soundVersion <= SCI_VERSION_2_1_MIDDLE) {
SciSpan<const byte> data = *resource; SciSpan<const byte> data = *_resource;
// Count # of tracks // Count # of tracks
_trackCount = 0; _trackCount = 0;
while ((*data++) != 0xFF) { while ((*data++) != 0xFF) {
@ -786,11 +781,11 @@ SoundResource::SoundResource(uint32 resourceNr, ResourceManager *resMan, SciVers
++data; ++data;
} }
_tracks = new Track[_trackCount]; _tracks = new Track[_trackCount];
data = *resource; data = *_resource;
byte channelCount; byte channelCount;
for (trackNr = 0; trackNr < _trackCount; trackNr++) { for (int trackNr = 0; trackNr < _trackCount; trackNr++) {
// Track info starts with track type:BYTE // Track info starts with track type:BYTE
// Then the channel information gets appended Unknown:WORD, ChannelOffset:WORD, ChannelSize:WORD // Then the channel information gets appended Unknown:WORD, ChannelOffset:WORD, ChannelSize:WORD
// 0xFF:BYTE as terminator to end that track and begin with another track type // 0xFF:BYTE as terminator to end that track and begin with another track type
@ -813,12 +808,12 @@ SoundResource::SoundResource(uint32 resourceNr, ResourceManager *resMan, SciVers
_tracks[trackNr].digitalSampleStart = 0; _tracks[trackNr].digitalSampleStart = 0;
_tracks[trackNr].digitalSampleEnd = 0; _tracks[trackNr].digitalSampleEnd = 0;
if (_tracks[trackNr].type != 0xF0) { // Digital track marker - not supported currently if (_tracks[trackNr].type != 0xF0) { // Digital track marker - not supported currently
channelNr = 0; int channelNr = 0;
while (channelCount--) { while (channelCount--) {
channel = &_tracks[trackNr].channels[channelNr]; channel = &_tracks[trackNr].channels[channelNr];
const uint16 dataOffset = data.getUint16LEAt(2); const uint16 dataOffset = data.getUint16LEAt(2);
if (dataOffset >= resource->size()) { if (dataOffset >= _resource->size()) {
warning("Invalid offset inside sound resource %d: track %d, channel %d", resourceNr, trackNr, channelNr); warning("Invalid offset inside sound resource %d: track %d, channel %d", resourceNr, trackNr, channelNr);
data += 6; data += 6;
continue; continue;
@ -826,12 +821,12 @@ SoundResource::SoundResource(uint32 resourceNr, ResourceManager *resMan, SciVers
uint16 size = data.getUint16LEAt(4); uint16 size = data.getUint16LEAt(4);
if ((uint32)dataOffset + size > resource->size()) { if ((uint32)dataOffset + size > _resource->size()) {
warning("Invalid size inside sound resource %d: track %d, channel %d", resourceNr, trackNr, channelNr); warning("Invalid size inside sound resource %d: track %d, channel %d", resourceNr, trackNr, channelNr);
size = resource->size() - dataOffset; size = _resource->size() - dataOffset;
} }
channel->data = resource->subspan(dataOffset, size); channel->data = _resource->subspan(dataOffset, size);
channel->curPos = 0; channel->curPos = 0;
channel->number = channel->data[0]; channel->number = channel->data[0];
@ -883,11 +878,15 @@ SoundResource::SoundResource(uint32 resourceNr, ResourceManager *resMan, SciVers
} }
SoundResource::~SoundResource() { SoundResource::~SoundResource() {
for (int trackNr = 0; trackNr < _trackCount; trackNr++) if (_tracks != nullptr) {
delete[] _tracks[trackNr].channels; for (int trackNr = 0; trackNr < _trackCount; trackNr++)
delete[] _tracks; delete[] _tracks[trackNr].channels;
delete[] _tracks;
}
_resMan->unlockResource(_innerResource); if (_resource != nullptr) {
_resMan->unlockResource(_resource);
}
} }
#if 0 #if 0
@ -922,7 +921,7 @@ SoundResource::Track *SoundResource::getDigitalTrack() {
// Gets the filter mask for SCI0 sound resources // Gets the filter mask for SCI0 sound resources
int SoundResource::getChannelFilterMask(int hardwareMask, bool wantsRhythm) { int SoundResource::getChannelFilterMask(int hardwareMask, bool wantsRhythm) {
SciSpan<const byte> data = *_innerResource; SciSpan<const byte> data = *_resource;
int channelMask = 0; int channelMask = 0;
if (_soundVersion > SCI_VERSION_0_LATE) if (_soundVersion > SCI_VERSION_0_LATE)
@ -990,7 +989,7 @@ byte SoundResource::getInitialVoiceCount(byte channel) {
return 0; // TODO return 0; // TODO
// Skip over digital sample flag // Skip over digital sample flag
SciSpan<const byte> data = _innerResource->subspan(1); SciSpan<const byte> data = _resource->subspan(1);
if (_soundVersion == SCI_VERSION_0_EARLY) if (_soundVersion == SCI_VERSION_0_EARLY)
return data[channel] >> 4; return data[channel] >> 4;

11
engines/sci/sound/soundcmd.cpp
View file

@ -91,10 +91,15 @@ int SoundCommandParser::getSoundResourceId(reg_t obj) {
} }
void SoundCommandParser::initSoundResource(MusicEntry *newSound) { void SoundCommandParser::initSoundResource(MusicEntry *newSound) {
if (newSound->resourceId && _resMan->testResource(ResourceId(kResourceTypeSound, newSound->resourceId))) if (newSound->resourceId) {
newSound->soundRes = new SoundResource(newSound->resourceId, _resMan, _soundVersion); newSound->soundRes = new SoundResource(newSound->resourceId, _resMan, _soundVersion);
else if (!newSound->soundRes->exists()) {
newSound->soundRes = 0; delete newSound->soundRes;
newSound->soundRes = nullptr;
}
} else {
newSound->soundRes = nullptr;
}
// In SCI1.1 games, sound effects are started from here. If we can find // In SCI1.1 games, sound effects are started from here. If we can find
// a relevant audio resource, play it, otherwise switch to synthesized // a relevant audio resource, play it, otherwise switch to synthesized