diff mbox series

[FFmpeg-devel] avutil/frame: Add av_frame_transfer_side_data() function

Message ID 20211206183747.21571-1-anton@khirnov.net
State New
Headers show
Series [FFmpeg-devel] avutil/frame: Add av_frame_transfer_side_data() function | expand

Checks

Context Check Description
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished
andriy/make_ppc success Make finished
andriy/make_fate_ppc success Make fate finished

Commit Message

Anton Khirnov Dec. 6, 2021, 6:37 p.m. UTC 
From: Soft Works <softworkz@hotmail.com>

Signed-off-by: softworkz <softworkz@hotmail.com>
Signed-off-by: Anton Khirnov <anton@khirnov.net>
---
 doc/APIchanges      |  4 +++
 libavutil/frame.c   | 63 ++++++++++++++++++++++++++-------------------
 libavutil/frame.h   | 20 ++++++++++++++
 libavutil/version.h |  4 +--
 4 files changed, 63 insertions(+), 28 deletions(-)

- renamed the function to av_frame_transfer_side_data(), since actually
  copying side data is only one of the two possible modes of behavior
  (and not even the default one)
- renamed flags accordingly
- added the AV_FRAME_TRANSFER_SD_FILTER flags as discussed

Comments

Soft Works Dec. 6, 2021, 6:48 p.m. UTC | #1 
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Anton
> Khirnov
> Sent: Monday, December 6, 2021 7:38 PM
> To: ffmpeg-devel@ffmpeg.org
> Subject: [FFmpeg-devel] [PATCH] avutil/frame: Add
> av_frame_transfer_side_data() function
> 
> From: Soft Works <softworkz@hotmail.com>
> 
> Signed-off-by: softworkz <softworkz@hotmail.com>
> Signed-off-by: Anton Khirnov <anton@khirnov.net>
> ---
>  doc/APIchanges      |  4 +++
>  libavutil/frame.c   | 63 ++++++++++++++++++++++++++-------------------
>  libavutil/frame.h   | 20 ++++++++++++++
>  libavutil/version.h |  4 +--
>  4 files changed, 63 insertions(+), 28 deletions(-)
> 
> - renamed the function to av_frame_transfer_side_data(), since actually
>   copying side data is only one of the two possible modes of behavior
>   (and not even the default one)
> - renamed flags accordingly
> - added the AV_FRAME_TRANSFER_SD_FILTER flags as discussed
> 
> diff --git a/doc/APIchanges b/doc/APIchanges
> index 2914ad6734..79cfea00b8 100644
> --- a/doc/APIchanges
> +++ b/doc/APIchanges
> @@ -14,6 +14,10 @@ libavutil:     2021-04-27
> 
>  API changes, most recent first:
> 
> +2021-12-02 - xxxxxxxxxx - lavu 57.11.100 - frame.h
> +  Add av_frame_transfer_side_data(), AV_FRAME_TRANSFER_SD_COPY, and
> +  AV_FRAME_TRANSFER_SD_FILTER.
> +
>  2021-11-xx - xxxxxxxxxx - lavfi 8.19.100 - avfilter.h
>    Add AVFILTER_FLAG_METADATA_ONLY.
> 
> diff --git a/libavutil/frame.c b/libavutil/frame.c
> index 0912ad9131..0b087cc4c9 100644
> --- a/libavutil/frame.c
> +++ b/libavutil/frame.c
> @@ -253,9 +253,40 @@ int av_frame_get_buffer(AVFrame *frame, int align)
>      return AVERROR(EINVAL);
>  }
> 
> +int av_frame_transfer_side_data(AVFrame *dst, const AVFrame *src, int flags)
> +{
> +    for (unsigned i = 0; i < src->nb_side_data; i++) {
> +        const AVFrameSideData *sd_src = src->side_data[i];
> +        AVFrameSideData *sd_dst;
> +        if ((flags & AV_FRAME_TRANSFER_SD_FILTER) &&
> +            sd_src->type == AV_FRAME_DATA_PANSCAN          &&
> +            (src->width != dst->width || src->height != dst->height))
> +            continue;
> +        if (flags & AV_FRAME_TRANSFER_SD_COPY) {
> +            sd_dst = av_frame_new_side_data(dst, sd_src->type,
> +                                            sd_src->size);
> +            if (!sd_dst) {
> +                wipe_side_data(dst);
> +                return AVERROR(ENOMEM);
> +            }
> +            memcpy(sd_dst->data, sd_src->data, sd_src->size);
> +        } else {
> +            AVBufferRef *ref = av_buffer_ref(sd_src->buf);
> +            sd_dst = av_frame_new_side_data_from_buf(dst, sd_src->type,
> ref);
> +            if (!sd_dst) {
> +                av_buffer_unref(&ref);
> +                wipe_side_data(dst);
> +                return AVERROR(ENOMEM);
> +            }
> +        }
> +        av_dict_copy(&sd_dst->metadata, sd_src->metadata, 0);
> +    }
> +    return 0;
> +}
> +
>  static int frame_copy_props(AVFrame *dst, const AVFrame *src, int
> force_copy)
>  {
> -    int ret, i;
> +    int ret;
> 
>      dst->key_frame              = src->key_frame;
>      dst->pict_type              = src->pict_type;
> @@ -291,31 +322,11 @@ static int frame_copy_props(AVFrame *dst, const AVFrame
> *src, int force_copy)
> 
>      av_dict_copy(&dst->metadata, src->metadata, 0);
> 
> -    for (i = 0; i < src->nb_side_data; i++) {
> -        const AVFrameSideData *sd_src = src->side_data[i];
> -        AVFrameSideData *sd_dst;
> -        if (   sd_src->type == AV_FRAME_DATA_PANSCAN
> -            && (src->width != dst->width || src->height != dst->height))
> -            continue;
> -        if (force_copy) {
> -            sd_dst = av_frame_new_side_data(dst, sd_src->type,
> -                                            sd_src->size);
> -            if (!sd_dst) {
> -                wipe_side_data(dst);
> -                return AVERROR(ENOMEM);
> -            }
> -            memcpy(sd_dst->data, sd_src->data, sd_src->size);
> -        } else {
> -            AVBufferRef *ref = av_buffer_ref(sd_src->buf);
> -            sd_dst = av_frame_new_side_data_from_buf(dst, sd_src->type,
> ref);
> -            if (!sd_dst) {
> -                av_buffer_unref(&ref);
> -                wipe_side_data(dst);
> -                return AVERROR(ENOMEM);
> -            }
> -        }
> -        av_dict_copy(&sd_dst->metadata, sd_src->metadata, 0);
> -    }
> +    ret = av_frame_transfer_side_data(dst, src,
> +            (force_copy ? AV_FRAME_TRANSFER_SD_COPY : 0) |
> +            AV_FRAME_TRANSFER_SD_FILTER);
> +    if (ret < 0)
> +        return ret;
> 
>      ret = av_buffer_replace(&dst->opaque_ref, src->opaque_ref);
>      ret |= av_buffer_replace(&dst->private_ref, src->private_ref);
> diff --git a/libavutil/frame.h b/libavutil/frame.h
> index 3f295f6b9e..deb399f1da 100644
> --- a/libavutil/frame.h
> +++ b/libavutil/frame.h
> @@ -873,6 +873,26 @@ AVFrameSideData *av_frame_get_side_data(const AVFrame
> *frame,
>   */
>  void av_frame_remove_side_data(AVFrame *frame, enum AVFrameSideDataType
> type);
> 
> +/**
> + * Copy side data, rather than creating new references.
> + */
> +#define AV_FRAME_TRANSFER_SD_COPY      (1 << 0)
> +/**
> + * Filter out side data that does not match dst properties.
> + */
> +#define AV_FRAME_TRANSFER_SD_FILTER    (1 << 1)
> +
> +/**
> + * Transfer all side-data from src to dst.
> + *
> + * @param flags a combination of AV_FRAME_TRANSFER_SD_*
> + *
> + * @return >= 0 on success, a negative AVERROR on error.
> + *
> + * @note This function will create new references to side data buffers in
> src,
> + * unless the AV_FRAME_TRANSFER_SD_COPY flag is passed.
> + */
> +int av_frame_transfer_side_data(AVFrame *dst, const AVFrame *src, int
> flags);
> 
>  /**
>   * Flags for frame cropping.
> diff --git a/libavutil/version.h b/libavutil/version.h
> index 017fc277a6..0e7b36865a 100644
> --- a/libavutil/version.h
> +++ b/libavutil/version.h
> @@ -79,8 +79,8 @@
>   */
> 
>  #define LIBAVUTIL_VERSION_MAJOR  57
> -#define LIBAVUTIL_VERSION_MINOR  10
> -#define LIBAVUTIL_VERSION_MICRO 101
> +#define LIBAVUTIL_VERSION_MINOR  11
> +#define LIBAVUTIL_VERSION_MICRO 100
> 
>  #define LIBAVUTIL_VERSION_INT   AV_VERSION_INT(LIBAVUTIL_VERSION_MAJOR, \
>                                                 LIBAVUTIL_VERSION_MINOR, \
> --

LGTM. Thanks for taking care of this!
Lynne Dec. 6, 2021, 7:12 p.m. UTC | #2 
6 Dec 2021, 19:37 by anton@khirnov.net:

> From: Soft Works <softworkz@hotmail.com>
>
> Signed-off-by: softworkz <softworkz@hotmail.com>
> Signed-off-by: Anton Khirnov <anton@khirnov.net>
> ---
>  doc/APIchanges      |  4 +++
>  libavutil/frame.c   | 63 ++++++++++++++++++++++++++-------------------
>  libavutil/frame.h   | 20 ++++++++++++++
>  libavutil/version.h |  4 +--
>  4 files changed, 63 insertions(+), 28 deletions(-)
>
> - renamed the function to av_frame_transfer_side_data(), since actually
>  copying side data is only one of the two possible modes of behavior
>  (and not even the default one)
> - renamed flags accordingly
> - added the AV_FRAME_TRANSFER_SD_FILTER flags as discussed
>
> diff --git a/doc/APIchanges b/doc/APIchanges
> index 2914ad6734..79cfea00b8 100644
> --- a/doc/APIchanges
> +++ b/doc/APIchanges
> @@ -14,6 +14,10 @@ libavutil:     2021-04-27
>  
>  API changes, most recent first:
>  
> +2021-12-02 - xxxxxxxxxx - lavu 57.11.100 - frame.h
> +  Add av_frame_transfer_side_data(), AV_FRAME_TRANSFER_SD_COPY, and
> +  AV_FRAME_TRANSFER_SD_FILTER.
> +
>  2021-11-xx - xxxxxxxxxx - lavfi 8.19.100 - avfilter.h
>  Add AVFILTER_FLAG_METADATA_ONLY.
>  
> diff --git a/libavutil/frame.c b/libavutil/frame.c
> index 0912ad9131..0b087cc4c9 100644
> --- a/libavutil/frame.c
> +++ b/libavutil/frame.c
> @@ -253,9 +253,40 @@ int av_frame_get_buffer(AVFrame *frame, int align)
>  return AVERROR(EINVAL);
>  }
>  
> +int av_frame_transfer_side_data(AVFrame *dst, const AVFrame *src, int flags)
> +{
> +    for (unsigned i = 0; i < src->nb_side_data; i++) {
> +        const AVFrameSideData *sd_src = src->side_data[i];
> +        AVFrameSideData *sd_dst;
> +        if ((flags & AV_FRAME_TRANSFER_SD_FILTER) &&
> +            sd_src->type == AV_FRAME_DATA_PANSCAN          &&
> +            (src->width != dst->width || src->height != dst->height))
> +            continue;
> +        if (flags & AV_FRAME_TRANSFER_SD_COPY) {
> +            sd_dst = av_frame_new_side_data(dst, sd_src->type,
> +                                            sd_src->size);
> +            if (!sd_dst) {
> +                wipe_side_data(dst);
> +                return AVERROR(ENOMEM);
> +            }
> +            memcpy(sd_dst->data, sd_src->data, sd_src->size);
> +        } else {
> +            AVBufferRef *ref = av_buffer_ref(sd_src->buf);
> +            sd_dst = av_frame_new_side_data_from_buf(dst, sd_src->type, ref);
> +            if (!sd_dst) {
> +                av_buffer_unref(&ref);
> +                wipe_side_data(dst);
> +                return AVERROR(ENOMEM);
> +            }
> +        }
> +        av_dict_copy(&sd_dst->metadata, sd_src->metadata, 0);
> +    }
> +    return 0;
> +}
> +
>  static int frame_copy_props(AVFrame *dst, const AVFrame *src, int force_copy)
>  {
> -    int ret, i;
> +    int ret;
>  
>  dst->key_frame              = src->key_frame;
>  dst->pict_type              = src->pict_type;
> @@ -291,31 +322,11 @@ static int frame_copy_props(AVFrame *dst, const AVFrame *src, int force_copy)
>  
>  av_dict_copy(&dst->metadata, src->metadata, 0);
>  
> -    for (i = 0; i < src->nb_side_data; i++) {
> -        const AVFrameSideData *sd_src = src->side_data[i];
> -        AVFrameSideData *sd_dst;
> -        if (   sd_src->type == AV_FRAME_DATA_PANSCAN
> -            && (src->width != dst->width || src->height != dst->height))
> -            continue;
> -        if (force_copy) {
> -            sd_dst = av_frame_new_side_data(dst, sd_src->type,
> -                                            sd_src->size);
> -            if (!sd_dst) {
> -                wipe_side_data(dst);
> -                return AVERROR(ENOMEM);
> -            }
> -            memcpy(sd_dst->data, sd_src->data, sd_src->size);
> -        } else {
> -            AVBufferRef *ref = av_buffer_ref(sd_src->buf);
> -            sd_dst = av_frame_new_side_data_from_buf(dst, sd_src->type, ref);
> -            if (!sd_dst) {
> -                av_buffer_unref(&ref);
> -                wipe_side_data(dst);
> -                return AVERROR(ENOMEM);
> -            }
> -        }
> -        av_dict_copy(&sd_dst->metadata, sd_src->metadata, 0);
> -    }
> +    ret = av_frame_transfer_side_data(dst, src,
> +            (force_copy ? AV_FRAME_TRANSFER_SD_COPY : 0) |
> +            AV_FRAME_TRANSFER_SD_FILTER);
> +    if (ret < 0)
> +        return ret;
>  
>  ret = av_buffer_replace(&dst->opaque_ref, src->opaque_ref);
>  ret |= av_buffer_replace(&dst->private_ref, src->private_ref);
> diff --git a/libavutil/frame.h b/libavutil/frame.h
> index 3f295f6b9e..deb399f1da 100644
> --- a/libavutil/frame.h
> +++ b/libavutil/frame.h
> @@ -873,6 +873,26 @@ AVFrameSideData *av_frame_get_side_data(const AVFrame *frame,
>  */
>  void av_frame_remove_side_data(AVFrame *frame, enum AVFrameSideDataType type);
>  
> +/**
> + * Copy side data, rather than creating new references.
> + */
> +#define AV_FRAME_TRANSFER_SD_COPY      (1 << 0)
> +/**
> + * Filter out side data that does not match dst properties.
> + */
> +#define AV_FRAME_TRANSFER_SD_FILTER    (1 << 1)
> +
> +/**
> + * Transfer all side-data from src to dst.
> + *
> + * @param flags a combination of AV_FRAME_TRANSFER_SD_*
> + *
> + * @return >= 0 on success, a negative AVERROR on error.
> + *
> + * @note This function will create new references to side data buffers in src,
> + * unless the AV_FRAME_TRANSFER_SD_COPY flag is passed.
> + */
> +int av_frame_transfer_side_data(AVFrame *dst, const AVFrame *src, int flags);
>  
>  /**
>  * Flags for frame cropping.
> diff --git a/libavutil/version.h b/libavutil/version.h
> index 017fc277a6..0e7b36865a 100644
> --- a/libavutil/version.h
> +++ b/libavutil/version.h
> @@ -79,8 +79,8 @@
>  */
>  
>  #define LIBAVUTIL_VERSION_MAJOR  57
> -#define LIBAVUTIL_VERSION_MINOR  10
> -#define LIBAVUTIL_VERSION_MICRO 101
> +#define LIBAVUTIL_VERSION_MINOR  11
> +#define LIBAVUTIL_VERSION_MICRO 100
>  
>  #define LIBAVUTIL_VERSION_INT   AV_VERSION_INT(LIBAVUTIL_VERSION_MAJOR, \
>  LIBAVUTIL_VERSION_MINOR, \
>

I think av_frame_copy_side_data() makes more sense. Transfer
implies that it strips the source frame of side data.
Anton Khirnov Dec. 7, 2021, 7:59 a.m. UTC | #3 
Quoting Lynne (2021-12-06 20:12:31)
> 6 Dec 2021, 19:37 by anton@khirnov.net:
> 
> 
> I think av_frame_copy_side_data() makes more sense. Transfer
> implies that it strips the source frame of side data.

The whole point is that it makes refs, not copies (except when it
doesn't). I am open to alternative names, but copy is a bad one IMO.
Hendrik Leppkes Dec. 7, 2021, 9:04 a.m. UTC | #4 
On Tue, Dec 7, 2021 at 9:00 AM Anton Khirnov <anton@khirnov.net> wrote:
>
> Quoting Lynne (2021-12-06 20:12:31)
> > 6 Dec 2021, 19:37 by anton@khirnov.net:
> >
> >
> > I think av_frame_copy_side_data() makes more sense. Transfer
> > implies that it strips the source frame of side data.
>
> The whole point is that it makes refs, not copies (except when it
> doesn't). I am open to alternative names, but copy is a bad one IMO.

How about staying close to avframe, and using av_frame_side_data_ref
or such (analogous to av_frame_ref)?

- Hendrik
Anton Khirnov Dec. 7, 2021, 10:28 a.m. UTC | #5 
Quoting Hendrik Leppkes (2021-12-07 10:04:47)
> On Tue, Dec 7, 2021 at 9:00 AM Anton Khirnov <anton@khirnov.net> wrote:
> >
> > Quoting Lynne (2021-12-06 20:12:31)
> > > 6 Dec 2021, 19:37 by anton@khirnov.net:
> > >
> > >
> > > I think av_frame_copy_side_data() makes more sense. Transfer
> > > implies that it strips the source frame of side data.
> >
> > The whole point is that it makes refs, not copies (except when it
> > doesn't). I am open to alternative names, but copy is a bad one IMO.
> 
> How about staying close to avframe, and using av_frame_side_data_ref
> or such (analogous to av_frame_ref)?

Possible I guess, but it still has the same problem that it doesn't
always create refs, only if you don't pass the relevant flag.
Andreas Rheinhardt Dec. 7, 2021, 11:28 a.m. UTC | #6 
Anton Khirnov:
> From: Soft Works <softworkz@hotmail.com>
> 
> Signed-off-by: softworkz <softworkz@hotmail.com>
> Signed-off-by: Anton Khirnov <anton@khirnov.net>
> ---
>  doc/APIchanges      |  4 +++
>  libavutil/frame.c   | 63 ++++++++++++++++++++++++++-------------------
>  libavutil/frame.h   | 20 ++++++++++++++
>  libavutil/version.h |  4 +--
>  4 files changed, 63 insertions(+), 28 deletions(-)
> 
> - renamed the function to av_frame_transfer_side_data(), since actually
>   copying side data is only one of the two possible modes of behavior
>   (and not even the default one)
> - renamed flags accordingly
> - added the AV_FRAME_TRANSFER_SD_FILTER flags as discussed
> 
> diff --git a/doc/APIchanges b/doc/APIchanges
> index 2914ad6734..79cfea00b8 100644
> --- a/doc/APIchanges
> +++ b/doc/APIchanges
> @@ -14,6 +14,10 @@ libavutil:     2021-04-27
>  
>  API changes, most recent first:
>  
> +2021-12-02 - xxxxxxxxxx - lavu 57.11.100 - frame.h
> +  Add av_frame_transfer_side_data(), AV_FRAME_TRANSFER_SD_COPY, and
> +  AV_FRAME_TRANSFER_SD_FILTER.
> +
>  2021-11-xx - xxxxxxxxxx - lavfi 8.19.100 - avfilter.h
>    Add AVFILTER_FLAG_METADATA_ONLY.
>  
> diff --git a/libavutil/frame.c b/libavutil/frame.c
> index 0912ad9131..0b087cc4c9 100644
> --- a/libavutil/frame.c
> +++ b/libavutil/frame.c
> @@ -253,9 +253,40 @@ int av_frame_get_buffer(AVFrame *frame, int align)
>      return AVERROR(EINVAL);
>  }
>  
> +int av_frame_transfer_side_data(AVFrame *dst, const AVFrame *src, int flags)
> +{
> +    for (unsigned i = 0; i < src->nb_side_data; i++) {
> +        const AVFrameSideData *sd_src = src->side_data[i];
> +        AVFrameSideData *sd_dst;
> +        if ((flags & AV_FRAME_TRANSFER_SD_FILTER) &&
> +            sd_src->type == AV_FRAME_DATA_PANSCAN          &&

Weird whitespace.

> +            (src->width != dst->width || src->height != dst->height))
> +            continue;
> +        if (flags & AV_FRAME_TRANSFER_SD_COPY) {
> +            sd_dst = av_frame_new_side_data(dst, sd_src->type,
> +                                            sd_src->size);
> +            if (!sd_dst) {
> +                wipe_side_data(dst);
> +                return AVERROR(ENOMEM);
> +            }
> +            memcpy(sd_dst->data, sd_src->data, sd_src->size);
> +        } else {
> +            AVBufferRef *ref = av_buffer_ref(sd_src->buf);
> +            sd_dst = av_frame_new_side_data_from_buf(dst, sd_src->type, ref);
> +            if (!sd_dst) {
> +                av_buffer_unref(&ref);
> +                wipe_side_data(dst);
> +                return AVERROR(ENOMEM);
> +            }
> +        }
> +        av_dict_copy(&sd_dst->metadata, sd_src->metadata, 0);
> +    }
> +    return 0;
> +}
> +
>  static int frame_copy_props(AVFrame *dst, const AVFrame *src, int force_copy)
>  {
> -    int ret, i;
> +    int ret;
>  
>      dst->key_frame              = src->key_frame;
>      dst->pict_type              = src->pict_type;
> @@ -291,31 +322,11 @@ static int frame_copy_props(AVFrame *dst, const AVFrame *src, int force_copy)
>  
>      av_dict_copy(&dst->metadata, src->metadata, 0);
>  
> -    for (i = 0; i < src->nb_side_data; i++) {
> -        const AVFrameSideData *sd_src = src->side_data[i];
> -        AVFrameSideData *sd_dst;
> -        if (   sd_src->type == AV_FRAME_DATA_PANSCAN
> -            && (src->width != dst->width || src->height != dst->height))
> -            continue;
> -        if (force_copy) {
> -            sd_dst = av_frame_new_side_data(dst, sd_src->type,
> -                                            sd_src->size);
> -            if (!sd_dst) {
> -                wipe_side_data(dst);
> -                return AVERROR(ENOMEM);
> -            }
> -            memcpy(sd_dst->data, sd_src->data, sd_src->size);
> -        } else {
> -            AVBufferRef *ref = av_buffer_ref(sd_src->buf);
> -            sd_dst = av_frame_new_side_data_from_buf(dst, sd_src->type, ref);
> -            if (!sd_dst) {
> -                av_buffer_unref(&ref);
> -                wipe_side_data(dst);
> -                return AVERROR(ENOMEM);
> -            }
> -        }
> -        av_dict_copy(&sd_dst->metadata, sd_src->metadata, 0);
> -    }
> +    ret = av_frame_transfer_side_data(dst, src,
> +            (force_copy ? AV_FRAME_TRANSFER_SD_COPY : 0) |
> +            AV_FRAME_TRANSFER_SD_FILTER);
> +    if (ret < 0)
> +        return ret;
>  
>      ret = av_buffer_replace(&dst->opaque_ref, src->opaque_ref);
>      ret |= av_buffer_replace(&dst->private_ref, src->private_ref);
> diff --git a/libavutil/frame.h b/libavutil/frame.h
> index 3f295f6b9e..deb399f1da 100644
> --- a/libavutil/frame.h
> +++ b/libavutil/frame.h
> @@ -873,6 +873,26 @@ AVFrameSideData *av_frame_get_side_data(const AVFrame *frame,
>   */
>  void av_frame_remove_side_data(AVFrame *frame, enum AVFrameSideDataType type);
>  
> +/**
> + * Copy side data, rather than creating new references.
> + */
> +#define AV_FRAME_TRANSFER_SD_COPY      (1 << 0)
> +/**
> + * Filter out side data that does not match dst properties.
> + */
> +#define AV_FRAME_TRANSFER_SD_FILTER    (1 << 1)
> +
> +/**
> + * Transfer all side-data from src to dst.
> + *
> + * @param flags a combination of AV_FRAME_TRANSFER_SD_*
> + *
> + * @return >= 0 on success, a negative AVERROR on error.
> + *
> + * @note This function will create new references to side data buffers in src,
> + * unless the AV_FRAME_TRANSFER_SD_COPY flag is passed.
> + */
> +int av_frame_transfer_side_data(AVFrame *dst, const AVFrame *src, int flags);
>  

I wonder whether this should be side-data only. The fields of an AVFrame
can be divided into several groups; we currently divide it into the
actual data fields and props and with this function props would be
further subdivided into side-data and non-side-data. We have functions
for copying some of these, but not for all; there is e.g. no function
that performs a copy of frame data (whether shallow or deep) and only
that. We also have no functions for moving or unref/resetting one group
of data.  If there were a flag for every group (plus another flag for
whether one wants deep or shallow copies (if that makes sense at all)),
one could reuse the same function. If there were
av_frame_make_writable() could be as follows in the non-writable case:
Allocate buffers in tmp frame, copy data into them, unref data stuff
from src frame and move data from tmp to src frame. (Whereas the current
implementation copies side-data (it is even an avoidable deep copy).)

(I am of course aware that the signature of this function would allow to
extend it to more general copying, it is just that the name would be
inappropriate.)

>  /**
>   * Flags for frame cropping.
> diff --git a/libavutil/version.h b/libavutil/version.h
> index 017fc277a6..0e7b36865a 100644
> --- a/libavutil/version.h
> +++ b/libavutil/version.h
> @@ -79,8 +79,8 @@
>   */
>  
>  #define LIBAVUTIL_VERSION_MAJOR  57
> -#define LIBAVUTIL_VERSION_MINOR  10
> -#define LIBAVUTIL_VERSION_MICRO 101
> +#define LIBAVUTIL_VERSION_MINOR  11
> +#define LIBAVUTIL_VERSION_MICRO 100
>  
>  #define LIBAVUTIL_VERSION_INT   AV_VERSION_INT(LIBAVUTIL_VERSION_MAJOR, \
>                                                 LIBAVUTIL_VERSION_MINOR, \
>
Soft Works May 23, 2022, 8 a.m. UTC | #7 
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Anton
> Khirnov
> Sent: Monday, December 6, 2021 7:38 PM
> To: ffmpeg-devel@ffmpeg.org
> Subject: [FFmpeg-devel] [PATCH] avutil/frame: Add
> av_frame_transfer_side_data() function
> 
> From: Soft Works <softworkz@hotmail.com>
> 
> Signed-off-by: softworkz <softworkz@hotmail.com>
> Signed-off-by: Anton Khirnov <anton@khirnov.net>
> ---
>  doc/APIchanges      |  4 +++
>  libavutil/frame.c   | 63 ++++++++++++++++++++++++++-------------------
>  libavutil/frame.h   | 20 ++++++++++++++
>  libavutil/version.h |  4 +--
>  4 files changed, 63 insertions(+), 28 deletions(-)
> 
> - renamed the function to av_frame_transfer_side_data(), since actually
>   copying side data is only one of the two possible modes of behavior
>   (and not even the default one)
> - renamed flags accordingly
> - added the AV_FRAME_TRANSFER_SD_FILTER flags as discussed
> 
> diff --git a/doc/APIchanges b/doc/APIchanges
> index 2914ad6734..79cfea00b8 100644
> --- a/doc/APIchanges
> +++ b/doc/APIchanges
> @@ -14,6 +14,10 @@ libavutil:     2021-04-27
> 
>  API changes, most recent first:
> 
> +2021-12-02 - xxxxxxxxxx - lavu 57.11.100 - frame.h
> +  Add av_frame_transfer_side_data(), AV_FRAME_TRANSFER_SD_COPY, and
> +  AV_FRAME_TRANSFER_SD_FILTER.
> +
>  2021-11-xx - xxxxxxxxxx - lavfi 8.19.100 - avfilter.h
>    Add AVFILTER_FLAG_METADATA_ONLY.
> 
> diff --git a/libavutil/frame.c b/libavutil/frame.c
> index 0912ad9131..0b087cc4c9 100644
> --- a/libavutil/frame.c
> +++ b/libavutil/frame.c
> @@ -253,9 +253,40 @@ int av_frame_get_buffer(AVFrame *frame, int align)
>      return AVERROR(EINVAL);
>  }
> 
> +int av_frame_transfer_side_data(AVFrame *dst, const AVFrame *src, int
> flags)
> +{
> +    for (unsigned i = 0; i < src->nb_side_data; i++) {
> +        const AVFrameSideData *sd_src = src->side_data[i];
> +        AVFrameSideData *sd_dst;
> +        if ((flags & AV_FRAME_TRANSFER_SD_FILTER) &&
> +            sd_src->type == AV_FRAME_DATA_PANSCAN          &&
> +            (src->width != dst->width || src->height != dst->height))
> +            continue;
> +        if (flags & AV_FRAME_TRANSFER_SD_COPY) {
> +            sd_dst = av_frame_new_side_data(dst, sd_src->type,
> +                                            sd_src->size);
> +            if (!sd_dst) {
> +                wipe_side_data(dst);
> +                return AVERROR(ENOMEM);
> +            }
> +            memcpy(sd_dst->data, sd_src->data, sd_src->size);
> +        } else {
> +            AVBufferRef *ref = av_buffer_ref(sd_src->buf);
> +            sd_dst = av_frame_new_side_data_from_buf(dst, sd_src->type,
> ref);
> +            if (!sd_dst) {
> +                av_buffer_unref(&ref);
> +                wipe_side_data(dst);
> +                return AVERROR(ENOMEM);
> +            }
> +        }
> +        av_dict_copy(&sd_dst->metadata, sd_src->metadata, 0);
> +    }
> +    return 0;
> +}
> +
>  static int frame_copy_props(AVFrame *dst, const AVFrame *src, int
> force_copy)
>  {
> -    int ret, i;
> +    int ret;
> 
>      dst->key_frame              = src->key_frame;
>      dst->pict_type              = src->pict_type;
> @@ -291,31 +322,11 @@ static int frame_copy_props(AVFrame *dst, const
> AVFrame *src, int force_copy)
> 
>      av_dict_copy(&dst->metadata, src->metadata, 0);
> 
> -    for (i = 0; i < src->nb_side_data; i++) {
> -        const AVFrameSideData *sd_src = src->side_data[i];
> -        AVFrameSideData *sd_dst;
> -        if (   sd_src->type == AV_FRAME_DATA_PANSCAN
> -            && (src->width != dst->width || src->height != dst->height))
> -            continue;
> -        if (force_copy) {
> -            sd_dst = av_frame_new_side_data(dst, sd_src->type,
> -                                            sd_src->size);
> -            if (!sd_dst) {
> -                wipe_side_data(dst);
> -                return AVERROR(ENOMEM);
> -            }
> -            memcpy(sd_dst->data, sd_src->data, sd_src->size);
> -        } else {
> -            AVBufferRef *ref = av_buffer_ref(sd_src->buf);
> -            sd_dst = av_frame_new_side_data_from_buf(dst, sd_src->type,
> ref);
> -            if (!sd_dst) {
> -                av_buffer_unref(&ref);
> -                wipe_side_data(dst);
> -                return AVERROR(ENOMEM);
> -            }
> -        }
> -        av_dict_copy(&sd_dst->metadata, sd_src->metadata, 0);
> -    }
> +    ret = av_frame_transfer_side_data(dst, src,
> +            (force_copy ? AV_FRAME_TRANSFER_SD_COPY : 0) |
> +            AV_FRAME_TRANSFER_SD_FILTER);
> +    if (ret < 0)
> +        return ret;
> 
>      ret = av_buffer_replace(&dst->opaque_ref, src->opaque_ref);
>      ret |= av_buffer_replace(&dst->private_ref, src->private_ref);
> diff --git a/libavutil/frame.h b/libavutil/frame.h
> index 3f295f6b9e..deb399f1da 100644
> --- a/libavutil/frame.h
> +++ b/libavutil/frame.h
> @@ -873,6 +873,26 @@ AVFrameSideData *av_frame_get_side_data(const AVFrame
> *frame,
>   */
>  void av_frame_remove_side_data(AVFrame *frame, enum AVFrameSideDataType
> type);
> 
> +/**
> + * Copy side data, rather than creating new references.
> + */
> +#define AV_FRAME_TRANSFER_SD_COPY      (1 << 0)
> +/**
> + * Filter out side data that does not match dst properties.
> + */
> +#define AV_FRAME_TRANSFER_SD_FILTER    (1 << 1)
> +
> +/**
> + * Transfer all side-data from src to dst.
> + *
> + * @param flags a combination of AV_FRAME_TRANSFER_SD_*
> + *
> + * @return >= 0 on success, a negative AVERROR on error.
> + *
> + * @note This function will create new references to side data buffers in
> src,
> + * unless the AV_FRAME_TRANSFER_SD_COPY flag is passed.
> + */
> +int av_frame_transfer_side_data(AVFrame *dst, const AVFrame *src, int
> flags);
> 
>  /**
>   * Flags for frame cropping.
> diff --git a/libavutil/version.h b/libavutil/version.h
> index 017fc277a6..0e7b36865a 100644
> --- a/libavutil/version.h
> +++ b/libavutil/version.h
> @@ -79,8 +79,8 @@
>   */
> 
>  #define LIBAVUTIL_VERSION_MAJOR  57
> -#define LIBAVUTIL_VERSION_MINOR  10
> -#define LIBAVUTIL_VERSION_MICRO 101
> +#define LIBAVUTIL_VERSION_MINOR  11
> +#define LIBAVUTIL_VERSION_MICRO 100
> 
>  #define LIBAVUTIL_VERSION_INT   AV_VERSION_INT(LIBAVUTIL_VERSION_MAJOR, \
>                                                 LIBAVUTIL_VERSION_MINOR, \
> --

Hi Anton,

I wonder whether we could move forward with this patch as I would like to
resubmit the original work that required this function?


There were two pending responses.

One from Lynne:

> I think av_frame_copy_side_data() makes more sense. Transfer
> implies that it strips the source frame of side data.

I wouldn't necessarily agree to that. Just think about a file
transfer: it doesn't imply that your file is gone afterwards.
But it also doesn't imply the opposite and other examples
could be found which confirm Lynne's point of view.

What's playing a more important role IMO is keeping the API
consistent, uniform and understandable. And when looking at
the API, I don't see that the term "Transfer" is part of the
common vocabulary for function names.
The only cases are in the area of hw frames data transfer 
and a single other function (something about timecode
transfer).

My original naming intended to establish some uniformity 
among the various functions for copying/cloning, like 
this for example:

int av_frame_copy_side_data(AVFrame* dst, const AVFrame* src, int force_copy)
int av_frame_copy_properties(AVFrame *dst, const AVFrame *src, int force_copy)
int av_frame_copy_subtitles(AVFrame *dst, const AVFrame *src, int force_copy)

To me that makes more sense than cooking a super-special
soup for each case - IMO a higher value IMO than extensibility
for some future unknown situations that may come or not.


Andreas commented as well:

> I wonder whether this should be side-data only. The fields of an AVFrame
> can be divided into several groups; we currently divide it into the
> actual data fields and props and with this function props would be
> further subdivided into side-data and non-side-data. We have functions
> for copying some of these, but not for all; there is e.g. no function
> that performs a copy of frame data (whether shallow or deep) and only
> that. 

I'm for creating such function when it's needed.

> We also have no functions for moving or unref/resetting one group
> of data.  If there were a flag for every group (plus another flag for
> whether one wants deep or shallow copies (if that makes sense at all)),
> one could reuse the same function. 

I wonder what would be the benefit? I mean, it's not that we would be
going short on functions and need to spare them. ;-)

Probably, you're having the usage side in mind, so let's compare the
code in both cases:

1. Separate Functions

av_frame_copy_side_data(dst, src, 1);
av_frame_copy_properties(dst, src, 0);
av_frame_copy_subtitles(dst, src, 1);

2. Single Function with flags

av_frame_copy_selected_data(dst, src, AV_FRAME_TRANSFER_SD 
                                    | AV_FRAME_TRANSFER_SD_COPY
                                    | AV_FRAME_TRANSFER_PROPS
                                    | AV_FRAME_TRANSFER_SUBTITLES 
                                    | AV_FRAME_TRANSFER_SUBTITLES_COPY);

Sure, you could merge the _copy flags, but it's mostly a matter
of taste, I suppose. I prefer code like #1, but all options are 
agreeable for me, I just hope we can get this little
function added soon in one way or the other :-)

Thanks,
sw
diff mbox series

Patch

diff --git a/doc/APIchanges b/doc/APIchanges
index 2914ad6734..79cfea00b8 100644
--- a/doc/APIchanges
+++ b/doc/APIchanges
@@ -14,6 +14,10 @@  libavutil:     2021-04-27
 
 API changes, most recent first:
 
+2021-12-02 - xxxxxxxxxx - lavu 57.11.100 - frame.h
+  Add av_frame_transfer_side_data(), AV_FRAME_TRANSFER_SD_COPY, and
+  AV_FRAME_TRANSFER_SD_FILTER.
+
 2021-11-xx - xxxxxxxxxx - lavfi 8.19.100 - avfilter.h
   Add AVFILTER_FLAG_METADATA_ONLY.
 
diff --git a/libavutil/frame.c b/libavutil/frame.c
index 0912ad9131..0b087cc4c9 100644
--- a/libavutil/frame.c
+++ b/libavutil/frame.c
@@ -253,9 +253,40 @@  int av_frame_get_buffer(AVFrame *frame, int align)
     return AVERROR(EINVAL);
 }
 
+int av_frame_transfer_side_data(AVFrame *dst, const AVFrame *src, int flags)
+{
+    for (unsigned i = 0; i < src->nb_side_data; i++) {
+        const AVFrameSideData *sd_src = src->side_data[i];
+        AVFrameSideData *sd_dst;
+        if ((flags & AV_FRAME_TRANSFER_SD_FILTER) &&
+            sd_src->type == AV_FRAME_DATA_PANSCAN          &&
+            (src->width != dst->width || src->height != dst->height))
+            continue;
+        if (flags & AV_FRAME_TRANSFER_SD_COPY) {
+            sd_dst = av_frame_new_side_data(dst, sd_src->type,
+                                            sd_src->size);
+            if (!sd_dst) {
+                wipe_side_data(dst);
+                return AVERROR(ENOMEM);
+            }
+            memcpy(sd_dst->data, sd_src->data, sd_src->size);
+        } else {
+            AVBufferRef *ref = av_buffer_ref(sd_src->buf);
+            sd_dst = av_frame_new_side_data_from_buf(dst, sd_src->type, ref);
+            if (!sd_dst) {
+                av_buffer_unref(&ref);
+                wipe_side_data(dst);
+                return AVERROR(ENOMEM);
+            }
+        }
+        av_dict_copy(&sd_dst->metadata, sd_src->metadata, 0);
+    }
+    return 0;
+}
+
 static int frame_copy_props(AVFrame *dst, const AVFrame *src, int force_copy)
 {
-    int ret, i;
+    int ret;
 
     dst->key_frame              = src->key_frame;
     dst->pict_type              = src->pict_type;
@@ -291,31 +322,11 @@  static int frame_copy_props(AVFrame *dst, const AVFrame *src, int force_copy)
 
     av_dict_copy(&dst->metadata, src->metadata, 0);
 
-    for (i = 0; i < src->nb_side_data; i++) {
-        const AVFrameSideData *sd_src = src->side_data[i];
-        AVFrameSideData *sd_dst;
-        if (   sd_src->type == AV_FRAME_DATA_PANSCAN
-            && (src->width != dst->width || src->height != dst->height))
-            continue;
-        if (force_copy) {
-            sd_dst = av_frame_new_side_data(dst, sd_src->type,
-                                            sd_src->size);
-            if (!sd_dst) {
-                wipe_side_data(dst);
-                return AVERROR(ENOMEM);
-            }
-            memcpy(sd_dst->data, sd_src->data, sd_src->size);
-        } else {
-            AVBufferRef *ref = av_buffer_ref(sd_src->buf);
-            sd_dst = av_frame_new_side_data_from_buf(dst, sd_src->type, ref);
-            if (!sd_dst) {
-                av_buffer_unref(&ref);
-                wipe_side_data(dst);
-                return AVERROR(ENOMEM);
-            }
-        }
-        av_dict_copy(&sd_dst->metadata, sd_src->metadata, 0);
-    }
+    ret = av_frame_transfer_side_data(dst, src,
+            (force_copy ? AV_FRAME_TRANSFER_SD_COPY : 0) |
+            AV_FRAME_TRANSFER_SD_FILTER);
+    if (ret < 0)
+        return ret;
 
     ret = av_buffer_replace(&dst->opaque_ref, src->opaque_ref);
     ret |= av_buffer_replace(&dst->private_ref, src->private_ref);
diff --git a/libavutil/frame.h b/libavutil/frame.h
index 3f295f6b9e..deb399f1da 100644
--- a/libavutil/frame.h
+++ b/libavutil/frame.h
@@ -873,6 +873,26 @@  AVFrameSideData *av_frame_get_side_data(const AVFrame *frame,
  */
 void av_frame_remove_side_data(AVFrame *frame, enum AVFrameSideDataType type);
 
+/**
+ * Copy side data, rather than creating new references.
+ */
+#define AV_FRAME_TRANSFER_SD_COPY      (1 << 0)
+/**
+ * Filter out side data that does not match dst properties.
+ */
+#define AV_FRAME_TRANSFER_SD_FILTER    (1 << 1)
+
+/**
+ * Transfer all side-data from src to dst.
+ *
+ * @param flags a combination of AV_FRAME_TRANSFER_SD_*
+ *
+ * @return >= 0 on success, a negative AVERROR on error.
+ *
+ * @note This function will create new references to side data buffers in src,
+ * unless the AV_FRAME_TRANSFER_SD_COPY flag is passed.
+ */
+int av_frame_transfer_side_data(AVFrame *dst, const AVFrame *src, int flags);
 
 /**
  * Flags for frame cropping.
diff --git a/libavutil/version.h b/libavutil/version.h
index 017fc277a6..0e7b36865a 100644
--- a/libavutil/version.h
+++ b/libavutil/version.h
@@ -79,8 +79,8 @@ 
  */
 
 #define LIBAVUTIL_VERSION_MAJOR  57
-#define LIBAVUTIL_VERSION_MINOR  10
-#define LIBAVUTIL_VERSION_MICRO 101
+#define LIBAVUTIL_VERSION_MINOR  11
+#define LIBAVUTIL_VERSION_MICRO 100
 
 #define LIBAVUTIL_VERSION_INT   AV_VERSION_INT(LIBAVUTIL_VERSION_MAJOR, \
                                                LIBAVUTIL_VERSION_MINOR, \