[Scummvm-devel] Idea: IndexedMp3AudioStream

[Johannes Schickel]

On Thu, Oct 7, 2010 at 10:21 AM, yotam barnoy <yotambarnoy at gmail.com> wrote:
> Hey guys
>
> I would like to address problems that occur with small devices and
> ripped mp3 CD tracks in those games that have them. There are 2
> problems:
>
> 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...

> 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.

> The combined effect of 1 & 2 is even worse, as the mp3 file first
> needs to be completely read to get its length, and then read again
> until the playback point is found.
>
> 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
> for the mp3 the first time it's played. The index file will consist of
> the total time of the file, followed by a list of (time, offset)
> values at regular intervals. The index file will remain loaded while
> the mp3AudioStream is initialized, and seeks will consist of finding
> the nearest earlier point in a linked list of (time, offset) pairs.
> This will be enabled only for small devices.
>

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. 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. Also
of course all our make*Stream factory functions never take a filename
but a SeekableReadStream object.

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?)

> 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.

// Johannes