[Ffmpeg-devel] clip_uint8

[Michael Niedermayer]

Hi

On Sun, Apr 30, 2006 at 11:52:43PM +0200, Aurelien Jacobs wrote:
[...]
> > > > @@ -437,7 +437,7 @@ static inline int clip(int a, int amin,
> > > >          return a;
> > > >  }
> > > > 
> > > > -static inline int clip_uint8(int a)
> > > > +static inline uint8_t clip_uint8(int a)
> > > >  {
> > > >      if (a&(~255)) return (-a)>>31;
> > > >      else          return a;
> > > 
> > > I verified the issue, tested your patch and applied it.
> > 
> > i assume theres a missunderstanding, my reply that the code was ok ad not
> > slow meant the original code not that the change is acceptable,
> 
> No missunderstanding here. It was clear that you didn't meant the change
> was acceptable. But you also didn't said it was not ;-)
> 
> > it obviously is not without extensive benchmarking 
> 
> What kind of benchmarking do you recommend here ?

after some greping, none, practically every use of this
writes the stuff to a uint8_t so all should be equally fast


> 
> > IMHO the behaviour should be documented and not changed
> 
> Hum... The behavior was somewhat unexpected when the result of
> clip_uint8 is assigned to anything else than uint8_t. And after
> this patch, I saw no behavior changes, no regression...
> So I guess clip_uint8 was only used assigned to uint8_t. Which means
> the (implicit) cast to uint8_t was done at some point anyway.
> 
> > or the cast to uint8_t could be done only on the overflow
> > side of the if()
> 
> Do you mean that the following patch would be better ?

maybe ...
either way, if you change the behaviour of a function you must
check every case where its used, the case in mpegvideo.c for example
contains a unneeded & 0xFF after either change
the function definitly behaved as expected, now it does something else
iam not sure what we gain with that change? its not a bugfix, behaviour
which is no longer what it was is still not documented

[...]
-- 
Michael

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