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

[josh]

Hello David,

Quoting David Henningsson <address@hidden>:
> > 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?

Good points.  It seems to me though, like a given MIDI source would be  
potentially doing its own NRPN/RPN messages and you wouldn't want them  
colliding with other sources.  So for a given RPN or NRPN operation it  
should be considered atomic, if possible.

> > 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.

Actually the settings can be assigned at runtime as well.  Not all of  
the settings are runtime capable though (its the purpose of the update  
callback for a particular setting).  The underlying hash table and  
string values need to be protected at least, if they are being  
changed.  I haven't fully looked into it, but a simple solution would  
be to just stick a mutex in the settings structure and use it in all  
the settings related functions.

> > 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?

True.  Not at *any* time.  Only if some other thread decides to unload  
a SoundFont.  Its just that there are no guarantees that it will  
remain and there is no policy of how long the caller might reference  
the given preset or SoundFont.  As long as no SoundFont unloading  
occurs, it should be fine.  Its still an issue though.


> > 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.


Yes, good idea.  Would clean up the code a little and remove some  
slight overhead.


> > 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.* ;-)

Ooops!  ;)  Done.


> // David

Best regards,
Josh