[fluid-dev] Re: FluidSynth event queue and thread safety changes

[David Henningsson]

address@hidden skrev:
> Hey David and list,

Hi there!

> I finally got around to getting my event queue and thread safe related
> FluidSynth changes cleaned up.  I merged with the latest trunk and
> created a branch in SVN.  I did this because it is a lot of significant
> changes to fluid_synth.c and fluid_chan.c and thought it would be the
> easiest way to collaborate and track the changes.  I didn't want to
> commit it just yet to the main branch, in case of serious issues.  We
> should merge it soon though, to prevent too much divergence.

It's great that you took the time to do so much refactoring as you have
done! I haven't looked into all the details of the change yet, but it
looks very good to me.

> There are some issues with the API that we need to deal with, to make
> FluidSynth thread safe.  Here is what I have on my list so far:
> 
> These public functions should be called only from within the synth thread:
> fluid_synth_nwrite_float
> fluid_synth_process
> fluid_synth_write_float
> fluid_synth_write_s16
> fluid_synth_alloc_voice
> fluid_synth_start_voice
> fluid_synth_get_voicelist
> 
> These functions can only be called if single threaded access is ensured
> (before synthesis starts for example):
> delete_fluid_synth
> fluid_synth_add_sfloader
> fluid_synth_set_midi_router
> 
> I don't think that should be a problem for existing software.

Neither do I. As long as we document it properly.

> TODO:
> NRPN and RPN system per MIDI source (could conflict currently)

I think I see your point, but
1) Do we even have the concept of "MIDI sources" inside the
synth/channel object?
2) Are we sure that this bug is not a feature, I mean, that it isn't
useful to keep it as it is?

> Make fluid_settings_t thread safe.

I think it is thread safe enough...? I mean, we only "set" settings in
the beginning, before threads are created, right? After that, to "get"
settings shouldn't change the object in any way, and there is no problem
in reading the same value from two threads at the same time.

> The following functions return instance pointers which could
> theoretically get freed at any time and are therefore currently
> problematic:
> fluid_synth_get_preset
> fluid_synth_get_channel_preset
> fluid_synth_get_sfont
> fluid_synth_get_sfont_by_id
> fluid_synth_get_sfont_by_name
> fluid_synth_find_preset

Is it really at *any* time? preset_t might get destroyed by MIDI events,
is that what you mean?

> Many fluid_synth_t functions make use of ->mutex locks, therefore they
> must not be recursively called by other functions when holding the same
> lock (deadlock).

Yes, that would be important to look through.

> There are likely some additional issues with some fluid_channel_t fields
> and some fluid_synth_* functions.  Things are likely better than they
> were though, in regards to thread safety!

Perhaps fluid_synth_handle_midi_event should call
fluid_synth_queue_midi_event instead of the current dispatching.

> Do let me know how I can help in regards to bringing you up to speed on
> my changes, etc.

You can start by checking in fluid_event_queue.* ;-)

// David