[FFmpeg-devel] [PATCHv3] avutil/common: add av_rint64_clip
gajjanagadde at gmail.com
Fri Nov 13 16:17:27 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?!?!?!?
I already addressed this in a comment to Hendrik above. Please don't
use all caps for this. Also, do bear in mind this is not my area of
interest or expertise: I am doing this because no one else caught the
undefined float to int stuff.
> 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.
Not yet, the patch is still in progress :).
>> + 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
This can be made better with something like llrint(a) < amin,
llrint(a) > amax etc maybe?
> 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.
Will test with all of your above cases.
More information about the ffmpeg-devel