[MPlayer-dev-eng] [PATCH] af_scaletempo

[Reimar Döffinger]

Hello,
On Wed, Sep 12, 2007 at 05:44:46PM -0400, Robert Juliano wrote:
[...]
> +      bytes_skip = min(s->bytes_to_slide, bytes_in);

I'd slightly prefer if new code uses FFMIN, since that is what the
remaining MPlayer code will move to.

> +  if (bytes_in > 0) {
> +    int bytes_copy = min(s->bytes_window - s->bytes_filled, bytes_in);
> +    memcpy((int8_t *)s->window + s->bytes_filled,
> +           (int8_t *)data->audio + offset,
> +           bytes_copy);

Check if fast_memcpy provides any speed advantage.

> +  po = s->overlap;
> +  pc = s->pre_corr;
> +  pb = s->blend_pre_corr;
> +  len = s->samples_overlap;
> +  while (len--) {

len-- > 0 is slightly safer, and --len >= 0 might even produce simpler
code (would have to check the generated asm to find out though.
[...]
> +    while (len--) {

Same here.

> +#ifdef USE_INT
> +      corr += ( *pc * *pw ) / s->sloping_divider;

No wonder the int version is slower. Divisions in an inner loop are
horrible.
And since you are doing a sum I don't really see why you have to do the
division here and not outside the loop? It should still be faster and
more precise to do it outside, even if you had to make corr 64 bit to
avoid an overflow.

> +    while (len--) {

len-- > 0 or similar...

> +      *pout = (1 - *pb) * *po + *pb * *pw;

Isn't this the same as (or at least close enough to)
*po + *pb * (*pw - *po)?
The above is only one multiplication instead of 2 (a proper benchmark is
still advisable though).

> +      while (ch--) {
> +        *pb = t;
> +        pb++;
> +      }

These really can be simplified to *pb++ = t;
But I'd actually prefer something like
for (ch = 0; ch < nch; ch++)
  s->blend_pre_corr[ch] = t;
which on x86 is quite likely to be at least not slower if not even
faster (but speed does not matter here anyway).

> diff --git a/mplayer.c b/mplayer.c
> index c0ca8d5..5847712 100644
> --- a/mplayer.c
> +++ b/mplayer.c
> @@ -1203,14 +1203,12 @@ int build_afilter_chain(sh_audio_t *sh_audio, ao_data_t *ao_data)
>      mpctx->mixer.afilter = NULL;
>      return 0;
>    }
> -  new_srate = sh_audio->samplerate * playback_speed;
> -  if (new_srate != ao_data->samplerate) {
> -    // limits are taken from libaf/af_resample.c
> -    if (new_srate < 8000)
> -      new_srate = 8000;
> -    if (new_srate > 192000)
> -      new_srate = 192000;
> -    playback_speed = (float)new_srate / (float)sh_audio->samplerate;
> +  if(af_control_any_rev(sh_audio->afilter,
> +                        AF_CONTROL_PLAYBACK_SPEED | AF_CONTROL_SET,
> +                        &playback_speed)) {
> +    new_srate = sh_audio->samplerate;
> +  } else {
> +    new_srate = sh_audio->samplerate * playback_speed;
>    }

Removing the old code should be done separately, for this patch IMO only
add the
>   if(af_control_any_rev(sh_audio->afilter,
>                         AF_CONTROL_PLAYBACK_SPEED | AF_CONTROL_SET,
>                         &playback_speed))
>     new_srate = sh_audio->samplerate;

After the

>       new_srate = 192000;
>     playback_speed = (float)new_srate / (float)sh_audio->samplerate;
>   }

part.

Greetings,
Reimar Döffinger