#723 closed defect (fixed)
SMUSH: The new smush player is (still) crashing
| Reported by: | eriktorbjorn | Owned by: | fingolfin |
|---|---|---|---|
| Priority: | normal | Component: | Engine: SCUMM |
| Version: | Keywords: | ||
| Cc: | Game: |
Description
Even with Fingolfin's mutex changes on March 19, the smush player is still crashing because it's trying to blit to a locked surface. I think it's because waitForTimer(), which is not protected by the mutex, will call update_screen() if ScummVM receives mouse movement events.
But should it really have to do that if the mouse cursor is hidden? It seems unnecessary. Though if we rely on that, we have to make damn sure the cursor *is* hidden during smush playback. At the moment that's not guaranteed. Except for the intro movie, it was visible during smush movies when I played CMI a few days ago.
Can it even be guaranteed in the case of Full Throttle's action sequences? I haven't seen what those should really look like.
Ticket imported from: #706754. Ticket imported from: bugs/723.
Change History (11)
comment:1 by , 22 years ago
comment:2 by , 22 years ago
Well, that crash is gone at least. Though if I try to quit while watching the CMI intro, ScummVM tends to hang. It'll also crash if I try to bring up the save/load dialog.
Another thing that bothers me is that right now we have a deliberate delay between lock_mutex() and unlock_mutex(). To my understanding, this is a Bad Thing. Perhaps we should change waitForTimer(1) to waitForTimer(0), and add a call to delay_msecs(10) outside of the locked code section?
comment:3 by , 22 years ago
crash at save/load dialog was before my smush changes, and I have no idea. Our quit code is horrible, it NEVER release resources, it only call _system->quit(), in SDL backend is only free gfx surface and exit(0).
comment:4 by , 22 years ago
| Owner: | set to |
|---|
comment:5 by , 22 years ago
aquadran made some changes (which I have not yet tested yet), so waitForTime is not called any more, I think.
In any case, I could change this by moving the mutex to the SDL backend. Then any calls (from anywhere) to copy_rect / update_screen will be protected. Using a mutex incurrs a slight time penalty of course, but compared to the time a full blit takes it should be neglectable.
comment:6 by , 22 years ago
hmm, old waitForTime was splitted into two funcs waitForTime and parseEvents. Current waitForTime func has delay_ms() and call to parseEvents(). In the smush there is delay_ms outside lock mutex but rest of old waitForTimer func exist in parseEvents which there is in lock mutex loop
comment:7 by , 22 years ago
The problems have mostly gone away for me now. I'm speculating that the "hang at exit" problem may still be there, but that it was much easier to trigger when the delay was inside the locked section.
The crash on save/load dialog is probably still there as well, but I haven't tried it in a few days. I don't remember whether or not it was there before the smush changes though. I thought it wasn't.
comment:9 by , 22 years ago
The issue I originally reported - blitting to a locked surface - appears to be gone now. Any other issues should probably be reported separately, in which case we can close this one.
comment:10 by , 22 years ago
| Owner: | changed from to |
|---|---|
| Resolution: | → fixed |
| Status: | new → closed |
comment:11 by , 6 years ago
| Component: | --Unset-- → Engine: SCUMM |
|---|
I believe Aquadran has fixed this now, but I haven't had the opportunity to test it yet. I did try a similar change before submitting the bug report, and I couldn't get it to crash. (Before making the change I could get it to crash almost instantly by moving the mouse around.)