[FFmpeg-devel] [PATCH 1/2] avutil/common: add av_rint64_clip

[Ganesh Ajjanagadde]

On Fri, Nov 13, 2015 at 9:10 AM, Ronald S. Bultje <rsbultje at gmail.com> wrote:
> Hi,
>
> On Fri, Nov 13, 2015 at 9:06 AM, Ganesh Ajjanagadde <gajjanagadde at gmail.com>
> wrote:
>>
>> On Fri, Nov 13, 2015 at 8:52 AM, Ronald S. Bultje <rsbultje at gmail.com>
>> wrote:
>> > Hi,
>> >
>> > On Fri, Nov 13, 2015 at 8:08 AM, Ronald S. Bultje <rsbultje at gmail.com>
>> > wrote:
>> >>
>> >> Hi,
>> >>
>> >> On Thu, Nov 12, 2015 at 9:46 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  | 31 +++++++++++++++++++++++++++++++
>> >>>  libavutil/version.h |  4 ++--
>> >>>  2 files changed, 33 insertions(+), 2 deletions(-)
>> >>>
>> >>> diff --git a/libavutil/common.h b/libavutil/common.h
>> >>> index 6f0f582..4f60e72 100644
>> >>> --- a/libavutil/common.h
>> >>> +++ b/libavutil/common.h
>> >>> @@ -298,6 +298,34 @@ 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)
>> >>> +        return INT64_MAX;
>> >>> +    if (a <= INT64_MIN)
>> >>> +        return INT64_MIN;
>> >>> +    // Finally safe to call av_clipd_c
>> >>> +    return (int64_t)av_clipd_c(a, amin, amax);
>> >>
>> >>
>> >> Also, please use llrint for casting (it rounds), and clip in integer
>> >> domain (e.g. what happens if I set amin=amax=INT64_MAX, or any other
>> >> number
>> >> not exactly representable in floats)?
>>
>> llrint does not work. Even casting "rounds", i.e rounds towards zero
>> (albeit implementation defined). All that llrint does is sets the
>> rounding behavior.
>
>
> You're trying to give the closest integer representation to a given double
> and clip it in some range. You need to round correctly for that. (int)0.9
> returns 0. That seems curious.
>
> I'm not saying use llrint only and it'll take care of everything. I'm
> saying, at some point you need to convert from double to int. For that step,
> I'm asking you to not just cast, but use llrint.

Everything taken into account, v3 posted. This should also fix build
failure pointed out by Michael.

>
> Ronald