[Scummvm-git-logs] scummvm master -> 505e9aff1596321ae86a7943a87dc0ad7063ab43
bluegr
bluegr at gmail.com
Sat Mar 13 15:41:58 UTC 2021
This automated email contains information about 3 new commits which have been
pushed to the 'scummvm' repo located at https://github.com/scummvm/scummvm .
Summary:
b67c2d72d6 SCI: (SCI0 sound) - make calls to initTrack consistent with our thread handling
5a626c8b78 SCI: (SCI0 sound) - fix sound restoring
505e9aff15 SCI: (SCI0 sound) - ensure that pauseAll() works correctly
Commit: b67c2d72d6c40fda2d67cfaf6a8aab9f44f7cd1e
https://github.com/scummvm/scummvm/commit/b67c2d72d6c40fda2d67cfaf6a8aab9f44f7cd1e
Author: athrxx (athrxx at scummvm.org)
Date: 2021-03-13T17:41:54+02:00
Commit Message:
SCI: (SCI0 sound) - make calls to initTrack consistent with our thread handling
When _mainThreadCalled is set the function call should be enqueued just like the Midi messages that get sent before the start of a new track, so that everything happens in the right order.
Changed paths:
engines/sci/sound/midiparser_sci.cpp
engines/sci/sound/music.cpp
engines/sci/sound/music.h
diff --git a/engines/sci/sound/midiparser_sci.cpp b/engines/sci/sound/midiparser_sci.cpp
index 5c5d9bf168..a88e4eac7c 100644
--- a/engines/sci/sound/midiparser_sci.cpp
+++ b/engines/sci/sound/midiparser_sci.cpp
@@ -399,12 +399,16 @@ void MidiParser_SCI::resetStateTracking() {
}
void MidiParser_SCI::initTrack() {
- if (_soundVersion > SCI_VERSION_0_LATE)
+ if (_soundVersion > SCI_VERSION_0_LATE || !_pSnd || !_track || !_track->header.byteSize())
return;
// Send header data to SCI0 sound drivers. The driver function which parses the header (opcode 3)
// seems to be implemented at least in all SCI0_LATE drivers. The things that the individual drivers
// do in that init function varies.
- if (_track->header.byteSize())
+ // Unlike the original (which doesn't need that due to the way it is implemented) we need to have a
+ // thread safe way to call this to avoid glitches (like permanently hanging notes in some situations).
+ if (_mainThreadCalled)
+ _music->putTrackInitCommandInQueue(_pSnd);
+ else
static_cast<MidiPlayer*>(_driver)->initTrack(_track->header);
}
@@ -440,8 +444,13 @@ void MidiParser_SCI::unloadMusic() {
if (_pSnd) {
resetTracking();
allNotesOff();
+ // Pending track init commands have to be removed from the queue,
+ // since the sound thread will otherwise continue to try executing these.
+ _music->removeTrackInitCommandsFromQueue(_pSnd);
}
_numTracks = 0;
+ _pSnd = 0;
+ _track = 0;
_activeTrack = 255;
_resetOnPause = false;
_mixedData.clear();
diff --git a/engines/sci/sound/music.cpp b/engines/sci/sound/music.cpp
index b8ad9daef1..1274e9815d 100644
--- a/engines/sci/sound/music.cpp
+++ b/engines/sci/sound/music.cpp
@@ -134,7 +134,7 @@ void SciMusic::init() {
case MT_TOWNS:
_pMidiDrv = MidiPlayer_FMTowns_create(_soundVersion);
break;
- case MT_PC98:
+ case MT_PC98:
_pMidiDrv = MidiPlayer_PC9801_create(_soundVersion);
break;
default:
@@ -214,7 +214,16 @@ void SciMusic::putMidiCommandInQueue(byte status, byte firstOp, byte secondOp) {
}
void SciMusic::putMidiCommandInQueue(uint32 midi) {
- _queuedCommands.push_back(midi);
+ _queuedCommands.push_back(MidiCommand(MidiCommand::kTypeMidiMessage, midi));
+}
+
+void SciMusic::putTrackInitCommandInQueue(MusicEntry *psnd) {
+ _queuedCommands.push_back(MidiCommand(MidiCommand::kTypeTrackInit, psnd));
+}
+
+void SciMusic::removeTrackInitCommandsFromQueue(MusicEntry *psnd) {
+ for (MidiCommandQueue::iterator i = _queuedCommands.begin(); i != _queuedCommands.end(); )
+ i = (i->_type == MidiCommand::kTypeTrackInit && i->_dataPtr == (void*)psnd) ? _queuedCommands.erase(i) : i + 1;
}
// This sends the stored commands from queue to driver (is supposed to get
@@ -226,7 +235,15 @@ void SciMusic::sendMidiCommandsFromQueue() {
uint commandCount = _queuedCommands.size();
while (curCommand < commandCount) {
- _pMidiDrv->send(_queuedCommands[curCommand]);
+ if (_queuedCommands[curCommand]._type == MidiCommand::kTypeTrackInit) {
+ if (_queuedCommands[curCommand]._dataPtr) {
+ MusicList::iterator psnd = Common::find(_playList.begin(), _playList.end(), static_cast<MusicEntry*>(_queuedCommands[curCommand]._dataPtr));
+ if (psnd != _playList.end() && (*psnd)->pMidiParser)
+ (*psnd)->pMidiParser->initTrack();
+ }
+ } else {
+ _pMidiDrv->send(_queuedCommands[curCommand]._dataVal);
+ }
curCommand++;
}
_queuedCommands.clear();
@@ -592,7 +609,11 @@ void SciMusic::soundPlay(MusicEntry *pSnd, bool restoring) {
// It is also safe to do this for paused tracks, since the jumpToTick() command further down will parse through
// the song from the beginning up to the resume position and ensure that the actual current voice mapping,
// instrument and volume settings etc. are correct.
- pSnd->pMidiParser->initTrack();
+ // First glance at disasm might suggest that it has to be called only once per sound. But the truth is that
+ // when calling the sound driver opcode for sound restoring (opcode no. 9, we don't have that) it will
+ // internally also call initTrack(). And it wouldn't make sense otherwise, since without that the channel setup
+ // from the last sound would still be active.
+ pSnd->pMidiParser->initTrack();
if (pSnd->status != kSoundPaused)
pSnd->pMidiParser->sendInitCommands();
diff --git a/engines/sci/sound/music.h b/engines/sci/sound/music.h
index 18c2478e6b..f1533e7ebd 100644
--- a/engines/sci/sound/music.h
+++ b/engines/sci/sound/music.h
@@ -160,8 +160,21 @@ struct ChannelRemapping {
int lowestPrio() const;
};
+struct MidiCommand {
+ enum CmdType {
+ kTypeMidiMessage = 0,
+ kTypeTrackInit
+ };
+ // Keeping this very simple, due to the very limited purpose of it.
+ MidiCommand(CmdType type, uint32 val) : _type(type), _dataPtr(0), _dataVal(val) {}
+ MidiCommand(CmdType type, void *ptr) : _type(type), _dataPtr(ptr), _dataVal(0) {}
+ CmdType _type;
+ void *_dataPtr;
+ uint32 _dataVal;
+};
+
typedef Common::Array<MusicEntry *> MusicList;
-typedef Common::Array<uint32> MidiCommandQueue;
+typedef Common::Array<MidiCommand> MidiCommandQueue;
class SciMusic : public Common::Serializable {
@@ -174,6 +187,8 @@ public:
void onTimer();
void putMidiCommandInQueue(byte status, byte firstOp, byte secondOp);
void putMidiCommandInQueue(uint32 midi);
+ void putTrackInitCommandInQueue(MusicEntry *psnd);
+ void removeTrackInitCommandsFromQueue(MusicEntry *psnd);
private:
static void miditimerCallback(void *p);
void sendMidiCommandsFromQueue();
Commit: 5a626c8b784c1b7d49ce26f7f7dd22fff4f009da
https://github.com/scummvm/scummvm/commit/5a626c8b784c1b7d49ce26f7f7dd22fff4f009da
Author: athrxx (athrxx at scummvm.org)
Date: 2021-03-13T17:41:54+02:00
Commit Message:
SCI: (SCI0 sound) - fix sound restoring
This mainly concerns restoring sounds after loading savefiles, but it should make the whole relationship between playing and paused sounds more accurate.
The test case which I was told about was KQ4, room 21, picking up the golden ball under the bridge, saving during playback of the pickup sound and then loading that savegame. It would result in hanging note due toe the sound being triggered multiple times by reconstructPlaylist() and updateSci0Cues(). Now, the sound should only start once.
I've changed the code to be more in line with disasm and tested some situations that sluicebox told me about or that I found in the comments (ICEMAN room 14, LSL3 start scene). I got rid of isQueued, since the original doesn't have that, it has caused some confusion and doesn't even get saved with the savegames.
I cleaned up updateSci0Cues(), so that it (together with processUpdateCues()) does a bit more what the original Midi timer proc does there. An exception is the sound fade out code in processUpdateCues(). It seems that we need that, as we don't have the fading code in the drivers like the original.
The original SCI0 code is actually much simpler than our code. It relies on a correctly sorted playlist (based on priority), but my impression is that we got that right, even if we do it slightly differently. I added a sortPlayList() to the sound init, since the original inserts the node at the right position, too.
Changed paths:
engines/sci/engine/savegame.cpp
engines/sci/sound/midiparser_sci.cpp
engines/sci/sound/music.cpp
engines/sci/sound/music.h
engines/sci/sound/soundcmd.cpp
diff --git a/engines/sci/engine/savegame.cpp b/engines/sci/engine/savegame.cpp
index 070efdbd9a..aaabeb99c8 100644
--- a/engines/sci/engine/savegame.cpp
+++ b/engines/sci/engine/savegame.cpp
@@ -755,6 +755,17 @@ void SoundCommandParser::reconstructPlayList() {
processPlaySound(entry->soundObj, entry->playBed, true);
}
}
+
+ // Emulate the original SCI0 behavior: If no sound with status kSoundPlaying was found we
+ // look for the first sound with status kSoundPaused and start that. It relies on a correctly
+ // sorted playlist, but we have that...
+ if (_soundVersion <= SCI_VERSION_0_LATE && !_music->getFirstSlotWithStatus(kSoundPlaying)) {
+ if (MusicEntry *pSnd = _music->getFirstSlotWithStatus(kSoundPaused)) {
+ writeSelectorValue(_segMan, pSnd->soundObj, SELECTOR(loop), pSnd->loop);
+ writeSelectorValue(_segMan, pSnd->soundObj, SELECTOR(priority), pSnd->priority);
+ processPlaySound(pSnd->soundObj, pSnd->playBed, true);
+ }
+ }
}
#ifdef ENABLE_SCI32
diff --git a/engines/sci/sound/midiparser_sci.cpp b/engines/sci/sound/midiparser_sci.cpp
index a88e4eac7c..85e22aacd8 100644
--- a/engines/sci/sound/midiparser_sci.cpp
+++ b/engines/sci/sound/midiparser_sci.cpp
@@ -840,7 +840,6 @@ bool MidiParser_SCI::processEvent(const EventInfo &info, bool fireEvents) {
return true;
} else {
- _pSnd->status = kSoundStopped;
_pSnd->setSignal(SIGNAL_OFFSET);
debugC(4, kDebugLevelSound, "signal EOT");
diff --git a/engines/sci/sound/music.cpp b/engines/sci/sound/music.cpp
index 1274e9815d..a263852359 100644
--- a/engines/sci/sound/music.cpp
+++ b/engines/sci/sound/music.cpp
@@ -315,22 +315,12 @@ MusicEntry *SciMusic::getSlot(reg_t obj) {
return NULL;
}
-// We return the currently active music slot for SCI0
-MusicEntry *SciMusic::getActiveSci0MusicSlot() {
- const MusicList::iterator end = _playList.end();
- MusicEntry *highestPrioritySlot = NULL;
- for (MusicList::iterator i = _playList.begin(); i != end; ++i) {
- MusicEntry *playSlot = *i;
- if (playSlot->pMidiParser) {
- if (playSlot->status == kSoundPlaying)
- return playSlot;
- if (playSlot->status == kSoundPaused) {
- if ((!highestPrioritySlot) || (highestPrioritySlot->priority < playSlot->priority))
- highestPrioritySlot = playSlot;
- }
- }
+MusicEntry *SciMusic::getFirstSlotWithStatus(SoundStatus status) {
+ for (MusicList::iterator i = _playList.begin(); i != _playList.end(); ++i) {
+ if ((*i)->status == status)
+ return *i;
}
- return highestPrioritySlot;
+ return 0;
}
void SciMusic::setGlobalReverb(int8 reverb) {
@@ -542,18 +532,18 @@ void SciMusic::soundPlay(MusicEntry *pSnd, bool restoring) {
_mutex.unlock(); // unlock to perform mixer-related calls
if (pSnd->pMidiParser) {
- if ((_soundVersion <= SCI_VERSION_0_LATE) && (alreadyPlaying)) {
+ // Original SCI0 doesn't use this function to restore sound. The function it has
+ // for that will not check priorities.
+ if ((_soundVersion <= SCI_VERSION_0_LATE) && alreadyPlaying && !restoring) {
// Music already playing in SCI0?
if (pSnd->priority > alreadyPlaying->priority) {
// And new priority higher? pause previous music and play new one immediately.
// Example of such case: lsl3, when getting points (jingle is played then)
soundPause(alreadyPlaying);
- alreadyPlaying->isQueued = true;
} else {
// And new priority equal or lower? queue up music and play it afterwards done by
// SoundCommandParser::updateSci0Cues()
// Example of such case: iceman room 14
- pSnd->isQueued = true;
pSnd->status = kSoundPaused;
return;
}
@@ -654,8 +644,7 @@ void SciMusic::soundPlay(MusicEntry *pSnd, bool restoring) {
void SciMusic::soundStop(MusicEntry *pSnd) {
SoundStatus previousStatus = pSnd->status;
pSnd->status = kSoundStopped;
- if (_soundVersion <= SCI_VERSION_0_LATE)
- pSnd->isQueued = false;
+
if (pSnd->isSample) {
#ifdef ENABLE_SCI32
if (_soundVersion >= SCI_VERSION_2) {
@@ -682,6 +671,10 @@ void SciMusic::soundStop(MusicEntry *pSnd) {
}
pSnd->fadeStep = 0; // end fading, if fading was in progress
+
+ // SSCI0 resumes the next available sound from the (priority ordered) list with a paused status.
+ if (_soundVersion <= SCI_VERSION_0_LATE && (pSnd = getFirstSlotWithStatus(kSoundPaused)))
+ soundResume(pSnd);
}
void SciMusic::soundSetVolume(MusicEntry *pSnd, byte volume) {
@@ -800,7 +793,7 @@ void SciMusic::soundResume(MusicEntry *pSnd) {
_pMixer->pauseHandle(pSnd->hCurrentAud, false);
pSnd->status = kSoundPlaying;
} else {
- soundPlay(pSnd);
+ soundPlay(pSnd, true);
}
}
@@ -931,8 +924,6 @@ MusicEntry::MusicEntry() {
soundRes = 0;
resourceId = 0;
- isQueued = false;
-
dataInc = 0;
ticker = 0;
signal = 0;
@@ -983,7 +974,7 @@ void MusicEntry::onTimer() {
}
}
- if (status != kSoundPlaying)
+ if (status != kSoundPlaying || !loop)
return;
// Fade MIDI and digital sound effects
diff --git a/engines/sci/sound/music.h b/engines/sci/sound/music.h
index f1533e7ebd..5fd596b8cd 100644
--- a/engines/sci/sound/music.h
+++ b/engines/sci/sound/music.h
@@ -84,8 +84,6 @@ public:
int time; // "tim"estamp to indicate in which order songs have been added
- bool isQueued; // for SCI0 only!
-
uint16 dataInc;
uint16 ticker;
uint16 signal;
@@ -229,11 +227,13 @@ public:
}
MusicEntry *getSlot(reg_t obj);
- MusicEntry *getActiveSci0MusicSlot();
+ MusicEntry *getFirstSlotWithStatus(SoundStatus status);
void pushBackSlot(MusicEntry *slotEntry) {
Common::StackLock lock(_mutex);
_playList.push_back(slotEntry);
+ if (_soundVersion <= SCI_VERSION_0_LATE) // I limit this to SCI0, since it always inserts the nodes at the correct position, but no idea about >=SCI1
+ sortPlayList();
}
void printPlayList(Console *con);
diff --git a/engines/sci/sound/soundcmd.cpp b/engines/sci/sound/soundcmd.cpp
index 34901c3690..237ee826dc 100644
--- a/engines/sci/sound/soundcmd.cpp
+++ b/engines/sci/sound/soundcmd.cpp
@@ -306,6 +306,9 @@ void SoundCommandParser::processStopSound(reg_t obj, bool sampleFinishedPlaying)
musicSlot->dataInc = 0;
musicSlot->signal = SIGNAL_OFFSET;
_music->soundStop(musicSlot);
+
+ if (_soundVersion <= SCI_VERSION_0_LATE && (musicSlot = _music->getFirstSlotWithStatus(kSoundPlaying)))
+ writeSelectorValue(_segMan, musicSlot->soundObj, SELECTOR(state), kSoundPlaying);
}
reg_t SoundCommandParser::kDoSoundPause(EngineState *s, int argc, reg_t *argv) {
@@ -318,16 +321,16 @@ reg_t SoundCommandParser::kDoSoundPause(EngineState *s, int argc, reg_t *argv) {
// SCI0 games give us 0/1 for either resuming or pausing the current music
// this one doesn't count, so pausing 2 times and resuming once means here that we are supposed to resume
uint16 value = argv[0].toUint16();
- MusicEntry *musicSlot = _music->getActiveSci0MusicSlot();
+ MusicEntry *musicSlot = _music->getFirstSlotWithStatus(kSoundPlaying);
switch (value) {
case 1:
- if ((musicSlot) && (musicSlot->status == kSoundPlaying)) {
+ if (musicSlot) {
_music->soundPause(musicSlot);
writeSelectorValue(_segMan, musicSlot->soundObj, SELECTOR(state), kSoundPaused);
}
return make_reg(0, 0);
case 0:
- if ((musicSlot) && (musicSlot->status == kSoundPaused)) {
+ if (!musicSlot && (musicSlot = _music->getFirstSlotWithStatus(kSoundPaused))) {
_music->soundResume(musicSlot);
writeSelectorValue(_segMan, musicSlot->soundObj, SELECTOR(state), kSoundPlaying);
return make_reg(0, 1);
@@ -848,33 +851,9 @@ reg_t SoundCommandParser::kDoSoundSuspend(EngineState *s, int argc, reg_t *argv)
}
void SoundCommandParser::updateSci0Cues() {
- bool noOnePlaying = true;
- MusicEntry *pWaitingForPlay = NULL;
-
- const MusicList::iterator end = _music->getPlayListEnd();
- for (MusicList::iterator i = _music->getPlayListStart(); i != end; ++i) {
- // Is the sound stopped, and the sound object updated too? If yes, skip
- // this sound, as SCI0 only allows one active song.
- if ((*i)->isQueued) {
- if (!pWaitingForPlay || pWaitingForPlay->priority < (*i)->priority) // fix #9907
- pWaitingForPlay = (*i);
- // FIXME(?): In iceman 2 songs are queued when playing the door
- // sound - if we use the first song for resuming then it's the wrong
- // one. Both songs have same priority. Maybe the new sound function
- // in sci0 is somehow responsible.
- continue;
- }
- if ((*i)->signal == 0 && (*i)->status != kSoundPlaying)
- continue;
-
- processUpdateCues((*i)->soundObj);
- noOnePlaying = false;
- }
-
- if (noOnePlaying && pWaitingForPlay) {
- // If there is a queued entry, play it now - check SciMusic::soundPlay()
- pWaitingForPlay->isQueued = false;
- _music->soundPlay(pWaitingForPlay);
+ for (MusicList::iterator i = _music->getPlayListStart(); i != _music->getPlayListEnd(); ++i) {
+ if ((*i)->status == kSoundPlaying || (*i)->signal)
+ processUpdateCues((*i)->soundObj);
}
}
Commit: 505e9aff1596321ae86a7943a87dc0ad7063ab43
https://github.com/scummvm/scummvm/commit/505e9aff1596321ae86a7943a87dc0ad7063ab43
Author: athrxx (athrxx at scummvm.org)
Date: 2021-03-13T17:41:54+02:00
Commit Message:
SCI: (SCI0 sound) - ensure that pauseAll() works correctly
The last 2 commits might not be fully compliant with the ScummVM GMM code and our handling of global sound pausing/resuming. This commit makes sure that only sounds will resume that were actually playing.
Changed paths:
engines/sci/sound/music.cpp
engines/sci/sound/music.h
diff --git a/engines/sci/sound/music.cpp b/engines/sci/sound/music.cpp
index a263852359..d0188b8b20 100644
--- a/engines/sci/sound/music.cpp
+++ b/engines/sci/sound/music.cpp
@@ -40,7 +40,7 @@
namespace Sci {
SciMusic::SciMusic(SciVersion soundVersion, bool useDigitalSFX)
- : _soundVersion(soundVersion), _soundOn(true), _masterVolume(15), _globalReverb(0), _useDigitalSFX(useDigitalSFX) {
+ : _soundVersion(soundVersion), _soundOn(true), _masterVolume(15), _globalReverb(0), _useDigitalSFX(useDigitalSFX), _needsResume(soundVersion > SCI_VERSION_0_LATE), _globalPause(0) {
// Reserve some space in the playlist, to avoid expensive insertion
// operations
@@ -262,6 +262,10 @@ void SciMusic::clearPlayList() {
void SciMusic::pauseAll(bool pause) {
const MusicList::iterator end = _playList.end();
+ if (pause)
+ _globalPause++;
+ else
+ _globalPause = MAX<int>(_globalPause - 1, 0);
for (MusicList::iterator i = _playList.begin(); i != end; ++i) {
#ifdef ENABLE_SCI32
// The entire DAC will have been paused by the caller;
@@ -768,6 +772,7 @@ void SciMusic::soundPause(MusicEntry *pSnd) {
pSnd->pauseCounter++;
if (pSnd->status != kSoundPlaying)
return;
+ _needsResume = true;
pSnd->status = kSoundPaused;
if (pSnd->pStreamAud) {
_pMixer->pauseHandle(pSnd->hCurrentAud, true);
@@ -787,8 +792,9 @@ void SciMusic::soundResume(MusicEntry *pSnd) {
pSnd->pauseCounter--;
if (pSnd->pauseCounter != 0)
return;
- if (pSnd->status != kSoundPaused)
+ if (pSnd->status != kSoundPaused || (_globalPause && !_needsResume))
return;
+ _needsResume = (_soundVersion > SCI_VERSION_0_LATE);
if (pSnd->pStreamAud) {
_pMixer->pauseHandle(pSnd->hCurrentAud, false);
pSnd->status = kSoundPlaying;
diff --git a/engines/sci/sound/music.h b/engines/sci/sound/music.h
index 5fd596b8cd..7a833e4519 100644
--- a/engines/sci/sound/music.h
+++ b/engines/sci/sound/music.h
@@ -287,6 +287,8 @@ private:
int8 _channelRemap[16];
int8 _globalReverb;
bool _needsRemap;
+ int _globalPause;
+ bool _needsResume;
DeviceChannelUsage _channelMap[16];
More information about the Scummvm-git-logs
mailing list