Opened 20 years ago
Closed 20 years ago
#1978 closed defect (fixed)
FT: Old Mine Road CTD
| Reported by: | SF/jerrywoolsey | Owned by: | Kirben |
|---|---|---|---|
| Priority: | normal | Component: | Engine: SCUMM |
| Version: | Keywords: | ||
| Cc: | Game: | Full Throttle |
Description
080CVS 3.29.05 Fighting on old mine road keeps crashing. English CD Win32 This has been happening since last week
Ticket imported from: #1173161. Ticket imported from: bugs/1978.
Attachments (1)
Change History (30)
by , 20 years ago
comment:1 by , 20 years ago
| Summary: | Old Mine Road CTD → FT: Old Mine Road CTD |
|---|
comment:2 by , 20 years ago
comment:3 by , 20 years ago
I got a "Assertion failed: offset == 0, file scumm/smush/chunk.cpp, line 159" crash when trying the savegame, but before that I got a "Game was saved with different gamedata - you may encounter problems" warning when loading the savegame, so I guess my English version is different from yours.
On the other hand, I got the same crash when using a boot param (550) to skip to the mine road. And in both cases, it would sometimes run very jerkily.
Some threading/locking problem - again - perhaps? Though the only such change I can think of recently are the sound handle changes...
Anyway, I didn't have either of those problems when I tried it with 0.7.0.
comment:4 by , 20 years ago
| Owner: | set to |
|---|
comment:5 by , 20 years ago
The assert in chunk.cpp is relatively new -- I added it some time ago when I modified the FileChunk class. The old version of the FileChunk::reinit method simply ignored the offset param and always used "0" as offset. To me, that looked like a bug, so I inserted an assert(). AFAIK this method is only ever invoked by the INSANE code, so maybe Eugene knows whether we should simply silently ignore the "offset" param, as the code used to; or whether a non-zero offset is valid (and how it should be dealt with).
The modification I made to FileChunk back then involved cloning the File object for each subchunk. The idea is that this should improve thread safety... The code used to share a single file object, and then seek before each read. That was problematic when used in multiple threads, though, since it was possible that a seek took place, then another thread got time, did yet another seek+read, and only after that the original thread resumed, and did its read -- now on the wrong position!
Finally, the SoundHandle change indeed could potentially cause problems -- isSoundHandleActive now locks the mixer mutex.
comment:6 by , 20 years ago
Erik, I tried boot param 550 but can't reproduce any problems. Is there anything specific I should do to trigger the assert?
comment:7 by , 20 years ago
The only thing I did was to make a turn to get onto the mine road - the one with all the biker fights and stuff - and then drive around for a while, getting into a few fights. That's where the occasional slowdowns and failed assertions would happen.
I didn't find any way to deliberately trigger it, though. It would just happen. It didn't take very long the two or three times I tried it.
comment:8 by , 20 years ago
There were no changes in INSANE code during last few months. So this is actually not related. Probably it is because of FIngolfin's changes to sound code which took place about a week ago.
comment:9 by , 20 years ago
| Owner: | removed |
|---|
comment:10 by , 20 years ago
I still can't put my finger on anything specific to trigger the failed assertion, but the slowdowns seem to happen every time when I'm about to catch up with another biker.
comment:11 by , 20 years ago
Looks to me as if any time smush_rewindCurrentSan() is called, that assertion will be triggered:
smush_rewindCurrentSan() calls smush_setupSanFile(0, 8, 0)
smush_setupSanFile() will then call _player->seekSan(0, 8, 0)
Since the 'file' parameter is 0, seekSan() will call reinit(8).
So unless I've accidentally strayed from the code path, 'offset' will be 8, not 0.
Question is, is it a bug that smush_setupSanFile() is called this way, or is the assertion bogus?
comment:12 by , 20 years ago
Eugene, did you fully read what I wrote? :-). Yes, the INSANE code *was* modified, by me, albeit indirectly: INSANE is the only thing in ScummVM which ever calls SmushPlayer::seekSan. And when doing so, it passes a parameter "pos" to that method. And this in turn then is passed on to FileChunk::reinit. Which, until recently, completely ignored it! As that looked bogus, I added an assert into reinit(), to check for it being non-0. That is the very assert described in this report (and one other report, I forgot which, where I also tried to point this out to you).
So, I still think you maybe should take a look at it, given that you are our INSANE expert (no pun intended :-). There are two possibillites: 1) reinit() was correct to ignore pos. In that case, I propose we simply remove the unused parameter (and the assert, of course). 2) reinit() should not ignore pos. In this case, the code needs to be modified to do whatever is the Right Thing (TM).
comment:13 by , 20 years ago
| Owner: | set to |
|---|---|
| Resolution: | → fixed |
| Status: | new → closed |
comment:14 by , 20 years ago
I check Full Throttle disasm. and an offset value of 8 when rewinding an san file via INSANE is correct. So reinit() is currently correct and should have always been setting that file offset. Judging by cvs logs, it looks like code just got lost.... Only needed to remove the assert.
comment:15 by , 20 years ago
| Status: | closed → new |
|---|
comment:16 by , 20 years ago
I'm going to reopen this bug - at least temporarily - because the original bug report never mentioned what kind of crash, and I just got another error message on the old mine road:
ERROR: (13:82:0xDE1): invalid seek request : 11206672 > 794 (delta == 11206664)
Does anyone know what that means, apart from the obvious?
comment:17 by , 20 years ago
| Resolution: | fixed |
|---|
comment:18 by , 20 years ago
A few more observations:
I'm still using boot param 550 to get to the mine road.
The numbers in the error message are not random. It's the same every time.
So far, the error happens after exactly one call to smush_rewindCurrentSan(). Because of the sub-optimal behaviour of this command-line shell I can't say for sure how soon after the call it happens, though.
comment:19 by , 20 years ago
| Status: | new → closed |
|---|
comment:20 by , 20 years ago
reinit() was seeking from current pos of san file, not start of san file. Fixed in ScummVM CVS.
comment:21 by , 20 years ago
| Resolution: | → fixed |
|---|
comment:22 by , 20 years ago
I'm still getting the "invalid seek" error, same as before.
By the way, I just noticed that after seekSan() calls _base->reinit(pos) it will call _base->seek(pos, FileChunk::seek_start) so while your change affects how reinit() sets _type, _size and _offset (which may be quite important) it doesn't seem to affect much else.
The reinit() function still looks strange to me. When a FileChunk object is created, it usually starts at position 0, and the eight first bytes become _type and _size. But in this particular case reinit() starts at position 8, and the *next* eight bytes become _type and _size.
comment:23 by , 20 years ago
| Resolution: | fixed |
|---|---|
| Status: | closed → new |
comment:24 by , 20 years ago
Actually, if _data is an instance of BaseScummFile then the default seek mode is SEEK_SET, in which case your change wouldn't make any difference at all, would it?
If the purpose of reinit() is to put the FileChunk object back to its original state, shouldn't it simply be like this?
void FileChunk::reinit() { _data.seek(_offset); _curPos = 0; }
The _type, _size and _offset variables are all set at creation time, so unless they change they're already correct.
Though if I make that change it crashes with an "ERROR: (13:82:0xDE1): Unknown Chunk found at 10: 2001405, 0" instead, so I guess that's not right either.
comment:25 by , 20 years ago
By the way, disregard the comment about reinit() and seekSan() both seeking to the same position -- reinit() seeks in _data (the ScummFile) and seekSan() seeks in _base (the FileChunk).
comment:26 by , 20 years ago
Last comment for tonight, I promise. :-)
If I were to guess - I don't have the knowledge to verify it - I'd say the original counted _curPos from the beginning of the chunk, so when reiniting it would start at position 8 to skip past the type and size fields. We, on the other hand, count _curPos from after the type and size fields.
If that's the case, then what we have to do is not just to change reinit() to put back the file at _offset, but we also have to either change seekSan() or the call to seekSan() so that _curPos is set to 0, not 8, after a reinit.
I made a very hackish change like that in my local copy of the source, and that seemed to fix the crashes. (Or at least *these* crashes. When I tried it on my Linux box yesterday I would sometimes get some Xlib error about async something or the other, which is probably a different bug. A threading/locking issue, perhaps?)
I'm not sure how to handle the case where seekSan() is used to search to a specific position in a new file. That case seems to work already, but I notice it also fiddles around with the number 8 in some way.
comment:27 by , 20 years ago
Hi,
I just downloaded the April 2 0.8.0CVS and now I get a debugger window on the old mine road crash. Here's the info: ERROR: (13:82:0xE8A): invalid seek request : 11206672 > 794 (delta == 11206664)
comment:28 by , 20 years ago
| Resolution: | → fixed |
|---|---|
| Status: | new → closed |
comment:29 by , 20 years ago
Thanks for the help eriktorbjorn, I didn't noticed that difference in ScummVM. I replaced reinit(), with offset values of zero.
Definately fixed this time, try the current Windows snapshot.
There is already another bug report open for Xlib errors, check bug report #1018588.
To process your bug report appropriately, we need you to provide the following additional information:
* ScummVM version (PLEASE test the latest CVS/Daily build) * Bug details, including instructions on reproducing it * Language of game (English, German, ...) * Version of game (talkie, floppy, ...) * Platform and Compiler (Win32, Linux, MacOS, ...) * Attach a save game if possible * If this bug only occurred recently, please note the last version without the bug, and the first version including the bug. That way we can fix it quicker by looking at the changes made.
This should only take you a little time but will make it much easier for us to process your bug report in a way that satisfies both you and us.
Thank you for your support!