[Ffmpeg-devel] [PATCH] rewrite vhook/drawtext.c

[Gustavo Sverzut Barbieri]

On 9/7/06, Michael Niedermayer <michaelni at gmx.at> wrote:
> Hi
>
> On Wed, Sep 06, 2006 at 11:11:21PM -0300, Gustavo Sverzut Barbieri wrote:
> > On 9/6/06, Diego Biurrun <diego at biurrun.de> wrote:
> > >On Wed, Sep 06, 2006 at 12:25:39PM -0300, Gustavo Sverzut Barbieri wrote:
> > >> On 9/6/06, Diego Biurrun <diego at biurrun.de> wrote:
> > >> >On Wed, Sep 06, 2006 at 11:22:04AM -0300, Gustavo Sverzut Barbieri
> > >wrote:
> > >> >> On 9/6/06, Diego Biurrun <diego at biurrun.de> wrote:
> > >> >> >On Wed, Sep 06, 2006 at 10:13:19AM -0300, Gustavo Sverzut Barbieri
> > >> >wrote:
> > >> >> >>
> > >> >> >> --- vhook/Makefile    (revision 6179)
> > >> >> >> +++ vhook/Makefile    (working copy)
> > >> >> >> @@ -35,7 +35,7 @@
> > >> >> >>
> > >> >> >>  %$(SLIBSUF): %.o
> > >> >> >> -     $(CC) $(LDFLAGS) -g -o $@ $(VHOOKSHFLAGS) $<
> > >> >> >> +     $(CC) $(LDFLAGS_$@) $(VHOOKLIBS) $(LDFLAGS) -g -o $@
> > >> >> >$(VHOOKSHFLAGS) $<
> > >> >> >
> > >> >> >Why do you add $(VHOOKLIBS) here?
> > >> >>
> > >> >> it was a remaining from tests with cygwin patch.
> > >> >>
> > >> >> before I sent another patch with just this fix, do you really require
> > >> >> incremental patches? It will take me some time and as I said, it's
> > >> >> better to review the new code instead of changes. But if really
> > >> >> required I can do :-)
> > >> >
> > >> >I'm not quite sure what you mean by incremental here.
> > >>
> > >> like the linux kernel guys like, something like "[PATCH 0/N] replace
> > >> macros with inline functions" "[PATCH 1/N] ..."
> > >
> > >It's not the Linux kernel guys.  Every reviewer loves small and
> > >self-contained patches.
> >
> > Okay, not split... but less moving around make it easier to read.
>
> rejected, mixing cosmetics and functional changes is not acceptable
>
> a few comments for the first few parts of the patch are below
>
>
> [...]
>
> >  #define MAXSIZE_TEXT 1024
> > +#define _XOPEN_SOURCE 600
>
> for what is that needed?

it's to get posix_fadivse(), anyway, this is causing much "trouble"
now and the way I used to avoid broken compiles on non conformant
systems (#ifdef posix_fadvise) does not work. It then requires a
configure test, that will go in another patch.

> [...]
> > -#define RGB_TO_YUV(rgb_color, yuv_color) { \
> > -    yuv_color[0] = ( 0.257 * rgb_color[0]) + (0.504 * rgb_color[1]) + (0.098 * rgb_color[2]) +  16; \
> > -    yuv_color[2] = ( 0.439 * rgb_color[0]) - (0.368 * rgb_color[1]) - (0.071 * rgb_color[2]) + 128; \
> > -    yuv_color[1] = (-0.148 * rgb_color[0]) - (0.291 * rgb_color[1]) + (0.439 * rgb_color[2]) + 128; \
> > +static inline void
> > +rgb_to_yuv(const uint8_t rgb[3],
> > +           uint8_t yuv[3])
> > +{
> > +    yuv[0] = (0.257 * rgb[0]) + (0.504 * rgb[1]) + (0.098 * rgb[2]) +  16;
> > +    yuv[2] = (0.439 * rgb[0]) - (0.368 * rgb[1]) - (0.071 * rgb[2]) + 128;
> > +    yuv[1] = (-0.148 * rgb[0]) - (0.291 * rgb[1]) + (0.439 * rgb[2]) + 128;
> >  }
>
> floating point code makes regression tests imposible and in that case its
> not needed at all

I was not aware of RGB_TO_{Y,U,V} macros, thanks.



> [...]
>
> > +static inline uint_fast16_t
> > +linearize_coord(const uint_fast16_t linesize,
> > +                const uint_fast16_t x,
> > +                const uint_fast16_t y)
> > +{
> > +    return x + y * linesize;
> >  }
>
> i would prefer not to wrap such trivial things in functions

these make code easier to read and review. sometimes variables are as
beauty as x + y * linesize, but sometimes they're not and you will not
figure what code is doing that fast.

Since it's marked as inline (maybe I should go with "always inline"?
but in this case I think gcc will do right), there is no overhead.


> [...]
>
> >
> > -void Release(void *ctx)
> > +void
> > +Release(void *ctx)
>
> as you are the author of this i wont object to changes like that even though
> i think this is making the code significantly harder to read but i will not
> accept such a change mixed with functional changes in the same patch

Ok, I'll split it into cosmetic and functional patches.


> [...]
> > +            if (bbox.yMax > yMax)
> > +                yMax = bbox.yMax;
>
> use FFMAX/FFMIN for such

ok


> [...]
> > +    for (j = y; j < h; j++)
> > +        for (i = x; i < w; i++)
> > +            set_pixel(picture, yuv_color, i, j);
>
> code like this is unaccptable as its too slow, use memset()


Okay, try #5 here we go.

This patch does not have cosmetic changes, I'll post a "indent" run on
it later, if this on is approved.

-- 
Gustavo Sverzut Barbieri
--------------------------------------
Jabber: barbieri at gmail.com
   MSN: barbieri at gmail.com
  ICQ#: 17249123
 Skype: gsbarbieri
Mobile: +55 (81) 9927 0010
 Phone:  +1 (347) 624 6296; 08122692 at sip.stanaphone.com
   GPG: 0xB640E1A2 @ wwwkeys.pgp.net
-------------- next part --------------
A non-text attachment was scrubbed...
Name: drawtext-rewrite5.patch
Type: text/x-patch
Size: 25577 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20060907/296b2cda/attachment.bin>