[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Denemo-devel] fluidsynth
From: |
Richard Shann |
Subject: |
[Denemo-devel] fluidsynth |
Date: |
Sun, 01 Nov 2009 16:14:31 +0000 |
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] fluidsynth,
Richard Shann <=