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

Sound Cleanup #5959

Merged
merged 6 commits into from
Nov 23, 2024
Merged

Sound Cleanup #5959

merged 6 commits into from
Nov 23, 2024

Conversation

WickedSmoke
Copy link
Contributor

@WickedSmoke WickedSmoke commented Nov 9, 2024

These commits cleanup the Sound and SoundMusic interfaces to allow plugging in an alternative backend (e.g. OpenAL) simply by switching out sound/Sound.cpp.

This pull request does not address the make file changes required to actually do this. However, a working proof of concept can be found on my repository.

Future work could include:

  • Adding a music crossfade method (to allow reducing the five mutex locks used in the SDL implementation down to one).
  • Moving common backend code to it's own file.

src/sound/Sound.cpp Outdated Show resolved Hide resolved
@Web-eWorks Web-eWorks self-requested a review November 13, 2024 18:07
@WickedSmoke
Copy link
Contributor Author

I have my sound back end working reasonably well, so I went ahead and did the crossfade consolidation.

@Web-eWorks
Copy link
Member

Will review this soon - been busy with some other concerns. What I'm reading so far seems to be moving in the right direction; I definitely appreciate moving the "fiddly details" of audio mixing entirely behind a higher-level API.

Do note that any proposed audio backend replacement would necessarily need to be able to handle 3d directional audio, so I have my eyes on something along the lines of soloud or an equivalent middleware - my understanding of OpenAL is that it is a somewhat heavyweight dependency.

@WickedSmoke
Copy link
Contributor Author

My immediate goal is to make switching out the backend easier so I can exercise my own audio library. After that I probably won't touch Sound again and I'm not going to advocate for any particular backend.

I did just build SoLoud to compare the shared library size. It weighs in at 751kB vs. the 1063kB for OpenAL that comes with my distro. SoLoud can use OpenAL as a backend, but it seems a bit dated as there is none for PulseAudio.

Copy link
Member

@Web-eWorks Web-eWorks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One minor issue I'd like you to address before merge. This is a general improvement of the Sound API by hiding the messy low-level details behind the API surface, which I'm very much a fan of.

src/sound/Sound.h Outdated Show resolved Hide resolved
}

return songs;
return GetMusicFiles();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is conceptually messy, but in the interest of not increasing the scope of this PR I'll consider this "not worse than before"; the compiler is very likely to perform RVO here, so it's not a significant performance issue.

 - Replace GetSamples() with GetMusicFiles().
MusicEvent differs from Event by only one line and is replaced by an
Event::PlayMusic method.
This cleans up lots of VolumeAnimate/SetOp calls and allows for future
optimizations (such as eliminating one mutex lock operation).
Consolidating all the stop/play/fade operations will allow the number of
mutex locks to be reduced in the future.
@Web-eWorks Web-eWorks merged commit 9949946 into pioneerspacesim:master Nov 23, 2024
4 checks passed
@WickedSmoke WickedSmoke deleted the sound_cleanup branch November 24, 2024 19:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants