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

[Michael Niedermayer]

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?


[...]
> -#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


[...]

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



[...]

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


[...]
> +            if (bbox.yMax > yMax)
> +                yMax = bbox.yMax;

use FFMAX/FFMIN for such


[...]
> +    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()


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

In the past you could go to a library and read, borrow or copy any book
Today you'd get arrested for mere telling someone where the library is