diff mbox series

[FFmpeg-devel,2/4] fftools/ffmpeg_enc: only promote first frame side data to global when meaningful

Message ID 20240323130409.20925-2-anton@khirnov.net
State New
Headers show
Series [FFmpeg-devel,1/4] lavu/frame: add side data descriptors | expand

Checks

Context Check Description
yinshiyou/configure_loongarch64 warning Failed to apply patch
andriy/configure_x86 warning Failed to apply patch

Commit Message

Anton Khirnov March 23, 2024, 1:04 p.m. UTC 
Skip those side data types that do not make sense as global side data.
---
 fftools/ffmpeg_enc.c | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Andreas Rheinhardt March 23, 2024, 1:15 p.m. UTC | #1 
Anton Khirnov:
> Skip those side data types that do not make sense as global side data.
> ---
>  fftools/ffmpeg_enc.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/fftools/ffmpeg_enc.c b/fftools/ffmpeg_enc.c
> index f01be1c22f..6a91fd0398 100644
> --- a/fftools/ffmpeg_enc.c
> +++ b/fftools/ffmpeg_enc.c
> @@ -247,6 +247,11 @@ int enc_open(void *opaque, const AVFrame *frame)
>          enc_ctx->chroma_sample_location = frame->chroma_location;
>  
>          for (int i = 0; i < frame->nb_side_data; i++) {
> +            const AVSideDataDescriptor *desc = av_frame_side_data_desc(frame->side_data[i]->type);
> +
> +            if (!desc || !(desc->props & AV_SIDE_DATA_PROP_GLOBAL))
> +                continue;

Why the first check? Is it intended that a defined side data type
doesn't have a descriptor? Because IIRC all side data types that can
occur here are defined and have not been created by letting the user
pass a number via options.

> +
>              ret = av_frame_side_data_clone(
>                  &enc_ctx->decoded_side_data, &enc_ctx->nb_decoded_side_data,
>                  frame->side_data[i], AV_FRAME_SIDE_DATA_FLAG_UNIQUE);
Anton Khirnov March 23, 2024, 1:19 p.m. UTC | #2 
Quoting Andreas Rheinhardt (2024-03-23 14:15:06)
> Anton Khirnov:
> > Skip those side data types that do not make sense as global side data.
> > ---
> >  fftools/ffmpeg_enc.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/fftools/ffmpeg_enc.c b/fftools/ffmpeg_enc.c
> > index f01be1c22f..6a91fd0398 100644
> > --- a/fftools/ffmpeg_enc.c
> > +++ b/fftools/ffmpeg_enc.c
> > @@ -247,6 +247,11 @@ int enc_open(void *opaque, const AVFrame *frame)
> >          enc_ctx->chroma_sample_location = frame->chroma_location;
> >  
> >          for (int i = 0; i < frame->nb_side_data; i++) {
> > +            const AVSideDataDescriptor *desc = av_frame_side_data_desc(frame->side_data[i]->type);
> > +
> > +            if (!desc || !(desc->props & AV_SIDE_DATA_PROP_GLOBAL))
> > +                continue;
> 
> Why the first check? Is it intended that a defined side data type
> doesn't have a descriptor? Because IIRC all side data types that can
> occur here are defined and have not been created by letting the user
> pass a number via options.

It shouldn't happen, but it's not inconceivable that e.g. a filter could
attach side data with an unknown type to a frame. I can remove the check
if you prefer it.
Andreas Rheinhardt March 23, 2024, 1:21 p.m. UTC | #3 
Anton Khirnov:
> Quoting Andreas Rheinhardt (2024-03-23 14:15:06)
>> Anton Khirnov:
>>> Skip those side data types that do not make sense as global side data.
>>> ---
>>>  fftools/ffmpeg_enc.c | 5 +++++
>>>  1 file changed, 5 insertions(+)
>>>
>>> diff --git a/fftools/ffmpeg_enc.c b/fftools/ffmpeg_enc.c
>>> index f01be1c22f..6a91fd0398 100644
>>> --- a/fftools/ffmpeg_enc.c
>>> +++ b/fftools/ffmpeg_enc.c
>>> @@ -247,6 +247,11 @@ int enc_open(void *opaque, const AVFrame *frame)
>>>          enc_ctx->chroma_sample_location = frame->chroma_location;
>>>  
>>>          for (int i = 0; i < frame->nb_side_data; i++) {
>>> +            const AVSideDataDescriptor *desc = av_frame_side_data_desc(frame->side_data[i]->type);
>>> +
>>> +            if (!desc || !(desc->props & AV_SIDE_DATA_PROP_GLOBAL))
>>> +                continue;
>>
>> Why the first check? Is it intended that a defined side data type
>> doesn't have a descriptor? Because IIRC all side data types that can
>> occur here are defined and have not been created by letting the user
>> pass a number via options.
> 
> It shouldn't happen, but it's not inconceivable that e.g. a filter could
> attach side data with an unknown type to a frame. I can remove the check
> if you prefer it.
> 

Yes. I'd consider such a filter buggy and that bug should not be covered up.

- Andreas
diff mbox series

Patch

diff --git a/fftools/ffmpeg_enc.c b/fftools/ffmpeg_enc.c
index f01be1c22f..6a91fd0398 100644
--- a/fftools/ffmpeg_enc.c
+++ b/fftools/ffmpeg_enc.c
@@ -247,6 +247,11 @@  int enc_open(void *opaque, const AVFrame *frame)
         enc_ctx->chroma_sample_location = frame->chroma_location;
 
         for (int i = 0; i < frame->nb_side_data; i++) {
+            const AVSideDataDescriptor *desc = av_frame_side_data_desc(frame->side_data[i]->type);
+
+            if (!desc || !(desc->props & AV_SIDE_DATA_PROP_GLOBAL))
+                continue;
+
             ret = av_frame_side_data_clone(
                 &enc_ctx->decoded_side_data, &enc_ctx->nb_decoded_side_data,
                 frame->side_data[i], AV_FRAME_SIDE_DATA_FLAG_UNIQUE);