Re: [Denemo-devel] fluidsynth

[Richard Shann]

I have put in the check for si->smf being NULL when the callback
happens.
Richard

On Sun, 2009-11-01 at 16:14 +0000, Richard Shann wrote:
> 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   }
> 
> 
> 
> 
> 
> 
> _______________________________________________
> Denemo-devel mailing list
> address@hidden
> http://lists.gnu.org/mailman/listinfo/denemo-devel