[FFmpeg-devel] [PATCH 2/7 v4] avutil/frame: add helper for adding side data w/ AVBufferRef to array

[Anton Khirnov]

Quoting James Almer (2024-03-28 12:49:08)
> On 3/28/2024 8:27 AM, Anton Khirnov wrote:
> > Quoting James Almer (2024-03-28 04:12:05)
> >> Signed-off-by: James Almer <jamrial at gmail.com>
> >> ---
> >>   libavutil/frame.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++
> >>   libavutil/frame.h | 34 ++++++++++++++++++++++++++++++
> >>   2 files changed, 87 insertions(+)
> >>
> >> diff --git a/libavutil/frame.c b/libavutil/frame.c
> >> index d9bd19b2aa..a165e56a64 100644
> >> --- a/libavutil/frame.c
> >> +++ b/libavutil/frame.c
> >> @@ -834,6 +834,59 @@ AVFrameSideData *av_frame_side_data_new(AVFrameSideData ***sd, int *nb_sd,
> >>       return ret;
> >>   }
> >>   
> >> +AVFrameSideData *av_frame_side_data_add(AVFrameSideData ***sd, int *nb_sd,
> >> +                                        enum AVFrameSideDataType type,
> >> +                                        AVBufferRef **pbuf, unsigned int flags)
> >> +{
> >> +    const AVSideDataDescriptor *desc = av_frame_side_data_desc(type);
> >> +    AVFrameSideData *sd_dst  = NULL;
> >> +    AVBufferRef *buf;
> >> +
> >> +    if (!sd || !pbuf || !*pbuf || !nb_sd || (*nb_sd && !*sd))
> > 
> > Overzealous checks like these tend to hide bugs. Any of these conditions
> > being false means the caller is insane and we should crash.
> 
> I'll remove some, but others simplify the code below (like knowing 
> beforehand that *pbuf is not NULL).

You can just assume them all to be true. Or use av_assert0().

> >> diff --git a/libavutil/frame.h b/libavutil/frame.h
> >> index 2ea129888e..3e5d170a5b 100644
> >> --- a/libavutil/frame.h
> >> +++ b/libavutil/frame.h
> >> @@ -1048,6 +1048,10 @@ void av_frame_side_data_free(AVFrameSideData ***sd, int *nb_sd);
> >>    * Don't add a new entry if another of the same type exists.
> >>    */
> >>   #define AV_FRAME_SIDE_DATA_FLAG_REPLACE (1 << 1)
> >> +/**
> >> + * Create a new reference instead of taking ownership of the passed in one.
> >> + */
> >> +#define AV_FRAME_SIDE_DATA_FLAG_NEW_REF (1 << 2)
> > 
> > Who needs this?
> 
> Someone who wants to keep the reference around, like when attaching a 
> buffer to several outputs (global to individual output frames).

Is that a common enough use case to warrant this flag? It complicates
the code quite substantially.

And if you're making some side data instance global, what is the point
of also attaching it to frames?

-- 
Anton Khirnov