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

[Marton Balint]

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.

Thanks,
Marton