[FFmpeg-devel] [PATCH] avfilter/xpsnr: avoid division by zero

[FFmpeg-devel] [PATCH] avfilter/xpsnr: avoid division by zero

[Gyan Doshi]

The ref input may have its frame rate unset, which would then lead to
SIGFPE.

Related to #11428
---
 libavfilter/vf_xpsnr.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/libavfilter/vf_xpsnr.c b/libavfilter/vf_xpsnr.c
index 1b2c2a7c2c..8f86c188c5 100644
--- a/libavfilter/vf_xpsnr.c
+++ b/libavfilter/vf_xpsnr.c
@@ -568,7 +568,8 @@ static int config_input_ref(AVFilterLink *inlink)
     s->max_error_64 = (1 << s->depth) - 1; /* conventional limit */
     s->max_error_64 *= s->max_error_64;
 
-    s->frame_rate = il->frame_rate.num / il->frame_rate.den;
+    // Avoid division by zero
+    s->frame_rate = il->frame_rate.den ? (il->frame_rate.num / il->frame_rate.den) : 25;
 
     s->num_comps = (desc->nb_components > 3 ? 3 : desc->nb_components);
 
-- 
2.46.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".

Re: [FFmpeg-devel] [PATCH] avfilter/xpsnr: avoid division by zero

[Marton Balint]

On Mon, 27 Jan 2025, Gyan Doshi wrote:

> The ref input may have its frame rate unset, which would then lead to
> SIGFPE.
>
> Related to #11428
> ---
> libavfilter/vf_xpsnr.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/libavfilter/vf_xpsnr.c b/libavfilter/vf_xpsnr.c
> index 1b2c2a7c2c..8f86c188c5 100644
> --- a/libavfilter/vf_xpsnr.c
> +++ b/libavfilter/vf_xpsnr.c
> @@ -568,7 +568,8 @@ static int config_input_ref(AVFilterLink *inlink)
>     s->max_error_64 = (1 << s->depth) - 1; /* conventional limit */
>     s->max_error_64 *= s->max_error_64;
>
> -    s->frame_rate = il->frame_rate.num / il->frame_rate.den;
> +    // Avoid division by zero
> +    s->frame_rate = il->frame_rate.den ? (il->frame_rate.num / il->frame_rate.den) : 25;

I kind of prefer 0 instead of 25, as far as I see the code does not care, 
and 0 is better than an arbitrary value.

Thanks,
Marton

>
>     s->num_comps = (desc->nb_components > 3 ? 3 : desc->nb_components);
>
> -- 
> 2.46.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".
>
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH] avfilter/xpsnr: avoid division by zero

[Jan Ekström]

On Mon, Jan 27, 2025 at 9:23 PM Marton Balint <cus@passwd.hu> wrote:
>
>
>
> On Mon, 27 Jan 2025, Gyan Doshi wrote:
>
> > The ref input may have its frame rate unset, which would then lead to
> > SIGFPE.
> >
> > Related to #11428
> > ---
> > libavfilter/vf_xpsnr.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/libavfilter/vf_xpsnr.c b/libavfilter/vf_xpsnr.c
> > index 1b2c2a7c2c..8f86c188c5 100644
> > --- a/libavfilter/vf_xpsnr.c
> > +++ b/libavfilter/vf_xpsnr.c
> > @@ -568,7 +568,8 @@ static int config_input_ref(AVFilterLink *inlink)
> >     s->max_error_64 = (1 << s->depth) - 1; /* conventional limit */
> >     s->max_error_64 *= s->max_error_64;
> >
> > -    s->frame_rate = il->frame_rate.num / il->frame_rate.den;
> > +    // Avoid division by zero
> > +    s->frame_rate = il->frame_rate.den ? (il->frame_rate.num / il->frame_rate.den) : 25;
>
> I kind of prefer 0 instead of 25, as far as I see the code does not care,
> and 0 is better than an arbitrary value.

I do agree with the sentiment, but I think at least for myself the
real question is whether we should attempt to get "correct results" or
not, and thus either fail if a frame rate is not properly set, or at
the very least warn loudly that the results may be incorrect
(interpreted as less than 32fps) if the fps value is left unset. Also
I do wonder if we should utilize time base if frame rate is not set,
as I see that 1/0 is utilized for "undefined/variable rate".

Best regards,
Jan
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH] avfilter/xpsnr: avoid division by zero

[Gyan Doshi]

On 2025-01-28 01:44 am, Jan Ekström wrote:
> On Mon, Jan 27, 2025 at 9:23 PM Marton Balint <cus@passwd.hu> wrote:
>>
>>
>> On Mon, 27 Jan 2025, Gyan Doshi wrote:
>>
>>> The ref input may have its frame rate unset, which would then lead to
>>> SIGFPE.
>>>
>>> Related to #11428
>>> ---
>>> libavfilter/vf_xpsnr.c | 3 ++-
>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/libavfilter/vf_xpsnr.c b/libavfilter/vf_xpsnr.c
>>> index 1b2c2a7c2c..8f86c188c5 100644
>>> --- a/libavfilter/vf_xpsnr.c
>>> +++ b/libavfilter/vf_xpsnr.c
>>> @@ -568,7 +568,8 @@ static int config_input_ref(AVFilterLink *inlink)
>>>      s->max_error_64 = (1 << s->depth) - 1; /* conventional limit */
>>>      s->max_error_64 *= s->max_error_64;
>>>
>>> -    s->frame_rate = il->frame_rate.num / il->frame_rate.den;
>>> +    // Avoid division by zero
>>> +    s->frame_rate = il->frame_rate.den ? (il->frame_rate.num / il->frame_rate.den) : 25;
>> I kind of prefer 0 instead of 25, as far as I see the code does not care,
>> and 0 is better than an arbitrary value.
> I do agree with the sentiment, but I think at least for myself the
> real question is whether we should attempt to get "correct results" or
> not, and thus either fail if a frame rate is not properly set, or at
> the very least warn loudly that the results may be incorrect
> (interpreted as less than 32fps) if the fps value is left unset. Also
> I do wonder if we should utilize time base if frame rate is not set,
> as I see that 1/0 is utilized for "undefined/variable rate".

Does PSNR expect an attributes match between the two inputs - 
resolution, framerate... ? If yes, then can we set it to the main input 
framerate. If that is unset, then we assume <32 and log a warning.

Regards,
Gyan

_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH] avfilter/xpsnr: avoid division by zero

[Gyan Doshi]

On 2025-01-28 03:18 pm, Gyan Doshi wrote:
>
>
> On 2025-01-28 01:44 am, Jan Ekström wrote:
>> On Mon, Jan 27, 2025 at 9:23 PM Marton Balint <cus@passwd.hu> wrote:
>>>
>>>
>>> On Mon, 27 Jan 2025, Gyan Doshi wrote:
>>>
>>>> The ref input may have its frame rate unset, which would then lead to
>>>> SIGFPE.
>>>>
>>>> Related to #11428
>>>> ---
>>>> libavfilter/vf_xpsnr.c | 3 ++-
>>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/libavfilter/vf_xpsnr.c b/libavfilter/vf_xpsnr.c
>>>> index 1b2c2a7c2c..8f86c188c5 100644
>>>> --- a/libavfilter/vf_xpsnr.c
>>>> +++ b/libavfilter/vf_xpsnr.c
>>>> @@ -568,7 +568,8 @@ static int config_input_ref(AVFilterLink *inlink)
>>>>      s->max_error_64 = (1 << s->depth) - 1; /* conventional limit */
>>>>      s->max_error_64 *= s->max_error_64;
>>>>
>>>> -    s->frame_rate = il->frame_rate.num / il->frame_rate.den;
>>>> +    // Avoid division by zero
>>>> +    s->frame_rate = il->frame_rate.den ? (il->frame_rate.num / 
>>>> il->frame_rate.den) : 25;
>>> I kind of prefer 0 instead of 25, as far as I see the code does not 
>>> care,
>>> and 0 is better than an arbitrary value.
>> I do agree with the sentiment, but I think at least for myself the
>> real question is whether we should attempt to get "correct results" or
>> not, and thus either fail if a frame rate is not properly set, or at
>> the very least warn loudly that the results may be incorrect
>> (interpreted as less than 32fps) if the fps value is left unset. Also
>> I do wonder if we should utilize time base if frame rate is not set,
>> as I see that 1/0 is utilized for "undefined/variable rate".
>
> Does PSNR expect an attributes match between the two inputs - 
> resolution, framerate... ? If yes, then can we set it to the main 
> input framerate. If that is unset, then we assume <32 and log a warning.

I'll send a patch soon using the other inlink's rate unless someone has 
a better idea.

Regards,
Gyan

_______________________________________________
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".