EmulationStation
EmulationStation copied to clipboard
RetroPie
Implement mp3/ogg Background Music Player using SDL2-mixer
This PR integrates a background music player in RetroPie's EmulationStation that uses SDL2-mixer. As is, it will play .mp3 and .ogg audio files in random order from 3 different directories. More extensions could be added but for simplicity, the 2 extensions should suffice for now. This PR mostly follows what was implemented in Batocera. I'm submitting this as a draft because it needs thorough testing and review, and any suggested cleanups where necessary. Overall this will eliminate the need for RetroPie users to run a script outside of EmulationStation, possibly saving CPU and memory resources.
Notes:
- Music can be enabled/disabled via the Sound Settings Menu.
- Audio files can be placed in $HOME/RetroPie/roms/music, /usr/share/RetroPie/music, or /.emulationstation/music.
- The Audio Volume slider was tweaked so that the volume dynamically changes as the slider moves.
- A separate Music Volume slider has been added (6 Mar) to control the BGM volume. The Audio Volume still controls the overall volume of the audio device.
- Requires sdl2-mixer-dev as a dependency in the emulationstation.sh build script.
Issues:
- ~~The fade-out needs some work, it doesn't seem to work as intended.~~
- This does not pause/unpause the song during gameplay. It will start a new song when exiting an emulator. -- Upstream SDL2-mixer has incorporated the Mix_GetMusicPosition function which can be used with other functions to resume playback at the position it left off at. This needs more investigating.
- Again this needs thorough testing. I've only been able to test on a Pi4.
I have fixed the fade-out, it is set up for 2 seconds. At some point, it would be good to make this user adjustable.
Thanks for this.
I've only commented on style related issues. In a couple of cases the original files have some bad formatting but it's best to keep whitespace changes of existing code separate so the functional changes are as minimal and clear as possible.
Also if this is ported across from a fork, I would be keen to make sure we have accurate attribution to the authors, and preferably, their blessing to include it.
Thanks for taking the time to review and I appreciate the comments. I'd still love for some testing by others to see if I broke something or if there are outstanding issues.
Added the original authors of the BGM code in the CREDITS.md file. They both have blessed the use of the code.
Can somebody point me to the line of code where I could hardcode or overwrite the volume of the background music? I set SDL_AUDIODRIVER=pulseaudio or SDL_AUDIODRIVER=alsa anyway & the built in volume slider, audio card & volume control device never worked anyway for me so I've patched them out:
https://github.com/SupervisedThinking/LibreELEC-RR/blob/master-rr/packages/supervisedthinking/emulation/frontends/Emulationstation/emulationstation/patches/emulationstation-999.07-sound-menu.patch
I've backported the patch to stable v2.9.4 which works great so far just the music is a bit too loud for my liking. So can I just change some code to set it to 75% or something? 🤔
Can somebody point me to the line of code where I could hardcode or overwrite the volume of the background music? I set
SDL_AUDIODRIVER=pulseaudioorSDL_AUDIODRIVER=alsaanyway & the built in volume slider, audio card & volume control device never worked anyway for me so I've patched them out:https://github.com/SupervisedThinking/LibreELEC-RR/blob/master-rr/packages/supervisedthinking/emulation/frontends/Emulationstation/emulationstation/patches/emulationstation-999.07-sound-menu.patch
I've backported the patch to stable v2.9.4 which works great so far just the music is a bit too loud for my liking. So can I just change some code to set it to 75% or something?
The volume of the background music is the same volume as all other sound outputs. I'm not sure why your volume slider is working, it should increase/decrease as the slider moves dynamically.
Are you using this specific branch for building EmulationStation, if no all bets are off to assist?
Lines 18-23 of your patch are the volume slider controls.
As I said the volume sliders don't work & never worked for me - I'm not using Retropie but LibreELEC which does not use amixer or something like https://retropie.org.uk/docs/Sound-Issues/ described here.
I build the stable branch https://github.com/RetroPie/EmulationStation/tree/stable & backported some patches & adjusted some menues for my retro stuff but basically it's vanilla ES beside that. The background music itself plays fine, no problem with that.
I've tested it again without the patch & it does no change the volume at all, well at least at my system. Which volume does the slider change? The system volume or does it change the volume of sdl_mixer?
As I said the volume sliders don't work & never worked for me - but I'm not using Retropie but LibreELEC which does not come with amixer or something like https://retropie.org.uk/docs/Sound-Issues/ described here.
I build the stable branch https://github.com/RetroPie/EmulationStation/tree/stable & backported some patches & adjusted some menues for my retro stuff but basically it's vanilla ES beside that.
RetroPie forces some audio settings through cmdline.txt but I just use a standard 64-bit RPiOS personally without those settings. Using Default for Audio Card and Master for Audio Device works for me.
I believe LibreELEC uses Alsa and Pulse? I know it was running a higher version of the Alsa library because of the KMS drivers.
LibreELEC itself uses ALSA basically exclusively because Kodi is the primary & only client which uses the ALSA device. Pulseaudio is also running but doesn't lock the device like it would do on usual distros & is only used for Bluetooth devices.
I use some scripts to shut down & create pulse audio sinks which works fine for ES otherwhise. I just can't change the volume. The audio settings are tailored for RPis if you look at these entries https://retropie.org.uk/docs/Sound-Issues/#step-2-choosing-the-audio-card-in-emulationstation Anyway my question is not how I maybe could use the sliders because I doubt they'll work with PulseAudio. Beside that I'm not running the distro on RPis currently but on a Generic Intel, Rockchip RK3399 & Amlogic A311D build.
If I understoof the documentation correct https://www.libsdl.org/projects/SDL_mixer/docs/SDL_mixer_frame.html the changing this var should alter the volume https://github.com/RetroPie/EmulationStation/pull/725/files#diff-3fb64ca7000b4851153abfc272874e3150965756456e71aed804189df5880378R75 so I guess there must be a way to hardcode the volume which is passed to PulseAudio by SDL?
LibreELEC itself uses ALSA basically exclusively because Kodi is the primary & only client which uses the ALSA device. Pulseaudio is also running but doesn't lock the device like it would do on usual distros & is only used for Bluetooth devices.
I use some scripts to shut down & create pulse audio sinks which works fine for ES otherwhise. I just can't change the volume. The audio settings are tailored for RPis if you look at these entries https://retropie.org.uk/docs/Sound-Issues/#step-2-choosing-the-audio-card-in-emulationstation Anyway my question is not how I maybe could use the sliders because I doubt they'll work with PulseAudio. Beside that I'm not running the distro on RPis currently but on a Generic Intel, Rockchip RK3399 & Amlogic A311D build.
If I understoof the documentation correct https://www.libsdl.org/projects/SDL_mixer/docs/SDL_mixer_frame.html the changing this var should alter the volume https://github.com/RetroPie/EmulationStation/pull/725/files#diff-3fb64ca7000b4851153abfc272874e3150965756456e71aed804189df5880378R75 so I guess there must be a way to hardcode the volume which is passed to PulseAudio by SDL?
Yes, setting the specific variable mIntMap["MusicVolume"] = 128; should alter the volume. MIX_MAX_VOLUME as defined here has a max value of 128, there is separate function that converts to 0-100 scale.
Yes, setting the specific variable mIntMap["MusicVolume"] = 128; should alter the volume. MIX_MAX_VOLUME as defined here has a max value of 128, there is separate function that converts to 0-100 scale.
Sounds Good, Doesn't Work I already tried to set it to 64 which should have halfed the volume but I did not had any result 🤔
As I said I suspect the slider, or whatever, is not actually changing the volume of SDL or ES or the mixed volume but rather the volume of the ALSA device.
Yes, setting the specific variable mIntMap["MusicVolume"] = 128; should alter the volume. MIX_MAX_VOLUME as defined here has a max value of 128, there is separate function that converts to 0-100 scale.
Sounds Good, Doesn't Work I already tried to set it to 64 which should have halfed the volume but I did not had any result
As I said I suspect the slider, or whatever, is not actually changing the volume of SDL or ES or the mixed volume but rather the volume of the ALSA device.
I had to re-look at the code, and the variable I mentioned earlier is not used. It was part of the backport that I did not implement.
The volume level is being set here That is where you need to start.
I'll need to take another whack at implementing a separate volume slider for the BGM. For now, it is being controlled by the Audio Volume slider in ES.
Well I guess if volume->setValue((float)VolumeControl::getInstance()->getVolume()); would work for me the slider have had already altered the volume, shouldn't it?
I've seen that there is already a second slider & TODO... I've uncommented it and gave it a try but also no luck.
Well I guess if
volume->setValue((float)VolumeControl::getInstance()->getVolume());would work for me the slider have had already altered the volume, shouldn't it?I've seen that there is already a second slider & TODO... I've uncommented it and gave it a try but also no luck.
What happens if you set the value to 64 instead of MIX_MAX_VOLUME?
https://github.com/RetroPie/EmulationStation/pull/725/commits/48d394d8f344792d890bd2716e9b5e58a32b036b#diff-41933b5f545e51724b5efc00ecb2dbf8ff22b0b10e23ed946f8fd79718f0ccdaR14
I tried AudioManager::AudioManager() : mInitialized(false), mCurrentMusic(nullptr), mMusicVolume(64) without success.
Can somebody point me to the line of code where I could hardcode or overwrite the volume of the background music? I set
SDL_AUDIODRIVER=pulseaudioorSDL_AUDIODRIVER=alsaanyway & the built in volume slider, audio card & volume control device never worked anyway for me so I've patched them out:
have you checked the Batocera fork? the Music Volume slider works on EmuELEC and its basically the same as LibreELEC https://github.com/batocera-linux/batocera-emulationstation
I tried
AudioManager::AudioManager() : mInitialized(false), mCurrentMusic(nullptr), mMusicVolume(64)without success.
I just added the separate music volume slider. Get rid of your patches in the Settings.cpp file and see if it works for you.
Your new music slider works 🥇 it alters the volume while adjusted, I guess just as intended.
If your PR gets merged it would probably a sane approach to drop all other sound config & adjustment stuff and let SDL & SDL_mixer deal with volume and else 🤔
Can somebody point me to the line of code where I could hardcode or overwrite the volume of the background music? I set
SDL_AUDIODRIVER=pulseaudioorSDL_AUDIODRIVER=alsaanyway & the built in volume slider, audio card & volume control device never worked anyway for me so I've patched them out:have you checked the Batocera fork? the Music Volume slider works on EmuELEC and its basically the same as LibreELEC https://github.com/batocera-linux/batocera-emulationstation
Well I've never used the Batocera fork but since it relies on SDL mixer it might also uses SDL to adjust volume? This implementation obviously works flawless & IMHO is a cleaner approach than altering the system volume.
If you think about any other situation you usually change the volume of a youtube tab or in Spotify but not the system volume.
@bluestang2006 after some testing I was wondering if you could improve the randomization a bit to never play the same song twice to avoid repeating songs?
https://github.com/RetroPie/EmulationStation/pull/725/files#diff-41933b5f545e51724b5efc00ecb2dbf8ff22b0b10e23ed946f8fd79718f0ccdaR174-R180
@bluestang2006 thank you for the work here. You keep doing on and off changes here, so let us know when this is ready for review and we can work on it at the time.
Thanks.
Latest master https://github.com/RetroPie/EmulationStation/commit/c612f23b03c8583dce479039afcc83a2cc242049 fails to compile for me:
FAILED: es-app/CMakeFiles/emulationstation.dir/src/main.cpp.o
/mnt/dev/LibreELEC-RR/build.LibreELEC-Generic.x86_64-10.0-devel/toolchain/bin/x86_64-libreelec-linux-gnu-g++ -DUSE_OPENGL_14 -I/mnt/dev/LibreELEC-RR/build.LibreELEC-Generic.x86_64-10.0-devel/toolchain/x86_64-libreelec-linux-gnu/sysroot/usr/include/freetype2 -I/mnt/dev/LibreELEC-RR/build.LibreELEC-Generic.x86_64-10.0-devel/toolchain/x86_64-libreelec-linux-gnu/sysroot/usr/include/SDL2 -I/mnt/dev/LibreELEC-RR/build.LibreELEC-Generic.x86_64-10.0-devel/toolchain/x86_64-libreelec-linux-gnu/sysroot/usr/include/vlc -I../external -I../es-core/src -I../es-app/src -march=x86-64 -m64 -mmmx -msse -msse2 -mfpmath=sse -Wall -pipe -flto -fno-fat-lto-objects -O2 -fomit-frame-pointer -std=c++11 -Wno-attributes -O2 -O3 -DNDEBUG -MD -MT es-app/CMakeFiles/emulationstation.dir/src/main.cpp.o -MF es-app/CMakeFiles/emulationstation.dir/src/main.cpp.o.d -o es-app/CMakeFiles/emulationstation.dir/src/main.cpp.o -c ../es-app/src/main.cpp
../es-app/src/main.cpp: In function 'int main(int, char**)':
../es-app/src/main.cpp:390:4: warning: left operand of comma operator has no effect [-Wunused-value]
390 | SDL_JOYAXISMOTION, SDL_JOYBALLMOTION, SDL_JOYHATMOTION, SDL_JOYBUTTONDOWN, SDL_JOYBUTTONUP,
| ^~~~~~~~~~~~~~~~~
../es-app/src/main.cpp:390:42: warning: right operand of comma operator has no effect [-Wunused-value]
390 | SDL_JOYAXISMOTION, SDL_JOYBALLMOTION, SDL_JOYHATMOTION, SDL_JOYBUTTONDOWN, SDL_JOYBUTTONUP,
| ^~~~~~~~~~~~~~~~
../es-app/src/main.cpp:390:60: warning: right operand of comma operator has no effect [-Wunused-value]
390 | SDL_JOYAXISMOTION, SDL_JOYBALLMOTION, SDL_JOYHATMOTION, SDL_JOYBUTTONDOWN, SDL_JOYBUTTONUP,
| ^~~~~~~~~~~~~~~~~
../es-app/src/main.cpp:390:79: warning: right operand of comma operator has no effect [-Wunused-value]
390 | SDL_JOYAXISMOTION, SDL_JOYBALLMOTION, SDL_JOYHATMOTION, SDL_JOYBUTTONDOWN, SDL_JOYBUTTONUP,
| ^~~~~~~~~~~~~~~
../es-app/src/main.cpp:391:2: error: expected primary-expression before 'const'
391 | const Uint32 event_list[] = {
| ^~~~~
../es-app/src/main.cpp:395:22: error: 'event_list' was not declared in this scope; did you mean 'va_list'?
395 | for(Uint32 ev_type: event_list) {
| ^~~~~~~~~~
| va_list
@bluestang2006 you might have a moment to have a look at this problem?
@pjft @bluestang2006 any chance that this will be properly rebased, build fixed & merged? It works great with ES 2.10.y-dev but unfortunately not with latest stable builds. It's broken after https://github.com/RetroPie/EmulationStation/commit/fe650965a0544cc6aae97efb6a608e8b97ee7322 as far as I can tell this branch works for me https://github.com/SupervisedThinking/EmulationStation/tree/up_bgm
I'll defer to @bluestang2006 in terms of the PR's current status - last time I said I was fine with merging, but since it was in active development I had asked for confirmation when it was stable so we could test and consider merging.
I'm still happy with doing so, assuming the PR is final and rebased.
@SupervisedThinking is your branch a rebase of this code?
@pjft @SupervisedThinking OK, I just fixed the issue...looks like I had bad merge somewhere along the way and some code made it the wrong line.
I have not tested this branch in long time and it needs to be tested thoroughly. This is a good solution as-is but it could use improvements as requested - i.e. better music randomization.
@pjft yes it was a rebased branch, the refreshed PR also applies & works fine for me now @bluestang2006 I'm using your patch since you've opened the PR and as far as I can say it worked fine. I'm not aware of all changes since the last working 2.10-dev commit I was using but I guess few, maybe none, touched anything regarding the sound output?
All in all it's a really cool addition to ES - having bgm while browsing through the game libs is quite nice 👍🏻
@bluestang2006 Thanks for the update, this is helpful.
@SupervisedThinking could you test the code in this PR as is, then, just to make sure we're working under the same code base and report back? It will help with merging it.
@pjft I've bumped my rebased patch to the rebased PR https://github.com/SupervisedThinking/LibreELEC-RR/commit/72bb54749fb4cead7aea21ce1c3e3c7e0c6d196f and stll wokrs fine, tested with carbon & pixel theme.
Thanks @bluestang2006 for putting this together. Overall from reading through the code I am for the most part comfortable with the changes.
There seem to be a few changes in the code - in VolumeControl and Settings, specifically, there seem to be some changes that, while positive, aren't clear if they are necessary for these changes or if they just came along because that was how the Batocera port was working. Specifically the get function returning a default value in Settings, and some of the changes in finding the right mixer device in VolumeControl, including the "if things go wrong, try to find a proper mixer".
Once again, I don't oppose the changes in principle, but it isn't clear to me if they're necessary for this to work (and if so, what exceptions are they addressing that could probably be addressed beforehand) vs a nice-to-have that comes with the code.
It's minor, but I just wanted to ask about it. I asked about a path as well, but once again, that's not an issue per se, more making sure that we use the right paths for RetroPie.
Would appreciate others' feedback.
Thanks @bluestang2006 for putting this together. Overall from reading through the code I am for the most part comfortable with the changes.
There seem to be a few changes in the code - in VolumeControl and Settings, specifically, there seem to be some changes that, while positive, aren't clear if they are necessary for these changes or if they just came along because that was how the Batocera port was working. Specifically the get function returning a default value in Settings, and some of the changes in finding the right mixer device in VolumeControl, including the "if things go wrong, try to find a proper mixer".
Once again, I don't oppose the changes in principle, but it isn't clear to me if they're necessary for this to work (and if so, what exceptions are they addressing that could probably be addressed beforehand) vs a nice-to-have that comes with the code.
It's minor, but I just wanted to ask about it. I asked about a path as well, but once again, that's not an issue per se, more making sure that we use the right paths for RetroPie.
Would appreciate others' feedback.
I can dig into this further this weekend, but the code was mostly a backport of Batocera implementation. I'm open to changing the PR to what makes sense for RetroPie.