[FFmpeg-devel] [PATCH v7 00/14] encoder AVCodecContext configuration side data

[FFmpeg-devel] [PATCH v7 00/14] encoder AVCodecContext configuration side data

[Jan Ekström]

Differences to v5:
1. rebased on top of current master
2. as requested by James, there is now a function to add new frame side data
   by means of providing a AVBufferRef.

Comparison URL (mostly configure and wrappers, avutil/frame.c):
https://github.com/jeeb/ffmpeg/compare/avcodec_cll_mdcv_side_data_v6..avcodec_cll_mdcv_side_data_v7

This patch set I've now been working for a while since I felt like it was weird
we couldn't pass through information such as static HDR metadata to encoders
from decoded input. This initial version adds the necessary framework, as well
as adds static HDR metadata support for libsvtav1, libx264 as well as libx265
wrappers.

An alternative to this would be to make encoders only properly initialize when
they receive the first AVFrame, but that seems to be a bigger, nastier change
than introducing an AVFrameSideDataSet in avctx as everything seems to
presume that extradata etc are available after opening the encoder.

Note: Any opinions on whether FFCodec or AVCodec should have
      handled_side_data list, so that if format specific generic logic is
      added, it could be checked whether the codec itself handles this side
      data? This could also be utilized to list handled side data from f.ex.
      `ffmpeg -h encoder=libsvtav1`.

Jan

Jan Ekström (14):
  avutil/frame: split side data list wiping out to non-AVFrame function
  avutil/frame: add helper for freeing arrays of side data
  avutil/frame: split side_data_from_buf to base and AVFrame func
  avutil/frame: split side data removal out to non-AVFrame function
  avutil/frame: add helper for adding side data to array
  avutil/frame: add helper for adding existing side data to array
  avutil/frame: add helper for adding side data w/ AVBufferRef to array
  avutil/frame: add helper for getting side data from array
  avcodec: add frame side data array to AVCodecContext
  avcodec: add helper for configuring AVCodecContext's frame side data
  ffmpeg: pass first video AVFrame's side data to encoder
  avcodec/libsvtav1: add support for writing out CLL and MDCV
  avcodec/libx264: add support for writing out CLL and MDCV
  avcodec/libx265: add support for writing out CLL and MDCV

 configure                       |   2 +
 fftools/ffmpeg_enc.c            |  10 ++
 libavcodec/avcodec.c            |  30 ++++++
 libavcodec/avcodec.h            |  28 +++++
 libavcodec/libsvtav1.c          |  68 ++++++++++++
 libavcodec/libx264.c            |  79 ++++++++++++++
 libavcodec/libx265.c            |  88 +++++++++++++++
 libavcodec/options.c            |   2 +
 libavutil/Makefile              |   1 +
 libavutil/frame.c               | 182 ++++++++++++++++++++++++++------
 libavutil/frame.h               |  85 +++++++++++++++
 libavutil/tests/side_data_set.c | 103 ++++++++++++++++++
 tests/fate/enc_external.mak     |  15 +++
 tests/fate/libavutil.mak        |   4 +
 tests/ref/fate/libsvtav1-hdr10  |  14 +++
 tests/ref/fate/libx264-hdr10    |  15 +++
 tests/ref/fate/libx265-hdr10    |  16 +++
 tests/ref/fate/side_data_set    |  14 +++
 18 files changed, 726 insertions(+), 30 deletions(-)
 create mode 100644 libavutil/tests/side_data_set.c
 create mode 100644 tests/ref/fate/libsvtav1-hdr10
 create mode 100644 tests/ref/fate/libx264-hdr10
 create mode 100644 tests/ref/fate/libx265-hdr10
 create mode 100644 tests/ref/fate/side_data_set


Full diff based on the same base hash for v6->v7:

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

-- 
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 v7 01/14] avutil/frame: split side data list wiping out to non-AVFrame function

[Jan Ekström]

This will make it possible to to reuse logic in further commits.
---
 libavutil/frame.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/libavutil/frame.c b/libavutil/frame.c
index a3f07ca089..d94710687b 100644
--- a/libavutil/frame.c
+++ b/libavutil/frame.c
@@ -75,14 +75,19 @@ static void free_side_data(AVFrameSideData **ptr_sd)
     av_freep(ptr_sd);
 }
 
-static void wipe_side_data(AVFrame *frame)
+static void wipe_side_data(AVFrameSideData ***sd, int *nb_side_data)
 {
-    for (int i = 0; i < frame->nb_side_data; i++) {
-        free_side_data(&frame->side_data[i]);
+    for (int i = 0; i < *nb_side_data; i++) {
+        free_side_data(&((*sd)[i]));
     }
-    frame->nb_side_data = 0;
+    *nb_side_data = 0;
+
+    av_freep(sd);
+}
 
-    av_freep(&frame->side_data);
+static void frame_side_data_wipe(AVFrame *frame)
+{
+    wipe_side_data(&frame->side_data, &frame->nb_side_data);
 }
 
 AVFrame *av_frame_alloc(void)
@@ -337,7 +342,7 @@ FF_ENABLE_DEPRECATION_WARNINGS
             sd_dst = av_frame_new_side_data(dst, sd_src->type,
                                             sd_src->size);
             if (!sd_dst) {
-                wipe_side_data(dst);
+                frame_side_data_wipe(dst);
                 return AVERROR(ENOMEM);
             }
             memcpy(sd_dst->data, sd_src->data, sd_src->size);
@@ -346,7 +351,7 @@ FF_ENABLE_DEPRECATION_WARNINGS
             sd_dst = av_frame_new_side_data_from_buf(dst, sd_src->type, ref);
             if (!sd_dst) {
                 av_buffer_unref(&ref);
-                wipe_side_data(dst);
+                frame_side_data_wipe(dst);
                 return AVERROR(ENOMEM);
             }
         }
@@ -525,7 +530,7 @@ FF_DISABLE_DEPRECATION_WARNINGS
 FF_ENABLE_DEPRECATION_WARNINGS
 #endif
 
-    wipe_side_data(dst);
+    frame_side_data_wipe(dst);
     av_dict_free(&dst->metadata);
     ret = frame_copy_props(dst, src, 0);
     if (ret < 0)
@@ -624,7 +629,7 @@ void av_frame_unref(AVFrame *frame)
     if (!frame)
         return;
 
-    wipe_side_data(frame);
+    frame_side_data_wipe(frame);
 
     for (int i = 0; i < FF_ARRAY_ELEMS(frame->buf); i++)
         av_buffer_unref(&frame->buf[i]);
-- 
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 v7 02/14] avutil/frame: add helper for freeing arrays of side data

[Jan Ekström]

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

diff --git a/libavutil/frame.c b/libavutil/frame.c
index d94710687b..615dcb1b83 100644
--- a/libavutil/frame.c
+++ b/libavutil/frame.c
@@ -90,6 +90,11 @@ static void frame_side_data_wipe(AVFrame *frame)
     wipe_side_data(&frame->side_data, &frame->nb_side_data);
 }
 
+void av_frame_side_data_free(AVFrameSideData ***sd, int *nb_sd)
+{
+    wipe_side_data(sd, nb_sd);
+}
+
 AVFrame *av_frame_alloc(void)
 {
     AVFrame *frame = av_malloc(sizeof(*frame));
diff --git a/libavutil/frame.h b/libavutil/frame.h
index c0c1b23db7..921f2212cf 100644
--- a/libavutil/frame.h
+++ b/libavutil/frame.h
@@ -1049,6 +1049,17 @@ int av_frame_apply_cropping(AVFrame *frame, int flags);
  */
 const char *av_frame_side_data_name(enum AVFrameSideDataType type);
 
+/**
+ * Free all side data entries and their contents, then zeroes out the
+ * struct values.
+ *
+ * @param sd    pointer to array of side data to free. Will be set to NULL
+ *              upon return.
+ * @param nb_sd pointer to an integer containing the number of entries in
+ *              the array. Will be set to 0 upon return.
+ */
+void av_frame_side_data_free(AVFrameSideData ***sd, int *nb_sd);
+
 /**
  * @}
  */
-- 
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 v7 03/14] avutil/frame: split side_data_from_buf to base and AVFrame func

[Jan Ekström]

---
 libavutil/frame.c | 25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/libavutil/frame.c b/libavutil/frame.c
index 615dcb1b83..5d30887ec9 100644
--- a/libavutil/frame.c
+++ b/libavutil/frame.c
@@ -787,23 +787,23 @@ FF_ENABLE_DEPRECATION_WARNINGS
     return NULL;
 }
 
-AVFrameSideData *av_frame_new_side_data_from_buf(AVFrame *frame,
-                                                 enum AVFrameSideDataType type,
-                                                 AVBufferRef *buf)
+static AVFrameSideData *add_side_data_to_set_from_buf(AVFrameSideData ***sd,
+                                                      int *nb_sd,
+                                                      enum AVFrameSideDataType type,
+                                                      AVBufferRef *buf)
 {
     AVFrameSideData *ret, **tmp;
 
     if (!buf)
         return NULL;
 
-    if (frame->nb_side_data > INT_MAX / sizeof(*frame->side_data) - 1)
+    if (*nb_sd > INT_MAX / sizeof(*sd) - 1)
         return NULL;
 
-    tmp = av_realloc(frame->side_data,
-                     (frame->nb_side_data + 1) * sizeof(*frame->side_data));
+    tmp = av_realloc(*sd, (*nb_sd + 1) * sizeof(*sd));
     if (!tmp)
         return NULL;
-    frame->side_data = tmp;
+    *sd = tmp;
 
     ret = av_mallocz(sizeof(*ret));
     if (!ret)
@@ -814,11 +814,20 @@ AVFrameSideData *av_frame_new_side_data_from_buf(AVFrame *frame,
     ret->size = buf->size;
     ret->type = type;
 
-    frame->side_data[frame->nb_side_data++] = ret;
+    (*sd)[(*nb_sd)++] = ret;
 
     return ret;
 }
 
+AVFrameSideData *av_frame_new_side_data_from_buf(AVFrame *frame,
+                                                 enum AVFrameSideDataType type,
+                                                 AVBufferRef *buf)
+{
+    return
+        add_side_data_to_set_from_buf(&frame->side_data, &frame->nb_side_data,
+                                      type, buf);
+}
+
 AVFrameSideData *av_frame_new_side_data(AVFrame *frame,
                                         enum AVFrameSideDataType type,
                                         size_t size)
-- 
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 v7 04/14] avutil/frame: split side data removal out to non-AVFrame function

[Jan Ekström]

This will make it possible to reuse logic in further commits.
---
 libavutil/frame.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/libavutil/frame.c b/libavutil/frame.c
index 5d30887ec9..788999bdeb 100644
--- a/libavutil/frame.c
+++ b/libavutil/frame.c
@@ -95,6 +95,21 @@ void av_frame_side_data_free(AVFrameSideData ***sd, int *nb_sd)
     wipe_side_data(sd, nb_sd);
 }
 
+static void remove_side_data(AVFrameSideData ***sd, int *nb_side_data,
+                             const enum AVFrameSideDataType type)
+{
+    for (int i = *nb_side_data - 1; i >= 0; i--) {
+        AVFrameSideData *entry = ((*sd)[i]);
+        if (entry->type != type)
+            continue;
+
+        free_side_data(&entry);
+
+        ((*sd)[i]) = ((*sd)[*nb_side_data - 1]);
+        (*nb_side_data)--;
+    }
+}
+
 AVFrame *av_frame_alloc(void)
 {
     AVFrame *frame = av_malloc(sizeof(*frame));
@@ -937,14 +952,7 @@ FF_ENABLE_DEPRECATION_WARNINGS
 
 void av_frame_remove_side_data(AVFrame *frame, enum AVFrameSideDataType type)
 {
-    for (int i = frame->nb_side_data - 1; i >= 0; i--) {
-        AVFrameSideData *sd = frame->side_data[i];
-        if (sd->type == type) {
-            free_side_data(&frame->side_data[i]);
-            frame->side_data[i] = frame->side_data[frame->nb_side_data - 1];
-            frame->nb_side_data--;
-        }
-    }
+    remove_side_data(&frame->side_data, &frame->nb_side_data, type);
 }
 
 const char *av_frame_side_data_name(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 v7 05/14] avutil/frame: add helper for adding side data to array

[Jan Ekström]

Additionally, add an API test to check that the no-duplicates
addition works after duplicates have been inserted.
---
 libavutil/Makefile              |   1 +
 libavutil/frame.c               |  17 ++++++
 libavutil/frame.h               |  21 +++++++
 libavutil/tests/side_data_set.c | 103 ++++++++++++++++++++++++++++++++
 tests/fate/libavutil.mak        |   4 ++
 tests/ref/fate/side_data_set    |  14 +++++
 6 files changed, 160 insertions(+)
 create mode 100644 libavutil/tests/side_data_set.c
 create mode 100644 tests/ref/fate/side_data_set

diff --git a/libavutil/Makefile b/libavutil/Makefile
index e7709b97d0..4415c913a1 100644
--- a/libavutil/Makefile
+++ b/libavutil/Makefile
@@ -271,6 +271,7 @@ TESTPROGS = adler32                                                     \
             ripemd                                                      \
             sha                                                         \
             sha512                                                      \
+            side_data_set                                               \
             softfloat                                                   \
             tree                                                        \
             twofish                                                     \
diff --git a/libavutil/frame.c b/libavutil/frame.c
index 788999bdeb..d9741240e7 100644
--- a/libavutil/frame.c
+++ b/libavutil/frame.c
@@ -855,6 +855,23 @@ AVFrameSideData *av_frame_new_side_data(AVFrame *frame,
     return ret;
 }
 
+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);
+    AVFrameSideData *ret = NULL;
+
+    if (flags & AV_FRAME_SIDE_DATA_SET_FLAG_NO_DUPLICATES)
+        remove_side_data(sd, nb_sd, type);
+
+    ret = add_side_data_to_set_from_buf(sd, nb_sd, type, buf);
+    if (!ret)
+        av_buffer_unref(&buf);
+
+    return ret;
+}
+
 AVFrameSideData *av_frame_get_side_data(const AVFrame *frame,
                                         enum AVFrameSideDataType type)
 {
diff --git a/libavutil/frame.h b/libavutil/frame.h
index 921f2212cf..726ca19a03 100644
--- a/libavutil/frame.h
+++ b/libavutil/frame.h
@@ -1060,6 +1060,27 @@ const char *av_frame_side_data_name(enum AVFrameSideDataType type);
  */
 void av_frame_side_data_free(AVFrameSideData ***sd, int *nb_sd);
 
+#define AV_FRAME_SIDE_DATA_SET_FLAG_NO_DUPLICATES (1 << 0)
+
+/**
+ * Add a new side data entry to a set.
+ *
+ * @param sd    pointer to array of side data to which to add another entry.
+ * @param nb_sd pointer to an integer containing the number of entries in
+ *              the array.
+ * @param type  type of the added side data
+ * @param size  size of the side data
+ * @param flags Some combination of AV_FRAME_SIDE_DATA_SET_FLAG_* flags, or 0.
+ *
+ * @return newly added side data on success, NULL on error. In case of
+ *         AV_FRAME_SIDE_DATA_SET_FLAG_NO_DUPLICATES being set, entries
+ *         of matching AVFrameSideDataType will be removed before the
+ *         addition is attempted.
+ */
+AVFrameSideData *av_frame_side_data_new(AVFrameSideData ***sd, int *nb_sd,
+                                        enum AVFrameSideDataType type,
+                                        size_t size, unsigned int flags);
+
 /**
  * @}
  */
diff --git a/libavutil/tests/side_data_set.c b/libavutil/tests/side_data_set.c
new file mode 100644
index 0000000000..7ef99db3d2
--- /dev/null
+++ b/libavutil/tests/side_data_set.c
@@ -0,0 +1,103 @@
+/*
+ * Copyright (c) 2023 Jan Ekström <jeebjp@gmail.com>
+ *
+ * This file is part of FFmpeg.
+ *
+ * FFmpeg is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * FFmpeg is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with FFmpeg; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+ */
+
+#include <stdio.h>
+#include "libavutil/frame.c"
+#include "libavutil/mastering_display_metadata.h"
+
+static void print_clls(const AVFrameSideData **sd, const int nb_sd)
+{
+    for (int i = 0; i < nb_sd; i++) {
+        const AVFrameSideData *entry = sd[i];
+
+        printf("sd %d, %s",
+               i, av_frame_side_data_name(entry->type));
+
+        if (entry->type != AV_FRAME_DATA_CONTENT_LIGHT_LEVEL) {
+            putchar('\n');
+            continue;
+        }
+
+        printf(": MaxCLL: %u\n",
+               ((AVContentLightMetadata *)entry->data)->MaxCLL);
+    }
+}
+
+typedef struct FrameSideDataSet {
+    AVFrameSideData **sd;
+    int            nb_sd;
+} FrameSideDataSet;
+
+int main(void)
+{
+    FrameSideDataSet set = { 0 };
+
+    av_assert0(
+        av_frame_side_data_new(&set.sd, &set.nb_sd,
+                               AV_FRAME_DATA_AMBIENT_VIEWING_ENVIRONMENT,
+                               0, 0));
+
+    // test entries in the middle
+    for (int value = 1; value < 4; value++) {
+        AVFrameSideData *sd = av_frame_side_data_new(
+            &set.sd, &set.nb_sd, AV_FRAME_DATA_CONTENT_LIGHT_LEVEL,
+            sizeof(AVContentLightMetadata), 0);
+
+        av_assert0(sd);
+
+        ((AVContentLightMetadata *)sd->data)->MaxCLL = value;
+    }
+
+    av_assert0(
+        av_frame_side_data_new(
+            &set.sd, &set.nb_sd, AV_FRAME_DATA_SPHERICAL, 0, 0));
+
+    // test entries at the end
+    for (int value = 1; value < 4; value++) {
+        AVFrameSideData *sd = av_frame_side_data_new(
+            &set.sd, &set.nb_sd, AV_FRAME_DATA_CONTENT_LIGHT_LEVEL,
+            sizeof(AVContentLightMetadata), 0);
+
+        av_assert0(sd);
+
+        ((AVContentLightMetadata *)sd->data)->MaxCLL = value + 3;
+    }
+
+    puts("Initial addition results with duplicates:");
+    print_clls((const AVFrameSideData **)set.sd, set.nb_sd);
+
+    {
+        AVFrameSideData *sd = av_frame_side_data_new(
+            &set.sd, &set.nb_sd, AV_FRAME_DATA_CONTENT_LIGHT_LEVEL,
+            sizeof(AVContentLightMetadata),
+            AV_FRAME_SIDE_DATA_SET_FLAG_NO_DUPLICATES);
+
+        av_assert0(sd);
+
+        ((AVContentLightMetadata *)sd->data)->MaxCLL = 1337;
+    }
+
+    puts("\nFinal state after a single 'no-duplicates' addition:");
+    print_clls((const AVFrameSideData **)set.sd, set.nb_sd);
+
+    av_frame_side_data_free(&set.sd, &set.nb_sd);
+
+    return 0;
+}
diff --git a/tests/fate/libavutil.mak b/tests/fate/libavutil.mak
index 80153f4395..6864ea9c03 100644
--- a/tests/fate/libavutil.mak
+++ b/tests/fate/libavutil.mak
@@ -148,6 +148,10 @@ FATE_LIBAVUTIL += fate-sha512
 fate-sha512: libavutil/tests/sha512$(EXESUF)
 fate-sha512: CMD = run libavutil/tests/sha512$(EXESUF)
 
+FATE_LIBAVUTIL += fate-side_data_set
+fate-side_data_set: libavutil/tests/side_data_set$(EXESUF)
+fate-side_data_set: CMD = run libavutil/tests/side_data_set$(EXESUF)
+
 FATE_LIBAVUTIL += fate-tree
 fate-tree: libavutil/tests/tree$(EXESUF)
 fate-tree: CMD = run libavutil/tests/tree$(EXESUF)
diff --git a/tests/ref/fate/side_data_set b/tests/ref/fate/side_data_set
new file mode 100644
index 0000000000..7d8c684d8f
--- /dev/null
+++ b/tests/ref/fate/side_data_set
@@ -0,0 +1,14 @@
+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
-- 
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 v7 05/14] avutil/frame: add helper for adding side data to array

[James Almer]

On 2/29/2024 1:42 PM, Jan Ekström wrote:
> diff --git a/libavutil/frame.h b/libavutil/frame.h
> index 921f2212cf..726ca19a03 100644
> --- a/libavutil/frame.h
> +++ b/libavutil/frame.h
> @@ -1060,6 +1060,27 @@ const char *av_frame_side_data_name(enum AVFrameSideDataType type);
>    */
>   void av_frame_side_data_free(AVFrameSideData ***sd, int *nb_sd);
>   
> +#define AV_FRAME_SIDE_DATA_SET_FLAG_NO_DUPLICATES (1 << 0)
> +
> +/**
> + * Add a new side data entry to a set.
> + *
> + * @param sd    pointer to array of side data to which to add another entry.

Or to NULL, 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 size  size of the side data
> + * @param flags Some combination of AV_FRAME_SIDE_DATA_SET_FLAG_* flags, or 0.
> + *
> + * @return newly added side data on success, NULL on error. In case of
> + *         AV_FRAME_SIDE_DATA_SET_FLAG_NO_DUPLICATES being set, entries
> + *         of matching AVFrameSideDataType will be removed before the
> + *         addition is attempted.
> + */
> +AVFrameSideData *av_frame_side_data_new(AVFrameSideData ***sd, int *nb_sd,
> +                                        enum AVFrameSideDataType type,
> +                                        size_t size, unsigned int flags);
> +
>   /**
>    * @}
>    */
_______________________________________________
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 v7 05/14] avutil/frame: add helper for adding side data to array

[Anton Khirnov]

Quoting Jan Ekström (2024-02-29 17:42:52)
> diff --git a/libavutil/frame.h b/libavutil/frame.h
> index 921f2212cf..726ca19a03 100644
> --- a/libavutil/frame.h
> +++ b/libavutil/frame.h
> @@ -1060,6 +1060,27 @@ const char *av_frame_side_data_name(enum AVFrameSideDataType type);
>   */
>  void av_frame_side_data_free(AVFrameSideData ***sd, int *nb_sd);
>  
> +#define AV_FRAME_SIDE_DATA_SET_FLAG_NO_DUPLICATES (1 << 0)

Long name is long, also obsolete reference to sets.

AV_FRAME_SIDE_DATA_FLAG_UNIQUE?

-- 
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 v7 06/14] avutil/frame: add helper for adding existing side data to array

[Jan Ekström]

---
 libavutil/frame.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++
 libavutil/frame.h | 19 ++++++++++++++++++
 2 files changed, 68 insertions(+)

diff --git a/libavutil/frame.c b/libavutil/frame.c
index d9741240e7..99c9ce4119 100644
--- a/libavutil/frame.c
+++ b/libavutil/frame.c
@@ -110,6 +110,23 @@ static void remove_side_data(AVFrameSideData ***sd, int *nb_side_data,
     }
 }
 
+static void remove_side_data_by_entry(AVFrameSideData ***sd, int *nb_sd,
+                                      const AVFrameSideData *target)
+{
+    for (int i = *nb_sd - 1; i >= 0; i--) {
+        AVFrameSideData *entry = ((*sd)[i]);
+        if (entry != target)
+            continue;
+
+        free_side_data(&entry);
+
+        ((*sd)[i]) = ((*sd)[*nb_sd - 1]);
+        (*nb_sd)--;
+
+        return;
+    }
+}
+
 AVFrame *av_frame_alloc(void)
 {
     AVFrame *frame = av_malloc(sizeof(*frame));
@@ -872,6 +889,38 @@ AVFrameSideData *av_frame_side_data_new(AVFrameSideData ***sd, int *nb_sd,
     return ret;
 }
 
+int av_frame_side_data_from_sd(AVFrameSideData ***sd, int *nb_sd,
+                               const AVFrameSideData *src,
+                               unsigned int flags)
+{
+    if (!sd || !src || !nb_sd || (*nb_sd && !*sd))
+        return AVERROR(EINVAL);
+
+    {
+        AVBufferRef           *buf    = av_buffer_ref(src->buf);
+        AVFrameSideData       *sd_dst = NULL;
+
+        if (flags & AV_FRAME_SIDE_DATA_SET_FLAG_NO_DUPLICATES)
+            remove_side_data(sd, nb_sd, src->type);
+
+        sd_dst = add_side_data_to_set_from_buf(sd, nb_sd, src->type, buf);
+        if (!sd_dst) {
+            av_buffer_unref(&buf);
+            return AVERROR(ENOMEM);
+        }
+
+        {
+            int ret = av_dict_copy(&sd_dst->metadata, src->metadata, 0);
+            if (ret < 0) {
+                remove_side_data_by_entry(sd, nb_sd, sd_dst);
+                return ret;
+            }
+        }
+
+        return 0;
+    }
+}
+
 AVFrameSideData *av_frame_get_side_data(const AVFrame *frame,
                                         enum AVFrameSideDataType type)
 {
diff --git a/libavutil/frame.h b/libavutil/frame.h
index 726ca19a03..47d0096bc4 100644
--- a/libavutil/frame.h
+++ b/libavutil/frame.h
@@ -1081,6 +1081,25 @@ AVFrameSideData *av_frame_side_data_new(AVFrameSideData ***sd, int *nb_sd,
                                         enum AVFrameSideDataType type,
                                         size_t size, unsigned int flags);
 
+/**
+ * Add a new side data entry to a set based on existing side data, taking
+ * a reference towards the contained AVBufferRef.
+ *
+ * @param sd    pointer to array of side data to which to add another entry.
+ * @param nb_sd pointer to an integer containing the number of entries in
+ *              the array.
+ * @param src   side data which should be added to the set
+ * @param flags Some combination of AV_FRAME_SIDE_DATA_SET_FLAG_* flags, or 0.
+ *
+ * @return negative error code on failure, >=0 on success. In case of
+ *         AV_FRAME_SIDE_DATA_SET_FLAG_NO_DUPLICATES being set, entries
+ *         of matching AVFrameSideDataType will be removed before the
+ *         addition is attempted.
+ */
+int av_frame_side_data_from_sd(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 v7 06/14] avutil/frame: add helper for adding existing side data to array

[Anton Khirnov]

Quoting Jan Ekström (2024-02-29 17:42:53)
> @@ -872,6 +889,38 @@ AVFrameSideData *av_frame_side_data_new(AVFrameSideData ***sd, int *nb_sd,
>      return ret;
>  }
>  
> +int av_frame_side_data_from_sd(AVFrameSideData ***sd, int *nb_sd,
> +                               const AVFrameSideData *src,
> +                               unsigned int flags)
> +{
> +    if (!sd || !src || !nb_sd || (*nb_sd && !*sd))
> +        return AVERROR(EINVAL);

Overzealous checking is evil. This should be removed, or made into an
assert.

> +
> +    {

These extra blocks are ugly.

> +        AVBufferRef           *buf    = av_buffer_ref(src->buf);
> +        AVFrameSideData       *sd_dst = NULL;
> +
> +        if (flags & AV_FRAME_SIDE_DATA_SET_FLAG_NO_DUPLICATES)
> +            remove_side_data(sd, nb_sd, src->type);
> +
> +        sd_dst = add_side_data_to_set_from_buf(sd, nb_sd, src->type, buf);
> +        if (!sd_dst) {
> +            av_buffer_unref(&buf);
> +            return AVERROR(ENOMEM);
> +        }
> +
> +        {
> +            int ret = av_dict_copy(&sd_dst->metadata, src->metadata, 0);
> +            if (ret < 0) {
> +                remove_side_data_by_entry(sd, nb_sd, sd_dst);

Removing by the pointer is ugly. You could either
* assume add_side_data_to_set_from_buf() adds to the end and remove the
  last element
* have add_side_data_to_set_from_buf() return the index of the added
  element

> +                return ret;
> +            }
> +        }
> +
> +        return 0;
> +    }
> +}
> +
>  AVFrameSideData *av_frame_get_side_data(const AVFrame *frame,
>                                          enum AVFrameSideDataType type)
>  {
> diff --git a/libavutil/frame.h b/libavutil/frame.h
> index 726ca19a03..47d0096bc4 100644
> --- a/libavutil/frame.h
> +++ b/libavutil/frame.h
> @@ -1081,6 +1081,25 @@ AVFrameSideData *av_frame_side_data_new(AVFrameSideData ***sd, int *nb_sd,
>                                          enum AVFrameSideDataType type,
>                                          size_t size, unsigned int flags);
>  
> +/**
> + * Add a new side data entry to a set based on existing side data, taking
> + * a reference towards the contained AVBufferRef.
> + *
> + * @param sd    pointer to array of side data to which to add another entry.
> + * @param nb_sd pointer to an integer containing the number of entries in
> + *              the array.
> + * @param src   side data which should be added to the set
> + * @param flags Some combination of AV_FRAME_SIDE_DATA_SET_FLAG_* flags, or 0.
> + *
> + * @return negative error code on failure, >=0 on success. In case of
> + *         AV_FRAME_SIDE_DATA_SET_FLAG_NO_DUPLICATES being set, entries
> + *         of matching AVFrameSideDataType will be removed before the
> + *         addition is attempted.
> + */
> +int av_frame_side_data_from_sd(AVFrameSideData ***sd, int *nb_sd,

av_frame_side_data_clone()?
av_frame_side_data_ref()?

-- 
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 v7 07/14] avutil/frame: add helper for adding side data w/ AVBufferRef to array

[Jan Ekström]

This was requested to be added in review.
---
 libavutil/frame.c | 57 +++++++++++++++++++++++++++++++----------------
 libavutil/frame.h | 20 +++++++++++++++++
 2 files changed, 58 insertions(+), 19 deletions(-)

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

[Anton Khirnov]

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

This seems inconsistent with av_frame_new_side_data_from_buf(), which
takes ownership of the buffer reference passed to it.
Is there a strong reason to do it differently here?

-- 
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 v7 07/14] avutil/frame: add helper for adding side data w/ AVBufferRef to array

[James Almer]

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

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

The alternative, to keep the behavior of taking ownership of the passed 
on reference, is to have this function take a pointer to pointer, and 
clearing it on success.
_______________________________________________
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 v7 07/14] avutil/frame: add helper for adding side data w/ AVBufferRef to array

[Anton Khirnov]

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

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

-- 
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 v7 08/14] avutil/frame: add helper for getting side data from array

[Jan Ekström]

---
 libavutil/frame.c | 20 +++++++++++++++-----
 libavutil/frame.h | 14 ++++++++++++++
 2 files changed, 29 insertions(+), 5 deletions(-)

diff --git a/libavutil/frame.c b/libavutil/frame.c
index 3a2084d1ca..fd4b53f9ff 100644
--- a/libavutil/frame.c
+++ b/libavutil/frame.c
@@ -940,16 +940,26 @@ int av_frame_side_data_from_sd(AVFrameSideData ***sd, int *nb_sd,
     return 0;
 }
 
-AVFrameSideData *av_frame_get_side_data(const AVFrame *frame,
-                                        enum AVFrameSideDataType type)
+const AVFrameSideData *av_frame_side_data_get(const AVFrameSideData **sd,
+                                              const int nb_sd,
+                                              enum AVFrameSideDataType type)
 {
-    for (int i = 0; i < frame->nb_side_data; i++) {
-        if (frame->side_data[i]->type == type)
-            return frame->side_data[i];
+    for (int i = 0; i < nb_sd; i++) {
+        if (sd[i]->type == type)
+            return sd[i];
     }
     return NULL;
 }
 
+AVFrameSideData *av_frame_get_side_data(const AVFrame *frame,
+                                        enum AVFrameSideDataType type)
+{
+    return (AVFrameSideData *)av_frame_side_data_get(
+        (const AVFrameSideData **)frame->side_data, frame->nb_side_data,
+        type
+    );
+}
+
 static int frame_copy_video(AVFrame *dst, const AVFrame *src)
 {
     int planes;
diff --git a/libavutil/frame.h b/libavutil/frame.h
index 908fd4a90d..01311e68eb 100644
--- a/libavutil/frame.h
+++ b/libavutil/frame.h
@@ -1120,6 +1120,20 @@ int av_frame_side_data_from_sd(AVFrameSideData ***sd, int *nb_sd,
                                const AVFrameSideData *src,
                                unsigned int flags);
 
+/**
+ * Get a side data entry of a specific type from a set.
+ *
+ * @param sd    array of side data.
+ * @param nb_sd integer containing the number of entries in the array.
+ * @param type  type of side data to be queried
+ *
+ * @return a pointer to the side data of a given type on success, NULL if there
+ *         is no side data with such type in this set.
+ */
+const AVFrameSideData *av_frame_side_data_get(const AVFrameSideData **sd,
+                                              const 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 v7 09/14] avcodec: add frame side data array to AVCodecContext

[Jan Ekström]

This allows configuring an encoder by using AVFrameSideData.
---
 libavcodec/avcodec.h | 8 ++++++++
 libavcodec/options.c | 2 ++
 2 files changed, 10 insertions(+)

diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
index 43859251cc..411f4caad3 100644
--- a/libavcodec/avcodec.h
+++ b/libavcodec/avcodec.h
@@ -2120,6 +2120,14 @@ typedef struct AVCodecContext {
      *   an error.
      */
     int64_t frame_num;
+
+    /**
+     * Set containing static side data, such as HDR10 CLL / MDCV structures.
+     * - encoding: set by user
+     * - decoding: unused
+     */
+    AVFrameSideData  **frame_side_data;
+    int             nb_frame_side_data;
 } AVCodecContext;
 
 /**
diff --git a/libavcodec/options.c b/libavcodec/options.c
index 928e430ce9..7e39b49b7e 100644
--- a/libavcodec/options.c
+++ b/libavcodec/options.c
@@ -181,6 +181,8 @@ void avcodec_free_context(AVCodecContext **pavctx)
     av_freep(&avctx->inter_matrix);
     av_freep(&avctx->rc_override);
     av_channel_layout_uninit(&avctx->ch_layout);
+    av_frame_side_data_free(
+        &avctx->frame_side_data, &avctx->nb_frame_side_data);
 
     av_freep(pavctx);
 }
-- 
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 v7 09/14] avcodec: add frame side data array to AVCodecContext

[James Almer]

On 2/29/2024 1:42 PM, Jan Ekström wrote:
> This allows configuring an encoder by using AVFrameSideData.

Maybe mention that in the doxy for the field. Explain that it needs to 
be set before avcodec_open2() and is used to initialize an encoder.

> ---
>   libavcodec/avcodec.h | 8 ++++++++
>   libavcodec/options.c | 2 ++
>   2 files changed, 10 insertions(+)
> 
> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> index 43859251cc..411f4caad3 100644
> --- a/libavcodec/avcodec.h
> +++ b/libavcodec/avcodec.h
> @@ -2120,6 +2120,14 @@ typedef struct AVCodecContext {
>        *   an error.
>        */
>       int64_t frame_num;
> +
> +    /**
> +     * Set containing static side data, such as HDR10 CLL / MDCV structures.
> +     * - encoding: set by user
> +     * - decoding: unused
> +     */
> +    AVFrameSideData  **frame_side_data;
> +    int             nb_frame_side_data;
>   } AVCodecContext;
>   
>   /**
> diff --git a/libavcodec/options.c b/libavcodec/options.c
> index 928e430ce9..7e39b49b7e 100644
> --- a/libavcodec/options.c
> +++ b/libavcodec/options.c
> @@ -181,6 +181,8 @@ void avcodec_free_context(AVCodecContext **pavctx)
>       av_freep(&avctx->inter_matrix);
>       av_freep(&avctx->rc_override);
>       av_channel_layout_uninit(&avctx->ch_layout);
> +    av_frame_side_data_free(
> +        &avctx->frame_side_data, &avctx->nb_frame_side_data);
>   
>       av_freep(pavctx);
>   }
_______________________________________________
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 v7 09/14] avcodec: add frame side data array to AVCodecContext

[James Almer]

On 3/1/2024 1:03 PM, James Almer wrote:
> On 2/29/2024 1:42 PM, Jan Ekström wrote:
>> This allows configuring an encoder by using AVFrameSideData.
> 
> Maybe mention that in the doxy for the field. Explain that it needs to 
> be set before avcodec_open2() and is used to initialize an encoder.

And that it must be allocated with av_frame_side_data_new(), 
av_frame_side_data_from_sd() or av_frame_side_data_from_buf().

> 
>> ---
>>   libavcodec/avcodec.h | 8 ++++++++
>>   libavcodec/options.c | 2 ++
>>   2 files changed, 10 insertions(+)
>>
>> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
>> index 43859251cc..411f4caad3 100644
>> --- a/libavcodec/avcodec.h
>> +++ b/libavcodec/avcodec.h
>> @@ -2120,6 +2120,14 @@ typedef struct AVCodecContext {
>>        *   an error.
>>        */
>>       int64_t frame_num;
>> +
>> +    /**
>> +     * Set containing static side data, such as HDR10 CLL / MDCV 
>> structures.
>> +     * - encoding: set by user
>> +     * - decoding: unused
>> +     */
>> +    AVFrameSideData  **frame_side_data;
>> +    int             nb_frame_side_data;
>>   } AVCodecContext;
>>   /**
>> diff --git a/libavcodec/options.c b/libavcodec/options.c
>> index 928e430ce9..7e39b49b7e 100644
>> --- a/libavcodec/options.c
>> +++ b/libavcodec/options.c
>> @@ -181,6 +181,8 @@ void avcodec_free_context(AVCodecContext **pavctx)
>>       av_freep(&avctx->inter_matrix);
>>       av_freep(&avctx->rc_override);
>>       av_channel_layout_uninit(&avctx->ch_layout);
>> +    av_frame_side_data_free(
>> +        &avctx->frame_side_data, &avctx->nb_frame_side_data);
>>       av_freep(pavctx);
>>   }
_______________________________________________
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 v7 09/14] avcodec: add frame side data array to AVCodecContext

[Anton Khirnov]

Quoting Jan Ekström (2024-02-29 17:42:56)
> This allows configuring an encoder by using AVFrameSideData.
> ---
>  libavcodec/avcodec.h | 8 ++++++++
>  libavcodec/options.c | 2 ++
>  2 files changed, 10 insertions(+)
> 
> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> index 43859251cc..411f4caad3 100644
> --- a/libavcodec/avcodec.h
> +++ b/libavcodec/avcodec.h
> @@ -2120,6 +2120,14 @@ typedef struct AVCodecContext {
>       *   an error.
>       */
>      int64_t frame_num;
> +
> +    /**
> +     * Set containing static side data, such as HDR10 CLL / MDCV structures.
> +     * - encoding: set by user

May be set by the caller before avcodec_open2(). Afterwards owned and
freed by the encoder.

> +     * - decoding: unused
> +     */
> +    AVFrameSideData  **frame_side_data;
> +    int             nb_frame_side_data;

I don't like calling the field 'frame' side data, because it is not
associated with any frame.

I'd prefer to call it 'decoded_side_data' or 'raw_side_data', to be
consistent with existing 'coded_side_data'.

-- 
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 v7 10/14] avcodec: add helper for configuring AVCodecContext's frame side data

[Jan Ekström]

This allows API clients that wish to configure multiple entries
at a time to do so without writing the looping code themselves.
---
 libavcodec/avcodec.c | 30 ++++++++++++++++++++++++++++++
 libavcodec/avcodec.h | 20 ++++++++++++++++++++
 2 files changed, 50 insertions(+)

diff --git a/libavcodec/avcodec.c b/libavcodec/avcodec.c
index a13b781996..d3ec41b280 100644
--- a/libavcodec/avcodec.c
+++ b/libavcodec/avcodec.c
@@ -720,3 +720,33 @@ int attribute_align_arg avcodec_receive_frame(AVCodecContext *avctx, AVFrame *fr
         return ff_decode_receive_frame(avctx, frame);
     return ff_encode_receive_frame(avctx, frame);
 }
+
+int avcodec_configure_side_data(AVCodecContext *avctx,
+                                const AVFrameSideData **sd, const int nb_sd,
+                                unsigned int flags)
+{
+    if (!avctx)
+        return AVERROR(EINVAL);
+
+    if (!sd) {
+        av_frame_side_data_free(
+            &avctx->frame_side_data, &avctx->nb_frame_side_data);
+        return 0;
+    }
+
+    if (nb_sd > 0 && nb_sd == avctx->nb_frame_side_data &&
+        sd == (const AVFrameSideData **)avctx->frame_side_data)
+        return AVERROR(EINVAL);
+
+    for (int i = 0; i < nb_sd; i++) {
+        int ret = av_frame_side_data_from_sd(
+            &avctx->frame_side_data, &avctx->nb_frame_side_data, sd[i], flags);
+        if (ret < 0) {
+            av_frame_side_data_free(
+                &avctx->frame_side_data, &avctx->nb_frame_side_data);
+            return ret;
+        }
+    }
+
+    return 0;
+}
diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
index 411f4caad3..3b5ca9c0ef 100644
--- a/libavcodec/avcodec.h
+++ b/libavcodec/avcodec.h
@@ -3162,6 +3162,26 @@ void av_fast_padded_mallocz(void *ptr, unsigned int *size, size_t min_size);
  */
 int avcodec_is_open(AVCodecContext *s);
 
+/**
+ * Add multiple side data entries to an AVCodecContext set in one go, for
+ * example from an AVFrame.
+ *
+ * In case the function fails to add a side data entry, it will clear the
+ * whole side data set.
+ *
+ * @param avctx context to which the side data should be added
+ * @param sd    array of side data to use as input.
+ *              if null, clears out the side data for this context.
+ * @param nb_sd integer containing the number of entries in the array.
+ * @param flags Some combination of AV_FRAME_SIDE_DATA_SET_FLAG_* flags, or 0.
+ *
+ * @return negative error code on failure, >=0 on success.
+ *
+ * @see av_frame_side_data_set_new_entry regarding the flags.
+ */
+int avcodec_configure_side_data(AVCodecContext *avctx,
+                                const AVFrameSideData **sd, const int nb_sd,
+                                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 v7 10/14] avcodec: add helper for configuring AVCodecContext's frame side data

[Anton Khirnov]

Quoting Jan Ekström (2024-02-29 17:42:57)
> +/**
> + * Add multiple side data entries to an AVCodecContext set in one go, for
> + * example from an AVFrame.
> + *
> + * In case the function fails to add a side data entry, it will clear the
> + * whole side data set.
> + *
> + * @param avctx context to which the side data should be added
> + * @param sd    array of side data to use as input.
> + *              if null, clears out the side data for this context.
> + * @param nb_sd integer containing the number of entries in the array.
> + * @param flags Some combination of AV_FRAME_SIDE_DATA_SET_FLAG_* flags, or 0.
> + *
> + * @return negative error code on failure, >=0 on success.
> + *
> + * @see av_frame_side_data_set_new_entry regarding the flags.
> + */
> +int avcodec_configure_side_data(AVCodecContext *avctx,
> +                                const AVFrameSideData **sd, const int nb_sd,
> +                                unsigned int flags);

Why should this be tied to avcodec and not just
av_frame_side_data_copy()?

-- 
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 v7 11/14] ffmpeg: pass first video AVFrame's side data to encoder

[Jan Ekström]

This enables further configuration of output based on the results
of input decoding and filtering in a similar manner as the color
information.
---
 fftools/ffmpeg_enc.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/fftools/ffmpeg_enc.c b/fftools/ffmpeg_enc.c
index bdba50df03..e39ba05b3b 100644
--- a/fftools/ffmpeg_enc.c
+++ b/fftools/ffmpeg_enc.c
@@ -245,6 +245,16 @@ int enc_open(void *opaque, const AVFrame *frame)
         enc_ctx->colorspace             = frame->colorspace;
         enc_ctx->chroma_sample_location = frame->chroma_location;
 
+        ret = avcodec_configure_side_data(
+            enc_ctx,
+            (const AVFrameSideData **)frame->side_data, frame->nb_side_data,
+            AV_FRAME_SIDE_DATA_SET_FLAG_NO_DUPLICATES);
+        if (ret < 0) {
+            av_log(NULL, AV_LOG_ERROR, "failed to configure video encoder: %s!\n",
+                   av_err2str(ret));
+            return ret;
+        }
+
         if (enc_ctx->flags & (AV_CODEC_FLAG_INTERLACED_DCT | AV_CODEC_FLAG_INTERLACED_ME) ||
             (frame->flags & AV_FRAME_FLAG_INTERLACED)
 #if FFMPEG_OPT_TOP
-- 
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 v7 11/14] ffmpeg: pass first video AVFrame's side data to encoder

[Anton Khirnov]

Quoting Jan Ekström (2024-02-29 17:42:58)
> This enables further configuration of output based on the results
> of input decoding and filtering in a similar manner as the color
> information.
> ---
>  fftools/ffmpeg_enc.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/fftools/ffmpeg_enc.c b/fftools/ffmpeg_enc.c
> index bdba50df03..e39ba05b3b 100644
> --- a/fftools/ffmpeg_enc.c
> +++ b/fftools/ffmpeg_enc.c
> @@ -245,6 +245,16 @@ int enc_open(void *opaque, const AVFrame *frame)
>          enc_ctx->colorspace             = frame->colorspace;
>          enc_ctx->chroma_sample_location = frame->chroma_location;
>  
> +        ret = avcodec_configure_side_data(
> +            enc_ctx,
> +            (const AVFrameSideData **)frame->side_data, frame->nb_side_data,
> +            AV_FRAME_SIDE_DATA_SET_FLAG_NO_DUPLICATES);

I'm not sure this makes sense in general.

The side data in AVCodecContext is supposed to be 'static', i.e.
applying to the whole stream. Many side data types do not make sense as
global side data (e.g. closed captions). Or it could be that the side
data is not actually static, but ffmpeg CLI will now always claim that
it is.

-- 
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 v7 12/14] avcodec/libsvtav1: add support for writing out CLL and MDCV

[Jan Ekström]

These two were added in 28e23d7f348c78d49a726c7469f9d4e38edec341
and 3558c1f2e97455e0b89edef31b9a72ab7fa30550 for version 0.9.0 of
SVT-AV1, which is also our minimum requirement right now.

In other words, no additional version limiting conditions seem
to be required.

Additionally, add a FATE test which verifies that pass-through of
the MDCV/CLL side data is working during encoding.
---
 libavcodec/libsvtav1.c         | 68 ++++++++++++++++++++++++++++++++++
 tests/fate/enc_external.mak    |  5 +++
 tests/ref/fate/libsvtav1-hdr10 | 14 +++++++
 3 files changed, 87 insertions(+)
 create mode 100644 tests/ref/fate/libsvtav1-hdr10

diff --git a/libavcodec/libsvtav1.c b/libavcodec/libsvtav1.c
index 1eda63200c..072496a945 100644
--- a/libavcodec/libsvtav1.c
+++ b/libavcodec/libsvtav1.c
@@ -27,6 +27,8 @@
 #include "libavutil/common.h"
 #include "libavutil/frame.h"
 #include "libavutil/imgutils.h"
+#include "libavutil/intreadwrite.h"
+#include "libavutil/mastering_display_metadata.h"
 #include "libavutil/opt.h"
 #include "libavutil/pixdesc.h"
 #include "libavutil/avassert.h"
@@ -146,6 +148,70 @@ static int alloc_buffer(EbSvtAv1EncConfiguration *config, SvtContext *svt_enc)
 
 }
 
+static void handle_mdcv(struct EbSvtAv1MasteringDisplayInfo *dst,
+                        const AVMasteringDisplayMetadata *mdcv)
+{
+    if (mdcv->has_primaries) {
+        const struct EbSvtAv1ChromaPoints *const points[] = {
+            &dst->r,
+            &dst->g,
+            &dst->b,
+        };
+
+        for (int i = 0; i < 3; i++) {
+            const struct EbSvtAv1ChromaPoints *dst = points[i];
+            const AVRational *src = mdcv->display_primaries[i];
+
+            AV_WB16(&dst->x,
+                    av_rescale_q(1, src[0], (AVRational){ 1, (1 << 16) }));
+            AV_WB16(&dst->y,
+                    av_rescale_q(1, src[1], (AVRational){ 1, (1 << 16) }));
+        }
+
+        AV_WB16(&dst->white_point.x,
+                av_rescale_q(1, mdcv->white_point[0],
+                             (AVRational){ 1, (1 << 16) }));
+        AV_WB16(&dst->white_point.y,
+                av_rescale_q(1, mdcv->white_point[1],
+                             (AVRational){ 1, (1 << 16) }));
+    }
+
+    if (mdcv->has_luminance) {
+        AV_WB32(&dst->max_luma,
+                av_rescale_q(1, mdcv->max_luminance,
+                             (AVRational){ 1, (1 << 8) }));
+        AV_WB32(&dst->min_luma,
+                av_rescale_q(1, mdcv->min_luminance,
+                             (AVRational){ 1, (1 << 14) }));
+    }
+}
+
+static void handle_side_data(AVCodecContext *avctx,
+                             EbSvtAv1EncConfiguration *param)
+{
+    const AVFrameSideData *cll_sd =
+        av_frame_side_data_get(
+            (const AVFrameSideData **)avctx->frame_side_data,
+            avctx->nb_frame_side_data, AV_FRAME_DATA_CONTENT_LIGHT_LEVEL);
+    const AVFrameSideData *mdcv_sd =
+        av_frame_side_data_get(
+            (const AVFrameSideData **)avctx->frame_side_data,
+            avctx->nb_frame_side_data, AV_FRAME_DATA_MASTERING_DISPLAY_METADATA);
+
+    if (cll_sd) {
+        const AVContentLightMetadata *cll =
+            (AVContentLightMetadata *)cll_sd->data;
+
+        AV_WB16(&param->content_light_level.max_cll, cll->MaxCLL);
+        AV_WB16(&param->content_light_level.max_fall, cll->MaxFALL);
+    }
+
+    if (mdcv_sd) {
+        handle_mdcv(&param->mastering_display,
+                    (AVMasteringDisplayMetadata *)mdcv_sd->data);
+    }
+}
+
 static int config_enc_params(EbSvtAv1EncConfiguration *param,
                              AVCodecContext *avctx)
 {
@@ -280,6 +346,8 @@ FF_ENABLE_DEPRECATION_WARNINGS
     /* 2 = IDR, closed GOP, 1 = CRA, open GOP */
     param->intra_refresh_type = avctx->flags & AV_CODEC_FLAG_CLOSED_GOP ? 2 : 1;
 
+    handle_side_data(avctx, param);
+
 #if SVT_AV1_CHECK_VERSION(0, 9, 1)
     while ((en = av_dict_get(svt_enc->svtav1_opts, "", en, AV_DICT_IGNORE_SUFFIX))) {
         EbErrorType ret = svt_av1_enc_parse_parameter(param, en->key, en->value);
diff --git a/tests/fate/enc_external.mak b/tests/fate/enc_external.mak
index 7eabebcc51..d787941c16 100644
--- a/tests/fate/enc_external.mak
+++ b/tests/fate/enc_external.mak
@@ -2,5 +2,10 @@ FATE_ENC_EXTERNAL-$(call ENCDEC, LIBX264 H264, MOV, H264_DEMUXER) += fate-libx26
 fate-libx264-simple: CMD = enc_external $(TARGET_SAMPLES)/h264-conformance/BA1_Sony_D.jsv \
     mp4 "-c:v libx264" "-show_entries frame=width,height,pix_fmt,pts,pkt_dts -of flat"
 
+# test for SVT-AV1 MDCV and CLL passthrough during encoding
+FATE_ENC_EXTERNAL-$(call ENCDEC, LIBSVTAV1 HEVC, MOV, HEVC_DEMUXER LIBDAV1D_DECODER) += fate-libsvtav1-hdr10
+fate-libsvtav1-hdr10: CMD = enc_external $(TARGET_SAMPLES)/hevc/hdr10_plus_h265_sample.hevc \
+    mp4 "-c:v libsvtav1" "-show_frames -show_entries frame=side_data_list -of flat"
+
 FATE_SAMPLES_FFMPEG_FFPROBE += $(FATE_ENC_EXTERNAL-yes)
 fate-enc-external: $(FATE_ENC_EXTERNAL-yes)
diff --git a/tests/ref/fate/libsvtav1-hdr10 b/tests/ref/fate/libsvtav1-hdr10
new file mode 100644
index 0000000000..6f0d34903b
--- /dev/null
+++ b/tests/ref/fate/libsvtav1-hdr10
@@ -0,0 +1,14 @@
+frames.frame.0.side_data_list.side_data.0.side_data_type="Mastering display metadata"
+frames.frame.0.side_data_list.side_data.0.red_x="17367/65536"
+frames.frame.0.side_data_list.side_data.0.red_y="45220/65536"
+frames.frame.0.side_data_list.side_data.0.green_x="9830/65536"
+frames.frame.0.side_data_list.side_data.0.green_y="3932/65536"
+frames.frame.0.side_data_list.side_data.0.blue_x="44564/65536"
+frames.frame.0.side_data_list.side_data.0.blue_y="20972/65536"
+frames.frame.0.side_data_list.side_data.0.white_point_x="20493/65536"
+frames.frame.0.side_data_list.side_data.0.white_point_y="21561/65536"
+frames.frame.0.side_data_list.side_data.0.min_luminance="82/16384"
+frames.frame.0.side_data_list.side_data.0.max_luminance="256000/256"
+frames.frame.0.side_data_list.side_data.1.side_data_type="Content light level metadata"
+frames.frame.0.side_data_list.side_data.1.max_content=1000
+frames.frame.0.side_data_list.side_data.1.max_average=200
-- 
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 v7 13/14] avcodec/libx264: add support for writing out CLL and MDCV

[Jan Ekström]

Both of these two structures were first available with X264_BUILD
163, so make relevant functionality conditional on the version
being at least such.

Keep handle_side_data available in all cases as this way X264_init
does not require additional version based conditions within it.

Finally, add a FATE test which verifies that pass-through of the
MDCV/CLL side data is working during encoding.
---
 configure                    |  2 +
 libavcodec/libx264.c         | 79 ++++++++++++++++++++++++++++++++++++
 tests/fate/enc_external.mak  |  5 +++
 tests/ref/fate/libx264-hdr10 | 15 +++++++
 4 files changed, 101 insertions(+)
 create mode 100644 tests/ref/fate/libx264-hdr10

diff --git a/configure b/configure
index bb5e630bad..f48cf46ffe 100755
--- a/configure
+++ b/configure
@@ -2527,6 +2527,7 @@ CONFIG_EXTRA="
     jpegtables
     lgplv3
     libx262
+    libx264_hdr10
     llauddsp
     llviddsp
     llvidencdsp
@@ -6927,6 +6928,7 @@ enabled libx264           && require_pkg_config libx264 x264 "stdint.h x264.h" x
                              require_cpp_condition libx264 x264.h "X264_BUILD >= 122" && {
                              [ "$toolchain" != "msvc" ] ||
                              require_cpp_condition libx264 x264.h "X264_BUILD >= 158"; } &&
+                             check_cpp_condition libx264_hdr10 x264.h "X264_BUILD >= 163" &&
                              check_cpp_condition libx262 x264.h "X264_MPEG2"
 enabled libx265           && require_pkg_config libx265 x265 x265.h x265_api_get &&
                              require_cpp_condition libx265 x265.h "X265_BUILD >= 89"
diff --git a/libavcodec/libx264.c b/libavcodec/libx264.c
index 10d646bd76..effdcfdb37 100644
--- a/libavcodec/libx264.c
+++ b/libavcodec/libx264.c
@@ -25,6 +25,7 @@
 #include "libavutil/eval.h"
 #include "libavutil/internal.h"
 #include "libavutil/opt.h"
+#include "libavutil/mastering_display_metadata.h"
 #include "libavutil/mem.h"
 #include "libavutil/pixdesc.h"
 #include "libavutil/stereo3d.h"
@@ -871,6 +872,82 @@ static int convert_pix_fmt(enum AVPixelFormat pix_fmt)
         return AVERROR(EINVAL);\
     }
 
+#if CONFIG_LIBX264_HDR10
+static void handle_mdcv(x264_param_t *params,
+                        const AVMasteringDisplayMetadata *mdcv)
+{
+    if (!mdcv->has_primaries && !mdcv->has_luminance)
+        return;
+
+    params->mastering_display.b_mastering_display = 1;
+
+    if (mdcv->has_primaries) {
+        int *const points[][2] = {
+            {
+                &params->mastering_display.i_red_x,
+                &params->mastering_display.i_red_y
+            },
+            {
+                &params->mastering_display.i_green_x,
+                &params->mastering_display.i_green_y
+            },
+            {
+                &params->mastering_display.i_blue_x,
+                &params->mastering_display.i_blue_y
+            },
+        };
+
+        for (int i = 0; i < 3; i++) {
+            const AVRational *src = mdcv->display_primaries[i];
+            int *dst[2] = { points[i][0], points[i][1] };
+
+            *dst[0] = av_rescale_q(1, src[0], (AVRational){ 1, 50000 });
+            *dst[1] = av_rescale_q(1, src[1], (AVRational){ 1, 50000 });
+        }
+
+        params->mastering_display.i_white_x =
+            av_rescale_q(1, mdcv->white_point[0], (AVRational){ 1, 50000 });
+        params->mastering_display.i_white_y =
+            av_rescale_q(1, mdcv->white_point[1], (AVRational){ 1, 50000 });
+    }
+
+    if (mdcv->has_luminance) {
+        params->mastering_display.i_display_max =
+            av_rescale_q(1, mdcv->max_luminance, (AVRational){ 1, 10000 });
+        params->mastering_display.i_display_min =
+            av_rescale_q(1, mdcv->min_luminance, (AVRational){ 1, 10000 });
+    }
+}
+#endif // CONFIG_LIBX264_HDR10
+
+static void handle_side_data(AVCodecContext *avctx, x264_param_t *params)
+{
+#if CONFIG_LIBX264_HDR10
+    const AVFrameSideData *cll_sd =
+        av_frame_side_data_get(
+            (const AVFrameSideData **)avctx->frame_side_data,
+            avctx->nb_frame_side_data, AV_FRAME_DATA_CONTENT_LIGHT_LEVEL);
+    const AVFrameSideData *mdcv_sd =
+        av_frame_side_data_get(
+            (const AVFrameSideData **)avctx->frame_side_data,
+            avctx->nb_frame_side_data, AV_FRAME_DATA_MASTERING_DISPLAY_METADATA);
+
+    if (cll_sd) {
+        const AVContentLightMetadata *cll =
+            (AVContentLightMetadata *)cll_sd->data;
+
+        params->content_light_level.i_max_cll  = cll->MaxCLL;
+        params->content_light_level.i_max_fall = cll->MaxFALL;
+
+        params->content_light_level.b_cll = 1;
+    }
+
+    if (mdcv_sd) {
+        handle_mdcv(params, (AVMasteringDisplayMetadata *)mdcv_sd->data);
+    }
+#endif // CONFIG_LIBX264_HDR10
+}
+
 static av_cold int X264_init(AVCodecContext *avctx)
 {
     X264Context *x4 = avctx->priv_data;
@@ -1171,6 +1248,8 @@ FF_ENABLE_DEPRECATION_WARNINGS
     if (avctx->chroma_sample_location != AVCHROMA_LOC_UNSPECIFIED)
         x4->params.vui.i_chroma_loc = avctx->chroma_sample_location - 1;
 
+    handle_side_data(avctx, &x4->params);
+
     if (avctx->flags & AV_CODEC_FLAG_GLOBAL_HEADER)
         x4->params.b_repeat_headers = 0;
 
diff --git a/tests/fate/enc_external.mak b/tests/fate/enc_external.mak
index d787941c16..4095a4b51a 100644
--- a/tests/fate/enc_external.mak
+++ b/tests/fate/enc_external.mak
@@ -7,5 +7,10 @@ FATE_ENC_EXTERNAL-$(call ENCDEC, LIBSVTAV1 HEVC, MOV, HEVC_DEMUXER LIBDAV1D_DECO
 fate-libsvtav1-hdr10: CMD = enc_external $(TARGET_SAMPLES)/hevc/hdr10_plus_h265_sample.hevc \
     mp4 "-c:v libsvtav1" "-show_frames -show_entries frame=side_data_list -of flat"
 
+# test for x264 MDCV and CLL passthrough during encoding
+FATE_ENC_EXTERNAL-$(call ENCDEC, LIBX264 HEVC, MOV, LIBX264_HDR10 HEVC_DEMUXER H264_DECODER) += fate-libx264-hdr10
+fate-libx264-hdr10: CMD = enc_external $(TARGET_SAMPLES)/hevc/hdr10_plus_h265_sample.hevc \
+    mp4 "-c:v libx264" "-show_frames -show_entries frame=side_data_list -of flat"
+
 FATE_SAMPLES_FFMPEG_FFPROBE += $(FATE_ENC_EXTERNAL-yes)
 fate-enc-external: $(FATE_ENC_EXTERNAL-yes)
diff --git a/tests/ref/fate/libx264-hdr10 b/tests/ref/fate/libx264-hdr10
new file mode 100644
index 0000000000..99c11677f0
--- /dev/null
+++ b/tests/ref/fate/libx264-hdr10
@@ -0,0 +1,15 @@
+frames.frame.0.side_data_list.side_data.0.side_data_type="H.26[45] User Data Unregistered SEI message"
+frames.frame.0.side_data_list.side_data.1.side_data_type="Mastering display metadata"
+frames.frame.0.side_data_list.side_data.1.red_x="13250/50000"
+frames.frame.0.side_data_list.side_data.1.red_y="34500/50000"
+frames.frame.0.side_data_list.side_data.1.green_x="7500/50000"
+frames.frame.0.side_data_list.side_data.1.green_y="3000/50000"
+frames.frame.0.side_data_list.side_data.1.blue_x="34000/50000"
+frames.frame.0.side_data_list.side_data.1.blue_y="16000/50000"
+frames.frame.0.side_data_list.side_data.1.white_point_x="15635/50000"
+frames.frame.0.side_data_list.side_data.1.white_point_y="16450/50000"
+frames.frame.0.side_data_list.side_data.1.min_luminance="50/10000"
+frames.frame.0.side_data_list.side_data.1.max_luminance="10000000/10000"
+frames.frame.0.side_data_list.side_data.2.side_data_type="Content light level metadata"
+frames.frame.0.side_data_list.side_data.2.max_content=1000
+frames.frame.0.side_data_list.side_data.2.max_average=200
-- 
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 v7 14/14] avcodec/libx265: add support for writing out CLL and MDCV

[Jan Ekström]

The newer of these two are the separate integers for content light
level, introduced in 3952bf3e98c76c31594529a3fe34e056d3e3e2ea ,
with X265_BUILD 75. As we already require X265_BUILD of at least
89, no further conditions are required.
---
 libavcodec/libx265.c         | 88 ++++++++++++++++++++++++++++++++++++
 tests/fate/enc_external.mak  |  5 ++
 tests/ref/fate/libx265-hdr10 | 16 +++++++
 3 files changed, 109 insertions(+)
 create mode 100644 tests/ref/fate/libx265-hdr10

diff --git a/libavcodec/libx265.c b/libavcodec/libx265.c
index 447e6da25f..8d366f51ab 100644
--- a/libavcodec/libx265.c
+++ b/libavcodec/libx265.c
@@ -28,9 +28,11 @@
 #include <float.h>
 
 #include "libavutil/avassert.h"
+#include "libavutil/bprint.h"
 #include "libavutil/buffer.h"
 #include "libavutil/internal.h"
 #include "libavutil/common.h"
+#include "libavutil/mastering_display_metadata.h"
 #include "libavutil/opt.h"
 #include "libavutil/pixdesc.h"
 #include "avcodec.h"
@@ -179,6 +181,85 @@ static av_cold int libx265_param_parse_int(AVCodecContext *avctx,
     return 0;
 }
 
+static int handle_mdcv(const AVClass **avcl, const x265_api *api,
+                       x265_param *params,
+                       const AVMasteringDisplayMetadata *mdcv)
+{
+    int ret = AVERROR_BUG;
+    AVBPrint buf;
+    av_bprint_init(&buf, 0, AV_BPRINT_SIZE_AUTOMATIC);
+
+    // G(%hu,%hu)B(%hu,%hu)R(%hu,%hu)WP(%hu,%hu)L(%u,%u)
+    av_bprintf(
+        &buf,
+        "G(%"PRId64",%"PRId64")B(%"PRId64",%"PRId64")R(%"PRId64",%"PRId64")"
+        "WP(%"PRId64",%"PRId64")L(%"PRId64",%"PRId64")",
+        av_rescale_q(1, mdcv->display_primaries[1][0], (AVRational){ 1, 50000 }),
+        av_rescale_q(1, mdcv->display_primaries[1][1], (AVRational){ 1, 50000 }),
+        av_rescale_q(1, mdcv->display_primaries[2][0], (AVRational){ 1, 50000 }),
+        av_rescale_q(1, mdcv->display_primaries[2][1], (AVRational){ 1, 50000 }),
+        av_rescale_q(1, mdcv->display_primaries[0][0], (AVRational){ 1, 50000 }),
+        av_rescale_q(1, mdcv->display_primaries[0][1], (AVRational){ 1, 50000 }),
+        av_rescale_q(1, mdcv->white_point[0], (AVRational){ 1, 50000 }),
+        av_rescale_q(1, mdcv->white_point[1], (AVRational){ 1, 50000 }),
+        av_rescale_q(1, mdcv->max_luminance,  (AVRational){ 1, 10000 }),
+        av_rescale_q(1, mdcv->min_luminance,  (AVRational){ 1, 10000 }));
+
+    if (!av_bprint_is_complete(&buf)) {
+        av_log(avcl, AV_LOG_ERROR,
+          "MDCV string too long for its available space!\n");
+        ret = AVERROR(ENOMEM);
+        goto end;
+    }
+
+    if (api->param_parse(params, "master-display", buf.str) ==
+            X265_PARAM_BAD_VALUE) {
+        av_log(avcl, AV_LOG_ERROR,
+               "Invalid value \"%s\" for param \"master-display\".\n",
+               buf.str);
+        ret = AVERROR(EINVAL);
+        goto end;
+    }
+
+    ret = 0;
+
+end:
+    av_bprint_finalize(&buf, NULL);
+
+    return ret;
+}
+
+static int handle_side_data(AVCodecContext *avctx, const x265_api *api,
+                            x265_param *params)
+{
+    const AVFrameSideData *cll_sd =
+        av_frame_side_data_get(
+            (const AVFrameSideData **)avctx->frame_side_data,
+            avctx->nb_frame_side_data, AV_FRAME_DATA_CONTENT_LIGHT_LEVEL);
+    const AVFrameSideData *mdcv_sd =
+        av_frame_side_data_get(
+            (const AVFrameSideData **)avctx->frame_side_data,
+            avctx->nb_frame_side_data, AV_FRAME_DATA_MASTERING_DISPLAY_METADATA);
+
+    if (cll_sd) {
+        const AVContentLightMetadata *cll =
+            (AVContentLightMetadata *)cll_sd->data;
+
+        params->maxCLL  = cll->MaxCLL;
+        params->maxFALL = cll->MaxFALL;
+    }
+
+    if (mdcv_sd) {
+        int ret = handle_mdcv(
+            &avctx->av_class, api, params,
+            (AVMasteringDisplayMetadata *)mdcv_sd->data);
+        if (ret < 0)
+            return ret;
+    }
+
+    return 0;
+}
+
 static av_cold int libx265_encode_init(AVCodecContext *avctx)
 {
     libx265Context *ctx = avctx->priv_data;
@@ -339,6 +420,13 @@ FF_ENABLE_DEPRECATION_WARNINGS
         return AVERROR_BUG;
     }
 
+    ret = handle_side_data(avctx, ctx->api, ctx->params);
+    if (ret < 0) {
+        av_log(avctx, AV_LOG_ERROR, "Failed handling side data! (%s)\n",
+               av_err2str(ret));
+        return ret;
+    }
+
     if (ctx->crf >= 0) {
         char crf[6];
 
diff --git a/tests/fate/enc_external.mak b/tests/fate/enc_external.mak
index 4095a4b51a..30021efbcd 100644
--- a/tests/fate/enc_external.mak
+++ b/tests/fate/enc_external.mak
@@ -12,5 +12,10 @@ FATE_ENC_EXTERNAL-$(call ENCDEC, LIBX264 HEVC, MOV, LIBX264_HDR10 HEVC_DEMUXER H
 fate-libx264-hdr10: CMD = enc_external $(TARGET_SAMPLES)/hevc/hdr10_plus_h265_sample.hevc \
     mp4 "-c:v libx264" "-show_frames -show_entries frame=side_data_list -of flat"
 
+# test for x265 MDCV and CLL passthrough during encoding
+FATE_ENC_EXTERNAL-$(call ENCDEC, LIBX265 HEVC, MOV, HEVC_DEMUXER) += fate-libx265-hdr10
+fate-libx265-hdr10: CMD = enc_external $(TARGET_SAMPLES)/hevc/hdr10_plus_h265_sample.hevc \
+    mp4 "-c:v libx265" "-show_frames -show_entries frame=side_data_list -of flat"
+
 FATE_SAMPLES_FFMPEG_FFPROBE += $(FATE_ENC_EXTERNAL-yes)
 fate-enc-external: $(FATE_ENC_EXTERNAL-yes)
diff --git a/tests/ref/fate/libx265-hdr10 b/tests/ref/fate/libx265-hdr10
new file mode 100644
index 0000000000..571c837cac
--- /dev/null
+++ b/tests/ref/fate/libx265-hdr10
@@ -0,0 +1,16 @@
+frames.frame.0.side_data_list.side_data.0.side_data_type="H.26[45] User Data Unregistered SEI message"
+frames.frame.0.side_data_list.side_data.1.side_data_type="H.26[45] User Data Unregistered SEI message"
+frames.frame.0.side_data_list.side_data.2.side_data_type="Mastering display metadata"
+frames.frame.0.side_data_list.side_data.2.red_x="13250/50000"
+frames.frame.0.side_data_list.side_data.2.red_y="34500/50000"
+frames.frame.0.side_data_list.side_data.2.green_x="7500/50000"
+frames.frame.0.side_data_list.side_data.2.green_y="3000/50000"
+frames.frame.0.side_data_list.side_data.2.blue_x="34000/50000"
+frames.frame.0.side_data_list.side_data.2.blue_y="16000/50000"
+frames.frame.0.side_data_list.side_data.2.white_point_x="15635/50000"
+frames.frame.0.side_data_list.side_data.2.white_point_y="16450/50000"
+frames.frame.0.side_data_list.side_data.2.min_luminance="50/10000"
+frames.frame.0.side_data_list.side_data.2.max_luminance="10000000/10000"
+frames.frame.0.side_data_list.side_data.3.side_data_type="Content light level metadata"
+frames.frame.0.side_data_list.side_data.3.max_content=1000
+frames.frame.0.side_data_list.side_data.3.max_average=200
-- 
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".