Re: [fluid-dev] Qsynth broken with FluidSynth 1.1.0

[Rui Nuno Capela]

address@hidden wrote:
> Quoting Rui Nuno Capela <address@hidden>:
>> address@hidden wrote:
>>>
>>> I notice that QSynth makes use of fluid_synth_get_channel_preset().
>>> There have been a number of thread safety issues with FluidSynth, many
>>> of which have been fixed in 1.1.0.  The issue with that particular
>>> function, is that presets are allocated when they are assigned to a
>>> channel and freed when another preset is assigned (a program change for
>>> example).  This means that currently there are no guarantees that the
>>> preset returned by fluid_synth_get_channel_preset() will be valid, in a
>>> multi-thread environment.
>>>
>>> I'd like to resolve this issue.  The ideal solution would be to add
>>> reference counting to presets, but this would require an additional API
>>> function (fluid_synth_ref_channel_preset() and fluid_preset_unref() for
>>> example).  Another option would be to add a function to query the
>>> current preset info, without having to retrieve the preset, something
>>> like:
>>> fluid_synth_channel_preset_info (synth, chan, &name, &bank, &program);
>>>
>>> Trying to get fluid_synth_get_channel_preset() to be thread safe, isn't
>>> so straightforward though, since there is no way for FluidSynth to know
>>> how long the calling application will access the preset.  Its documented
>>> as a "low level access" function, so I was considering just making it a
>>> function restricted to synthesis context (from a note-on method or in
>>> the audio driver) and providing other methods of safely retrieving the
>>> preset info.
>>>
>>> By the way, this issue is nothing new.  Older versions of FluidSynth
>>> could potentially crash QSynth if a program change occurred at the right
>>> moment.
>>>
>>> Any ideas or preferences?
>>>
>>
>> a variation of what you suggested would be fine,
>>
>> fluid_preset_info_t info;
>> fluid_synth_get_channel_preset_info(synth, chan, &info);
>>
>> seems to be a solution. the returned new struct fluid_preset_info_t
>> would have bank, num, name and sfont as public api accessible members.
>>
>> then we could throw fluid_synth_get_channel_preset() into the deep low
>> deprecation abyss :)
>>
>> cheers
>> -- 
>> rncbc aka Rui Nuno Capela
>> address@hidden
>>
> 
> 
> Sounds good.  How about adding a size field to, so that there can be
> potential future extensions to fluid_preset_info_t.  Example:
> fluid_preset_info_t info;
> fluid_synth_get_channel_preset_info(synth, chan, &info, sizeof (info));
> 

omg, that additional size argument just raises from the grave all too
many windows api'esque nightmares. i'll pretend i did not read that and
close my eyes if you really think it's necessary :))

one possible and alternative solution is about having the
fluid_preset_info_t as an opaque struct, accessible trough additional
api functions (or macros):

   const char *fluid_preset_info_get_name(&info);
   int fluid_preset_info_get_bank(&info);
   int fluid_preset_info_get_num(&info);
   fluid_sfont_t *fluid_preset_info_get_sfont(&info);

any additional fields would have corresponding accessors in a possible
future.

cheers
-- 
rncbc aka Rui Nuno Capela
address@hidden