[FFmpeg-devel] [PATCH 1/2] avutil/common: add av_clipd64

[Ganesh Ajjanagadde]

On Mon, Nov 2, 2015 at 1:59 PM, Daniel Serpell <dserpell at gmail.com> wrote:
> Hi,
>
> On Sun, Nov 01, 2015 at 01:03:35PM -0500, Ganesh Ajjanagadde 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.
>> @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.
>>
>> ------------------------------------------------------------------------------
>> As a general remark, Clang/GCC has a -Wfloat-conversion so that at least
>> implicit conversions can be found. This helped me in auditing the
>> codebase. In order to reduce noise while testing, an explicit cast on the return
>> was placed. I can remove it if people prefer, though I like the cast as
>> it makes the intent of possible narrowing explicit.
>>
>> Signed-off-by: Ganesh Ajjanagadde <gajjanagadde at gmail.com>
>> ---
>>  libavutil/common.h | 31 +++++++++++++++++++++++++++++++
>>  1 file changed, 31 insertions(+)
>>
>> diff --git a/libavutil/common.h b/libavutil/common.h
>> index 6f0f582..e778dd2 100644
>> --- a/libavutil/common.h
>> +++ b/libavutil/common.h
>> @@ -298,6 +298,34 @@ static av_always_inline av_const double av_clipd_c(double a, double amin, double
>>      else               return a;
>>  }
>>
>> +/**
>> + * 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.
>> + * @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_clipd64_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 >= 9223372036854775808.0)
>> +        return INT64_MAX;
>> +    if (a <= INT64_MIN)
>> +        return INT64_MIN;
>> +    // Finally safe to call av_clipd_c
>> +    return (int64_t)av_clipd_c(a, amin, amax);
>> +}
>> +
>
> This wont work, because double precision has less than 64bit of mantisa,
> as the number 0x7FFFFFFFFFFFFC00 is the highest double precision number
> that can be converted to a 64bit integer, values near INT64_MAX will be
> rounded up to INT64_MAX and give undefined results.
>
> So, I propose you do (a >= 9223372036854775296.0) instead, (this is
> 0x7FFFFFFFFFFFFE00, half the way to INT64_MAX+1).

I see what you are saying, but think your reasoning is wrong. I have
tested with a ubsan build on clang and gcc.
The reason why the code is fine is as follows:
Values near INT64_MAX will be rounded to INT64_MAX + 1 (closest
representable point), and the first condition will hold true (yes, it
is weird, but that is what happens). Technically, they don't need to
round that way, C does not specify "nearest". All C specifies is that
it must be one of the two neighboring representation points. However,
this does not matter either, since if it rounds down, it will round
down subsequently in av_clipd_c, so it is ok. This is because the
floating point environment is unchanged during the call.

I think your confusion comes from thinking that INT64_MAX is
representable: it is not, hence the comment and choice of INT64_MAX +
1.

I suggest you try out a ubsan build to test it out - I did look at
values near INT64_MAX (the trouble spot), but it is entirely possible
my reasoning above is faulty. If you could reproduce evidence
supporting your claim, it will be very helpful.

>
>     Daniel.
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel