[MPlayer-dev-eng] [PATCH] SMPlayer's audio equalizer slave command patch

[Reimar Döffinger]

On Thu, Oct 14, 2010 at 10:27:45PM +0200, Adrian Stutz wrote:
> On Tue, Oct 12, 2010 at 23:06, Reimar Döffinger
> <Reimar.Doeffinger at gmx.de> wrote:
> > I think that would be a good idea.
> 
> Ok, now as two patches:
> 1 adds af_reinit to af.h
> 2 is the old patch with af_reinit instead of af_init
> 
> Is this fine to apply now?
> 
> Greetings,
> Adrian

> Index: libaf/af.c
> ===================================================================
> --- libaf/af.c	(revision 32492)
> +++ libaf/af.c	(working copy)
> @@ -254,7 +254,7 @@
>  /* Reinitializes all filters downstream from the filter given in the
>     argument the return value is AF_OK if success and AF_ERROR if
>     failure */
> -static int af_reinit(af_stream_t* s, af_instance_t* af)
> +int af_reinit(af_stream_t* s, af_instance_t* af)
>  {
>    do{
>      af_data_t in; // Format of the input to current filter
> Index: libaf/af.h
> ===================================================================
> --- libaf/af.h	(revision 32492)
> +++ libaf/af.h	(working copy)
> @@ -163,6 +163,13 @@
>  void af_uninit(af_stream_t* s);
>  
>  /**
> + * \brief  Reinit the filter list from the given filter on downwards
> + * \param  Filter instance to begin the reinit from
> + * \return AF_OK on success or AF_ERROR on failure
> + */
> +int af_reinit(af_stream_t* s, af_instance_t* af);

Remove the documentation from the .c file, duplicated documentation
only means that sooner or later at least on will be wrong.

> +    case MP_CMD_AF_CMDLINE:
> +        if (sh_audio) {
> +            af_instance_t* af = af_get(sh_audio->afilter, cmd->args[0].v.s);
> +            if (af != NULL)
> +                af->control(af, AF_CONTROL_COMMAND_LINE, cmd->args[1].v.s);
> +            else
> +                mp_msg(MSGT_CPLAYER, MSGL_WARN,
> +                       "Filter '%s' not found in chain.\n", cmd->args[0].v.s);
> +            af_reinit(sh_audio->afilter, af);

I don't think you should call reinit if af is NULL.
I actually propose to write it like this:
if (!af) {
    mp_msg(...);
    break;
}
handling of "exceptional" cases should clutter the code as little
as possible and stand out as a special case IMO.