Opened 17 years ago

Closed 17 years ago

Last modified 13 months 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:


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, 17 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, 17 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, 17 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, 17 years ago

Owner: set to aquadran

comment:5 by fingolfin, 17 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, 17 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, 17 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, 17 years ago

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

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

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

comment:11 by digitall, 13 months ago

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