Re: [fluid-dev] Patch for fast midi file rendering

[David Henningsson]

Pedro Lopez-Cabanillas skrev:
> Hi,
> 
> I've not yet said so in appreciation of all the work you have done, and it 
> must be shout: Thank you very much! :-)

You're welcome :-)

And to give some praise back - I think the existing code is well written
and easy to understand. (Actually the only thing that has annoyed me so
far is the long main function in fluidsynth.c.)

> * Setting "player.timing-source". Bad name, and bad place. The same setting 
> would be used from the player and from the sequencer. But for me, it belongs 
> to the synth and the type is boolean. Either you trigger the timer callback 
> from the synth or you don't. That was my first proposal: synth.slave-timer = 
> yes/no.

OTOH, that sample timers is available in the synth does not make it
mandatory for the player/sequencer to use it.

But if you ask me; I think the system timers in the player/sequencer
should be skipped altogether and then no parameter would be needed at
all. Especially if the system timer solution has concurrency problems...?

> * Define a new "slave-timer" member of _fluid_synth_t, and initialize it 
> according to the corresponding setting in new_fluid_synth(). 

As we're deep into details and nomenclature here: IMHO it's time to skip
the words "slave timer" and use "sample timer" instead, as it says more
about what kind of timer it is.

> * fluid_sample_timer_process() is called unconditionally from 
> fluid_synth_one_block(). This would impact the usage of FS when there are no 
> timers needed (for RT-only clients) in a very sensitive place. Here you need 
> to use the setting value. An "if" is much less expensive than a "call".

I would say that performance-wise it does not matter, today's pipelined
and branch-predicted processors will pipeline the call as well. But if
you're worried, we could make compiler hints to inline the function.

> * fluid_renderer* is another bad name. It is not rendering anything, in the 
> sense of translation, like the verb used in "rendering SVG into bitmap 
> graphics". The proposed functions are only audio file helpers, because you 
> don't include the MIDI player into the scope. 

It's the synth that renders MIDI into samples, and the file_renderer is
a wrapper around the synth for doing just that, and then to write it to
a file. But I'm open for better suggestions.

> * new_fluid_file_renderer() has a parameter "period_size", that is available 
> as a setting. Instead, I would prefer to read synth->setting inside this 
> function. You may have access here to "fluid_synth.h" :-)

I guess that's a matter of taste. But for the sake of consistency, the
filename should be read from the settings then as well.

> * The raw audio format is not very useful by itself, but can be converted to 
> anything else, for instance to mp3 with lame. It would be nice to send the 
> output to stdout if the file name is not specified, so the output of FS can  
> be piped to lame.

I'm not sure if that would work? If you call "fluidsynth -F mybank.sf2
myfile.mid", it would assume that you want to output to mybank.sf2. So I
guess we'll have to stick to the somewhat messier "fluidsynth -F
/dev/stdout mybank.sf2 myfile.mid" if you want to pipe it further.

We must also make sure that fluidsynth does not make any noise on
stdout, but I don't know if that is a problem.

Anyway, if we have any good examples for rendering to wave/mp3/ogg/etc,
that could be written in the man page.

> * "fluid_seq.c" should be converted to this timer, as discussed recently.

Agreed, but I don't have a test environment for the sequencer here.
Either we leave this up to Antoine or I'll have to install a test
environment somehow. Anyway I prefer to have it in a separate patch.

// David