[FFmpeg-devel] [PATCH 1/6 v2] avutil/frame: add a flag to not create duplicate entries in a side data array

[FFmpeg-devel] [PATCH 1/6 v2] avutil/frame: add a flag to not create duplicate entries in a side data array

[James Almer]

Signed-off-by: James Almer <jamrial@gmail.com>
---
 libavutil/frame.c | 14 ++++++++++++++
 libavutil/frame.h | 28 ++++++++++++++++++++--------
 2 files changed, 34 insertions(+), 8 deletions(-)

diff --git a/libavutil/frame.c b/libavutil/frame.c
index d7a32cdc92..a780e62fd0 100644
--- a/libavutil/frame.c
+++ b/libavutil/frame.c
@@ -774,6 +774,13 @@ AVFrameSideData *av_frame_side_data_new(AVFrameSideData ***sd, int *nb_sd,
 
     if (flags & AV_FRAME_SIDE_DATA_FLAG_UNIQUE)
         remove_side_data(sd, nb_sd, type);
+    if (flags & AV_FRAME_SIDE_DATA_FLAG_DONT_APPEND) {
+        for (int i = 0; i < *nb_sd; i++) {
+            AVFrameSideData *entry = ((*sd)[i]);
+            if (entry->type == type)
+                return entry;
+        }
+    }
 
     ret = add_side_data_from_buf(sd, nb_sd, type, buf);
     if (!ret)
@@ -798,6 +805,13 @@ int av_frame_side_data_clone(AVFrameSideData ***sd, int *nb_sd,
 
     if (flags & AV_FRAME_SIDE_DATA_FLAG_UNIQUE)
         remove_side_data(sd, nb_sd, src->type);
+    if (flags & AV_FRAME_SIDE_DATA_FLAG_DONT_APPEND) {
+        for (int i = 0; i < *nb_sd; i++) {
+            AVFrameSideData *entry = ((*sd)[i]);
+            if (entry->type == src->type)
+                return 0;
+        }
+    }
 
     sd_dst = add_side_data_from_buf(sd, nb_sd, src->type, buf);
     if (!sd_dst) {
diff --git a/libavutil/frame.h b/libavutil/frame.h
index 8aa05ec127..7cc55a455e 100644
--- a/libavutil/frame.h
+++ b/libavutil/frame.h
@@ -1003,7 +1003,14 @@ const char *av_frame_side_data_name(enum AVFrameSideDataType type);
  */
 void av_frame_side_data_free(AVFrameSideData ***sd, int *nb_sd);
 
+/**
+ * Remove existing entries before adding new ones.
+ */
 #define AV_FRAME_SIDE_DATA_FLAG_UNIQUE (1 << 0)
+/**
+ * Don't add a new entry if another of the same type exists.
+ */
+#define AV_FRAME_SIDE_DATA_FLAG_DONT_APPEND (1 << 1)
 
 /**
  * Add new side data entry to an array.
@@ -1016,10 +1023,12 @@ void av_frame_side_data_free(AVFrameSideData ***sd, int *nb_sd);
  * @param size  size of the side data
  * @param flags Some combination of AV_FRAME_SIDE_DATA_FLAG_* flags, or 0.
  *
- * @return newly added side data on success, NULL on error. In case of
- *         AV_FRAME_SIDE_DATA_FLAG_UNIQUE being set, entries of matching
- *         AVFrameSideDataType will be removed before the addition is
- *         attempted.
+ * @return newly added side data on success, NULL on error.
+ * @note In case of AV_FRAME_SIDE_DATA_FLAG_UNIQUE being set, entries of
+ *       matching AVFrameSideDataType will be removed before the addition
+ *       is attempted.
+ * @note In case of AV_FRAME_SIDE_DATA_FLAG_DONT_APPEND being set, if an
+ *       entry of the same type already exists, it will be returned instead.
  */
 AVFrameSideData *av_frame_side_data_new(AVFrameSideData ***sd, int *nb_sd,
                                         enum AVFrameSideDataType type,
@@ -1037,10 +1046,13 @@ AVFrameSideData *av_frame_side_data_new(AVFrameSideData ***sd, int *nb_sd,
  *              for the buffer.
  * @param flags Some combination of AV_FRAME_SIDE_DATA_FLAG_* flags, or 0.
  *
- * @return negative error code on failure, >=0 on success. In case of
- *         AV_FRAME_SIDE_DATA_FLAG_UNIQUE being set, entries of matching
- *         AVFrameSideDataType will be removed before the addition is
- *         attempted.
+ * @return negative error code on failure, >=0 on success.
+ * @note In case of AV_FRAME_SIDE_DATA_FLAG_UNIQUE being set, entries of
+ *       matching AVFrameSideDataType will be removed before the addition
+ *       is attempted.
+ * @note In case of AV_FRAME_SIDE_DATA_FLAG_DONT_APPEND being set, if an
+ *       entry of the same type as the one from src already exists, this
+ *       function will be a no-op.
  */
 int av_frame_side_data_clone(AVFrameSideData ***sd, int *nb_sd,
                              const AVFrameSideData *src, unsigned int flags);
-- 
2.44.0

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH 2/6 v2] avutil/frame: add helper for adding side data w/ AVBufferRef to array

[James Almer]

Signed-off-by: James Almer <jamrial@gmail.com>
---
 libavutil/frame.c | 43 +++++++++++++++++++++++++++++++++++++++++++
 libavutil/frame.h | 35 +++++++++++++++++++++++++++++++++++
 2 files changed, 78 insertions(+)

diff --git a/libavutil/frame.c b/libavutil/frame.c
index a780e62fd0..33c077998a 100644
--- a/libavutil/frame.c
+++ b/libavutil/frame.c
@@ -789,6 +789,49 @@ AVFrameSideData *av_frame_side_data_new(AVFrameSideData ***sd, int *nb_sd,
     return ret;
 }
 
+AVFrameSideData *av_frame_side_data_add(AVFrameSideData ***sd, int *nb_sd,
+                                        enum AVFrameSideDataType type,
+                                        AVBufferRef **pbuf, unsigned int flags)
+{
+    AVFrameSideData *sd_dst  = NULL;
+    AVBufferRef *buf;
+
+    if (!sd || !pbuf || !nb_sd || (*nb_sd && !*sd))
+        return NULL;
+
+    if (flags & AV_FRAME_SIDE_DATA_FLAG_UNIQUE)
+        remove_side_data(sd, nb_sd, type);
+    if (flags & AV_FRAME_SIDE_DATA_FLAG_DONT_APPEND) {
+        for (int i = 0; i < *nb_sd; i++) {
+            AVFrameSideData *entry = ((*sd)[i]);
+            if (entry->type == type) {
+                if (!(flags & AV_FRAME_SIDE_DATA_FLAG_NEW_REF))
+                    av_buffer_unref(pbuf);
+                return entry;
+            }
+        }
+    }
+
+    if (flags & AV_FRAME_SIDE_DATA_FLAG_NEW_REF)
+        buf = av_buffer_ref(*pbuf);
+    else
+        buf = *pbuf;
+    if (!buf)
+        return NULL;
+
+    sd_dst = add_side_data_from_buf(sd, nb_sd, type, buf);
+    if (!sd_dst) {
+        if (flags & AV_FRAME_SIDE_DATA_FLAG_NEW_REF)
+            av_buffer_unref(&buf);
+        return NULL;
+    }
+
+    if (!(flags & AV_FRAME_SIDE_DATA_FLAG_NEW_REF))
+        *pbuf = NULL;
+
+    return sd_dst;
+}
+
 int av_frame_side_data_clone(AVFrameSideData ***sd, int *nb_sd,
                              const AVFrameSideData *src, unsigned int flags)
 {
diff --git a/libavutil/frame.h b/libavutil/frame.h
index 7cc55a455e..e03ce39af7 100644
--- a/libavutil/frame.h
+++ b/libavutil/frame.h
@@ -1011,6 +1011,10 @@ void av_frame_side_data_free(AVFrameSideData ***sd, int *nb_sd);
  * Don't add a new entry if another of the same type exists.
  */
 #define AV_FRAME_SIDE_DATA_FLAG_DONT_APPEND (1 << 1)
+/**
+ * Create a new reference instead of taking ownership of the passed in one.
+ */
+#define AV_FRAME_SIDE_DATA_FLAG_NEW_REF (1 << 2)
 
 /**
  * Add new side data entry to an array.
@@ -1029,11 +1033,41 @@ void av_frame_side_data_free(AVFrameSideData ***sd, int *nb_sd);
  *       is attempted.
  * @note In case of AV_FRAME_SIDE_DATA_FLAG_DONT_APPEND being set, if an
  *       entry of the same type already exists, it will be returned instead.
+ * @note AV_FRAME_SIDE_DATA_FLAG_NEW_REF has no effect in this function.
  */
 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 an array from an existing AVBufferRef.
+ *
+ * @param sd    pointer to array of side data to which to add another entry,
+ *              or to NULL in order to start a new array.
+ * @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   Pointer to AVBufferRef to add to the array. On success,
+ *              the function takes ownership of the AVBufferRef and *buf is
+ *              set to NULL, unless AV_FRAME_SIDE_DATA_FLAG_NEW_REF is set
+ *              in which case the ownership will remain with the caller.
+ * @param flags Some combination of AV_FRAME_SIDE_DATA_FLAG_* flags, or 0.
+ *
+ * @return newly added side data on success, NULL on error.
+ * @note In case of AV_FRAME_SIDE_DATA_FLAG_UNIQUE being set, entries of
+ *       matching AVFrameSideDataType will be removed before the addition
+ *       is attempted.
+ * @note In case of AV_FRAME_SIDE_DATA_FLAG_DONT_APPEND being set, if an
+ *       entry of the same type already exists, it will be returned instead
+ *       and the array will be unchanged.
+ * @note In case of AV_FRAME_SIDE_DATA_FLAG_NEW_REF being set, the ownership
+ *       of *buf will remain with the caller.
+ *
+ */
+AVFrameSideData *av_frame_side_data_add(AVFrameSideData ***sd, int *nb_sd,
+                                        enum AVFrameSideDataType type,
+                                        AVBufferRef **buf, unsigned int flags);
+
 /**
  * Add a new side data entry to an array based on existing side data, taking
  * a reference towards the contained AVBufferRef.
@@ -1053,6 +1087,7 @@ AVFrameSideData *av_frame_side_data_new(AVFrameSideData ***sd, int *nb_sd,
  * @note In case of AV_FRAME_SIDE_DATA_FLAG_DONT_APPEND being set, if an
  *       entry of the same type as the one from src already exists, this
  *       function will be a no-op.
+ * @note AV_FRAME_SIDE_DATA_FLAG_NEW_REF has no effect in this function.
  */
 int av_frame_side_data_clone(AVFrameSideData ***sd, int *nb_sd,
                              const AVFrameSideData *src, unsigned int flags);
-- 
2.44.0

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH 02/10] avutil/frame: add helper for adding side data w/ AVBufferRef to array

[James Almer]

Signed-off-by: James Almer <jamrial@gmail.com>
---
 libavutil/frame.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++
 libavutil/frame.h | 34 ++++++++++++++++++++++++++++++
 2 files changed, 87 insertions(+)

diff --git a/libavutil/frame.c b/libavutil/frame.c
index 2e40018b83..08b6475cc2 100644
--- a/libavutil/frame.c
+++ b/libavutil/frame.c
@@ -834,6 +834,59 @@ AVFrameSideData *av_frame_side_data_new(AVFrameSideData ***sd, int *nb_sd,
     return ret;
 }
 
+AVFrameSideData *av_frame_side_data_add(AVFrameSideData ***sd, int *nb_sd,
+                                        enum AVFrameSideDataType type,
+                                        AVBufferRef **pbuf, unsigned int flags)
+{
+    const AVSideDataDescriptor *desc = av_frame_side_data_desc(type);
+    AVFrameSideData *sd_dst  = NULL;
+    AVBufferRef *buf;
+
+    if (!sd || !pbuf || *pbuf || !nb_sd || (*nb_sd && !*sd))
+        return NULL;
+
+    if (flags & AV_FRAME_SIDE_DATA_FLAG_UNIQUE)
+        remove_side_data(sd, nb_sd, type);
+    if (!desc || !(desc->props & AV_SIDE_DATA_PROP_MULTI)) {
+        for (int i = 0; i < *nb_sd; i++) {
+            AVFrameSideData *entry = ((*sd)[i]);
+
+            if (entry->type != type)
+                continue;
+            if (!(flags & AV_FRAME_SIDE_DATA_FLAG_REPLACE))
+                return NULL;
+
+            buf = *pbuf;
+            if (flags & AV_FRAME_SIDE_DATA_FLAG_NEW_REF) {
+                int ret = av_buffer_replace(&entry->buf, buf);
+                if (ret < 0)
+                    return NULL;
+            } else
+                *pbuf = NULL;
+
+            av_dict_free(&entry->metadata);
+            entry->data = buf->data;
+            entry->size = buf->size;
+            return 0;
+        }
+    }
+
+    buf = (flags & AV_FRAME_SIDE_DATA_FLAG_NEW_REF) ?
+           av_buffer_ref(*pbuf) : *pbuf;
+
+    sd_dst = add_side_data_from_buf(sd, nb_sd, type, buf);
+    if (!sd_dst) {
+        if (flags & AV_FRAME_SIDE_DATA_FLAG_NEW_REF)
+            av_buffer_unref(&buf);
+        return NULL;
+    }
+
+    if (!(flags & AV_FRAME_SIDE_DATA_FLAG_NEW_REF))
+        *pbuf = NULL;
+
+    return sd_dst;
+}
+
 int av_frame_side_data_clone(AVFrameSideData ***sd, int *nb_sd,
                              const AVFrameSideData *src, unsigned int flags)
 {
diff --git a/libavutil/frame.h b/libavutil/frame.h
index 2ea129888e..3e5d170a5b 100644
--- a/libavutil/frame.h
+++ b/libavutil/frame.h
@@ -1048,6 +1048,10 @@ void av_frame_side_data_free(AVFrameSideData ***sd, int *nb_sd);
  * Don't add a new entry if another of the same type exists.
  */
 #define AV_FRAME_SIDE_DATA_FLAG_REPLACE (1 << 1)
+/**
+ * Create a new reference instead of taking ownership of the passed in one.
+ */
+#define AV_FRAME_SIDE_DATA_FLAG_NEW_REF (1 << 2)
 
 /**
  * Add new side data entry to an array.
@@ -1066,11 +1070,40 @@ void av_frame_side_data_free(AVFrameSideData ***sd, int *nb_sd);
  *       is attempted.
  * @note In case of AV_FRAME_SIDE_DATA_FLAG_REPLACE being set, if an
  *       entry of the same type already exists, it will be replaced instead.
+ * @note AV_FRAME_SIDE_DATA_FLAG_NEW_REF has no effect in this function.
  */
 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 an array from an existing AVBufferRef.
+ *
+ * @param sd    pointer to array of side data to which to add another entry,
+ *              or to NULL in order to start a new array.
+ * @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   Pointer to AVBufferRef to add to the array. On success,
+ *              the function takes ownership of the AVBufferRef and *buf is
+ *              set to NULL, unless AV_FRAME_SIDE_DATA_FLAG_NEW_REF is set
+ *              in which case the ownership will remain with the caller.
+ * @param flags Some combination of AV_FRAME_SIDE_DATA_FLAG_* flags, or 0.
+ *
+ * @return newly added side data on success, NULL on error.
+ * @note In case of AV_FRAME_SIDE_DATA_FLAG_UNIQUE being set, entries of
+ *       matching AVFrameSideDataType will be removed before the addition
+ *       is attempted.
+ * @note In case of AV_FRAME_SIDE_DATA_FLAG_REPLACE being set, if an
+ *       entry of the same type already exists, it will be replaced instead.
+ * @note In case of AV_FRAME_SIDE_DATA_FLAG_NEW_REF being set, the ownership
+ *       of *buf will remain with the caller.
+ *
+ */
+AVFrameSideData *av_frame_side_data_add(AVFrameSideData ***sd, int *nb_sd,
+                                        enum AVFrameSideDataType type,
+                                        AVBufferRef **buf, unsigned int flags);
+
 /**
  * Add a new side data entry to an array based on existing side data, taking
  * a reference towards the contained AVBufferRef.
@@ -1089,6 +1122,7 @@ AVFrameSideData *av_frame_side_data_new(AVFrameSideData ***sd, int *nb_sd,
  *       is attempted.
  * @note In case of AV_FRAME_SIDE_DATA_FLAG_REPLACE being set, if an
  *       entry of the same type already exists, it will be replaced instead.
+ * @note AV_FRAME_SIDE_DATA_FLAG_NEW_REF has no effect in this function.
  */
 int av_frame_side_data_clone(AVFrameSideData ***sd, int *nb_sd,
                              const AVFrameSideData *src, unsigned int flags);
-- 
2.44.0

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH 2/6 v3] avutil/frame: add helper for adding side data w/ AVBufferRef to array

[James Almer]

Signed-off-by: James Almer <jamrial@gmail.com>
---
Same as the wrongly named 02/10 i sent earlier.

 libavutil/frame.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++
 libavutil/frame.h | 34 ++++++++++++++++++++++++++++++
 2 files changed, 87 insertions(+)

diff --git a/libavutil/frame.c b/libavutil/frame.c
index 2e40018b83..08b6475cc2 100644
--- a/libavutil/frame.c
+++ b/libavutil/frame.c
@@ -834,6 +834,59 @@ AVFrameSideData *av_frame_side_data_new(AVFrameSideData ***sd, int *nb_sd,
     return ret;
 }
 
+AVFrameSideData *av_frame_side_data_add(AVFrameSideData ***sd, int *nb_sd,
+                                        enum AVFrameSideDataType type,
+                                        AVBufferRef **pbuf, unsigned int flags)
+{
+    const AVSideDataDescriptor *desc = av_frame_side_data_desc(type);
+    AVFrameSideData *sd_dst  = NULL;
+    AVBufferRef *buf;
+
+    if (!sd || !pbuf || *pbuf || !nb_sd || (*nb_sd && !*sd))
+        return NULL;
+
+    if (flags & AV_FRAME_SIDE_DATA_FLAG_UNIQUE)
+        remove_side_data(sd, nb_sd, type);
+    if (!desc || !(desc->props & AV_SIDE_DATA_PROP_MULTI)) {
+        for (int i = 0; i < *nb_sd; i++) {
+            AVFrameSideData *entry = ((*sd)[i]);
+
+            if (entry->type != type)
+                continue;
+            if (!(flags & AV_FRAME_SIDE_DATA_FLAG_REPLACE))
+                return NULL;
+
+            buf = *pbuf;
+            if (flags & AV_FRAME_SIDE_DATA_FLAG_NEW_REF) {
+                int ret = av_buffer_replace(&entry->buf, buf);
+                if (ret < 0)
+                    return NULL;
+            } else
+                *pbuf = NULL;
+
+            av_dict_free(&entry->metadata);
+            entry->data = buf->data;
+            entry->size = buf->size;
+            return 0;
+        }
+    }
+
+    buf = (flags & AV_FRAME_SIDE_DATA_FLAG_NEW_REF) ?
+           av_buffer_ref(*pbuf) : *pbuf;
+
+    sd_dst = add_side_data_from_buf(sd, nb_sd, type, buf);
+    if (!sd_dst) {
+        if (flags & AV_FRAME_SIDE_DATA_FLAG_NEW_REF)
+            av_buffer_unref(&buf);
+        return NULL;
+    }
+
+    if (!(flags & AV_FRAME_SIDE_DATA_FLAG_NEW_REF))
+        *pbuf = NULL;
+
+    return sd_dst;
+}
+
 int av_frame_side_data_clone(AVFrameSideData ***sd, int *nb_sd,
                              const AVFrameSideData *src, unsigned int flags)
 {
diff --git a/libavutil/frame.h b/libavutil/frame.h
index 2ea129888e..3e5d170a5b 100644
--- a/libavutil/frame.h
+++ b/libavutil/frame.h
@@ -1048,6 +1048,10 @@ void av_frame_side_data_free(AVFrameSideData ***sd, int *nb_sd);
  * Don't add a new entry if another of the same type exists.
  */
 #define AV_FRAME_SIDE_DATA_FLAG_REPLACE (1 << 1)
+/**
+ * Create a new reference instead of taking ownership of the passed in one.
+ */
+#define AV_FRAME_SIDE_DATA_FLAG_NEW_REF (1 << 2)
 
 /**
  * Add new side data entry to an array.
@@ -1066,11 +1070,40 @@ void av_frame_side_data_free(AVFrameSideData ***sd, int *nb_sd);
  *       is attempted.
  * @note In case of AV_FRAME_SIDE_DATA_FLAG_REPLACE being set, if an
  *       entry of the same type already exists, it will be replaced instead.
+ * @note AV_FRAME_SIDE_DATA_FLAG_NEW_REF has no effect in this function.
  */
 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 an array from an existing AVBufferRef.
+ *
+ * @param sd    pointer to array of side data to which to add another entry,
+ *              or to NULL in order to start a new array.
+ * @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   Pointer to AVBufferRef to add to the array. On success,
+ *              the function takes ownership of the AVBufferRef and *buf is
+ *              set to NULL, unless AV_FRAME_SIDE_DATA_FLAG_NEW_REF is set
+ *              in which case the ownership will remain with the caller.
+ * @param flags Some combination of AV_FRAME_SIDE_DATA_FLAG_* flags, or 0.
+ *
+ * @return newly added side data on success, NULL on error.
+ * @note In case of AV_FRAME_SIDE_DATA_FLAG_UNIQUE being set, entries of
+ *       matching AVFrameSideDataType will be removed before the addition
+ *       is attempted.
+ * @note In case of AV_FRAME_SIDE_DATA_FLAG_REPLACE being set, if an
+ *       entry of the same type already exists, it will be replaced instead.
+ * @note In case of AV_FRAME_SIDE_DATA_FLAG_NEW_REF being set, the ownership
+ *       of *buf will remain with the caller.
+ *
+ */
+AVFrameSideData *av_frame_side_data_add(AVFrameSideData ***sd, int *nb_sd,
+                                        enum AVFrameSideDataType type,
+                                        AVBufferRef **buf, unsigned int flags);
+
 /**
  * Add a new side data entry to an array based on existing side data, taking
  * a reference towards the contained AVBufferRef.
@@ -1089,6 +1122,7 @@ AVFrameSideData *av_frame_side_data_new(AVFrameSideData ***sd, int *nb_sd,
  *       is attempted.
  * @note In case of AV_FRAME_SIDE_DATA_FLAG_REPLACE being set, if an
  *       entry of the same type already exists, it will be replaced instead.
+ * @note AV_FRAME_SIDE_DATA_FLAG_NEW_REF has no effect in this function.
  */
 int av_frame_side_data_clone(AVFrameSideData ***sd, int *nb_sd,
                              const AVFrameSideData *src, unsigned int flags);
-- 
2.44.0

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH 3/6 v2] avutil/frame: add helper to remove side data of a given type from an array

[James Almer]

Signed-off-by: James Almer <jamrial@gmail.com>
---
 libavutil/frame.c | 6 ++++++
 libavutil/frame.h | 5 +++++
 2 files changed, 11 insertions(+)

diff --git a/libavutil/frame.c b/libavutil/frame.c
index 33c077998a..10c7d3ebf0 100644
--- a/libavutil/frame.c
+++ b/libavutil/frame.c
@@ -882,6 +882,12 @@ const AVFrameSideData *av_frame_side_data_get_c(const AVFrameSideData * const *s
     return NULL;
 }
 
+void av_frame_side_data_remove(AVFrameSideData ***sd, int *nb_sd,
+                               enum AVFrameSideDataType type)
+{
+    remove_side_data(sd, nb_sd, type);
+}
+
 AVFrameSideData *av_frame_get_side_data(const AVFrame *frame,
                                         enum AVFrameSideDataType type)
 {
diff --git a/libavutil/frame.h b/libavutil/frame.h
index e03ce39af7..043f446b38 100644
--- a/libavutil/frame.h
+++ b/libavutil/frame.h
@@ -1121,6 +1121,11 @@ const AVFrameSideData *av_frame_side_data_get(AVFrameSideData * const *sd,
                                     nb_sd, type);
 }
 
+/**
+ * Remove and free all side data instances of the given type from an array.
+ */
+void av_frame_side_data_remove(AVFrameSideData ***sd, int *nb_sd,
+                               enum AVFrameSideDataType type);
 /**
  * @}
  */
-- 
2.44.0

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH 4/6 v2] avutil/mastering_display_metadata: add a new allocator function that returns a size

[James Almer]

av_mastering_display_metadata_alloc() is not useful in scenarios where you need to
know the runtime size of AVMasteringDisplayMetadata.

Signed-off-by: James Almer <jamrial@gmail.com>
---
 libavutil/mastering_display_metadata.c | 13 +++++++++++++
 libavutil/mastering_display_metadata.h |  9 +++++++++
 2 files changed, 22 insertions(+)

diff --git a/libavutil/mastering_display_metadata.c b/libavutil/mastering_display_metadata.c
index 6069347617..ea41f13f9d 100644
--- a/libavutil/mastering_display_metadata.c
+++ b/libavutil/mastering_display_metadata.c
@@ -18,6 +18,7 @@
  * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
  */
 
+#include <stddef.h>
 #include <stdint.h>
 #include <string.h>
 
@@ -29,6 +30,18 @@ AVMasteringDisplayMetadata *av_mastering_display_metadata_alloc(void)
     return av_mallocz(sizeof(AVMasteringDisplayMetadata));
 }
 
+AVMasteringDisplayMetadata *av_mastering_display_metadata_alloc_size(size_t *size)
+{
+    AVMasteringDisplayMetadata *mastering = av_mallocz(sizeof(AVMasteringDisplayMetadata));
+    if (!mastering)
+        return NULL;
+
+    if (size)
+        *size = sizeof(*mastering);
+
+    return mastering;
+}
+
 AVMasteringDisplayMetadata *av_mastering_display_metadata_create_side_data(AVFrame *frame)
 {
     AVFrameSideData *side_data = av_frame_new_side_data(frame,
diff --git a/libavutil/mastering_display_metadata.h b/libavutil/mastering_display_metadata.h
index c23b07c3cd..52fcef9e37 100644
--- a/libavutil/mastering_display_metadata.h
+++ b/libavutil/mastering_display_metadata.h
@@ -77,6 +77,15 @@ typedef struct AVMasteringDisplayMetadata {
  */
 AVMasteringDisplayMetadata *av_mastering_display_metadata_alloc(void);
 
+/**
+ * Allocate an AVMasteringDisplayMetadata structure and set its fields to
+ * default values. The resulting struct can be freed using av_freep().
+ *
+ * @return An AVMasteringDisplayMetadata filled with default values or NULL
+ *         on failure.
+ */
+AVMasteringDisplayMetadata *av_mastering_display_metadata_alloc_size(size_t *size);
+
 /**
  * Allocate a complete AVMasteringDisplayMetadata and add it to the frame.
  *
-- 
2.44.0

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 4/6 v2] avutil/mastering_display_metadata: add a new allocator function that returns a size

[Andreas Rheinhardt]

James Almer:
> av_mastering_display_metadata_alloc() is not useful in scenarios where you need to
> know the runtime size of AVMasteringDisplayMetadata.
> 
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
>  libavutil/mastering_display_metadata.c | 13 +++++++++++++
>  libavutil/mastering_display_metadata.h |  9 +++++++++
>  2 files changed, 22 insertions(+)
> 
> diff --git a/libavutil/mastering_display_metadata.c b/libavutil/mastering_display_metadata.c
> index 6069347617..ea41f13f9d 100644
> --- a/libavutil/mastering_display_metadata.c
> +++ b/libavutil/mastering_display_metadata.c
> @@ -18,6 +18,7 @@
>   * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
>   */
>  
> +#include <stddef.h>
>  #include <stdint.h>
>  #include <string.h>
>  
> @@ -29,6 +30,18 @@ AVMasteringDisplayMetadata *av_mastering_display_metadata_alloc(void)
>      return av_mallocz(sizeof(AVMasteringDisplayMetadata));
>  }
>  
> +AVMasteringDisplayMetadata *av_mastering_display_metadata_alloc_size(size_t *size)
> +{
> +    AVMasteringDisplayMetadata *mastering = av_mallocz(sizeof(AVMasteringDisplayMetadata));
> +    if (!mastering)
> +        return NULL;
> +
> +    if (size)
> +        *size = sizeof(*mastering);
> +
> +    return mastering;
> +}
> +
>  AVMasteringDisplayMetadata *av_mastering_display_metadata_create_side_data(AVFrame *frame)
>  {
>      AVFrameSideData *side_data = av_frame_new_side_data(frame,
> diff --git a/libavutil/mastering_display_metadata.h b/libavutil/mastering_display_metadata.h
> index c23b07c3cd..52fcef9e37 100644
> --- a/libavutil/mastering_display_metadata.h
> +++ b/libavutil/mastering_display_metadata.h
> @@ -77,6 +77,15 @@ typedef struct AVMasteringDisplayMetadata {
>   */
>  AVMasteringDisplayMetadata *av_mastering_display_metadata_alloc(void);
>  
> +/**
> + * Allocate an AVMasteringDisplayMetadata structure and set its fields to
> + * default values. The resulting struct can be freed using av_freep().
> + *
> + * @return An AVMasteringDisplayMetadata filled with default values or NULL
> + *         on failure.
> + */
> +AVMasteringDisplayMetadata *av_mastering_display_metadata_alloc_size(size_t *size);
> +
>  /**
>   * Allocate a complete AVMasteringDisplayMetadata and add it to the frame.
>   *

Instead of this we should have a generic allocator like
void *av_frame_side_data_allocate(enum AVFrameSideDataType, size_t
*size, size_t elem_count), with the latter being used for the allocators
that allocate arrays (like AVRegionOfInterest); it has to be set to zero
for the others. This will also avoid creating new
av_*_create_side_data() functions.

- Andreas

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 4/6 v2] avutil/mastering_display_metadata: add a new allocator function that returns a size

[James Almer]

On 3/25/2024 5:40 PM, Andreas Rheinhardt wrote:
> James Almer:
>> av_mastering_display_metadata_alloc() is not useful in scenarios where you need to
>> know the runtime size of AVMasteringDisplayMetadata.
>>
>> Signed-off-by: James Almer <jamrial@gmail.com>
>> ---
>>   libavutil/mastering_display_metadata.c | 13 +++++++++++++
>>   libavutil/mastering_display_metadata.h |  9 +++++++++
>>   2 files changed, 22 insertions(+)
>>
>> diff --git a/libavutil/mastering_display_metadata.c b/libavutil/mastering_display_metadata.c
>> index 6069347617..ea41f13f9d 100644
>> --- a/libavutil/mastering_display_metadata.c
>> +++ b/libavutil/mastering_display_metadata.c
>> @@ -18,6 +18,7 @@
>>    * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
>>    */
>>   
>> +#include <stddef.h>
>>   #include <stdint.h>
>>   #include <string.h>
>>   
>> @@ -29,6 +30,18 @@ AVMasteringDisplayMetadata *av_mastering_display_metadata_alloc(void)
>>       return av_mallocz(sizeof(AVMasteringDisplayMetadata));
>>   }
>>   
>> +AVMasteringDisplayMetadata *av_mastering_display_metadata_alloc_size(size_t *size)
>> +{
>> +    AVMasteringDisplayMetadata *mastering = av_mallocz(sizeof(AVMasteringDisplayMetadata));
>> +    if (!mastering)
>> +        return NULL;
>> +
>> +    if (size)
>> +        *size = sizeof(*mastering);
>> +
>> +    return mastering;
>> +}
>> +
>>   AVMasteringDisplayMetadata *av_mastering_display_metadata_create_side_data(AVFrame *frame)
>>   {
>>       AVFrameSideData *side_data = av_frame_new_side_data(frame,
>> diff --git a/libavutil/mastering_display_metadata.h b/libavutil/mastering_display_metadata.h
>> index c23b07c3cd..52fcef9e37 100644
>> --- a/libavutil/mastering_display_metadata.h
>> +++ b/libavutil/mastering_display_metadata.h
>> @@ -77,6 +77,15 @@ typedef struct AVMasteringDisplayMetadata {
>>    */
>>   AVMasteringDisplayMetadata *av_mastering_display_metadata_alloc(void);
>>   
>> +/**
>> + * Allocate an AVMasteringDisplayMetadata structure and set its fields to
>> + * default values. The resulting struct can be freed using av_freep().
>> + *
>> + * @return An AVMasteringDisplayMetadata filled with default values or NULL
>> + *         on failure.
>> + */
>> +AVMasteringDisplayMetadata *av_mastering_display_metadata_alloc_size(size_t *size);
>> +
>>   /**
>>    * Allocate a complete AVMasteringDisplayMetadata and add it to the frame.
>>    *
> 
> Instead of this we should have a generic allocator like
> void *av_frame_side_data_allocate(enum AVFrameSideDataType, size_t
> *size, size_t elem_count), with the latter being used for the allocators
> that allocate arrays (like AVRegionOfInterest); it has to be set to zero
> for the others. This will also avoid creating new
> av_*_create_side_data() functions.

I don't mind a function like that being added to simplify future 
additions, but this API is orthogonal to the frame side data one. It's 
also used in packets, for example, and right now lavf is using 
sizeof(AVMasteringDisplayMetadata) because 
av_mastering_display_metadata_alloc() is not useful.

> 
> - Andreas
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 4/6 v2] avutil/mastering_display_metadata: add a new allocator function that returns a size

[Andreas Rheinhardt]

James Almer:
> On 3/25/2024 5:40 PM, Andreas Rheinhardt wrote:
>> James Almer:
>>> av_mastering_display_metadata_alloc() is not useful in scenarios
>>> where you need to
>>> know the runtime size of AVMasteringDisplayMetadata.
>>>
>>> Signed-off-by: James Almer <jamrial@gmail.com>
>>> ---
>>>   libavutil/mastering_display_metadata.c | 13 +++++++++++++
>>>   libavutil/mastering_display_metadata.h |  9 +++++++++
>>>   2 files changed, 22 insertions(+)
>>>
>>> diff --git a/libavutil/mastering_display_metadata.c
>>> b/libavutil/mastering_display_metadata.c
>>> index 6069347617..ea41f13f9d 100644
>>> --- a/libavutil/mastering_display_metadata.c
>>> +++ b/libavutil/mastering_display_metadata.c
>>> @@ -18,6 +18,7 @@
>>>    * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
>>> 02110-1301 USA
>>>    */
>>>   +#include <stddef.h>
>>>   #include <stdint.h>
>>>   #include <string.h>
>>>   @@ -29,6 +30,18 @@ AVMasteringDisplayMetadata
>>> *av_mastering_display_metadata_alloc(void)
>>>       return av_mallocz(sizeof(AVMasteringDisplayMetadata));
>>>   }
>>>   +AVMasteringDisplayMetadata
>>> *av_mastering_display_metadata_alloc_size(size_t *size)
>>> +{
>>> +    AVMasteringDisplayMetadata *mastering =
>>> av_mallocz(sizeof(AVMasteringDisplayMetadata));
>>> +    if (!mastering)
>>> +        return NULL;
>>> +
>>> +    if (size)
>>> +        *size = sizeof(*mastering);
>>> +
>>> +    return mastering;
>>> +}
>>> +
>>>   AVMasteringDisplayMetadata
>>> *av_mastering_display_metadata_create_side_data(AVFrame *frame)
>>>   {
>>>       AVFrameSideData *side_data = av_frame_new_side_data(frame,
>>> diff --git a/libavutil/mastering_display_metadata.h
>>> b/libavutil/mastering_display_metadata.h
>>> index c23b07c3cd..52fcef9e37 100644
>>> --- a/libavutil/mastering_display_metadata.h
>>> +++ b/libavutil/mastering_display_metadata.h
>>> @@ -77,6 +77,15 @@ typedef struct AVMasteringDisplayMetadata {
>>>    */
>>>   AVMasteringDisplayMetadata *av_mastering_display_metadata_alloc(void);
>>>   +/**
>>> + * Allocate an AVMasteringDisplayMetadata structure and set its
>>> fields to
>>> + * default values. The resulting struct can be freed using av_freep().
>>> + *
>>> + * @return An AVMasteringDisplayMetadata filled with default values
>>> or NULL
>>> + *         on failure.
>>> + */
>>> +AVMasteringDisplayMetadata
>>> *av_mastering_display_metadata_alloc_size(size_t *size);
>>> +
>>>   /**
>>>    * Allocate a complete AVMasteringDisplayMetadata and add it to the
>>> frame.
>>>    *
>>
>> Instead of this we should have a generic allocator like
>> void *av_frame_side_data_allocate(enum AVFrameSideDataType, size_t
>> *size, size_t elem_count), with the latter being used for the allocators
>> that allocate arrays (like AVRegionOfInterest); it has to be set to zero
>> for the others. This will also avoid creating new
>> av_*_create_side_data() functions.
> 
> I don't mind a function like that being added to simplify future
> additions, but this API is orthogonal to the frame side data one. It's
> also used in packets, for example, and right now lavf is using
> sizeof(AVMasteringDisplayMetadata) because
> av_mastering_display_metadata_alloc() is not useful.
> 

The API proposed by me is supposed to make API like
av_mastering_display_metadata_alloc_size() redundant and therefore these
two additions are not orthogonal.

- Andreas

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 4/6 v2] avutil/mastering_display_metadata: add a new allocator function that returns a size

[James Almer]

On 3/25/2024 6:02 PM, Andreas Rheinhardt wrote:
> James Almer:
>> On 3/25/2024 5:40 PM, Andreas Rheinhardt wrote:
>>> James Almer:
>>>> av_mastering_display_metadata_alloc() is not useful in scenarios
>>>> where you need to
>>>> know the runtime size of AVMasteringDisplayMetadata.
>>>>
>>>> Signed-off-by: James Almer <jamrial@gmail.com>
>>>> ---
>>>>    libavutil/mastering_display_metadata.c | 13 +++++++++++++
>>>>    libavutil/mastering_display_metadata.h |  9 +++++++++
>>>>    2 files changed, 22 insertions(+)
>>>>
>>>> diff --git a/libavutil/mastering_display_metadata.c
>>>> b/libavutil/mastering_display_metadata.c
>>>> index 6069347617..ea41f13f9d 100644
>>>> --- a/libavutil/mastering_display_metadata.c
>>>> +++ b/libavutil/mastering_display_metadata.c
>>>> @@ -18,6 +18,7 @@
>>>>     * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
>>>> 02110-1301 USA
>>>>     */
>>>>    +#include <stddef.h>
>>>>    #include <stdint.h>
>>>>    #include <string.h>
>>>>    @@ -29,6 +30,18 @@ AVMasteringDisplayMetadata
>>>> *av_mastering_display_metadata_alloc(void)
>>>>        return av_mallocz(sizeof(AVMasteringDisplayMetadata));
>>>>    }
>>>>    +AVMasteringDisplayMetadata
>>>> *av_mastering_display_metadata_alloc_size(size_t *size)
>>>> +{
>>>> +    AVMasteringDisplayMetadata *mastering =
>>>> av_mallocz(sizeof(AVMasteringDisplayMetadata));
>>>> +    if (!mastering)
>>>> +        return NULL;
>>>> +
>>>> +    if (size)
>>>> +        *size = sizeof(*mastering);
>>>> +
>>>> +    return mastering;
>>>> +}
>>>> +
>>>>    AVMasteringDisplayMetadata
>>>> *av_mastering_display_metadata_create_side_data(AVFrame *frame)
>>>>    {
>>>>        AVFrameSideData *side_data = av_frame_new_side_data(frame,
>>>> diff --git a/libavutil/mastering_display_metadata.h
>>>> b/libavutil/mastering_display_metadata.h
>>>> index c23b07c3cd..52fcef9e37 100644
>>>> --- a/libavutil/mastering_display_metadata.h
>>>> +++ b/libavutil/mastering_display_metadata.h
>>>> @@ -77,6 +77,15 @@ typedef struct AVMasteringDisplayMetadata {
>>>>     */
>>>>    AVMasteringDisplayMetadata *av_mastering_display_metadata_alloc(void);
>>>>    +/**
>>>> + * Allocate an AVMasteringDisplayMetadata structure and set its
>>>> fields to
>>>> + * default values. The resulting struct can be freed using av_freep().
>>>> + *
>>>> + * @return An AVMasteringDisplayMetadata filled with default values
>>>> or NULL
>>>> + *         on failure.
>>>> + */
>>>> +AVMasteringDisplayMetadata
>>>> *av_mastering_display_metadata_alloc_size(size_t *size);
>>>> +
>>>>    /**
>>>>     * Allocate a complete AVMasteringDisplayMetadata and add it to the
>>>> frame.
>>>>     *
>>>
>>> Instead of this we should have a generic allocator like
>>> void *av_frame_side_data_allocate(enum AVFrameSideDataType, size_t
>>> *size, size_t elem_count), with the latter being used for the allocators
>>> that allocate arrays (like AVRegionOfInterest); it has to be set to zero
>>> for the others. This will also avoid creating new
>>> av_*_create_side_data() functions.
>>
>> I don't mind a function like that being added to simplify future
>> additions, but this API is orthogonal to the frame side data one. It's
>> also used in packets, for example, and right now lavf is using
>> sizeof(AVMasteringDisplayMetadata) because
>> av_mastering_display_metadata_alloc() is not useful.
>>
> 
> The API proposed by me is supposed to make API like
> av_mastering_display_metadata_alloc_size() redundant and therefore these
> two additions are not orthogonal.

Just because there's a frame side data type for MDM does not make the 
new alloc function redundant. The frame side data API is one API 
wrapping others. You don't see code using the AVHash API when they only 
care about AVMD5. MDM is and should be standalone.
av_mastering_display_metadata_alloc() should be deprecated and removed 
after this, too. That will ensure we still have a single symbol for this.

Regarding the av_*_create_side_data() functions, they were never needed. 
They are a convenient wrapper/shortcut, and your 
av_frame_side_data_allocate() would not really replace them as you'd 
still need to use av_frame_add_side_data() afterwards.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 4/6 v2] avutil/mastering_display_metadata: add a new allocator function that returns a size

[Anton Khirnov]

Quoting James Almer (2024-03-25 22:13:25)
> On 3/25/2024 6:02 PM, Andreas Rheinhardt wrote:
> > James Almer:
> >> I don't mind a function like that being added to simplify future
> >> additions, but this API is orthogonal to the frame side data one. It's
> >> also used in packets, for example, and right now lavf is using
> >> sizeof(AVMasteringDisplayMetadata) because
> >> av_mastering_display_metadata_alloc() is not useful.
> >>
> > 
> > The API proposed by me is supposed to make API like
> > av_mastering_display_metadata_alloc_size() redundant and therefore these
> > two additions are not orthogonal.
> 
> Just because there's a frame side data type for MDM does not make the 
> new alloc function redundant.

The commit message says:

> av_mastering_display_metadata_alloc() is not useful in scenarios where you need to
> know the runtime size of AVMasteringDisplayMetadata.

In what scenarios besides side data do you need to know the size of this
struct?

-- 
Anton Khirnov
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 4/6 v2] avutil/mastering_display_metadata: add a new allocator function that returns a size

[James Almer]

On 3/27/2024 4:41 AM, Anton Khirnov wrote:
> Quoting James Almer (2024-03-25 22:13:25)
>> On 3/25/2024 6:02 PM, Andreas Rheinhardt wrote:
>>> James Almer:
>>>> I don't mind a function like that being added to simplify future
>>>> additions, but this API is orthogonal to the frame side data one. It's
>>>> also used in packets, for example, and right now lavf is using
>>>> sizeof(AVMasteringDisplayMetadata) because
>>>> av_mastering_display_metadata_alloc() is not useful.
>>>>
>>>
>>> The API proposed by me is supposed to make API like
>>> av_mastering_display_metadata_alloc_size() redundant and therefore these
>>> two additions are not orthogonal.
>>
>> Just because there's a frame side data type for MDM does not make the
>> new alloc function redundant.
> 
> The commit message says:
> 
>> av_mastering_display_metadata_alloc() is not useful in scenarios where you need to
>> know the runtime size of AVMasteringDisplayMetadata.
> 
> In what scenarios besides side data do you need to know the size of this
> struct?

None within our libraries that i can think of, but library users can 
have scenarios we need to provide for. MDM is a standalone API, so lets 
not try to make its usability depend on a separate one.
I'm replacing a helper with a better one, it should not be so controversial.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 4/6 v2] avutil/mastering_display_metadata: add a new allocator function that returns a size

[Anton Khirnov]

Quoting James Almer (2024-03-27 13:35:35)
> On 3/27/2024 4:41 AM, Anton Khirnov wrote:
> > Quoting James Almer (2024-03-25 22:13:25)
> >> On 3/25/2024 6:02 PM, Andreas Rheinhardt wrote:
> >>> James Almer:
> >>>> I don't mind a function like that being added to simplify future
> >>>> additions, but this API is orthogonal to the frame side data one. It's
> >>>> also used in packets, for example, and right now lavf is using
> >>>> sizeof(AVMasteringDisplayMetadata) because
> >>>> av_mastering_display_metadata_alloc() is not useful.
> >>>>
> >>>
> >>> The API proposed by me is supposed to make API like
> >>> av_mastering_display_metadata_alloc_size() redundant and therefore these
> >>> two additions are not orthogonal.
> >>
> >> Just because there's a frame side data type for MDM does not make the
> >> new alloc function redundant.
> > 
> > The commit message says:
> > 
> >> av_mastering_display_metadata_alloc() is not useful in scenarios where you need to
> >> know the runtime size of AVMasteringDisplayMetadata.
> > 
> > In what scenarios besides side data do you need to know the size of this
> > struct?
> 
> None within our libraries that i can think of, but library users can 
> have scenarios we need to provide for. MDM is a standalone API, so lets 
> not try to make its usability depend on a separate one.
> I'm replacing a helper with a better one, it should not be so controversial.

Breaking API to benefit hypothetical use cases IS a controversial change
in my view.

-- 
Anton Khirnov
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 4/6 v2] avutil/mastering_display_metadata: add a new allocator function that returns a size

[James Almer]

On 3/27/2024 9:40 AM, Anton Khirnov wrote:
> Quoting James Almer (2024-03-27 13:35:35)
>> On 3/27/2024 4:41 AM, Anton Khirnov wrote:
>>> Quoting James Almer (2024-03-25 22:13:25)
>>>> On 3/25/2024 6:02 PM, Andreas Rheinhardt wrote:
>>>>> James Almer:
>>>>>> I don't mind a function like that being added to simplify future
>>>>>> additions, but this API is orthogonal to the frame side data one. It's
>>>>>> also used in packets, for example, and right now lavf is using
>>>>>> sizeof(AVMasteringDisplayMetadata) because
>>>>>> av_mastering_display_metadata_alloc() is not useful.
>>>>>>
>>>>>
>>>>> The API proposed by me is supposed to make API like
>>>>> av_mastering_display_metadata_alloc_size() redundant and therefore these
>>>>> two additions are not orthogonal.
>>>>
>>>> Just because there's a frame side data type for MDM does not make the
>>>> new alloc function redundant.
>>>
>>> The commit message says:
>>>
>>>> av_mastering_display_metadata_alloc() is not useful in scenarios where you need to
>>>> know the runtime size of AVMasteringDisplayMetadata.
>>>
>>> In what scenarios besides side data do you need to know the size of this
>>> struct?
>>
>> None within our libraries that i can think of, but library users can
>> have scenarios we need to provide for. MDM is a standalone API, so lets
>> not try to make its usability depend on a separate one.
>> I'm replacing a helper with a better one, it should not be so controversial.
> 
> Breaking API to benefit hypothetical use cases IS a controversial change
> in my view.

Then I can leave the old on in place. But don't insist on telling people 
to use a frame side data specific alloc function if they want a MDM 
struct, because said function will not be usable for all side data types 
anyway (see video enc params, iamf params), so it's just making things 
more complex for the user for no reason.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH 5/6 v2] avcodec/decode: make the AVFrameSideData helper wrappers not depend on frames

[James Almer]

They will be useful to fill arrays stored in other structs.

Signed-off-by: James Almer <jamrial@gmail.com>
---
 libavcodec/av1dec.c     |  7 +--
 libavcodec/cri.c        |  3 +-
 libavcodec/decode.c     | 97 ++++++++++++++++++++++++++---------------
 libavcodec/decode.h     | 28 ++++++------
 libavcodec/dpx.c        |  3 +-
 libavcodec/h2645_sei.c  |  4 +-
 libavcodec/h264_slice.c |  3 +-
 libavcodec/hevcdec.c    |  6 ++-
 libavcodec/libdav1d.c   |  7 +--
 libavcodec/libjxldec.c  |  3 +-
 libavcodec/mjpegdec.c   |  3 +-
 libavcodec/mpeg12dec.c  | 11 +++--
 libavcodec/pngdec.c     |  8 ++--
 libavcodec/qsvdec.c     |  4 +-
 libavcodec/tiff.c       |  3 +-
 libavcodec/webp.c       |  3 +-
 16 files changed, 117 insertions(+), 76 deletions(-)

diff --git a/libavcodec/av1dec.c b/libavcodec/av1dec.c
index 32a795e758..54bedc27e1 100644
--- a/libavcodec/av1dec.c
+++ b/libavcodec/av1dec.c
@@ -964,7 +964,8 @@ static int export_itut_t35(AVCodecContext *avctx, AVFrame *frame,
             if (!ret)
                 break;
 
-            ret = ff_frame_new_side_data_from_buf(avctx, frame, AV_FRAME_DATA_A53_CC, &buf, NULL);
+            ret = ff_frame_new_side_data_from_buf(avctx, &frame->side_data, &frame->nb_side_data,
+                                                  AV_FRAME_DATA_A53_CC, &buf);
             if (ret < 0)
                 return ret;
 
@@ -1028,7 +1029,7 @@ static int export_metadata(AVCodecContext *avctx, AVFrame *frame)
     if (s->mdcv) {
         AVMasteringDisplayMetadata *mastering;
 
-        ret = ff_decode_mastering_display_new(avctx, frame, &mastering);
+        ret = ff_decode_mastering_display_new(avctx, &frame->side_data, &frame->nb_side_data, &mastering);
         if (ret < 0)
             return ret;
 
@@ -1051,7 +1052,7 @@ static int export_metadata(AVCodecContext *avctx, AVFrame *frame)
     if (s->cll) {
         AVContentLightMetadata *light;
 
-        ret = ff_decode_content_light_new(avctx, frame, &light);
+        ret = ff_decode_content_light_new(avctx, &frame->side_data, &frame->nb_side_data, &light);
         if (ret < 0)
             return ret;
 
diff --git a/libavcodec/cri.c b/libavcodec/cri.c
index 990e52ac99..94468e7515 100644
--- a/libavcodec/cri.c
+++ b/libavcodec/cri.c
@@ -398,7 +398,8 @@ skip:
     }
 
     if (hflip || vflip) {
-        ff_frame_new_side_data(avctx, p, AV_FRAME_DATA_DISPLAYMATRIX,
+        ff_frame_new_side_data(avctx, &p->side_data, &p->nb_side_data,
+                               AV_FRAME_DATA_DISPLAYMATRIX,
                                sizeof(int32_t) * 9, &rotation);
         if (rotation) {
             av_display_rotation_set((int32_t *)rotation->data, 0.f);
diff --git a/libavcodec/decode.c b/libavcodec/decode.c
index 34bcb7cc64..ca7519d3c2 100644
--- a/libavcodec/decode.c
+++ b/libavcodec/decode.c
@@ -1799,87 +1799,114 @@ int ff_decode_preinit(AVCodecContext *avctx)
  * @retval 0 side data of this type can be added to frame
  * @retval 1 side data of this type should not be added to frame
  */
-static int side_data_pref(const AVCodecContext *avctx, AVFrame *frame,
-                          enum AVFrameSideDataType type)
+static int side_data_pref(const AVCodecContext *avctx, AVFrameSideData ***sd,
+                          int *nb_sd, enum AVFrameSideDataType type)
 {
     DecodeContext *dc = decode_ctx(avctx->internal);
 
     // Note: could be skipped for `type` without corresponding packet sd
-    if (av_frame_get_side_data(frame, type)) {
+    if (av_frame_side_data_get(*sd, *nb_sd, type)) {
         if (dc->side_data_pref_mask & (1ULL << type))
             return 1;
-        av_frame_remove_side_data(frame, type);
+        av_frame_side_data_remove(sd, nb_sd, type);
     }
 
     return 0;
 }
 
-
-int ff_frame_new_side_data(const AVCodecContext *avctx, AVFrame *frame,
-                           enum AVFrameSideDataType type, size_t size,
+int ff_frame_new_side_data(const AVCodecContext *avctx, AVFrameSideData ***sd,
+                           int *nb_sd, enum AVFrameSideDataType type, size_t size,
                            AVFrameSideData **psd)
 {
-    AVFrameSideData *sd;
-
-    if (side_data_pref(avctx, frame, type)) {
+    if (side_data_pref(avctx, sd, nb_sd, type)) {
         if (psd)
             *psd = NULL;
         return 0;
     }
 
-    sd = av_frame_new_side_data(frame, type, size);
-    if (psd)
-        *psd = sd;
+    *psd = av_frame_side_data_new(sd, nb_sd, type, size, 0);
 
-    return sd ? 0 : AVERROR(ENOMEM);
+    return *psd ? 0 : AVERROR(ENOMEM);
 }
 
-int ff_frame_new_side_data_from_buf(const AVCodecContext *avctx,
-                                    AVFrame *frame, enum AVFrameSideDataType type,
-                                    AVBufferRef **buf, AVFrameSideData **psd)
+int ff_frame_new_side_data_from_buf(const AVCodecContext *avctx, AVFrameSideData ***sd,
+                                    int *nb_sd, enum AVFrameSideDataType type,
+                                    AVBufferRef **buf)
 {
-    AVFrameSideData *sd = NULL;
     int ret = 0;
 
-    if (side_data_pref(avctx, frame, type))
+    if (side_data_pref(avctx, sd, nb_sd, type))
         goto finish;
 
-    sd = av_frame_new_side_data_from_buf(frame, type, *buf);
-    if (sd)
-        *buf = NULL;
-    else
+    if (!av_frame_side_data_add(sd, nb_sd, type, buf, 0))
         ret = AVERROR(ENOMEM);
 
 finish:
     av_buffer_unref(buf);
-    if (psd)
-        *psd = sd;
 
     return ret;
 }
 
-int ff_decode_mastering_display_new(const AVCodecContext *avctx, AVFrame *frame,
-                                    AVMasteringDisplayMetadata **mdm)
+int ff_decode_mastering_display_new(const AVCodecContext *avctx, AVFrameSideData ***sd,
+                                    int *nb_sd, AVMasteringDisplayMetadata **mdm)
 {
-    if (side_data_pref(avctx, frame, AV_FRAME_DATA_MASTERING_DISPLAY_METADATA)) {
+    AVBufferRef *buf;
+    size_t size;
+
+    if (side_data_pref(avctx, sd, nb_sd, AV_FRAME_DATA_MASTERING_DISPLAY_METADATA)) {
         *mdm = NULL;
         return 0;
     }
 
-    *mdm = av_mastering_display_metadata_create_side_data(frame);
-    return *mdm ? 0 : AVERROR(ENOMEM);
+    *mdm = av_mastering_display_metadata_alloc_size(&size);
+    if (!*mdm)
+        return AVERROR(ENOMEM);
+
+    buf = av_buffer_create((uint8_t *)*mdm, size, NULL, NULL, 0);
+    if (!buf) {
+        av_freep(mdm);
+        return AVERROR(ENOMEM);
+    }
+
+    if (!av_frame_side_data_add(sd, nb_sd, AV_FRAME_DATA_MASTERING_DISPLAY_METADATA,
+                                &buf, AV_FRAME_SIDE_DATA_FLAG_UNIQUE)) {
+        *mdm = NULL;
+        av_buffer_unref(&buf);
+        return AVERROR(ENOMEM);
+    }
+
+    return 0;
 }
 
-int ff_decode_content_light_new(const AVCodecContext *avctx, AVFrame *frame,
-                                AVContentLightMetadata **clm)
+int ff_decode_content_light_new(const AVCodecContext *avctx, AVFrameSideData ***sd,
+                                int *nb_sd, AVContentLightMetadata **clm)
 {
-    if (side_data_pref(avctx, frame, AV_FRAME_DATA_CONTENT_LIGHT_LEVEL)) {
+    AVBufferRef *buf;
+    size_t size;
+
+    if (side_data_pref(avctx, sd, nb_sd, AV_FRAME_DATA_CONTENT_LIGHT_LEVEL)) {
         *clm = NULL;
         return 0;
     }
 
-    *clm = av_content_light_metadata_create_side_data(frame);
-    return *clm ? 0 : AVERROR(ENOMEM);
+    *clm = av_content_light_metadata_alloc(&size);
+    if (!*clm)
+        return AVERROR(ENOMEM);
+
+    buf = av_buffer_create((uint8_t *)*clm, size, NULL, NULL, 0);
+    if (!buf) {
+        av_freep(clm);
+        return AVERROR(ENOMEM);
+    }
+
+    if (!av_frame_side_data_add(sd, nb_sd, AV_FRAME_DATA_CONTENT_LIGHT_LEVEL,
+                                &buf, AV_FRAME_SIDE_DATA_FLAG_UNIQUE)) {
+        *clm = NULL;
+        av_buffer_unref(&buf);
+        return AVERROR(ENOMEM);
+    }
+
+    return 0;
 }
 
 int ff_copy_palette(void *dst, const AVPacket *src, void *logctx)
diff --git a/libavcodec/decode.h b/libavcodec/decode.h
index 4ffbd9db8e..1026c9dae9 100644
--- a/libavcodec/decode.h
+++ b/libavcodec/decode.h
@@ -156,14 +156,14 @@ const AVPacketSideData *ff_get_coded_side_data(const AVCodecContext *avctx,
                                                enum AVPacketSideDataType type);
 
 /**
- * Wrapper around av_frame_new_side_data, which rejects side data overridden by
+ * Wrapper around av_frame_side_data_new, which rejects side data overridden by
  * the demuxer. Returns 0 on success, and a negative error code otherwise.
- * If successful and sd is not NULL, *sd may either contain a pointer to the new
+ * If successful and psd is not NULL, *psd may either contain a pointer to the new
  * side data, or NULL in case the side data was already present.
  */
-int ff_frame_new_side_data(const AVCodecContext *avctx, AVFrame *frame,
-                           enum AVFrameSideDataType type, size_t size,
-                           AVFrameSideData **sd);
+int ff_frame_new_side_data(const AVCodecContext *avctx, AVFrameSideData ***sd,
+                           int *nb_sd, enum AVFrameSideDataType type, size_t size,
+                           AVFrameSideData **psd);
 
 /**
  * Similar to `ff_frame_new_side_data`, but using an existing buffer ref.
@@ -171,29 +171,29 @@ int ff_frame_new_side_data(const AVCodecContext *avctx, AVFrame *frame,
  * *buf is ALWAYS consumed by this function and NULL written in its place, even
  * on failure.
  */
-int ff_frame_new_side_data_from_buf(const AVCodecContext *avctx,
-                                    AVFrame *frame, enum AVFrameSideDataType type,
-                                    AVBufferRef **buf, AVFrameSideData **sd);
+int ff_frame_new_side_data_from_buf(const AVCodecContext *avctx, AVFrameSideData ***sd,
+                                    int *nb_sd, enum AVFrameSideDataType type,
+                                    AVBufferRef **buf);
 
 struct AVMasteringDisplayMetadata;
 struct AVContentLightMetadata;
 
 /**
- * Wrapper around av_mastering_display_metadata_create_side_data(), which
+ * Similar to av_mastering_display_metadata_create_side_data(), but
  * rejects side data overridden by the demuxer. Returns 0 on success, and a
  * negative error code otherwise. If successful, *mdm may either be a pointer to
  * the new side data, or NULL in case the side data was already present.
  */
-int ff_decode_mastering_display_new(const AVCodecContext *avctx, AVFrame *frame,
-                                    struct AVMasteringDisplayMetadata **mdm);
+int ff_decode_mastering_display_new(const AVCodecContext *avctx, AVFrameSideData ***sd,
+                                    int *nb_sd, struct AVMasteringDisplayMetadata **mdm);
 
 /**
- * Wrapper around av_content_light_metadata_create_side_data(), which
+ * Similar to av_content_light_metadata_create_side_data(), but
  * rejects side data overridden by the demuxer. Returns 0 on success, and a
  * negative error code otherwise. If successful, *clm may either be a pointer to
  * the new side data, or NULL in case the side data was already present.
  */
-int ff_decode_content_light_new(const AVCodecContext *avctx, AVFrame *frame,
-                                struct AVContentLightMetadata **clm);
+int ff_decode_content_light_new(const AVCodecContext *avctx, AVFrameSideData ***sd,
+                                int *nb_sd, struct AVContentLightMetadata **clm);
 
 #endif /* AVCODEC_DECODE_H */
diff --git a/libavcodec/dpx.c b/libavcodec/dpx.c
index 80616d98a2..ff9115bf86 100644
--- a/libavcodec/dpx.c
+++ b/libavcodec/dpx.c
@@ -288,7 +288,8 @@ static int decode_frame(AVCodecContext *avctx, AVFrame *p,
 
         if (i != 0xFFFFFFFF) {
             AVFrameSideData *tcside;
-            ret = ff_frame_new_side_data(avctx, p, AV_FRAME_DATA_S12M_TIMECODE,
+            ret = ff_frame_new_side_data(avctx, &p->side_data, &p->nb_side_data,
+                                         AV_FRAME_DATA_S12M_TIMECODE,
                                          sizeof(uint32_t) * 4, &tcside);
             if (ret < 0)
                 return ret;
diff --git a/libavcodec/h2645_sei.c b/libavcodec/h2645_sei.c
index afc103b69c..a9f4e01de8 100644
--- a/libavcodec/h2645_sei.c
+++ b/libavcodec/h2645_sei.c
@@ -750,7 +750,7 @@ FF_ENABLE_DEPRECATION_WARNINGS
         int i;
         AVMasteringDisplayMetadata *metadata;
 
-        ret = ff_decode_mastering_display_new(avctx, frame, &metadata);
+        ret = ff_decode_mastering_display_new(avctx, &frame->side_data, &frame->nb_side_data, &metadata);
         if (ret < 0)
             return ret;
 
@@ -793,7 +793,7 @@ FF_ENABLE_DEPRECATION_WARNINGS
     if (sei->content_light.present) {
         AVContentLightMetadata *metadata;
 
-        ret = ff_decode_content_light_new(avctx, frame, &metadata);
+        ret = ff_decode_content_light_new(avctx, &frame->side_data, &frame->nb_side_data, &metadata);
         if (ret < 0)
             return ret;
 
diff --git a/libavcodec/h264_slice.c b/libavcodec/h264_slice.c
index e9a404e41b..6d9d2ddc14 100644
--- a/libavcodec/h264_slice.c
+++ b/libavcodec/h264_slice.c
@@ -1253,7 +1253,8 @@ static int h264_export_frame_props(H264Context *h)
         uint32_t *tc_sd;
         char tcbuf[AV_TIMECODE_STR_SIZE];
         AVFrameSideData *tcside;
-        ret = ff_frame_new_side_data(h->avctx, out, AV_FRAME_DATA_S12M_TIMECODE,
+        ret = ff_frame_new_side_data(h->avctx, &out->side_data, &out->nb_side_data,
+                                     AV_FRAME_DATA_S12M_TIMECODE,
                                      sizeof(uint32_t)*4, &tcside);
         if (ret < 0)
             return ret;
diff --git a/libavcodec/hevcdec.c b/libavcodec/hevcdec.c
index 575836e340..d744e6f598 100644
--- a/libavcodec/hevcdec.c
+++ b/libavcodec/hevcdec.c
@@ -2791,7 +2791,8 @@ static int set_side_data(HEVCContext *s)
         uint32_t *tc_sd;
         char tcbuf[AV_TIMECODE_STR_SIZE];
         AVFrameSideData *tcside;
-        ret = ff_frame_new_side_data(s->avctx, out, AV_FRAME_DATA_S12M_TIMECODE,
+        ret = ff_frame_new_side_data(s->avctx, &out->side_data, &out->nb_side_data,
+                                     AV_FRAME_DATA_S12M_TIMECODE,
                                      sizeof(uint32_t) * 4, &tcside);
         if (ret < 0)
             return ret;
@@ -2821,7 +2822,8 @@ static int set_side_data(HEVCContext *s)
         if (!info_ref)
             return AVERROR(ENOMEM);
 
-        ret = ff_frame_new_side_data_from_buf(s->avctx, out, AV_FRAME_DATA_DYNAMIC_HDR_PLUS, &info_ref, NULL);
+        ret = ff_frame_new_side_data_from_buf(s->avctx, &out->side_data, &out->nb_side_data,
+                                              AV_FRAME_DATA_DYNAMIC_HDR_PLUS, &info_ref);
         if (ret < 0)
             return ret;
     }
diff --git a/libavcodec/libdav1d.c b/libavcodec/libdav1d.c
index ddcd0708b4..465c56e757 100644
--- a/libavcodec/libdav1d.c
+++ b/libavcodec/libdav1d.c
@@ -471,7 +471,7 @@ static int libdav1d_receive_frame(AVCodecContext *c, AVFrame *frame)
     if (p->mastering_display) {
         AVMasteringDisplayMetadata *mastering;
 
-        res = ff_decode_mastering_display_new(c, frame, &mastering);
+        res = ff_decode_mastering_display_new(c, &frame->side_data, &frame->nb_side_data, &mastering);
         if (res < 0)
             goto fail;
 
@@ -493,7 +493,7 @@ static int libdav1d_receive_frame(AVCodecContext *c, AVFrame *frame)
     if (p->content_light) {
         AVContentLightMetadata *light;
 
-        res = ff_decode_content_light_new(c, frame, &light);
+        res = ff_decode_content_light_new(c, &frame->side_data, &frame->nb_side_data, &light);
         if (res < 0)
             goto fail;
 
@@ -528,7 +528,8 @@ static int libdav1d_receive_frame(AVCodecContext *c, AVFrame *frame)
                 if (!res)
                     break;
 
-                res = ff_frame_new_side_data_from_buf(c, frame, AV_FRAME_DATA_A53_CC, &buf, NULL);
+                res = ff_frame_new_side_data_from_buf(c, &frame->side_data, &frame->nb_side_data,
+                                                      AV_FRAME_DATA_A53_CC, &buf);
                 if (res < 0)
                     goto fail;
 
diff --git a/libavcodec/libjxldec.c b/libavcodec/libjxldec.c
index d57a27418f..6e95c79177 100644
--- a/libavcodec/libjxldec.c
+++ b/libavcodec/libjxldec.c
@@ -483,7 +483,8 @@ static int libjxl_receive_frame(AVCodecContext *avctx, AVFrame *frame)
             /* full image is one frame, even if animated */
             av_log(avctx, AV_LOG_DEBUG, "FULL_IMAGE event emitted\n");
             if (ctx->iccp) {
-                ret = ff_frame_new_side_data_from_buf(avctx, ctx->frame, AV_FRAME_DATA_ICC_PROFILE, &ctx->iccp, NULL);
+                ret = ff_frame_new_side_data_from_buf(avctx, &ctx->frame->side_data, &ctx->frame->nb_side_data,
+                                                      AV_FRAME_DATA_ICC_PROFILE, &ctx->iccp);
                 if (ret < 0)
                     return ret;
             }
diff --git a/libavcodec/mjpegdec.c b/libavcodec/mjpegdec.c
index c9409eac6c..89443dc4cd 100644
--- a/libavcodec/mjpegdec.c
+++ b/libavcodec/mjpegdec.c
@@ -2839,7 +2839,8 @@ the_end:
         for (i = 0; i < s->iccnum; i++)
             total_size += s->iccentries[i].length;
 
-        ret = ff_frame_new_side_data(avctx, frame, AV_FRAME_DATA_ICC_PROFILE, total_size, &sd);
+        ret = ff_frame_new_side_data(avctx, &frame->side_data, &frame->nb_side_data,
+                                     AV_FRAME_DATA_ICC_PROFILE, total_size, &sd);
         if (ret < 0) {
             av_log(avctx, AV_LOG_ERROR, "Could not allocate frame side data\n");
             return ret;
diff --git a/libavcodec/mpeg12dec.c b/libavcodec/mpeg12dec.c
index 4ad1eb6572..2665ee2f95 100644
--- a/libavcodec/mpeg12dec.c
+++ b/libavcodec/mpeg12dec.c
@@ -1314,7 +1314,8 @@ static int mpeg_field_start(MpegEncContext *s, const uint8_t *buf, int buf_size)
             }
         }
 
-        ret = ff_frame_new_side_data(s->avctx, s->current_picture_ptr->f,
+        ret = ff_frame_new_side_data(s->avctx, &s->current_picture_ptr->f->side_data,
+                                     &s->current_picture_ptr->f->nb_side_data,
                                      AV_FRAME_DATA_PANSCAN, sizeof(s1->pan_scan),
                                      &pan_scan);
         if (ret < 0)
@@ -1324,8 +1325,9 @@ static int mpeg_field_start(MpegEncContext *s, const uint8_t *buf, int buf_size)
 
         if (s1->a53_buf_ref) {
             ret = ff_frame_new_side_data_from_buf(
-                s->avctx, s->current_picture_ptr->f, AV_FRAME_DATA_A53_CC,
-                &s1->a53_buf_ref, NULL);
+                 s->avctx, &s->current_picture_ptr->f->side_data,
+                 &s->current_picture_ptr->f->nb_side_data,
+                 AV_FRAME_DATA_A53_CC, &s1->a53_buf_ref);
             if (ret < 0)
                 return ret;
         }
@@ -1341,7 +1343,8 @@ static int mpeg_field_start(MpegEncContext *s, const uint8_t *buf, int buf_size)
 
         if (s1->has_afd) {
             AVFrameSideData *sd;
-            ret = ff_frame_new_side_data(s->avctx, s->current_picture_ptr->f,
+            ret = ff_frame_new_side_data(s->avctx, &s->current_picture_ptr->f->side_data,
+                                         &s->current_picture_ptr->f->nb_side_data,
                                          AV_FRAME_DATA_AFD, 1, &sd);
             if (ret < 0)
                 return ret;
diff --git a/libavcodec/pngdec.c b/libavcodec/pngdec.c
index de50e6a5b6..68c25cb5bd 100644
--- a/libavcodec/pngdec.c
+++ b/libavcodec/pngdec.c
@@ -680,8 +680,8 @@ static int populate_avctx_color_fields(AVCodecContext *avctx, AVFrame *frame)
         }
     } else if (s->iccp_data) {
         AVFrameSideData *sd;
-        ret = ff_frame_new_side_data(avctx, frame, AV_FRAME_DATA_ICC_PROFILE,
-                                     s->iccp_data_len, &sd);
+        ret = ff_frame_new_side_data(avctx, &frame->side_data, &frame->nb_side_data,
+                                     AV_FRAME_DATA_ICC_PROFILE, s->iccp_data_len, &sd);
         if (ret < 0)
             return ret;
         if (sd) {
@@ -748,7 +748,7 @@ static int populate_avctx_color_fields(AVCodecContext *avctx, AVFrame *frame)
     if (s->have_clli) {
         AVContentLightMetadata *clli;
 
-        ret = ff_decode_content_light_new(avctx, frame, &clli);
+        ret = ff_decode_content_light_new(avctx, &frame->side_data, &frame->nb_side_data, &clli);
         if (ret < 0)
             return ret;
 
@@ -765,7 +765,7 @@ static int populate_avctx_color_fields(AVCodecContext *avctx, AVFrame *frame)
     if (s->have_mdvc) {
         AVMasteringDisplayMetadata *mdvc;
 
-        ret = ff_decode_mastering_display_new(avctx, frame, &mdvc);
+        ret = ff_decode_mastering_display_new(avctx, &frame->side_data, &frame->nb_side_data, &mdvc);
         if (ret < 0)
             return ret;
 
diff --git a/libavcodec/qsvdec.c b/libavcodec/qsvdec.c
index fd9267c6f4..de3ed3c357 100644
--- a/libavcodec/qsvdec.c
+++ b/libavcodec/qsvdec.c
@@ -661,7 +661,7 @@ static int qsv_export_hdr_side_data(AVCodecContext *avctx, mfxExtMasteringDispla
         const int luma_den = 10000;
         int i;
 
-        ret = ff_decode_mastering_display_new(avctx, frame, &mastering);
+        ret = ff_decode_mastering_display_new(avctx, &frame->side_data, &frame->nb_side_data, &mastering);
         if (ret < 0)
             return ret;
 
@@ -687,7 +687,7 @@ static int qsv_export_hdr_side_data(AVCodecContext *avctx, mfxExtMasteringDispla
     if (clli->InsertPayloadToggle) {
         AVContentLightMetadata *light;
 
-        ret = ff_decode_content_light_new(avctx, frame, &light);
+        ret = ff_decode_content_light_new(avctx, &frame->side_data, &frame->nb_side_data, &light);
         if (ret < 0)
             return ret;
 
diff --git a/libavcodec/tiff.c b/libavcodec/tiff.c
index 7ce1ab32f6..9cde2e60e1 100644
--- a/libavcodec/tiff.c
+++ b/libavcodec/tiff.c
@@ -1706,7 +1706,8 @@ static int tiff_decode_tag(TiffContext *s, AVFrame *frame)
         if (bytestream2_get_bytes_left(&gb_temp) < count)
             return AVERROR_INVALIDDATA;
 
-        ret = ff_frame_new_side_data(s->avctx, frame, AV_FRAME_DATA_ICC_PROFILE, count, &sd);
+        ret = ff_frame_new_side_data(s->avctx, &frame->side_data, &frame->nb_side_data,
+                                     AV_FRAME_DATA_ICC_PROFILE, count, &sd);
         if (ret < 0)
             return ret;
         if (sd)
diff --git a/libavcodec/webp.c b/libavcodec/webp.c
index 9308ea2b69..ec23533ef2 100644
--- a/libavcodec/webp.c
+++ b/libavcodec/webp.c
@@ -1501,7 +1501,8 @@ exif_end:
 
             s->has_iccp = 1;
 
-            ret = ff_frame_new_side_data(avctx, p, AV_FRAME_DATA_ICC_PROFILE, chunk_size, &sd);
+            ret = ff_frame_new_side_data(avctx, &p->side_data, &p->nb_side_data,
+                                         AV_FRAME_DATA_ICC_PROFILE, chunk_size, &sd);
             if (ret < 0)
                 return ret;
 
-- 
2.44.0

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH 6/6 v2] avcodec/hevcdec: export global side data in AVCodecContext

[James Almer]

Signed-off-by: James Almer <jamrial@gmail.com>
---
 libavcodec/avcodec.h       |   2 +-
 libavcodec/h2645_sei.c     | 217 ++++++++++++++++++++++---------------
 libavcodec/h2645_sei.h     |   2 +
 libavcodec/hevcdec.c       |   4 +
 libavcodec/pthread_frame.c |  10 ++
 5 files changed, 146 insertions(+), 89 deletions(-)

diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
index 83dc487251..968009a192 100644
--- a/libavcodec/avcodec.h
+++ b/libavcodec/avcodec.h
@@ -2071,7 +2071,7 @@ typedef struct AVCodecContext {
      * - encoding: may be set by user before calling avcodec_open2() for
      *             encoder configuration. Afterwards owned and freed by the
      *             encoder.
-     * - decoding: unused
+     * - decoding: may be set by libavcodec in avcodec_open2().
      */
     AVFrameSideData  **decoded_side_data;
     int             nb_decoded_side_data;
diff --git a/libavcodec/h2645_sei.c b/libavcodec/h2645_sei.c
index a9f4e01de8..7c11d7e449 100644
--- a/libavcodec/h2645_sei.c
+++ b/libavcodec/h2645_sei.c
@@ -529,6 +529,124 @@ static int is_frame_packing_type_valid(SEIFpaType type, enum AVCodecID codec_id)
                type >= SEI_FPA_TYPE_SIDE_BY_SIDE;
 }
 
+static int h2645_sei_to_side_data(AVCodecContext *avctx, H2645SEI *sei,
+                                  AVFrameSideData ***sd, int *nb_sd)
+{
+    int ret;
+
+    for (unsigned i = 0; i < sei->unregistered.nb_buf_ref; i++) {
+        H2645SEIUnregistered *unreg = &sei->unregistered;
+        AVBufferRef *buf;
+
+        if (!unreg->buf_ref[i])
+            continue;
+
+        buf = av_buffer_ref(unreg->buf_ref[i]);
+        if (!buf)
+            return AVERROR(ENOMEM);
+
+        if (!av_frame_side_data_add(sd, nb_sd, AV_FRAME_DATA_SEI_UNREGISTERED, &buf, 0)) {
+            av_buffer_unref(&buf);
+            return AVERROR(ENOMEM);
+        }
+    }
+
+    if (sei->ambient_viewing_environment.present) {
+        H2645SEIAmbientViewingEnvironment *env =
+            &sei->ambient_viewing_environment;
+        AVBufferRef *buf;
+        size_t size;
+
+        AVAmbientViewingEnvironment *dst_env =
+            av_ambient_viewing_environment_alloc(&size);
+        if (!dst_env)
+            return AVERROR(ENOMEM);
+
+        buf = av_buffer_create((uint8_t *)dst_env, size, NULL, NULL, 0);
+        if (!buf) {
+            av_free(dst_env);
+            return AVERROR(ENOMEM);
+        }
+
+        ret = ff_frame_new_side_data_from_buf(avctx, sd, nb_sd,
+                                              AV_FRAME_DATA_AMBIENT_VIEWING_ENVIRONMENT, &buf);
+
+        if (ret < 0)
+            return ret;
+
+        dst_env->ambient_illuminance = av_make_q(env->ambient_illuminance, 10000);
+        dst_env->ambient_light_x     = av_make_q(env->ambient_light_x,     50000);
+        dst_env->ambient_light_y     = av_make_q(env->ambient_light_y,     50000);
+    }
+
+    if (sei->mastering_display.present) {
+        // HEVC uses a g,b,r ordering, which we convert to a more natural r,g,b
+        const int mapping[3] = {2, 0, 1};
+        const int chroma_den = 50000;
+        const int luma_den = 10000;
+        int i;
+        AVMasteringDisplayMetadata *metadata;
+
+        ret = ff_decode_mastering_display_new(avctx, sd, nb_sd, &metadata);
+        if (ret < 0)
+            return ret;
+
+        if (metadata) {
+            for (i = 0; i < 3; i++) {
+                const int j = mapping[i];
+                metadata->display_primaries[i][0].num = sei->mastering_display.display_primaries[j][0];
+                metadata->display_primaries[i][0].den = chroma_den;
+                metadata->display_primaries[i][1].num = sei->mastering_display.display_primaries[j][1];
+                metadata->display_primaries[i][1].den = chroma_den;
+            }
+            metadata->white_point[0].num = sei->mastering_display.white_point[0];
+            metadata->white_point[0].den = chroma_den;
+            metadata->white_point[1].num = sei->mastering_display.white_point[1];
+            metadata->white_point[1].den = chroma_den;
+
+            metadata->max_luminance.num = sei->mastering_display.max_luminance;
+            metadata->max_luminance.den = luma_den;
+            metadata->min_luminance.num = sei->mastering_display.min_luminance;
+            metadata->min_luminance.den = luma_den;
+            metadata->has_luminance = 1;
+            metadata->has_primaries = 1;
+
+            av_log(avctx, AV_LOG_DEBUG, "Mastering Display Metadata:\n");
+            av_log(avctx, AV_LOG_DEBUG,
+                   "r(%5.4f,%5.4f) g(%5.4f,%5.4f) b(%5.4f %5.4f) wp(%5.4f, %5.4f)\n",
+                   av_q2d(metadata->display_primaries[0][0]),
+                   av_q2d(metadata->display_primaries[0][1]),
+                   av_q2d(metadata->display_primaries[1][0]),
+                   av_q2d(metadata->display_primaries[1][1]),
+                   av_q2d(metadata->display_primaries[2][0]),
+                   av_q2d(metadata->display_primaries[2][1]),
+                   av_q2d(metadata->white_point[0]), av_q2d(metadata->white_point[1]));
+            av_log(avctx, AV_LOG_DEBUG,
+                   "min_luminance=%f, max_luminance=%f\n",
+                   av_q2d(metadata->min_luminance), av_q2d(metadata->max_luminance));
+        }
+    }
+
+    if (sei->content_light.present) {
+        AVContentLightMetadata *metadata;
+
+        ret = ff_decode_content_light_new(avctx, sd, nb_sd, &metadata);
+        if (ret < 0)
+            return ret;
+
+        if (metadata) {
+            metadata->MaxCLL  = sei->content_light.max_content_light_level;
+            metadata->MaxFALL = sei->content_light.max_pic_average_light_level;
+
+            av_log(avctx, AV_LOG_DEBUG, "Content Light Level Metadata:\n");
+            av_log(avctx, AV_LOG_DEBUG, "MaxCLL=%d, MaxFALL=%d\n",
+                   metadata->MaxCLL, metadata->MaxFALL);
+        }
+    }
+
+    return 0;
+}
+
 int ff_h2645_sei_to_frame(AVFrame *frame, H2645SEI *sei,
                           enum AVCodecID codec_id,
                           AVCodecContext *avctx, const H2645VUI *vui,
@@ -625,17 +743,13 @@ int ff_h2645_sei_to_frame(AVFrame *frame, H2645SEI *sei,
             avctx->properties |= FF_CODEC_PROPERTY_CLOSED_CAPTIONS;
     }
 
+    ret = h2645_sei_to_side_data(avctx, sei, &frame->side_data, &frame->nb_side_data);
+    if (ret < 0)
+        return ret;
+
     for (unsigned i = 0; i < sei->unregistered.nb_buf_ref; i++) {
         H2645SEIUnregistered *unreg = &sei->unregistered;
-
-        if (unreg->buf_ref[i]) {
-            AVFrameSideData *sd = av_frame_new_side_data_from_buf(frame,
-                    AV_FRAME_DATA_SEI_UNREGISTERED,
-                    unreg->buf_ref[i]);
-            if (!sd)
-                av_buffer_unref(&unreg->buf_ref[i]);
-            unreg->buf_ref[i] = NULL;
-        }
+        av_buffer_unref(&unreg->buf_ref[i]);
     }
     sei->unregistered.nb_buf_ref = 0;
 
@@ -728,88 +842,15 @@ FF_ENABLE_DEPRECATION_WARNINGS
         return ret;
 #endif
 
-    if (sei->ambient_viewing_environment.present) {
-        H2645SEIAmbientViewingEnvironment *env =
-            &sei->ambient_viewing_environment;
-
-        AVAmbientViewingEnvironment *dst_env =
-            av_ambient_viewing_environment_create_side_data(frame);
-        if (!dst_env)
-            return AVERROR(ENOMEM);
-
-        dst_env->ambient_illuminance = av_make_q(env->ambient_illuminance, 10000);
-        dst_env->ambient_light_x     = av_make_q(env->ambient_light_x,     50000);
-        dst_env->ambient_light_y     = av_make_q(env->ambient_light_y,     50000);
-    }
-
-    if (sei->mastering_display.present) {
-        // HEVC uses a g,b,r ordering, which we convert to a more natural r,g,b
-        const int mapping[3] = {2, 0, 1};
-        const int chroma_den = 50000;
-        const int luma_den = 10000;
-        int i;
-        AVMasteringDisplayMetadata *metadata;
-
-        ret = ff_decode_mastering_display_new(avctx, &frame->side_data, &frame->nb_side_data, &metadata);
-        if (ret < 0)
-            return ret;
-
-        if (metadata) {
-            for (i = 0; i < 3; i++) {
-                const int j = mapping[i];
-                metadata->display_primaries[i][0].num = sei->mastering_display.display_primaries[j][0];
-                metadata->display_primaries[i][0].den = chroma_den;
-                metadata->display_primaries[i][1].num = sei->mastering_display.display_primaries[j][1];
-                metadata->display_primaries[i][1].den = chroma_den;
-            }
-            metadata->white_point[0].num = sei->mastering_display.white_point[0];
-            metadata->white_point[0].den = chroma_den;
-            metadata->white_point[1].num = sei->mastering_display.white_point[1];
-            metadata->white_point[1].den = chroma_den;
-
-            metadata->max_luminance.num = sei->mastering_display.max_luminance;
-            metadata->max_luminance.den = luma_den;
-            metadata->min_luminance.num = sei->mastering_display.min_luminance;
-            metadata->min_luminance.den = luma_den;
-            metadata->has_luminance = 1;
-            metadata->has_primaries = 1;
-
-            av_log(avctx, AV_LOG_DEBUG, "Mastering Display Metadata:\n");
-            av_log(avctx, AV_LOG_DEBUG,
-                   "r(%5.4f,%5.4f) g(%5.4f,%5.4f) b(%5.4f %5.4f) wp(%5.4f, %5.4f)\n",
-                   av_q2d(metadata->display_primaries[0][0]),
-                   av_q2d(metadata->display_primaries[0][1]),
-                   av_q2d(metadata->display_primaries[1][0]),
-                   av_q2d(metadata->display_primaries[1][1]),
-                   av_q2d(metadata->display_primaries[2][0]),
-                   av_q2d(metadata->display_primaries[2][1]),
-                   av_q2d(metadata->white_point[0]), av_q2d(metadata->white_point[1]));
-            av_log(avctx, AV_LOG_DEBUG,
-                   "min_luminance=%f, max_luminance=%f\n",
-                   av_q2d(metadata->min_luminance), av_q2d(metadata->max_luminance));
-        }
-    }
-
-    if (sei->content_light.present) {
-        AVContentLightMetadata *metadata;
-
-        ret = ff_decode_content_light_new(avctx, &frame->side_data, &frame->nb_side_data, &metadata);
-        if (ret < 0)
-            return ret;
-
-        if (metadata) {
-            metadata->MaxCLL  = sei->content_light.max_content_light_level;
-            metadata->MaxFALL = sei->content_light.max_pic_average_light_level;
-
-            av_log(avctx, AV_LOG_DEBUG, "Content Light Level Metadata:\n");
-            av_log(avctx, AV_LOG_DEBUG, "MaxCLL=%d, MaxFALL=%d\n",
-                   metadata->MaxCLL, metadata->MaxFALL);
-        }
-    }
-
     return 0;
 }
 
+int ff_h2645_sei_to_context(AVCodecContext *avctx, H2645SEI *sei)
+{
+    return h2645_sei_to_side_data(avctx, sei, &avctx->decoded_side_data,
+                                  &avctx->nb_decoded_side_data);
+}
+
 void ff_h2645_sei_reset(H2645SEI *s)
 {
     av_buffer_unref(&s->a53_caption.buf_ref);
diff --git a/libavcodec/h2645_sei.h b/libavcodec/h2645_sei.h
index b9a6c7587b..488dbcad7e 100644
--- a/libavcodec/h2645_sei.h
+++ b/libavcodec/h2645_sei.h
@@ -168,4 +168,6 @@ int ff_h2645_sei_to_frame(AVFrame *frame, H2645SEI *sei,
                           unsigned bit_depth_luma, unsigned bit_depth_chroma,
                           int seed);
 
+int ff_h2645_sei_to_context(AVCodecContext *avctx, H2645SEI *sei);
+
 #endif /* AVCODEC_H2645_SEI_H */
diff --git a/libavcodec/hevcdec.c b/libavcodec/hevcdec.c
index d744e6f598..70d22e5976 100644
--- a/libavcodec/hevcdec.c
+++ b/libavcodec/hevcdec.c
@@ -3658,6 +3658,10 @@ static av_cold int hevc_decode_init(AVCodecContext *avctx)
             if (ret < 0) {
                 return ret;
             }
+
+            ret = ff_h2645_sei_to_context(avctx, &s->sei.common);
+            if (ret < 0)
+                return ret;
         }
 
         sd = ff_get_coded_side_data(avctx, AV_PKT_DATA_DOVI_CONF);
diff --git a/libavcodec/pthread_frame.c b/libavcodec/pthread_frame.c
index fd356bd190..bbf8a546dc 100644
--- a/libavcodec/pthread_frame.c
+++ b/libavcodec/pthread_frame.c
@@ -334,6 +334,14 @@ FF_ENABLE_DEPRECATION_WARNINGS
     if (for_user) {
         if (codec->update_thread_context_for_user)
             err = codec->update_thread_context_for_user(dst, src);
+
+        av_frame_side_data_free(&dst->decoded_side_data, &dst->nb_decoded_side_data);
+        for (int i = 0; i < src->nb_decoded_side_data; i++) {
+            int ret = av_frame_side_data_clone(&dst->decoded_side_data, &dst->nb_decoded_side_data,
+                                               src->decoded_side_data[i], AV_FRAME_SIDE_DATA_FLAG_UNIQUE);
+            if (ret < 0)
+                return ret;
+        }
     } else {
         const PerThreadContext *p_src = src->internal->thread_ctx;
         PerThreadContext       *p_dst = dst->internal->thread_ctx;
@@ -767,6 +775,8 @@ static av_cold int init_thread(PerThreadContext *p, int *threads_to_free,
     if (!copy)
         return AVERROR(ENOMEM);
     copy->priv_data = NULL;
+    copy->decoded_side_data = NULL;
+    copy->nb_decoded_side_data = 0;
 
     /* From now on, this PerThreadContext will be cleaned up by
      * ff_frame_thread_free in case of errors. */
-- 
2.44.0

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/6 v2] avutil/frame: add a flag to not create duplicate entries in a side data array

[Anton Khirnov]

Quoting James Almer (2024-03-25 21:05:57)
> +/**
> + * Remove existing entries before adding new ones.
> + */
>  #define AV_FRAME_SIDE_DATA_FLAG_UNIQUE (1 << 0)
> +/**
> + * Don't add a new entry if another of the same type exists.
> + */
> +#define AV_FRAME_SIDE_DATA_FLAG_DONT_APPEND (1 << 1)

I don't really like this API, because it leaves too much work to the
user.

With my descriptor set, we know when it makes sense to have duplicate
side data. So the cases that can occur when adding side data of type T
are:
* T not present, call succeeds
* T present, then
    * T does not have a MULTI prop, then user decides whether to
      replace or do nothing (or perhaps fail)
    * T does have a MULTI prop, then user decides whether to replace,
      add, or do nothing

I think the default behaviour for MULTI types should be adding, and the
other two cases should be covered by the user explicitly.

Then we only need a single flag, which only applies to non-MULTI types,
and chooses whether to replace or not.

-- 
Anton Khirnov
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/6 v2] avutil/frame: add a flag to not create duplicate entries in a side data array

[James Almer]

On 3/27/2024 5:05 AM, Anton Khirnov wrote:
> Quoting James Almer (2024-03-25 21:05:57)
>> +/**
>> + * Remove existing entries before adding new ones.
>> + */
>>   #define AV_FRAME_SIDE_DATA_FLAG_UNIQUE (1 << 0)
>> +/**
>> + * Don't add a new entry if another of the same type exists.
>> + */
>> +#define AV_FRAME_SIDE_DATA_FLAG_DONT_APPEND (1 << 1)
> 
> I don't really like this API, because it leaves too much work to the
> user.
> 
> With my descriptor set, we know when it makes sense to have duplicate
> side data. So the cases that can occur when adding side data of type T
> are:
> * T not present, call succeeds
> * T present, then
>      * T does not have a MULTI prop, then user decides whether to
>        replace or do nothing (or perhaps fail)

av_frame_side_data_new() returns an entry, so for non-MULTI types, it 
should return the existing one. There's no "do nothing" scenario, and i 
don't particularly like failing, more so now that 7.0 is branched so we 
can't stealthly change behavior and for example define EEXIST error code 
as "did nothing".

>      * T does have a MULTI prop, then user decides whether to replace,
>        add, or do nothing
> 
> I think the default behaviour for MULTI types should be adding, and the
> other two cases should be covered by the user explicitly.
> 
> Then we only need a single flag, which only applies to non-MULTI types,
> and chooses whether to replace or not.

I don't follow. How does a single flag let you choose between all these 
scenarios?
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/6 v2] avutil/frame: add a flag to not create duplicate entries in a side data array

[James Almer]

On 3/27/2024 8:49 AM, James Almer wrote:
> On 3/27/2024 5:05 AM, Anton Khirnov wrote:
>> Quoting James Almer (2024-03-25 21:05:57)
>>> +/**
>>> + * Remove existing entries before adding new ones.
>>> + */
>>>   #define AV_FRAME_SIDE_DATA_FLAG_UNIQUE (1 << 0)
>>> +/**
>>> + * Don't add a new entry if another of the same type exists.
>>> + */
>>> +#define AV_FRAME_SIDE_DATA_FLAG_DONT_APPEND (1 << 1)
>>
>> I don't really like this API, because it leaves too much work to the
>> user.
>>
>> With my descriptor set, we know when it makes sense to have duplicate
>> side data. So the cases that can occur when adding side data of type T
>> are:
>> * T not present, call succeeds
>> * T present, then
>>      * T does not have a MULTI prop, then user decides whether to
>>        replace or do nothing (or perhaps fail)
> 
> av_frame_side_data_new() returns an entry, so for non-MULTI types, it 
> should return the existing one. There's no "do nothing" scenario, and i 
> don't particularly like failing, more so now that 7.0 is branched so we 
> can't stealthly change behavior and for example define EEXIST error code 
> as "did nothing".

Err, error code would be for av_frame_side_data_clone(). 
av_frame_side_data_new() can either return an entry or NULL, which the 
user will consider a failure.

> 
>>      * T does have a MULTI prop, then user decides whether to replace,
>>        add, or do nothing
>>
>> I think the default behaviour for MULTI types should be adding, and the
>> other two cases should be covered by the user explicitly.
>>
>> Then we only need a single flag, which only applies to non-MULTI types,
>> and chooses whether to replace or not.
> 
> I don't follow. How does a single flag let you choose between all these 
> scenarios?
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/6 v2] avutil/frame: add a flag to not create duplicate entries in a side data array

[Anton Khirnov]

Quoting James Almer (2024-03-27 12:49:49)
> >      * T does have a MULTI prop, then user decides whether to replace,
> >        add, or do nothing
> > 
> > I think the default behaviour for MULTI types should be adding, and the
> > other two cases should be covered by the user explicitly.
> > 
> > Then we only need a single flag, which only applies to non-MULTI types,
> > and chooses whether to replace or not.
> 
> I don't follow. How does a single flag let you choose between all these 
> scenarios?

I doesn't, my point is that for MULTI types the functions should always
add a new instance, because very few scenarios need anything else.

-- 
Anton Khirnov
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH 1/6 v3] avutil/frame: add a flag to allow overwritting existing entries

[James Almer]

Signed-off-by: James Almer <jamrial@gmail.com>
---
 libavutil/frame.c | 59 +++++++++++++++++++++++++++++++++++++++++++----
 libavutil/frame.h | 27 +++++++++++++++-------
 2 files changed, 74 insertions(+), 12 deletions(-)

diff --git a/libavutil/frame.c b/libavutil/frame.c
index ef1613c344..2e40018b83 100644
--- a/libavutil/frame.c
+++ b/libavutil/frame.c
@@ -799,12 +799,34 @@ AVFrameSideData *av_frame_side_data_new(AVFrameSideData ***sd, int *nb_sd,
                                         enum AVFrameSideDataType type,
                                         size_t size, unsigned int flags)
 {
-    AVBufferRef     *buf = av_buffer_alloc(size);
+    const AVSideDataDescriptor *desc = av_frame_side_data_desc(type);
+    AVBufferRef     *buf;
     AVFrameSideData *ret = NULL;
 
     if (flags & AV_FRAME_SIDE_DATA_FLAG_UNIQUE)
         remove_side_data(sd, nb_sd, type);
+    if (!desc || !(desc->props & AV_SIDE_DATA_PROP_MULTI)) {
+        for (int i = 0; i < *nb_sd; i++) {
+            AVFrameSideData *entry = ((*sd)[i]);
+            if (entry->type != type)
+                continue;
+            if (!(flags & AV_FRAME_SIDE_DATA_FLAG_REPLACE))
+                return NULL;
+
+            buf = av_buffer_alloc(size);
+            if (!buf)
+                return NULL;
+
+            av_buffer_unref(&entry->buf);
+            av_dict_free(&entry->metadata);
+            entry->buf  = buf;
+            entry->data = buf->data;
+            entry->size = buf->size;
+            return 0;
+        }
+    }
 
+    buf = av_buffer_alloc(size);
     ret = add_side_data_from_buf(sd, nb_sd, type, buf);
     if (!ret)
         av_buffer_unref(&buf);
@@ -815,6 +837,7 @@ AVFrameSideData *av_frame_side_data_new(AVFrameSideData ***sd, int *nb_sd,
 int av_frame_side_data_clone(AVFrameSideData ***sd, int *nb_sd,
                              const AVFrameSideData *src, unsigned int flags)
 {
+    const AVSideDataDescriptor *desc;
     AVBufferRef     *buf    = NULL;
     AVFrameSideData *sd_dst = NULL;
     int              ret    = AVERROR_BUG;
@@ -822,13 +845,41 @@ int av_frame_side_data_clone(AVFrameSideData ***sd, int *nb_sd,
     if (!sd || !src || !nb_sd || (*nb_sd && !*sd))
         return AVERROR(EINVAL);
 
+    desc = av_frame_side_data_desc(src->type);
+    if (flags & AV_FRAME_SIDE_DATA_FLAG_UNIQUE)
+        remove_side_data(sd, nb_sd, src->type);
+    if (!desc || !(desc->props & AV_SIDE_DATA_PROP_MULTI)) {
+        for (int i = 0; i < *nb_sd; i++) {
+            AVFrameSideData *entry = ((*sd)[i]);
+            AVDictionary *dict = NULL;
+
+            if (entry->type != src->type)
+                continue;
+            if (!(flags & AV_FRAME_SIDE_DATA_FLAG_REPLACE))
+                return AVERROR(EEXIST);
+
+            ret = av_dict_copy(&dict, src->metadata, 0);
+            if (ret < 0)
+                return ret;
+
+            ret = av_buffer_replace(&entry->buf, src->buf);
+            if (ret < 0) {
+                av_dict_free(&dict);
+                return ret;
+            }
+
+            av_dict_free(&entry->metadata);
+            entry->metadata = dict;
+            entry->data     = src->data;
+            entry->size     = src->size;
+            return 0;
+        }
+    }
+
     buf = av_buffer_ref(src->buf);
     if (!buf)
         return AVERROR(ENOMEM);
 
-    if (flags & AV_FRAME_SIDE_DATA_FLAG_UNIQUE)
-        remove_side_data(sd, nb_sd, src->type);
-
     sd_dst = add_side_data_from_buf(sd, nb_sd, src->type, buf);
     if (!sd_dst) {
         av_buffer_unref(&buf);
diff --git a/libavutil/frame.h b/libavutil/frame.h
index 3b6d746a16..2ea129888e 100644
--- a/libavutil/frame.h
+++ b/libavutil/frame.h
@@ -1040,7 +1040,14 @@ const AVSideDataDescriptor *av_frame_side_data_desc(enum AVFrameSideDataType typ
  */
 void av_frame_side_data_free(AVFrameSideData ***sd, int *nb_sd);
 
+/**
+ * Remove existing entries before adding new ones.
+ */
 #define AV_FRAME_SIDE_DATA_FLAG_UNIQUE (1 << 0)
+/**
+ * Don't add a new entry if another of the same type exists.
+ */
+#define AV_FRAME_SIDE_DATA_FLAG_REPLACE (1 << 1)
 
 /**
  * Add new side data entry to an array.
@@ -1053,10 +1060,12 @@ void av_frame_side_data_free(AVFrameSideData ***sd, int *nb_sd);
  * @param size  size of the side data
  * @param flags Some combination of AV_FRAME_SIDE_DATA_FLAG_* flags, or 0.
  *
- * @return newly added side data on success, NULL on error. In case of
- *         AV_FRAME_SIDE_DATA_FLAG_UNIQUE being set, entries of matching
- *         AVFrameSideDataType will be removed before the addition is
- *         attempted.
+ * @return newly added side data on success, NULL on error.
+ * @note In case of AV_FRAME_SIDE_DATA_FLAG_UNIQUE being set, entries of
+ *       matching AVFrameSideDataType will be removed before the addition
+ *       is attempted.
+ * @note In case of AV_FRAME_SIDE_DATA_FLAG_REPLACE being set, if an
+ *       entry of the same type already exists, it will be replaced instead.
  */
 AVFrameSideData *av_frame_side_data_new(AVFrameSideData ***sd, int *nb_sd,
                                         enum AVFrameSideDataType type,
@@ -1074,10 +1083,12 @@ AVFrameSideData *av_frame_side_data_new(AVFrameSideData ***sd, int *nb_sd,
  *              for the buffer.
  * @param flags Some combination of AV_FRAME_SIDE_DATA_FLAG_* flags, or 0.
  *
- * @return negative error code on failure, >=0 on success. In case of
- *         AV_FRAME_SIDE_DATA_FLAG_UNIQUE being set, entries of matching
- *         AVFrameSideDataType will be removed before the addition is
- *         attempted.
+ * @return negative error code on failure, >=0 on success.
+ * @note In case of AV_FRAME_SIDE_DATA_FLAG_UNIQUE being set, entries of
+ *       matching AVFrameSideDataType will be removed before the addition
+ *       is attempted.
+ * @note In case of AV_FRAME_SIDE_DATA_FLAG_REPLACE being set, if an
+ *       entry of the same type already exists, it will be replaced instead.
  */
 int av_frame_side_data_clone(AVFrameSideData ***sd, int *nb_sd,
                              const AVFrameSideData *src, unsigned int flags);
-- 
2.44.0

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/6 v3] avutil/frame: add a flag to allow overwritting existing entries

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 1453 bytes --]

On Wed, Mar 27, 2024 at 04:10:43PM -0300, James Almer wrote:
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
>  libavutil/frame.c | 59 +++++++++++++++++++++++++++++++++++++++++++----
>  libavutil/frame.h | 27 +++++++++++++++-------
>  2 files changed, 74 insertions(+), 12 deletions(-)

broke fate, but i see you already posted a new patch (which i didnt test yet)

@@ -1,14 +0,0 @@
-Initial addition results with duplicates:
-sd 0, Ambient viewing environment
-sd 1, Content light level metadata: MaxCLL: 1
-sd 2, Content light level metadata: MaxCLL: 2
-sd 3, Content light level metadata: MaxCLL: 3
-sd 4, Spherical Mapping
-sd 5, Content light level metadata: MaxCLL: 4
-sd 6, Content light level metadata: MaxCLL: 5
-sd 7, Content light level metadata: MaxCLL: 6
-
-Final state after a single 'no-duplicates' addition:
-sd 0, Ambient viewing environment
-sd 1, Spherical Mapping
-sd 2, Content light level metadata: MaxCLL: 1337
Test side_data_array failed. Look at tests/data/fate/side_data_array.err for details.
Assertion sd failed at libavutil/tests/side_data_array.c:63
Aborted (core dumped)
threads=1
tests/Makefile:309: recipe for target 'fate-side_data_array' failed
make: *** [fate-side_data_array] Error 134


[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

It is a danger to trust the dream we wish for rather than
the science we have, -- Dr. Kenneth Brown

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/6 v3] avutil/frame: add a flag to allow overwritting existing entries

[James Almer]

On 3/28/2024 12:25 AM, Michael Niedermayer wrote:
> On Wed, Mar 27, 2024 at 04:10:43PM -0300, James Almer wrote:
>> Signed-off-by: James Almer <jamrial@gmail.com>
>> ---
>>   libavutil/frame.c | 59 +++++++++++++++++++++++++++++++++++++++++++----
>>   libavutil/frame.h | 27 +++++++++++++++-------
>>   2 files changed, 74 insertions(+), 12 deletions(-)
> 
> broke fate, but i see you already posted a new patch (which i didnt test yet)

I sent a new patchset as a new thread just now, yes. Sorry for the noise.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".