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

---
 libavutil/frame.c | 23 +++++++++++++++++++++++
 libavutil/frame.h | 16 ++++++++++++++++
 2 files changed, 39 insertions(+)

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.

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

Missing check.

> +    }
> +
> +    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);)

> + * @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.

> + *
> + * @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?

> +
>  /**
>   * Get a side data entry of a specific type from a set.
>   *

On Sun, Aug 20, 2023 at 12:44 PM Andreas Rheinhardt
<andreas.rheinhardt@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