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

[Michael Niedermayer]

Hi

On Thu, Sep 07, 2006 at 12:15:36PM -0300, Gustavo Sverzut Barbieri wrote:
[...]
> >[...]
> >
> >> +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.

yaddadidadda + hahihohuhuhohiho * gigugigogigugig
vs.
ohhmygodwhatshit(yaddadidadda, hahihohuhuhohiho, gigugigogigugig)

its trivial to find out what the first does, but figuring out what the
second does is not, you need to first find out what that function does


[...]
> This patch does not have cosmetic changes, I'll post a "indent" run on
> it later, if this on is approved.
[...]
>  
> -
>  typedef struct {

cosmetic


[...]
>  
> -  RGB_TO_YUV(rgb_color, yuv_color);
> +  rgb_to_yuv(rgb_color, yuv_color);

cosmetic


[...]
> -    ContextInfo *ci=NULL;
> -    char *font=NULL;
> -    unsigned int size=16;
[...]
> +    ContextInfo *ci = NULL;
> +    char *font = NULL;
> +    unsigned int size = 16;

cosmetic

remainder not reviewed as you did not fix the found issues
also dont expect me to review this again anytime soon if you dont split it in
self contained parts as required, if we could have reached a accpetable patch
with 1 or 2 reviews then reviewing such a big change would have been doable
though a lot more work on my part but as i have to point to the same issues
several times its not doable

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