[MPlayer-dev-eng] [PATCH] af_scale* command-line only

[Reimar Doeffinger]

Hello,
On Wed, Jun 13, 2007 at 12:50:47AM -0400, Robert Juliano wrote:
[...]
> + * scale rate (both time and tempo) (i.e. chipmunk effect)
> + *
> + * basic technique:
> + *   pretend input rate is scaled rate and resample to input rate
[...]
> +#ifdef USE_LIBAVCODEC
> +  #define RESAMPLER "lavcresample"
> +#else
> +  #define RESAMPLER "resample"
> +#endif

Hm... is there any specific reason to do it in a separate filter?
Seems to me like adding it to resample would be a three-line change (
adding a speedup double to af_resample_t and using it in two places in
REINIT) + adding the two controls.
Advantages of that approach: An already loaded resample filter can be
reused so this feature does not use extra CPU in this case, the
selection of fast vs. high-quality resampling via -af-adv works.

> +  af_scalerate_t* s   = af->setup;

A space too much.

> +#define AOF(a, i) &(((float *)a)[i])

Really ugly.
Just use a local float* variable.

[...]
> +int fill_window(struct af_instance_s* af, af_data_t* c, int ic) {

Why do you "always" name the af_data_t pointers "c"? That name is really
unhelpful and even "d" would be a saner (but still horrible) choice.

> +  af_scaletempo_t* s = af->setup;
> +  int ns_in = c->len/c->bps - ic;

"ns" is not a descriptive name either.

> +  int ic0 = ic;
> +  int bps = af->data->bps;
> +  float* win = s->buf_win;

Hmm... the code looks to me like you're doing something fifo-like?
Maybe you can use libavutil/fifo.h to simplify things?

> +// Filter data through filter
> +static af_data_t* play(struct af_instance_s* af, af_data_t* data)
> +{
> +  af_scaletempo_t* s = af->setup;
> +  af_data_t* c = data;          // Current working data
> +  af_data_t* l = af->data;      // Local data
> +  int ns_in    = c->len/c->bps; // Length of input in samples

How about not commenting it but instead giving it a better name like
samplecnt? There sure are even better names for it to be found...

> +  // RESIZE_LOCAL_BUFFER - can't use macro
> +  // number of strides in input * size of output stride
> +  nb_out = (int) ((ns_in / s->ns_stride_scaled + 1) * s->ns_stride * bps + 1);

Why the cast?

> +    // output overlap
> +    for (i=0, io=0, iw=ns_off; i<s->np_overlap; i++) {
> +      float t = i / (float)s->np_overlap;

Doing a floating point division for every sample is bound to be really
slow.
While avoiding floating point completely would be the nicest solution
for many reasons, pre-calculating 1/(float)s->np_overlap is still an
improvement (though I somewhat have the feeling that operating on 16 bit
samples instead of float would only barely make the code more
complicated).

[...]
> +    float srate = c->rate / 1000;
[...]
> +    // set through experimentation - same as SoundTouch
> +    s->np_seek = srate * 14;
> +    s->np_window = srate * 82;
> +    s->np_overlap = srate * 12;

using directly srate * 14 / 1000 etc. would work just as well unless you
expect sample rates > 40.000.000 and not have architecture-specific
rounding errors.

> +      if (AF_ERROR == (rv = converter->control(converter, AF_CONTROL_FORMAT_FMT | AF_CONTROL_SET, &(fmt_tempo.format))) )
> +	return rv;
> +      if (AF_ERROR == (rv = converter->control(converter, AF_CONTROL_REINIT, &fmtc)) )
> +      return rv;

Weird indentation, and a tab slipped in.

> @@ -131,6 +137,8 @@
>    
>    af_msg(AF_MSG_VERBOSE,"[libaf] Adding filter %s \n",name);
>    
> +  new->stream = s;
> +

What is this needed for?

> Index: libaf/af.h
> ===================================================================
> --- libaf/af.h	(revision 23550)
> +++ libaf/af.h	(working copy)
> @@ -64,6 +64,7 @@
>    af_data_t* data; // configuration for outgoing data stream
>    struct af_instance_s* next;
>    struct af_instance_s* prev;  
> +  struct af_stream_s* stream; // allow filters to create, add, etc. other filters

Hm. Probably should have paid more attention to that part, will review
it more closely another time.

Greetings,
Reimar Doeffinger