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

[Ganesh Ajjanagadde]

On Fri, Nov 13, 2015 at 1:28 PM, Ronald S. Bultje <rsbultje at gmail.com> wrote:
> 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?

The link I gave originally
http://blog.frama-c.com/index.php?post/2013/10/09/Overflow-float-integer
gives an illustration. Maybe the weird behavior happens only on
9223372036854775808.0. This happens because INT64_MAX+1 is not
representable in long long, and hence signed overflow occurs yielding
INT64_MIN (of course undefined). Here is a minimal test program:
#include <stdio.h>
#include <math.h>

int main(void) {
    printf("%lld\n", llrint(9223372036854775808.0));
    return 0;
}

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

yes, this is the cvtsd2si mentioned in the link and alluded to by me
in this thread and related discussion. Rest is the standard
boilerplate.

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

It is trivial enough, will push later.

>
> Ronald
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel