diff mbox series

[FFmpeg-devel,v2] avcodec/nvenc: fix flushing for encoder-reuse

Message ID 20210716160942.266207-1-me@jailuthra.in
State New
Headers show
Series [FFmpeg-devel,v2] avcodec/nvenc: fix flushing for encoder-reuse | expand

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate success Make fate finished

Commit Message

Jai Luthra July 16, 2021, 4:09 p.m. UTC 
Recent encode API restructure (827d6fe73d) removed some state - which broke
the API for flushing without closing the encoder.

This functionality was originally added in 3ea7057677 and is useful for
segmented video, where we don't want to do expensive re-init of HW sessions
for every segment.
---
v2: Forgot to rebase off master in v1

 libavcodec/nvenc.c | 16 +++++++++++-----
 libavcodec/nvenc.h |  2 ++
 2 files changed, 13 insertions(+), 5 deletions(-)

Comments

Timo Rothenpieler July 16, 2021, 4:36 p.m. UTC | #1 
On 16.07.2021 18:09, Jai Luthra wrote:
> Recent encode API restructure (827d6fe73d) removed some state - which broke
> the API for flushing without closing the encoder.
> 
> This functionality was originally added in 3ea7057677 and is useful for
> segmented video, where we don't want to do expensive re-init of HW sessions
> for every segment.
> ---
> v2: Forgot to rebase off master in v1
> 
>   libavcodec/nvenc.c | 16 +++++++++++-----
>   libavcodec/nvenc.h |  2 ++
>   2 files changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/libavcodec/nvenc.c b/libavcodec/nvenc.c
> index ee046b9cef..ee0beb4795 100644
> --- a/libavcodec/nvenc.c
> +++ b/libavcodec/nvenc.c
> @@ -2281,6 +2281,11 @@ static int nvenc_send_frame(AVCodecContext *avctx, const AVFrame *frame)
>           return AVERROR(EINVAL);
>   
>       if (frame && frame->buf[0]) {
> +        if (ctx->encoder_flushing) {
> +            ctx->encoder_flushing = 0;
> +            av_fifo_reset(ctx->timestamp_list);
> +        }
> +
>           in_surf = get_free_frame(ctx);
>           if (!in_surf)
>               return AVERROR(EAGAIN);
> @@ -2335,6 +2340,7 @@ static int nvenc_send_frame(AVCodecContext *avctx, const AVFrame *frame)
>           nvenc_codec_specific_pic_params(avctx, &pic_params, ctx->sei_data, sei_count);
>       } else {
>           pic_params.encodePicFlags = NV_ENC_PIC_FLAG_EOS;
> +        ctx->encoder_flushing = 1;
>       }
>   
>       res = nvenc_push_context(avctx);
> @@ -2384,8 +2390,11 @@ int ff_nvenc_receive_packet(AVCodecContext *avctx, AVPacket *pkt)
>   
>       if (!frame->buf[0]) {
>           res = ff_encode_get_frame(avctx, frame);
> -        if (res < 0 && res != AVERROR_EOF)
> +        if (res == AVERROR_EOF || (ctx->encoder_flushing && res == AVERROR(EAGAIN))) {
> +            // flushing mode, continue to send packets
> +        } else if (res < 0) {
>               return res;
> +        }
>       }
>   
>       res = nvenc_send_frame(avctx, frame);
> @@ -2395,7 +2404,7 @@ int ff_nvenc_receive_packet(AVCodecContext *avctx, AVPacket *pkt)
>       } else
>           av_frame_unref(frame);
>   
> -    if (output_ready(avctx, avctx->internal->draining)) {
> +    if (output_ready(avctx, ctx->encoder_flushing)) {
>           av_fifo_generic_read(ctx->output_surface_ready_queue, &tmp_out_surf, sizeof(tmp_out_surf), NULL);
>   
>           res = nvenc_push_context(avctx);
> @@ -2423,8 +2432,5 @@ int ff_nvenc_receive_packet(AVCodecContext *avctx, AVPacket *pkt)
>   
>   av_cold void ff_nvenc_encode_flush(AVCodecContext *avctx)
>   {
> -    NvencContext *ctx = avctx->priv_data;
> -
>       nvenc_send_frame(avctx, NULL);
> -    av_fifo_reset(ctx->timestamp_list);
>   }

Is it really correct for this to do absolutely nothing now?

> diff --git a/libavcodec/nvenc.h b/libavcodec/nvenc.h
> index 85d3a33601..6af3aaf0ce 100644
> --- a/libavcodec/nvenc.h
> +++ b/libavcodec/nvenc.h
> @@ -169,6 +169,8 @@ typedef struct NvencContext
>       NV_ENC_SEI_PAYLOAD *sei_data;
>       int sei_data_size;
>   
> +    int encoder_flushing;
> +
>       struct {
>           void *ptr;
>           int ptr_index;
> 

Can you explain the logic this follows some more? I'm quite confused 
what exactly changed, and how it broke this.
How do you trigger a flush, if calling flush does nothing?
Jai Luthra July 16, 2021, 5:23 p.m. UTC | #2 
On Fri, Jul 16, 2021, at 10:06 PM, Timo Rothenpieler wrote:

-- snip --

> Can you explain the logic this follows some more? I'm quite confused 
> what exactly changed, and how it broke this.
> How do you trigger a flush, if calling flush does nothing?

Since https://github.com/ffmpeg/ffmpeg/commit/3ea7057677 it was possible to flush nvenc buffers without sending a NULL frame like:

// trigger ff_nvenc_encode_flush which will send a NULL frame internally
// this doesn't set avctx->internal->draining
avcodec_flush_buffers(encoder) 

// get remaining packets out of the buffer
while ... {
    avcodec_receive_packet(encoder, &pkt)
}

// whenever next segment comes in, reuse encoder without closing & re-opening
avcodec_send_frame(encoder, frame)

But after the recent API changes [1] if someone tries to fetch packets after calling avcodec_flush_buffers() they won't receive any and hit EAGAIN.

As the user ran out of frames to encode, no frame is present at avctx->internal->buffer_frame - and avctx->internal->draining is unset because the user didn't want to send a NULL frame. So ff_encode_get_frame returns EAGAIN, which is what the user gets without these changes:

> > @@ -2384,8 +2390,11 @@ int ff_nvenc_receive_packet(AVCodecContext *avctx, AVPacket *pkt)
> >   
> >       if (!frame->buf[0]) {
> >           res = ff_encode_get_frame(avctx, frame);
> > -        if (res < 0 && res != AVERROR_EOF)
> > +        if (res == AVERROR_EOF || (ctx->encoder_flushing && res == AVERROR(EAGAIN))) {
> > +            // flushing mode, continue to send packets
> > +        } else if (res < 0) {
> >               return res;
> > +        }
> >       }

----

> >   
> >   av_cold void ff_nvenc_encode_flush(AVCodecContext *avctx)
> >   {
> > -    NvencContext *ctx = avctx->priv_data;
> > -
> >       nvenc_send_frame(avctx, NULL);
> > -    av_fifo_reset(ctx->timestamp_list);
> >   }
> 
> Is it really correct for this to do absolutely nothing now?

It's still sending the NULL frame internally - which will mark encoder_flushing as 1 here:

> > @@ -2335,6 +2340,7 @@ static int nvenc_send_frame(AVCodecContext *avctx, const AVFrame *frame)
> >           nvenc_codec_specific_pic_params(avctx, &pic_params, ctx->sei_data, sei_count);
> >       } else {
> >           pic_params.encodePicFlags = NV_ENC_PIC_FLAG_EOS;
> > +        ctx->encoder_flushing = 1;

We don't wanna cleanup ctx->timestamp_list until the user has retrieved all packets from the buffer after triggering a flush.

[1]: https://github.com/ffmpeg/ffmpeg/commit/827d6fe73d#diff-ba8b555333cbe9c4d768147d8a7abb152281257770aa314cc057a31f43460fec
diff mbox series

Patch

diff --git a/libavcodec/nvenc.c b/libavcodec/nvenc.c
index ee046b9cef..ee0beb4795 100644
--- a/libavcodec/nvenc.c
+++ b/libavcodec/nvenc.c
@@ -2281,6 +2281,11 @@  static int nvenc_send_frame(AVCodecContext *avctx, const AVFrame *frame)
         return AVERROR(EINVAL);
 
     if (frame && frame->buf[0]) {
+        if (ctx->encoder_flushing) {
+            ctx->encoder_flushing = 0;
+            av_fifo_reset(ctx->timestamp_list);
+        }
+
         in_surf = get_free_frame(ctx);
         if (!in_surf)
             return AVERROR(EAGAIN);
@@ -2335,6 +2340,7 @@  static int nvenc_send_frame(AVCodecContext *avctx, const AVFrame *frame)
         nvenc_codec_specific_pic_params(avctx, &pic_params, ctx->sei_data, sei_count);
     } else {
         pic_params.encodePicFlags = NV_ENC_PIC_FLAG_EOS;
+        ctx->encoder_flushing = 1;
     }
 
     res = nvenc_push_context(avctx);
@@ -2384,8 +2390,11 @@  int ff_nvenc_receive_packet(AVCodecContext *avctx, AVPacket *pkt)
 
     if (!frame->buf[0]) {
         res = ff_encode_get_frame(avctx, frame);
-        if (res < 0 && res != AVERROR_EOF)
+        if (res == AVERROR_EOF || (ctx->encoder_flushing && res == AVERROR(EAGAIN))) {
+            // flushing mode, continue to send packets
+        } else if (res < 0) {
             return res;
+        }
     }
 
     res = nvenc_send_frame(avctx, frame);
@@ -2395,7 +2404,7 @@  int ff_nvenc_receive_packet(AVCodecContext *avctx, AVPacket *pkt)
     } else
         av_frame_unref(frame);
 
-    if (output_ready(avctx, avctx->internal->draining)) {
+    if (output_ready(avctx, ctx->encoder_flushing)) {
         av_fifo_generic_read(ctx->output_surface_ready_queue, &tmp_out_surf, sizeof(tmp_out_surf), NULL);
 
         res = nvenc_push_context(avctx);
@@ -2423,8 +2432,5 @@  int ff_nvenc_receive_packet(AVCodecContext *avctx, AVPacket *pkt)
 
 av_cold void ff_nvenc_encode_flush(AVCodecContext *avctx)
 {
-    NvencContext *ctx = avctx->priv_data;
-
     nvenc_send_frame(avctx, NULL);
-    av_fifo_reset(ctx->timestamp_list);
 }
diff --git a/libavcodec/nvenc.h b/libavcodec/nvenc.h
index 85d3a33601..6af3aaf0ce 100644
--- a/libavcodec/nvenc.h
+++ b/libavcodec/nvenc.h
@@ -169,6 +169,8 @@  typedef struct NvencContext
     NV_ENC_SEI_PAYLOAD *sei_data;
     int sei_data_size;
 
+    int encoder_flushing;
+
     struct {
         void *ptr;
         int ptr_index;