Opened 11 years ago

Closed 11 years ago

Last modified 5 months ago

#8751 closed patch

Possible workaround for bugs related to cloneToFadeOutTrack

Reported by: bluegr Owned by: fingolfin
Priority: normal Component: Engine: SCUMM
Keywords: Cc:
Game: Monkey Island 3

Description

Hello

I don't really know how iMUSE digital works, but I think I've found a possible workaround which prevents ScummVM from crashing when an issue with cloneToFadeOutTrack occurs in MI3. I've tried this with the savegames from bug reports #1527274 and #1763227, and the issue still occurs, but ScummVM doesn't crash anymore.

As detailed in bug report #1635361, this issue occured for me when a track was being unloaded and another one was being loaded. Specifically, the problem started at line 387 of scumm/dimuse.cpp, and then continued at line 418 of scumm/dimuse.cpp, which in turn kept being called - it seems that line 418 is the place it's failing when this occurs.

The workaround is to place the following lines right after line 316 in scumm/dimuse_track.cpp:

if (track->toBeRemoved) {
warning("IMuseDigital::cloneToFadeOutTrack: Tried to play a track to be removed");
return NULL;
}

Normally, this shouldn't occur, but as mentioned, it seems there is a racing issue occuring sometimes when a track is being unloaded, where cloneToFadeOutTrack shouldn't be called at that point.

As I said this is a workaround, it doesn't address the actual issue, but it prevents ScummVM from crashing when it occurs (at least for me). I don't know if the game audio stops playing when ScummVM reaches such a state (it shouldn't though - at least it didn't for me) so I'm submitting this as a patch in case someone finds it useful.

Ticket imported from: #1839861. Ticket imported from: patches/856.

Attachments (1)

imuse.patch (8.7 KB) - added by fingolfin 11 years ago.

Download all attachments as: .zip

Change History (11)

comment:1 Changed 11 years ago by bluegr

Owner: set to aquadran

comment:2 Changed 11 years ago by bluegr

Assigning this to aquadran, since he's the one who knows about iMUSE digital

comment:3 Changed 11 years ago by fingolfin

I am reluctant to commit such a work around right now. If it's a racing condition, then from this fact we should be able to deduce how it occurs, and then fix it properly. A check for a member var can never be a complete fix/workaround for a race, as the value might change even while reading it from memory.

comment:4 Changed 11 years ago by sev-

Pawel, may you take a look at it before the release?

comment:5 Changed 11 years ago by fingolfin

The attached patch was inspired by Filippos observation, but goes beyond it.
First off, we now generate an error if cloneToFadeOutTrack is called on a track which is toBeRemoved.
Secondly, I removed a spot where we set toBeRemoved to true and then may have proceeded to call cloneToFadeOutTrack (namely, we did that in IMuseDigital::callback(), when a fade out is complete).
Third, I removed the readyToRemove member var. This is not necessary with some clever extra coding
Fourth, we now attempt to "flush" tracks much more often, e.g. in callback(), not just in flushTracks, so we "clean up" with a much higher frequency
Next, I made allocSlot thread safe (it wasn't -- ouch), and moved the busy-waiting-for-a-free-track code into it (from startSound).

Combined, I hope that this patch fixes these bugs, or at the very least, makes them far less likely to occur.
(Possibly) affected bugs:
#1848399: fixed.
#1527274, #1763227: fixed, I think (but I never was able to reliably reproduce this issue in the first place)
#1624464: ???

File Added: imuse.patch

comment:6 Changed 11 years ago by fingolfin

To sum it up: It would be nice if people could verify this patch helps them and doesn't cause bad regressions. In which case I would recommend committing it to both trunk and the 0.11.0 branch.

Changed 11 years ago by fingolfin

Attachment: imuse.patch added

comment:7 Changed 11 years ago by fingolfin

File Added: imuse.patch

comment:8 Changed 11 years ago by bluegr

Nice work Max :) As I mentioned in the relevant bug reports, I think the racing issue with cloneToFadeOutTrack should be fixed now, so I believe this can be closed

comment:9 Changed 11 years ago by fingolfin

Owner: changed from aquadran to fingolfin
Status: newclosed

comment:10 Changed 5 months ago by digitall

Component: Engine: SCUMM
Game: Monkey Island 3
Note: See TracTickets for help on using tickets.