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

[Ganesh Ajjanagadde]

On Fri, Nov 13, 2015 at 9:59 AM, Ronald S. Bultje <rsbultje at gmail.com> wrote:
> Hi,
>
> On Fri, Nov 13, 2015 at 9:23 AM, 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  | 34 ++++++++++++++++++++++++++++++++++
>>  libavutil/version.h |  4 ++--
>>  2 files changed, 36 insertions(+), 2 deletions(-)
>>
>> diff --git a/libavutil/common.h b/libavutil/common.h
>> index 6f0f582..54e5109 100644
>> --- a/libavutil/common.h
>> +++ b/libavutil/common.h
>> @@ -298,6 +298,37 @@ 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 >=  9223372036854775807.0)
>>
>> +        return FFMIN( 9223372036854775807, amax);
>> +    if (a <= -9223372036854775808.0)
>> +        return FFMAX(-9223372036854775807-1, amin);
>
>
> Uhm... OK, so this is turning magical very quickly. Now, we understand what
> you're doing here, but to a casual observer coming in here two years ahead,
> this makes no sense. I don't see INT64_MAX + 1 anywhere as an IEEE double,
> so the comment is confusing. What are these constants? And why is the double
> version of INT64_MIN written one way but the integer version another.
>
> WHAT IS GOING ON HERE?!?!?!?
>
> Imagine someone else wrote this two years ago and you're trying to
> understand this code. In your original patch this was more readable.
>
> And I'm with Hendrik, you just replaced 3.1415 with M_PI everywhere, that's
> great stuff. Keep that up here.
>
>> +    if (a < amin)
>> +        return amin;
>> +    if (a > amax)
>> +        return amax;
>
>
> This doesn't round correctly:
>
> av_rint64_clip_c(9223372036755030016.000000, 9223372036755030008,
> 9223372036755030008) returns 9223372036755030016
>
> whereas obviously it should return 9223372036755030008. The reason is
> probably because these checks are done as doubles, but should be done as
> ints.
>
> Then there's also this funny thing:
>
> 0.500000 clip(0,1) -> 0
>
> Which may just be my llrint() misbehaving but I thought it was funny anyway.

It is funny, but is one of the few good things about llrint (or really
cvtsd2si https://github.com/bminor/glibc/blob/master/sysdeps/x86_64/fpu/s_llrint.S).
In elementary school, at least for me, rounding of .5000... was always
upward. Then, in high school, we had a more complex rule: in .d5000...
or similar such form, if d is even, round down, else round up.
Apparently this is for reasons of statistical bias: one does not want
a skew if e.g a large graction

Seems like the Intel engineers implemented this :).

v4 posted. It takes care of all of your test cases at least, tested
with clang's undefined sanitizer.

>
> Ronald