Implement mp3/ogg Background Music Player using SDL2-mixer #725
Conversation
…r/images if the gamelist didn't specify a path" This reverts commit cd2f2ee. temporarily reverted for performance reasons - RetroPie#381
update to use legacy thegamesdb url
…romfolder/images if the gamelist didn't specify a path"" This reverts commit 9ab5223.
… detection If filterTrigger detects a positive axis event on a common trigger axis while also configuring a trigger, the next input event will be a negative axis press (as the trigger needs to transition from >0 to rest at -32767). Filter this negative event or else the next item in the configuration dialog (typically "left thumb") will erroneously detect this as a separate event.
|
@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 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 SupervisedThinking/LibreELEC-RR@72bb547 and stll wokrs fine, tested with carbon & pixel theme. |
| getMusicIn(Utils::FileSystem::getHomePath() + "/RetroPie/roms/music", musics); | ||
|
|
||
| // check in system sound directory | ||
| if(musics.empty()) |
There was a problem hiding this comment.
Is this an actual RetroPie folder? Wouldn't it probably something closer to /opt/RetroPie/music? Or is it a non-pi specific folder?
There was a problem hiding this comment.
Audio files can be placed in $HOME/RetroPie/roms/music, /usr/share/RetroPie/music, or /.emulationstation/music.
Other folders can be added too, so whatever folder we want to make it is a simple change.
There was a problem hiding this comment.
Correct. What I'm saying is that "/usr/share/XXX" is a Batocera path :) RetroPie doesn't create that folder by default - the path you're probably looking for here is "/opt/retropie/music" then. @joolswills thoughts?
There was a problem hiding this comment.
Doesn't this mean that if there is an mp3 or ogg in "$HOME/RetroPie/roms/music" it will ignore any files in the other directories?
|
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. |
|
Thanks. It just occurred to me that, while I understand the issue with video snaps and their audio, I imagine, that if you're using a video screensaver (or image screensaver with audio tracks), this should stop and then restart when we exit the screensaver. I saw the code for that when launching a game, but didn't seem to spot that upon my first review. Thoughts on that? |
bluestang2006
force-pushed
the
bgm_pr
branch
4 times, most recently
from
ae2be2f to
56e9618
Compare
I pushed new commits that revert those changes back to the original code because at the end of the day I agree we should not introduce new code without good justification. TBH I can't justify it other than just copying over Batocera code that I thought at the time would be relevant during my initial backport.
I fixed /usr/share/RetroPie/music to /opt/RetroPie/music, I suppose a follow-on PR is needed for RetroPie-Setup to create the music folders during a build/install.
Just brainstorming this, it can be either...there can be an option to continue to play music or stop it. I'm also thinking that the user should be able to select via a slider how many secs they want the music fade for...right now its hard coded for 2 secs (2000 ms). It also occurred to me that the RetroPie image does not use/install PulseAudio by default? I believe that is going to be an issue with anyone that is using the KMS driver (the new default btw) because it can't hw mix multiple audio streams. So a sound server is a must - i.e. PulseAudio. |
- Add a background music player using SDL2 mixer - Backport various fixes
- Volume updates as slider moves
- Separate slider that controls the music volume
original code is fine to use as-is
original code is fine to use as-is
Fix build error
Let's use the correct default folders that RetroPie uses.
Thanks.
I don't think that'd be required - I don't think that the other screensaver folders are created either, but I wouldn't oppose it. It'd probably be a PR for https://github.com/RetroPie/RetroPie-Setup/blob/master/scriptmodules/supplementary/emulationstation.sh I'd imagine. It'd also be here that one could install pulseaudio if required, but I'll leave it to @cmitu and others as I am not an expert on what gets installed or not.
I wouldn't want to delay the PR more than needed - I think it's perfectly fine to have that as a subsequent PR if needed. In this case, the only thought I had was a simple (pseudo-code): Let me know when you are comfortable with the changes. I'll also tag @tomaz82 as he also has a different perspective on things here and might spot something that I missed, but other than that I'm happy to test it and merge it. Thank you! |
Yes, I'm aware of that - will impact all audio clients on the system, including EmulationStation. Thanks. |
|
old grumpy man has been summoned These 3 combined makes me assume (I could be totally wrong tho) that the author of the PR doesn't have full knowledge of what the code does, thus back to my first point. I agree with the addition of a BGM player, I disagree with the execution of this PR. |
If you read the whole comments I don't got the intention that he doesn't know how it works & what he does.
Well it just looks like it needs a simple rebase. Since this code waits for over a year for a merge nobody should be surprised about that.
Again read the whole conversation, he added or modified the functionality which probably requires knowledge about the code.
I'm using this PR with several ES versions in my build since the PR was opened, it works very well so if you have objections - what about having a look at the code and ask for specific changes to get this thing finally in a "proper" state? |
| #define PROGRAM_VERSION_STRING "2.11.0rp-dev" | ||
| #define PROGRAM_VERSION_MINOR 10 | ||
| #define PROGRAM_VERSION_MAINTENANCE 2 | ||
| #define PROGRAM_VERSION_STRING "2.10.2rp" |
There was a problem hiding this comment.
Why is the version changed?
|
|
||
| #define PROGRAM_BUILT_STRING __DATE__ " - " __TIME__ | ||
|
|
||
| #define RESOURCE_VERSION_STRING "2,11,0\0" | ||
| #define RESOURCE_VERSION_STRING "2,10,2\0" | ||
|
|
There was a problem hiding this comment.
Why is the version changed?
| @@ -321,7 +363,6 @@ int VolumeControl::getVolume() const | |||
| { | |||
| LOG(LogError) << "VolumeControl::getVolume() - Failed to get master volume!"; | |||
| } | |||
|
|
|||
| { | ||
| //check if an AudioManager instance is already created, if not create one | ||
| if (sInstance == nullptr && Settings::getInstance()->getBool("EnableSounds")) { |
| for(unsigned int i = 0; i < sSoundVector.size(); i++) | ||
| // Open the audio device and pause | ||
| if(Mix_OpenAudio(44100, MIX_DEFAULT_FORMAT, 2, 4096) < 0) | ||
| LOG(LogError) << "MUSIC Error - Unable to open SDLMixer audio: " << SDL_GetError() << std::endl; |
There was a problem hiding this comment.
Not sure why the error message says "MUSIC Error"
| getMusicIn(Utils::FileSystem::getHomePath() + "/RetroPie/roms/music", musics); | ||
|
|
||
| // check in system sound directory | ||
| if(musics.empty()) |
There was a problem hiding this comment.
Doesn't this mean that if there is an mp3 or ogg in "$HOME/RetroPie/roms/music" it will ignore any files in the other directories?
| @@ -286,7 +288,7 @@ void Settings::processBackwardCompatibility() | |||
| } \ | |||
| void Settings::setMethodName(const std::string& name, type value) \ | |||
| { \ | |||
| mapName[name] = value; \ | |||
| mapName[name] = value; \ | |||
|
|
||
| static bool DebugText; | ||
| static bool DebugImage; | ||
| static bool DebugGrid; |
| std::map<std::string, bool> mDefaultBoolMap; | ||
| std::map<std::string, int> mDefaultIntMap; | ||
| std::map<std::string, float> mDefaultFloatMap; | ||
| std::map<std::string, std::string> mDefaultStringMap; |
| return; | ||
|
|
||
|
My personal opinion (and what I say will always be my personal opinion, not the RetroPie teams opinion) is that this PR would be much easier to handle if it has been split in two, one for changing ES to use SDL_Mixer instead of SDL_Audio, then another ontop to add background music support. It is always more problematic to reivew a PR that changes too many things at once. With that said, if my concerns are addressed and the commit history cleaned with a rebase/squish then I'm all for merging this. |
|
Well any chance that this will ever get some attention, clean-up und will finally be merged? |
|
@tomaz82 @bluestang2006 Could you please bump the PR based on this & see if this could finally be merged? 🤔 |
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:
Issues:
The fade-out needs some work, it doesn't seem to work as intended.-- 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.