diff mbox series

[FFmpeg-devel] avutil/timestamp: avoid using INFINITY for log10 result in av_ts_make_time_string2

Message ID 20240827000341.21853-1-cus@passwd.hu
State New
Headers show
Series [FFmpeg-devel] avutil/timestamp: avoid using INFINITY for log10 result in av_ts_make_time_string2 | expand

Checks

Context Check Description
yinshiyou/make_loongarch64 success Make finished
yinshiyou/make_fate_loongarch64 success Make fate finished
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished

Commit Message

Marton Balint Aug. 27, 2024, 12:03 a.m. UTC
Using INFINITY can cause issues with -ffast-math, and since we only use this
value to decide the formatting, we can just as easily use 0 for log10 of zero.

Signed-off-by: Marton Balint <cus@passwd.hu>
---
 libavutil/timestamp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Rémi Denis-Courmont Aug. 27, 2024, 5:57 a.m. UTC | #1
Le 27 août 2024 03:03:37 GMT+03:00, Marton Balint <cus@passwd.hu> a écrit :
>Using INFINITY can cause issues with -ffast-math, and since we only use this
>value to decide the formatting, we can just as easily use 0 for log10 of zero.

FFmpeg does not enable fast-math and last I tried it won't work at all anyway: FATE commits seppuku by OoM.

The statement above is completely false if applied to the whole code base rather than just this function. Notably FFMAX and FFMIN behave differently under -fno-infinite-math (which is implied by -ffast-math).

I suspect that *this* code is just the tip of the iceberg. It's not hard to imagine that ignoring infinities will break stuff, for one: reading raw binary content as float can lead to infinities and other NaNs.

So IMO this patch is wrong. Either we've audited the whole code base, and then we should enable -ffast-math, or at least -fno-infinite-math by default, or we haven't and then we should not be papering over the issue like this.

>
>Signed-off-by: Marton Balint <cus@passwd.hu>
>---
> libavutil/timestamp.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/libavutil/timestamp.c b/libavutil/timestamp.c
>index 6c231a517d..be4540d4c8 100644
>--- a/libavutil/timestamp.c
>+++ b/libavutil/timestamp.c
>@@ -24,7 +24,7 @@ char *av_ts_make_time_string2(char *buf, int64_t ts, AVRational tb)
>         snprintf(buf, AV_TS_MAX_STRING_SIZE, "NOPTS");
>     } else {
>         double val = av_q2d(tb) * ts;
>-        double log = (fpclassify(val) == FP_ZERO ? -INFINITY : floor(log10(fabs(val))));
>+        double log = (fpclassify(val) == FP_ZERO ? 0 : floor(log10(fabs(val))));
>         int precision = (isfinite(log) && log < 0) ? -log + 5 : 6;
>         int last = snprintf(buf, AV_TS_MAX_STRING_SIZE, "%.*f", precision, val);
>         last = FFMIN(last, AV_TS_MAX_STRING_SIZE - 1) - 1;
Marton Balint Aug. 27, 2024, 7 a.m. UTC | #2
On Tue, 27 Aug 2024, Rémi Denis-Courmont wrote:

>
>
> Le 27 août 2024 03:03:37 GMT+03:00, Marton Balint <cus@passwd.hu> a écrit :
>> Using INFINITY can cause issues with -ffast-math, and since we only use this
>> value to decide the formatting, we can just as easily use 0 for log10 of zero.
>
> FFmpeg does not enable fast-math and last I tried it won't work at all anyway: FATE commits seppuku by OoM.
>
> The statement above is completely false if applied to the whole code 
> base rather than just this function. Notably FFMAX and FFMIN behave 
> differently under -fno-infinite-math (which is implied by -ffast-math).

I meant the statement to the code in question, not generically.

>
> I suspect that *this* code is just the tip of the iceberg. It's not hard 
> to imagine that ignoring infinities will break stuff, for one: reading 
> raw binary content as float can lead to infinities and other NaNs.

Agreed.

>
> So IMO this patch is wrong. Either we've audited the whole code base, 
> and then we should enable -ffast-math, or at least -fno-infinite-math by 
> default, or we haven't and then we should not be papering over the issue 
> like this.
>

IMHO the patch itself is harmless (simple, easy to follow), and if it 
helps somebody (no matter how unupported its use-case is), then why the 
hell not. But I can understand your point of view, that we should not 
bother with it, if it is unsupported in the first place, so I will just 
drop this, did not feel strongly about it anyway.

Regards,
Marton


>>
>> Signed-off-by: Marton Balint <cus@passwd.hu>
>> ---
>> libavutil/timestamp.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/libavutil/timestamp.c b/libavutil/timestamp.c
>> index 6c231a517d..be4540d4c8 100644
>> --- a/libavutil/timestamp.c
>> +++ b/libavutil/timestamp.c
>> @@ -24,7 +24,7 @@ char *av_ts_make_time_string2(char *buf, int64_t ts, AVRational tb)
>>         snprintf(buf, AV_TS_MAX_STRING_SIZE, "NOPTS");
>>     } else {
>>         double val = av_q2d(tb) * ts;
>> -        double log = (fpclassify(val) == FP_ZERO ? -INFINITY : floor(log10(fabs(val))));
>> +        double log = (fpclassify(val) == FP_ZERO ? 0 : floor(log10(fabs(val))));
>>         int precision = (isfinite(log) && log < 0) ? -log + 5 : 6;
>>         int last = snprintf(buf, AV_TS_MAX_STRING_SIZE, "%.*f", precision, val);
>>         last = FFMIN(last, AV_TS_MAX_STRING_SIZE - 1) - 1;
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Rémi Denis-Courmont Aug. 27, 2024, 7:50 a.m. UTC | #3
Le 27 août 2024 10:00:08 GMT+03:00, Marton Balint <cus@passwd.hu> a écrit :

>IMHO the patch itself is harmless (simple, easy to follow), and if it helps somebody (no matter how unupported its use-case is), then why the hell not. But I can understand your point of view, that we should not bother with it, if it is unsupported in the first place, so I will just drop this, did not feel strongly about it anyway.

Can't we just fail configure or compilation hard if __FINITE_MATH_ONLY__ is true?

If MPV or whichever downstream wants to get this flag working, it's on them to do the hard work (and it's a lot harder than this patch).
Michael Niedermayer Aug. 27, 2024, 2:04 p.m. UTC | #4
On Tue, Aug 27, 2024 at 02:03:37AM +0200, Marton Balint wrote:
> Using INFINITY can cause issues with -ffast-math, and since we only use this
> value to decide the formatting, we can just as easily use 0 for log10 of zero.
> 
> Signed-off-by: Marton Balint <cus@passwd.hu>
> ---
>  libavutil/timestamp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libavutil/timestamp.c b/libavutil/timestamp.c
> index 6c231a517d..be4540d4c8 100644
> --- a/libavutil/timestamp.c
> +++ b/libavutil/timestamp.c
> @@ -24,7 +24,7 @@ char *av_ts_make_time_string2(char *buf, int64_t ts, AVRational tb)
>          snprintf(buf, AV_TS_MAX_STRING_SIZE, "NOPTS");
>      } else {
>          double val = av_q2d(tb) * ts;
> -        double log = (fpclassify(val) == FP_ZERO ? -INFINITY : floor(log10(fabs(val))));
> +        double log = (fpclassify(val) == FP_ZERO ? 0 : floor(log10(fabs(val))));
>          int precision = (isfinite(log) && log < 0) ? -log + 5 : 6;

please add a comment as log(0) = 0 is not correct (in the general case)
otherwise LGTM

thx

[...]
Kacper Michajlow Aug. 28, 2024, 3:29 a.m. UTC | #5
On Tue, 27 Aug 2024 at 09:51, Rémi Denis-Courmont <remi@remlab.net> wrote:
>
>
>
> Le 27 août 2024 10:00:08 GMT+03:00, Marton Balint <cus@passwd.hu> a écrit :
>
> >IMHO the patch itself is harmless (simple, easy to follow), and if it helps somebody (no matter how unupported its use-case is), then why the hell not. But I can understand your point of view, that we should not bother with it, if it is unsupported in the first place, so I will just drop this, did not feel strongly about it anyway.
>
> Can't we just fail configure or compilation hard if __FINITE_MATH_ONLY__ is true?
>
> If MPV or whichever downstream wants to get this flag working, it's on them to do the hard work (and it's a lot harder than this patch).

Whoa, don't call out mpv like that. There are no plans to do any of
that. -ffast-math should be considered harmful unless proven otherwise
for given codebase. mpv itself depends on infinites.
Rémi Denis-Courmont Aug. 28, 2024, 5:56 a.m. UTC | #6
Le 28 août 2024 06:29:30 GMT+03:00, Kacper Michajlow <kasper93@gmail.com> a écrit :
>On Tue, 27 Aug 2024 at 09:51, Rémi Denis-Courmont <remi@remlab.net> wrote:
>>
>>
>>
>> Le 27 août 2024 10:00:08 GMT+03:00, Marton Balint <cus@passwd.hu> a écrit :
>>
>> >IMHO the patch itself is harmless (simple, easy to follow), and if it helps somebody (no matter how unupported its use-case is), then why the hell not. But I can understand your point of view, that we should not bother with it, if it is unsupported in the first place, so I will just drop this, did not feel strongly about it anyway.
>>
>> Can't we just fail configure or compilation hard if __FINITE_MATH_ONLY__ is true?
>>
>> If MPV or whichever downstream wants to get this flag working, it's on them to do the hard work (and it's a lot harder than this patch).
>
>Whoa, don't call out mpv like that. There are no plans to do any of
>that.

Don't publicly accuse me of something I did not do. I don't remember who blamed this code for breaking mpv but it certainly wasn't me. And why the heck do you make it a problem now rather than then?
Kacper Michajlow Aug. 28, 2024, 6:10 p.m. UTC | #7
On Wed, Aug 28, 2024, 07:56 Rémi Denis-Courmont <remi@remlab.net> wrote:

>
>
> Le 28 août 2024 06:29:30 GMT+03:00, Kacper Michajlow <kasper93@gmail.com>
> a écrit :
> >On Tue, 27 Aug 2024 at 09:51, Rémi Denis-Courmont <remi@remlab.net>
> wrote:
> >>
> >>
> >>
> >> Le 27 août 2024 10:00:08 GMT+03:00, Marton Balint <cus@passwd.hu> a
> écrit :
> >>
> >> >IMHO the patch itself is harmless (simple, easy to follow), and if it
> helps somebody (no matter how unupported its use-case is), then why the
> hell not. But I can understand your point of view, that we should not
> bother with it, if it is unsupported in the first place, so I will just
> drop this, did not feel strongly about it anyway.
> >>
> >> Can't we just fail configure or compilation hard if
> __FINITE_MATH_ONLY__ is true?
> >>
> >> If MPV or whichever downstream wants to get this flag working, it's on
> them to do the hard work (and it's a lot harder than this patch).
> >
> >Whoa, don't call out mpv like that. There are no plans to do any of
> >that.
>
> Don't publicly accuse me of something I did not do. I don't remember who
> blamed this code for breaking mpv but it certainly wasn't me. And why the
> heck do you make it a problem now rather than then?
>

I don't know what you did or didn't do; I'm only responding to your mention
of mpv in this thread. If I'm missing some context outside this thread, I
apologize. I'm simply saying that mpv won't work with the mentioned flags
on its own, and I find it unnecessary to mention it as something that would
drive a change like that.

- Kacper
diff mbox series

Patch

diff --git a/libavutil/timestamp.c b/libavutil/timestamp.c
index 6c231a517d..be4540d4c8 100644
--- a/libavutil/timestamp.c
+++ b/libavutil/timestamp.c
@@ -24,7 +24,7 @@  char *av_ts_make_time_string2(char *buf, int64_t ts, AVRational tb)
         snprintf(buf, AV_TS_MAX_STRING_SIZE, "NOPTS");
     } else {
         double val = av_q2d(tb) * ts;
-        double log = (fpclassify(val) == FP_ZERO ? -INFINITY : floor(log10(fabs(val))));
+        double log = (fpclassify(val) == FP_ZERO ? 0 : floor(log10(fabs(val))));
         int precision = (isfinite(log) && log < 0) ? -log + 5 : 6;
         int last = snprintf(buf, AV_TS_MAX_STRING_SIZE, "%.*f", precision, val);
         last = FFMIN(last, AV_TS_MAX_STRING_SIZE - 1) - 1;