Re: [fluid-dev] Thread safety

[josh]

Quoting David Henningsson <address@hidden>:
> > I think ideas like these are good.  Having each voice be processed and
> > then mixed, would only require one buffer (64 bytes) per voice and
> > would not require much extra CPU.  This could also facilitate moving
> > to the multi-thread voice processing model.
>
> I guess you would need at least an audio-buffer-size of samples to make
> any difference in practice though. While this would add stability, it
> will also add some CPU consumption and increase memory usage (and
> bandwidth). So perhaps this would be something that should be enabled or
> disabled.


There probably wouldn't be any extra data copies, since FluidSynth is  
currently rendering each voice to a temporary buffer, which is then  
summed into the final buffer.  Perhaps rather than a buffer per voice,  
it could be a buffer per rendering thread.  In the single thread case,  
it would work pretty much like it is now.  If someone has multiple CPU  
cores, they could add additional threads (long term goal).


> > I've been thinking about what the ideal FluidSynth thread model would
> > be for lock-free (or as close to lock free as possible) and of course
> > crash free ;)
> >
> > Here are some initial thoughts, though perhaps faulty by design and
> > lacking completeness.
>
> Right, and we should not forget about the embedded / rendering case,
> which probably still would benefit from being single-threaded. (And
> also, they must be predictable.)


True.  I've been putting a bit more work into analysis of the  
FluidSynth code base (in particular fluid_voice.c and fluid_synth.c).   
I'm starting to dislike the idea of queuing every event to the  
synthesis thread, since that just adds more overhead.  I'm also  
realizing how complicated it is, to make FluidSynth truly thread safe,  
without locking in the audio thread.  One good thing though, is that  
the voice processing itself is self contained and does not rely on any  
variables outside of the FluidSynth voice instance.

The public fluid_voice_* functions could be restricted to only being  
usable from the synthesis thread.  The current biggest use of this API  
is for SoundFont loaders.  In particular, note-on events trigger the  
creation of voices.  If the SoundFont loader note-on callback was  
always executed from within the synthesis thread, then that would  
satisfy the restrictions.

For other publicly available functions, which are expected to be  
thread safe, events could be queued as we have been discussing.  For  
the MIDI thread, sequencer and player though, we might be able to  
convert them from being individual threads to being MIDI event  
processing callbacks which are executed from within the synthesis  
thread.

That is just at the idea stage though and I'm not sure yet what  
exactly this would entail and how it might affect the current  
FluidSynth API.

Queuing events for the non-synthesis thread case though, is not so  
trivial.  The current API exposes certain functions which complicate  
matters too.  I think in order to do things right, some API would need  
to be deprecated or its use restricted.  Having FluidSynth event  
related functions queue MIDI events, would probably be the easiest  
solution and could be processed exactly as other MIDI sources in the  
synthesis thread.


> > * Make active voices and voice pool private to the synthesis thread.
> >
> > * Parameter updates (MIDI events, etc) go through a lock free FIFO queue.
> >
> > * Voices are allocated outside of the synthesis process, initialized and
> > added to the FIFO queue for processing.
> >
> > * Note-off events are also appended to the event queue.
>
> Do I understand you correctly, that you want to treat note-on events
> differently from the rest of the events, based on the assumption that
> these events are the only that will take a large amount of time?


Good question.  I like the idea of the note-on processing being done  
in a separate thread from the synthesis thread, since as you say, it  
can entail a lot of CPU consumption.  If the synthesis thread was  
running high priority, versus MIDI note-on events, then it would have  
a tendency to throttle the MIDI note-ons when CPU consumption  
approaches maximum, which is good.  I'm not certain though, if this in  
and of itself warrants doing the note-on processing in a separate  
thread.


> > This would leave the main synthesis thread (and future helper threads)
> > to do the dirty work, without pesky meddling from other threads.
> >
> > The issue that I have run into, is that I haven't been able to find a
> > FIFO queue algorithm which allows multiple producers.
>
> I think that can be solved with one FIFO for every producer thread. That
> leaves us however with another problem of how to add and remove FIFO
> queues dynamically in a thread-safe way...


Yeah, I also came to that conclusion (one FIFO per thread).  Adding  
the FIFOs wouldn't be that difficult.  Just have a fixed size array  
(maximum number of threads), which is initialized with NULLs.  A  
GPrivate (private data per thread) could be used to assign a FIFO to  
the thread, the pointer to the FIFO could then be added to the array,  
by looping on it and using g_atomic_int_compare_and_exchange() on NULL  
values until it returns TRUE.

The synthesis thread could then just use g_atomic_int_get() when  
iterating over the array to process the FIFOs and stop at the first  
NULL (queues would never go away).


> > A lock free FIFO
> > with single producer and consumer is pretty trivial,
>
> Will these support resizing if the FIFO gets full? And if they don't, is
> that feature important?


No, resizing would not be possible.  It would just be set to a compile  
time maximum, which equates to maximum expected events per audio  
buffer.  I just implemented the lock-free queue code yesterday, using  
glib primitives, though untested.


> > > My idea was that if I commit my patch in its current state, at least we
> > > will have something more stable (fix for ticket #43). And I think the
> > > possibility to route midi events through the sequencer makes a valuable
> > > addition to the sequencer. And when we have improved the synth thread
> > > safety, we will simply revert the three lines in fluidsynth.c that
> > > inserts the sequencer in the chain. What do you think?
> > Sure, if it improves things in the short term, go ahead add it.  Fixing
> > FluidSynth's threading issues, and doing it right, is likely going to be
> > a bit of a larger task than doing simple fixes.  So it might be good to
> > try and address the more severe issues, while coming up with a long term
> > solution.
>
> In an open-source project where people can suddenly disappear without
> notice, my assumption is that taking a lot of small steps (while keeping
> it stable) is often better than taking one big step, even if those small
> steps sometimes means additional work.


True.  I'm not planning on disappearing at any point soon though and I  
hope you aren't ;)


> // David

Cheers!

Josh