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

[Marton Balint]

On Thu, 1 Feb 2024, Marton Balint wrote:

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

Will apply the series.

Regards,
Marton