On Sun, Nov 15, 2015 at 10:24:52AM -0500, Ganesh Ajjanagadde wrote:
> On Sun, Nov 15, 2015 at 10:23 AM, wm4 <nfx...@gmail.com> wrote:
> > On Sun, 15 Nov 2015 10:20:41 -0500
> > Ganesh Ajjanagadde <gajja...@mit.edu> wrote:
> >
> >> On Sat, Nov 14, 2015 at 9:01 PM, James Almer <jamr...@gmail.com> wrote:
> >> > On 11/14/2015 10:48 PM, Michael Niedermayer wrote:
> >> >> From: Michael Niedermayer <mich...@niedermayer.cc>
> >> >>
> >> >> This should avoid build failures on VS2012
> >> >> Feel free to changes this to a different solution
> >> >>
> >> >> Signed-off-by: Michael Niedermayer <mich...@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

Attachment: signature.asc
Description: Digital signature

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Reply via email to