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

Wolfgang Hrauda wolfgang.hrauda at gmx.at
Tue May 12 12:44:41 CEST 2015


Thanks again, Francois. We have now finally posted an updated patch containing corrections based on your comments plus new functionality (resampling of HRTFs and performance improvement by using fast convolution). See my posts from today (May 12th).

I answer your comments below. Some answers just confirm our agreement, some explain our point of view and/or raise follow-up questions.
We'd appreciate if you could answer our follow-up questions! Thank you!


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.
We were advised to do so by Jean-Baptiste Kempf: „Please use just the PKG check, in one line, for all the architectures.“ (https://mailman.videolan.org/pipermail/vlc-devel/2013-September/094739.html) That's why we are doing it this way.
Could explain a bit more detailed what you mean?

> 
>> +/*****************************************************************************
>> + * sofalizer.c : SOFAlizer plugin to use SOFA files in vlc
>> + *****************************************************************************
> 
> Where else ?
Done. Updated short description.

> 
>> + * 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.
Actually, SOFAlizer is a project of the Acoustics Research Institute (ARI). Piotr Majdak is the project coordinator at ARI. I now clarified this in the text.

> 
>> +
>> +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.
We were following the code conventions found in the developer's corner of VLC (https://wiki.videolan.org/Code_Conventions/).
It says: „If a variable has no basic type (for instance a complex structure), don't put any prefix (except p_ if it's a pointer).“ I'm not sure what to do now, please advise.


> 
>> +#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.
Could you explain what you mean by “use format string”? We oriented us towards the use of strings in the other audio modules.

> 
>> +#define SELECT_VALUE_TEXT N_( "Select SOFA file" )
> 
> You are supposed to provide text, not UI actions.
Agreed, changed to „Use SOFA file number:“

> 
>> +#define ROTATION_VALUE_TEXT N_( "Rotation [°]" )
> 
> You are supposed to provide text, no symbols.
Agreed, changed.

>> +vlc_module_begin ()
>> +    set_description( N_("SOFAlizer") )
>> +    set_shortname( N_("SOFAlizer") )
> 
> No translation for that.
Agreed, changed.

>> +
>> +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.
Are you addressing the prefixes? We were following the code conventions found in the developer's corner of VLC (https://wiki.videolan.org/Code_Conventions/):
„That is, we have the following prefixes: i_ for integers etc.“
Please confirm that you are proposing to violate the Code Conventions. We will do accordingely.


> 
>> +    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 ??
„ii“ has the advantage that you can search for it within the code. If you search for „i“, you will obviously get a lot of results you don't want.
I admit that its use here is not consistent with the rest of the source code, therefore I changed it to „i“.

> 
>> +
>> +    /* -- 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"
Agreed, changed order of arguments.

> 
>> +    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
When an error occurs, the function CloseSofa() (which is called within that corresponding if statement) correctly frees all the allocated memory associated with the currently loaded file, even on partial allocation.
However, there was a problem with an uninitialized id variable, but I fixed that. Please give me a note if this solved the problem you addressed.

> 
>> +    /* 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.
Agreed, changed.

>> +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..
Agreed, removed VLC_UNUSED(p_this).

> 
>> +    /* 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 ?
Note the ceil in the code. So, we first do log2 then ceil (to obtain an integer power of 2), then power of 2. For example, if one impulse response is 200 samples long, we want the ringbuffer to be 256 samples for computational efficiency.

> 
>> +    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
What's wrong here? I couldn't figure out, despite reading documentation of calloc.

> 
>> +    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
If you refer to a partial allocation problem again, the free'ing is correctly done by the functions FreeAllSofa and FreeFilter in the error handling.
Note that while re-checking the code, we found an initialization problem which we have resolved now. However, we cannot find any leak (we reconfirmed this with a simulated partial allocation and valgrind. No leak).

> 
>> +        memset( (float *)p_out_buf->p_buffer , 0 , sizeof( float ) * p_in_buf->i_nb_samples * 2 );
> 
> no cast
Basically, the out buffer is always cast to float* (also e.g. in the compressor module). However, for setting the memory to 0 it doesn't matter anyway, so I removed the casting to float.

> 
>> +    }
>> +    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
Agreed, changed priority to PRIORITY_AUDIO.

> 
>> +        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 ?
I'm not sure what you mean!? In case that the second thread fails cloning, would I have to join the first thread and then “goto out”?


Best,
Wolfgang


> 
> 
> Francois
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20150512/fcd0842e/attachment.html>


More information about the vlc-devel mailing list