[MPlayer-dev-eng] [PATCH 3/7] af_scale*: af_scaletempo

[Reimar Doeffinger]

Hello,
On Mon, Jun 11, 2007 at 12:48:46AM -0400, Robert Juliano wrote:
> af_scaletempo
> 
> This is the main patch.  It adds a filter that, when used
> (-af scaletempo), syncs with playback_speed to scale the audio
> tempo while preserving the pitch.

This filter should include commandline parsing so it can be applied,
tested and used without the other patch.
Also if this filter is any good, people in PAL-land might want to use it
without adjusting playback speed, so not having a comamndline makes this
filter less useful without any good reason.

[...]
> +// Initialization and runtime control
> +static int control(struct af_instance_s* af, int cmd, void* arg)
> +{
> +  af_scaletempo_t* s   = (af_scaletempo_t*)af->setup;

This cast is useless.

[...]
> +    // set af->mul.[nd]
> +    af->mul.n = s->ns_stride;
> +    af->mul.d = s->ns_stride_scaled;
> +    af_frac_cancel(&af->mul);

If it is not yet done, this af_frac_cancel should probably be moved to
af_create or so.

> +    // allocate buffers
> +    if (!s->buf_win) {
> +      s->buf_win = calloc(s->ns_min_win, sizeof(float));
> +      if(!s->buf_win)
> +        af_msg(AF_MSG_FATAL,"[scaletempo] Out of memory\n");
> +    }
> +
> +    if (!s->buf_ovl) {
> +      s->buf_ovl = calloc(s->np_overlap * nch, sizeof(float));
> +      if(!s->buf_ovl)
> +        af_msg(AF_MSG_FATAL,"[scaletempo] Out of memory\n");
> +    }
> +
> +    if (!s->buf_calc) {
> +      s->buf_calc = calloc(s->np_overlap * nch, sizeof(float));
> +      if (!s->buf_calc)
> +        af_msg(AF_MSG_FATAL,"[scaletempo] Out of memory\n");
> +    }

This seems like the wrong place to do it, it should be done either on
open or at the place where np_overlap etc. are changed and then use
realloc.
Also the "Out of memory\n" checks and messages are useless if we just
crash later anyway.


> +// Deallocate memory
> +static void uninit(struct af_instance_s* af)
> +{
> +  af_scaletempo_t* s = (af_scaletempo_t *)af->setup;
> +
> +  if (af->data)
> +      free(af->data->audio);
> +  free(af->data);
> +
> +  if(s->buf_win)
> +    free(s->buf_win);
> +
> +  if(s->buf_ovl)
> +    free(s->buf_ovl);
> +
> +  if (s->buf_calc)
> +    free(s->buf_calc);

While I sometimes do it as well, there is no need to check the argument
to free against NULL.

[...]
> +// Description of this filter
> +af_info_t af_info_scaletempo = {
> +  "Scale audio tempo while maintaining pitch",
> +  "scaletempo",
> +  "Robert Juliano",
> +  "",
> +  AF_FLAGS_NOT_REENTRANT,

Why?

> Index: mplayer-HEAD/libaf/control.h
> ===================================================================
> --- mplayer-HEAD.orig/libaf/control.h	2007-06-10 16:33:13.000000000 -0400
> +++ mplayer-HEAD/libaf/control.h	2007-06-10 16:35:02.000000000 -0400
> @@ -232,4 +232,7 @@
>  // Generic: respond to change in playback_speed
>  #define AF_CONTROL_PLAYBACK_SPEED	0x00002500 | AF_CONTROL_FILTER_SPECIFIC
>  
> +// ScaleTempo
> +#define AF_CONTROL_SCALETEMPO_AMOUNT	0x00002600 | AF_CONTROL_FILTER_SPECIFIC

Useless (and non-doxygen) comment, it says even less than the name
itself.

Greetings,
Reimar Doeffinger