[Ffmpeg-devel] clip_uint8

Aurelien Jacobs aurel
Sun Apr 30 23:52:43 CEST 2006


On Sun, 30 Apr 2006 22:18:25 +0200
Michael Niedermayer <michaelni at gmx.at> wrote:

> Hi
> 
> On Sun, Apr 30, 2006 at 07:49:30PM +0200, Aurelien Jacobs wrote:
> > On Fri, 28 Apr 2006 17:58:56 +0200
> > Panagiotis Issaris <takis.issaris at uhasselt.be> wrote:
> > 
> > > Hi,
> > > 
> > > The clip function clips between the amin and amax values given as parameters, 
> > > the clip_uint8 function, actually behaves a bit differently. The lower bound
> > > is indeed 0 as the uint8 postfix in the name implies, but upper values (values 
> > > higher then 255) are returned as -1. This works if the value which stores the 
> > > returned value is uint8, but in other cases will cause strange behavior imho.
> > > 
> > > [...]
> > > 
> > > Changing the return type to uint8_t fixes this, or was the -1 
> > > return value intentional?
> > > 
> > > diff --git a/libavutil/common.h b/libavutil/common.h
> > > index d8f2c40..92c0d08 100644
> > > --- a/libavutil/common.h
> > > +++ b/libavutil/common.h
> > > @@ -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 ?

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

--- libavutil/common.h	30 Apr 2006 17:49:11 -0000	1.164
+++ libavutil/common.h	30 Apr 2006 21:37:39 -0000
@@ -437,9 +437,9 @@
         return a;
 }
 
-static inline uint8_t clip_uint8(int a)
+static inline int clip_uint8(int a)
 {
-    if (a&(~255)) return (-a)>>31;
+    if (a&(~255)) return (uint8_t) (-a)>>31;
     else          return a;
 }

Aurel





More information about the ffmpeg-devel mailing list