#9066 closed patch
Streaming for VOC files in ScummVM
| Reported by: | agent-q | Owned by: | agent-q |
|---|---|---|---|
| Priority: | normal | Component: | Audio |
| Version: | Keywords: | ||
| Cc: | Game: |
Description
The following patch enables VOC streams created with makeVOCStream() to stream audio a chunk at a time from disk rather than loading the whole sample into memory. This massively reduces the memory requirements of a VOC stream.
A new class, LinearDiskStream, takes an array of positions and sizes within a stream and plays them as if they were appended together as one sample. This is needed for VOC files because they can (and do) contain more than one block of sample data non-contiguous in a file.
New code in voc.cpp parses the VOC format and creates the sample block data which is used by LinearDiskStream. There is a certain amount of duplication of code here, but I couldn't find a way to avoid it.
It is anticipated that LinearDiskStream could be used to stream WAVs or any other file format containing uncompressed data. All that is needed is a function to create a block list (perhaps of only one block for WAV file format) specifying where the same data is in the stream.
The preprocessor symbol STREAM_AUDIO_FROM_DISK must be added to enable the new streaming functionality (currently used only for VOC files), without it everything runs as before. I added it to common/scummsys.h
This is tested in the Scumm engine and the Kyra engine, and there are no problems. I'll be testing others as I do my normal testing.
A small change is needed (included in this patch) to the Kyra engine to prevent it from deleting the stream object after creating the audio stream. This may be needed for other engines too.
Looping is not yet supported, mainly because it seems to never be used, at least for VOC files.
Ticket imported from: #2834001. Ticket imported from: patches/1171.
Attachments (6)
Change History (24)
by , 15 years ago
| Attachment: | streaming_voc_patch.diff added |
|---|
comment:1 by , 15 years ago
Sound looping is used by VOC segments in the DOS versions of Simon the Sorcerer 2.
The DOS CD demos of Simon the Sorcerer 2 could be used for testing, specifically the background sounds at 3 Bears Cottage and Town Square locations.
comment:2 by , 15 years ago
That KYRA patch would not be needed if the signature of "makeVOCStream" would be changed to the ones, we use for MP3, OGG/Vorbis and FLAC. Actually the "makeVOCStream" in KYRA is, as noted in the comment, having the same signature as the MP3, OGG/Vorbis and FLAC ones, so I can just have all supported formats in a table.
I'll try to add an updated patch for that later.
comment:3 by , 15 years ago
I did my proposed "makeVOCStream" change:
First of all the signature of Audio::makeVOCStream is now the same to the ones for MP3, OGG/Vorbis and FLAC. Next I added "FLAG_UNSIGNED" inside makeVOCStream, since all VOCs are unsigned (at least the 8bit ones, we support).
Meanwhile I fixed some leaks:
The "block" variable inside "makeVOCDiskStream" was never freed (or rather delete[]ed). I moved this deletion inside "LinearDiskStream" now. I added a "TODO/FIXME" about that, since it is not exactly nice to do so.
The "_audioBlock" member of "LinearDiskStream" was not freed before, I also added a free for that one.
This makes me wonder, is there any reason not to use Common::List or Common::Array for the block list? This would remove the need for all the aforementioned, also we wouldn't need to allocate 256 blocks as MAX inside "makeVOCDiskStream".
Some general things:
The "LinearDiskStream" is a nice idea. But VOC has some features, which might make it unusable for it.
For example the "Silence" block in IGOR, could not be implemented properly with it. I don't know how long the silence in IGOR's VOC files is, but it could be 1 or 2 secs, which might be important. The comment talks about it not being much difference, but sadly it does not give any more information about it.
Next if a VOC file has more than one block 1 and 9, they might have different sample rates. I doubt that happens so far, of course we don't have any paranoia checks for it... :-)
For now I guess using the LinearDiskStream approch is fine. Especially doing loads of more work now, might make it impossible to include it in 1.0.0. So I consider this use fine for now.
The looping seems to be needed, as Kirben says, I had to remove the "flags" for "makeVOCStream" to make it compatible with the signature of MP3 etc. creation. Thus I changed the AGOS implementation , which only does endless loops as far as I can see, for VOC playback to mimic the MP3 one, thus wrapping it inside "LoopingAudioStream" of the AGOS engine. The towns square in SImon 2 CD works fine for me.
by , 15 years ago
| Attachment: | streaming_voc_patch_v2.patch added |
|---|
This time the correct version 2 of the patch, the last one missed the AGOS loop change.
comment:4 by , 15 years ago
What I seem to have totally missed in my last comment: I think now the patch is good enough to make it into the release. I hope Kirben can give some feedback, whether the AGOS change works fine for him or not, before we commit it.
comment:5 by , 15 years ago
Yes, only complete and endless sound looping is used by Simon the Sorcerer 2.
The VOC segments in the Amiga CD32 version of Simon the Sorcerer 1 are actually signed, that was the main reason flags are been passed to makeVOCStream().
comment:6 by , 15 years ago
Hm that's of course not nice :-/. According to the VOC docs I found type 0 should be uncompressed 8bit unsigned format. Since that's the only one we support so far, we need to think of a nicer handling of this.
comment:7 by , 15 years ago
Here's another patch for SAGA, for the CD music in ITE CD. ITE CD uses raw PCM data, so it might be good as a test case here, and the code would certainly benefit from the usage of LinearDiskStream. I've removed the custom stream used in SAGA, since it pretty much does what LinearDiskStream does. Right now, all it does is to preload the music track in a buffer and access it using makeLinearInputStream, so this bit could be changed to use LinearDiskStream.
On a side note, I've also tried to stream compressed audio files from disk, but the starting offset seems to be wrong for some reason :( Shouldn't it be equal to startTime, in theory? Pushing the data in a MemoryReadStream works, but preloads it all in memory, which is bad...
comment:8 by , 15 years ago
Another thing: the digital music in ITE CD (i.e. music.rsc) can be found in Wyrmkeep's site, if you don't have it (original Dreamer's Guild titles didn't have it). To get it, download patch 1.2 from Wyrmkeep, uncompress it and place music.rsc in ITE's folder.
Patch 1.2 can be found at: http://wyrmkeep.com/ite/update.html
comment:9 by , 15 years ago
Actually I'm not sure about which "startTime" you are talking. If it's for the "loop_start"/"loopStart" parameter, I guess it's not working because as agent-q pointed out, the LinearDiskStream does not support looping, thus it probably does not support that parameter yet either.
comment:10 by , 15 years ago
I'm sorry, I should have been more clear on this. My problem lies with the startTime parameter of the compressed stream constructors (i.e. makeMP3Stream, makeVorbisStream and makeFlacStream),, I just noted it as an issue I have with my patch (there are FIXME/TODO comments in the ITE patch concerning this)
comment:11 by , 15 years ago
Ah yes the "start" etc. parameter should be AFAIK the playtime and you seem to pass the file offset. The correct solution would be to wrap the resource stream into an SeekableSubReadStream (see common/stream.h for more information) and pass that to the compressed sound stream creation functions.
comment:12 by , 15 years ago
I did made a clean version of my patch, which removed the "makeVOCStream" signature changes, so AGOS should be fine again. The AGOS part of the patch also adds looping and handles signed VOCs correctly (I hope it does, I don't know how to test that actually ;-).
by , 15 years ago
| Attachment: | streaming_voc_patch_v2_clean.patch added |
|---|
My patch without the signature changes of "makeVOCStream".
comment:13 by , 15 years ago
Thanks, that fixed it :)
Here's a second version of the ITE patch, with compressed digital music being streamed from disk. I haven't yet changed the bit for the uncompressed (raw PCM) music to use LinearDiskStream, but that should be easy to change.
I've also changed the part where it plays sound effects and speech (in sound.cpp) so that these are streamed from disk instead of being loaded in memory, but I'm not sure if that's a good idea or not - particularly, if this will be slow and cause sound stuttering in some platforms
by , 15 years ago
| Attachment: | saga-itecd-music-v2.patch added |
|---|
SAGA ITE CD music patch v2, with streaming of compressed audio from disk
comment:14 by , 15 years ago
Overall, this looks very nice. Good work, Neil, and thanks to all the people who already reviewed and improved this!
Some comments:
* Based on what LordHoto suggested (using a Common::Array or Common::List to keep track of the blocks), I would change the way LinearDiskStream instances are created: Follow the example of AppendableAudioStream and do *not* specify all blocks in the constructor of the stream. Instead, have a method which adds a single block. So, instead of creating a list of blocks, then creating a new LinearDiskStream and passing that list to it (and wondering when to delete that list ;), do it the other way around: First create the (empty) LinearDiskStream. Then, in parseVOCFormat(), call myLinearDiskStream.addBlock(pos, len) as needed. The data internally could be stored in a Common::List -- really, most of the code would be very similar to what the BaseAppendableMemoryStream class does.
* Based on this, LinearDiskStreamAudioBlock would become a *private* struct. I'd consider moving LinearDiskStream to its own .h / .cpp, by the way (we probably should do that with the AppendableAudioStream code, too).
* To support silence, let's augment the LinearDiskStreamAudioBlock struct: Add a "bool silence" flag to it. (Alternatively, interpret negative "pos" values to indicate silence). A "LinearDiskStream::addSilence(int len)". Here, a different frequency can occur, the comments say -- well, since it is silence, it is actually pretty easy to change the "frequency" here and adjust the "len" value accordingly:
int realSilenceLen = (double)silenceLen * (double)playbackAudioFreq / (double)silenceFreq;
myStream.addSilence(realSilenceLen);
* We could add proper looping support, if we ever need to, by further extending the internal LinearDiskStreamAudioBlock class, by adding the relevant looping information.
* I have never seen a VOC file that uses multiple different rates for actual audio data. Of course they might exist, but I think it shall suffice to deal with it if we ever encounter it. In the meantime, we can add a check for it and error out if needed.
* Please change _ownsStream and takeOwnership to disposeAfterUse resp disposeStream, to match the naming pattern we used in common/stream.h and sound/*.h (helps to grep for these things). In fact, I guess we should change sound/*.h from disposeAfterUse to disposeStream, for further consistency.
@thebluegr: Any reason you write "identifier == byte(0)" instead of just "identifier == 0" ? Anyway, I like how much simpler the SAGA code gets with this :). Unfortunately, the patch does not compile for me, I get this: engines/saga/music.cpp:283: warning: declaration of ‘resourceData’ shadows a previous local engines/saga/music.cpp:245: warning: shadowed declaration is here engines/saga/music.cpp:318: error: ‘_looping’ was not declared in this scope
comment:15 by , 15 years ago
Oops, thanks for the comments fingolfin. MSVC didn't warn me about the shadowed variable, and I have disabled FLAC in my build. Uploaded a new patch, which should hopefully work (but doesn't utilize the new stream class yet - there's a TODO for that in the patch).
by , 15 years ago
| Attachment: | saga-itecd-music-v3.patch added |
|---|
SAGA ITE CD music patch v3 (fixed v2)
comment:16 by , 15 years ago
It seems this work has been commited to trunk by Neil and Filippos, closing it.
comment:17 by , 15 years ago
| Owner: | set to |
|---|---|
| Status: | new → closed |
comment:18 by , 6 years ago
| Component: | → Audio |
|---|
Streaming audio patch v1.0. Only VOC supported for now.