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

[josh]

Quoting David Henningsson <address@hidden>:
> > 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.
>
> Event though I think the point is valid, I must say I have a bad feeling
> about implementing that. It just doesn't fit in well with the current
> design.
>
> It means we should move some state information from the synth out to
> some kind of "MIDI source", right? Or should the "MIDI Source" buffer
> some midi messages? When we then ask for that state information, should
> the MIDI source answer?


I think I changed my mind.  Lets just leave it the way it is for now :)


> > 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.
>
> If we're allowed to use glib, GStaticRWLock seems optimal for this usage.
>
> Another solution would be to restrict usage of settings structure from
> other threads than the "main" thread, but I haven't researched if this
> would mean lots of work or not.


Often times it is better to just use a regular mutex, since it is more  
efficient than a RWLock.  If there isn't a lot of lock contention and  
duration of locks is kept low, then just using a regular mutex is  
probably more efficient.  The settings aren't set/queried very often,  
so I think it is best to just use a regular mutex.

> > 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.
>
> Soundfont unloading can only happen through an API call (and then, the
> user should be smart enough not to reference a pointer he has deleted
> himself), or through the command shell.


It would be simple enough, if it was just a matter of the user not  
holding pointers.  But many FluidSynth functions access SoundFont  
instances, such as during program change MIDI events.  Perhaps we  
could add reference counting to SoundFont instances and all internal  
functions could atomically reference them when returning a SoundFont  
pointer from functions.  As long as a SoundFont has a reference, it  
wont actually be freed (even if unloaded).  This is starting to sound  
like GObject.


> > I'm reviewing the code in fluid_synth.c and fluid_chan.c currently to
> > check for other threading issues.
> >
> > Here is what I have so far:
> >
> > Assignment/read of fluid_preset_t on MIDI channels is problematic.
> > Preset selection (fluid_sfont_get_preset) should occur outside of the
> > synthesis thread, since it contains malloc calls and may perform other
> > types of locking depending on the SoundFont loader backend being used.
>
> >  The returned preset from fluid_sfont_get_preset has been allocated and
> > is owned by the caller, so it could be passed to the synthesis thread
> > for assignment to a channel via the queue, at which point the synthesis
> > thread (channel structure) would "own" the preset.
>
> I see now that program change midi messages are not moved into the queue
> but handled directly. That is bad, because it can reorder things
> (imagine a note-on queued, program change handled, the same note-on when
> it is processed will be affected by the program change that actually
> came after the note-on).
>
> If we can't do instrument selection in constant time (if we could, that
> would ease up things dramatically), we should prepare the new preset and
> then post a queue message that will just switch the preset (in constant
> time), to make sure nothing is reordered. Btw, this will also mean we
> can't post bank selection messages either...or at least not without
> buffering them separately as well.


I just committed changes which queue up preset assignment events.  I  
also combined the SoundFont ID, bank and program channel fields to a  
single integer, as I spoke of before.  This allows for atomic  
retrieval/assignment of these values.  From what I can tell, the  
synthesis thread only uses the preset.  The SoundFont ID, bank and  
program values are used with the various functions which ultimately  
select the preset (program change, bank change, SoundFont select,  
etc).  The parameters can be treated independently also, so they don't  
necessarily match that of the currently active preset.


> > A selected preset on a channel has other associated information with it,
> > including: SoundFont ID, MIDI Bank # and MIDI Program #.  Currently they
> > aren't atomic with respect to one another (so there is no way to
> > guarantee that the retrieved values aren't actually out of sync, due to
> > another thread switching the current instrument in the middle of
> > retrieving the values).  An atomic integer could be used though to store
> > all 3 fields.  7 bits for program #, 14 bits for bank # and the rest (11
> > bits, 2048 SoundFonts max) for SoundFont ID.  Atomic operations could
> > then be used for setting and retrieving this when a preset is assigned.
>
> Hah, that's the best motivation for using a 64-bit system I've ever
> heard ;-) Well, I don't mind limiting the number of SoundFonts to 2048,
>  and if it makes the implementation of fluid_synth_get_program easier,
> why not?
>
> > I'm going to remove a bunch of the documentation in the synth.h file,
> > which is already covered in the C code.
>
> As long as we have a way to separate the public API documentation from
> the rest, and we leave a pointer to that documentation in the synth.h
> file, I'm fine with that.


I think doxygen can be told which header files are public and extract  
the relevant documentation from the C source.  The documentation is  
much more useful to have in the C code, in my opinion.  Its more  
likely to be kept in sync, when it is right there with the function  
for which it applies.


> // David


The following API should be considered for addition to FluidSynth  
1.1.0, though perhaps some renaming is in order (I don't like just  
appending a 2).  These functions were meant to be made public at some  
point, as the comment suggests at the top of fluid_synth.c

fluid_synth_program_select2
fluid_synth_get_sfont_by_name
fluid_synth_set_gen2

I also think it would be good to make FLUID_OK and FLUID_FAILED  
public.  I think this should have been done at the beginning.  When  
writing code which should support older versions of FluidSynth,  
something like the following would need to be added:

#ifndef FLUID_OK
#define FLUID_OK         0
#define FLUID_FAILED     -1
#endif

What do you think?

Josh