On Fri, Nov 13, 2015 at 10:37 AM, Ronald S. Bultje <rsbul...@gmail.com> wrote:
> Hi,
>
> On Fri, Nov 13, 2015 at 10:17 AM, Ganesh Ajjanagadde
> <gajjanaga...@gmail.com> wrote:
>>
>> On Fri, Nov 13, 2015 at 9:59 AM, Ronald S. Bultje <rsbul...@gmail.com>
>> wrote:
>> > Hi,
>> >
>> > On Fri, Nov 13, 2015 at 9:23 AM, Ganesh Ajjanagadde
>> > <gajjanaga...@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 <rsbul...@gmail.com>
>> >> Signed-off-by: Ganesh Ajjanagadde <gajjanaga...@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.
>
> 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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
