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

[Ganesh Ajjanagadde]

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.

>
>
> [...]
> --
> 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
>