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

2024-02-03 Thread 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/20,yadif,showinfo -f
 null none

 Signed-off-by: Marton Balint 
 ---
  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(>cc_fifo, yadif->out);
  av_frame_free(>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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

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

2024-02-01 Thread 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/20,yadif,showinfo -f null none

Signed-off-by: Marton Balint 
---
 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(>cc_fifo, yadif->out);
 av_frame_free(>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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

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

2024-01-31 Thread 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/20,yadif,showinfo -f 
> > > null none
> > > 
> > > Signed-off-by: Marton Balint 
> > > ---
> > >  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(>cc_fifo, yadif->out);
> > >  av_frame_free(>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


signature.asc
Description: PGP signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

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

2024-01-30 Thread 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/20,yadif,showinfo -f null none

Signed-off-by: Marton Balint 
---
 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(>cc_fifo, yadif->out);
 av_frame_free(>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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

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

2024-01-30 Thread Michael Niedermayer 
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/20,yadif,showinfo -f null 
> none
> 
> Signed-off-by: Marton Balint 
> ---
>  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(>cc_fifo, yadif->out);
>  av_frame_free(>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

otherwise should be ok

thx


[...]
-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Avoid a single point of failure, be that a person or equipment.


signature.asc
Description: PGP signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

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

2024-01-27 Thread Marton Balint 
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/20,yadif,showinfo -f null none

Signed-off-by: Marton Balint 
---
 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(>cc_fifo, yadif->out);
 av_frame_free(>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;
 
 return return_frame(ctx, 0);
 }
@@ -213,9 +213,17 @@ int ff_yadif_config_output_common(AVFilterLink *outlink)
 {
 AVFilterContext *ctx = outlink->src;
 YADIFContext *yadif = ctx->priv;
+AVRational tb = ctx->inputs[0]->time_base;
 int ret;
 
-outlink->time_base = av_mul_q(ctx->inputs[0]->time_base, (AVRational){1, 
2});
+if (av_reduce(>time_base.num, >time_base.den, tb.num, 
tb.den * 2LL, INT_MAX)) {
+yadif->pts_divisor = 1;
+} else {
+av_log(ctx, AV_LOG_WARNING, "Cannot use exact output timebase\n");
+outlink->time_base = tb;
+yadif->pts_divisor = 2;
+}
+
 outlink->w = ctx->inputs[0]->w;
 outlink->h = ctx->inputs[0]->h;
 
-- 
2.35.3

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".