Re: [fluid-dev] [PATCH] Effect level clip

[Rui Nuno Capela]

Josh Green wrote:
> Hello David,
> 
> On Mon, 2007-09-17 at 04:48 -0500, David Hilvert wrote:
>> On Mon, 17 Sep 2007 00:43:30 -0700
>> Josh Green <address@hidden> wrote:
>>
>>> I think I need to get a better idea of how your patch is helping to keep
>>> Reverb and Chorus at a more constant output level, regardless of the
>>> synth.gain setting.
>> If I recall correctly, effective gain was split in 1.0.7, for each effect
>> module, into two general controls combining roughly multiplicatively -- one
>> controlling data sent to the effect module (*_SEND), and one controlling the
>> module's behavior (gain).  The *_SEND controls, in turn, are an additive
>> combination of (at least) two inputs -- one specified within the soundfont, 
>> and
>> one specified by MIDI controllers (such as those found on vkeybd).  While the
>> *_SEND logic produces a clipped value, ensuring that effect output does not
>> considerably exceed dry output, the module gain control did not.
>>
>> One way to resolve this would be to place a hard limit on the 1.0.7 effect
>> gain; while doing this would serve to control output levels, it would also
>> limit the usefulness of the control.  Instead combining all controls 
>> additively
>> into the *_SEND factor allows adjustment across the entire range of effect 
>> for
>> any given (single) instrument, while preventing effect levels from
>> significantly exceeding dry output.  This latter approach requires that the
>> user-configurable gain levels be somehow combined into the *_SEND factors,
>> which task has no immediately obvious implementation both trivial and clean.
>>
>> My first patch was the most trivial I could manage; the second was less 
>> trivial, 
>> and perhaps only marginally cleaner from the perspective of computational 
>> expense.
>>
> 
> I think I understood most of what you said there, but I think there is
> still something fundamental I am missing.  As I mentioned previously,
> I'm discovering how this thing works as I go along ;)
> 
> I realize my previous comment about the global reverb level not being
> realtime, was wrong.  It should be a realtime effect (as it seems the
> other reverb parameters are currently).
> 
> Prior to your patch, it looks like the fluid_revmodel_setlevel()
> function was doing nothing (code was commented out).  I'm at a loss as
> to how the reverb effect level was even working before (was it?).  From
> the code which was commented out, it looks like it was at some point
> modifying the 'wet' field of the revmodel structure.
> 
> The modulation of the Reverb send parameter is handled by the
> fluid_voice_update_param() for voices for which this parameter changes,
> which seems fine (how most other generators are processed).
> 
> The global reverb level on the other hand I would think would modify
> parameters which affect the mix of the entire reverb buffer as handled
> in fluid_revmodel_processreplace() and fluid_revmodel_processmix(), and
> would not need to modify the reverb send on a per voice basis (or is
> that where I'm missing something?).  If there was a need to do per voice
> reverb changes based on the global reverb level, I would think it should
> be calculated in fluid_voice_write().
> 
> Looking at the previously commented out code in
> fluid_revmodel_setlevel() I see:
> 
> /*   fluid_clip(value, 0.0f, 1.0f); */
> /*   rev->wet = value * scalewet; */
> /*   fluid_revmodel_update(rev); */
> 
> 
> My question is, what was wrong with this code?  It seems like it is
> updating the wet mix parameter and then updating the rest of the reverb
> parameters accordingly (when a change occurs), which affects the
> fluid_revmodel_processreplace() and fluid_revmodel_processmix()
> functions.
> 
> I guess I'm still not clear on the basic questions:
> - What was wrong with the previous Reverb level code?
> - What should be done to fix it?
> 

this is in fact one of my proposed patches (attached once again).

without this one qsynth reverb level is stuck to 100%, surely everyone
has this for granted ;)

i remember it was argued by Peter himself during 1.0.4 release iirc;
never quite understood his point and still unconvinced ever since.

all my personal fluidsynth builds include this simple patch and am 100%
confident it does not harm to regress.

cheers
-- 
rncbc aka Rui Nuno Capela
address@hidden

diff -dupr fluidsynth-1.0.7a.0/src/fluid_rev.c 
fluidsynth-1.0.7a.1/src/fluid_rev.c
--- fluidsynth-1.0.7a.0/src/fluid_rev.c 2004-03-29 11:05:18.000000000 +0100
+++ fluidsynth-1.0.7a.1/src/fluid_rev.c 2006-02-21 23:01:12.000000000 +0000
@@ -535,8 +535,8 @@ void 
 fluid_revmodel_setlevel(fluid_revmodel_t* rev, fluid_real_t value)
 {
 /*   fluid_clip(value, 0.0f, 1.0f); */
-/*   rev->wet = value * scalewet; */
-/*   fluid_revmodel_update(rev); */
+  rev->wet = value * scalewet;
+  fluid_revmodel_update(rev);
 }
 
 fluid_real_t