[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 at 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