[MPlayer-dev-eng] [PATCH 1/7] af_scale*: af_playback_speed_change

[Reimar Doeffinger]

Hello,
On Mon, Jun 11, 2007 at 12:39:10AM -0400, Robert Juliano wrote:
[...]
> -	build_afilter_chain(mpctx->sh_audio, &ao_data);
> +        if (mpctx->sh_audio && !af_playback_speed_change(mpctx->sh_audio->afilter, playback_speed, mpctx->sh_audio->samplerate)) {
> +            playback_speed = prev;
> +            return M_PROPERTY_ERROR;
> +        }

Please do not change the indentation. Also switching from sh_audio to
mpctx->sh_audio (done in e.g. MP_CMD_SPEED_INCR) is an unrelated change
and should not be mixed in.
I also don't really like that the sh_audio NULL check and "playback_speed = prev;"
is needed here each time, esp. since I think your af_playback_speed_change is not
flexible enough to handle correctly failures or filters that can not do the exact
requested speed but only something close.
I think you would get a cleaner solution if you move
af_playback_speed_change with a different name and more functionality
(and preferably a more obvious fallback to the old code, thus also
reducing the number of changes) to mplayer.c



> Index: mplayer-HEAD/libaf/af.c
> ===================================================================
> --- mplayer-HEAD.orig/libaf/af.c	2007-06-10 11:21:55.000000000 -0400
> +++ mplayer-HEAD/libaf/af.c	2007-06-10 12:27:59.000000000 -0400
> @@ -648,6 +648,16 @@
>    return NULL;
>  }
>  
> +// called when playback speed changes
> +int af_playback_speed_change(af_stream_t* s, float speed, int base_srate)
> +{
> +  if(!af_control_any_rev(s, AF_CONTROL_PLAYBACK_SPEED | AF_CONTROL_SET, &speed)) {
> +    // revert to old behavior
> +    s->input.rate = base_srate * speed;
> +  }
> +  return !af_init(s); // make 0 failure
> +}
> +

New functions must include doxygen-compatible descriptions (see e.g. function below).
Also the "// make 0 failure" is rather stupid IMO.

> +/**
> + * \brief responds to change in playback_speed
> + * \param speed of playback
> + * \param base_srate samplerate when speed = 1
> + */
> +int af_playback_speed_change(af_stream_t* s, float speed, int base_srate);

Ok, having the comment here is okay as well, but then (re)move the "//
called when playback speed changes" comemnt above, no good having half
the docs here and the other half there.

> Index: mplayer-HEAD/mencoder.c
> ===================================================================
> --- mplayer-HEAD.orig/mencoder.c	2007-06-10 11:18:40.000000000 -0400
> +++ mplayer-HEAD/mencoder.c	2007-06-10 11:46:53.000000000 -0400

Please try to change the existing code as little as possible for the
first patch, esp. since the new way of doing it has very little
advantages for mencoder.

Greetings,
Reimar Doeffinger