[FFmpeg-devel] configure: check if NAN can be used as a constant initializer

Submitted by James Almer on Sept. 13, 2017, 5:10 p.m.

Details

Message ID 20170913171039.9476-1-jamrial@gmail.com
State New
Headers show

Commit Message

James Almer Sept. 13, 2017, 5:10 p.m.
Some targets, like NetBSD and DJGPP, don't seem to support it.

Signed-off-by: James Almer <jamrial@gmail.com>
---
 configure | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

Carl Eugen Hoyos Sept. 13, 2017, 5:23 p.m.
2017-09-13 19:10 GMT+02:00 James Almer <jamrial@gmail.com>:
> Some targets, like NetBSD and DJGPP, don't seem to support it.
>
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
>  configure | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/configure b/configure
> index ab1550d39b..4855ca54c9 100755
> --- a/configure
> +++ b/configure
> @@ -3222,6 +3222,7 @@ pixfmts_super2xsai_test_deps="super2xsai_filter"
>  tinterlace_filter_deps="gpl"
>  tinterlace_merge_test_deps="tinterlace_filter"
>  tinterlace_pad_test_deps="tinterlace_filter"
> +tonemap_filter_deps="const_nan"

No objections but what was wrong with using another
default to avoid the additional check?

Carl Eugen
James Almer Sept. 13, 2017, 5:49 p.m.
On 9/13/2017 2:23 PM, Carl Eugen Hoyos wrote:
> 2017-09-13 19:10 GMT+02:00 James Almer <jamrial@gmail.com>:
>> Some targets, like NetBSD and DJGPP, don't seem to support it.
>>
>> Signed-off-by: James Almer <jamrial@gmail.com>
>> ---
>>  configure | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/configure b/configure
>> index ab1550d39b..4855ca54c9 100755
>> --- a/configure
>> +++ b/configure
>> @@ -3222,6 +3222,7 @@ pixfmts_super2xsai_test_deps="super2xsai_filter"
>>  tinterlace_filter_deps="gpl"
>>  tinterlace_merge_test_deps="tinterlace_filter"
>>  tinterlace_pad_test_deps="tinterlace_filter"
>> +tonemap_filter_deps="const_nan"
> 
> No objections but what was wrong with using another
> default to avoid the additional check?

DBL_MIN and DBL_MAX are both valid values for the option. Turning one of
them into "auto" does not seem like a good idea to me.

If someone else has a better solution than a simple check for broken
toolchains at configure time then I'll drop this patch.
Reimar Döffinger Sept. 13, 2017, 6:08 p.m.
On Wed, Sep 13, 2017 at 02:49:59PM -0300, James Almer wrote:
> On 9/13/2017 2:23 PM, Carl Eugen Hoyos wrote:
> > 2017-09-13 19:10 GMT+02:00 James Almer <jamrial@gmail.com>:
> >> Some targets, like NetBSD and DJGPP, don't seem to support it.
> >>
> >> Signed-off-by: James Almer <jamrial@gmail.com>
> >> ---
> >>  configure | 6 ++++++
> >>  1 file changed, 6 insertions(+)
> >>
> >> diff --git a/configure b/configure
> >> index ab1550d39b..4855ca54c9 100755
> >> --- a/configure
> >> +++ b/configure
> >> @@ -3222,6 +3222,7 @@ pixfmts_super2xsai_test_deps="super2xsai_filter"
> >>  tinterlace_filter_deps="gpl"
> >>  tinterlace_merge_test_deps="tinterlace_filter"
> >>  tinterlace_pad_test_deps="tinterlace_filter"
> >> +tonemap_filter_deps="const_nan"
> > 
> > No objections but what was wrong with using another
> > default to avoid the additional check?
> 
> DBL_MIN and DBL_MAX are both valid values for the option. Turning one of
> them into "auto" does not seem like a good idea to me.
> 
> If someone else has a better solution than a simple check for broken
> toolchains at configure time then I'll drop this patch.

Not having a better solution, but at least vf_zscale looks to me like it should
have the same issue.
It would look at but less "silly" if it was a check used for more
than just one filter.
Hendrik Leppkes Sept. 13, 2017, 6:36 p.m.
On Wed, Sep 13, 2017 at 7:49 PM, James Almer <jamrial@gmail.com> wrote:
> On 9/13/2017 2:23 PM, Carl Eugen Hoyos wrote:
>> 2017-09-13 19:10 GMT+02:00 James Almer <jamrial@gmail.com>:
>>> Some targets, like NetBSD and DJGPP, don't seem to support it.
>>>
>>> Signed-off-by: James Almer <jamrial@gmail.com>
>>> ---
>>>  configure | 6 ++++++
>>>  1 file changed, 6 insertions(+)
>>>
>>> diff --git a/configure b/configure
>>> index ab1550d39b..4855ca54c9 100755
>>> --- a/configure
>>> +++ b/configure
>>> @@ -3222,6 +3222,7 @@ pixfmts_super2xsai_test_deps="super2xsai_filter"
>>>  tinterlace_filter_deps="gpl"
>>>  tinterlace_merge_test_deps="tinterlace_filter"
>>>  tinterlace_pad_test_deps="tinterlace_filter"
>>> +tonemap_filter_deps="const_nan"
>>
>> No objections but what was wrong with using another
>> default to avoid the additional check?
>
> DBL_MIN and DBL_MAX are both valid values for the option. Turning one of
> them into "auto" does not seem like a good idea to me.
>

valid perhaps, but also reasonably useful? ie. would anyone ever use
them and get a useful result?

- Hendrik
James Almer Sept. 13, 2017, 6:36 p.m.
On 9/13/2017 3:08 PM, Reimar Döffinger wrote:
> On Wed, Sep 13, 2017 at 02:49:59PM -0300, James Almer wrote:
>> On 9/13/2017 2:23 PM, Carl Eugen Hoyos wrote:
>>> 2017-09-13 19:10 GMT+02:00 James Almer <jamrial@gmail.com>:
>>>> Some targets, like NetBSD and DJGPP, don't seem to support it.
>>>>
>>>> Signed-off-by: James Almer <jamrial@gmail.com>
>>>> ---
>>>>  configure | 6 ++++++
>>>>  1 file changed, 6 insertions(+)
>>>>
>>>> diff --git a/configure b/configure
>>>> index ab1550d39b..4855ca54c9 100755
>>>> --- a/configure
>>>> +++ b/configure
>>>> @@ -3222,6 +3222,7 @@ pixfmts_super2xsai_test_deps="super2xsai_filter"
>>>>  tinterlace_filter_deps="gpl"
>>>>  tinterlace_merge_test_deps="tinterlace_filter"
>>>>  tinterlace_pad_test_deps="tinterlace_filter"
>>>> +tonemap_filter_deps="const_nan"
>>>
>>> No objections but what was wrong with using another
>>> default to avoid the additional check?
>>
>> DBL_MIN and DBL_MAX are both valid values for the option. Turning one of
>> them into "auto" does not seem like a good idea to me.
>>
>> If someone else has a better solution than a simple check for broken
>> toolchains at configure time then I'll drop this patch.
> 
> Not having a better solution, but at least vf_zscale looks to me like it should
> have the same issue.
Ah, thanks for spotting that. No affected FATE client seems to link
libzimg so they wouldn't have failed in vf_zscale after this or any
other solution for vf_tonemap was committed.

> It would look at but less "silly" if it was a check used for more
> than just one filter.

Will add it and push. If someone has a better solution then this one can
be replaced.
James Almer Sept. 13, 2017, 7:39 p.m.
On 9/13/2017 3:36 PM, Hendrik Leppkes wrote:
> On Wed, Sep 13, 2017 at 7:49 PM, James Almer <jamrial@gmail.com> wrote:
>> On 9/13/2017 2:23 PM, Carl Eugen Hoyos wrote:
>>> 2017-09-13 19:10 GMT+02:00 James Almer <jamrial@gmail.com>:
>>>> Some targets, like NetBSD and DJGPP, don't seem to support it.
>>>>
>>>> Signed-off-by: James Almer <jamrial@gmail.com>
>>>> ---
>>>>  configure | 6 ++++++
>>>>  1 file changed, 6 insertions(+)
>>>>
>>>> diff --git a/configure b/configure
>>>> index ab1550d39b..4855ca54c9 100755
>>>> --- a/configure
>>>> +++ b/configure
>>>> @@ -3222,6 +3222,7 @@ pixfmts_super2xsai_test_deps="super2xsai_filter"
>>>>  tinterlace_filter_deps="gpl"
>>>>  tinterlace_merge_test_deps="tinterlace_filter"
>>>>  tinterlace_pad_test_deps="tinterlace_filter"
>>>> +tonemap_filter_deps="const_nan"
>>>
>>> No objections but what was wrong with using another
>>> default to avoid the additional check?
>>
>> DBL_MIN and DBL_MAX are both valid values for the option. Turning one of
>> them into "auto" does not seem like a good idea to me.
>>
> 
> valid perhaps, but also reasonably useful? ie. would anyone ever use
> them and get a useful result?

Maybe not, but I'm not going to spend time reading and testing the code
to find out if that's the case when the maintainer hasn't dealt with
this himself for an entire month.

As i said, if anyone has a better or preferred solution I'll not oppose
a patch implementing it and reverting mine.

> 
> - Hendrik
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
Derek Buitenhuis Sept. 13, 2017, 8:26 p.m.
On 9/13/2017 8:39 PM, James Almer wrote:
> Maybe not, but I'm not going to spend time reading and testing the code
> to find out if that's the case when the maintainer hasn't dealt with
> this himself for an entire month.

He's on vacation, I think.

> As i said, if anyone has a better or preferred solution I'll not oppose
> a patch implementing it and reverting mine.

I prefer this one, because, for example, it covers zscale, too, which
can't be fixed by using DBL_MAX/DBL_MIN, since NAN is part of libzimg's
public API.

- Derek

Patch hide | download patch | download mbox

diff --git a/configure b/configure
index ab1550d39b..4855ca54c9 100755
--- a/configure
+++ b/configure
@@ -3222,6 +3222,7 @@  pixfmts_super2xsai_test_deps="super2xsai_filter"
 tinterlace_filter_deps="gpl"
 tinterlace_merge_test_deps="tinterlace_filter"
 tinterlace_pad_test_deps="tinterlace_filter"
+tonemap_filter_deps="const_nan"
 uspp_filter_deps="gpl avcodec"
 vaguedenoiser_filter_deps="gpl"
 vidstabdetect_filter_deps="libvidstab"
@@ -5324,6 +5325,11 @@  unsigned int endian = 'B' << 24 | 'I' << 16 | 'G' << 8 | 'E';
 EOF
 od -t x1 $TMPO | grep -q '42 *49 *47 *45' && enable bigendian
 
+check_cc <<EOF && enable const_nan
+#include <math.h>
+void foo(void) { struct { double d; } static const bar[] = { { NAN } }; }
+EOF
+
 if ! enabled ppc64 || enabled bigendian; then
     disable vsx
 fi