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

[Rémi Denis-Courmont]

Le mardi 12 mai 2015, 12:44:41 Wolfgang Hrauda a écrit :
> > 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.htm
> l) That's why we are doing it this way. Could explain a bit more detailed
> what you mean?

Well, the way you patched configure.ac will become a problem if/when the netcdf 
is reused in any other module. Of course, you could argue that this is a 
bridge that has yet to be crossed.

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

I doubt that Mr. Majdak will continue as the project coordinator of the VLC 
SOFalizer plugin going forward. It´s totally fine to give credit where due...
But in 5 years, if someone reads that line, they might think that they need to 
contact him to change the VLC plugin code. And that just would not be right.

> >> +
> >> +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.

I would advise to avoid _t as it violates the POSIX specification. And yes, I 
know there are plenty of violations in the existing code, but lets not add 
more.

> >> +#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.

Indeed, you cannot use format strings here. But as others have pointed out, 
having three identical configuration entries is arbitrary and questionable.

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

You are supposed to provide a description of what the option does.

> >> +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.

I would advise to side step the problem by not using the Hungarian notation at 
all. But if you use it, please stick to the VLC convention. In particular, 
psz_ for nul-terminated strings.

> >> +    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“.

Yes but this is confusing. "i" is well known to mean index in this context, 
not "ii". If you don´t like "i", then use "idx" or "it" or something.

> >> +    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.

François´s point was that you should use the log2() function.

But actually, if you only care about the integral part of the binary log, then 
either frexp(), or casting to unsigned integer and clz() would be faster.


By the way, please use the "float" functions with the f-suffix when dealing with 
"float" values. Double precision is a big waste of CPU power.

> >> +        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”?

Yes. Joining a non-existent thread is undefined (and will typically crash).

-- 
Rémi Denis-Courmont
http://www.remlab.net/