[Denemo-devel] fluidsynth

[Richard Shann]

I have merged in some changes to the fluidsynth work as it was not
building with fluidsynth disabled. This led me into looking at the new
code and I want to make some comments, the first of which is to say how
glad I am to see this work progressing.
There are three issues I have noticed
      * There was a serious memory leak (sorry, my fault), which I
        noticed because your code did not crash when deleting the score
        currently being played. I was expecting it to do so, because you
        have introduced a dangling pointer. I have fixed the memory
        leak, but surrounded it with #ifdef  DANGLING_SMF_POINTER_FIXED
        since it cannot be fixed without fixing the dangling pointer.
        More detail below.
      * There is the problem which you already have noted in a comment
        in fluid.c (trying to use the fluidsynth structure for a midi
        event). The structure is opaque and would need access via
        function calls to set its values.
      * You have included an extra timer which is needlessly complex.
        Instead of tracking ticks with g_timeout_add(2,
        tick_timer_callback, NULL); you should consult the time in the
        idle callback and decide whether or not to output the midi event
        directly there. That is, whenever the idle callback fires, look
        at the next event(s) and output them if it is time.

The important one is the dangling pointer. This is introduced by the
line:

  smf = Denemo.gui->si->smf;

there is no means of ensuring this value is not used after the memory it
points to has been freed. Instead, in the callback refer to
Denemo.gui->si->smf itself, checking that it is not NULL. If it is NULL,
then stop the playback and remove the idle callback.

This would mean that if someone changed movements or deleted something
etc, the idle callback would carry on playing the new movement from
wherever it had been left. We could, of course, take measures to turn
off the playback when that happens, but the main point is we don't have
to in order to keep the program structurally sound: the playback will
never attempt to play freed memory.

The second bullet point above needs more digging in the fluidsynth docs,
it looks like we *could* move to using fluidsynth in place of libsmf, it
would give smoother integration. However, I would be nervous of putting
our eggs in that basket too early, and it would require a whole chunk
more work. For now, perhaps it we can create the fluidsynth midi event
from the smf midi event without too much pain; if not we can write a
switch like the one I paste below, taken from their code for the midi
handle function.
Richard
fluidsynth code:
switch(type) {
03158       case NOTE_ON:
03159         return fluid_synth_noteon(synth, chan,
03160                                  fluid_midi_event_get_key(event),
03161                                  fluid_midi_event_get_velocity(event));
03162 
03163       case NOTE_OFF:
03164         return fluid_synth_noteoff(synth, chan, 
fluid_midi_event_get_key(event));
03165 
03166       case CONTROL_CHANGE:
03167         return fluid_synth_cc(synth, chan,
03168                              fluid_midi_event_get_control(event),
03169                              fluid_midi_event_get_value(event));
03170 
03171       case PROGRAM_CHANGE:
03172         return fluid_synth_program_change(synth, chan, 
fluid_midi_event_get_program(event));
03173 
03174       case CHANNEL_PRESSURE:
03175       return fluid_synth_channel_pressure(synth, chan, 
fluid_midi_event_get_program(event));
03176 
03177       case PITCH_BEND:
03178         return fluid_synth_pitch_bend(synth, chan, 
fluid_midi_event_get_pitch(event));
03179 
03180       case MIDI_SYSTEM_RESET:
03181         return fluid_synth_system_reset(synth);
03182   }