| Message ID | 20200409182722.1426-1-jamrial@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [FFmpeg-devel,v2] avcodec/nvenc: adapt to the new internal encode API | expand |
| Context | Check | Description |
|---|---|---|
| andriy/ffmpeg-patchwork | success | Make fate finished |
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of > James Almer > Sent: Friday, April 10, 2020 02:27 > To: ffmpeg-devel@ffmpeg.org > Subject: [FFmpeg-devel] [PATCH v2] avcodec/nvenc: adapt to the new > internal encode API > > Signed-off-by: James Almer <jamrial@gmail.com> > --- > Version with the flush() callback left in place. But it will need the > changes i originally added to avcodec_flush_buffers() and then removed > for the latest iteration of this set, in some form or another. > > libavcodec/nvenc.c | 78 ++++++++++++++++++----------------------- > libavcodec/nvenc.h | 9 ++--- > libavcodec/nvenc_h264.c | 6 ---- > libavcodec/nvenc_hevc.c | 4 --- > 4 files changed, 36 insertions(+), 61 deletions(-) > > diff --git a/libavcodec/nvenc.c b/libavcodec/nvenc.c > index 9a96bf2bba..700a9a7a97 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); > Is there any specific reason to drop ff_alloc_packet2? This function calls av_new_packet() with a return check while !pkt->data, which seems to have an identical logic with this modification, so how about: res = ff_alloc_packet2(avctx, pkt, lock_params.bitstreamSizeInBytes, lock_params.bitstreamSizeInBytes) Honestly I've got one question on how to choose between: 1. ff_alloc_packet2(avctx, pkt, lock_params.bitstreamSizeInBytes, lock_params.bitstreamSizeInBytes);// seems to equals av_new_packet while !pkt->data 2. ff_alloc_packet2(avctx, pkt, lock_params.bitstreamSizeInBytes, 0);// seems to be the majority usage of encoder, which uses av_fast_padded_malloc(); 3. av_new_packet(pkt, lock_params.bitstreamSizeInBytes);// basic - Linjie
On 4/9/2020 10:04 PM, Fu, Linjie wrote: >> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of >> James Almer >> Sent: Friday, April 10, 2020 02:27 >> To: ffmpeg-devel@ffmpeg.org >> Subject: [FFmpeg-devel] [PATCH v2] avcodec/nvenc: adapt to the new >> internal encode API >> >> Signed-off-by: James Almer <jamrial@gmail.com> >> --- >> Version with the flush() callback left in place. But it will need the >> changes i originally added to avcodec_flush_buffers() and then removed >> for the latest iteration of this set, in some form or another. >> >> libavcodec/nvenc.c | 78 ++++++++++++++++++----------------------- >> libavcodec/nvenc.h | 9 ++--- >> libavcodec/nvenc_h264.c | 6 ---- >> libavcodec/nvenc_hevc.c | 4 --- >> 4 files changed, 36 insertions(+), 61 deletions(-) >> >> diff --git a/libavcodec/nvenc.c b/libavcodec/nvenc.c >> index 9a96bf2bba..700a9a7a97 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); >> > > Is there any specific reason to drop ff_alloc_packet2? > > This function calls av_new_packet() with a return check while !pkt->data, > which seems to have an identical logic with this modification, so how about: > res = ff_alloc_packet2(avctx, pkt, lock_params.bitstreamSizeInBytes, lock_params.bitstreamSizeInBytes) ff_alloc_packet2() is meant for the old encode2() internal API, or more strictly speaking the avcodec_encode_video2() public API, which allows the user to provide a data buffer where the packet will be written. The avcodec_receive_packet() public API doesn't allow for this, so packet allocation should be done using the standard AVPacket API. > > Honestly I've got one question on how to choose between: > 1. ff_alloc_packet2(avctx, pkt, lock_params.bitstreamSizeInBytes, lock_params.bitstreamSizeInBytes);// seems to equals av_new_packet while !pkt->data > 2. ff_alloc_packet2(avctx, pkt, lock_params.bitstreamSizeInBytes, 0);// seems to be the majority usage of encoder, which uses av_fast_padded_malloc(); > 3. av_new_packet(pkt, lock_params.bitstreamSizeInBytes);// basic I'm not sure what's the difference between passing 0 or some other value as min_size to ff_alloc_packet2(). Apparently it makes use of some internal byte_buffer or not, depending on it.
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of > James Almer > Sent: Friday, April 10, 2020 02:27 > To: ffmpeg-devel@ffmpeg.org > Subject: [FFmpeg-devel] [PATCH v2] avcodec/nvenc: adapt to the new > internal encode API > > Signed-off-by: James Almer <jamrial@gmail.com> > --- > Version with the flush() callback left in place. But it will need the > changes i originally added to avcodec_flush_buffers() and then removed > for the latest iteration of this set, in some form or another. > > libavcodec/nvenc.c | 78 ++++++++++++++++++----------------------- > libavcodec/nvenc.h | 9 ++--- > libavcodec/nvenc_h264.c | 6 ---- > libavcodec/nvenc_hevc.c | 4 --- > 4 files changed, 36 insertions(+), 61 deletions(-) > > diff --git a/libavcodec/nvenc.c b/libavcodec/nvenc.c > index 9a96bf2bba..700a9a7a97 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" > > > - if (output_ready(avctx, ctx->encoder_flushing)) { > + if (!frame->buf[0]) { > + res = ff_encode_get_frame(avctx, frame); > + if (res < 0 && res != AVERROR_EOF) > + return res; > + } > + > + res = nvenc_send_frame(avctx, frame); > + if (res < 0) { > + if (res != AVERROR(EAGAIN)) > + return res; > + } else > + av_frame_unref(frame); Would it be better to use av_frame_move_ref inside nvenc_upload_frame() in nvenc_send_frame() instead of add frame reference again? Didn't verify in nvenc, but such modification leads to some performance improvements for vaapi. - Linjie
On 4/9/2020 10:25 PM, Fu, Linjie wrote: >> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of >> James Almer >> Sent: Friday, April 10, 2020 02:27 >> To: ffmpeg-devel@ffmpeg.org >> Subject: [FFmpeg-devel] [PATCH v2] avcodec/nvenc: adapt to the new >> internal encode API >> >> Signed-off-by: James Almer <jamrial@gmail.com> >> --- >> Version with the flush() callback left in place. But it will need the >> changes i originally added to avcodec_flush_buffers() and then removed >> for the latest iteration of this set, in some form or another. >> >> libavcodec/nvenc.c | 78 ++++++++++++++++++----------------------- >> libavcodec/nvenc.h | 9 ++--- >> libavcodec/nvenc_h264.c | 6 ---- >> libavcodec/nvenc_hevc.c | 4 --- >> 4 files changed, 36 insertions(+), 61 deletions(-) >> >> diff --git a/libavcodec/nvenc.c b/libavcodec/nvenc.c >> index 9a96bf2bba..700a9a7a97 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" >> >> >> - if (output_ready(avctx, ctx->encoder_flushing)) { >> + if (!frame->buf[0]) { >> + res = ff_encode_get_frame(avctx, frame); >> + if (res < 0 && res != AVERROR_EOF) >> + return res; >> + } >> + >> + res = nvenc_send_frame(avctx, frame); >> + if (res < 0) { >> + if (res != AVERROR(EAGAIN)) >> + return res; >> + } else >> + av_frame_unref(frame); > > Would it be better to use av_frame_move_ref inside nvenc_upload_frame() > in nvenc_send_frame() instead of add frame reference again? > > Didn't verify in nvenc, but such modification leads to some performance improvements > for vaapi. The frame is used after the call to nvenc_upload_frame(), so not possible without bigger changes. > > - Linjie > _______________________________________________ > 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". >
On 09.04.2020 20:27, James Almer wrote: > Signed-off-by: James Almer <jamrial@gmail.com> > --- > Version with the flush() callback left in place. But it will need the > changes i originally added to avcodec_flush_buffers() and then removed > for the latest iteration of this set, in some form or another. > > libavcodec/nvenc.c | 78 ++++++++++++++++++----------------------- > libavcodec/nvenc.h | 9 ++--- > libavcodec/nvenc_h264.c | 6 ---- > libavcodec/nvenc_hevc.c | 4 --- > 4 files changed, 36 insertions(+), 61 deletions(-) Looks fine to me, feel free to push along encode API changes whenever they are ready.
diff --git a/libavcodec/nvenc.c b/libavcodec/nvenc.c index 9a96bf2bba..700a9a7a97 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) +static int nvenc_send_frame(AVCodecContext *avctx, const AVFrame *frame) { NVENCSTATUS nv_status; NvencSurface *tmp_out_surf, *in_surf; @@ -2093,18 +2098,7 @@ int ff_nvenc_send_frame(AVCodecContext *avctx, const AVFrame *frame) 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) { + if (frame && frame->buf[0]) { in_surf = get_free_frame(ctx); if (!in_surf) return AVERROR(EAGAIN); @@ -2164,7 +2158,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 +2175,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 && 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); @@ -2210,10 +2203,25 @@ int ff_nvenc_receive_packet(AVCodecContext *avctx, AVPacket *pkt) NvencContext *ctx = avctx->priv_data; + AVFrame *frame = ctx->frame; + if ((!ctx->cu_context && !ctx->d3d11_device) || !ctx->nvencoder) return AVERROR(EINVAL); - if (output_ready(avctx, ctx->encoder_flushing)) { + if (!frame->buf[0]) { + res = ff_encode_get_frame(avctx, frame); + if (res < 0 && res != AVERROR_EOF) + return res; + } + + res = nvenc_send_frame(avctx, frame); + if (res < 0) { + if (res != AVERROR(EAGAIN)) + return res; + } else + av_frame_unref(frame); + + 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 +2238,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); @@ -2239,31 +2247,13 @@ 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) +av_cold void ff_nvenc_encode_flush(AVCodecContext *avctx) { 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); + nvenc_send_frame(avctx, NULL); + 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); } diff --git a/libavcodec/nvenc.h b/libavcodec/nvenc.h index c44c81e675..48dcfae05c 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; @@ -207,13 +207,8 @@ int ff_nvenc_encode_init(AVCodecContext *avctx); int ff_nvenc_encode_close(AVCodecContext *avctx); -int ff_nvenc_send_frame(AVCodecContext *avctx, const AVFrame *frame); - int ff_nvenc_receive_packet(AVCodecContext *avctx, AVPacket *pkt); -int ff_nvenc_encode_frame(AVCodecContext *avctx, AVPacket *pkt, - const AVFrame *frame, int *got_packet); - void ff_nvenc_encode_flush(AVCodecContext *avctx); extern const enum AVPixelFormat ff_nvenc_pix_fmts[]; diff --git a/libavcodec/nvenc_h264.c b/libavcodec/nvenc_h264.c index 479155fe15..bdb1423bb1 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,9 +232,7 @@ 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), diff --git a/libavcodec/nvenc_hevc.c b/libavcodec/nvenc_hevc.c index 7c9b3848f1..449ecc85ea 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,9 +192,7 @@ 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),
Signed-off-by: James Almer <jamrial@gmail.com> --- Version with the flush() callback left in place. But it will need the changes i originally added to avcodec_flush_buffers() and then removed for the latest iteration of this set, in some form or another. libavcodec/nvenc.c | 78 ++++++++++++++++++----------------------- libavcodec/nvenc.h | 9 ++--- libavcodec/nvenc_h264.c | 6 ---- libavcodec/nvenc_hevc.c | 4 --- 4 files changed, 36 insertions(+), 61 deletions(-)