#8751 closed patch
Possible workaround for bugs related to cloneToFadeOutTrack
| Reported by: | bluegr | Owned by: | fingolfin |
|---|---|---|---|
| Priority: | normal | Component: | Engine: SCUMM |
| Version: | 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)
Change History (11)
comment:1 by , 16 years ago
| Owner: | set to |
|---|
comment:2 by , 16 years ago
comment:3 by , 16 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:5 by , 16 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 , 16 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 , 16 years ago
| Attachment: | imuse.patch added |
|---|
comment:8 by , 16 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 , 16 years ago
| Owner: | changed from to |
|---|---|
| Status: | new → closed |
comment:10 by , 5 years ago
| Component: | → Engine: SCUMM |
|---|---|
| Game: | → Monkey Island 3 |
Assigning this to aquadran, since he's the one who knows about iMUSE digital