[vlc-devel] [PATCH 1/2] audio_filter: SOFAlizer new module

[Francois Cartegnie]

Le 16/02/2015 13:28, Wolfgang Hrauda a écrit :
>  dnl
> +dnl  NetCDF library for SOFAlizer plugin
> +dnl
> +PKG_ENABLE_MODULES_VLC([SOFALIZER], [], [netcdf >= 4.1.1], [netcdf support for sofalizer module], [auto])
> +
> +dnl
>  dnl Libnotify notification plugin
>  dnl
>  PKG_ENABLE_MODULES_VLC([NOTIFY], [], [libnotify gtk+-2.0], [libnotify notification], [auto])

Not standalone libraries.
Enable module based on library checks.

> +/*****************************************************************************
> + * sofalizer.c : SOFAlizer plugin to use SOFA files in vlc
> + *****************************************************************************

Where else ?

> + * Copyright (C) 2013-2015 Andreas Fuchs, Wolfgang Hrauda, ARI

No contact for "ARI".

> + * Project coordinator: Piotr Majdak <piotr at majdak.at>

We don't care.

> +
> +struct nc_sofa_t /* contains data of one SOFA file */
> +{
> +   int i_ncid; /* netCDF ID of the opened SOFA file */
> +   int i_n_samples; /* length of one impulse response (IR) */
> +   int i_m_dim; /* number of measurement positions */
> +   int *p_data_delay; /* broadband delay of IR, dimension [I R] or [M R] */
> +   /* all measurement positions for each receiver (i.e. ear): */
> +   float *p_sp_a; /* azimuth angles */
> +   float *p_sp_e; /* elevation angles */
> +   float *p_sp_r; /* radii */
> +   float *p_data_ir; /* IRs at each measurement position for each receiver */
> +};

struct naming is _s.

> +#define FILE1_NAME_TEXT N_( "SOFA file 1" )
> +#define FILE2_NAME_TEXT N_( "SOFA file 2" )
> +#define FILE3_NAME_TEXT N_( "SOFA file 3" )

Don't put pressure on translators. Use format string where possible.
Same for others strings.

> +#define SELECT_VALUE_TEXT N_( "Select SOFA file" )

You are supposed to provide text, not UI actions.

> +#define ROTATION_VALUE_TEXT N_( "Rotation [°]" )

You are supposed to provide text, no symbols.

> +vlc_module_begin ()
> +    set_description( N_("SOFAlizer") )
> +    set_shortname( N_("SOFAlizer") )

No translation for that.
> +
> +static int LoadSofa ( filter_t *p_filter, char *c_filename, /* loads one file*/
> +                      int i_i_sofa , int *p_samplingrate)

*c_
i_i_
*p_

not really matching code writing guidelines. Same everywhere.

> +    for( int ii = 0; ii<i_n_dims; ii++ ) /* go through all dimensions of file */
> +    {
> +        nc_inq_dim( i_ncid, ii, c_dim_names[ii], &i_dim_length[ii] ); /* get dimensions */
> +        if ( !strncmp("M", c_dim_names[ii], 1 ) ) /* get ID of dimension "M" */
> +            i_m_dim_id = ii;
> +        if ( !strncmp("N", c_dim_names[ii], 1 ) ) /* get ID of dimension "N" */
> +            i_n_dim_id = ii;
> +    }

ii ??

> +
> +    /* -- allocate memory for one value for each measurement position: -- */
> +    int *p_data_delay = p_sys->sofa[i_i_sofa].p_data_delay =
> +        calloc ( sizeof( int ) , i_m_dim * 2 );

"man calloc"

> +    float *p_sp_a = p_sys->sofa[i_i_sofa].p_sp_a = malloc( sizeof(float) * i_m_dim);
> +    float *p_sp_e = p_sys->sofa[i_i_sofa].p_sp_e = malloc( sizeof(float) * i_m_dim);
> +    float *p_sp_r = p_sys->sofa[i_i_sofa].p_sp_r = malloc( sizeof(float) * i_m_dim);
> +    /* delay values are required for each ear and each measurement position: */
> +    float *p_data_ir = p_sys->sofa[i_i_sofa].p_data_ir =
> +        malloc( sizeof( float ) * 2 * i_m_dim * i_n_samples );
> +
> +    if ( !p_data_delay || !p_sp_a || !p_sp_e || !p_sp_r || !p_data_ir )
> +    {   /* if memory could not be allocated */

Partial allocation = LEAK

> +    /* Data.Delay dimension check */
> +       /* dimension of Data.Delay is [I R]: */
> +    if ( !strncmp ( psz_data_delay_dim_name, "I\0", 2 ) )

String is already zero terminated.
> +static int GainCallback( vlc_object_t *p_this, char const *psz_var,
> +                          vlc_value_t oldval, vlc_value_t newval, void *p_data )
> +{
> +    VLC_UNUSED(p_this); VLC_UNUSED(psz_var); VLC_UNUSED(oldval);

UNUSED USED..

> +    /* get user settings */
> +    p_sys->f_rotation   = abs ( ( - (int) var_CreateGetFloat ( p_out, "sofalizer-rotation" ) + 720 ) % 360 );
> +    p_sys->i_i_sofa     = (int) (var_CreateGetFloat ( p_out, "sofalizer-select" ) ) - 1;
> +    p_sys->i_switch     = (int) ( var_CreateGetFloat ( p_out, "sofalizer-switch" ) );
> +    p_sys->f_gain       = var_CreateGetFloat( p_out, "sofalizer-gain" );
> +    p_sys->f_elevation  = var_CreateGetFloat( p_out, "sofalizer-elevation" );
> +    p_sys->f_radius     = var_CreateGetFloat( p_out, "sofalizer-radius");
> +

> +    p_sys->i_buffer_length = pow(2, ceil(log( i_n_max )/ log(2) ) ); /* buffer length as power of 2 */

1 << ceil(log( i_n_max )/ log(2) )

Maybe I'm wrong but:

log( i_n_max ) / log(2) = log2(i_n_max)
doing a power of 2 of log2 ?

> +    p_sys->p_ringbuffer_l = calloc( p_sys->i_buffer_length * i_input_nb, sizeof( float ) );
> +    p_sys->p_ringbuffer_r = calloc( p_sys->i_buffer_length * i_input_nb, sizeof( float ) );

man calloc again

> +    if ( !p_sys->p_ir_l || !p_sys->p_ir_r || !p_sys->p_delay_l ||
> +         !p_sys->p_delay_r || !p_sys->p_ringbuffer_l || !p_sys->p_ringbuffer_r
> +         || !p_sys->p_speaker_pos )
> +    {

re-leak

> +        memset( (float *)p_out_buf->p_buffer , 0 , sizeof( float ) * p_in_buf->i_nb_samples * 2 );

no cast

> +    }
> +    else /* do the actual convolution for left and right channel */
> +    {
> +        if( vlc_clone( &left_thread, (void *)&sofalizer_Convolute,
> +        (void *)&t_data_l, VLC_THREAD_PRIORITY_HIGHEST ) ) goto out;

I doubt someone would allow highest priority

> +        if( vlc_clone( &right_thread, (void *)&sofalizer_Convolute,
> +        (void *)&t_data_r, VLC_THREAD_PRIORITY_HIGHEST ) ) goto out;
> +        vlc_join ( left_thread, NULL );
> +        vlc_join ( right_thread, NULL );
> +        p_sys->i_write = t_data_l.i_write;
> +    }

And if second thread fails cloning ?


Francois