Opened 18 years ago

Closed 18 years ago

Last modified 2 years ago

#723 closed defect (fixed)

SMUSH: The new smush player is (still) crashing

Reported by: eriktorbjorn Owned by: fingolfin
Priority: normal Component: Engine: SCUMM
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 eriktorbjorn, 18 years ago

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.)

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

Owner: set to aquadran

comment:5 by fingolfin, 18 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 aquadran, 18 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 eriktorbjorn, 18 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:8 by fingolfin, 18 years ago

So, is this still an issue, or not, or yes, or what ?!

comment:9 by eriktorbjorn, 18 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 fingolfin, 18 years ago

Owner: changed from aquadran to fingolfin
Resolution: fixed
Status: newclosed

comment:11 by digitall, 2 years ago

Component: --Unset--Engine: SCUMM
Note: See TracTickets for help on using tickets.