[FFmpeg-devel] [PATCH v3 07/12] avutil/frame: add helper for extending a set of side data

[Jan Ekström]

On Sun, Aug 20, 2023 at 12:44 PM Andreas Rheinhardt
<andreas.rheinhardt at outlook.com> wrote:
>
> Jan Ekström:
> > ---
> >  libavutil/frame.c | 23 +++++++++++++++++++++++
> >  libavutil/frame.h | 16 ++++++++++++++++
> >  2 files changed, 39 insertions(+)
> >
> > diff --git a/libavutil/frame.c b/libavutil/frame.c
> > index d8910a2120..04d56853f0 100644
> > --- a/libavutil/frame.c
> > +++ b/libavutil/frame.c
> > @@ -880,6 +880,29 @@ AVFrameSideData *av_side_data_set_new_item(AVFrameSideDataSet *set,
> >      return ret;
> >  }
> >
> > +int av_side_data_set_extend(AVFrameSideDataSet *dst,
> > +                            const AVFrameSideDataSet src,
> > +                            unsigned int allow_duplicates)
> > +{
> > +    for (int i = 0; i < src.nb_sd; i++) {
> > +        const AVFrameSideData *sd_src = src.sd[i];
> > +        AVFrameSideData *sd_dst =
> > +            av_side_data_set_new_item(dst, sd_src->type,
> > +                                      sd_src->size,
> > +                                      allow_duplicates);
> > +        if (!sd_dst) {
> > +            av_side_data_set_uninit(dst);
> > +            return AVERROR(ENOMEM);
> > +        }
> > +
> > +        memcpy(sd_dst->data, sd_src->data, sd_src->size);
>
> AVFrame side data is reference-counted.
>

Seems like I based this on av_frame_copy_props, so the force_copy=1
case. I guess following av_frame_ref makes more sense to follow.

> > +
> > +        av_dict_copy(&sd_dst->metadata, sd_src->metadata, 0);
>
> Missing check.
>

This comes straight out of frame_copy_props, so it seems like at least
a couple of av_dict_copy calls within it are unchecked :) .

I guess that warrants a separate fixup patch.

> > +    }
> > +
> > +    return 0;
> > +}
> > +
> >  AVFrameSideData *av_side_data_set_get_item(const AVFrameSideDataSet set,
> >                                             enum AVFrameSideDataType type)
> >  {
> > diff --git a/libavutil/frame.h b/libavutil/frame.h
> > index 0cafc9c51f..2413000c9a 100644
> > --- a/libavutil/frame.h
> > +++ b/libavutil/frame.h
> > @@ -1083,6 +1083,22 @@ AVFrameSideData *av_side_data_set_new_item(AVFrameSideDataSet *set,
> >                                             size_t size,
> >                                             unsigned int allow_duplicates);
> >
> > +/**
> > + * Add multiple side data entries to a set in one go.
> > + *
> > + * @param dst a set to which the side data should be added
> > + * @param src a set from which the side data should be copied from
>
> This needs to add something to ensure that dst and src refer to
> different side-data (i.e. to disallow calls like
> AVFrameSideDataSet set = { ... };
>
> av_side_data_set_extend(&set, set, 0);)
>

Sure, but interestingly enough only av_frame_replace currently has
something like this. av_frame_ref does check that dst has zero
width/height or channel count, but does not attempt to check that it
is being called with different contents.

> > + * @param allow_duplicates an unsigned integer noting whether duplicates are
> > + *                         allowed or not. If duplicates are not allowed, all
> > + *                         entries of the same side data type are first removed
> > + *                         and freed before the new entry is added.
>
> Better use flags.
>

Done, the current state of the branch as I go through people's reviews
is available at
https://github.com/jeeb/ffmpeg/commits/avcodec_cll_mdcv_side_data_v4 .

> > + *
> > + * @return negative error code on failure, >=0 on success.
> > + */
> > +int av_side_data_set_extend(AVFrameSideDataSet *dst,
> > +                            const AVFrameSideDataSet src,
> > +                            unsigned int allow_duplicates);
>
> Do we really need this function? Are there enough potential users of it?
>

I mostly added this for ffmpeg.c and if we want other API clients to
do something similar, there should be a public function to allow for
following similar behavior.

Or would you rather prefer a function related to avctx that takes in
an AVFrame, which does something like this internally?

Best regards,
Jan