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

[Ronald S. Bultje]

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.

Ronald