[vlc-devel] Dynamic range compressor patch

[Laurent Aimar]

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