[FFmpeg-devel] [PATCH] avcodec/nvenc: fix b-frame DTS behavior with fractional framerates

[FFmpeg-devel] [PATCH] avcodec/nvenc: fix b-frame DTS behavior with fractional framerates

[tt2468]

When using fractional framerates (or any fraction with a numerator != 1),
DTS values for packets would be calculated incorrectly.

Signed-off-by: tt2468 <tt2468@irltoolkit.com>
---
 libavcodec/nvenc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/libavcodec/nvenc.c b/libavcodec/nvenc.c
index 9acf3e8697..8b02856507 100644
--- a/libavcodec/nvenc.c
+++ b/libavcodec/nvenc.c
@@ -2266,7 +2266,8 @@ static int nvenc_set_timestamp(AVCodecContext *avctx,
     dts = reorder_queue_dequeue(ctx->reorder_queue, avctx, pkt);
 
     if (avctx->codec_descriptor->props & AV_CODEC_PROP_REORDER) {
-        pkt->dts = dts - FFMAX(ctx->encode_config.frameIntervalP - 1, 0) * FFMAX(avctx->ticks_per_frame, 1);
+        pkt->dts = dts -
+            FFMAX(ctx->encode_config.frameIntervalP - 1, 0) * FFMAX(avctx->time_base.num * avctx->ticks_per_frame, avctx->time_base.num);
     } else {
         pkt->dts = pkt->pts;
     }
-- 
2.34.1

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

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

Re: [FFmpeg-devel] [PATCH] avcodec/nvenc: fix b-frame DTS behavior with fractional framerates

[Timo Rothenpieler]

On 03.04.2023 00:02, tt2468 wrote:
> When using fractional framerates (or any fraction with a numerator != 1),
> DTS values for packets would be calculated incorrectly.
> 
> Signed-off-by: tt2468 <tt2468@irltoolkit.com>

If you want to sign your commits off, please use a proper name.
A real name is not strictly required, but without it, a Sign-off becomes 
a bit pointless.

> ---
>   libavcodec/nvenc.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/libavcodec/nvenc.c b/libavcodec/nvenc.c
> index 9acf3e8697..8b02856507 100644
> --- a/libavcodec/nvenc.c
> +++ b/libavcodec/nvenc.c
> @@ -2266,7 +2266,8 @@ static int nvenc_set_timestamp(AVCodecContext *avctx,
>       dts = reorder_queue_dequeue(ctx->reorder_queue, avctx, pkt);
>   
>       if (avctx->codec_descriptor->props & AV_CODEC_PROP_REORDER) {
> -        pkt->dts = dts - FFMAX(ctx->encode_config.frameIntervalP - 1, 0) * FFMAX(avctx->ticks_per_frame, 1);
> +        pkt->dts = dts -
> +            FFMAX(ctx->encode_config.frameIntervalP - 1, 0) * FFMAX(avctx->time_base.num * avctx->ticks_per_frame, avctx->time_base.num);

What if the time_base is unset?
It's not really used anywhere else, so it might potentially never be set 
by some users.
The whole right-side term has to at least stay 1.
So probably something clunky like the following is needed:

> FFMAX(avctx->time_base.num * avctx->ticks_per_frame, FFMAX(avctx->time_base.num, 1))


I'm also not sure if this is the right way to calculate this value still.
What if someone sets a super weird timebase, like the mpeg-ts one?
Not like it'd work correctly as it is right now, so I'm more wondering 
on how to make this more robust in general.

Definitely looks like an improvement though, given that timebases like 
1001/30000 exist, where just subtracting 1 or 2 would do barely anything.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

Re: [FFmpeg-devel] [PATCH] avcodec/nvenc: fix b-frame DTS behavior with fractional framerates

[Hendrik Leppkes]

On Mon, Apr 3, 2023 at 12:25 AM Timo Rothenpieler <timo@rothenpieler.org> wrote:
>
> On 03.04.2023 00:02, tt2468 wrote:
> > When using fractional framerates (or any fraction with a numerator != 1),
> > DTS values for packets would be calculated incorrectly.
> >
> > Signed-off-by: tt2468 <tt2468@irltoolkit.com>
>
> If you want to sign your commits off, please use a proper name.
> A real name is not strictly required, but without it, a Sign-off becomes
> a bit pointless.
>
> > ---
> >   libavcodec/nvenc.c | 3 ++-
> >   1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/libavcodec/nvenc.c b/libavcodec/nvenc.c
> > index 9acf3e8697..8b02856507 100644
> > --- a/libavcodec/nvenc.c
> > +++ b/libavcodec/nvenc.c
> > @@ -2266,7 +2266,8 @@ static int nvenc_set_timestamp(AVCodecContext *avctx,
> >       dts = reorder_queue_dequeue(ctx->reorder_queue, avctx, pkt);
> >
> >       if (avctx->codec_descriptor->props & AV_CODEC_PROP_REORDER) {
> > -        pkt->dts = dts - FFMAX(ctx->encode_config.frameIntervalP - 1, 0) * FFMAX(avctx->ticks_per_frame, 1);
> > +        pkt->dts = dts -
> > +            FFMAX(ctx->encode_config.frameIntervalP - 1, 0) * FFMAX(avctx->time_base.num * avctx->ticks_per_frame, avctx->time_base.num);
>
> What if the time_base is unset?
> It's not really used anywhere else, so it might potentially never be set
> by some users.

time_base is a mandatory property on encoders. It is validated to be
set on avcodec_open2. Of course if someone puts in some random values,
thats on them.

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

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

Re: [FFmpeg-devel] [PATCH] avcodec/nvenc: fix b-frame DTS behavior with fractional framerates

[Kyle Manning]

I have submitted a v2 patch to instead use the NVENC context's framerate
numerator instead of time_base. Since this value is used everywhere else,
it should theoretically be safe to assume is valid either way. I've also
removed the sign-off, as it was added by mistake when I was fumbling with
git send-email.

- Kyle Manning


On Sun, Apr 2, 2023 at 3:58 PM Hendrik Leppkes <h.leppkes@gmail.com> wrote:

> On Mon, Apr 3, 2023 at 12:25 AM Timo Rothenpieler <timo@rothenpieler.org>
> wrote:
> >
> > On 03.04.2023 00:02, tt2468 wrote:
> > > When using fractional framerates (or any fraction with a numerator !=
> 1),
> > > DTS values for packets would be calculated incorrectly.
> > >
> > > Signed-off-by: tt2468 <tt2468@irltoolkit.com>
> >
> > If you want to sign your commits off, please use a proper name.
> > A real name is not strictly required, but without it, a Sign-off becomes
> > a bit pointless.
> >
> > > ---
> > >   libavcodec/nvenc.c | 3 ++-
> > >   1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/libavcodec/nvenc.c b/libavcodec/nvenc.c
> > > index 9acf3e8697..8b02856507 100644
> > > --- a/libavcodec/nvenc.c
> > > +++ b/libavcodec/nvenc.c
> > > @@ -2266,7 +2266,8 @@ static int nvenc_set_timestamp(AVCodecContext
> *avctx,
> > >       dts = reorder_queue_dequeue(ctx->reorder_queue, avctx, pkt);
> > >
> > >       if (avctx->codec_descriptor->props & AV_CODEC_PROP_REORDER) {
> > > -        pkt->dts = dts - FFMAX(ctx->encode_config.frameIntervalP - 1,
> 0) * FFMAX(avctx->ticks_per_frame, 1);
> > > +        pkt->dts = dts -
> > > +            FFMAX(ctx->encode_config.frameIntervalP - 1, 0) *
> FFMAX(avctx->time_base.num * avctx->ticks_per_frame, avctx->time_base.num);
> >
> > What if the time_base is unset?
> > It's not really used anywhere else, so it might potentially never be set
> > by some users.
>
> time_base is a mandatory property on encoders. It is validated to be
> set on avcodec_open2. Of course if someone puts in some random values,
> thats on them.
>
> - Hendrik
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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