[FFmpeg-devel] port mplayer eq filter to libavfilter

[William Yu]

2010/11/25 Ronald S. Bultje <rsbultje at gmail.com> wrote:
> Hi,
>
> On Thu, Nov 25, 2010 at 4:27 AM, William Yu <genwillyu at gmail.com> wrote:
>> I have remove unnecessary param from process functions.
>> Please check this patch. Thanks.
>
>[..]
>> + * FFmpeg is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>
> GStreamer has a LGPL variant of this. why not use that? I hate this
> "everything-is-GPL-except-the-wrapper" concept. For trivial code such
> as this, I'd almost say we should reject patches...
>

I had send a mail to original author [D Richard Felker III <dalias
<at> aerifal.cx>]
to ask whether he would like relicense it under LGPL,
But till today, I haven't received his reply.
GStreamer's videobalance filter may be ported after this patch be accepted
Or this porting be rejected forever.

>> +static void process_c( uint8_t *line, int stride,
>> + ? ?int w, int h, int brightness, int contrast)
>> +{
> [..]
>> + ? ?contrast = ((contrast+100)*256*256)/100;
>> + ? ?brightness = ((brightness+100)*511)/200-128 - (contrast>>9);
>
> Can FASTDIV() be used here? (Leave the original code in comments for
> easier understanding).
>
>> +void ff_eq_filter_process_mmx(uint8_t *line, int stride,
>> + ? ?int w, int h, int brightness, int contrast)
> [..]
>> + ? ?while (h--) {
>> + ? ? ? ?__asm__ volatile (
>> + ? ? ? ? ? ?"movq (%3), %%mm3 \n\t"
>> + ? ? ? ? ? ?"movq (%4), %%mm4 \n\t"
>> + ? ? ? ? ? ?"pxor %%mm0, %%mm0 \n\t"
>> + ? ? ? ? ? ?"movl %2, %%eax\n\t"
>
> You will recalculate each of these per row iteration, that sounds like
> a huge waste.
>
>> + ? ? ? ? ? ?ASMALIGN(4)
>> + ? ? ? ? ? ?"1: \n\t"
>> + ? ? ? ? ? ?"movq (%0), %%mm1 \n\t"
>> + ? ? ? ? ? ?"movq (%0), %%mm2 \n\t"
>
> Why not movq mm1, mm2?
>
>> + ? ? ? ?for (i = w&7; i; i--) {
>> + ? ? ? ? ? ?pel = ((*line* contrast)>>12) + brightness;
>> + ? ? ? ? ? ?if (pel&768) pel = (-pel)>>31;
>> + ? ? ? ? ? ?*line++ = pel;
>> + ? ? ? ?}
>
> Please don't mix and match C and ASM, this takes about 10-20 lines in
> asm, if you want you can even compile it using gcc and directly copy
> it (see above, use FASTDIV also). That will prevent it from using eax
> and then the function gets faster.
>

Can you tell why don't mix and match C and ASM. I think compiler's optimizer
can does better than my manual code except those MMX instruction.

> This is so simple, why not write a SSE2 version also? Cheap shot (untested):
> [..]
>> + ? ? ? ?for (i = w&15; i; i--) {
>
> Maybe even a SSSE3 version using pmaddubsw (not sure if that's
> possible)? If you don't understand ASM you don't have to do the SSSE3
> version, but still.

Sorry, I am not enough experience on assembly. When i study these knowledge
enough, I will add those version. Or another people may more suitable
to does these job than me.

>
>> + ? ?__asm__ volatile ( "emms \n\t" ::: "memory" );
>
> Uh... So is every filter going to do this? This is hideous. Let's
> generalize this. Michael, what's the reason for not doing this
> generically?
>
> Ronald

I have fix code to use FASTDIV and av_clip_uint8, Please check this patch again.
Thanks

> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at mplayerhq.hu
> https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: vf_eq.diff
Type: application/octet-stream
Size: 12595 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20101126/cb3edc6f/attachment.obj>