[FFmpeg-devel] port mplayer eq filter to libavfilter

[Michael Niedermayer]

On Thu, Nov 25, 2010 at 10:18:21AM -0500, Ronald S. Bultje 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.
> 
> Please don't top-post.
> 
> > +It accepts the following parameters:
> > + at var{brightness}:@var{constrast}
> 
> Range of values?
> 
> > + * 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...
> 

> > +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).

this code does not look speed critical


> 
> > +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.

per row code is >300 times less important speedwise than per pixel.
Not saying this cant or shoulndt be improved, just saying that it wont help
speed



[...]
> > +    __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?

you leave too little context in the code chunk for me to understand your
question unambihgously


[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Good people do not need laws to tell them to act responsibly, while bad
people will find a way around the laws. -- Plato
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20101125/4b3d5fa4/attachment.pgp>