| Message ID | 20200408175836.8231-1-jamrial@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [FFmpeg-devel] avcodec/nvenc: adapt to the new internal encode API | expand |
| Context | Check | Description |
|---|---|---|
| andriy/ffmpeg-patchwork | success | Make fate finished |
On Wed, 8 Apr 2020 14:58:36 -0300 James Almer <jamrial@gmail.com> wrote: > Signed-off-by: James Almer <jamrial@gmail.com> > --- > This removes the encode2() implementation as it'll never be used if a > receive_packet() one exists, and the flush() implementation since > according to Anton Khirnov avcodec_flush_buffers() is not meant to be > used with encoders, where its behavior is undefined. Nevertheless, there is a use case for flushing an encoder (see 3ea705), and when you call avcodec_flush_buffers() in nvenc, it does the right thing. Removing this will return us to the world before that change where there was no way to flush the encoder, and the original bug reporter was asking about adding an API to do it. It seems the right thing to do is to define the behaviour - which seems reasonable as-is today and documment that encoders can implement flush if necessary. Or we'll just end up adding a new API that flushes encoders but has a different name... --phil
On Wed, 8 Apr 2020, Philip Langdale wrote: > On Wed, 8 Apr 2020 14:58:36 -0300 > James Almer <jamrial@gmail.com> wrote: > >> Signed-off-by: James Almer <jamrial@gmail.com> >> --- >> This removes the encode2() implementation as it'll never be used if a >> receive_packet() one exists, and the flush() implementation since >> according to Anton Khirnov avcodec_flush_buffers() is not meant to be >> used with encoders, where its behavior is undefined. > > Nevertheless, there is a use case for flushing an encoder (see 3ea705), > and when you call avcodec_flush_buffers() in nvenc, it does the right > thing. > > Removing this will return us to the world before that change where > there was no way to flush the encoder, and the original bug reporter > was asking about adding an API to do it. > > It seems the right thing to do is to define the behaviour - which seems > reasonable as-is today and documment that encoders can implement flush > if necessary. Or we'll just end up adding a new API that flushes > encoders but has a different name... I guess the problem is that avcodec_flush_buffers() does not return a result so it can't signal if it is implemented by the encoder, therefore the application will not know if avcodec_flush_buffers() is enough or it has to create a new encode context. Maybe a capability flag can be added to signal support, but a different function might just be cleaner... Regards, Marton
Marton Balint <cus@passwd.hu>, 9 Nis 2020 Per, 01:23 tarihinde şunu yazdı: > > > On Wed, 8 Apr 2020, Philip Langdale wrote: > > > On Wed, 8 Apr 2020 14:58:36 -0300 > > James Almer <jamrial@gmail.com> wrote: > > > >> Signed-off-by: James Almer <jamrial@gmail.com> > >> --- > >> This removes the encode2() implementation as it'll never be used if a > >> receive_packet() one exists, and the flush() implementation since > >> according to Anton Khirnov avcodec_flush_buffers() is not meant to be > >> used with encoders, where its behavior is undefined. > > > > Nevertheless, there is a use case for flushing an encoder (see 3ea705), > > and when you call avcodec_flush_buffers() in nvenc, it does the right > > thing. > > > > Removing this will return us to the world before that change where > > there was no way to flush the encoder, and the original bug reporter > > was asking about adding an API to do it. > > > > It seems the right thing to do is to define the behaviour - which seems > > reasonable as-is today and documment that encoders can implement flush > > if necessary. Or we'll just end up adding a new API that flushes > > encoders but has a different name... > > I guess the problem is that avcodec_flush_buffers() does not return a > result so it can't signal if it is implemented by the encoder, therefore > the application will not know if avcodec_flush_buffers() is enough or it > has to create a new encode context. Maybe a capability flag can be added > to signal support, but a different function might just be cleaner... > > 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-request@ffmpeg.org with subject "unsubscribe". Does this patch offer any solution to ticket https://trac.ffmpeg.org/ticket/8225 ?
On 4/8/2020 7:04 PM, Philip Langdale wrote: > On Wed, 8 Apr 2020 14:58:36 -0300 > James Almer <jamrial@gmail.com> wrote: > >> Signed-off-by: James Almer <jamrial@gmail.com> >> --- >> This removes the encode2() implementation as it'll never be used if a >> receive_packet() one exists, and the flush() implementation since >> according to Anton Khirnov avcodec_flush_buffers() is not meant to be >> used with encoders, where its behavior is undefined. > > Nevertheless, there is a use case for flushing an encoder (see 3ea705), > and when you call avcodec_flush_buffers() in nvenc, it does the right > thing. > > Removing this will return us to the world before that change where > there was no way to flush the encoder, and the original bug reporter > was asking about adding an API to do it. > > It seems the right thing to do is to define the behaviour - which seems > reasonable as-is today and documment that encoders can implement flush > if necessary. Or we'll just end up adding a new API that flushes > encoders but has a different name... > > --phil I'm not against it, but as Marton said the function as it is is not enough. The changes Anton asked me to remove would need to be re-added (trivial to do), and the threading related code would probably need some changes, if anything to not call decoding specific functions on an encoder context. I like the codec capabilities suggestion from Marton. Could you look into it? I'll change this patch to not remove the flush call while at it.
On Thu, 9 Apr 2020 11:20:09 -0300 James Almer <jamrial@gmail.com> wrote: > On 4/8/2020 7:04 PM, Philip Langdale wrote: > > On Wed, 8 Apr 2020 14:58:36 -0300 > > James Almer <jamrial@gmail.com> wrote: > > > >> Signed-off-by: James Almer <jamrial@gmail.com> > >> --- > >> This removes the encode2() implementation as it'll never be used > >> if a receive_packet() one exists, and the flush() implementation > >> since according to Anton Khirnov avcodec_flush_buffers() is not > >> meant to be used with encoders, where its behavior is undefined. > > > > Nevertheless, there is a use case for flushing an encoder (see > > 3ea705), and when you call avcodec_flush_buffers() in nvenc, it > > does the right thing. > > > > Removing this will return us to the world before that change where > > there was no way to flush the encoder, and the original bug reporter > > was asking about adding an API to do it. > > > > It seems the right thing to do is to define the behaviour - which > > seems reasonable as-is today and documment that encoders can > > implement flush if necessary. Or we'll just end up adding a new API > > that flushes encoders but has a different name... > > > > --phil > > I'm not against it, but as Marton said the function as it is is not > enough. The changes Anton asked me to remove would need to be re-added > (trivial to do), and the threading related code would probably need > some changes, if anything to not call decoding specific functions on > an encoder context. > > I like the codec capabilities suggestion from Marton. Could you look > into it? I'll change this patch to not remove the flush call while at > it. Yes, please leave the flush as-is for now so we don't regress. At the very least I think adding an encoder vs decoder path into the function makes sense and we can skip all the decoder specific parts. Today, the emergent behaviour is that an encoder is only affected by the flush call if it provides a flush callback - none of the other parts will alter its state, so perhaps the simple thing to do is have the encoder path just call the callback if it's there and naturally do nothing if not. That removes the need for a separate capability flag. --phil
On 4/9/2020 1:45 PM, Philip Langdale wrote: > On Thu, 9 Apr 2020 11:20:09 -0300 > James Almer <jamrial@gmail.com> wrote: > >> On 4/8/2020 7:04 PM, Philip Langdale wrote: >>> On Wed, 8 Apr 2020 14:58:36 -0300 >>> James Almer <jamrial@gmail.com> wrote: >>> >>>> Signed-off-by: James Almer <jamrial@gmail.com> >>>> --- >>>> This removes the encode2() implementation as it'll never be used >>>> if a receive_packet() one exists, and the flush() implementation >>>> since according to Anton Khirnov avcodec_flush_buffers() is not >>>> meant to be used with encoders, where its behavior is undefined. >>> >>> Nevertheless, there is a use case for flushing an encoder (see >>> 3ea705), and when you call avcodec_flush_buffers() in nvenc, it >>> does the right thing. >>> >>> Removing this will return us to the world before that change where >>> there was no way to flush the encoder, and the original bug reporter >>> was asking about adding an API to do it. >>> >>> It seems the right thing to do is to define the behaviour - which >>> seems reasonable as-is today and documment that encoders can >>> implement flush if necessary. Or we'll just end up adding a new API >>> that flushes encoders but has a different name... >>> >>> --phil >> >> I'm not against it, but as Marton said the function as it is is not >> enough. The changes Anton asked me to remove would need to be re-added >> (trivial to do), and the threading related code would probably need >> some changes, if anything to not call decoding specific functions on >> an encoder context. >> >> I like the codec capabilities suggestion from Marton. Could you look >> into it? I'll change this patch to not remove the flush call while at >> it. > > Yes, please leave the flush as-is for now so we don't regress. At the > very least I think adding an encoder vs decoder path into the function > makes sense and we can skip all the decoder specific parts. Today, the > emergent behaviour is that an encoder is only affected by the flush > call if it provides a flush callback - none of the other parts will > alter its state No, right now buffer_pkt is unrefferenced, and buffer_pkt_valid and draining are reset to 0 by this function. All of them fields used by the encode API, before and after this patchset. If an encoder doesn't have a flush() callback, then the encoder state will be altered and things could behave in an unpredictable way. , so perhaps the simple thing to do is have the encoder > path just call the callback if it's there and naturally do nothing if > not. That removes the need for a separate capability flag. Yes, but how will the user know what encoder lets them flush without the need to recreate the context, and which doesn't? avcodec_flush_buffers() could do something or it could not, and the user will never know because it doesn't return any kind of error code or result. avcodec_flush_buffers() can easily be turned into a no-op for encoders that don't implement a flush callback, but a codec cap or a similar solution would let user know when he has no other option than to recreate the context.
diff --git a/libavcodec/nvenc.c b/libavcodec/nvenc.c index 9a96bf2bba..6c0baf4d86 100644 --- a/libavcodec/nvenc.c +++ b/libavcodec/nvenc.c @@ -30,6 +30,7 @@ #include "libavutil/avassert.h" #include "libavutil/mem.h" #include "libavutil/pixdesc.h" +#include "encode.h" #include "internal.h" #define CHECK_CU(x) FF_CUDA_CHECK_DL(avctx, dl_fn->cuda_dl, x) @@ -1491,6 +1492,8 @@ av_cold int ff_nvenc_encode_close(AVCodecContext *avctx) av_freep(&ctx->surfaces); ctx->nb_surfaces = 0; + av_frame_free(&ctx->frame); + if (ctx->nvencoder) { p_nvenc->nvEncDestroyEncoder(ctx->nvencoder); @@ -1544,6 +1547,10 @@ av_cold int ff_nvenc_encode_init(AVCodecContext *avctx) ctx->data_pix_fmt = avctx->pix_fmt; } + ctx->frame = av_frame_alloc(); + if (!ctx->frame) + return AVERROR(ENOMEM); + if ((ret = nvenc_load_libraries(avctx)) < 0) return ret; @@ -1881,9 +1888,7 @@ static int process_output_surface(AVCodecContext *avctx, AVPacket *pkt, NvencSur goto error; } - res = pkt->data ? - ff_alloc_packet2(avctx, pkt, lock_params.bitstreamSizeInBytes, lock_params.bitstreamSizeInBytes) : - av_new_packet(pkt, lock_params.bitstreamSizeInBytes); + res = av_new_packet(pkt, lock_params.bitstreamSizeInBytes); if (res < 0) { p_nvenc->nvEncUnlockBitstream(ctx->nvencoder, tmpoutsurf->output_surface); @@ -2075,7 +2080,7 @@ static void reconfig_encoder(AVCodecContext *avctx, const AVFrame *frame) } } -int ff_nvenc_send_frame(AVCodecContext *avctx, const AVFrame *frame) +int ff_nvenc_receive_packet(AVCodecContext *avctx, AVPacket *pkt) { NVENCSTATUS nv_status; NvencSurface *tmp_out_surf, *in_surf; @@ -2087,27 +2092,24 @@ int ff_nvenc_send_frame(AVCodecContext *avctx, const AVFrame *frame) NvencDynLoadFunctions *dl_fn = &ctx->nvenc_dload_funcs; NV_ENCODE_API_FUNCTION_LIST *p_nvenc = &dl_fn->nvenc_funcs; + AVFrame *frame = ctx->frame; + NV_ENC_PIC_PARAMS pic_params = { 0 }; pic_params.version = NV_ENC_PIC_PARAMS_VER; if ((!ctx->cu_context && !ctx->d3d11_device) || !ctx->nvencoder) return AVERROR(EINVAL); - if (ctx->encoder_flushing) { - if (avctx->internal->draining) - return AVERROR_EOF; - - ctx->encoder_flushing = 0; - ctx->first_packet_output = 0; - ctx->initial_pts[0] = AV_NOPTS_VALUE; - ctx->initial_pts[1] = AV_NOPTS_VALUE; - av_fifo_reset(ctx->timestamp_list); + if (!frame->buf[0]) { + res = ff_encode_get_frame(avctx, frame); + if (res < 0 && res != AVERROR_EOF) + return res; } - if (frame) { + if (frame->buf[0]) { in_surf = get_free_frame(ctx); if (!in_surf) - return AVERROR(EAGAIN); + goto output; res = nvenc_push_context(avctx); if (res < 0) @@ -2164,7 +2166,6 @@ int ff_nvenc_send_frame(AVCodecContext *avctx, const AVFrame *frame) nvenc_codec_specific_pic_params(avctx, &pic_params, sei_data); } else { pic_params.encodePicFlags = NV_ENC_PIC_FLAG_EOS; - ctx->encoder_flushing = 1; } res = nvenc_push_context(avctx); @@ -2182,7 +2183,7 @@ int ff_nvenc_send_frame(AVCodecContext *avctx, const AVFrame *frame) nv_status != NV_ENC_ERR_NEED_MORE_INPUT) return nvenc_print_error(avctx, nv_status, "EncodePicture failed!"); - if (frame) { + if (frame->buf[0]) { av_fifo_generic_write(ctx->output_surface_queue, &in_surf, sizeof(in_surf), NULL); timestamp_queue_enqueue(ctx->timestamp_list, frame->pts); @@ -2190,6 +2191,8 @@ int ff_nvenc_send_frame(AVCodecContext *avctx, const AVFrame *frame) ctx->initial_pts[0] = frame->pts; else if (ctx->initial_pts[1] == AV_NOPTS_VALUE) ctx->initial_pts[1] = frame->pts; + + av_frame_unref(frame); } /* all the pending buffers are now ready for output */ @@ -2200,20 +2203,8 @@ int ff_nvenc_send_frame(AVCodecContext *avctx, const AVFrame *frame) } } - return 0; -} - -int ff_nvenc_receive_packet(AVCodecContext *avctx, AVPacket *pkt) -{ - NvencSurface *tmp_out_surf; - int res, res2; - - NvencContext *ctx = avctx->priv_data; - - if ((!ctx->cu_context && !ctx->d3d11_device) || !ctx->nvencoder) - return AVERROR(EINVAL); - - if (output_ready(avctx, ctx->encoder_flushing)) { +output: + if (output_ready(avctx, avctx->internal->draining)) { av_fifo_generic_read(ctx->output_surface_ready_queue, &tmp_out_surf, sizeof(tmp_out_surf), NULL); res = nvenc_push_context(avctx); @@ -2230,7 +2221,7 @@ int ff_nvenc_receive_packet(AVCodecContext *avctx, AVPacket *pkt) return res; av_fifo_generic_write(ctx->unused_surface_queue, &tmp_out_surf, sizeof(tmp_out_surf), NULL); - } else if (ctx->encoder_flushing) { + } else if (avctx->internal->draining) { return AVERROR_EOF; } else { return AVERROR(EAGAIN); @@ -2238,32 +2229,3 @@ int ff_nvenc_receive_packet(AVCodecContext *avctx, AVPacket *pkt) return 0; } - -int ff_nvenc_encode_frame(AVCodecContext *avctx, AVPacket *pkt, - const AVFrame *frame, int *got_packet) -{ - NvencContext *ctx = avctx->priv_data; - int res; - - if (!ctx->encoder_flushing) { - res = ff_nvenc_send_frame(avctx, frame); - if (res < 0) - return res; - } - - res = ff_nvenc_receive_packet(avctx, pkt); - if (res == AVERROR(EAGAIN) || res == AVERROR_EOF) { - *got_packet = 0; - } else if (res < 0) { - return res; - } else { - *got_packet = 1; - } - - return 0; -} - -av_cold void ff_nvenc_encode_flush(AVCodecContext *avctx) -{ - ff_nvenc_send_frame(avctx, NULL); -} diff --git a/libavcodec/nvenc.h b/libavcodec/nvenc.h index c44c81e675..6e0d418365 100644 --- a/libavcodec/nvenc.h +++ b/libavcodec/nvenc.h @@ -137,6 +137,8 @@ typedef struct NvencContext CUstream cu_stream; ID3D11Device *d3d11_device; + AVFrame *frame; + int nb_surfaces; NvencSurface *surfaces; @@ -145,8 +147,6 @@ typedef struct NvencContext AVFifoBuffer *output_surface_ready_queue; AVFifoBuffer *timestamp_list; - int encoder_flushing; - struct { void *ptr; int ptr_index; diff --git a/libavcodec/nvenc_h264.c b/libavcodec/nvenc_h264.c index 479155fe15..43ec48f89d 100644 --- a/libavcodec/nvenc_h264.c +++ b/libavcodec/nvenc_h264.c @@ -178,9 +178,7 @@ AVCodec ff_nvenc_encoder = { .type = AVMEDIA_TYPE_VIDEO, .id = AV_CODEC_ID_H264, .init = nvenc_old_init, - .send_frame = ff_nvenc_send_frame, .receive_packet = ff_nvenc_receive_packet, - .encode2 = ff_nvenc_encode_frame, .close = ff_nvenc_encode_close, .priv_data_size = sizeof(NvencContext), .priv_class = &nvenc_class, @@ -207,9 +205,7 @@ AVCodec ff_nvenc_h264_encoder = { .type = AVMEDIA_TYPE_VIDEO, .id = AV_CODEC_ID_H264, .init = nvenc_old_init, - .send_frame = ff_nvenc_send_frame, .receive_packet = ff_nvenc_receive_packet, - .encode2 = ff_nvenc_encode_frame, .close = ff_nvenc_encode_close, .priv_data_size = sizeof(NvencContext), .priv_class = &nvenc_h264_class, @@ -236,11 +232,8 @@ AVCodec ff_h264_nvenc_encoder = { .type = AVMEDIA_TYPE_VIDEO, .id = AV_CODEC_ID_H264, .init = ff_nvenc_encode_init, - .send_frame = ff_nvenc_send_frame, .receive_packet = ff_nvenc_receive_packet, - .encode2 = ff_nvenc_encode_frame, .close = ff_nvenc_encode_close, - .flush = ff_nvenc_encode_flush, .priv_data_size = sizeof(NvencContext), .priv_class = &h264_nvenc_class, .defaults = defaults, diff --git a/libavcodec/nvenc_hevc.c b/libavcodec/nvenc_hevc.c index 7c9b3848f1..a575d64092 100644 --- a/libavcodec/nvenc_hevc.c +++ b/libavcodec/nvenc_hevc.c @@ -166,9 +166,7 @@ AVCodec ff_nvenc_hevc_encoder = { .type = AVMEDIA_TYPE_VIDEO, .id = AV_CODEC_ID_HEVC, .init = nvenc_old_init, - .send_frame = ff_nvenc_send_frame, .receive_packet = ff_nvenc_receive_packet, - .encode2 = ff_nvenc_encode_frame, .close = ff_nvenc_encode_close, .priv_data_size = sizeof(NvencContext), .priv_class = &nvenc_hevc_class, @@ -194,11 +192,8 @@ AVCodec ff_hevc_nvenc_encoder = { .type = AVMEDIA_TYPE_VIDEO, .id = AV_CODEC_ID_HEVC, .init = ff_nvenc_encode_init, - .send_frame = ff_nvenc_send_frame, .receive_packet = ff_nvenc_receive_packet, - .encode2 = ff_nvenc_encode_frame, .close = ff_nvenc_encode_close, - .flush = ff_nvenc_encode_flush, .priv_data_size = sizeof(NvencContext), .priv_class = &hevc_nvenc_class, .defaults = defaults,
Signed-off-by: James Almer <jamrial@gmail.com> --- This removes the encode2() implementation as it'll never be used if a receive_packet() one exists, and the flush() implementation since according to Anton Khirnov avcodec_flush_buffers() is not meant to be used with encoders, where its behavior is undefined. libavcodec/nvenc.c | 84 +++++++++++------------------------------ libavcodec/nvenc.h | 4 +- libavcodec/nvenc_h264.c | 7 ---- libavcodec/nvenc_hevc.c | 5 --- 4 files changed, 25 insertions(+), 75 deletions(-)