[MPlayer-dev-eng] [PATCH] New commands for loading/unloading audio filters at runtime

[Jehan Pagès]

Hi,

attached is the modified patch. I made it a "if" because I find it
clearer than a switch in this case (just made a 2 cases branching, if
MP_CMD_AF_DEL, else…). I hope you don't mind (is there any performance
difference anyway, or any other notable difference, between an if and
a switch?).
To be honest, I don't really agree with this choice of compacity over
efficiency. Ok, as you noted, this piece of code is not performance
critical, and anyway it probably won't make any difference on any
computer, even old ones (moreover the usage won't have people loading
or unloading a lot of filters at the same time, I guess). But still,
by principle, just to have a code more compact, we are adding
comparisons at every loop. This is not my idea of the most beautiful
code. :p But you are the boss anyway, and this is not that a big deal,
so I won't argue. I just give my opinion on it because maybe you have
a nice answer to enlighten my vision on beautiful code. :)

I also modified the two other issues that you noted previously (I hope
I didn't miss any occurrence of these, though my patch is not that
big).
Anyway I hope that now you can include the code into mplayer. :)
See you.

Jehan

On Sun, Apr 4, 2010 at 3:40 PM, Reimar Döffinger
<Reimar.Doeffinger at gmx.de> wrote:
> On Sun, Apr 04, 2010 at 02:58:38AM +0900, Jehan Pagès wrote:
>> >> +    case MP_CMD_AF_ADD:
>> >> +        if (!sh_audio)
>> >> +            break;
>> >> +        {
>> >> +            char* af_args = strdup(cmd->args[0].v.s);
>> >> +            char* af_commands = af_args;
>> >> +            char* af_command = strsep (&af_commands, ",");
>> >> +            while (af_command != NULL)
>> >> +            {
>> >> +                af_add ((&mpctx->mixer)->afilter, af_command);
>> >> +                af_command = strsep (&af_commands, ",");
>> >> +            }
>> >> +            build_afilter_chain(sh_audio, &ao_data);
>> >> +            free (af_args);
>> >> +        }
>> >
>> > Hm, this boiler-plate code is always the same and it's not performance-critical.
>> > Maybe you could do a switch inside the while and this avoid duplicating
>> > any code?
>>
>> The duplicated code that annoys you here is the strsep line before the
>> loop and then inside it? What do you mean "do a switch inside the
>> while"?
>
> No I meant
>> >> +        if (!sh_audio)
>> >> +            break;
>> >> +        {
>> >> +            char* af_args = strdup(cmd->args[0].v.s);
>> >> +            char* af_commands = af_args;
>> >> +            char* af_command = strsep (&af_commands, ",");
>> >> +            while (af_command != NULL)
>> >> +            {
> switch (command)
> {
> case MP_CMD_AF_ADD:
>> >> +                af_add ((&mpctx->mixer)->afilter, af_command);
> break;
> case MP_CMD_AF_DEL:
> af_del(...);
> break;
>> >> +                af_command = strsep (&af_commands, ",");
>> >> +            }
>> >> +            build_afilter_chain(sh_audio, &ao_data);
>> >> +            free (af_args);
>> >> +        }
> _______________________________________________
> MPlayer-dev-eng mailing list
> MPlayer-dev-eng at mplayerhq.hu
> https://lists.mplayerhq.hu/mailman/listinfo/mplayer-dev-eng
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: mplayer_af_filters2.patch
Type: text/x-patch
Size: 3919 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20100407/b470bde9/attachment.bin>