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

[Ronald S. Bultje]

Hi,

On Fri, Nov 13, 2015 at 10:42 AM, Ganesh Ajjanagadde <gajjanagadde at gmail.com
> wrote:

> On Fri, Nov 13, 2015 at 10:37 AM, Ronald S. Bultje <rsbultje at gmail.com>
> wrote:
> > Hi,
> >
> > On Fri, Nov 13, 2015 at 10:17 AM, Ganesh Ajjanagadde
> > <gajjanagadde at gmail.com> wrote:
> >>
> >> On Fri, Nov 13, 2015 at 9:59 AM, Ronald S. Bultje <rsbultje at gmail.com>
> >> wrote:
> >> > 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?!?!?!?
> >>
> >> 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.
> >
> >
> > Let me explain my issue with the above. In the sense that you use float
> > literals, I think we should use the value actually being represented. So
> > using 9223372036854775807.0 is questionable, because the actual float
> > (double) value is 9223372036854775808.0, and the correct operation of
> this
> > function requires it to that value. I wonder if there's conditions in
> which
> > the compiler is allowed to round it down depending on build machine
> > settings. If that were the case, terrible things would happen, but I
> don't
> > feel like scouring the C spec for this kind of stuff.
>
> That was a typo on my end, meant an 8 at the end. I really want
> exactly representable values there due to your above reasoning.


Ah, makes sense now. Thanks.

Ronald