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

[David Henningsson]

(This is a merged reply from two messages)

address@hidden skrev:
> 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.

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?

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

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.

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

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.

> 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 think the preset field of the MIDI channel should be private to the
> synthesis thread.  There are 2 public functions which deal with
> fluid_preset_t instances: fluid_synth_get_channel_preset and
> fluid_synth_start.  I don't think it will be an issue to restrict these
> to synthesis thread use only (from within the noteon handler for
> example), since I don't see why else they would be used.

That's fine with me.

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

// David