diff mbox series

[FFmpeg-devel,v7,07/14] avutil/frame: add helper for adding side data w/ AVBufferRef to array

Message ID 20240229164307.3535613-8-jeebjp@gmail.com
State New
Headers show
Series [FFmpeg-devel,v7,01/14] avutil/frame: split side data list wiping out to non-AVFrame function | expand

Checks

Context Check Description
yinshiyou/make_loongarch64 success Make finished
yinshiyou/make_fate_loongarch64 success Make fate finished
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished

Commit Message

Jan Ekström Feb. 29, 2024, 4:42 p.m. UTC 
This was requested to be added in review.
---
 libavutil/frame.c | 57 +++++++++++++++++++++++++++++++----------------
 libavutil/frame.h | 20 +++++++++++++++++
 2 files changed, 58 insertions(+), 19 deletions(-)

Comments

Anton Khirnov March 1, 2024, 6:19 p.m. UTC | #1 
Quoting Jan Ekström (2024-02-29 17:42:54)
> diff --git a/libavutil/frame.h b/libavutil/frame.h
> index 47d0096bc4..908fd4a90d 100644
> --- a/libavutil/frame.h
> +++ b/libavutil/frame.h
> @@ -1081,6 +1081,26 @@ AVFrameSideData *av_frame_side_data_new(AVFrameSideData ***sd, int *nb_sd,
>                                          enum AVFrameSideDataType type,
>                                          size_t size, unsigned int flags);
>  
> +/**
> + * Add a new side data entry to a set from an existing AVBufferRef.
> + *
> + * @param sd    pointer to array of side data to which to add another entry.
> + * @param nb_sd pointer to an integer containing the number of entries in
> + *              the array.
> + * @param type  type of the added side data
> + * @param buf   AVBufferRef for which a new reference will be made

This seems inconsistent with av_frame_new_side_data_from_buf(), which
takes ownership of the buffer reference passed to it.
Is there a strong reason to do it differently here?
James Almer March 1, 2024, 8:42 p.m. UTC | #2 
On 3/1/2024 3:19 PM, Anton Khirnov wrote:
> Quoting Jan Ekström (2024-02-29 17:42:54)
>> diff --git a/libavutil/frame.h b/libavutil/frame.h
>> index 47d0096bc4..908fd4a90d 100644
>> --- a/libavutil/frame.h
>> +++ b/libavutil/frame.h
>> @@ -1081,6 +1081,26 @@ AVFrameSideData *av_frame_side_data_new(AVFrameSideData ***sd, int *nb_sd,
>>                                           enum AVFrameSideDataType type,
>>                                           size_t size, unsigned int flags);
>>   
>> +/**
>> + * Add a new side data entry to a set from an existing AVBufferRef.
>> + *
>> + * @param sd    pointer to array of side data to which to add another entry.
>> + * @param nb_sd pointer to an integer containing the number of entries in
>> + *              the array.
>> + * @param type  type of the added side data
>> + * @param buf   AVBufferRef for which a new reference will be made
> 
> This seems inconsistent with av_frame_new_side_data_from_buf(), which
> takes ownership of the buffer reference passed to it.
> Is there a strong reason to do it differently here?

I asked for it. Taking ownership of the AVBufferRef and then expecting 
the caller to discard the pointer to it is awkward, and unlike any other 
function using AVBufferRef we have in the tree.

The alternative, to keep the behavior of taking ownership of the passed 
on reference, is to have this function take a pointer to pointer, and 
clearing it on success.
Anton Khirnov March 5, 2024, 11:04 a.m. UTC | #3 
Quoting James Almer (2024-03-01 21:42:28)
> On 3/1/2024 3:19 PM, Anton Khirnov wrote:
> > Quoting Jan Ekström (2024-02-29 17:42:54)
> >> diff --git a/libavutil/frame.h b/libavutil/frame.h
> >> index 47d0096bc4..908fd4a90d 100644
> >> --- a/libavutil/frame.h
> >> +++ b/libavutil/frame.h
> >> @@ -1081,6 +1081,26 @@ AVFrameSideData *av_frame_side_data_new(AVFrameSideData ***sd, int *nb_sd,
> >>                                           enum AVFrameSideDataType type,
> >>                                           size_t size, unsigned int flags);
> >>   
> >> +/**
> >> + * Add a new side data entry to a set from an existing AVBufferRef.
> >> + *
> >> + * @param sd    pointer to array of side data to which to add another entry.
> >> + * @param nb_sd pointer to an integer containing the number of entries in
> >> + *              the array.
> >> + * @param type  type of the added side data
> >> + * @param buf   AVBufferRef for which a new reference will be made
> > 
> > This seems inconsistent with av_frame_new_side_data_from_buf(), which
> > takes ownership of the buffer reference passed to it.
> > Is there a strong reason to do it differently here?
> 
> I asked for it. Taking ownership of the AVBufferRef and then expecting 
> the caller to discard the pointer to it is awkward, and unlike any other 
> function using AVBufferRef we have in the tree.
> 
> The alternative, to keep the behavior of taking ownership of the passed 
> on reference, is to have this function take a pointer to pointer, and 
> clearing it on success.

That seems preferable to me, in most cases the caller does not want to
keep a reference.
diff mbox series

Patch

diff --git a/libavutil/frame.c b/libavutil/frame.c
index 99c9ce4119..3a2084d1ca 100644
--- a/libavutil/frame.c
+++ b/libavutil/frame.c
@@ -889,36 +889,55 @@  AVFrameSideData *av_frame_side_data_new(AVFrameSideData ***sd, int *nb_sd,
     return ret;
 }
 
-int av_frame_side_data_from_sd(AVFrameSideData ***sd, int *nb_sd,
-                               const AVFrameSideData *src,
-                               unsigned int flags)
+AVFrameSideData *av_frame_side_data_from_buf(AVFrameSideData ***sd, int *nb_sd,
+                                             enum AVFrameSideDataType type,
+                                             const AVBufferRef *buf,
+                                             unsigned int flags)
 {
-    if (!sd || !src || !nb_sd || (*nb_sd && !*sd))
-        return AVERROR(EINVAL);
+    if (!sd || !buf || !nb_sd || (*nb_sd && !*sd))
+        return NULL;
+
+    if (flags & AV_FRAME_SIDE_DATA_SET_FLAG_NO_DUPLICATES)
+        remove_side_data(sd, nb_sd, type);
 
     {
-        AVBufferRef           *buf    = av_buffer_ref(src->buf);
-        AVFrameSideData       *sd_dst = NULL;
+        AVBufferRef     *new_buf = av_buffer_ref(buf);
+        AVFrameSideData *sd_dst  = NULL;
 
         if (flags & AV_FRAME_SIDE_DATA_SET_FLAG_NO_DUPLICATES)
-            remove_side_data(sd, nb_sd, src->type);
+            remove_side_data(sd, nb_sd, type);
 
-        sd_dst = add_side_data_to_set_from_buf(sd, nb_sd, src->type, buf);
+        sd_dst = add_side_data_to_set_from_buf(sd, nb_sd, type, new_buf);
         if (!sd_dst) {
-            av_buffer_unref(&buf);
-            return AVERROR(ENOMEM);
+            av_buffer_unref(&new_buf);
+            return NULL;
         }
 
-        {
-            int ret = av_dict_copy(&sd_dst->metadata, src->metadata, 0);
-            if (ret < 0) {
-                remove_side_data_by_entry(sd, nb_sd, sd_dst);
-                return ret;
-            }
-        }
+        return sd_dst;
+    }
+}
 
-        return 0;
+int av_frame_side_data_from_sd(AVFrameSideData ***sd, int *nb_sd,
+                               const AVFrameSideData *src,
+                               unsigned int flags)
+{
+    AVFrameSideData *sd_dst = NULL;
+    int ret = AVERROR_BUG;
+    if (!src)
+        return AVERROR(EINVAL);
+
+    sd_dst =
+        av_frame_side_data_from_buf(sd, nb_sd, src->type, src->buf, flags);
+    if (!sd_dst)
+        return AVERROR(ENOMEM);
+
+    ret = av_dict_copy(&sd_dst->metadata, src->metadata, 0);
+    if (ret < 0) {
+        remove_side_data_by_entry(sd, nb_sd, sd_dst);
+        return ret;
     }
+
+    return 0;
 }
 
 AVFrameSideData *av_frame_get_side_data(const AVFrame *frame,
diff --git a/libavutil/frame.h b/libavutil/frame.h
index 47d0096bc4..908fd4a90d 100644
--- a/libavutil/frame.h
+++ b/libavutil/frame.h
@@ -1081,6 +1081,26 @@  AVFrameSideData *av_frame_side_data_new(AVFrameSideData ***sd, int *nb_sd,
                                         enum AVFrameSideDataType type,
                                         size_t size, unsigned int flags);
 
+/**
+ * Add a new side data entry to a set from an existing AVBufferRef.
+ *
+ * @param sd    pointer to array of side data to which to add another entry.
+ * @param nb_sd pointer to an integer containing the number of entries in
+ *              the array.
+ * @param type  type of the added side data
+ * @param buf   AVBufferRef for which a new reference will be made
+ * @param flags Some combination of AV_FRAME_SIDE_DATA_SET_FLAG_* flags, or 0.
+ *
+ * @return newly added side data on success, NULL on error. In case of
+ *         AV_FRAME_SIDE_DATA_SET_FLAG_NO_DUPLICATES being set, entries
+ *         of matching AVFrameSideDataType will be removed before the
+ *         addition is attempted.
+ */
+AVFrameSideData *av_frame_side_data_from_buf(AVFrameSideData ***sd, int *nb_sd,
+                                             enum AVFrameSideDataType type,
+                                             const AVBufferRef *buf,
+                                             unsigned int flags);
+
 /**
  * Add a new side data entry to a set based on existing side data, taking
  * a reference towards the contained AVBufferRef.