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

One fluid synth per track #102

Closed
wants to merge 10 commits into from

Conversation

@swesterfeld
Copy link
Collaborator

swesterfeld commented Apr 23, 2019

This gets rid of all bse module locks in the fluid synth code, by using one fluid synth instance per track.

I observed that if we load the soundfont every time the user presses play, this can be slow (50ms to load fluid r2 * number of soundfont tracks), so this keeps the fluid synth instances even after the project stops playing, and re-uses the instances on play.

What this doesn't implement is changing the soundfont on a track while the project is playing (the gui doesn't let me do this anyway), but this could be implemented by preparing a new fluid_synth instance and exchanging it on the fly using an engine job.

I believe that this is the best/correct way to do it, better than #85

swesterfeld added 4 commits Apr 21, 2019
Signed-off-by: Stefan Westerfeld <stefan@space.twc.de>
Signed-off-by: Stefan Westerfeld <stefan@space.twc.de>
Signed-off-by: Stefan Westerfeld <stefan@space.twc.de>
Without free function, crashes could happen because process() was called after
the fluid synth instance was freed.

Signed-off-by: Stefan Westerfeld <stefan@space.twc.de>
swesterfeld added 5 commits Apr 23, 2019
Signed-off-by: Stefan Westerfeld <stefan@space.twc.de>
Signed-off-by: Stefan Westerfeld <stefan@space.twc.de>
Instead, cache & reuse fluid synth instance if possible

Signed-off-by: Stefan Westerfeld <stefan@space.twc.de>
We no longer use the sound font repo fluid synth instance for synthesis,
this simplifies the code.

Signed-off-by: Stefan Westerfeld <stefan@space.twc.de>
Signed-off-by: Stefan Westerfeld <stefan@space.twc.de>
@swesterfeld swesterfeld force-pushed the swesterfeld:one-fluid-per-track branch from a2451c6 to 85b667d Apr 30, 2019
@swesterfeld

This comment has been minimized.

Copy link
Collaborator Author

swesterfeld commented Apr 30, 2019

As requested, I've updated this branch so that it doesn't require fluidsynth >= 2.0.5.

@swesterfeld swesterfeld assigned tim-janik and unassigned swesterfeld May 2, 2019
Copy link
Owner

tim-janik left a comment

What this doesn't implement is changing the soundfont on a track while the project is playing
(the gui doesn't let me do this anyway), but this could be implemented by preparing a new
fluid_synth instance and exchanging it on the fly using an engine job.

The reason the GUI blocks instrument changes is that the BSE core hasn't had support for this yet. If soundfonts can implement changes during runtime, the associated, we'll of course release that restriction and make it user accessible. I guess that's better left for another PR, right?

@@ -12,6 +12,8 @@

#define parse_or_return bse_storage_scanner_parse_or_return

using std::string;

This comment has been minimized.

Copy link
@tim-janik

tim-janik May 21, 2019

Owner

We need to get rid of global using clauses. The std namespace defines lots of lower case symbols/types that are likely to clash with variable names in our code, so it's best to always use explicit prefixing (like std::string, std::max). For this case specifically, there's also Bse::String, so at least inside the Bse namespacse, String can simply be used. Also, using clauses are not as bad in inner scopes, like do { using Bse; String s; ... } while (...)

This comment has been minimized.

Copy link
@swesterfeld

swesterfeld May 21, 2019

Author Collaborator

Right, I've fixed this now by using std::string - I also tried using Bse::String - this doesn't have any advantage here, as all code is outside the Bse namespace.

@@ -186,12 +186,12 @@ bse_sound_font_reload (BseSoundFont *sound_font)
return bse_sound_font_load_blob (sound_font, sound_font_impl->blob, FALSE);
}

int
bse_sound_font_get_id (BseSoundFont *sound_font)
string

This comment has been minimized.

Copy link
@tim-janik

tim-janik May 21, 2019

Owner

Should be std::string.

@tim-janik tim-janik assigned swesterfeld and unassigned tim-janik May 21, 2019
@@ -387,10 +277,6 @@ SoundFontRepoImpl::~SoundFontRepoImpl ()
delete_fluid_settings (fluid_settings);
fluid_settings = NULL;

This comment has been minimized.

Copy link
@tim-janik

tim-janik May 21, 2019

Owner

It'd be better to merge the above two lines into delete_fluid_settings (&fluid_settings); where delete_fluid_settings automatically assigns NULL to the pointer passed in.

This comment has been minimized.

Copy link
@swesterfeld

swesterfeld May 21, 2019

Author Collaborator

delete_fluid_settings is part of the fluid synth api, so I cannot change what it does

Signed-off-by: Stefan Westerfeld <stefan@space.twc.de>
@swesterfeld swesterfeld assigned tim-janik and unassigned swesterfeld May 21, 2019
@swesterfeld

This comment has been minimized.

Copy link
Collaborator Author

swesterfeld commented May 21, 2019

The reason the GUI blocks instrument changes is that the BSE core hasn't had support for this yet. If soundfonts can implement changes during runtime, the associated, we'll of course release that restriction and make it user accessible. I guess that's better left for another PR, right?

I agree. As I understand it, updating beast-gtk is to be avoided, I suppose this should be done once ebeast supports assigning soundfont & preset with the new ui. The fluid code as it is supports changing the preset of an existing track during play. It doesn't support changing the SF2 filename of a track during play, but this could be implemented by swapping the fluid_synth_t instance in this case. Maybe that should be done after you have the new engine module api.

Anyway I think I've addressed the other issues now, will reassign this to you so you can merge it.

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