[Ffmpeg-devel] [PATCH] drawtext.c: 07fix glyphs cache and ft_pos_to_pixels

[Michael Niedermayer]

Hi

On Sat, Sep 23, 2006 at 01:26:29PM -0300, Gustavo Sverzut Barbieri wrote:
> On 9/12/06, Gustavo Sverzut Barbieri <barbieri at gmail.com> wrote:
> >On 9/12/06, Michael Niedermayer <michaelni at gmx.at> wrote:
> >> Hi
> >>
> >> On Sun, Sep 10, 2006 at 03:55:12PM -0300, Gustavo Sverzut Barbieri wrote:
> >> > This fix glyphs cache when it cannot load character (fix possible
> >> > access to non-initialized memory) an declare ft_pos_to_pixels()
> >> > function.
> >> >
> >> > it's simple, but seems long since code was moved into another block
> >> > (and thus indentation level) due an "if" clause.
> >>
> >> to quote our policy:
> >> "NOTE: If you had to put if(){ .. } over a large (> 5 lines) chunk of 
> >code,
> >> then either do NOT change the indentation of the inner part within (don't
> >> move it to the right)! or do so in a separate commit"
> >
> >I know what you mean, but given this patch size, are this really required?
> >
> >
> >> [...]
> >> > +/**
> >> > + * FreeType represent values in 64/th of pixels.
> >> > + */
> >> > +static inline int ft_pos_to_pixels(FT_Pos value)
> >> > +{
> >> > +  return value >> 6;
> >> > +}
> >> > +
> >>
> >> is this really needed? if so why dont you replace every a+b by add(a,b) ?
> >> anyway is the rounding correct at all?
> >
> >I put this inside a function because I was lost when I read my own
> >code on why 64... this make it clear... and it have nothing to do with
> >add(a,b) since you have a constant... maybe add_b(a) would be more
> >likely to express what you mean. I would keep it.
> >
> >as if it's ok or not, I just followed FreeType examples when writing this.
> 
> any other objection?

repost the patch without the reindentions, i wont review patches which mix
functional changes with cosmetics

[...]

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