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

[Ilkka Ollakka]

On Mon, Sep 23, 2013 at 08:38:25AM -0700, Andreas Fuchs wrote:

Hi,

Thanks for the patch, few remarks inline that I noticed while taking
quick look.

>  modules/audio_filter/sofalizer.c               | 1285 ++++++++++++++++++++++++

> --- /dev/null
> +++ b/modules/audio_filter/sofalizer.c
> @@ -0,0 +1,1285 @@
> +/******************************************************************************
> + * sofalizer.c : SOFAlizer plugin to us SOFA files in vlc
> + *****************************************************************************
> + * Copyright (C) 2013 Andreas Fuchs, ARI
> + *
> + * Authors: Andreas Fuchs <andi.fuchs.mail at gmail.com>
> + *
> + * Licensed under the EUPL, Version 1.1 or – as soon they will be approved by
> + * the European Commission - subsequent versions of the EUPL (the "Licence");
> + * You may not use this work except in compliance with the Licence. You may
> + * obtain a copy of the Licence at: http://ec.europa.eu/idabc/eupl

EUPL seems to be close to GPL?

> +#define N_SOFA 5

Why 5?

> +
> +struct nc_sofa_t
> +{
> +   int i_ncid;
> +   int i_n_samples;
> +   int i_m_dim;
> +   float *p_sp_a;
> +   float *p_sp_e;
> +   float *p_sp_r;
> +   int *p_data_delay;
> +   float *p_data_ir;

Move ints and floats together? I mean that p_data_delay before p_sp_a ?

> +static int MaxDelay ( struct nc_sofa_t *sofa );

Place all forward declarations in one place? (would be better to avoid
those)

> +vlc_module_begin ()
> +    set_description( N_("SOFAlizer") )
> +    set_shortname( N_("SOFAlizer") )
> +    set_capability( "audio filter", 0)
> +    set_help( HELP_TEXT )
> +    add_loadfile( "sofalizer-filename1", "./mit_kemar_normal_pinna.sofa", FILE1_NAME_TEXT, FILE_NAME_LONGTEXT, false)

Is that default something that comes with default with that library or ?

> +static int Open( vlc_object_t *p_this )
> +{

> +    /* Load Sofa files, check for Sampling Rate and valid Selection in the Preferences */
> +    for ( i = 0 ; i < N_SOFA ; i++ )
> +    {
> +        if ( LoadSofa ( p_filter, &c_filename[0], i , &i_samplingrate) != VLC_SUCCESS )

Why givin array of filenames as you only use c_filename[i] in there?

> +    if ( !p_sys->sofa[p_sys->i_i_sofa].i_ncid )
> +    {
> +         for ( i = 0 ; i < N_SOFA ; i++)
> +         {
> +             if ( p_sys->sofa[i].i_ncid )
> +             {
> +                p_sys->i_i_sofa = i;
> +                msg_Err ( intf, "Selected File from Settings invalid. Use File %d", i + 1 );
> +                goto goon;

goon ?

> +/* if ( p_filter->fmt_in.audio.i_rate != i_samplingrate ) //Check Sampling Rate
> +    {
> +        msg_Err( p_filter, "Sampling rate of Input und Sofa file doesn't match. Can't load filter" );
> +        return  VLC_EGENERIC;
> +    }
> +*/

Any reason it's included but commented out?

> +    /*for ( i = 0 ; i < p_sys->i_n_conv ; i++ )
> +    {
> +        msg_Info( intf, "Speakerpostion: %f", *(p_sys->p_speaker_pos+i) );
> +    }*/

Leftover debug code?

> +/* Prepares the data structures for the threads and starts them */
> +static block_t *DoWork( filter_t *p_filter, block_t *p_in_buf )
> +{

> +    t_data_l.p_sys = p_sys;
> +    t_data_r.p_sys = p_sys;

You could set these like this (and others that are actually same data
for some reason duplicated?):

t_data_l.p_sys =
t_data_r.p_sys = p_sys;


> +    float *p_dest_l = (float *)p_out_buf->p_buffer;
> +    float *p_dest_r = (float *)p_out_buf->p_buffer + 1;
> +    if ( p_sys->b_mute )
> +    {
> +        for ( uint32_t i = 0 ; i < p_in_buf->i_nb_samples ; i++ )
> +        {
> +             *(p_dest_l) = 0;
> +             *(p_dest_r) = 0;
> +             p_dest_l += 2;
> +             p_dest_r += 2;
> +        }

Maybe memset would also do the trick in here instead of that for-loop ?

> +/* Writes the samples of the input buffer in the ringbuffer and convolutes with the impulse response */
> +void sofalizer_Convolute ( void *p_ptr )
> +{
> +    for ( int i = t_data->p_in_buf->i_nb_samples ; i-- ; )
> +    {
> +        *( p_dest ) = 0;
> +        for ( int l = 0 ; l < i_input_nb ; l++ )
> +        {
> +            *( p_ringbuffer[l] + i_write ) = *( p_src++);  

memcpy ?
> +
> +static int GetSpeakerPos ( filter_t *p_filter, float *p_speaker_pos )
> +{

Switch/case or even if/elseif in here?

> +}
> +

These could be separated patch (qt4 part), even if they are quite small
> --- a/modules/gui/qt4/components/extended_panels.cpp
> +++ b/modules/gui/qt4/components/extended_panels.hpp
> --- a/modules/gui/qt4/dialogs/extended.cpp

I didn't look at threads/locking at all

-- 
Ilkka Ollakka
What kind of love is that?  Not to be loved; never to have shown love.
		-- Commissioner Nancy Hedford, "Metamorphosis",
		   stardate 3219.8
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 490 bytes
Desc: Digital signature
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20130923/1cd62e33/attachment.sig>