[FFmpeg-devel] [PATCHv2] avutil/libm: correct isnan, isinf compat hacks

[Ganesh Ajjanagadde]

On Tue, Nov 24, 2015 at 8:48 PM, Ganesh Ajjanagadde <gajjanag at mit.edu> wrote:
> On Tue, Nov 24, 2015 at 6:53 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
>> On Tue, Nov 24, 2015 at 09:17:18AM +0100, Hendrik Leppkes wrote:
>>> On Sat, Nov 21, 2015 at 2:53 PM, Ganesh Ajjanagadde <gajjanag at mit.edu> wrote:
>>> > On Wed, Nov 18, 2015 at 3:04 PM, Ganesh Ajjanagadde <gajjanag at mit.edu> wrote:
>>> >> On Wed, Nov 18, 2015 at 2:58 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
>>> >>> On Tue, Nov 17, 2015 at 04:54:10PM -0500, Ganesh Ajjanagadde wrote:
>>> >>>> isnan and isinf are actually macros as per the standard. In particular,
>>> >>>> the existing implementation has incorrect signature. Furthermore, this
>>> >>>> results in undefined behavior for e.g double values outside float range
>>> >>>> as per the standard.
>>> >>>>
>>> >>>> This patch corrects the undefined behavior for all usage within FFmpeg.
>>> >>>>
>>> >>>> Note that long double is not handled as it is not used in FFmpeg.
>>> >>>> Furthermore, even if at some point long double gets used, it is likely
>>> >>>> not needed to modify the macro in practice for usage in FFmpeg. See
>>> >>>> below for analysis.
>>> >>>>
>>> >>>> Getting long double to work strictly per the spec is significantly harder
>>> >>>> since a long double may be an IEEE 128 bit quad (very rare), 80 bit
>>> >>>> extended precision value (on GCC/Clang), or simply double (on recent Microsoft).
>>> >>>> On the other hand, any potential future usage of long double is likely
>>> >>>> for precision (when a platform offers extra precision) and not for range, since
>>> >>>> the range anyway varies and is not as portable as IEEE 754 single/double
>>> >>>> precision. In such cases, the implicit cast to a double is well defined
>>> >>>> and isinf and isnan should work as intended.
>>> >>>>
>>> >>>> Reviewed-by: Michael Niedermayer <michael at niedermayer.cc>
>>> >>>> Signed-off-by: Ganesh Ajjanagadde <gajjanagadde at gmail.com>
>>> >>>> ---
>>> >>>>  libavutil/libm.h | 34 ++++++++++++++++++++++++++++++++--
>>> >>>>  1 file changed, 32 insertions(+), 2 deletions(-)
>>> >>>
>>> >>> probably ok
>>> >>> maybe wait a day or 2 before pushing so people can test it on more
>>> >>> obscure platforms
>>> >>>
>>> >>> thx
>>> >>
>>> >> ok, will wait for 2 days for the hypot hack as well. Thanks.
>>> >
>>> > pushed, thanks
>>> >
>>>
>>> nan/inf behavior on VS2012 seems to have broken with this (probably
>>> nan, but as the patch touches both..)
>>> Reverting this particular patch resolves the failures.
>>>
>>> http://fate.ffmpeg.org/report.cgi?time=20151124040137&slot=x86_32-msvc11-windows-native
>>> (ebur128 was broken before)
>>
>> heres a patch to reproduce these failures on linux
>>
>> diff --git a/libavutil/libm.h b/libavutil/libm.h
>> index 9e5ec5d..1669970 100644
>> --- a/libavutil/libm.h
>> +++ b/libavutil/libm.h
>> @@ -82,6 +82,7 @@ static av_always_inline float cbrtf(float x)
>>  #define exp2f(x) ((float)exp2(x))
>>  #endif /* HAVE_EXP2F */
>>
>> +#define HAVE_ISINF 0
>>  #if !HAVE_ISINF
>>  #undef isinf
>>  /* Note: these do not follow the BSD/Apple/GNU convention of returning -1 for
>> @@ -109,6 +110,7 @@ static av_always_inline av_const int avpriv_isinf(double x)
>>          : avpriv_isinf(x))
>>  #endif /* HAVE_ISINF */
>>
>> +#define HAVE_ISNAN 0
>>  #if !HAVE_ISNAN
>>  static av_always_inline av_const int avpriv_isnanf(float x)
>>  {
>
> problem is with the isnan alone, reproduced and under investigation.

Analysis complete and it is due to implementation defined behavior of
the cast of the uint64_t result into int. Worked around with a hack:
basically add a && 1 at the return to guarantee returning of 0 or 1.
Tested with FATE on Michael's lines and pushed.
Apologies all for the breakage.

>
>>
>>
>> [...]
>> --
>> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>>
>> I am the wisest man alive, for I know one thing, and that is that I know
>> nothing. -- Socrates
>>
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel at ffmpeg.org
>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>