[vlc-devel] Dynamic range compressor patch
Ron Wright
logiconcepts819 at gmail.com
Fri Jun 25 22:39:30 CEST 2010
Hello,
I have made many drastic changes in the compressor.c file and some minor
changes in the UI code. Here are some of the changes:
1. Added VLC copyright and $id$ tag to the banner.
2. Made the filter structure, RMS envelope structure, and attack lookup
table contiguous in memory so that only one memory allocation and one memory
deallocation is needed.
3. Reorganized the functions so that they don't appear out of place.
4. Removed two wasteful members (amplitude and gain reduction) from the
filter structure (this is probably used to implement output meters in
certain LADSPA frontends).
5. The release of the mutex lock in the DoWork function now occurs
immediately after retrieving the shared values.
6. Replaced the logic in the callback functions with clipping functions.
7. Removed useless casts.
8. Changed default values.
Here are some notes to take into consideration:
1. The f_max and f_clamp functions work as expected. Proof:
f_max (with arguments x and a):
If x < a, then 0.5 * ((x - a) + fabs(x - a)) + a = 0.5 * ((x - a) - (x - a))
+ a = a
If x >= a, then 0.5 * ((x - a) + fabs(x - a)) + a = 0.5 * ((x - a) + (x -
a)) + a = 0.5*2*(x - a) + a = x
f_clamp (with arguments x, a, and b):
If x < a, then 0.5 * (fabs(x - a) + a + b - fabs(x - b)) = 0.5 * (-(x - a) +
a + b + (x - b)) = 0.5 * (2*a - x + b + x - b) = a
If a <= x < b, then 0.5 * (fabs(x - a) + a + b - fabs(x - b)) = 0.5 * ((x -
a) + a + b + (x - b)) = 0.5 * (x + b + x - b) = x
If x >= b, then 0.5 * (fabs(x - a) + a + b - fabs(x - b)) = 0.5 * ((x - a) +
a + b - (x - b)) = 0.5 * (x + b - x + b) = b
These are branchless clipping functions from Laurent de Soras.
2. A buffer overflow will not occur when either attack or release converted
to seconds is multiplied by (A_TBL - 1), with the resulting value used as an
index to retrieve the attack (or release) value from the lookup table. The
maximum of the maximum values of attack and release is 800, and A_TBL is
256, so 800 * 0.001 * 255 = 204. This means that the index will neither go
beyond 255 nor even reach 255. The minimum of the minimum values of attack
and release is 1.5. However, the real minimum value used in the code is 2.
Nevertheless, there will also be no buffer underflow, since this value is
positive.
If you wish to see the original LADSPA SWH code, look here:
http://github.com/swh/ladspa. The compressor is based on code in
sc4_1882.xml along with the necessary helper functions in other files.
Attached are the updated compressor core and UI patches.
Regards,
Ronald Wright
On Thu, Jun 24, 2010 at 2:05 PM, Laurent Aimar <fenrir at elivagar.org> wrote:
> On Mon, Jun 21, 2010 at 11:28:55PM -0500, Ron Wright wrote:
> >
> +/*****************************************************************************
> > + * Open: initialize interface
> > +
> *****************************************************************************/
> > +
> > +static int Open( vlc_object_t *p_this )
> > +{
> > + filter_t *p_filter = (filter_t*)p_this;
> > + vlc_object_t *p_aout = p_filter->p_parent;
> I see what you need it but I find it really really ugly. Dunno how to
> avoid that. Anyone with an idea ?
>
> > + struct filter_sys_t *p_sys;
> > + float amp;
> > + float *as = NULL;
> Useless init.
> > + unsigned int count;
> > + float env;
> > + float env_peak;
> > + float env_rms;
> > + float gain;
> > + float gain_t;
> > + rms_env *rms = NULL;
> Useless init.
> > + float sum;
> > +
> > + sample_rate = (float)p_filter->fmt_in.audio.i_rate;
> Useless cast.
>
> > + rms = rms_env_new();
> > + if ( !rms )
> > + {
> > + free( p_sys );
> > + return VLC_ENOMEM;
> > + }
> > +
> > + sum = 0.0f;
> > + amp = 0.0f;
> > + gain = 0.0f;
> > + gain_t = 0.0f;
> > + env = 0.0f;
> > + env_rms = 0.0f;
> > + env_peak = 0.0f;
> > + count = 0;
> > +
> > + as = malloc( A_TBL * sizeof(float) );
> calloc( A_TBL, sizeof(*as) )
>
> > + if ( !as )
> > + {
> > + rms_env_free( rms );
> > + free( p_sys );
> > + return VLC_ENOMEM;
> > + }
> But moving as and rms into the context (instead of allocating them
> independantly) would avoid thoses 2 mallocs and so simplify the code
> (only one error path).
>
> > + as[0] = 1.0f;
> > + for ( i = 1; i < A_TBL; i++ ) {
> > + as[i] = expf( -1.0f / ( sample_rate * (float)i / (float)A_TBL )
> );
> Useless float casts.
>
> > + p_sys->amp = amp;
> > + p_sys->as = as;
> > + p_sys->count = count;
> > + p_sys->env = env;
> > + p_sys->env_peak = env_peak;
> > + p_sys->env_rms = env_rms;
> > + p_sys->gain = gain;
> > + p_sys->gain_t = gain_t;
> > + p_sys->rms = rms;
> > + p_sys->sum = sum;
> I think you call remove most of the local variables and directly init
> p_sys.
>
> > + vlc_mutex_init( &p_sys->lock );
> > +
> > + p_sys->rms_peak = var_CreateGetFloat( p_aout,
> "compressor-rms-peak" );
> > + p_sys->attack = var_CreateGetFloat( p_aout, "compressor-attack"
> );
> > + p_sys->release = var_CreateGetFloat( p_aout,
> "compressor-release" );
> > + p_sys->threshold = var_CreateGetFloat( p_aout,
> "compressor-threshold" );
> > + p_sys->ratio = var_CreateGetFloat( p_aout, "compressor-ratio"
> );
> > + p_sys->knee = var_CreateGetFloat( p_aout, "compressor-knee"
> );
> > + p_sys->makeup_gain = var_CreateGetFloat( p_aout,
> "compressor-makeup-gain" );
> > + p_sys->amplitude = 0;
> > + p_sys->gain_red = 0;
> > +
> > + /* Add our own callbacks */
> > + var_AddCallback( p_aout, "compressor-rms-peak", RMSPeakCallback,
> p_sys );
> > + var_AddCallback( p_aout, "compressor-attack", AttackCallback, p_sys
> );
> > + var_AddCallback( p_aout, "compressor-release", ReleaseCallback,
> p_sys );
> > + var_AddCallback( p_aout, "compressor-threshold", ThresholdCallback,
> p_sys );
> > + var_AddCallback( p_aout, "compressor-ratio", RatioCallback, p_sys );
> > + var_AddCallback( p_aout, "compressor-knee", KneeCallback, p_sys );
> > + var_AddCallback( p_aout, "compressor-makeup-gain",
> MakeupGainCallback,
> > + p_sys );
> > +
> > + msg_Dbg( p_filter, "compressor successfully initialized" );
> > + return VLC_SUCCESS;
> > +}
> > +
> > +static float f_db2lin_cube( float db, filter_sys_t * p_sys )
> > +{
> > + float scale = ( db - DB_MIN ) * (float)LIN_TABLE_SIZE / ( DB_MAX -
> DB_MIN );
> > + int base = f_round( scale - 0.5f );
> > + float ofs = scale - base;
> > + float *lin_data = p_sys->lin_data;
> > +
> > + if ( base < 1 )
> > + {
> > + return 0.0f;
> > + }
> > + else if ( base > LIN_TABLE_SIZE - 3 )
> > + {
> > + return lin_data[LIN_TABLE_SIZE - 2];
> > + }
> > +
> > + return cube_interp( ofs, lin_data[base - 1],
> > + lin_data[base],
> > + lin_data[base + 1],
> > + lin_data[base + 2] );
> > +}
> > +
> > +static float f_db2lin_lerp( float db, filter_sys_t * p_sys )
> > +{
> > + float scale = ( db - DB_MIN ) * (float)LIN_TABLE_SIZE / ( DB_MAX -
> DB_MIN );
> > + int base = f_round( scale - 0.5f );
> > + float ofs = scale - base;
> > + float *lin_data = p_sys->lin_data;
> > +
> > + if ( base < 1 )
> > + {
> > + return 0.0f;
> > + }
> > + else if ( base > LIN_TABLE_SIZE - 3 )
> > + {
> > + return lin_data[LIN_TABLE_SIZE - 2];
> > + }
> > +
> > + return ( 1.0f - ofs ) * lin_data[base] + ofs * lin_data[base + 1];
> > +}
> f_db2lin_cube and f_db2lin_lerp could be merged and simply use the define
> for
> the only specific line.
>
> > +static float f_lin2db_cube( float lin, filter_sys_t * p_sys )
> > +{
> > + float scale = ( lin - LIN_MIN ) * (float)DB_TABLE_SIZE /
> > + ( LIN_MAX - LIN_MIN );
> > + int base = f_round( scale - 0.5f );
> > + float ofs = scale - base;
> > + float *db_data = p_sys->db_data;
> > +
> > + if ( base < 2 )
> > + {
> > + return db_data[2] * scale * 0.5f - 23.0f * ( 2.0f - scale );
> > + }
> > + else if ( base > DB_TABLE_SIZE - 3 )
> > + {
> > + return db_data[DB_TABLE_SIZE - 2];
> > + }
> > +
> > + return cube_interp( ofs, db_data[base - 1],
> > + db_data[base],
> > + db_data[base + 1],
> > + db_data[base + 2] );
> > +}
> > +
> > +static float f_lin2db_lerp( float lin, filter_sys_t * p_sys )
> > +{
> > + float scale = ( lin - LIN_MIN ) * (float)DB_TABLE_SIZE /
> > + ( LIN_MAX - LIN_MIN );
> > + int base = f_round( scale - 0.5f );
> > + float ofs = scale - base;
> > + float *db_data = p_sys->db_data;
> > +
> > + if ( base < 2 )
> > + {
> > + return db_data[2] * scale * 0.5f - 23.0f * ( 2.0f - scale );
> > + }
> > + else if ( base > DB_TABLE_SIZE - 2 )
> > + {
> > + return db_data[DB_TABLE_SIZE - 1];
> > + }
> > +
> > + return ( 1.0f - ofs ) * db_data[base] + ofs * db_data[base + 1];
> > +}
> Same here (dunno if the -2 vs -3 is a bug or not, it doesn't match
> f_db2lin_cube vs f_db2lin_lerp).
>
> > +static void round_to_zero( volatile float *f )
> > +{
> > + *f += 1e-18;
> > + *f -= 1e-18;
> > +}
> I don't think volatile is needed here.
> Btw, what do you want to do with it ? Is it to remove not normalized
> number ?
>
> > +static float f_max( float x, float a )
> > +{
> > + x -= a;
> > + x += fabs( x );
> > + x *= 0.5;
> > + x += a;
> > +
> > + return x;
> > +}
> Is it really a max like function ?
>
> > +static block_t * DoWork( filter_t * p_filter, block_t * p_in_buf )
> > +{
> > + int i_samples = p_in_buf->i_nb_samples;
> > + int i_channels = aout_FormatNbChannels( &p_filter->fmt_in.audio );
> > + float *p_out = (float*)p_in_buf->p_buffer;
> > + float *p_in = (float*)p_in_buf->p_buffer;
> > +
> > + float rms_peak, attack, release, threshold, ratio, knee,
> makeup_gain;
> > + float amp, *as, env, env_peak, env_rms, gain, gain_t, sum;
> > + unsigned int count;
> > + rms_env *rms;
> > +
> > + float ga, gr, rs, mug, knee_min, knee_max, ef_a, ef_ai;
> > +
> > + int pos, pos_chan;
> > +
> > + /* Current configuration */
> > + struct filter_sys_t *p_sys = p_filter->p_sys;
> > +
> > + vlc_mutex_lock( &p_sys->lock );
> > +
> > + /* RMS/peak (float value) */
> > + rms_peak = p_sys->rms_peak;
> > +
> > + /* Attack time (ms) (float value) */
> > + attack = p_sys->attack;
> > +
> > + /* Release time (ms) (float value) */
> > + release = p_sys->release;
> > +
> > + /* Threshold level (dB) (float value) */
> > + threshold = p_sys->threshold;
> > +
> > + /* Ratio (n:1) (float value) */
> > + ratio = p_sys->ratio;
> > +
> > + /* Knee radius (dB) (float value) */
> > + knee = p_sys->knee;
> > +
> > + /* Makeup gain (dB) (float value) */
> > + makeup_gain = p_sys->makeup_gain;
>
> As you made a copy of all the variable protected by the lock, you could
> release it here (instead of later). It is always good to minimized the time
> you hold a lock when possible.
>
> > +
> > + amp = p_sys->amp;
> > + as = p_sys->as;
> > + count = p_sys->count;
> > + env = p_sys->env;
> > + env_peak = p_sys->env_peak;
> > + env_rms = p_sys->env_rms;
> > + gain = p_sys->gain;
> > + gain_t = p_sys->gain_t;
> I would prefer if you don't use _t for a variable.
>
> > + rms = p_sys->rms;
> > + sum = p_sys->sum;
> > +
> > + ga = attack < 2.0f ? 0.0f
> > + : as[f_round( attack * 0.001f * (float)( A_TBL -
> 1 ) )];
> > + gr = as[f_round( release * 0.001f * (float)( A_TBL - 1 ) )];
> Are thoses as[] accesses guaranted to be insided bound ? (besides, with
> float
> I am a bit paranoiac).
>
> > + rs = ( ratio - 1.0f ) / ratio;
> > + mug = db2lin( makeup_gain, p_sys );
> > + knee_min = db2lin( threshold - knee, p_sys );
> > + knee_max = db2lin( threshold + knee, p_sys );
> > + ef_a = ga * 0.25f;
> > + ef_ai = 1.0f - ef_a;
> > +
> > + for ( pos = 0; pos < i_samples; pos++ )
> > + {
> > + float lev_in = fabs( p_in[0] );
> > + for( pos_chan = 1; pos_chan < i_channels; pos_chan++ )
> > + {
> > + lev_in = f_max( lev_in, fabs( p_in[pos_chan] ) );
> > + }
> > + sum += lev_in * lev_in;
> > +
> > + if ( amp > env_rms )
> > + {
> > + env_rms = env_rms * ga + amp * ( 1.0f - ga );
> > + }
> > + else
> > + {
> > + env_rms = env_rms * gr + amp * ( 1.0f - gr );
> > + }
> > + round_to_zero( &env_rms );
> > + if ( lev_in > env_peak )
> > + {
> > + env_peak = env_peak * ga + lev_in * ( 1.0f - ga );
> > + }
> > + else
> > + {
> > + env_peak = env_peak * gr + lev_in * ( 1.0f - gr );
> > + }
> > + round_to_zero( &env_peak );
> > + if ( ( count++ & 3 ) == 3 )
> > + {
> > + amp = rms_env_process( rms, sum * 0.25f );
> > + sum = 0.0f;
> > + if ( isnan( env_rms ) )
> > + {
> > + /* This can happen sometimes, but I don't know why */
> > + env_rms = 0.0f;
> Maybe because your env_rms equation diverges (easy with float) or one
> of the other numbers used is a NaN.
> > + }
> > +
> > + env = LIN_INTERP( rms_peak, env_rms, env_peak );
> > +
> > + if ( env <= knee_min )
> > + {
> > + gain_t = 1.0f;
> > + }
> > + else if ( env < knee_max )
> > + {
> > + const float x = -( threshold - knee
> > + - lin2db( env, p_sys ) ) /
> knee;
> > + gain_t = db2lin( -knee * rs * x * x * 0.25f, p_sys );
> > + }
> > + else
> > + {
> > + gain_t = db2lin( ( threshold - lin2db( env, p_sys ) ) *
> rs,
> > + p_sys );
> > + }
> > + }
> > +
> > + gain = gain * ef_a + gain_t * ef_ai;
> > +
> > + for ( pos_chan = 0; pos_chan < i_channels; pos_chan++ )
> > + {
> > + p_out[pos_chan] = p_in[pos_chan] * gain * mug;
> > + }
> > +
> > + p_in += i_channels;
> > + p_out += i_channels;
> You may also one only one pointer (but not needed).
>
> > + }
> > +
> > + p_sys->sum = sum;
> > + p_sys->amp = amp;
> > + p_sys->gain = gain;
> > + p_sys->gain_t = gain_t;
> > + p_sys->env = env;
> > + p_sys->env_rms = env_rms;
> > + p_sys->env_peak = env_peak;
> > + p_sys->count = count;
> > +
> > + p_sys->amplitude = lin2db( env, p_sys );
> > + p_sys->gain_red = lin2db( gain, p_sys );
> > +
> > + vlc_mutex_unlock( &p_sys->lock );
> > +
> > + return p_in_buf;
> > +}
> > +
> >
> +/*****************************************************************************
> > + * Callback functions
> > +
> *****************************************************************************/
> > +
> > +static int RMSPeakCallback( vlc_object_t *p_this, char const *psz_cmd,
> > + vlc_value_t oldval, vlc_value_t newval,
> > + void * p_data )
> > +{
> > + VLC_UNUSED(p_this); VLC_UNUSED(psz_cmd); VLC_UNUSED(oldval);
> > + filter_sys_t *p_sys = p_data;
> > +
> > + if ( newval.f_float < 0.0 )
> > + {
> > + newval.f_float = 0.0;
> > + }
> > + else if ( newval.f_float > 1.0 )
> > + {
> > + newval.f_float = 1.0;
> > + }
> > +
> > + vlc_mutex_lock( &p_sys->lock );
> > + p_sys->rms_peak = newval.f_float;
> > + vlc_mutex_unlock( &p_sys->lock );
> > +
> > + return VLC_SUCCESS;
> > +}
> > +
> > +static int AttackCallback( vlc_object_t *p_this, char const *psz_cmd,
> > + vlc_value_t oldval, vlc_value_t newval,
> > + void * p_data )
> > +{
> > + VLC_UNUSED(p_this); VLC_UNUSED(psz_cmd); VLC_UNUSED(oldval);
> > + filter_sys_t *p_sys = p_data;
> > +
> > + if ( newval.f_float < 1.5 )
> > + {
> > + newval.f_float = 1.5;
> > + }
> > + else if ( newval.f_float > 400.0 )
> > + {
> > + newval.f_float = 400.0;
> > + }
> > +
> > + vlc_mutex_lock( &p_sys->lock );
> > + p_sys->attack = newval.f_float;
> > + vlc_mutex_unlock( &p_sys->lock );
> > +
> > + return VLC_SUCCESS;
> > +}
> > +
> > +static int ReleaseCallback( vlc_object_t *p_this, char const *psz_cmd,
> > + vlc_value_t oldval, vlc_value_t newval,
> > + void * p_data )
> > +{
> > + VLC_UNUSED(p_this); VLC_UNUSED(psz_cmd); VLC_UNUSED(oldval);
> > + filter_sys_t *p_sys = p_data;
> > +
> > + if ( newval.f_float < 2.0 )
> > + {
> > + newval.f_float = 2.0;
> > + }
> > + else if ( newval.f_float > 800.0 )
> > + {
> > + newval.f_float = 800.0;
> > + }
> > +
> > + vlc_mutex_lock( &p_sys->lock );
> > + p_sys->release = newval.f_float;
> > + vlc_mutex_unlock( &p_sys->lock );
> > +
> > + return VLC_SUCCESS;
> > +}
> > +
> > +static int ThresholdCallback( vlc_object_t *p_this, char const *psz_cmd,
> > + vlc_value_t oldval, vlc_value_t newval,
> > + void * p_data )
> > +{
> > + VLC_UNUSED(p_this); VLC_UNUSED(psz_cmd); VLC_UNUSED(oldval);
> > + filter_sys_t *p_sys = p_data;
> > +
> > + if ( newval.f_float < -30.0 )
> > + {
> > + newval.f_float = -30.0;
> > + }
> > + else if ( newval.f_float > 0.0 )
> > + {
> > + newval.f_float = 0.0;
> > + }
> > +
> > + vlc_mutex_lock( &p_sys->lock );
> > + p_sys->threshold = newval.f_float;
> > + vlc_mutex_unlock( &p_sys->lock );
> > +
> > + return VLC_SUCCESS;
> > +}
> > +
> > +static int RatioCallback( vlc_object_t *p_this, char const *psz_cmd,
> > + vlc_value_t oldval, vlc_value_t newval,
> > + void * p_data )
> > +{
> > + VLC_UNUSED(p_this); VLC_UNUSED(psz_cmd); VLC_UNUSED(oldval);
> > + filter_sys_t *p_sys = p_data;
> > +
> > + if ( newval.f_float < 1.0 )
> > + {
> > + newval.f_float = 1.0;
> > + }
> > + else if ( newval.f_float > 20.0 )
> > + {
> > + newval.f_float = 20.0;
> > + }
> > +
> > + vlc_mutex_lock( &p_sys->lock );
> > + p_sys->ratio = newval.f_float;
> > + vlc_mutex_unlock( &p_sys->lock );
> > +
> > + return VLC_SUCCESS;
> > +}
> > +
> > +static int KneeCallback( vlc_object_t *p_this, char const *psz_cmd,
> > + vlc_value_t oldval, vlc_value_t newval,
> > + void * p_data )
> > +{
> > + VLC_UNUSED(p_this); VLC_UNUSED(psz_cmd); VLC_UNUSED(oldval);
> > + filter_sys_t *p_sys = p_data;
> > +
> > + if ( newval.f_float < 1.0 )
> > + {
> > + newval.f_float = 1.0;
> > + }
> > + else if ( newval.f_float > 10.0 )
> > + {
> > + newval.f_float = 10.0;
> > + }
> > +
> > + vlc_mutex_lock( &p_sys->lock );
> > + p_sys->knee = newval.f_float;
> > + vlc_mutex_unlock( &p_sys->lock );
> > +
> > + return VLC_SUCCESS;
> > +}
> > +
> > +static int MakeupGainCallback( vlc_object_t *p_this, char const
> *psz_cmd,
> > + vlc_value_t oldval, vlc_value_t newval,
> > + void * p_data )
> > +{
> > + VLC_UNUSED(p_this); VLC_UNUSED(psz_cmd); VLC_UNUSED(oldval);
> > + filter_sys_t *p_sys = p_data;
> > +
> > + if ( newval.f_float < 0.0 )
> > + {
> > + newval.f_float = 0.0;
> > + }
> > + else if ( newval.f_float > 24.0 )
> > + {
> > + newval.f_float = 24.0;
> > + }
> > +
> > + vlc_mutex_lock( &p_sys->lock );
> > + p_sys->makeup_gain = newval.f_float;
> > + vlc_mutex_unlock( &p_sys->lock );
> > +
> > + return VLC_SUCCESS;
> > +}
> Doing a small float cliping function would reduce the code (and so improve
> readability) IMHO.
> You may also create only one callback and check the value of psz_cmd
> inside
> but that depends on one preferences.
>
>
> Regards,
>
> --
> fenrir
>
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> http://mailman.videolan.org/listinfo/vlc-devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20100625/ed15589e/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: vlc-compressor-core.patch
Type: application/octet-stream
Size: 23624 bytes
Desc: not available
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20100625/ed15589e/attachment.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: vlc-compressor-ui.patch
Type: application/octet-stream
Size: 7903 bytes
Desc: not available
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20100625/ed15589e/attachment-0001.obj>
More information about the vlc-devel
mailing list