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

[Ronald S. Bultje]

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.

The integer literals looks like it's disappearing so let's ignore that, yes.

> 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
> > ints.
>
> This can be made better with something like llrint(a) < amin,
> llrint(a) > amax etc maybe?
>

Probably.

(I guess you'd do a single int64_t res = llrint(a); and then use res
instead of a anywhere below, but you probably already were planning to do
that anyway.)

> 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.
>

Ty.

Ronald