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

[Wolfgang Hrauda]

Francois, thanks a lot for your comments. We really appreciate your feedback.
I will improve the code based on your remarks, next week, and then perhaps get back at you with some questions if anything remains unclear.

Best regards,
Wolfgang


Am 14.04.2015 um 18:11 schrieb Francois Cartegnie <fcvlcdev at free.fr>:

> 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
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel