Opened 12 years ago

Closed 12 years ago

Last modified 11 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 12 years ago.

Download all attachments as: .zip

Change History (11)

comment:1 by bluegr, 12 years ago

Owner: set to aquadran

comment:2 by bluegr, 12 years ago

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

comment:3 by fingolfin, 12 years ago

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 by sev-, 12 years ago

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

comment:5 by fingolfin, 12 years ago

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 by fingolfin, 12 years ago

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.

by fingolfin, 12 years ago

Attachment: imuse.patch added

comment:7 by fingolfin, 12 years ago

File Added: imuse.patch

comment:8 by bluegr, 12 years ago

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 by fingolfin, 12 years ago

Owner: changed from aquadran to fingolfin
Status: newclosed

comment:10 by digitall, 11 months ago

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