[FFmpeg-devel] [PATCH] avutil/frame: Add av_frame_transfer_side_data() function
Soft Works
softworkz at hotmail.com
Mon May 23 11:00:12 EEST 2022
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of Anton
> Khirnov
> Sent: Monday, December 6, 2021 7:38 PM
> To: ffmpeg-devel at ffmpeg.org
> Subject: [FFmpeg-devel] [PATCH] avutil/frame: Add
> av_frame_transfer_side_data() function
>
> From: Soft Works <softworkz at hotmail.com>
>
> Signed-off-by: softworkz <softworkz at hotmail.com>
> Signed-off-by: Anton Khirnov <anton at 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
More information about the ffmpeg-devel
mailing list