[FFmpeg-devel] [PATCHv4] avutil/common: add av_rint64_clip

[Ronald S. Bultje]

Hi,

On Fri, Nov 13, 2015 at 12:17 PM, Ganesh Ajjanagadde <gajjanag at mit.edu>
wrote:

> On Fri, Nov 13, 2015 at 12:10 PM, Ronald S. Bultje <rsbultje at gmail.com>
> wrote:
> > Hi Ganesh,
> > On Nov 13, 2015 12:02 PM, "Ganesh Ajjanagadde" <gajjanagadde at gmail.com>
> > wrote:
> >>
> >> The rationale for this function is reflected in the documentation for
> >> it, and is copied here:
> >>
> >> Clip a double value into the long long amin-amax range.
> >> This function is needed because conversion of floating point to integers
> > when
> >> it does not fit in the integer's representation does not necessarily
> > saturate
> >> correctly (usually converted to a cvttsd2si on x86) which saturates
> > numbers
> >> > INT64_MAX to INT64_MIN. The standard marks such conversions as
> undefined
> >> behavior, allowing this sort of mathematically bogus conversions. This
> > provides
> >> a safe alternative that is slower obviously but assures safety and
> better
> >> mathematical behavior.
> >> API:
> >> @param a value to clip
> >> @param amin minimum value of the clip range
> >> @param amax maximum value of the clip range
> >> @return clipped value
> >>
> >> Note that a priori if one can guarantee from the calling side that the
> >> double is in range, it is safe to simply do an explicit/implicit cast,
> >> and that will be far faster. However, otherwise this function should be
> >> used.
> >>
> >> avutil minor version is bumped.
> >>
> >> Reviewed-by: Ronald S. Bultje <rsbultje at gmail.com>
> >> Signed-off-by: Ganesh Ajjanagadde <gajjanagadde at gmail.com>
> >> ---
> >>  libavutil/common.h  | 30 ++++++++++++++++++++++++++++++
> >>  libavutil/version.h |  2 +-
> >>  2 files changed, 31 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/libavutil/common.h b/libavutil/common.h
> >> index 6f0f582..f4687ab 100644
> >> --- a/libavutil/common.h
> >> +++ b/libavutil/common.h
> >> @@ -298,6 +298,33 @@ static av_always_inline av_const double
> > av_clipd_c(double a, double amin, double
> >>      else               return a;
> >>  }
> >>
> >> +/**
> >> + * Clip and convert a double value into the long long amin-amax range.
> >> + * This function is needed because conversion of floating point to
> > integers when
> >> + * it does not fit in the integer's representation does not necessarily
> > saturate
> >> + * correctly (usually converted to a cvttsd2si on x86) which saturates
> > numbers
> >> + * > INT64_MAX to INT64_MIN. The standard marks such conversions as
> > undefined
> >> + * behavior, allowing this sort of mathematically bogus conversions.
> > This provides
> >> + * a safe alternative that is slower obviously but assures safety and
> > better
> >> + * mathematical behavior.
> >> + * @param a value to clip
> >> + * @param amin minimum value of the clip range
> >> + * @param amax maximum value of the clip range
> >> + * @return clipped value
> >> + */
> >> +static av_always_inline av_const int64_t av_rint64_clip_c(double a,
> > int64_t amin, int64_t amax)
> >> +{
> >> +#if defined(HAVE_AV_CONFIG_H) && defined(ASSERT_LEVEL) && ASSERT_LEVEL
> >>= 2
> >> +    if (amin > amax) abort();
> >> +#endif
> >> +    // INT64_MAX+1,INT64_MIN are exactly representable as IEEE doubles
> >> +    if (a >=  9223372036854775808.0 || llrint(a) >= amax)
> >> +        return amax;
> >> +    if (a <= -9223372036854775808.0 || llrint(a) <= amin)
> >> +        return amin;
> >
> > Doesn't this allow negative overflows in the max check? I think you need
> > both overflow checks before the min/max checks. Try the next float val
> > smaller than int64_min as input with a small amax, eg 0. I bet it
> returns 0
> > instead of amin.
>
> They are needed. As you and others can clearly see, I am quite bad
> with this stuff.


Hm, so, getting back to my computer, I wanted to test this, and I have this
problem: llrint() works correctly for me for the "undefined" cases, i.e.,
it already does what you're trying to fix in av_rint64_clip_c.

llrint(-10223372056756029440.000000) returns -9223372036854775808
llrint(10223372056756029440.000000) returns 9223372036854775807

So, how do I reproduce that llrint() overflows?

(lldb) disass
libsystem_m.dylib`llrint:
->  0x7fff8a397e70 <+0>:  movl   $0x43e00000, %eax
    0x7fff8a397e75 <+5>:  movd   %eax, %xmm1
    0x7fff8a397e79 <+9>:  psllq  $0x20, %xmm1
    0x7fff8a397e7e <+14>: cmplesd %xmm0, %xmm1
    0x7fff8a397e83 <+19>: cvtsd2si %xmm0, %rax
    0x7fff8a397e88 <+24>: movd   %xmm1, %rdx
    0x7fff8a397e8d <+29>: xorq   %rdx, %rax
    0x7fff8a397e90 <+32>: retq

Since the docs clearly specify that the result is undefined, we still need
your fix, but so the problem is that I can't create a case that I know will
fail, because llrint() doesn't overflow for me, which would be required to
create a failing testcase for the above example I gave...

> Rest looks OK. I'd personally store llrint retval in a variable but let's
> > assume compiler is clever enough to optimize that out.
>
> While refactoring, made sense for me to add a temporary. Unless you
> want to see an updated patch, will push.


Up to you :)

Ronald