[Scummvm-devel] VorbisInputStream ctr & check catch

[sunmax at libero.it]

Hi there ScummVM team!

It's me again ;-)

Ok, while I was debugging the ogg bun issue on PS2 I noticed a
behaviour that might affect other platforms too, so here comes:

there is a catch in VorbisInputStream ctr and control logic in
dimuse_sndmgr.cpp !

If opening the compressed stream fails we still have a valid obj
(with some uninitialized members) which will later assert in the
Timestamp ctr. While it survives the assert that should actually
detect this condition (not that the obj was created, but whether
the object was successful to open the vorbis file).

#2  0x00000000002d0cc0 in Timestamp (this=0x4d06d0, ms=0, fr=0)
    at ../../../sound/timestamp.cpp:44
#3  0x000000000036222c in Audio::SeekableAudioStream::seek (this=0x797ee0, 
    where=0) at audiostream.h:178
#4  0x0000000000165994 in Scumm::ImuseDigiSndMgr::getDataFromRegion (
    this=0x4f5690, soundDesc=0x4f5708, region=0, buf=0x4d07c8, offset=0, 
    size=8820) at ../../../engines/scumm/imuse_digi/dimuse_sndmgr.cpp:718

(gdb) print soundDesc->compressedStream 
$6 = (class Audio::SeekableAudioStream *) 0x797ee0
(gdb) print *soundDesc->compressedStream 

Now, "compressedStream" is (supposed to be) filled up couple lines above :

  soundDesc->compressedStream = Audio::makeVorbisStream(tmp, DisposeAfterUse::YES);

According to the Vorbis factory...

  SeekableAudioStream *makeVorbisStream(
          Common::SeekableReadStream *stream,
          DisposeAfterUse::Flag disposeAfterUse) {
          return new VorbisInputStream(stream, disposeAfterUse);
  }

...it's going to be a VorbisInputStream object.

Now if we check VorbisInputStream ctr we see:

        int res = ov_open_callbacks(inStream, &_ovFile, NULL, 0, g_stream_wrap);

        if (res < 0) {
                warning("Could not create Vorbis stream (%d)", res);
                _pos = _bufferEnd;
                return;
        }

So if opening the vorbis failed, we return -but- 

a) this does not mean that the object has not been created ;-)

   just some of its members (most notably _rate) ain't initialized

b) thus compressedStream has a value != 0

   so the  assert(soundDesc->compressedStream);  in dimuse_sndmgr.cpp @ 717

   won't stop the following seek call with a getRate() which will return 0 and trigger the assert in Timestamp.cpp:40

Dizzy, but true! ;-)

We should have a flag (since we can only return from ctr) to check
whether the VorbisInputStream object has been properly constructed.

Of course if your ScummVM flavor is correctly opening .bun(s), you'll
never see this. This part I still have to figure out on PS2 side ;-)

Thanks,
 -max