[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Denemo-devel] fluidsynth
From: |
Richard Shann |
Subject: |
Re: [Denemo-devel] fluidsynth |
Date: |
Sun, 01 Nov 2009 23:00:50 +0000 |
I've pushed a small fix in fluid.c
Richard
On Sun, 2009-11-01 at 20:12 +0000, Richard Shann wrote:
> ... and I have now fixed the memory leak, as you can switch movements
> etc while playing back with fluidsynth or external player without a
> crash.
> Richard
>
> On Sun, 2009-11-01 at 20:05 +0000, Richard Shann wrote:
> > 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
> >
> >
> >
> > _______________________________________________
> > Denemo-devel mailing list
> > address@hidden
> > http://lists.gnu.org/mailman/listinfo/denemo-devel
>
>
>
> _______________________________________________
> Denemo-devel mailing list
> address@hidden
> http://lists.gnu.org/mailman/listinfo/denemo-devel