Opened 11 years ago

Closed 11 years ago

Last modified 11 years ago

#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 digitall, 11 years ago

Summary: Simon2: Assertion in IntroAGOS: Simon2 crashes with assertion in Intro on Big Endian

comment:2 by digitall, 11 years ago

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.

comment:3 by raziel-, 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 digitall, 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 digitall, 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 raziel-, 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 digitall, 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 digitall, 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 digitall, 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 raziel-, 11 years ago

You pinpointed it :-)

c82a75df69aa5d8f36eae52deee508ef9a61e49e is infact the one breaking

comment:11 by raziel-, 11 years ago

Continuing this on https://sourceforge.net/p/scummvm/bugs/6553/

Please close

comment:12 by digitall, 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 digitall, 11 years ago

Owner: set to Kirben
Status: newclosed

comment:14 by Kirben, 11 years ago

Status: closednew

comment:15 by Kirben, 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 digitall, 11 years ago

Summary: AGOS: Simon2 crashes with assertion in Intro on Big EndianAGOS: Simon2 Amiga Datafiles crashes with assertion in Intro

comment:17 by digitall, 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 digitall, 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 digitall, 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 lordhoto, 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 raziel-, 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 lordhoto, 11 years ago

Owner: changed from Kirben to lordhoto
Resolution: fixed
Status: newclosed

comment:23 by lordhoto, 11 years ago

Confirmed fixed with 8a5ceb976804f51e60b94fcef73bc21b779eaac6. Closing this as fixed.

Note: See TracTickets for help on using tickets.