#6549 closed defect (fixed)
AGOS: Simon2 Amiga Datafiles crashes with assertion in Intro
| Reported by: | raziel- | Owned by: | lordhoto |
|---|---|---|---|
| Priority: | normal | Component: | Engine: AGOS |
| Version: | Keywords: | ||
| Cc: | Game: | Simon the Sorcerer 2 |
Description
ScummVM 1.7.0git (Feb 25 2014 13:52:47) Features compiled in: Vorbis FLAC MP3 RGB zLib MPEG2 Theora AAC FreeType2 JPEG PNG
I'm getting an assertion in the intro of Simon 2. It takes place right after the apprentice let the floating wardrobe vanish. He faces Sordid and everything, apart the MIDI music, stops.
assertion "_begin <= _end" failed: file "common/stream.cpp", line 213
Simon the Sorcerer 2 (CD/Windows/English)
AmigaOS4 - PPC - SDL - BE gcc (GCC) 4.2.4 (adtools build 20090118)
Ticket imported from: bugs/6549.
Change History (23)
comment:1 by , 11 years ago
| Summary: | Simon2: Assertion in Intro → AGOS: Simon2 crashes with assertion in Intro on Big Endian |
|---|
comment:2 by , 11 years ago
comment:3 by , 11 years ago
Bisecting is not possible on my system, as i can only use hg git which doesn't support "bisect". I can do a manual search for the revision though, but that will take time.
I checked the full releases and it stopped working with 1.5.0 (1.4.1 is fine)
comment:4 by , 11 years ago
Ah, that is helpful, so will look at possible revisions to try manually... You can do the bisection manually, but hg does support bisection from v1.0 natively or in earlier versions by using an extension: http://mercurial.selenic.com/wiki/BisectExtension
comment:5 by , 11 years ago
This should work with hg-git as once you have imported, the local repo is basically a hg repo.
comment:6 by , 11 years ago
Thanks for the hint, i'll check it out
For now i can give you two revisions closer together i tried manually
Good revision is a837bb409a0235e4b459bfbdfb66684c8b5d33e3
Bad revision is 335ba979a2e3aab9c1856bf84a18b60ab91de687
comment:7 by , 11 years ago
Hmm... That is still quite a large range i.e. 1385 commits from 2012-01-01 05:12:07 to 2012-06-30 20:47:29.
Can you try testing 4bdd38923ad7ef66199e6ed6ccb3f89f4ec782c7 (that will cut the range in half... hence bisection)... We can then repeat..
comment:8 by , 11 years ago
To speed up compilation, you can test with --disable-all-engines --enable-engines=agos,agos2 (Note that you will need to enable AGOS2 as well as there was a breakage of compiling only AGOS1 in this range, so you need to enable both).
comment:9 by , 11 years ago
I suspect the issue could be introduced by c82a75df69aa5d8f36eae52deee508ef9a61e49e which is in this range:
Author: Johannes Schickel lordhoto@scummvm.org 2012-01-22 01:25:17
AGOS: Rework digital sound playback.
The BaseSound class does now only save the sound filename instead of a file
handle. When a new sound is started a new file handle is created, which
assures that each sound uses a different file handle and thus allows for
directly streaming sounds from disk.
This fixes bug #3475610
"AGOS: Wrong sound effects during intro of Simon 2 (DOS)".
This commit touches the sound functions and uses ReadStreams...
@Raziel: Could you test this commit and the previous commit, 174b6c4f1c6a6fb2e9a9051aa509af1e8cca9bd1 to see if this is the good/bad change ie. the point of regression.
comment:10 by , 11 years ago
You pinpointed it :-)
c82a75df69aa5d8f36eae52deee508ef9a61e49e is infact the one breaking
comment:11 by , 11 years ago
Continuing this on https://sourceforge.net/p/scummvm/bugs/6553/
Please close
comment:12 by , 11 years ago
This is being continued on a new bug #6553 as this original issue is now solved: http://sourceforge.net/p/scummvm/bugs/6553/
The cause was that the Amiga conversion of Simon the Sorcerer 2 - The Lion, the Wizard and the Wardrobe is incorrectly detected as Simon the Sorcerer 2 (CD/Windows/English) leading to problems with audio access and the crash indicated.
Since the main issue is the detection problem, this is a separate issue from the crash/assertion investigation. Closing...
comment:13 by , 11 years ago
| Owner: | set to |
|---|---|
| Status: | new → closed |
comment:14 by , 11 years ago
| Status: | closed → new |
|---|
comment:15 by , 11 years ago
I can reproduce this problem on Windows, so it isn't specific to Big Endian.
It looks like the problem is in getSoundStream() of engines/agos/sound.cpp, which passes along the start and end of the sound. If the last sound of a voice file is been played, it passes along the correct offset, but the incorrect end point (zero). Since the next offset, is actually the first offset for the next file.
The layout of voices.idx is for multiple files, with offsets going from zero to the last offset in each file. So it isn't safe to calculate the end point via these offsets, specially in the case of the last sound for each file.
comment:16 by , 11 years ago
| Summary: | AGOS: Simon2 crashes with assertion in Intro on Big Endian → AGOS: Simon2 Amiga Datafiles crashes with assertion in Intro |
|---|
comment:17 by , 11 years ago
@kirben: Yes, Sorry.
There was some further discussion on this on IRC, but the bug wasn't updated since it was closed: http://logs.scummvm.org/log.php?log=scummvm.log.08Mar2014&format=html
The issue is triggered by the variant datafiles rather than BE/LE or OS.
P.S. Your description of the issue sounds very similar to a latent issue in Touche engine in the music data where the last end offset is calculated incorrectly... See eb30e5c59417329544d671632d2db1ca4f8fe655 as I came across it while looking at adding the external music file integration. Though not sure if it ever manifests...
comment:18 by , 11 years ago
Ah yes: index 16a95d3..b1e7d56 100644 --- a/engines/touche/resource.cpp +++ b/engines/touche/resource.cpp @@ -214,6 +214,17 @@ uint32 ToucheEngine::res_getDataOffset(ResourceType type, int num, uint32 *size) if (num < 0 || num > rd->count) { error("Invalid resource number %d (type %d)", num, type); } + + if (type == kResourceTypeMusic) { + for (int i = 0; i < rd->count; i++) { + _fData.seek(rd->offs + i * 4); + uint32 offs = _fData.readUint32LE(); + uint32 nextOffs = _fData.readUint32LE(); + if (offs != 0) + debug(1, "Sound %d: Offset:0x%08X Size:%u", i, offs, (nextOffs - offs)); + } + } + _fData.seek(rd->offs + num * 4); uint32 offs = _fData.readUint32LE();
Will show the issue...
comment:19 by , 11 years ago
P.P.S Sorry again.. A little off-topic, but I did wonder whether any solution you apply to AGOS could be applied to this as well. Thanks.
comment:20 by , 11 years ago
I made a pull request for a possible fix over here: https://github.com/scummvm/scummvm/pull/446 If somebody could test that, that would be most appreciated.
comment:21 by , 11 years ago
Tested with my Amiga copy of StS2. This fixes the assertion and the intro runs through until the game starts.
Thanks a lot
comment:22 by , 11 years ago
| Owner: | changed from to |
|---|---|
| Resolution: | → fixed |
| Status: | new → closed |
comment:23 by , 11 years ago
Confirmed fixed with 8a5ceb976804f51e60b94fcef73bc21b779eaac6. Closing this as fixed.
This looks like a BE bug.
@raziel_ : Can you try a "gross" bisection by trying v1.6.0, v1.5.0 etc. and see if this is a recent regression or if this has been occurring for some time? And note here which two releases the bug was introduced...
Then if you can do a "git bisect" building from source to locate the exact revision which caused the regression... Thanks in advance.
If you need any help getting this done, let me know.