[vlc-devel] [PATCH] Add Scaletempo audio filter

[Rov Juvano]

On Thu, Jun 19, 2008 at 09:31:32PM +0200, Laurent Aimar wrote:
> > > > +static int StrideCallback( vlc_object_t *p_this, char const *psz_cmd,
> > > > +static int OverlapCallback( vlc_object_t *p_this, char const *psz_cmd,
> > > > +static int SearchCallback( vlc_object_t *p_this, char const *psz_cmd,
> > > > +                           vlc_value_t oldval, vlc_value_t newval, void *p_data )
> > > > +{
> > > > +    VLC_UNUSED(p_this); VLC_UNUSED(psz_cmd); VLC_UNUSED(oldval);
> > > > +    aout_filter_sys_t *p = (aout_filter_sys_t *)p_data;
> > > > +    uint new_value = newval.i_int;
> > > > +    if( p->ms_search != new_value ) {
> > > > +        p->ms_search = new_value;
> > > > +        p->reinit_buffers = true;
> > > > +    }
> > > > +    return VLC_SUCCESS;
> > > > +}
> > >  Those callbacks are not thread safe. They can be called while you are in
> > > DoWork(). You need to add a proper locking scheme. Reading your code, it
> > > seems you just need to lock inside these callbacks and around
> > >  if( p->reinit_buffers ) { reinit_buffers() }
> > > in DoWork.
> > 
> > Because these values are independent of each other, are used
> > only within reinit_buffers, and then used only once, the worse
> > case scenario would be a delay for them taking effect, right?
>  You need to use a memory barrier around variable accessed by multiple
> threads to ensure that you actually see the real value. The lock provides
> that. On patological system, one CPU could modify the value without the other
> CPU never seeing it.
>  Beside it avoids future errors in case the current behavior (ie no variable
> dependency) is changed.
> 
> > >  You probably want to check thoses value against sane bounds (here or in
> > > reinit_buffers). What happens if ms_stride is 0 for example ?
> > 
> > I thought they wouldn't be called if the values were out
> > bounds.  Are the min/max of add_*_with_range just informative?
>  I have no idea if they work when set by var_Set*. So I will wait for someone
> to correct me.

You were right.

I'm going to remove them since they aren't really interactive
parameters.

> 
> > > > +    if( p_filter->input.i_format != VLC_FOURCC('f','l','3','2') )
> > > > +    {
> > > > +        b_fit = false;
> > > > +        p_filter->input.i_format = p_filter->output.i_format = VLC_FOURCC('f','l','3','2');
> > > > +        msg_Warn( p_filter, "bad input or output format" );
> > > > +    }
> > > > +    if( ! AOUT_FMTS_SIMILAR( &p_filter->input, &p_filter->output ) )
> > > > +    {
> > > > +        b_fit = false;
> > > > +        memcpy( &p_filter->output, &p_filter->input, sizeof(audio_sample_format_t) );
> > > > +        msg_Warn( p_filter, "input and output formats are not similar" );
> > > > +    }
> > > > +
> > > > +    if( ! b_fit )
> > > > +    {
> > > > +        return VLC_EGENERIC;
> > > > +    }
> > >  I am not sure you can change p_filter->input/p_filter->output if you
> > > return VLC_EGENERIC.
> > 
> > The input and output formats should be the same.  Did I do
> > something to change them?
>  You do
>   p_filter->input.i_format = p_filter->output.i_format = VLC_FOURCC('f','l','3','2');
> but latter you may refuse by
>   if( ! b_fit ) return VLC_EGENERIC;
> The current way the filters are probed will be broken by that. You should write
> to p_filter->input and p_filter->output only if your filter will accept to process
> the data. I think in your case you could just remove the b_fit test.

You're going to have convince me to do it differently than
the other audio filters, or is this because I left out the
check for the output format:

if( p_filter->input.i_format != VLC_FOURCC('f','l','3','2') )
should be
if( p_filter->input.i_format != VLC_FOURCC('f','l','3','2' ) ||
    p_filter->output.i_format != VLC_FOURCC('f','l','3','2') )

> > >   About the algo, I did find the quality a bit low (especially at high speed).

This also depends on what you mean by high speed, which by
my definition, vlc's faster/slower falls into that category.
I'm targetting upto +-25% where speech is still fairly easy
to understand.  I haven't been too concerned with speeds
beyond +-100%.

--
rovjuvano