[FFmpeg-devel] [PATCHv3] avutil/common: add av_rint64_clip
gajjanagadde at gmail.com
Fri Nov 13 18:02:38 CET 2015
On Fri, Nov 13, 2015 at 9:59 AM, Ronald S. Bultje <rsbultje at gmail.com> wrote:
> On Fri, Nov 13, 2015 at 9:23 AM, Ganesh Ajjanagadde <gajjanagadde at gmail.com>
>> 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
>> it does not fit in the integer's representation does not necessarily
>> correctly (usually converted to a cvttsd2si on x86) which saturates
>> > INT64_MAX to INT64_MIN. The standard marks such conversions as undefined
>> behavior, allowing this sort of mathematically bogus conversions. This
>> 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
>> 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
>> 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
>> + * correctly (usually converted to a cvttsd2si on x86) which saturates
>> + * > INT64_MAX to INT64_MIN. The standard marks such conversions as
>> + * behavior, allowing this sort of mathematically bogus conversions. This
>> + * a safe alternative that is slower obviously but assures safety and
>> + * 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 >=
>> + if (amin > amax) abort();
>> + // 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
> 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
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.
More information about the ffmpeg-devel