#1706 closed defect (fixed)
Simon sound code causes lockup
| Reported by: | anotherguest | Owned by: | eriktorbjorn |
|---|---|---|---|
| Priority: | normal | Component: | Engine: AGOS |
| Version: | Keywords: | ||
| Cc: | Game: | Simon the Sorcerer 1 |
Description
In this function the Midiplayer locks the mutex, and then setLoop wants to lock it again. Result.. lockup. Suggested fix.. release mutex lock.. and lockit after setLoop has been made.
void MidiPlayer::loadSMF (File *in, int song, bool sfx) { { _system->lock_mutex(_mutex);
. . . Before if (!sfx) setLoop(p->data[6] != 0);
should be if (!sfx) { _system->unlock_mutex (_mutex); setLoop (p->data[6] != 0); _system->lock_mutex (_mutex); }
Ticket imported from: #1004919. Ticket imported from: bugs/1706.
Attachments (1)
Change History (6)
by , 20 years ago
| Attachment: | simon_midi.diff added |
|---|
comment:1 by , 20 years ago
Looks like the same thing can happen in MidiPlayer::queueTrack() as well.
I assume the locks in loadSMF() and queueTrack() are there for a good reason, so unlocking even temporarily seems risky to me. What if someone else manages to "slip past" the lock in that brief space of time?
The iMUSE player in the SCUMM engine gets around the problem by having "internal" versions of the functions which does not use locking, presumably because they are always called from inside a lock, and "external" versions which lock, call the internal function, and then unlock.
I don't want to go to such lengths. At least not for a simple proof-of-concept. I've attached a patch to show how I'd do it. It simply adds a setLoopInternal() function for the MidiParser class to use.
However, since I have no idea how to trigger the lockup during the game, I can't test it.
comment:3 by , 20 years ago
Old war wound. I had some lecturers at the university who seemed to believe that you would go blind if you ever touched a variable directly instead of defining a function to do it for you. :-)
comment:4 by , 20 years ago
| Owner: | set to |
|---|---|
| Resolution: | → fixed |
| Status: | new → closed |
comment:5 by , 20 years ago
I've made Fingolfin's suggested change, so I assume this bug is fixed now, even if I still don't know how to reproduce it myself. Feel free to reopen if it turns out I'm wrong.
I've also made the change on branch-0-6-0, so if there's ever another 0.6.x release, it will be included there as well.
Patch against an August 24 CVS snapshot