Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Streaming music - WIP #127

Merged
merged 31 commits into from Aug 16, 2019

Conversation

@and3md
Copy link
Contributor

commented Jul 31, 2019

After #126 I still have problems with loading music. Decoding even one whole file results in a noticeable performance drop on mobile devices. Testers do not have mercy;) So I spent two evenings trying to implement streaming music. To see if this will improve performance. After that I can say that streaming music buffers can fix this issue.

This is first draft so do not check the details, let me know if you like the direction and general API changes. Or propose how it should be done :)

Currently only OGG is supported and OpenAL. To make sound buffer streamed add stream="true"
in XML Repo File. With this changes my game load levels immediately. Without streaming level load need 1-2 second to start. Even with one 700kB ogg music file to load, RAM usage drops by 20MB.

@michaliskambi
Copy link
Member

left a comment

Thank you!

Below is just a quick 5-minute review :) Sorry that I wrote some comments quickly.

In general:

  • I like how you modified CastleInternalVorbisDecoder, I like how new VorbisDecode uses the smaller lower-level routines.

  • I like the thread that fills music buffers. Although we must have also a fallback for platforms that don't support threading (preferably the fallback should "look nice" in code, without a lot of {$ifdef xxx} here and there).

  • I have unconstructive comments about the TStreamedSoundFile. They are unconstructive, because I don't know how to make it better :)

I saw you left some old pieces of code commented out, instead of removed -- I didn't point it out as this is a WIP, so I guess you plan to remove them :)

src/audio/castleinternalsoxsoundbackend.pas Outdated Show resolved Hide resolved
src/audio/castlesoundbase.pas Outdated Show resolved Hide resolved
@@ -90,7 +90,58 @@ TSoundFile = class
procedure ConvertTo16bit; virtual;
end;

{ TStreamedSoundFile }

This comment has been minimized.

Copy link
@michaliskambi

michaliskambi Jul 31, 2019

Member

TStreamedSoundFile (interface, implementation of TStreamedSoundFile.CreateFromFile) is in part a copy-and-paste of TSoundFile. I know you had a reason for it (one returns TStreamedSoundFile), but maybe we can make the code more shared (to avoid code duplication, and have everything "coded once"). E.g. introduce something like TAbstractSoundFile that is common ancestor of TStreamedSoundFile and TSoundFile, and allows to reuse code better, e.g. provides a skeleton of CreateFromFile that is shared by both TStreamedSoundFile and TSoundFile?

I'm not sure what is the best solution here (sorry -- falling asleep now :) ). I know that the current solution copies some code from TSoundFile, so this will make harder to maintain.

This comment has been minimized.

Copy link
@and3md

and3md Aug 6, 2019

Author Contributor

I was thinking about this and it's hard for me to create some reasonable base class, the use of which will not require continuous casting to the sub-type (as in the case of TAbstractSoundFile.CreateFromFile(...):TAbstractSoundFile). I think the differences in the method of use are too big for me to connect these two branches. A common ancestor will always require casting after createFromFile() or class responsibility will be unclean (by adding all methods and implement only some of them). Unless I do not see any obvious solution, that you see.

BTW. I know it's quite annoying to have two classes for each sound format like TSoundOggVorbis and TStreamedSoundOggVorbis. But I think it's easier to maintain two simple classes (with well-defined responsibility) than one with a lot of "if streming then" conditions.

This comment has been minimized.

Copy link
@michaliskambi

michaliskambi Aug 6, 2019

Member

I will need to think about it more :)

I understand the case you make. Indeed we want to have a clean code -- and I can see that sometimes (like in this case) it may be cleaner (producing code simpler to understand) to duplicate some stuff, than to try to reuse everything by a common class.

So, I think you're right here, and I'll try to do some thinking about it later :), maybe I'll be able to propose something.

@michaliskambi
Copy link
Member

left a comment

P.S. I like the simple API change of LoadBuffer (just additional parameter). This makes it easy for code not using sounds.xml to load buffers with streaming. Kudos.

I also submitted a comment for ReadVorbisFile and friends.

@@ -40,9 +40,20 @@ EVorbisFileError = class(EVorbisLoadError);
function VorbisDecode(Stream: TStream; out DataFormat: TSoundDataFormat;
out Frequency: LongWord): TMemoryStream;

procedure OpenVorbisFile(var VorbisFile: TOggVorbis_File; Stream: TStream; out DataFormat: TSoundDataFormat; out Frequency: LongWord);

This comment has been minimized.

Copy link
@michaliskambi

michaliskambi Jul 31, 2019

Member

Can we maybe implement a class like TOggVorbisStream, that descends from TStream ? It's constructor / Read / destructor methods could play the same role as current OpenVorbisFile / ReadVorbisFile / CloseVorbisFile do. Seeking and writing can just raise exceptions.

This would make the code using this even more obvious, as the code would use standard Pascal TStream API.

And then VorbisDecode would basically be a function that copies the TOggVorbisStream into TMemoryStream, which seems sensible, since that's what happens when we "load a file as a whole".

This comment has been minimized.

Copy link
@and3md

and3md Aug 6, 2019

Author Contributor

This is so internal so... can we move that to another PR? I want now finish my game (ASAP) :)

@and3md

This comment has been minimized.

Copy link
Contributor Author

commented Aug 1, 2019

P.S. I like the simple API change of LoadBuffer (just additional parameter). This makes it easy for code not using sounds.xml to load buffers with streaming. Kudos.

Thanks :) At first I just wanted to add Streamed: Boolean but we have const ExceptionOnError: Boolean = true and that makes confusion (what true/false means), so I decided to make enumeration (TSoundBufferType). This allows to easily add an overloaded LoadBuffer function without this new parameter for backward compatibility.

Draft 2 of streaming sound: Multiple playback at the same time, Don't…
… allocate anything before start using stream buffer.
@and3md

This comment has been minimized.

Copy link
Contributor Author

commented Aug 4, 2019

I made draft 2 of streaming music feature. Some changes in the concept were needed. But the result is much better.
Now multiple playback on the same time is supported :) So there is a dictionary of buffer resources for each sound source. Maybe code is more complicated but adding music to your xml repository with stream=true will not allocate/read form disk anything before start using stream buffer (playing). Android likes that :) The last thing I need to figure out is looping. After that I clean the code.

@and3md

This comment has been minimized.

Copy link
Contributor Author

commented Aug 5, 2019

OK, now sound looping works! Tomorrow more code cleaning :)

@and3md

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2019

I think it is now ready for review. Works perfect in my game (no lags when load levels) and make CGE more more mobile game friendly. I think it will be helpful for many users.

@michaliskambi

This comment has been minimized.

Copy link
Member

commented Aug 6, 2019

Cool, thank you! I'll review it around this weekend.

( Yes, the switch to TOggVorbisStream can be made later, that's low priority indeed. )

@eugeneloza

This comment has been minimized.

Copy link
Collaborator

commented Aug 7, 2019

I think it will be helpful for many users.

Indeed, thanks!!! :)

@michaliskambi

This comment has been minimized.

Copy link
Member

commented Aug 10, 2019

I think I have an idea how to make the code a bit more flexible in castleinternalsoundfile.pas. I will want to experiment with it myself ASAP :) I'll see if this makes sense then.

Idea:

  1. Implement RegisterSoundFormat(const MimeType: String; const SoundReader: TSoundReadEvent), and TSoundReadEvent is a callback that for URL returns a stream. So we make 1 implementation of it for OggVorbis (that returns TOggVorbisStream, doing in TOggVorbisStream.Create/Read/Destroy the same things we do now in OpenVorbisFile/ReadVorbisFile/CloseVorbisFile), and 2nd implementation for WAV (that can return just TMemoryStream, this will not require to change current WAV reader much).

    The TSoundReadEvent is advised to not return TMemoryStream. Instead it should actually read the file in each Read call. This allows streaming of the registered sound format.

    But it is still correct if the TSoundReadEvent returns TMemoryStream. Everything will work, just requesting "streaming" for this format is useless (since the whole data will be read at loading anyway).

    Benefits:

    • Everything else is sound-format-independent. We avoid TStreamedSoundOggVorbis class. If we implement other sound formats in the future (like MP3, or a zilion other formats supported by libsndfile), they will automatically work in "streaming" mode too.

    • IOW, you implement support for each sound format once. There's no need to implement both TStreamedSoundOggVorbis and TSoundOggVorbis to support a format in both streaming and non-streaming modes.

    • I wanted to introduce RegisterSoundFormat at some point anyway.

    • I wanted to introduce TOggVorbisStream anyway.

  2. TSoundFile, when reading uses the appropriate reader (calls some TSoundReadEvent).

    • A method like TSoundFile.LoadStream more-or-less just returns the stream returned by proper registered TSoundReadEvent (this way it allows streaming).

    • A method like TSoundFile.LoadMemoryStream calls LoadStream and converts the stream to TMemoryStream (if it is not TMemoryStream already). It uses ReadGrowingStream to move data from any TStream to TMemoryStream.

    Benefits:

    • I think we can avoid TStreamedSoundFile class this way. Instead, TSoundFile can be useful both for streaming and non-streaming needs.

    • We no longer need VorbisDecode. Instead, TSoundFile.LoadMemoryStream does the equivalent work in format-independent way.

@and3md

This comment has been minimized.

Copy link
Contributor Author

commented Aug 11, 2019

Sounds good, I think your solution will be better in long term (will be better with each added format, I like RegisterSoundFormat idea :). In my solution I wanted to make changes as similar as possible to the current code and not change current SoundFile so much (I'm still learning CGE code base, don't wanted to break too much things). So I was blind for some solutions...

Especially because this division to two classes, looks very clear for me. When you look at TSoundFile you know that it loads full data (there is Data, DataSize), and in streaming version there is only Read, Rewind. But unfortunately it comes with a price. So I think your solution will be better.

But high level changes are OK?
I think division to TSoundBufferBackendFromSoundFile and TSoundBufferBackendFromStreamedFile should stay.

They job is most important for me: When you have streamed file, you don't want to load anything at game start time (contextOpen) . This will be always waste of resources (and will kill android on activity change). Everything should be done in start playing (to be exact when buffer is added to sound source). TSoundBufferBackendFromStreamedFile cares about that.
In other hand when you want load full sound you don't want to make it on first usage but on game/level start (to be exact in contextOpen). TSoundBufferBackendFromSoundFile is for that.

I think TOpenALStreamBuffersBackend (TSoundBufferBackendFromStreamedFile descendant) is complicated enough to justify such a division.

BTW, Within one module, I was quite comfortable using private variables between classes. What is your view on this? Should I make more setters/getters?

@michaliskambi

This comment has been minimized.

Copy link
Member

commented Aug 11, 2019

I think division to TSoundBufferBackendFromSoundFile and TSoundBufferBackendFromStreamedFile should stay.

Sure, your explanation makes sense :) I'll be playing with the code, and for 90% leave them be.

BTW, Within one module, I was quite comfortable using private variables between classes. What is your view on this? Should I make more setters/getters?

It's cool for me. All classes in the same unit are "friends" in C++ sense (they can use each other's private sense), it's cool to use this feature.

One thing I would advise (I try to do in most new code) is to have different sections for strict private and private. This way, when reading class declaration, it's obvious what is really only used by this class (strict private) and what is used potentially by all classes in this unit (private). In most cases, most stuff will go to strict private. In my experience, this division helps me easily understand the code.

@and3md

This comment has been minimized.

Copy link
Contributor Author

commented Aug 11, 2019

One thing I would advise (I try to do in most new code) is to have different sections for strict private and private. This way, when reading class declaration, it's obvious what is really only used by this class (strict private) and what is used potentially by all classes in this unit (private). In most cases, most stuff will go to strict private. In my experience, this division helps me easily understand the code.

Ok, I will remember that in the new code :)

and3md and others added some commits Aug 11, 2019

Wrap OggVorbis loading in TOggVorbisStream
Makes code simpler, as

- TOggVorbisStream hides more internals,
- TOggVorbisStream exposes familiar TStream API.
- Also instead of VorbisDecode we can now just use
  ReadGrowingStream (extended to have configurable BufferSize).
Allow testing streaming in examples/audio/play_sounds
Also add trivial -dCASTLE_THREADS support.

michaliskambi added some commits Aug 13, 2019

Every sound format (OggVorbis, WAV) is now expressed as TSoundReadEvent
- Allow to register sounds by RegisterSoundFormat
- Sound is implemented once this way, and it is useful for both streaming and non-streaming
- Makes TSoundFile and TStreamedSoundFile much simpler, with simple constructor
Remove large and unused TSoundFile.DataStatistics implementation
(Maybe it will be restored some day, but for now it turned out to be
useless.)
FMOD supports streaming too, and some code cleanups
Improve some comments,
use "const" for parameter when possible,
move TSoundBufferBackendFromSoundFile lower (reflect the order of declaration).
Get correct Duration for OggVorbis sounds, even when streaming
Also:

- Fix ConvertTo16bit implementation

- CastleInternalVorbisDecoder now has more things internal (and is simpler), and calls RegisteredSoundFormat in initialization

@michaliskambi michaliskambi merged commit bb6c9b7 into castle-engine:master Aug 16, 2019

@michaliskambi

This comment has been minimized.

Copy link
Member

commented Aug 16, 2019

Merged! Sorry for taking so long, I invented a few things I wanted to add, and pushed them :)

Summary of the changes I did:

  • RegisterSoundFormat introduced, and Vorbis decoding expressed as TOggVorbisStream.

  • FMOD backend supports streaming too.

  • Also (completely unrelated to this PR, but I needed it to test FMOD on Debian testing) FMOD library is now loaded dynamically, in a smarter way (we can detect at runtime if it's not available, and it works with GNU binutils 2.32).

  • Reading Duration for streamed sounds works (it requires reading file header, which in case of OggVorbis contains the duration information, without the need to decompress whole file).

  • examples/audio/play_sounds/ has comments how to trivially activate streaming of all sounds.

  • Fix streaming of short sounds (that do not need 4 buffers), previously they were causing OpenAL errors.

Notes:

  • I made care to preserve the logic you mentioned, that merely loading a buffer with slStreaming should not do anything (not even reading the file header). The header is read on-demand when accessing various stuff (like Duration, after my changes).

  • Note that I did not merge the TSoundFile and TStreamedSoundFile classes, after all (even though I planned this). Working with them, I saw clearly what you already mentioned earlier -- they are similar in some regards, but they also need to do some things differently, and merging them into one didn't seem like something beneficial after all.

    However, I simplified whole implementation of CastleInternalSoundFile :)

Kudos for developing this!

This was a large change, and I saw better the reasons for some things (e.g. why TOpenALStreamBuffersSoundSourceRes.ContextOpen creates TStreamedSoundFile) when trying to fiddle with them :) A few times, I wanted to try something, only to realize that your solution is optimal, and I ended up merely adding comments:) Thank you!

@and3md

This comment has been minimized.

Copy link
Contributor Author

commented Aug 17, 2019

I thank you for finding time to introduce such big changes in such a short time. And you improved them a lot. I read all changes. Code looks great. CGE gained another important functionality :) I'm glad that I could help in this :)

michaliskambi added a commit that referenced this pull request Aug 17, 2019

Simplify a lot TOpenALStreamBuffersSoundSourceRes usage
There's no need to keep dictionary
source->TOpenALStreamBuffersSoundSourceRes at buffer. Instead, inside
TOpenALSoundSourceBackend we can keep a single reference to
TOpenALStreamBuffersSoundSourceRes.

This way TOpenALStreamBufferBackend is an empty class -- no need to do
anything, it's only a container for URL.

Also rename TOpenALStreamBuffersSoundSourceRes to simpler TOpenALStreaming.

See #127

and3md added a commit to and3md/castle-engine that referenced this pull request Aug 17, 2019

Simplify a lot TOpenALStreamBuffersSoundSourceRes usage
There's no need to keep dictionary
source->TOpenALStreamBuffersSoundSourceRes at buffer. Instead, inside
TOpenALSoundSourceBackend we can keep a single reference to
TOpenALStreamBuffersSoundSourceRes.

This way TOpenALStreamBufferBackend is an empty class -- no need to do
anything, it's only a container for URL.

Also rename TOpenALStreamBuffersSoundSourceRes to simpler TOpenALStreaming.

See castle-engine#127

and3md added a commit to and3md/castle-engine that referenced this pull request Aug 17, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.