[FFmpeg-devel] [PATCH] avutil: Move av_rint64_clip_* to internal.h

Michael Niedermayer michael at niedermayer.cc
Sun Nov 15 17:00:55 CET 2015


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.
ff_rint64_clip() might still be usefull elsewhere but i think all
user supplied values should be checked and not clipped

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

No great genius has ever existed without some touch of madness. -- Aristotle
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20151115/6d93f851/attachment.sig>


More information about the ffmpeg-devel mailing list