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

[Laurent Aimar]

> On Wed, Jun 18, 2008 at 11:26:54PM +0200, Laurent Aimar wrote:
> >  As asked by Remi, are you the only author of all the code of scaletempo.c ? (as
> > you have said  it is "Ported from GStreamer"). If not you should add back the original
> > authors.
> 
> Sole author.  As well as for the MPlayer version under a
> different name, which is stuck in svn due to a code freeze
> just before submitting the patch.  Darn my timing.
 Ok.

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

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

> > > +    aout_instance_t *p_aout = (aout_instance_t *)p_filter->p_parent;
> > > +    p_sys->ms_stride       = var_CreateGetInteger( p_aout, "scaletempo-stride" );
> > > +    p_sys->percent_overlap = var_CreateGetFloat( p_aout,   "scaletempo-overlap" );
> > > +    p_sys->ms_search       = var_CreateGetInteger( p_aout, "scaletempo-search" );
> >  I find it weird to create those variables in aout but I don't know if it is
> > the current policy.
> 
> I saw other filters do it.  This allows dynamically changing
> the parameters.  It's unnecessary, so I can change them to
> config_Get...
 Ok, no need.

> >   I think it would be more userfriendly if scaletempo was selected by
> > default (instead of specifying by audio-filter). The code loading filters
> > (audio-filter) could be made a function and then reused. Of course, an option
> > should be added to disable this behavior.
> 
> If there's consensus then, yeah.  I don't know how to make
> it the default.
 It could be done latter.

> > > -        if( p_input->pp_resamplers[0]->input.i_rate == 1000 * p_input->input.i_rate / i_input_rate )
> > > +        uint i_nominal_rate =
> >  Use unsigned int instead.
> 
> Instead of what?  Isn't it an unsigned int?
 For consistency purpose, we do not use uint inside VLC. (No need to change
scaletempo.c as you have imported the code and it will simplify update/synch)

> > > + * Scaletempo works by producing audio in constant sized chunks (a "stride") but
> > > + * consuming chunks proportional to the playback rate.
> > > + *
> > > + * Scaletempo then smooths the output by blending the end of one stride with
> > > + * the next ("overlap").
> > > + *
> > > + * Scaletempo smooths the overlap further by searching within the input buffer
> > > + * for the best overlap position.  Scaletempo uses a statistical cross correlation
> > > + * (roughly a dot-product).  Scaletempo consumes most of its CPU cycles here.
> > > + *
> > > + * NOTE:
> > > + * sample: a single audio sample for one channel
> > > + * frame: a single set of samples, one for each channel
> > > + * VLC uses these terms differently
> > > + */
> >   About the algo, I did find the quality a bit low (especially at high speed).
> 
> I don't doubt that, if you have good ears.
> 
> I find the VLC version produces the worse sound because
> aout_InputPlay doesn't seem to like filters buffering/skipping,
> which can get relatively large at high speeds.  msg_Dbg
> reports a lot of dropped buffers and drift correction.  That's
> not something I can fix.  Maybe there is some way to let
> aout_InputPlay know about the buffering, but I didn't figure
> it out.
 Ok, it could be improved latter on VLC.

> > I have played with audio time-stretching before and I had a better quality if
> > I did not use "constant sized chunks".
> >   For that I estimate the period using autocorellation and then I duplicated or
> > overlapped the audio by chunk equal to the local period. The duplication or
> > overlapping amount was controled by the scale factor I wanted. It is a bit
> > more complicated though.
> 
> I'd like to see more, code would be nice.
 Yep, you can download my old try at (LGPL, tar gz of my mercurial project)
http://people.via.ecp.fr/~fenrir/xTs-dev-23-0f88d02b4cb6.tar.gz
But as you say, maybe the quality problem come from VLC (I have not yet
checked).

-- 
fenrir