[vlc-devel] [PATCH] audio_filter: SOFAlizer new module

Andi Fuchs andi.fuchs.mail at gmail.com
Fri Sep 27 13:32:10 CEST 2013


I tried to put all your remarks from both of you into the code, but i have a few follow up questions. 

Am 26.09.2013 um 22:12 schrieb Rémi Denis-Courmont:
>> 
>> +SOURCES_sofalizer = sofalizer.c
>> SOURCES_spatializer = \
>> 	spatializer/allpass.cpp spatializer/allpass.hpp \
>> 	spatializer/comb.cpp spatializer/comb.hpp \
>> @@ -27,7 +28,8 @@ audio_filter_LTLIBRARIES += \
>> 	libparam_eq_plugin.la \
>> 	libscaletempo_plugin.la \
>> 	libspatializer_plugin.la \
>> -	libstereo_widen_plugin.la
>> +	libstereo_widen_plugin.la \
>> +	libsofalizer_plugin.la
> 
> Either the plugin is built always or it is triggered conditionally (in this 
> case by VLC_ADD_PLUGIN), but it cannot be both.

I use now PKG_ENABLE_MODULES_VLC([SOFALIZER], [], [netcdf >= 4.1.3] ) ... in configure.ac as jean-baptiste suggested, but i need now these lines to build it. 

> 
>> +static int Open( vlc_object_t *p_this )
>> +{
>> +    filter_t *p_filter = (filter_t *)p_this;
>> +    filter_sys_t *p_sys = p_filter->p_sys = malloc( sizeof( *p_sys ) );
>> +    if( unlikely( p_sys == NULL ) )
>> +        return VLC_ENOMEM;
>> +    p_sys->p_parent = p_filter;
>> +    vlc_object_t *p_aout = p_filter->p_parent;
> 
> There is no warranty that the parent object is an audio output. It might be a 
> transcode plugin instance, say.
> 

What would be a better name for the vlc_object_t *? Equalizer, compressor and spatializer use this name for the exact same purpose. 

> 
>> +    /* Callbacks can call function LoadIR */
>> +    var_AddCallback( p_aout, "sofalizer-gain", GainCallback, p_sys );
>> +    var_AddCallback( p_aout, "sofalizer-rotation", RotationCallback,
>> p_sys);
>> +    var_AddCallback( p_aout, "sofalizer-elevation",
>> ElevationCallback, p_sys );
>> +    var_AddCallback( p_aout,
>> "sofalizer-switch", SwitchCallback, p_sys );
>> +    var_AddCallback( p_aout,
>> "sofalizer-select", SelectCallback, p_sys );
>> +    var_AddCallback( p_aout,
>> "sofalizer-radius", RadiusCallback, p_sys );
>> +    p_sys->b_Enable = true;
> 
> You cannot write a value while it is potentially read (or written) by other 
> tasks.

I deleted the whole variable b_Enable.  I lock with the mutex_lock all writing operations into p_sys except in the Open and DoWork function, but the Callbacks get now active after everything is done in the Open. There is one write in the DoWork to i_write, but only the DoWork function itselfs writes into this variable. So I think this can't be a problem any more.


>> +        if( vlc_clone( &left_thread, (void *)&sofalizer_Convolute, (void
>> *)&t_data_l, VLC_THREAD_PRIORITY_HIGHEST ) ) goto out;
>> +        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 );
> 
> Threads are relatively cheap, but creating and destroying two per audio block 
> may be pushing it a bit...

I nearly doubled the performance by putting the main computation of the the DoWork function into the two threads. Convolution in real time needs lots of computation time and the computed data for the right and left ear is independent from each other. 

> 
>> +        p_sys->i_write = t_data_l.i_write;
>> +    }
>> +
>> +    if ( ( i_n_clippings_l + i_n_clippings_r ) > 0 )
>> +    {
>> +        msg_Err(p_filter, "%d of %d Samples in the Outputbuffer clipped.
>> Please reduce gain.", i_n_clippings_l + i_n_clippings_r,
>> p_out_buf->i_nb_samples * 2 );
>> +    }
>> +out: block_Release( p_in_buf );
>> +    return p_out_buf;
>> +}
>> +
> 
> (...)


Does (...) mean Trailing Spaces? I will run a script to remove them all next time, sorry for that.


Am 25.09.2013 um 15:50 schrieb Jean-Baptiste Kempf:

> 
>> +struct filter_sys_t
>> +{
>> +    filter_t *p_parent;
>> +    vlc_mutex_t lock;
>> +    bool b_Enable;
> 
> Avoid that. You are destroying packing.
> 
>> +    float *p_speaker_pos;
>> +
>> +    /* Control variables */
>> +    float f_gain;
>> +    float f_rotation;
>> +    float f_elevation;
>> +    float f_radius;
>> +    int i_azimuth_array[4];
>> +    int i_elevation_array[4];
>> +    int i_switch;
>> +    bool b_mute;
> 
> idem
> 
>> +    /* N of Channels to convolute */
>> +    int i_n_conv;
>> +    bool b_lfe;
> 
> idem.
> 
>> +    /* Buffer variables */
>> +    float *p_ringbuffer_l;
>> +    float *p_ringbuffer_r;
>> +    int i_write;
>> +    int i_buffer_length;
>> +
>> +    /* NetCDF variables */
>> +    struct nc_sofa_t sofa[N_SOFA];
> 
> And probably the same.

I never had do worry about packing before, and i'm a little confused. bools in c are represented by int, so on 32bit all floats, ints and pointers should all be 4byte aligned. The struct is an array, so maybe also only a pointer? What can i do to make it better?

> 
>> +    int i_i_sofa;  /* Selected Sofa File */
>> +    int *p_delay_l;
>> +    int *p_delay_r;
>> +    float *p_ir_l;
>> +    float *p_ir_r;
>> +};
> 
> Why so many pointers to float and int?
> 

All of these pointers are for arrays, whose lengths are dependent on the media and sofa file. It would be possible to merge the arrays p_sp_a, p_sp_e and p_sp_r into one array, but i think this wouldn't improve readability of the code. 

> 
>> +#define HELP_TEXT N_( "SOFAlizer creates a virtual auditory display, i.e., virtual loudspeakers around the user for listening via headphones. The position of the virtual loudspeakers depends on the audio format of the input file (up to 8.1 supported). SOFAlizer filters audio channels with head-related transfer functions (HRTFs). SOFAlizer uses HRTFs stored in SOFA files (www.sofaconventions.org) following the SimpleFreeFieldHRIR Convention. A database of SOFA files can be found at www.sofacoustics.org.\nWith listener-specific HRTFs, the virtual loudspeakers can be rotated and elevated. With near-field HRTFs, the distance between the loudspeakers and the listener can be varied. SOFAlizer allows to load 5 different SOFA files and easily switch between them. The audio channels can also be presented from one of four pre-defined virtual positions.\nSOFAlizer is developed at the Acoustics Research Institute (ARI) of the Austrian Academy of Sciences." )
> 
> Come on... This is way too long...

The second paragraph describes the function of the parameters, so i put it in the respective LONGTEXT descriptions instead of the add_help()

>> +#define FILE1_NAME_TEXT N_( "SOFA file 1" )
>> +#define FILE2_NAME_TEXT N_( "SOFA file 2" )
>> +#define FILE3_NAME_TEXT N_( "SOFA file 3" )
>> +#define FILE4_NAME_TEXT N_( "SOFA file 4" )
>> +#define FILE5_NAME_TEXT N_( "SOFA file 5" )
> 
> Useless strings addition.

I'm not sure what you mean. Shouldn't i use the predefined strings here, but put them directly in add_loadfile()??

>> 
>> +    p_sys->i_elevation_array[3] = var_CreateGetInteger ( p_aout, "sofalizer-pos4-ele" );
> 
> Use var_Inherit*

Can you please name me one ore two source files to learn the correct use of var_Inherit* ? If I use it, the callbacks from the interface don't work. I orientate on the compressor and spatializer, because they are closest...

Best Regards, Andi


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


More information about the vlc-devel mailing list