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

[Michael Niedermayer]

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

thx

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Concerning the gods, I have no means of knowing whether they exist or not
or of what sort they may be, because of the obscurity of the subject, and
the brevity of human life -- Protagoras
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20240201/7d066276/attachment.sig>