[Scummvm-devel] Idea: IndexedMp3AudioStream

[yotam barnoy]

>> 1. We immediately get the playback length of every AudioCD
>> AudioStream, both in AudioCDManager and in makeLoopingAudioStream. In
>> our mp3 files (which are usually VBR) we have to read all the way from
>> the start of the file to the end of the file in order to get the
>> playback length. CD rips can be in the order of tens of megabytes and
>> mp3 files need to be read frame by frame, which on slow devices causes
>> read times of 20 seconds or more.
>>
>
> To be precise currently we always compute the length of the MP3 file
> in the MP3Stream constructor. The "getLength()" call is a O(1)
> operation later on (or rather should be for all SeekableAudioStream
> implementations :-P). So I am unsure why you mention these code parts
> here...

Yeah that's a lack of clarity as a result of re-editing my email. What
I meant to say is that those are the places that retrieve the length,
so if we deal with one the other will still have the issue.
Specifically, the indexing would be for CD tracks, but then
loopingAudioStreams will still have the issue.

>> 2. Every time we seek, we have to peruse the file until we find the
>> appropriate frame. If the seek is earlier in the file we go from the
>> beginning of the file. This process can take a long time on large
>> audio files as well.
>
> Yeah the seeking of MP3 files was never really good, but then again
> the libmad API doesn't provide any good documentation, so it's hard to
> improve stuff there.

I don't think it can be improved with variable bitrate files.

>> To deal with this issue, I propose a wrap to the mp3 AudioStream when
>> used for CD tracks. An IndexedMp3AudioStream will create an index file...
>
> I take this index file will be written to disk too? I mean else we
> would still suffer from the long delay on every time the MP3 file is
> opened.

Correct.

> Now how do you plan to handle filename clashes, like you have
> two games with MP3 files for CDDA setup and then start one and then
> the other, since savepath is the only real save place to write you
> can't just use the mp3 filename and save the index to savepath.

OK that's a very good point. I didn't know you can't save to the game paths.

> Also of course all our make*Stream factory functions never take a filename
> but a SeekableReadStream object.

I believe SeekableAudioStream::openStreamFile(const Common::String
&basename) is the key function here.

>
> Anyway I wonder whether we can do any improved seeking / length
> detection for CBR MP3 files. I am not sure about MP3 internals, but
> maybe it is possible to easily calculate the length or the seeking
> position for CBR files. (Actually I remember that we only supported
> CBR MP3 files at one time or something like that... maybe it was
> because of this?)

I believe CBR is easier to calculate the length for, but VBR is out
there and that's what's currently used... Not the greatest argument
for it. Maybe it also sounds better...?

>
>> As an additional measure, we may want to get rid of the automatic
>> playback length retrieval in  makeLoopingAudioStream for small
>> devices, as this could cause slowdown in mp3 files other than CD
>> tracks as well.
>>
>
> So why do we want to get rid of that after we have a cached MP3 stream
> length? It should be a O(1) operation to retrieve that after it's
> saved in MP3Stream... Apart most of the AudioStream code (like
> SubSeekableAudioStream and SubLoopingAudioStream) relies on a proper
> length being given, since else it can't detect whether it should
> rewind to the loops start again (in case of SubLoopingAudioStream for
> example), or it can't determine the end of the stream
> (SubSeekableAudioStream), thus I think it's rather hard (or at least
> very very ugly) to remove all the length retrievals there. It might be
> possible to have a "magic" time for "loop till end" inside the
> SubLoopingAudioStream implementation for example, but that only
> clutters the code.

Yes I was thinking of a magic time constant meaning loop till end,
just as you say. The reason being that the index files will only be
for CD tracks. I think that our other sound formats already implement
indexing... Am I wrong about this? I don't get delays in games that
don't have CD tracks so I thought the indexing was built into our
formats - please correct me if I'm wrong.

Yotam