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

[David Henningsson]

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

Okay.

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

Fluid_preset_t contains a pointer to the soundfont, so all channels must
be notified to delete their corresponding presets when a soundfont is
unloaded.
For the other short-term uses of fluid_sfont_t, we could let the synth
mutex include protection of all soundfont instances.

Either that or reference-counting - I don't know what's more messy.

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

I had a quick look and spotted two things:

1) Since the program-change message is not queued, we must also move the
handling of BANK_SELECT_MSB and BANK_SELECT_LSB from
fluid_synth_cc_LOCAL to fluid_synth_cc to prevent reordering.

2) This might be independently of your thread-safe changes, but doesn't
the three functions that set the preset (fluid_synth_program_change,
fluid_synth_program_select, fluid_synth_program_select2) look a bit too
different? Can they all be right? And why don't they call each other
(especially fluid_synth_program_select2 that should make an name to id
conversion and then call fluid_synth_program_select)?

> 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 see no reason not to make them public, so go ahead. (and choose a name
of your preference.)

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

FLUID_OK and FLUID_FAILED should be public. Perhaps we should even
redefine them if they are defined.

// David