Opened 16 years ago

Closed 16 years ago

Last modified 15 years ago

#1706 closed defect (fixed)

Simon sound code causes lockup

Reported by: anotherguest Owned by: eriktorbjorn
Priority: normal Component: Engine: AGOS
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)

simon_midi.diff (1.3 KB ) - added by eriktorbjorn 16 years ago.
Patch against an August 24 CVS snapshot

Download all attachments as: .zip

Change History (6)

by eriktorbjorn, 16 years ago

Attachment: simon_midi.diff added

Patch against an August 24 CVS snapshot

comment:1 by eriktorbjorn, 16 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:2 by fingolfin, 16 years ago

Why even add a setLoopInternal(), why not just set _loopTrack directly?

comment:3 by eriktorbjorn, 16 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 eriktorbjorn, 16 years ago

Owner: set to eriktorbjorn
Resolution: fixed
Status: newclosed

comment:5 by eriktorbjorn, 16 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.

Note: See TracTickets for help on using tickets.