[FFmpeg-devel] [PATCH 3/3] avfilter/yadif_common: fix timestamps with very small timebases

[Marton Balint]

On Thu, 1 Feb 2024, Michael Niedermayer wrote:

> On Wed, Jan 31, 2024 at 03:42:46AM +0100, Marton Balint wrote:
>>
>>
>> On Wed, 31 Jan 2024, Michael Niedermayer wrote:
>>
>>> On Sun, Jan 28, 2024 at 04:01:36AM +0100, Marton Balint wrote:
>>>> Yadif filter assumed that the output timebase is always half of the input
>>>> timebase. This is not true if halving the input time base is not representable
>>>> as an AVRational causing the output timestamps to be invalidly scaled in such a
>>>> case.
>>>>
>>>> So let's use av_reduce instead of av_mul_q when calculating the output time
>>>> base and if the conversion is inexact then let's fall back to the original
>>>> timebase which probably makes more parctical sense than using x/INT_MAX.
>>>>
>>>> Fixes invalidly scaled pts_time values in this command line:
>>>> ffmpeg -f lavfi -i testsrc -vf settb=tb=1/2000000000,yadif,showinfo -f null none
>>>>
>>>> Signed-off-by: Marton Balint <cus at passwd.hu>
>>>> ---
>>>>  libavfilter/yadif.h        |  2 ++
>>>>  libavfilter/yadif_common.c | 20 ++++++++++++++------
>>>>  2 files changed, 16 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/libavfilter/yadif.h b/libavfilter/yadif.h
>>>> index 2c4fed62d2..c144568242 100644
>>>> --- a/libavfilter/yadif.h
>>>> +++ b/libavfilter/yadif.h
>>>> @@ -86,6 +86,8 @@ typedef struct YADIFContext {
>>>>       * the first field.
>>>>       */
>>>>      int current_field;  ///< YADIFCurrentField
>>>> +
>>>> +    int pts_divisor;
>>>>  } YADIFContext;
>>>>
>>>>  void ff_yadif_init_x86(YADIFContext *yadif);
>>>> diff --git a/libavfilter/yadif_common.c b/libavfilter/yadif_common.c
>>>> index 933372529e..90a5cffc2d 100644
>>>> --- a/libavfilter/yadif_common.c
>>>> +++ b/libavfilter/yadif_common.c
>>>> @@ -61,7 +61,7 @@ FF_ENABLE_DEPRECATION_WARNINGS
>>>>          int64_t next_pts = yadif->next->pts;
>>>>
>>>>          if (next_pts != AV_NOPTS_VALUE && cur_pts != AV_NOPTS_VALUE) {
>>>> -            yadif->out->pts = cur_pts + next_pts;
>>>> +            yadif->out->pts = (cur_pts + next_pts) / yadif->pts_divisor;
>>>>          } else {
>>>>              yadif->out->pts = AV_NOPTS_VALUE;
>>>>          }
>>>> @@ -150,8 +150,8 @@ int ff_yadif_filter_frame(AVFilterLink *link, AVFrame *frame)
>>>>          ff_ccfifo_inject(&yadif->cc_fifo, yadif->out);
>>>>          av_frame_free(&yadif->prev);
>>>>          if (yadif->out->pts != AV_NOPTS_VALUE)
>>>> -            yadif->out->pts *= 2;
>>>> -        yadif->out->duration *= 2;
>>>> +            yadif->out->pts *= 2 / yadif->pts_divisor;
>>>> +        yadif->out->duration *= 2 / yadif->pts_divisor;
>>>>          return ff_filter_frame(ctx->outputs[0], yadif->out);
>>>>      }
>>>>
>>>> @@ -168,9 +168,9 @@ FF_ENABLE_DEPRECATION_WARNINGS
>>>>      yadif->out->flags &= ~AV_FRAME_FLAG_INTERLACED;
>>>>
>>>>      if (yadif->out->pts != AV_NOPTS_VALUE)
>>>> -        yadif->out->pts *= 2;
>>>> +        yadif->out->pts *= 2 / yadif->pts_divisor;
>>>>      if (!(yadif->mode & 1))
>>>> -        yadif->out->duration *= 2;
>>>> +        yadif->out->duration *= 2 / yadif->pts_divisor;
>>>
>>> you can use >> instead of division for all above
>>
>> Even for the first case? Because the right shift would be implementation
>> defined for negative timestamps.
>
> we are only supporting twos-complement systems
> i thought that was somewhete in teh docs

Ok, I will change the /= 2 divisions to >>= 1 shifts in v2 patch then.

Thanks,
Marton