[FFmpeg-devel] [PATCH] avutil: Move av_rint64_clip_* to internal.h
Ganesh Ajjanagadde
gajjanag at mit.edu
Sun Nov 15 17:35:04 CET 2015
On Sun, Nov 15, 2015 at 11:19 AM, wm4 <nfxjfg at gmail.com> wrote:
> On Sun, 15 Nov 2015 11:10:23 -0500
> Ganesh Ajjanagadde <gajjanag at mit.edu> wrote:
>
>> On Sun, Nov 15, 2015 at 11:00 AM, Michael Niedermayer
>> <michael at niedermayer.cc> wrote:
>> > On Sun, Nov 15, 2015 at 10:24:52AM -0500, Ganesh Ajjanagadde wrote:
>> >> On Sun, Nov 15, 2015 at 10:23 AM, wm4 <nfxjfg at gmail.com> wrote:
>> >> > On Sun, 15 Nov 2015 10:20:41 -0500
>> >> > Ganesh Ajjanagadde <gajjanag at mit.edu> wrote:
>> >> >
>> >> >> On Sat, Nov 14, 2015 at 9:01 PM, James Almer <jamrial at gmail.com> wrote:
>> >> >> > On 11/14/2015 10:48 PM, Michael Niedermayer wrote:
>> >> >> >> From: Michael Niedermayer <michael at niedermayer.cc>
>> >> >> >>
>> >> >> >> This should avoid build failures on VS2012
>> >> >> >> Feel free to changes this to a different solution
>> >> >> >>
>> >> >> >> Signed-off-by: Michael Niedermayer <michael at niedermayer.cc>
>> >> >> >> ---
>> >> >> >> libavutil/common.h | 39 ---------------------------------------
>> >> >> >> libavutil/internal.h | 40 ++++++++++++++++++++++++++++++++++++++++
>> >> >> >> 2 files changed, 40 insertions(+), 39 deletions(-)
>> >> >> >>
>> >> >> >> diff --git a/libavutil/common.h b/libavutil/common.h
>> >> >> >> index 813fb37..6f0f582 100644
>> >> >> >> --- a/libavutil/common.h
>> >> >> >> +++ b/libavutil/common.h
>> >> >> >> @@ -298,42 +298,6 @@ 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)
>> >> >> >> -{
>> >> >> >> - int64_t res;
>> >> >> >> -#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
>> >> >> >> - // do range checks first
>> >> >> >> - if (a >= 9223372036854775808.0)
>> >> >> >> - return amax;
>> >> >> >> - if (a <= -9223372036854775808.0)
>> >> >> >> - return amin;
>> >> >> >> -
>> >> >> >> - // safe to call llrint and clip accordingly
>> >> >> >> - res = llrint(a);
>> >> >> >> - if (res > amax)
>> >> >> >> - return amax;
>> >> >> >> - if (res < amin)
>> >> >> >> - return amin;
>> >> >> >> - return res;
>> >> >> >> -}
>> >> >> >> -
>> >> >> >> /** Compute ceil(log2(x)).
>> >> >> >> * @param x value used to compute ceil(log2(x))
>> >> >> >> * @return computed ceiling of log2(x)
>> >> >> >> @@ -547,9 +511,6 @@ static av_always_inline av_const int av_popcount64_c(uint64_t x)
>> >> >> >> #ifndef av_clipd
>> >> >> >> # define av_clipd av_clipd_c
>> >> >> >> #endif
>> >> >> >> -#ifndef av_rint64_clip
>> >> >> >> -# define av_rint64_clip av_rint64_clip_c
>> >> >> >> -#endif
>> >> >> >> #ifndef av_popcount
>> >> >> >> # define av_popcount av_popcount_c
>> >> >> >> #endif
>> >> >> >> diff --git a/libavutil/internal.h b/libavutil/internal.h
>> >> >> >> index 5c2cd99..cb0c8cd 100644
>> >> >> >> --- a/libavutil/internal.h
>> >> >> >> +++ b/libavutil/internal.h
>> >> >> >> @@ -257,6 +257,46 @@ void avpriv_request_sample(void *avc,
>> >> >> >> #endif
>> >> >> >>
>> >> >> >> /**
>> >> >> >> + * 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)
>> >> >> >
>> >> >> > IMO rename it to avpriv_rint64_clip() or even ff_rint64_clip() since it's inlined
>> >> >> > and not public/exported.
>> >> >>
>> >> >> Just noticed an issue: Ronald mentioned to me that ffserver and other
>> >> >> such programs should not use internal API. This therefore needs to be
>> >> >> exported somehow.
>> >> >
>> >> > If only ffserver needs it, implement it there?
>> >> >
>> >> > Or even better, just delete ffserver.
>> >>
>> >> I have repeated this many times in the past, but ffserver was given as
>> >> a mere illustration. cmdutils.c also needs it, and there are likely
>> >> more such instances that I have not checked.
>> >
>> > i think both cmdutils and ffserver should check their arguments
>> > and not clip them
>> > this can not be done after *rint64_clip() in all cases because
>> > if the whole int64 range is valid then there is not enough information
>> > left.
>> > If the check is done before on the doubles then normal llrint() should
>> > work for these cases.
>>
>> Such checks will be ugly, since direct llrint on a value out of range
>> is undefined. Essentially the checks of the style of the clip function
>> need to be done.
>
> But aren't these checks simple (as seen in the patch), or am I missing
> something?
They are simple, but out of context, it is unclear where the "magic"
922... comes from. Even worse, 922... does not do the range check by
itself, but only corrects the undefined behavior. Separate range check
will need to be done if one wants to get e.g INT_MIN to INT_MAX.
At the cost of loss of context, one could add a log line at some level
in the rint64_clip warning whenever the value is clipped. I personally
would favor that.
Anyway, I don't know - these things are extremely subjective. At some
level, we are anyway not honoring user's requests - if someone passes
an opt near INT64_MAX, it is treated as a double, and then cast to an
int64_t (see parse_number_or_die). This is very lossy due to the poor
resolution of doubles at large numbers. I thus view clipping out of
range numbers to the endpoints, with possible logging in the clip
function, acceptable.
At its worst, it is a Pareto improvement - no undefined behavior,
actual rounding instead of casting.
>
>> > ff_rint64_clip() might still be usefull elsewhere but i think all
>> > user supplied values should be checked and not clipped
>>
>> It is subjective to me. I personally think that if a user supplies e.g
>> INT64_MAX + 1, one should not simply fail. Clipping to the range of
>> the integer is a best effort solution. Diagnostics can be added, but
>> it is a separate issue.
>>
>> >
>> > [...]
>> > --
>> > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>> >
>> > No great genius has ever existed without some touch of madness. -- Aristotle
>> >
>> > _______________________________________________
>> > ffmpeg-devel mailing list
>> > ffmpeg-devel at ffmpeg.org
>> > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>> >
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel at ffmpeg.org
>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
More information about the ffmpeg-devel
mailing list