diff mbox series

[FFmpeg-devel,4/5] libavcodec: write out user data unregistered SEI for nvenc

Message ID 20210430113432.308139-5-bradh@frogmouth.net
State New
Headers show
Series [FFmpeg-devel,1/5] libavutil: add convenience accessors for frame side data | 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

Brad Hards April 30, 2021, 11:34 a.m. UTC 
Signed-off-by: Brad Hards <bradh@frogmouth.net>
---
 libavcodec/nvenc.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

Comments

Timo Rothenpieler April 30, 2021, 3:27 p.m. UTC | #1 
On 30.04.2021 13:34, Brad Hards wrote:
> Signed-off-by: Brad Hards <bradh@frogmouth.net>
> ---
>   libavcodec/nvenc.c | 16 ++++++++++++++++
>   1 file changed, 16 insertions(+)
> 
> diff --git a/libavcodec/nvenc.c b/libavcodec/nvenc.c
> index 0dcd93a99c..1a895a64f4 100644
> --- a/libavcodec/nvenc.c
> +++ b/libavcodec/nvenc.c
> @@ -2173,6 +2173,8 @@ static int nvenc_send_frame(AVCodecContext *avctx, const AVFrame *frame)
>       NV_ENC_SEI_PAYLOAD sei_data[8];
>       int sei_count = 0;
>       int i;
> +    AVFrameSideData *sd;
> +    int num_unregistered_sei;
>   
>       NvencContext *ctx = avctx->priv_data;
>       NvencDynLoadFunctions *dl_fn = &ctx->nvenc_dload_funcs;
> @@ -2261,6 +2263,20 @@ static int nvenc_send_frame(AVCodecContext *avctx, const AVFrame *frame)
>               }
>           }
>   
> +        num_unregistered_sei = av_frame_num_side_data(frame, AV_FRAME_DATA_SEI_UNREGISTERED);
> +        for (int i = 0; i < num_unregistered_sei; i++) {

redefines i from above. Not sure if that's even valid.

> +            sd = av_frame_get_side_data_n(frame, AV_FRAME_DATA_SEI_UNREGISTERED, i);
> +            if (sd) {
> +                sei_data[sei_count].payloadSize = sd->size;
> +                sei_data[sei_count].payloadType = SEI_TYPE_USER_DATA_UNREGISTERED;
> +                sei_data[sei_count].payload = av_memdup(sd->data, sd->size);
> +                sei_count ++;
> +                if (sei_count >= 8) {
> +                    break;
> +                }
> +            }
> +        }

I'm not at all a fan of writing an arbitrary amount of stuff into a 
small fixed size array.

>           nvenc_codec_specific_pic_params(avctx, &pic_params, sei_data, 
sei_count);
>       } else {
>           pic_params.encodePicFlags = NV_ENC_PIC_FLAG_EOS;
>
Brad Hards April 30, 2021, 11:30 p.m. UTC | #2 
On Saturday, 1 May 2021 1:27:44 AM AEST Timo Rothenpieler wrote:
> On 30.04.2021 13:34, Brad Hards wrote:
> > +        num_unregistered_sei = av_frame_num_side_data(frame,
> > AV_FRAME_DATA_SEI_UNREGISTERED); +        for (int i = 0; i <
> > num_unregistered_sei; i++) {
> 
> redefines i from above. Not sure if that's even valid.
Will fix.

> > +            sd = av_frame_get_side_data_n(frame,
> > AV_FRAME_DATA_SEI_UNREGISTERED, i); +            if (sd) {
> > +                sei_data[sei_count].payloadSize = sd->size;
> > +                sei_data[sei_count].payloadType =
> > SEI_TYPE_USER_DATA_UNREGISTERED; +               
> > sei_data[sei_count].payload = av_memdup(sd->data, sd->size); +           
> >     sei_count ++;
> > +                if (sei_count >= 8) {
> > +                    break;
> > +                }
> > +            }
> > +        }
> 
> I'm not at all a fan of writing an arbitrary amount of stuff into a
> small fixed size array.
I don't understand this comment. The existing code defines a fixed size array, 
and I don't want to change it because I couldn't find an authoritative source 
as to why that size or approach. I assume it could be a hardware limitation on 
at least some versions.
So I've tried to follow the existing approach, and ensure that the loop never 
exceeds the available space by tracking the count.
Am I missing something?

Brad
Timo Rothenpieler April 30, 2021, 11:38 p.m. UTC | #3 
On 01.05.2021 01:30, Brad Hards wrote:
> I don't understand this comment. The existing code defines a fixed size 
array,
> and I don't want to change it because I couldn't find an authoritative source
> as to why that size or approach. I assume it could be a hardware limitation on
> at least some versions.
> So I've tried to follow the existing approach, and ensure that the loop 
never
> exceeds the available space by tracking the count.
> Am I missing something?

Pretty sure the 8 is just an arbitrary number picked when originally 
writing this code.
It now no longer seems appropriate, so a dynamic array might be in order.
diff mbox series

Patch

diff --git a/libavcodec/nvenc.c b/libavcodec/nvenc.c
index 0dcd93a99c..1a895a64f4 100644
--- a/libavcodec/nvenc.c
+++ b/libavcodec/nvenc.c
@@ -2173,6 +2173,8 @@  static int nvenc_send_frame(AVCodecContext *avctx, const AVFrame *frame)
     NV_ENC_SEI_PAYLOAD sei_data[8];
     int sei_count = 0;
     int i;
+    AVFrameSideData *sd;
+    int num_unregistered_sei;
 
     NvencContext *ctx = avctx->priv_data;
     NvencDynLoadFunctions *dl_fn = &ctx->nvenc_dload_funcs;
@@ -2261,6 +2263,20 @@  static int nvenc_send_frame(AVCodecContext *avctx, const AVFrame *frame)
             }
         }
 
+        num_unregistered_sei = av_frame_num_side_data(frame, AV_FRAME_DATA_SEI_UNREGISTERED);
+        for (int i = 0; i < num_unregistered_sei; i++) {
+            sd = av_frame_get_side_data_n(frame, AV_FRAME_DATA_SEI_UNREGISTERED, i);
+            if (sd) {
+                sei_data[sei_count].payloadSize = sd->size;
+                sei_data[sei_count].payloadType = SEI_TYPE_USER_DATA_UNREGISTERED;
+                sei_data[sei_count].payload = av_memdup(sd->data, sd->size);
+                sei_count ++;
+                if (sei_count >= 8) {
+                    break;
+                }
+            }
+        }
+
         nvenc_codec_specific_pic_params(avctx, &pic_params, sei_data, sei_count);
     } else {
         pic_params.encodePicFlags = NV_ENC_PIC_FLAG_EOS;