[Scummvm-devel] Idea: IndexedMp3AudioStream

[Johannes Schickel]

On Thu, Oct 7, 2010 at 2:22 PM, yotam barnoy <yotambarnoy at gmail.com> wrote:
>>> 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.

See below.

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

It should not be forgotten that this is only used in a very few
places. Actually I think the only big use place is the audio cd
emulation :-P.

So I am not sure how this would help the "looping" subject you talk
about, since I think many engines just use makeMP3Stream etc. directly
(especially since the datafiles are usually embedded). Anyway as you
later on say you never had any problems with looping except for audio
cd emulation, so I wouldn't touch this.

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

Best example is of course the DC port or the PS2 port here, where game
path is on CD usually, so no real possibility to write to it :-P.

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

Well I mean we could say that for improved performance people should
use CBR files on devices with slow I/O. And if that helps seeking /
length calculation it might be the easiest way out.

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

I am actually not sure at all why we need to horrify our code when you
say you never had any problems except with CD audio tracks. I mean
when you have indexed the MP3 files for the audio cd emulation that
should take care of all the looping code too, since the looping code
just uses the SeekableAudioStream API. But maybe you have a good
reason why you want to change the looping code too?

// Johannes