| Message ID | 20230402220225.342600-1-tt2468@irltoolkit.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] avcodec/nvenc: fix b-frame DTS behavior with fractional framerates | expand |
| Context | Check | Description |
|---|---|---|
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
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.
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
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". >
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; }
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(-)