[FFmpeg-devel] [PATCH v3 00/12] encoder AVCodecContext configuration side data

[FFmpeg-devel] [PATCH v3 00/12] encoder AVCodecContext configuration side data

[Jan Ekström]

Differences to v2:
1. rebased on top of current master
2. addition of external encoder tests as the attached side data can now be
   parsed from all formats after the movement of side data from being HEVC
   specific to the common h2645 SEI parsing.
3. Various miscellaneous fixups/updates.

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 (12):
  avutil/frame: add AVFrameSideDataSet for passing sets of side data
  avutil/frame: split side data list wiping out to non-AVFrame function
  avutil/frame: add helper for uninitializing side data sets
  avutil/frame: split side_data_from_buf to base and AVFrame func
  avutil/frame: add helper for adding side data to set
  avutil/frame: add helper for getting side data from set
  avutil/frame: add helper for extending a set of side data
  avcodec: add side data set to AVCodecContext
  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

 fftools/ffmpeg_enc.c            |  11 +++
 libavcodec/avcodec.h            |   7 ++
 libavcodec/libsvtav1.c          |  70 ++++++++++++++++
 libavcodec/libx264.c            |  79 ++++++++++++++++++
 libavcodec/libx265.c            |  82 +++++++++++++++++++
 libavcodec/options.c            |   1 +
 libavutil/Makefile              |   1 +
 libavutil/frame.c               | 138 +++++++++++++++++++++++++++-----
 libavutil/frame.h               |  62 ++++++++++++++
 libavutil/tests/side_data_set.c |  71 ++++++++++++++++
 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    |   7 ++
 16 files changed, 571 insertions(+), 22 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

-- 
2.41.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 v3 01/12] avutil/frame: add AVFrameSideDataSet for passing sets of side data

[Jan Ekström]

---
 libavutil/frame.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/libavutil/frame.h b/libavutil/frame.h
index c0c1b23db7..6155226c1d 100644
--- a/libavutil/frame.h
+++ b/libavutil/frame.h
@@ -251,6 +251,14 @@ typedef struct AVFrameSideData {
     AVBufferRef *buf;
 } AVFrameSideData;
 
+/**
+ * Structure to hold a set of AVFrameSideData
+ */
+typedef struct AVFrameSideDataSet {
+    AVFrameSideData **sd;
+    int            nb_sd;
+} AVFrameSideDataSet;
+
 /**
  * Structure describing a single Region Of Interest.
  *
-- 
2.41.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 v3 02/12] 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 b6cee2d886..4b8481b756 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.41.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 v3 03/12] avutil/frame: add helper for uninitializing side data sets

[Jan Ekström]

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

diff --git a/libavutil/frame.c b/libavutil/frame.c
index 4b8481b756..c90bf5d9b1 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_side_data_set_uninit(AVFrameSideDataSet *set)
+{
+    wipe_side_data(&set->sd, &set->nb_sd);
+}
+
 AVFrame *av_frame_alloc(void)
 {
     AVFrame *frame = av_malloc(sizeof(*frame));
diff --git a/libavutil/frame.h b/libavutil/frame.h
index 6155226c1d..e5ca7af651 100644
--- a/libavutil/frame.h
+++ b/libavutil/frame.h
@@ -1057,6 +1057,14 @@ int av_frame_apply_cropping(AVFrame *frame, int flags);
  */
 const char *av_frame_side_data_name(enum AVFrameSideDataType type);
 
+/**
+ * Free all side data items and their contents, then zeroes out the
+ * struct values.
+ *
+ * @param set the set which should be uninitialized
+ */
+void av_side_data_set_uninit(AVFrameSideDataSet *set);
+
 /**
  * @}
  */
-- 
2.41.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 v3 04/12] avutil/frame: split side_data_from_buf to base and AVFrame func

[Jan Ekström]

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

diff --git a/libavutil/frame.c b/libavutil/frame.c
index c90bf5d9b1..46ea603511 100644
--- a/libavutil/frame.c
+++ b/libavutil/frame.c
@@ -787,23 +787,22 @@ 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(AVFrameSideDataSet *set,
+                                                      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 (set->nb_sd > INT_MAX / sizeof(*set->sd) - 1)
         return NULL;
 
-    tmp = av_realloc(frame->side_data,
-                     (frame->nb_side_data + 1) * sizeof(*frame->side_data));
+    tmp = av_realloc(set->sd, (set->nb_sd + 1) * sizeof(*set->sd));
     if (!tmp)
         return NULL;
-    frame->side_data = tmp;
+    set->sd = tmp;
 
     ret = av_mallocz(sizeof(*ret));
     if (!ret)
@@ -814,7 +813,23 @@ 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;
+    set->sd[set->nb_sd++] = ret;
+
+    return ret;
+}
+
+AVFrameSideData *av_frame_new_side_data_from_buf(AVFrame *frame,
+                                                 enum AVFrameSideDataType type,
+                                                 AVBufferRef *buf)
+{
+    AVFrameSideDataSet set = {
+        .sd    = frame->side_data,
+        .nb_sd = frame->nb_side_data,
+    };
+    AVFrameSideData *ret = add_side_data_to_set_from_buf(&set, type, buf);
+
+    frame->side_data    = set.sd;
+    frame->nb_side_data = set.nb_sd;
 
     return ret;
 }
-- 
2.41.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 v3 05/12] avutil/frame: add helper for adding side data to set

[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               | 34 ++++++++++++++++
 libavutil/frame.h               | 18 +++++++++
 libavutil/tests/side_data_set.c | 71 +++++++++++++++++++++++++++++++++
 tests/fate/libavutil.mak        |  4 ++
 tests/ref/fate/side_data_set    |  7 ++++
 6 files changed, 135 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 7828c94dc5..339eaf3539 100644
--- a/libavutil/Makefile
+++ b/libavutil/Makefile
@@ -264,6 +264,7 @@ TESTPROGS = adler32                                                     \
             ripemd                                                      \
             sha                                                         \
             sha512                                                      \
+            side_data_set                                               \
             softfloat                                                   \
             tree                                                        \
             twofish                                                     \
diff --git a/libavutil/frame.c b/libavutil/frame.c
index 46ea603511..27ccbc52c7 100644
--- a/libavutil/frame.c
+++ b/libavutil/frame.c
@@ -846,6 +846,40 @@ AVFrameSideData *av_frame_new_side_data(AVFrame *frame,
     return ret;
 }
 
+
+AVFrameSideData *av_side_data_set_new_item(AVFrameSideDataSet *set,
+                                           enum AVFrameSideDataType type,
+                                           size_t size,
+                                           unsigned int allow_duplicates)
+{
+    AVBufferRef     *buf = av_buffer_alloc(size);
+    AVFrameSideData *ret = NULL;
+
+    if (!allow_duplicates) {
+        for (int i = 0; i < set->nb_sd; i++) {
+            if (set->sd[i]->type != type)
+                continue;
+
+            free_side_data(&set->sd[i]);
+
+            for (int j = i + 1; j < set->nb_sd; j++) {
+                set->sd[j - 1] = set->sd[j];
+            }
+
+            // finally, cause a retry of the same index and update state
+            // regarding there being one less side data item in the set.
+            i--;
+            set->nb_sd--;
+        }
+    }
+
+    ret = add_side_data_to_set_from_buf(set, 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 e5ca7af651..b9d7c76461 100644
--- a/libavutil/frame.h
+++ b/libavutil/frame.h
@@ -1065,6 +1065,24 @@ const char *av_frame_side_data_name(enum AVFrameSideDataType type);
  */
 void av_side_data_set_uninit(AVFrameSideDataSet *set);
 
+/**
+ * Add a new side data entry to a set.
+ *
+ * @param set a set to which the side data should be added
+ * @param type type of the added side data
+ * @param size size of the side data
+ * @param allow_duplicates an unsigned integer noting whether duplicates are
+ *                         allowed or not. If duplicates are not allowed, all
+ *                         entries of the same side data type are first removed
+ *                         and freed before the new entry is added.
+ *
+ * @return newly added side data on success, NULL on error
+ */
+AVFrameSideData *av_side_data_set_new_item(AVFrameSideDataSet *set,
+                                           enum AVFrameSideDataType type,
+                                           size_t size,
+                                           unsigned int allow_duplicates);
+
 /**
  * @}
  */
diff --git a/libavutil/tests/side_data_set.c b/libavutil/tests/side_data_set.c
new file mode 100644
index 0000000000..ad420440d5
--- /dev/null
+++ b/libavutil/tests/side_data_set.c
@@ -0,0 +1,71 @@
+/*
+ * 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 AVFrameSideDataSet set)
+{
+    for (int i = 0; i < set.nb_sd; i++) {
+        AVFrameSideData *sd = set.sd[i];
+
+        if (sd->type != AV_FRAME_DATA_CONTENT_LIGHT_LEVEL)
+            continue;
+
+        printf("sd %d, MaxCLL: %u\n",
+               i, ((AVContentLightMetadata *)sd->data)->MaxCLL);
+    }
+}
+
+int main(void)
+{
+    AVFrameSideDataSet set = { 0 };
+
+    for (int value = 1; value < 4; value++) {
+        AVFrameSideData *sd = av_side_data_set_new_item(
+            &set, AV_FRAME_DATA_CONTENT_LIGHT_LEVEL,
+            sizeof(AVContentLightMetadata), 1);
+
+        av_assert0(sd);
+
+        ((AVContentLightMetadata *)sd->data)->MaxCLL = value;
+    }
+
+    puts("Initial addition results with duplicates:");
+    print_clls(set);
+
+    {
+        AVFrameSideData *sd = av_side_data_set_new_item(
+            &set, AV_FRAME_DATA_CONTENT_LIGHT_LEVEL,
+            sizeof(AVContentLightMetadata), 0);
+
+        av_assert0(sd);
+
+        ((AVContentLightMetadata *)sd->data)->MaxCLL = 1337;
+    }
+
+    puts("\nFinal state after a single 'no-duplicates' addition:");
+    print_clls(set);
+
+    av_side_data_set_uninit(&set);
+
+    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..6fb95d3f7c
--- /dev/null
+++ b/tests/ref/fate/side_data_set
@@ -0,0 +1,7 @@
+Initial addition results with duplicates:
+sd 0, MaxCLL: 1
+sd 1, MaxCLL: 2
+sd 2, MaxCLL: 3
+
+Final state after a single 'no-duplicates' addition:
+sd 0, MaxCLL: 1337
-- 
2.41.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 v3 05/12] avutil/frame: add helper for adding side data to set

[Andreas Rheinhardt]

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               | 34 ++++++++++++++++
>  libavutil/frame.h               | 18 +++++++++
>  libavutil/tests/side_data_set.c | 71 +++++++++++++++++++++++++++++++++
>  tests/fate/libavutil.mak        |  4 ++
>  tests/ref/fate/side_data_set    |  7 ++++
>  6 files changed, 135 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 7828c94dc5..339eaf3539 100644
> --- a/libavutil/Makefile
> +++ b/libavutil/Makefile
> @@ -264,6 +264,7 @@ TESTPROGS = adler32                                                     \
>              ripemd                                                      \
>              sha                                                         \
>              sha512                                                      \
> +            side_data_set                                               \
>              softfloat                                                   \
>              tree                                                        \
>              twofish                                                     \
> diff --git a/libavutil/frame.c b/libavutil/frame.c
> index 46ea603511..27ccbc52c7 100644
> --- a/libavutil/frame.c
> +++ b/libavutil/frame.c
> @@ -846,6 +846,40 @@ AVFrameSideData *av_frame_new_side_data(AVFrame *frame,
>      return ret;
>  }
>  
> +
> +AVFrameSideData *av_side_data_set_new_item(AVFrameSideDataSet *set,
> +                                           enum AVFrameSideDataType type,
> +                                           size_t size,
> +                                           unsigned int allow_duplicates)
> +{
> +    AVBufferRef     *buf = av_buffer_alloc(size);
> +    AVFrameSideData *ret = NULL;
> +
> +    if (!allow_duplicates) {
> +        for (int i = 0; i < set->nb_sd; i++) {
> +            if (set->sd[i]->type != type)
> +                continue;
> +
> +            free_side_data(&set->sd[i]);
> +
> +            for (int j = i + 1; j < set->nb_sd; j++) {
> +                set->sd[j - 1] = set->sd[j];

Ever heard of memmove?
(Alternatively, if the order is not to be preserved, one could simply
move the last side data to the just freed slot, similarly how
av_frame_remove_side_data() does it.)

> +            }
> +
> +            // finally, cause a retry of the same index and update state
> +            // regarding there being one less side data item in the set.
> +            i--;

In code like this, it is easier to count down instead of up. This also
has the advantage that entries that are removed later are not
unnecessarily moved.

> +            set->nb_sd--;
> +        }
> +    }
> +
> +    ret = add_side_data_to_set_from_buf(set, 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 e5ca7af651..b9d7c76461 100644
> --- a/libavutil/frame.h
> +++ b/libavutil/frame.h
> @@ -1065,6 +1065,24 @@ const char *av_frame_side_data_name(enum AVFrameSideDataType type);
>   */
>  void av_side_data_set_uninit(AVFrameSideDataSet *set);
>  
> +/**
> + * Add a new side data entry to a set.
> + *
> + * @param set a set to which the side data should be added
> + * @param type type of the added side data
> + * @param size size of the side data
> + * @param allow_duplicates an unsigned integer noting whether duplicates are
> + *                         allowed or not. If duplicates are not allowed, all
> + *                         entries of the same side data type are first removed
> + *                         and freed before the new entry is added.

Why not flags?

> + *
> + * @return newly added side data on success, NULL on error

This does not mention whether duplicates have been removed on error (if
allow_duplicates == 0).

> + */
> +AVFrameSideData *av_side_data_set_new_item(AVFrameSideDataSet *set,
> +                                           enum AVFrameSideDataType type,
> +                                           size_t size,
> +                                           unsigned int allow_duplicates);
> +
>  /**
>   * @}
>   */
> diff --git a/libavutil/tests/side_data_set.c b/libavutil/tests/side_data_set.c
> new file mode 100644
> index 0000000000..ad420440d5
> --- /dev/null
> +++ b/libavutil/tests/side_data_set.c
> @@ -0,0 +1,71 @@
> +/*
> + * 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 AVFrameSideDataSet set)
> +{
> +    for (int i = 0; i < set.nb_sd; i++) {
> +        AVFrameSideData *sd = set.sd[i];
> +
> +        if (sd->type != AV_FRAME_DATA_CONTENT_LIGHT_LEVEL)
> +            continue;
> +
> +        printf("sd %d, MaxCLL: %u\n",
> +               i, ((AVContentLightMetadata *)sd->data)->MaxCLL);
> +    }
> +}
> +
> +int main(void)
> +{
> +    AVFrameSideDataSet set = { 0 };
> +
> +    for (int value = 1; value < 4; value++) {
> +        AVFrameSideData *sd = av_side_data_set_new_item(
> +            &set, AV_FRAME_DATA_CONTENT_LIGHT_LEVEL,
> +            sizeof(AVContentLightMetadata), 1);
> +
> +        av_assert0(sd);
> +
> +        ((AVContentLightMetadata *)sd->data)->MaxCLL = value;
> +    }
> +
> +    puts("Initial addition results with duplicates:");
> +    print_clls(set);
> +
> +    {
> +        AVFrameSideData *sd = av_side_data_set_new_item(
> +            &set, AV_FRAME_DATA_CONTENT_LIGHT_LEVEL,
> +            sizeof(AVContentLightMetadata), 0);
> +
> +        av_assert0(sd);
> +
> +        ((AVContentLightMetadata *)sd->data)->MaxCLL = 1337;
> +    }
> +
> +    puts("\nFinal state after a single 'no-duplicates' addition:");
> +    print_clls(set);
> +
> +    av_side_data_set_uninit(&set);
> +
> +    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..6fb95d3f7c
> --- /dev/null
> +++ b/tests/ref/fate/side_data_set
> @@ -0,0 +1,7 @@
> +Initial addition results with duplicates:
> +sd 0, MaxCLL: 1
> +sd 1, MaxCLL: 2
> +sd 2, MaxCLL: 3
> +
> +Final state after a single 'no-duplicates' addition:
> +sd 0, MaxCLL: 1337

_______________________________________________
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 v3 06/12] avutil/frame: add helper for getting side data from set

[Jan Ekström]

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

diff --git a/libavutil/frame.c b/libavutil/frame.c
index 27ccbc52c7..d8910a2120 100644
--- a/libavutil/frame.c
+++ b/libavutil/frame.c
@@ -880,16 +880,28 @@ AVFrameSideData *av_side_data_set_new_item(AVFrameSideDataSet *set,
     return ret;
 }
 
-AVFrameSideData *av_frame_get_side_data(const AVFrame *frame,
-                                        enum AVFrameSideDataType type)
+AVFrameSideData *av_side_data_set_get_item(const AVFrameSideDataSet set,
+                                           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 < set.nb_sd; i++) {
+        if (set.sd[i]->type == type)
+            return set.sd[i];
     }
     return NULL;
 }
 
+AVFrameSideData *av_frame_get_side_data(const AVFrame *frame,
+                                        enum AVFrameSideDataType type)
+{
+    return av_side_data_set_get_item(
+        (const AVFrameSideDataSet){
+            .sd    = frame->side_data,
+            .nb_sd = frame->nb_side_data
+        },
+        type
+    );
+}
+
 static int frame_copy_video(AVFrame *dst, const AVFrame *src)
 {
     const uint8_t *src_data[4];
diff --git a/libavutil/frame.h b/libavutil/frame.h
index b9d7c76461..0cafc9c51f 100644
--- a/libavutil/frame.h
+++ b/libavutil/frame.h
@@ -1083,6 +1083,18 @@ AVFrameSideData *av_side_data_set_new_item(AVFrameSideDataSet *set,
                                            size_t size,
                                            unsigned int allow_duplicates);
 
+/**
+ * Get a side data entry of a specific type from a set.
+ *
+ * @param set the set from which side data should be queried from
+ * @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.
+ */
+AVFrameSideData *av_side_data_set_get_item(const AVFrameSideDataSet set,
+                                           enum AVFrameSideDataType type);
+
 /**
  * @}
  */
-- 
2.41.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 v3 07/12] avutil/frame: add helper for extending a set of side data

[Jan Ekström]

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

diff --git a/libavutil/frame.c b/libavutil/frame.c
index d8910a2120..04d56853f0 100644
--- a/libavutil/frame.c
+++ b/libavutil/frame.c
@@ -880,6 +880,29 @@ AVFrameSideData *av_side_data_set_new_item(AVFrameSideDataSet *set,
     return ret;
 }
 
+int av_side_data_set_extend(AVFrameSideDataSet *dst,
+                            const AVFrameSideDataSet src,
+                            unsigned int allow_duplicates)
+{
+    for (int i = 0; i < src.nb_sd; i++) {
+        const AVFrameSideData *sd_src = src.sd[i];
+        AVFrameSideData *sd_dst =
+            av_side_data_set_new_item(dst, sd_src->type,
+                                      sd_src->size,
+                                      allow_duplicates);
+        if (!sd_dst) {
+            av_side_data_set_uninit(dst);
+            return AVERROR(ENOMEM);
+        }
+
+        memcpy(sd_dst->data, sd_src->data, sd_src->size);
+
+        av_dict_copy(&sd_dst->metadata, sd_src->metadata, 0);
+    }
+
+    return 0;
+}
+
 AVFrameSideData *av_side_data_set_get_item(const AVFrameSideDataSet set,
                                            enum AVFrameSideDataType type)
 {
diff --git a/libavutil/frame.h b/libavutil/frame.h
index 0cafc9c51f..2413000c9a 100644
--- a/libavutil/frame.h
+++ b/libavutil/frame.h
@@ -1083,6 +1083,22 @@ AVFrameSideData *av_side_data_set_new_item(AVFrameSideDataSet *set,
                                            size_t size,
                                            unsigned int allow_duplicates);
 
+/**
+ * Add multiple side data entries to a set in one go.
+ *
+ * @param dst a set to which the side data should be added
+ * @param src a set from which the side data should be copied from
+ * @param allow_duplicates an unsigned integer noting whether duplicates are
+ *                         allowed or not. If duplicates are not allowed, all
+ *                         entries of the same side data type are first removed
+ *                         and freed before the new entry is added.
+ *
+ * @return negative error code on failure, >=0 on success.
+ */
+int av_side_data_set_extend(AVFrameSideDataSet *dst,
+                            const AVFrameSideDataSet src,
+                            unsigned int allow_duplicates);
+
 /**
  * Get a side data entry of a specific type from a set.
  *
-- 
2.41.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 v3 07/12] avutil/frame: add helper for extending a set of side data

[Andreas Rheinhardt]

Jan Ekström:
> ---
>  libavutil/frame.c | 23 +++++++++++++++++++++++
>  libavutil/frame.h | 16 ++++++++++++++++
>  2 files changed, 39 insertions(+)
> 
> diff --git a/libavutil/frame.c b/libavutil/frame.c
> index d8910a2120..04d56853f0 100644
> --- a/libavutil/frame.c
> +++ b/libavutil/frame.c
> @@ -880,6 +880,29 @@ AVFrameSideData *av_side_data_set_new_item(AVFrameSideDataSet *set,
>      return ret;
>  }
>  
> +int av_side_data_set_extend(AVFrameSideDataSet *dst,
> +                            const AVFrameSideDataSet src,
> +                            unsigned int allow_duplicates)
> +{
> +    for (int i = 0; i < src.nb_sd; i++) {
> +        const AVFrameSideData *sd_src = src.sd[i];
> +        AVFrameSideData *sd_dst =
> +            av_side_data_set_new_item(dst, sd_src->type,
> +                                      sd_src->size,
> +                                      allow_duplicates);
> +        if (!sd_dst) {
> +            av_side_data_set_uninit(dst);
> +            return AVERROR(ENOMEM);
> +        }
> +
> +        memcpy(sd_dst->data, sd_src->data, sd_src->size);

AVFrame side data is reference-counted.

> +
> +        av_dict_copy(&sd_dst->metadata, sd_src->metadata, 0);

Missing check.

> +    }
> +
> +    return 0;
> +}
> +
>  AVFrameSideData *av_side_data_set_get_item(const AVFrameSideDataSet set,
>                                             enum AVFrameSideDataType type)
>  {
> diff --git a/libavutil/frame.h b/libavutil/frame.h
> index 0cafc9c51f..2413000c9a 100644
> --- a/libavutil/frame.h
> +++ b/libavutil/frame.h
> @@ -1083,6 +1083,22 @@ AVFrameSideData *av_side_data_set_new_item(AVFrameSideDataSet *set,
>                                             size_t size,
>                                             unsigned int allow_duplicates);
>  
> +/**
> + * Add multiple side data entries to a set in one go.
> + *
> + * @param dst a set to which the side data should be added
> + * @param src a set from which the side data should be copied from

This needs to add something to ensure that dst and src refer to
different side-data (i.e. to disallow calls like
AVFrameSideDataSet set = { ... };

av_side_data_set_extend(&set, set, 0);)

> + * @param allow_duplicates an unsigned integer noting whether duplicates are
> + *                         allowed or not. If duplicates are not allowed, all
> + *                         entries of the same side data type are first removed
> + *                         and freed before the new entry is added.

Better use flags.

> + *
> + * @return negative error code on failure, >=0 on success.
> + */
> +int av_side_data_set_extend(AVFrameSideDataSet *dst,
> +                            const AVFrameSideDataSet src,
> +                            unsigned int allow_duplicates);

Do we really need this function? Are there enough potential users of it?

> +
>  /**
>   * Get a side data entry of a specific type from a set.
>   *

_______________________________________________
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 v3 07/12] avutil/frame: add helper for extending a set of side data

[Jan Ekström]

On Sun, Aug 20, 2023 at 12:44 PM Andreas Rheinhardt
<andreas.rheinhardt@outlook.com> wrote:
>
> Jan Ekström:
> > ---
> >  libavutil/frame.c | 23 +++++++++++++++++++++++
> >  libavutil/frame.h | 16 ++++++++++++++++
> >  2 files changed, 39 insertions(+)
> >
> > diff --git a/libavutil/frame.c b/libavutil/frame.c
> > index d8910a2120..04d56853f0 100644
> > --- a/libavutil/frame.c
> > +++ b/libavutil/frame.c
> > @@ -880,6 +880,29 @@ AVFrameSideData *av_side_data_set_new_item(AVFrameSideDataSet *set,
> >      return ret;
> >  }
> >
> > +int av_side_data_set_extend(AVFrameSideDataSet *dst,
> > +                            const AVFrameSideDataSet src,
> > +                            unsigned int allow_duplicates)
> > +{
> > +    for (int i = 0; i < src.nb_sd; i++) {
> > +        const AVFrameSideData *sd_src = src.sd[i];
> > +        AVFrameSideData *sd_dst =
> > +            av_side_data_set_new_item(dst, sd_src->type,
> > +                                      sd_src->size,
> > +                                      allow_duplicates);
> > +        if (!sd_dst) {
> > +            av_side_data_set_uninit(dst);
> > +            return AVERROR(ENOMEM);
> > +        }
> > +
> > +        memcpy(sd_dst->data, sd_src->data, sd_src->size);
>
> AVFrame side data is reference-counted.
>

Seems like I based this on av_frame_copy_props, so the force_copy=1
case. I guess following av_frame_ref makes more sense to follow.

> > +
> > +        av_dict_copy(&sd_dst->metadata, sd_src->metadata, 0);
>
> Missing check.
>

This comes straight out of frame_copy_props, so it seems like at least
a couple of av_dict_copy calls within it are unchecked :) .

I guess that warrants a separate fixup patch.

> > +    }
> > +
> > +    return 0;
> > +}
> > +
> >  AVFrameSideData *av_side_data_set_get_item(const AVFrameSideDataSet set,
> >                                             enum AVFrameSideDataType type)
> >  {
> > diff --git a/libavutil/frame.h b/libavutil/frame.h
> > index 0cafc9c51f..2413000c9a 100644
> > --- a/libavutil/frame.h
> > +++ b/libavutil/frame.h
> > @@ -1083,6 +1083,22 @@ AVFrameSideData *av_side_data_set_new_item(AVFrameSideDataSet *set,
> >                                             size_t size,
> >                                             unsigned int allow_duplicates);
> >
> > +/**
> > + * Add multiple side data entries to a set in one go.
> > + *
> > + * @param dst a set to which the side data should be added
> > + * @param src a set from which the side data should be copied from
>
> This needs to add something to ensure that dst and src refer to
> different side-data (i.e. to disallow calls like
> AVFrameSideDataSet set = { ... };
>
> av_side_data_set_extend(&set, set, 0);)
>

Sure, but interestingly enough only av_frame_replace currently has
something like this. av_frame_ref does check that dst has zero
width/height or channel count, but does not attempt to check that it
is being called with different contents.

> > + * @param allow_duplicates an unsigned integer noting whether duplicates are
> > + *                         allowed or not. If duplicates are not allowed, all
> > + *                         entries of the same side data type are first removed
> > + *                         and freed before the new entry is added.
>
> Better use flags.
>

Done, the current state of the branch as I go through people's reviews
is available at
https://github.com/jeeb/ffmpeg/commits/avcodec_cll_mdcv_side_data_v4 .

> > + *
> > + * @return negative error code on failure, >=0 on success.
> > + */
> > +int av_side_data_set_extend(AVFrameSideDataSet *dst,
> > +                            const AVFrameSideDataSet src,
> > +                            unsigned int allow_duplicates);
>
> Do we really need this function? Are there enough potential users of it?
>

I mostly added this for ffmpeg.c and if we want other API clients to
do something similar, there should be a public function to allow for
following similar behavior.

Or would you rather prefer a function related to avctx that takes in
an AVFrame, which does something like this internally?

Best regards,
Jan
_______________________________________________
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 v3 08/12] avcodec: add side data set to AVCodecContext

[Jan Ekström]

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

diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
index 649411ac79..500a8f3e49 100644
--- a/libavcodec/avcodec.h
+++ b/libavcodec/avcodec.h
@@ -2100,6 +2100,13 @@ 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
+     */
+    AVFrameSideDataSet side_data_set;
 } AVCodecContext;
 
 /**
diff --git a/libavcodec/options.c b/libavcodec/options.c
index a9b35ee1c3..fff2c53ae3 100644
--- a/libavcodec/options.c
+++ b/libavcodec/options.c
@@ -180,6 +180,7 @@ void avcodec_free_context(AVCodecContext **pavctx)
     av_freep(&avctx->inter_matrix);
     av_freep(&avctx->rc_override);
     av_channel_layout_uninit(&avctx->ch_layout);
+    av_side_data_set_uninit(&avctx->side_data_set);
 
     av_freep(pavctx);
 }
-- 
2.41.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 v3 09/12] 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 | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/fftools/ffmpeg_enc.c b/fftools/ffmpeg_enc.c
index 96424272bf..de60a14868 100644
--- a/fftools/ffmpeg_enc.c
+++ b/fftools/ffmpeg_enc.c
@@ -314,6 +314,17 @@ int enc_open(OutputStream *ost, AVFrame *frame)
         enc_ctx->colorspace             = frame->colorspace;
         enc_ctx->chroma_sample_location = frame->chroma_location;
 
+        ret = av_side_data_set_extend(&enc_ctx->side_data_set,
+                                      (const AVFrameSideDataSet){
+                                         .sd    = frame->side_data,
+                                         .nb_sd = frame->nb_side_data
+                                      }, 0);
+        if (ret < 0) {
+            av_log(NULL, AV_LOG_ERROR, "failed to configure video encoder: %s!\n",
+                   av_err2str(ret));
+            return ret;
+        }
+
         enc_ctx->framerate = fr;
 
         ost->st->avg_frame_rate = fr;
-- 
2.41.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 v3 10/12] 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         | 70 ++++++++++++++++++++++++++++++++++
 tests/fate/enc_external.mak    |  5 +++
 tests/ref/fate/libsvtav1-hdr10 | 14 +++++++
 3 files changed, 89 insertions(+)
 create mode 100644 tests/ref/fate/libsvtav1-hdr10

diff --git a/libavcodec/libsvtav1.c b/libavcodec/libsvtav1.c
index f2b73361d8..d4f9fa14ba 100644
--- a/libavcodec/libsvtav1.c
+++ b/libavcodec/libsvtav1.c
@@ -24,9 +24,11 @@
 #include <EbSvtAv1ErrorCodes.h>
 #include <EbSvtAv1Enc.h>
 
+#include "libavutil/bswap.h"
 #include "libavutil/common.h"
 #include "libavutil/frame.h"
 #include "libavutil/imgutils.h"
+#include "libavutil/mastering_display_metadata.h"
 #include "libavutil/opt.h"
 #include "libavutil/pixdesc.h"
 #include "libavutil/avassert.h"
@@ -146,6 +148,72 @@ static int alloc_buffer(EbSvtAv1EncConfiguration *config, SvtContext *svt_enc)
 
 }
 
+static void handle_mdcv(struct EbSvtAv1MasteringDisplayInfo *dst,
+                        const AVMasteringDisplayMetadata *mdcv)
+{
+    struct EbSvtAv1ChromaPoints *points[] = {
+        &dst->r,
+        &dst->g,
+        &dst->b,
+    };
+
+    if (!mdcv->has_primaries)
+        goto skip_primaries;
+
+    for (int i = 0; i < 3; i++) {
+        struct EbSvtAv1ChromaPoints *dst = points[i];
+        const  AVRational           *src = mdcv->display_primaries[i];
+
+        dst->x =
+            AV_BSWAP16C(av_rescale_q(1, src[0],
+                                     (AVRational){ 1, (1 << 16) }));
+        dst->y =
+            AV_BSWAP16C(av_rescale_q(1, src[1],
+                                     (AVRational){ 1, (1 << 16) }));
+    }
+
+    dst->white_point.x =
+        AV_BSWAP16C(av_rescale_q(1, mdcv->white_point[0],
+                                 (AVRational){ 1, (1 << 16) }));
+    dst->white_point.y =
+        AV_BSWAP16C(av_rescale_q(1, mdcv->white_point[1],
+                                 (AVRational){ 1, (1 << 16) }));
+
+skip_primaries:
+    if (!mdcv->has_luminance)
+        return;
+
+    dst->max_luma =
+        AV_BSWAP32C(av_rescale_q(1, mdcv->max_luminance,
+                                 (AVRational){ 1, (1 << 8) }));
+    dst->min_luma =
+        AV_BSWAP32C(av_rescale_q(1, mdcv->min_luminance,
+                                 (AVRational){ 1, (1 << 14) }));
+}
+
+static void handle_side_data(AVCodecContext *avctx,
+                             EbSvtAv1EncConfiguration *param)
+{
+    const AVFrameSideDataSet set = avctx->side_data_set;
+    const AVFrameSideData *cll_sd =
+        av_side_data_set_get_item(set, AV_FRAME_DATA_CONTENT_LIGHT_LEVEL);
+    const AVFrameSideData *mdcv_sd =
+        av_side_data_set_get_item(set, AV_FRAME_DATA_MASTERING_DISPLAY_METADATA);
+
+    if (cll_sd) {
+        const AVContentLightMetadata *cll =
+            (AVContentLightMetadata *)cll_sd->data;
+
+        param->content_light_level.max_cll  = AV_BSWAP16C(cll->MaxCLL);
+        param->content_light_level.max_fall = AV_BSWAP16C(cll->MaxFALL);
+    }
+
+    if (mdcv_sd) {
+        handle_mdcv(&param->mastering_display,
+                    (AVMasteringDisplayMetadata *)mdcv_sd->data);
+    }
+}
+
 static int config_enc_params(EbSvtAv1EncConfiguration *param,
                              AVCodecContext *avctx)
 {
@@ -273,6 +341,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.41.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 v3 10/12] avcodec/libsvtav1: add support for writing out CLL and MDCV

[Andreas Rheinhardt]

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         | 70 ++++++++++++++++++++++++++++++++++
>  tests/fate/enc_external.mak    |  5 +++
>  tests/ref/fate/libsvtav1-hdr10 | 14 +++++++
>  3 files changed, 89 insertions(+)
>  create mode 100644 tests/ref/fate/libsvtav1-hdr10
> 
> diff --git a/libavcodec/libsvtav1.c b/libavcodec/libsvtav1.c
> index f2b73361d8..d4f9fa14ba 100644
> --- a/libavcodec/libsvtav1.c
> +++ b/libavcodec/libsvtav1.c
> @@ -24,9 +24,11 @@
>  #include <EbSvtAv1ErrorCodes.h>
>  #include <EbSvtAv1Enc.h>
>  
> +#include "libavutil/bswap.h"
>  #include "libavutil/common.h"
>  #include "libavutil/frame.h"
>  #include "libavutil/imgutils.h"
> +#include "libavutil/mastering_display_metadata.h"
>  #include "libavutil/opt.h"
>  #include "libavutil/pixdesc.h"
>  #include "libavutil/avassert.h"
> @@ -146,6 +148,72 @@ static int alloc_buffer(EbSvtAv1EncConfiguration *config, SvtContext *svt_enc)
>  
>  }
>  
> +static void handle_mdcv(struct EbSvtAv1MasteringDisplayInfo *dst,
> +                        const AVMasteringDisplayMetadata *mdcv)
> +{
> +    struct EbSvtAv1ChromaPoints *points[] = {
> +        &dst->r,
> +        &dst->g,
> +        &dst->b,
> +    };
> +
> +    if (!mdcv->has_primaries)
> +        goto skip_primaries;
> +
> +    for (int i = 0; i < 3; i++) {
> +        struct EbSvtAv1ChromaPoints *dst = points[i];
> +        const  AVRational           *src = mdcv->display_primaries[i];
> +
> +        dst->x =
> +            AV_BSWAP16C(av_rescale_q(1, src[0],
> +                                     (AVRational){ 1, (1 << 16) }));

Can you explain what's the matter with the AV_BSWAP16C here? And if I am
not mistaken, then the av_rescale_q() above is equivalent to src[0] * (1
<< 16) (if we had multiplications of AVRationals by integers).

> +        dst->y =
> +            AV_BSWAP16C(av_rescale_q(1, src[1],
> +                                     (AVRational){ 1, (1 << 16) }));
> +    }
> +
> +    dst->white_point.x =
> +        AV_BSWAP16C(av_rescale_q(1, mdcv->white_point[0],
> +                                 (AVRational){ 1, (1 << 16) }));
> +    dst->white_point.y =
> +        AV_BSWAP16C(av_rescale_q(1, mdcv->white_point[1],
> +                                 (AVRational){ 1, (1 << 16) }));
> +
> +skip_primaries:
> +    if (!mdcv->has_luminance)
> +        return;
> +
> +    dst->max_luma =
> +        AV_BSWAP32C(av_rescale_q(1, mdcv->max_luminance,
> +                                 (AVRational){ 1, (1 << 8) }));
> +    dst->min_luma =
> +        AV_BSWAP32C(av_rescale_q(1, mdcv->min_luminance,
> +                                 (AVRational){ 1, (1 << 14) }));
> +}
> +
> +static void handle_side_data(AVCodecContext *avctx,
> +                             EbSvtAv1EncConfiguration *param)
> +{
> +    const AVFrameSideDataSet set = avctx->side_data_set;
> +    const AVFrameSideData *cll_sd =
> +        av_side_data_set_get_item(set, AV_FRAME_DATA_CONTENT_LIGHT_LEVEL);
> +    const AVFrameSideData *mdcv_sd =
> +        av_side_data_set_get_item(set, AV_FRAME_DATA_MASTERING_DISPLAY_METADATA);
> +
> +    if (cll_sd) {
> +        const AVContentLightMetadata *cll =
> +            (AVContentLightMetadata *)cll_sd->data;
> +
> +        param->content_light_level.max_cll  = AV_BSWAP16C(cll->MaxCLL);
> +        param->content_light_level.max_fall = AV_BSWAP16C(cll->MaxFALL);
> +    }
> +
> +    if (mdcv_sd) {
> +        handle_mdcv(&param->mastering_display,
> +                    (AVMasteringDisplayMetadata *)mdcv_sd->data);
> +    }
> +}
> +
>  static int config_enc_params(EbSvtAv1EncConfiguration *param,
>                               AVCodecContext *avctx)
>  {
> @@ -273,6 +341,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

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

[Jan Ekström]

On Sun, Aug 20, 2023 at 10:10 AM Andreas Rheinhardt
<andreas.rheinhardt@outlook.com> wrote:
>
> 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         | 70 ++++++++++++++++++++++++++++++++++
> >  tests/fate/enc_external.mak    |  5 +++
> >  tests/ref/fate/libsvtav1-hdr10 | 14 +++++++
> >  3 files changed, 89 insertions(+)
> >  create mode 100644 tests/ref/fate/libsvtav1-hdr10
> >
> > diff --git a/libavcodec/libsvtav1.c b/libavcodec/libsvtav1.c
> > index f2b73361d8..d4f9fa14ba 100644
> > --- a/libavcodec/libsvtav1.c
> > +++ b/libavcodec/libsvtav1.c
> > @@ -24,9 +24,11 @@
> >  #include <EbSvtAv1ErrorCodes.h>
> >  #include <EbSvtAv1Enc.h>
> >
> > +#include "libavutil/bswap.h"
> >  #include "libavutil/common.h"
> >  #include "libavutil/frame.h"
> >  #include "libavutil/imgutils.h"
> > +#include "libavutil/mastering_display_metadata.h"
> >  #include "libavutil/opt.h"
> >  #include "libavutil/pixdesc.h"
> >  #include "libavutil/avassert.h"
> > @@ -146,6 +148,72 @@ static int alloc_buffer(EbSvtAv1EncConfiguration *config, SvtContext *svt_enc)
> >
> >  }
> >
> > +static void handle_mdcv(struct EbSvtAv1MasteringDisplayInfo *dst,
> > +                        const AVMasteringDisplayMetadata *mdcv)
> > +{
> > +    struct EbSvtAv1ChromaPoints *points[] = {
> > +        &dst->r,
> > +        &dst->g,
> > +        &dst->b,
> > +    };
> > +
> > +    if (!mdcv->has_primaries)
> > +        goto skip_primaries;
> > +
> > +    for (int i = 0; i < 3; i++) {
> > +        struct EbSvtAv1ChromaPoints *dst = points[i];
> > +        const  AVRational           *src = mdcv->display_primaries[i];
> > +
> > +        dst->x =
> > +            AV_BSWAP16C(av_rescale_q(1, src[0],
> > +                                     (AVRational){ 1, (1 << 16) }));
>
> Can you explain what's the matter with the AV_BSWAP16C here? And if I am
> not mistaken, then the av_rescale_q() above is equivalent to src[0] * (1
> << 16) (if we had multiplications of AVRationals by integers).

It's basically a way that I got the big-endian values written out to
the uint16_t. Same goes for the uint32_t values and AV_BSWAP32C. Not
sure if there is a better macro for handling this on both BE and LE.

SVT-AV1 seems to just write these values into the bit stream as-is and
the interface seems to be defined around that (f.ex. in the string
based interface `x << 8 | x >> 8` is always done to the parsed value).

https://gitlab.com/AOMediaCodec/SVT-AV1/-/blob/7233e7ead5fe8a0f1dc33b91aba0c5a24cb9f673/Source/Lib/Encoder/Codec/EbEntropyCoding.c#L3626-3637

Jan
_______________________________________________
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 v3 10/12] avcodec/libsvtav1: add support for writing out CLL and MDCV

[Andreas Rheinhardt]

Jan Ekström:
> On Sun, Aug 20, 2023 at 10:10 AM Andreas Rheinhardt
> <andreas.rheinhardt@outlook.com> wrote:
>>
>> 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         | 70 ++++++++++++++++++++++++++++++++++
>>>  tests/fate/enc_external.mak    |  5 +++
>>>  tests/ref/fate/libsvtav1-hdr10 | 14 +++++++
>>>  3 files changed, 89 insertions(+)
>>>  create mode 100644 tests/ref/fate/libsvtav1-hdr10
>>>
>>> diff --git a/libavcodec/libsvtav1.c b/libavcodec/libsvtav1.c
>>> index f2b73361d8..d4f9fa14ba 100644
>>> --- a/libavcodec/libsvtav1.c
>>> +++ b/libavcodec/libsvtav1.c
>>> @@ -24,9 +24,11 @@
>>>  #include <EbSvtAv1ErrorCodes.h>
>>>  #include <EbSvtAv1Enc.h>
>>>
>>> +#include "libavutil/bswap.h"
>>>  #include "libavutil/common.h"
>>>  #include "libavutil/frame.h"
>>>  #include "libavutil/imgutils.h"
>>> +#include "libavutil/mastering_display_metadata.h"
>>>  #include "libavutil/opt.h"
>>>  #include "libavutil/pixdesc.h"
>>>  #include "libavutil/avassert.h"
>>> @@ -146,6 +148,72 @@ static int alloc_buffer(EbSvtAv1EncConfiguration *config, SvtContext *svt_enc)
>>>
>>>  }
>>>
>>> +static void handle_mdcv(struct EbSvtAv1MasteringDisplayInfo *dst,
>>> +                        const AVMasteringDisplayMetadata *mdcv)
>>> +{
>>> +    struct EbSvtAv1ChromaPoints *points[] = {
>>> +        &dst->r,
>>> +        &dst->g,
>>> +        &dst->b,
>>> +    };
>>> +
>>> +    if (!mdcv->has_primaries)
>>> +        goto skip_primaries;
>>> +
>>> +    for (int i = 0; i < 3; i++) {
>>> +        struct EbSvtAv1ChromaPoints *dst = points[i];
>>> +        const  AVRational           *src = mdcv->display_primaries[i];
>>> +
>>> +        dst->x =
>>> +            AV_BSWAP16C(av_rescale_q(1, src[0],
>>> +                                     (AVRational){ 1, (1 << 16) }));
>>
>> Can you explain what's the matter with the AV_BSWAP16C here? And if I am
>> not mistaken, then the av_rescale_q() above is equivalent to src[0] * (1
>> << 16) (if we had multiplications of AVRationals by integers).
> 
> It's basically a way that I got the big-endian values written out to
> the uint16_t. Same goes for the uint32_t values and AV_BSWAP32C. 

This would only work on a little endian system (on big endian systems,
this approach would write the values as little-endian); the correct way
to write BE is via AV_WB16/32.

Not
> sure if there is a better macro for handling this on both BE and LE.
> 
> SVT-AV1 seems to just write these values into the bit stream as-is and
> the interface seems to be defined around that (f.ex. in the string
> based interface `x << 8 | x >> 8` is always done to the parsed value).

Honestly, I do not get why they are using intswap16/32 unconditionally
even for BE systems. It seems to contradict their own documentation of
always putting BE values into the struct.

> 
> https://gitlab.com/AOMediaCodec/SVT-AV1/-/blob/7233e7ead5fe8a0f1dc33b91aba0c5a24cb9f673/Source/Lib/Encoder/Codec/EbEntropyCoding.c#L3626-3637
> 

The correct reference would be
https://gitlab.com/AOMediaCodec/SVT-AV1/-/blob/7233e7ead5fe8a0f1dc33b91aba0c5a24cb9f673/Source/API/EbSvtAv1Enc.h#L93
("values are stored in BE format"); you should not rely on internals of
other projects.
The reason for using BE should be explicitly documented.

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

[Jan Ekström]

On Tue, Aug 22, 2023 at 12:59 AM Andreas Rheinhardt
<andreas.rheinhardt@outlook.com> wrote:
>
> Jan Ekström:
> > On Sun, Aug 20, 2023 at 10:10 AM Andreas Rheinhardt
> > <andreas.rheinhardt@outlook.com> wrote:
> >>
> >> 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         | 70 ++++++++++++++++++++++++++++++++++
> >>>  tests/fate/enc_external.mak    |  5 +++
> >>>  tests/ref/fate/libsvtav1-hdr10 | 14 +++++++
> >>>  3 files changed, 89 insertions(+)
> >>>  create mode 100644 tests/ref/fate/libsvtav1-hdr10
> >>>
> >>> diff --git a/libavcodec/libsvtav1.c b/libavcodec/libsvtav1.c
> >>> index f2b73361d8..d4f9fa14ba 100644
> >>> --- a/libavcodec/libsvtav1.c
> >>> +++ b/libavcodec/libsvtav1.c
> >>> @@ -24,9 +24,11 @@
> >>>  #include <EbSvtAv1ErrorCodes.h>
> >>>  #include <EbSvtAv1Enc.h>
> >>>
> >>> +#include "libavutil/bswap.h"
> >>>  #include "libavutil/common.h"
> >>>  #include "libavutil/frame.h"
> >>>  #include "libavutil/imgutils.h"
> >>> +#include "libavutil/mastering_display_metadata.h"
> >>>  #include "libavutil/opt.h"
> >>>  #include "libavutil/pixdesc.h"
> >>>  #include "libavutil/avassert.h"
> >>> @@ -146,6 +148,72 @@ static int alloc_buffer(EbSvtAv1EncConfiguration *config, SvtContext *svt_enc)
> >>>
> >>>  }
> >>>
> >>> +static void handle_mdcv(struct EbSvtAv1MasteringDisplayInfo *dst,
> >>> +                        const AVMasteringDisplayMetadata *mdcv)
> >>> +{
> >>> +    struct EbSvtAv1ChromaPoints *points[] = {
> >>> +        &dst->r,
> >>> +        &dst->g,
> >>> +        &dst->b,
> >>> +    };
> >>> +
> >>> +    if (!mdcv->has_primaries)
> >>> +        goto skip_primaries;
> >>> +
> >>> +    for (int i = 0; i < 3; i++) {
> >>> +        struct EbSvtAv1ChromaPoints *dst = points[i];
> >>> +        const  AVRational           *src = mdcv->display_primaries[i];
> >>> +
> >>> +        dst->x =
> >>> +            AV_BSWAP16C(av_rescale_q(1, src[0],
> >>> +                                     (AVRational){ 1, (1 << 16) }));
> >>
> >> Can you explain what's the matter with the AV_BSWAP16C here? And if I am
> >> not mistaken, then the av_rescale_q() above is equivalent to src[0] * (1
> >> << 16) (if we had multiplications of AVRationals by integers).
> >
> > It's basically a way that I got the big-endian values written out to
> > the uint16_t. Same goes for the uint32_t values and AV_BSWAP32C.
>
> This would only work on a little endian system (on big endian systems,
> this approach would write the values as little-endian); the correct way
> to write BE is via AV_WB16/32.
>

Yup, that was the correct one (I knew mine was technically incorrect
as it would always do the swap, but kept it as part of the first
versions). Thanks.

So it ends up being like the following as the WB macros deal with
pointers and not in x = THING() style.

AV_WB16(&dst->x,
        av_rescale_q(1, src[0], (AVRational){ 1, (1 << 16) }));

Applied this as well as the more classic if (has_primaries) style that
you mentioned with the libx264 wrapper patch in my v4 branch.

> Not
> > sure if there is a better macro for handling this on both BE and LE.
> >
> > SVT-AV1 seems to just write these values into the bit stream as-is and
> > the interface seems to be defined around that (f.ex. in the string
> > based interface `x << 8 | x >> 8` is always done to the parsed value).
>
> Honestly, I do not get why they are using intswap16/32 unconditionally
> even for BE systems. It seems to contradict their own documentation of
> always putting BE values into the struct.
>

Either an oversight, or due to BE not really being a consideration
(which kind of makes sense with even PPC going LE now). Not that I can
do much more than guess.

> >
> > https://gitlab.com/AOMediaCodec/SVT-AV1/-/blob/7233e7ead5fe8a0f1dc33b91aba0c5a24cb9f673/Source/Lib/Encoder/Codec/EbEntropyCoding.c#L3626-3637
> >
>
> The correct reference would be
> https://gitlab.com/AOMediaCodec/SVT-AV1/-/blob/7233e7ead5fe8a0f1dc33b91aba0c5a24cb9f673/Source/API/EbSvtAv1Enc.h#L93
> ("values are stored in BE format"); you should not rely on internals of
> other projects.
> The reason for using BE should be explicitly documented.
>

I based the decision of putting BE values there on the public headers.
I only looked into the internals after writing "seems to just write
these values into the bit stream", and then ending up looking at the
internals to see whether it looked like that or not.

Jan
_______________________________________________
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 v3 11/12] 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.
---
 libavcodec/libx264.c         | 79 ++++++++++++++++++++++++++++++++++++
 tests/fate/enc_external.mak  |  5 +++
 tests/ref/fate/libx264-hdr10 | 15 +++++++
 3 files changed, 99 insertions(+)
 create mode 100644 tests/ref/fate/libx264-hdr10

diff --git a/libavcodec/libx264.c b/libavcodec/libx264.c
index 1a7dc7bdd5..30ea3dae4c 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"
@@ -842,6 +843,82 @@ static int convert_pix_fmt(enum AVPixelFormat pix_fmt)
         return AVERROR(EINVAL);\
     }
 
+#if X264_BUILD >= 163
+static void handle_mdcv(x264_param_t *params,
+                        const AVMasteringDisplayMetadata *mdcv)
+{
+    int *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
+        },
+    };
+
+    if (!mdcv->has_primaries && !mdcv->has_luminance)
+        return;
+
+    params->mastering_display.b_mastering_display = 1;
+
+    if (!mdcv->has_primaries)
+        goto skip_primaries;
+
+    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 });
+
+skip_primaries:
+    if (!mdcv->has_luminance)
+        return;
+
+    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 // X264_BUILD >= 163
+
+static void handle_side_data(AVCodecContext *avctx, x264_param_t *params)
+{
+#if X264_BUILD >= 163
+    const AVFrameSideDataSet set = avctx->side_data_set;
+    const AVFrameSideData *cll_sd =
+        av_side_data_set_get_item(set, AV_FRAME_DATA_CONTENT_LIGHT_LEVEL);
+    const AVFrameSideData *mdcv_sd =
+        av_side_data_set_get_item(set, 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 // X264_BUILD >= 163
+}
+
 static av_cold int X264_init(AVCodecContext *avctx)
 {
     X264Context *x4 = avctx->priv_data;
@@ -1142,6 +1219,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..90d8894d04 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, 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.41.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 v3 11/12] avcodec/libx264: add support for writing out CLL and MDCV

[Michael Niedermayer]

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

On Fri, Aug 18, 2023 at 12:48:49AM +0300, Jan Ekström wrote:
> 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.
> ---
>  libavcodec/libx264.c         | 79 ++++++++++++++++++++++++++++++++++++
>  tests/fate/enc_external.mak  |  5 +++
>  tests/ref/fate/libx264-hdr10 | 15 +++++++
>  3 files changed, 99 insertions(+)
>  create mode 100644 tests/ref/fate/libx264-hdr10

fate fails with X264_BUILD 152

The filters 'Parsed_null_0' and 'format' do not have a common format and automatic conversion is disabled.
[vf#0:0 @ 0x55eddf8d4780] Error reinitializing filters!
Failed to inject frame into filter network: Invalid argument
Error while filtering: Invalid argument
[out#0/mp4 @ 0x55eddf87b980] Nothing was written into output file, because at least one of its streams received no packets.
frame=    0 fps=0.0 q=0.0 Lsize=       0kB time=N/A bitrate=N/A speed=N/A
Conversion failed!
threads=1
tests/Makefile:307: recipe for target 'fate-libx264-hdr10' failed
make: *** [fate-libx264-hdr10] Error 234


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

If the United States is serious about tackling the national security threats 
related to an insecure 5G network, it needs to rethink the extent to which it
values corporate profits and government espionage over security.-Bruce Schneier

[-- 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 v3 11/12] avcodec/libx264: add support for writing out CLL and MDCV

[Jan Ekström]

On Sat, Aug 19, 2023 at 7:53 PM Michael Niedermayer
<michael@niedermayer.cc> wrote:
>
> On Fri, Aug 18, 2023 at 12:48:49AM +0300, Jan Ekström wrote:
> > 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.
> > ---
> >  libavcodec/libx264.c         | 79 ++++++++++++++++++++++++++++++++++++
> >  tests/fate/enc_external.mak  |  5 +++
> >  tests/ref/fate/libx264-hdr10 | 15 +++++++
> >  3 files changed, 99 insertions(+)
> >  create mode 100644 tests/ref/fate/libx264-hdr10
>
> fate fails with X264_BUILD 152
>
> The filters 'Parsed_null_0' and 'format' do not have a common format and automatic conversion is disabled.
> [vf#0:0 @ 0x55eddf8d4780] Error reinitializing filters!
> Failed to inject frame into filter network: Invalid argument
> Error while filtering: Invalid argument
> [out#0/mp4 @ 0x55eddf87b980] Nothing was written into output file, because at least one of its streams received no packets.
> frame=    0 fps=0.0 q=0.0 Lsize=       0kB time=N/A bitrate=N/A speed=N/A
> Conversion failed!
> threads=1
> tests/Makefile:307: recipe for target 'fate-libx264-hdr10' failed
> make: *** [fate-libx264-hdr10] Error 234
>

Without having more information, that sounds more like a 8bit only
build rather than an X264_BUILD related issue, as the error seems to
come from a conversion from the input 10bit content to whatever not
being available.

Can you check if that is the case?

Jan
_______________________________________________
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 v3 11/12] avcodec/libx264: add support for writing out CLL and MDCV

[Andreas Rheinhardt]

Jan Ekström:
> On Sat, Aug 19, 2023 at 7:53 PM Michael Niedermayer
> <michael@niedermayer.cc> wrote:
>>
>> On Fri, Aug 18, 2023 at 12:48:49AM +0300, Jan Ekström wrote:
>>> 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.
>>> ---
>>>  libavcodec/libx264.c         | 79 ++++++++++++++++++++++++++++++++++++
>>>  tests/fate/enc_external.mak  |  5 +++
>>>  tests/ref/fate/libx264-hdr10 | 15 +++++++
>>>  3 files changed, 99 insertions(+)
>>>  create mode 100644 tests/ref/fate/libx264-hdr10
>>
>> fate fails with X264_BUILD 152
>>
>> The filters 'Parsed_null_0' and 'format' do not have a common format and automatic conversion is disabled.
>> [vf#0:0 @ 0x55eddf8d4780] Error reinitializing filters!
>> Failed to inject frame into filter network: Invalid argument
>> Error while filtering: Invalid argument
>> [out#0/mp4 @ 0x55eddf87b980] Nothing was written into output file, because at least one of its streams received no packets.
>> frame=    0 fps=0.0 q=0.0 Lsize=       0kB time=N/A bitrate=N/A speed=N/A
>> Conversion failed!
>> threads=1
>> tests/Makefile:307: recipe for target 'fate-libx264-hdr10' failed
>> make: *** [fate-libx264-hdr10] Error 234
>>
> 
> Without having more information, that sounds more like a 8bit only
> build rather than an X264_BUILD related issue, as the error seems to
> come from a conversion from the input 10bit content to whatever not
> being available.
> 
> Can you check if that is the case?
> 

That is probably true, but your test nevertheless requires X264_BUILD >=
163, but the test requirements don't check for this.

- 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 v3 11/12] avcodec/libx264: add support for writing out CLL and MDCV

[Michael Niedermayer]

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

On Sun, Aug 20, 2023 at 01:25:27AM +0300, Jan Ekström wrote:
> On Sat, Aug 19, 2023 at 7:53 PM Michael Niedermayer
> <michael@niedermayer.cc> wrote:
> >
> > On Fri, Aug 18, 2023 at 12:48:49AM +0300, Jan Ekström wrote:
> > > 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.
> > > ---
> > >  libavcodec/libx264.c         | 79 ++++++++++++++++++++++++++++++++++++
> > >  tests/fate/enc_external.mak  |  5 +++
> > >  tests/ref/fate/libx264-hdr10 | 15 +++++++
> > >  3 files changed, 99 insertions(+)
> > >  create mode 100644 tests/ref/fate/libx264-hdr10
> >
> > fate fails with X264_BUILD 152
> >
> > The filters 'Parsed_null_0' and 'format' do not have a common format and automatic conversion is disabled.
> > [vf#0:0 @ 0x55eddf8d4780] Error reinitializing filters!
> > Failed to inject frame into filter network: Invalid argument
> > Error while filtering: Invalid argument
> > [out#0/mp4 @ 0x55eddf87b980] Nothing was written into output file, because at least one of its streams received no packets.
> > frame=    0 fps=0.0 q=0.0 Lsize=       0kB time=N/A bitrate=N/A speed=N/A
> > Conversion failed!
> > threads=1
> > tests/Makefile:307: recipe for target 'fate-libx264-hdr10' failed
> > make: *** [fate-libx264-hdr10] Error 234
> >
> 
> Without having more information, that sounds more like a 8bit only
> build rather than an X264_BUILD related issue, as the error seems to
> come from a conversion from the input 10bit content to whatever not
> being available.
> 
> Can you check if that is the case?

x264 152 was either 8bit or 10bit but not both. That box on which this
fails has x264 builds for both for 8bit and 10bit.
ffmpeg can at runtime be linked with either
for example
LD_PRELOAD=/usr/lib/x86_64-linux-gnu/x264-10bit/libx264.so.152 ./ffmpeg -i ~/videos/mm-short.mpg  test.mkv
produces a 10bit output

maybe you should just disable this test for 152 and prior to avoid this complexity

thx

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

Elect your leaders based on what they did after the last election, not
based on what they say before an election.


[-- 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 v3 11/12] avcodec/libx264: add support for writing out CLL and MDCV

[Jan Ekström]

On Sun, Aug 20, 2023 at 4:12 PM Michael Niedermayer
<michael@niedermayer.cc> wrote:
>
> On Sun, Aug 20, 2023 at 01:25:27AM +0300, Jan Ekström wrote:
> > On Sat, Aug 19, 2023 at 7:53 PM Michael Niedermayer
> > <michael@niedermayer.cc> wrote:
> > >
> > > On Fri, Aug 18, 2023 at 12:48:49AM +0300, Jan Ekström wrote:
> > > > 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.
> > > > ---
> > > >  libavcodec/libx264.c         | 79 ++++++++++++++++++++++++++++++++++++
> > > >  tests/fate/enc_external.mak  |  5 +++
> > > >  tests/ref/fate/libx264-hdr10 | 15 +++++++
> > > >  3 files changed, 99 insertions(+)
> > > >  create mode 100644 tests/ref/fate/libx264-hdr10
> > >
> > > fate fails with X264_BUILD 152
> > >
> > > The filters 'Parsed_null_0' and 'format' do not have a common format and automatic conversion is disabled.
> > > [vf#0:0 @ 0x55eddf8d4780] Error reinitializing filters!
> > > Failed to inject frame into filter network: Invalid argument
> > > Error while filtering: Invalid argument
> > > [out#0/mp4 @ 0x55eddf87b980] Nothing was written into output file, because at least one of its streams received no packets.
> > > frame=    0 fps=0.0 q=0.0 Lsize=       0kB time=N/A bitrate=N/A speed=N/A
> > > Conversion failed!
> > > threads=1
> > > tests/Makefile:307: recipe for target 'fate-libx264-hdr10' failed
> > > make: *** [fate-libx264-hdr10] Error 234
> > >
> >
> > Without having more information, that sounds more like a 8bit only
> > build rather than an X264_BUILD related issue, as the error seems to
> > come from a conversion from the input 10bit content to whatever not
> > being available.
> >
> > Can you check if that is the case?
>
> x264 152 was either 8bit or 10bit but not both. That box on which this
> fails has x264 builds for both for 8bit and 10bit.
> ffmpeg can at runtime be linked with either
> for example
> LD_PRELOAD=/usr/lib/x86_64-linux-gnu/x264-10bit/libx264.so.152 ./ffmpeg -i ~/videos/mm-short.mpg  test.mkv
> produces a 10bit output
>
> maybe you should just disable this test for 152 and prior to avoid this complexity
>
> thx

Yup, before Vittorio allowed for both bit depths to be included that
indeed was the case, just didn't recall it was at X264_BUILD >= 153
where the work was done.

As for skipping, I was seemingly tired and focused more on the exact
error than the overall case. Indeed, >= 163 is required for the test
to pass, so some sort of disablement for older versions is required.

Is the simplest way to add a configure check for this, or can the
define be utilized in some manner from the test's Makefile?

Jan
_______________________________________________
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 v3 11/12] avcodec/libx264: add support for writing out CLL and MDCV

[Andreas Rheinhardt]

Jan Ekström:
> On Sun, Aug 20, 2023 at 4:12 PM Michael Niedermayer
> <michael@niedermayer.cc> wrote:
>>
>> On Sun, Aug 20, 2023 at 01:25:27AM +0300, Jan Ekström wrote:
>>> On Sat, Aug 19, 2023 at 7:53 PM Michael Niedermayer
>>> <michael@niedermayer.cc> wrote:
>>>>
>>>> On Fri, Aug 18, 2023 at 12:48:49AM +0300, Jan Ekström wrote:
>>>>> 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.
>>>>> ---
>>>>>  libavcodec/libx264.c         | 79 ++++++++++++++++++++++++++++++++++++
>>>>>  tests/fate/enc_external.mak  |  5 +++
>>>>>  tests/ref/fate/libx264-hdr10 | 15 +++++++
>>>>>  3 files changed, 99 insertions(+)
>>>>>  create mode 100644 tests/ref/fate/libx264-hdr10
>>>>
>>>> fate fails with X264_BUILD 152
>>>>
>>>> The filters 'Parsed_null_0' and 'format' do not have a common format and automatic conversion is disabled.
>>>> [vf#0:0 @ 0x55eddf8d4780] Error reinitializing filters!
>>>> Failed to inject frame into filter network: Invalid argument
>>>> Error while filtering: Invalid argument
>>>> [out#0/mp4 @ 0x55eddf87b980] Nothing was written into output file, because at least one of its streams received no packets.
>>>> frame=    0 fps=0.0 q=0.0 Lsize=       0kB time=N/A bitrate=N/A speed=N/A
>>>> Conversion failed!
>>>> threads=1
>>>> tests/Makefile:307: recipe for target 'fate-libx264-hdr10' failed
>>>> make: *** [fate-libx264-hdr10] Error 234
>>>>
>>>
>>> Without having more information, that sounds more like a 8bit only
>>> build rather than an X264_BUILD related issue, as the error seems to
>>> come from a conversion from the input 10bit content to whatever not
>>> being available.
>>>
>>> Can you check if that is the case?
>>
>> x264 152 was either 8bit or 10bit but not both. That box on which this
>> fails has x264 builds for both for 8bit and 10bit.
>> ffmpeg can at runtime be linked with either
>> for example
>> LD_PRELOAD=/usr/lib/x86_64-linux-gnu/x264-10bit/libx264.so.152 ./ffmpeg -i ~/videos/mm-short.mpg  test.mkv
>> produces a 10bit output
>>
>> maybe you should just disable this test for 152 and prior to avoid this complexity
>>
>> thx
> 
> Yup, before Vittorio allowed for both bit depths to be included that
> indeed was the case, just didn't recall it was at X264_BUILD >= 153
> where the work was done.
> 
> As for skipping, I was seemingly tired and focused more on the exact
> error than the overall case. Indeed, >= 163 is required for the test
> to pass, so some sort of disablement for older versions is required.
> 

Actually, x264 allows to select only eight or ten bit bitdepth (or both)
at configure-time. IIRC opening the encoder will fail if an eight-bit
only libx264 is used when one tries to use a 10bit pixel format.
(This is actually a regression since x264 version 153; with older
versions the codec's advertised pixel formats actually match the really
supported pixel formats.)

> Is the simplest way to add a configure check for this, or can the
> define be utilized in some manner from the test's Makefile?
> 

_______________________________________________
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 v3 11/12] avcodec/libx264: add support for writing out CLL and MDCV

[Vittorio Giovara]

On Mon, Aug 21, 2023 at 9:49 PM Andreas Rheinhardt <
andreas.rheinhardt@outlook.com> wrote:

> Actually, x264 allows to select only eight or ten bit bitdepth (or both)
> at configure-time. IIRC opening the encoder will fail if an eight-bit
> only libx264 is used when one tries to use a 10bit pixel format.
> (This is actually a regression since x264 version 153; with older
> versions the codec's advertised pixel formats actually match the really
> supported pixel formats.)
>

This changed a few years ago, x264 supports both bitdepths in a single
binary since 153 (hence why the codepath got updated like so)
-- 
Vittorio
_______________________________________________
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 v3 11/12] avcodec/libx264: add support for writing out CLL and MDCV

[Andreas Rheinhardt]

Vittorio Giovara:
> On Mon, Aug 21, 2023 at 9:49 PM Andreas Rheinhardt <
> andreas.rheinhardt@outlook.com> wrote:
> 
>> Actually, x264 allows to select only eight or ten bit bitdepth (or both)
>> at configure-time. IIRC opening the encoder will fail if an eight-bit
>> only libx264 is used when one tries to use a 10bit pixel format.
>> (This is actually a regression since x264 version 153; with older
>> versions the codec's advertised pixel formats actually match the really
>> supported pixel formats.)
>>
> 
> This changed a few years ago, x264 supports both bitdepths in a single
> binary since 153 (hence why the codepath got updated like so)

But if you use an x264 build that only supports one bitdepth, you can
run into the regression I mentioned (and the test will fail if you use
an 8-bit only libx264).

- 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 v3 11/12] avcodec/libx264: add support for writing out CLL and MDCV

[Andreas Rheinhardt]

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.
> ---
>  libavcodec/libx264.c         | 79 ++++++++++++++++++++++++++++++++++++
>  tests/fate/enc_external.mak  |  5 +++
>  tests/ref/fate/libx264-hdr10 | 15 +++++++
>  3 files changed, 99 insertions(+)
>  create mode 100644 tests/ref/fate/libx264-hdr10
> 
> diff --git a/libavcodec/libx264.c b/libavcodec/libx264.c
> index 1a7dc7bdd5..30ea3dae4c 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"
> @@ -842,6 +843,82 @@ static int convert_pix_fmt(enum AVPixelFormat pix_fmt)
>          return AVERROR(EINVAL);\
>      }
>  
> +#if X264_BUILD >= 163
> +static void handle_mdcv(x264_param_t *params,
> +                        const AVMasteringDisplayMetadata *mdcv)
> +{
> +    int *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
> +        },
> +    };
> +
> +    if (!mdcv->has_primaries && !mdcv->has_luminance)
> +        return;
> +
> +    params->mastering_display.b_mastering_display = 1;
> +
> +    if (!mdcv->has_primaries)
> +        goto skip_primaries;

Normally we try to avoid gotos for non-error stuff. You are basically
replacing an ordinary "if" here.

> +
> +    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 });
> +
> +skip_primaries:
> +    if (!mdcv->has_luminance)
> +        return;
> +
> +    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 // X264_BUILD >= 163
> +
> +static void handle_side_data(AVCodecContext *avctx, x264_param_t *params)
> +{
> +#if X264_BUILD >= 163
> +    const AVFrameSideDataSet set = avctx->side_data_set;
> +    const AVFrameSideData *cll_sd =
> +        av_side_data_set_get_item(set, AV_FRAME_DATA_CONTENT_LIGHT_LEVEL);
> +    const AVFrameSideData *mdcv_sd =
> +        av_side_data_set_get_item(set, AV_FRAME_DATA_MASTERING_DISPLAY_METADATA);

You can improve code locality by not using two variables for the side
data, but only one:
side_data = av_side_data_set_get_item(set,
AV_FRAME_DATA_CONTENT_LIGHT_LEVEL);
if (side_data) { ... }
side_data = av_side_data_set_get_item(set,
AV_FRAME_DATA_MASTERING_DISPLAY_METADATA);
if (side_data) { ... }

> +
> +    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 // X264_BUILD >= 163
> +}
> +
>  static av_cold int X264_init(AVCodecContext *avctx)
>  {
>      X264Context *x4 = avctx->priv_data;
> @@ -1142,6 +1219,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..90d8894d04 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, 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

_______________________________________________
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 v3 11/12] avcodec/libx264: add support for writing out CLL and MDCV

[Jan Ekström]

On Sun, Aug 20, 2023 at 9:54 AM Andreas Rheinhardt
<andreas.rheinhardt@outlook.com> wrote:
>
> 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.
> > ---
> >  libavcodec/libx264.c         | 79 ++++++++++++++++++++++++++++++++++++
> >  tests/fate/enc_external.mak  |  5 +++
> >  tests/ref/fate/libx264-hdr10 | 15 +++++++
> >  3 files changed, 99 insertions(+)
> >  create mode 100644 tests/ref/fate/libx264-hdr10
> >
> > diff --git a/libavcodec/libx264.c b/libavcodec/libx264.c
> > index 1a7dc7bdd5..30ea3dae4c 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"
> > @@ -842,6 +843,82 @@ static int convert_pix_fmt(enum AVPixelFormat pix_fmt)
> >          return AVERROR(EINVAL);\
> >      }
> >
> > +#if X264_BUILD >= 163
> > +static void handle_mdcv(x264_param_t *params,
> > +                        const AVMasteringDisplayMetadata *mdcv)
> > +{
> > +    int *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
> > +        },
> > +    };
> > +
> > +    if (!mdcv->has_primaries && !mdcv->has_luminance)
> > +        return;
> > +
> > +    params->mastering_display.b_mastering_display = 1;
> > +
> > +    if (!mdcv->has_primaries)
> > +        goto skip_primaries;
>
> Normally we try to avoid gotos for non-error stuff. You are basically
> replacing an ordinary "if" here.
>

Yes, I think this was mostly me attempting to follow the "early exit
if possible" best practice, which mostly brings usefulness that the
following code no longer needs to be think about that condition (which
often then leads to less indentation etc).

You might be right that checking it the other way and just putting the
contents into the if might be better in this spot, will adjust and
check.


> > +static void handle_side_data(AVCodecContext *avctx, x264_param_t *params)
> > +{
> > +#if X264_BUILD >= 163
> > +    const AVFrameSideDataSet set = avctx->side_data_set;
> > +    const AVFrameSideData *cll_sd =
> > +        av_side_data_set_get_item(set, AV_FRAME_DATA_CONTENT_LIGHT_LEVEL);
> > +    const AVFrameSideData *mdcv_sd =
> > +        av_side_data_set_get_item(set, AV_FRAME_DATA_MASTERING_DISPLAY_METADATA);
>
> You can improve code locality by not using two variables for the side
> data, but only one:
> side_data = av_side_data_set_get_item(set,
> AV_FRAME_DATA_CONTENT_LIGHT_LEVEL);
> if (side_data) { ... }
> side_data = av_side_data_set_get_item(set,
> AV_FRAME_DATA_MASTERING_DISPLAY_METADATA);
> if (side_data) { ... }

This is something where I wonder whether this actually brings
performance benefits, and whether those are worth it VS the separation
of the pre-optimization variable names?

Jan
_______________________________________________
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 v3 12/12] 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         | 82 ++++++++++++++++++++++++++++++++++++
 tests/fate/enc_external.mak  |  5 +++
 tests/ref/fate/libx265-hdr10 | 16 +++++++
 3 files changed, 103 insertions(+)
 create mode 100644 tests/ref/fate/libx265-hdr10

diff --git a/libavcodec/libx265.c b/libavcodec/libx265.c
index 873b3021ee..2204b5146a 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,79 @@ static av_cold int libx265_param_parse_int(AVCodecContext *avctx,
     return 0;
 }
 
+static int handle_mdcv(AVCodecContext *avctx, const x265_api *api,
+                       x265_param *params,
+                       const AVMasteringDisplayMetadata *mdcv)
+{
+    int ret = AVERROR_BUG;
+    static const char *option = "master-display";
+    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)) {
+        ret = AVERROR(ENOMEM);
+        goto end;
+    }
+
+    if (api->param_parse(params, option, buf.str) ==
+            X265_PARAM_BAD_VALUE) {
+        av_log(avctx, AV_LOG_ERROR,
+               "Invalid value \"%s\" for param \"%s\".\n",
+               buf.str, option);
+        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 AVFrameSideDataSet set = avctx->side_data_set;
+    const AVFrameSideData *cll_sd =
+        av_side_data_set_get_item(set, AV_FRAME_DATA_CONTENT_LIGHT_LEVEL);
+    const AVFrameSideData *mdcv_sd =
+        av_side_data_set_get_item(set, 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, 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 +414,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 90d8894d04..8d6df2febd 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, HEVC_DEMUXER H264_DECODER) +
 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.41.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 v3 12/12] avcodec/libx265: add support for writing out CLL and MDCV

[Andreas Rheinhardt]

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         | 82 ++++++++++++++++++++++++++++++++++++
>  tests/fate/enc_external.mak  |  5 +++
>  tests/ref/fate/libx265-hdr10 | 16 +++++++
>  3 files changed, 103 insertions(+)
>  create mode 100644 tests/ref/fate/libx265-hdr10
> 
> diff --git a/libavcodec/libx265.c b/libavcodec/libx265.c
> index 873b3021ee..2204b5146a 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,79 @@ static av_cold int libx265_param_parse_int(AVCodecContext *avctx,
>      return 0;
>  }
>  
> +static int handle_mdcv(AVCodecContext *avctx, const x265_api *api,

You are not using an AVCodecContext* at all in this function; you only
need a logcontext (i.e. void*).

> +                       x265_param *params,
> +                       const AVMasteringDisplayMetadata *mdcv)
> +{
> +    int ret = AVERROR_BUG;
> +    static const char *option = "master-display";

This pointer is completely unnecessary.

> +    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)) {
> +        ret = AVERROR(ENOMEM);

When using AV_BPRINT_SIZE_AUTOMATIC the buffer being incomplete does
not mean that an allocation failure occurred, as no allocations are
attempted at all.
Furthermore, I don't see the point of using the AVBPrint API here at
all. All you need is a big enough buffer and snprintf.

> +        goto end;
> +    }
> +
> +    if (api->param_parse(params, option, buf.str) ==
> +            X265_PARAM_BAD_VALUE) {
> +        av_log(avctx, AV_LOG_ERROR,
> +               "Invalid value \"%s\" for param \"%s\".\n",
> +               buf.str, option);
> +        ret = AVERROR(EINVAL);
> +        goto end;
> +    }
> +
> +    ret = 0;
> +
> +end:
> +    av_bprint_finalize(&buf, NULL);

This is unnecessary: AV_BPRINT_SIZE_AUTOMATIC uses only the buffer
available in the AVBPrint itself; there are no dynamic allocations.

> +
> +    return ret;
> +}
> +
> +static int handle_side_data(AVCodecContext *avctx, const x265_api *api,
> +                            x265_param *params)
> +{
> +    const AVFrameSideDataSet set = avctx->side_data_set;
> +    const AVFrameSideData *cll_sd =
> +        av_side_data_set_get_item(set, AV_FRAME_DATA_CONTENT_LIGHT_LEVEL);
> +    const AVFrameSideData *mdcv_sd =
> +        av_side_data_set_get_item(set, 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, 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 +414,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 90d8894d04..8d6df2febd 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, HEVC_DEMUXER H264_DECODER) +
>  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

_______________________________________________
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".