Re: [fluid-dev] Making MIDI player read from a buffer

[David Henningsson]

On 2010-10-19 02:07, Matt Giuca wrote:
> Hi David, James, Pedro,
>
> Thanks for a quick discussion.
>
> David Henningson wrote:
>
> > ...although this would only be in fluid_midi.c, right? It wouldn't affect other 
> > files.
>
> Correct.
>
> > I'd personally prefer the client handling memory entirely. This has two 
> > advantages:
> >   * We won't have to do "free" in a context that might be real-time sensitive.
> >   * The client does not have to use the same memory allocator as fluidsynth.
>
> OK, I thought about this when I was typing my first email, but I
> realised there would be a major drawback in that without a garbage
> collector, the client would need to somehow know when it is safe to
> free the memory. As far as I can tell, you can really only be sure the
> fluid_player is finished with it if the entire playlist has ended, or
> you have manually stopped or deleted the fluid_player. Therefore if
> you were creating a playlist from some file-like objects, you would
> need to load them all into memory, store pointers to each of them
> somewhere, wait for the fluid_player to end, then free them all.

We could also copy the memory. While copying memory around is inoptimal, 
that might be a secondary concern. At least that would give us the most 
future flexibility.

> It seems in almost all cases I can think of, the client is going to
> want to allocate a buffer specially for FluidSynth to read, populate
> it from whatever file-like object it has in mind, and then pass it to
> FluidSynth, never to read the buffer (in the client) again. There
> would be a lot of overhead if all the clients had to do manual garbage
> collection on the buffers.
>
> As for the client having to use the same memory allocator, true, but
> as I said most clients will simply quickly allocate a buffer which is
> immediately passed to Fluid and forgotten about, so even if the client
> is using another allocator, it could just use malloc at this one part.
> For example, a C++ program would typically use new[] to construct
> arrays, but it would use malloc to allocate this particular buffer to
> pass to FluidSynth. (Though I agree it's not too nice for a library to
> free a data structure allocated by the client.)

If so, we should probably also add fluid_malloc / fluid_realloc / 
fluid_free as API additions to make sure the same memory allocator is 
used. After all, in a future version of FS, we might want to switch 
memory allocator, write our own for some reason, etc.

> I don't know much about real-time programming. But I imagine that if
> you are loading MIDI files into memory each time you hit a new
> playlist entry, you have lost your real-time-ness anyway (at least, in
> between songs), since you will be allocating a new internal data
> structure, reading a file from disk, and freeing the previous one.
> Does adding this free make matters any worse for real-time
> applications?

Not really, but this is more about not making API additions that will 
make it hard or impossible to make the MIDI player real-time safe in the 
future.

One hard part of FluidSynth is that it has so many use cases, and we 
can't fail a single one of them. What we can't do good enough today 
(IMHO), but what I would like to do, is good looping, which would be 
important for games, which would like to loop a background song 
seamlessly. It could also be that somebody wants to play along with a 
drum track (and the drum track is in the MIDI file), and so he adds the 
midi file 100 times. (Also player.reset-synth must be set to "no")

So to give you an overview - with the 1.1.2 architecture, while it 
improves some use cases, it doesn't do much to help out midi players.
The 1.1.2 architecture splits FS into two parts - one hard real-time 
safe part, which contains the rendering. So when you (or the audio 
drivers) call fluid_synth_write_s16/float, it should be guaranteed a 
quick return. This is to avoid underruns and the clicks that comes from 
it. This part is now in the rvoice directory.

To complicate matters, there are some MIDI synth events which we cannot 
do in a hard real-time safe way. So the MIDI engine - that transforms 
MIDI events into individual voices - is not real-time safe, and calls 
into that API are synchronized. Once the MIDI engine has completed the 
processing of a MIDI event, it stores commands to the rvoice engine in a 
queue.

Also important in this context is the system timer vs the sample timer. 
If we use the system timer, the problem with real-time safety is solved, 
because we have an additional thread checking the computer's system 
timer, calling into the API at predefined times. This however leads to 
another problem instead, which we called the "drunk drummer" problem a 
year or two ago. That is, we can't intercept a fluid_synth_write_s16 
call in the middle to insert a MIDI event, so all MIDI events become 
quantizised to the buffer size, causing bad timing.

So therefore the sample timer was invented, which enables callbacks from 
the middle of the rendering, which could in turn can call the MIDI 
player or sequencer to insert MIDI events appropriately. This is good 
for timing and reliability, and a must for 
faster-than-realtime-rendering, but bad for real-time/underrun safety.

So the situation is not optimal either way (and has never been). I've 
had loose thoughts about how to attack this problem, but not thought 
them through enough to start discussing an implementation.

So to sum up, I'd like our MIDI player to be real-time safe one day. 
It's a long way there, so right now I just want to make sure we don't 
walk in the wrong direction.

> A possible work-around for this is having fluid_player_add_mem take an
> extra argument of type void(*)(void*) (a function pointer which
> accepts a void* and returns void), which is called on the buffer when
> the player is done with it. This lets you specify your own
> de-allocation function (for example, C++ clients could pass a function
> which calls delete[] on a new[]-allocated array), or pass a function
> which does nothing, allowing clients to ignore the signal and perform
> their own memory-management. But the majority of clients could just
> pass 'free' as an argument, which means FluidSynth would simply free
> the memory with no further work on the part of the client. Or maybe
> that's going too far.

No, that's a solution I've thought of as well.

> > In addition, here are things that might or might not be real-time sensitive. 
> > This conversion sounds like time intensive.
>
> OK, again I don't know much about real-time programming,

Neither did I when I first started to be involved in this project.

> so I might
> have to get someone to check my code afterwards to let me know if I've
> "broken" some real-time-ness of FluidSynth.

The MIDI player isn't much of real-time safe currently, and writing 
real-time safe code is hard, so don't worry here. It is merely for the 
API addition I'm concerned at this point, because we can't easily 
refactor and/or drop them later on.

> > Here's a compromise:
> >
> > struct loadable_file
> > {
> >         char* filename;
> >         void* data;
> >         size_t data_length;
> > }
> >
> > (You don't need an enum because either one will be NULL.)
> >
> > But then, at file load time, if filename != NULL, load the file into the data 
> > and data_length fields, then do midi parsing, followed by freeing the data 
> > buffer again. Then we at least have only one midi file in memory at a time.
>
> OK that does seem like a good solution. Note however that before when
> you said "That is probably a good thing though," (referring to the
> fact that you get errors upon calling fluid_player_add rather than at
> load time), that is no longer the case -- with this design you would
> get file access errors as each file comes up in the playlist.

Correct.

> > The other end of it would be to run through all of the midi conversion 
> > functions inside fluid_player_add_mem. With all time intensive tasks done, we 
> > might be able to switch from one midi file to another very quickly (without 
> > risking underruns). That'd be nice, but I don't know about the memory 
> > consumption.
>
> You mean fully parse the MIDI file into track data in
> fluid_player_add_mem. I see, well I don't think I'll tackle that just
> yet, but I agree it could be an interesting trade-off (somewhat
> increased memory consumption, but having no overhead -- disk or memory
> -- in switching MIDI files).

This might be a future path, which means that is one more thing you 
shouldn't make harder or impossible.

> Well, garbage collection issues notwithstanding, I think I'll start
> implementing the "compromise" method you describe. I will share the
> code once I have something,

Sounds good to me. My job is here to keep you informed of all the 
thoughts and use cases here, in order for you to have them in mind when 
you write code. Hope you're up for the challenge :-)

// David