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

[Reimar Döffinger]

On Tue, Oct 12, 2010 at 10:15:27PM +0200, Adrian Stutz wrote:
> Index: command.c
> ===================================================================
> --- command.c	(revision 32482)
> +++ command.c	(working copy)
> @@ -3439,6 +3439,22 @@
>          af_init(mpctx->mixer.afilter);
>          build_afilter_chain(sh_audio, &ao_data);
>          break;
> +    case MP_CMD_AF_CMDLINE:
> +        if (!sh_audio)
> +            break;
> +        char* af_name = strdup(cmd->args[0].v.s);
> +        char* af_args = strdup(cmd->args[1].v.s);
> +        af_instance_t* af = af_get(mpctx->mixer.afilter, af_name);

I don't see the point for the strdups.
Also, adding (initialized) variable declarations in the middle
of a case without opening a new block (using {}) is a really bad idea,
a lot of people are not aware of the slightly tricky semantics (and
tools like coverity will complain about it).
Also (I notice a few things are copied from the surrounding code.
Well, unfortunately for you that is quite a mess)
1) checking for !sh_audio when actually using mixer.afilter does
   not exactly make sense
2) it should use sh_audio->afilter, not mixer.afilter
3) it probably should check that one is not NULL

> +        if (af != NULL)
> +            af->control(af, AF_CONTROL_COMMAND_LINE, af_args);
> +        else
> +            mp_msg(MSGT_CPLAYER, MSGL_WARN,
> +                   "Filter '%s' not found in chain.\n", af_name);
> +        af_init(mpctx->mixer.afilter);

This almost certainly should be
af_reinit(sh_audio->afilter, af);
It should at least have much better performance when modifying
only the last filter in a long filter chain.