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

Ronald S. Bultje rsbultje at gmail.com
Fri Nov 13 15:10:35 CET 2015


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.

Ronald


More information about the ffmpeg-devel mailing list