[FFmpeg-devel] [PATCH v2] avcodec/nvenc: adapt to the new internal encode API

[James Almer]

On 4/9/2020 10:04 PM, Fu, Linjie wrote:
>> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of
>> James Almer
>> Sent: Friday, April 10, 2020 02:27
>> To: ffmpeg-devel at ffmpeg.org
>> Subject: [FFmpeg-devel] [PATCH v2] avcodec/nvenc: adapt to the new
>> internal encode API
>>
>> Signed-off-by: James Almer <jamrial at 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.