[FFmpeg-devel] [PATCH v5 3/3] avcodec/nvenc: write out user data unregistered SEI

[Brad Hards]

On Monday, 17 May 2021 9:25:10 PM AEST Timo Rothenpieler wrote:
> On 17.05.2021 10:19, Brad Hards wrote:
> > Signed-off-by: Brad Hards <bradh at frogmouth.net>
> > ---
> > 
> >   libavcodec/nvenc.c | 64 ++++++++++++++++++++++++++++++++++++++--------
> >   1 file changed, 53 insertions(+), 11 deletions(-)
> > 
> > diff --git a/libavcodec/nvenc.c b/libavcodec/nvenc.c
> > index 0dcd93a99c..e22fdfb5a8 100644
> > --- a/libavcodec/nvenc.c
> > +++ b/libavcodec/nvenc.c
> > @@ -2170,9 +2170,10 @@ static int nvenc_send_frame(AVCodecContext *avctx,
> > const AVFrame *frame)> 
> >       NVENCSTATUS nv_status;
> >       NvencSurface *tmp_out_surf, *in_surf;
> >       int res, res2;
> > 
> > -    NV_ENC_SEI_PAYLOAD sei_data[8];
> > +    NV_ENC_SEI_PAYLOAD *sei_data = 0;
> > 
> >       int sei_count = 0;
> >       int i;
> > 
> > +    int total_unregistered_sei = 0;
> > 
> >       NvencContext *ctx = avctx->priv_data;
> >       NvencDynLoadFunctions *dl_fn = &ctx->nvenc_dload_funcs;
> > 
> > @@ -2185,6 +2186,8 @@ static int nvenc_send_frame(AVCodecContext *avctx,
> > const AVFrame *frame)> 
> >           return AVERROR(EINVAL);
> >       
> >       if (frame && frame->buf[0]) {
> > 
> > +        void *a53_data = NULL;
> > +        void *tc_data = NULL;
> > 
> >           in_surf = get_free_frame(ctx);
> >           if (!in_surf)
> >           
> >               return AVERROR(EAGAIN);
> > 
> > @@ -2230,7 +2233,6 @@ static int nvenc_send_frame(AVCodecContext *avctx,
> > const AVFrame *frame)> 
> >           pic_params.inputTimeStamp = frame->pts;
> >           
> >           if (ctx->a53_cc && av_frame_get_side_data(frame,
> >           AV_FRAME_DATA_A53_CC)) {
> > 
> > -            void *a53_data = NULL;
> > 
> >               size_t a53_size = 0;
> >               
> >               if (ff_alloc_a53_sei(frame, 0, (void**)&a53_data, &a53_size)
> >               < 0) {
> > 
> > @@ -2238,15 +2240,21 @@ static int nvenc_send_frame(AVCodecContext *avctx,
> > const AVFrame *frame)> 
> >               }
> >               
> >               if (a53_data) {
> > 
> > -                sei_data[sei_count].payloadSize = (uint32_t)a53_size;
> > -                sei_data[sei_count].payloadType = 4;
> > -                sei_data[sei_count].payload = (uint8_t*)a53_data;
> > -                sei_count ++;
> > +                sei_data = av_realloc_array(sei_data, sei_count + 1,
> > sizeof(NV_ENC_SEI_PAYLOAD));
> Doing this realloc dance every frame is extremely inefficient.
> The sei_data array and its current size could instead be stored in the
> nvenc context, with a best-guess initial size, and then only grow when
> the already allocated size is too small.
I recognise that this might be frustrating you. Would you prefer to persist, 
or would it be easier just to take over the patch and do it the way you want 
it? I'm fine with either - just let me know.


> > +                if (!sei_data) {
> > +                    av_free(a53_data);
> > +                    sei_count = 0;
> > +                    return AVERROR(ENOMEM);
> > +                } else {
> > +                    sei_data[sei_count].payloadSize = (uint32_t)a53_size;
> > +                    sei_data[sei_count].payloadType = 4;
> > +                    sei_data[sei_count].payload = (uint8_t*)a53_data;
> > +                    sei_count ++;
> > +                }
> > 
> >               }
> >           
> >           }
> >           
> >           if (ctx->s12m_tc && av_frame_get_side_data(frame,
> >           AV_FRAME_DATA_S12M_TIMECODE)) {> 
> > -            void *tc_data = NULL;
> > 
> >               size_t tc_size = 0;
> >               
> >               if (ff_alloc_timecode_sei(frame, avctx->framerate, 0,
> >               (void**)&tc_data, &tc_size) < 0) {> 
> > @@ -2254,13 +2262,46 @@ static int nvenc_send_frame(AVCodecContext *avctx,
> > const AVFrame *frame)> 
> >               }
> >               
> >               if (tc_data) {
> > 
> > -                sei_data[sei_count].payloadSize = (uint32_t)tc_size;
> > -                sei_data[sei_count].payloadType = SEI_TYPE_TIME_CODE;
> > -                sei_data[sei_count].payload = (uint8_t*)tc_data;
> > -                sei_count ++;
> > +                sei_data = av_realloc_array(sei_data, sei_count + 1,
> > sizeof(NV_ENC_SEI_PAYLOAD)); +                if (!sei_data) {
> > +                    av_free(a53_data);
> > +                    av_free(tc_data);
> > +                    sei_count = 0;
> > +                    return AVERROR(ENOMEM);
> > +                } else {
> > +                    sei_data[sei_count].payloadSize = (uint32_t)tc_size;
> > +                    sei_data[sei_count].payloadType = SEI_TYPE_TIME_CODE;
> > +                    sei_data[sei_count].payload = (uint8_t*)tc_data;
> > +                    sei_count ++;
> > +                }
> > 
> >               }
> >           
> >           }
> > 
> > +        for (int j = 0; j < frame->nb_side_data; j++)
> > +            if (frame->side_data[j]->type ==
> > AV_FRAME_DATA_SEI_UNREGISTERED) +               
> > total_unregistered_sei++;
> > +        if (total_unregistered_sei > 0) {
> > +            sei_data = av_realloc_array(sei_data,
> > +                                        sei_count +
> > total_unregistered_sei, +                                       
> > sizeof(NV_ENC_SEI_PAYLOAD)); +            if (!sei_data) {
> > +                av_free(a53_data);
> > +                av_free(tc_data);
> > +                sei_count = 0;
> > +                return AVERROR(ENOMEM);
> > +            } else
> > +                for (int j = 0; j < frame->nb_side_data; j++) {
> > +                    AVFrameSideData *side_data = frame->side_data[j];
> > +                    if (side_data->type ==
> > AV_FRAME_DATA_SEI_UNREGISTERED) { +                       
> > sei_data[sei_count].payloadSize = side_data->size; +                     
> >   sei_data[sei_count].payloadType = SEI_TYPE_USER_DATA_UNREGISTERED; +   
> >                     sei_data[sei_count].payload =
> > av_memdup(side_data->data, side_data->size);
> Does this ever get freed anywhere?
Yes it does (just as for the other two payload allocations), in existing code.
See
https://github.com/FFmpeg/FFmpeg/blob/master/libavcodec/nvenc.c#L2275-L2276