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

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

[Jan Ekström]

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.

The first patch, poking at loop counters, was already posted earlier separately,
and only part of this set in order to make the patch set applicable on top of
master, as the patch had not yet received any ACKs.

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 a private AVFrameSideDataSet in avctx as everything seems to
presume that extradata etc are available after opening the encoder.


Jan

Jan Ekström (14):
  avutil/frame: move counters utilized in loops to their scope
  avcodec: move AVCodecInternal allocation to avcodec_alloc_context3
  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 clearing out 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
  avcodec: add private side data set to AVCodecInternal
  avcodec: add function for setting avctx side data
  ffmpeg: pass first video AVFrame's side data to encoder
  avcodec/libsvtav1: add support for writing out CLL and MDCV
  avcodec/libx264: add support for writing out CLL and MDCV
  avcodec/libx265: add support for writing out CLL and MDCV

 fftools/ffmpeg.c                  |  12 +++
 libavcodec/avcodec.c              |  49 +++++++--
 libavcodec/avcodec.h              |  12 +++
 libavcodec/frame_thread_encoder.c |   7 +-
 libavcodec/internal.h             |  12 +++
 libavcodec/libsvtav1.c            |  70 +++++++++++++
 libavcodec/libx264.c              |  79 +++++++++++++++
 libavcodec/libx265.c              |  82 +++++++++++++++
 libavcodec/options.c              |  25 ++++-
 libavutil/frame.c                 | 161 ++++++++++++++++++------------
 libavutil/frame.h                 |  38 +++++++
 11 files changed, 474 insertions(+), 73 deletions(-)

-- 
2.39.2

_______________________________________________
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 v0 01/14] avutil/frame: move counters utilized in loops to their scope

[Jan Ekström]

This way we can clean up separate definitions in functions with
just a single loop, as well as have no reuse between different
loops' counters in functions with multiple.
---
 libavutil/frame.c | 76 ++++++++++++++++++++---------------------------
 1 file changed, 32 insertions(+), 44 deletions(-)

diff --git a/libavutil/frame.c b/libavutil/frame.c
index 5c9afb0d03..c905e8d611 100644
--- a/libavutil/frame.c
+++ b/libavutil/frame.c
@@ -78,9 +78,7 @@ static void free_side_data(AVFrameSideData **ptr_sd)
 
 static void wipe_side_data(AVFrame *frame)
 {
-    int i;
-
-    for (i = 0; i < frame->nb_side_data; i++) {
+    for (int i = 0; i < frame->nb_side_data; i++) {
         free_side_data(&frame->side_data[i]);
     }
     frame->nb_side_data = 0;
@@ -112,7 +110,7 @@ void av_frame_free(AVFrame **frame)
 static int get_video_buffer(AVFrame *frame, int align)
 {
     const AVPixFmtDescriptor *desc = av_pix_fmt_desc_get(frame->format);
-    int ret, i, padded_height, total_size;
+    int ret, padded_height, total_size;
     int plane_padding = FFMAX(16 + 16/*STRIDE_ALIGN*/, align);
     ptrdiff_t linesizes[4];
     size_t sizes[4];
@@ -127,7 +125,7 @@ static int get_video_buffer(AVFrame *frame, int align)
         if (align <= 0)
             align = 32; /* STRIDE_ALIGN. Should be av_cpu_max_align() */
 
-        for(i=1; i<=align; i+=i) {
+        for (int i = 1; i <= align; i += i) {
             ret = av_image_fill_linesizes(frame->linesize, frame->format,
                                           FFALIGN(frame->width, i));
             if (ret < 0)
@@ -136,11 +134,11 @@ static int get_video_buffer(AVFrame *frame, int align)
                 break;
         }
 
-        for (i = 0; i < 4 && frame->linesize[i]; i++)
+        for (int i = 0; i < 4 && frame->linesize[i]; i++)
             frame->linesize[i] = FFALIGN(frame->linesize[i], align);
     }
 
-    for (i = 0; i < 4; i++)
+    for (int i = 0; i < 4; i++)
         linesizes[i] = frame->linesize[i];
 
     padded_height = FFALIGN(frame->height, 32);
@@ -149,7 +147,7 @@ static int get_video_buffer(AVFrame *frame, int align)
         return ret;
 
     total_size = 4*plane_padding;
-    for (i = 0; i < 4; i++) {
+    for (int i = 0; i < 4; i++) {
         if (sizes[i] > INT_MAX - total_size)
             return AVERROR(EINVAL);
         total_size += sizes[i];
@@ -165,7 +163,7 @@ static int get_video_buffer(AVFrame *frame, int align)
                                       frame->buf[0]->data, frame->linesize)) < 0)
         goto fail;
 
-    for (i = 1; i < 4; i++) {
+    for (int i = 1; i < 4; i++) {
         if (frame->data[i])
             frame->data[i] += i * plane_padding;
     }
@@ -182,7 +180,7 @@ static int get_audio_buffer(AVFrame *frame, int align)
 {
     int planar   = av_sample_fmt_is_planar(frame->format);
     int channels, planes;
-    int ret, i;
+    int ret;
 
 #if FF_API_OLD_CHANNEL_LAYOUT
 FF_DISABLE_DEPRECATION_WARNINGS
@@ -223,7 +221,7 @@ FF_ENABLE_DEPRECATION_WARNINGS
     } else
         frame->extended_data = frame->data;
 
-    for (i = 0; i < FFMIN(planes, AV_NUM_DATA_POINTERS); i++) {
+    for (int i = 0; i < FFMIN(planes, AV_NUM_DATA_POINTERS); i++) {
         frame->buf[i] = av_buffer_alloc(frame->linesize[0]);
         if (!frame->buf[i]) {
             av_frame_unref(frame);
@@ -231,7 +229,7 @@ FF_ENABLE_DEPRECATION_WARNINGS
         }
         frame->extended_data[i] = frame->data[i] = frame->buf[i]->data;
     }
-    for (i = 0; i < planes - AV_NUM_DATA_POINTERS; i++) {
+    for (int i = 0; i < planes - AV_NUM_DATA_POINTERS; i++) {
         frame->extended_buf[i] = av_buffer_alloc(frame->linesize[0]);
         if (!frame->extended_buf[i]) {
             av_frame_unref(frame);
@@ -265,7 +263,7 @@ FF_ENABLE_DEPRECATION_WARNINGS
 
 static int frame_copy_props(AVFrame *dst, const AVFrame *src, int force_copy)
 {
-    int ret, i;
+    int ret;
 
     dst->key_frame              = src->key_frame;
     dst->pict_type              = src->pict_type;
@@ -318,7 +316,7 @@ FF_ENABLE_DEPRECATION_WARNINGS
 
     av_dict_copy(&dst->metadata, src->metadata, 0);
 
-    for (i = 0; i < src->nb_side_data; i++) {
+    for (int i = 0; i < src->nb_side_data; i++) {
         const AVFrameSideData *sd_src = src->side_data[i];
         AVFrameSideData *sd_dst;
         if (   sd_src->type == AV_FRAME_DATA_PANSCAN
@@ -351,7 +349,7 @@ FF_ENABLE_DEPRECATION_WARNINGS
 
 int av_frame_ref(AVFrame *dst, const AVFrame *src)
 {
-    int i, ret = 0;
+    int ret = 0;
 
     av_assert1(dst->width == 0 && dst->height == 0);
 #if FF_API_OLD_CHANNEL_LAYOUT
@@ -406,7 +404,7 @@ FF_ENABLE_DEPRECATION_WARNINGS
     }
 
     /* ref the buffers */
-    for (i = 0; i < FF_ARRAY_ELEMS(src->buf); i++) {
+    for (int i = 0; i < FF_ARRAY_ELEMS(src->buf); i++) {
         if (!src->buf[i])
             continue;
         dst->buf[i] = av_buffer_ref(src->buf[i]);
@@ -425,7 +423,7 @@ FF_ENABLE_DEPRECATION_WARNINGS
         }
         dst->nb_extended_buf = src->nb_extended_buf;
 
-        for (i = 0; i < src->nb_extended_buf; i++) {
+        for (int i = 0; i < src->nb_extended_buf; i++) {
             dst->extended_buf[i] = av_buffer_ref(src->extended_buf[i]);
             if (!dst->extended_buf[i]) {
                 ret = AVERROR(ENOMEM);
@@ -485,16 +483,14 @@ AVFrame *av_frame_clone(const AVFrame *src)
 
 void av_frame_unref(AVFrame *frame)
 {
-    int i;
-
     if (!frame)
         return;
 
     wipe_side_data(frame);
 
-    for (i = 0; i < FF_ARRAY_ELEMS(frame->buf); i++)
+    for (int i = 0; i < FF_ARRAY_ELEMS(frame->buf); i++)
         av_buffer_unref(&frame->buf[i]);
-    for (i = 0; i < frame->nb_extended_buf; i++)
+    for (int i = 0; i < frame->nb_extended_buf; i++)
         av_buffer_unref(&frame->extended_buf[i]);
     av_freep(&frame->extended_buf);
     av_dict_free(&frame->metadata);
@@ -531,16 +527,16 @@ FF_ENABLE_DEPRECATION_WARNINGS
 
 int av_frame_is_writable(AVFrame *frame)
 {
-    int i, ret = 1;
+    int ret = 1;
 
     /* assume non-refcounted frames are not writable */
     if (!frame->buf[0])
         return 0;
 
-    for (i = 0; i < FF_ARRAY_ELEMS(frame->buf); i++)
+    for (int i = 0; i < FF_ARRAY_ELEMS(frame->buf); i++)
         if (frame->buf[i])
             ret &= !!av_buffer_is_writable(frame->buf[i]);
-    for (i = 0; i < frame->nb_extended_buf; i++)
+    for (int i = 0; i < frame->nb_extended_buf; i++)
         ret &= !!av_buffer_is_writable(frame->extended_buf[i]);
 
     return ret;
@@ -607,7 +603,7 @@ int av_frame_copy_props(AVFrame *dst, const AVFrame *src)
 AVBufferRef *av_frame_get_plane_buffer(AVFrame *frame, int plane)
 {
     uint8_t *data;
-    int planes, i;
+    int planes;
 
     if (frame->nb_samples) {
         int channels = frame->ch_layout.nb_channels;
@@ -630,12 +626,12 @@ FF_ENABLE_DEPRECATION_WARNINGS
         return NULL;
     data = frame->extended_data[plane];
 
-    for (i = 0; i < FF_ARRAY_ELEMS(frame->buf) && frame->buf[i]; i++) {
+    for (int i = 0; i < FF_ARRAY_ELEMS(frame->buf) && frame->buf[i]; i++) {
         AVBufferRef *buf = frame->buf[i];
         if (data >= buf->data && data < buf->data + buf->size)
             return buf;
     }
-    for (i = 0; i < frame->nb_extended_buf; i++) {
+    for (int i = 0; i < frame->nb_extended_buf; i++) {
         AVBufferRef *buf = frame->extended_buf[i];
         if (data >= buf->data && data < buf->data + buf->size)
             return buf;
@@ -690,9 +686,7 @@ AVFrameSideData *av_frame_new_side_data(AVFrame *frame,
 AVFrameSideData *av_frame_get_side_data(const AVFrame *frame,
                                         enum AVFrameSideDataType type)
 {
-    int i;
-
-    for (i = 0; i < frame->nb_side_data; i++) {
+    for (int i = 0; i < frame->nb_side_data; i++) {
         if (frame->side_data[i]->type == type)
             return frame->side_data[i];
     }
@@ -702,7 +696,7 @@ AVFrameSideData *av_frame_get_side_data(const AVFrame *frame,
 static int frame_copy_video(AVFrame *dst, const AVFrame *src)
 {
     const uint8_t *src_data[4];
-    int i, planes;
+    int planes;
 
     if (dst->width  < src->width ||
         dst->height < src->height)
@@ -712,7 +706,7 @@ static int frame_copy_video(AVFrame *dst, const AVFrame *src)
         return av_hwframe_transfer_data(dst, src, 0);
 
     planes = av_pix_fmt_count_planes(dst->format);
-    for (i = 0; i < planes; i++)
+    for (int i = 0; i < planes; i++)
         if (!dst->data[i] || !src->data[i])
             return AVERROR(EINVAL);
 
@@ -729,7 +723,6 @@ static int frame_copy_audio(AVFrame *dst, const AVFrame *src)
     int planar   = av_sample_fmt_is_planar(dst->format);
     int channels = dst->ch_layout.nb_channels;
     int planes   = planar ? channels : 1;
-    int i;
 
 #if FF_API_OLD_CHANNEL_LAYOUT
 FF_DISABLE_DEPRECATION_WARNINGS
@@ -757,7 +750,7 @@ FF_ENABLE_DEPRECATION_WARNINGS
 #endif
         return AVERROR(EINVAL);
 
-    for (i = 0; i < planes; i++)
+    for (int i = 0; i < planes; i++)
         if (!dst->extended_data[i] || !src->extended_data[i])
             return AVERROR(EINVAL);
 
@@ -789,9 +782,7 @@ FF_ENABLE_DEPRECATION_WARNINGS
 
 void av_frame_remove_side_data(AVFrame *frame, enum AVFrameSideDataType type)
 {
-    int i;
-
-    for (i = frame->nb_side_data - 1; i >= 0; i--) {
+    for (int i = frame->nb_side_data - 1; i >= 0; i--) {
         AVFrameSideData *sd = frame->side_data[i];
         if (sd->type == type) {
             free_side_data(&frame->side_data[i]);
@@ -838,9 +829,7 @@ const char *av_frame_side_data_name(enum AVFrameSideDataType type)
 static int calc_cropping_offsets(size_t offsets[4], const AVFrame *frame,
                                  const AVPixFmtDescriptor *desc)
 {
-    int i, j;
-
-    for (i = 0; frame->data[i]; i++) {
+    for (int i = 0; frame->data[i]; i++) {
         const AVComponentDescriptor *comp = NULL;
         int shift_x = (i == 1 || i == 2) ? desc->log2_chroma_w : 0;
         int shift_y = (i == 1 || i == 2) ? desc->log2_chroma_h : 0;
@@ -851,7 +840,7 @@ static int calc_cropping_offsets(size_t offsets[4], const AVFrame *frame,
         }
 
         /* find any component descriptor for this plane */
-        for (j = 0; j < desc->nb_components; j++) {
+        for (int j = 0; j < desc->nb_components; j++) {
             if (desc->comp[j].plane == i) {
                 comp = &desc->comp[j];
                 break;
@@ -871,7 +860,6 @@ int av_frame_apply_cropping(AVFrame *frame, int flags)
 {
     const AVPixFmtDescriptor *desc;
     size_t offsets[4];
-    int i;
 
     if (!(frame->width > 0 && frame->height > 0))
         return AVERROR(EINVAL);
@@ -906,7 +894,7 @@ int av_frame_apply_cropping(AVFrame *frame, int flags)
         int log2_crop_align = frame->crop_left ? ff_ctz(frame->crop_left) : INT_MAX;
         int min_log2_align = INT_MAX;
 
-        for (i = 0; frame->data[i]; i++) {
+        for (int i = 0; frame->data[i]; i++) {
             int log2_align = offsets[i] ? ff_ctz(offsets[i]) : INT_MAX;
             min_log2_align = FFMIN(log2_align, min_log2_align);
         }
@@ -922,7 +910,7 @@ int av_frame_apply_cropping(AVFrame *frame, int flags)
         }
     }
 
-    for (i = 0; frame->data[i]; i++)
+    for (int i = 0; frame->data[i]; i++)
         frame->data[i] += offsets[i];
 
     frame->width      -= (frame->crop_left + frame->crop_right);
-- 
2.39.2

_______________________________________________
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 v0 02/14] avcodec: move AVCodecInternal allocation to avcodec_alloc_context3

[Jan Ekström]

This allows for private values to be stored before the {de,en}coder
has been opened and initialized.

Add a new unsigned boolean entry to specifically note that a
context has been opened instead of just depending on the internal
pointer.
---
 libavcodec/avcodec.c              | 18 +++++++++++-------
 libavcodec/frame_thread_encoder.c |  7 ++++---
 libavcodec/internal.h             |  5 +++++
 libavcodec/options.c              | 20 +++++++++++++++++++-
 4 files changed, 39 insertions(+), 11 deletions(-)

diff --git a/libavcodec/avcodec.c b/libavcodec/avcodec.c
index fb1362290f..c110b19e08 100644
--- a/libavcodec/avcodec.c
+++ b/libavcodec/avcodec.c
@@ -115,7 +115,7 @@ static int64_t get_bit_rate(AVCodecContext *ctx)
 int attribute_align_arg avcodec_open2(AVCodecContext *avctx, const AVCodec *codec, AVDictionary **options)
 {
     int ret = 0;
-    AVCodecInternal *avci;
+    AVCodecInternal *avci = NULL;
     const FFCodec *codec2;
 
     if (avcodec_is_open(avctx))
@@ -147,12 +147,13 @@ int attribute_align_arg avcodec_open2(AVCodecContext *avctx, const AVCodec *code
     if (avctx->extradata_size < 0 || avctx->extradata_size >= FF_MAX_EXTRADATA_SIZE)
         return AVERROR(EINVAL);
 
-    avci = av_mallocz(sizeof(*avci));
+    avci = avctx->internal;
     if (!avci) {
-        ret = AVERROR(ENOMEM);
-        goto end;
+        av_log(avctx, AV_LOG_ERROR,
+               "This AVCodecContext was not properly allocated! Please utilize "
+               "avcodec_alloc_context3!\n");
+        return AVERROR(EINVAL);
     }
-    avctx->internal = avci;
 
     avci->buffer_frame = av_frame_alloc();
     avci->buffer_pkt = av_packet_alloc();
@@ -360,6 +361,9 @@ FF_ENABLE_DEPRECATION_WARNINGS
 
 end:
 
+    if (ret >= 0 && avci)
+        avci->ctx_opened = 1;
+
     return ret;
 free_and_end:
     avcodec_close(avctx);
@@ -470,7 +474,7 @@ av_cold int avcodec_close(AVCodecContext *avctx)
         ff_icc_context_uninit(&avci->icc);
 #endif
 
-        av_freep(&avctx->internal);
+        avci->ctx_opened = 0;
     }
 
     for (i = 0; i < avctx->nb_coded_side_data; i++)
@@ -703,7 +707,7 @@ void avcodec_string(char *buf, int buf_size, AVCodecContext *enc, int encode)
 
 int avcodec_is_open(AVCodecContext *s)
 {
-    return !!s->internal;
+    return s->internal && s->internal->ctx_opened;
 }
 
 int attribute_align_arg avcodec_receive_frame(AVCodecContext *avctx, AVFrame *frame)
diff --git a/libavcodec/frame_thread_encoder.c b/libavcodec/frame_thread_encoder.c
index 62d9580ad4..683ba52608 100644
--- a/libavcodec/frame_thread_encoder.c
+++ b/libavcodec/frame_thread_encoder.c
@@ -110,8 +110,7 @@ static void * attribute_align_arg worker(void *v){
         pthread_mutex_unlock(&c->finished_task_mutex);
     }
 end:
-    avcodec_close(avctx);
-    av_freep(&avctx);
+    avcodec_free_context(&avctx);
     return NULL;
 }
 
@@ -195,15 +194,17 @@ av_cold int ff_frame_thread_encoder_init(AVCodecContext *avctx)
 
     for(i=0; i<avctx->thread_count ; i++){
         void *tmpv;
+        AVCodecInternal *avci;
         thread_avctx = avcodec_alloc_context3(avctx->codec);
         if (!thread_avctx) {
             ret = AVERROR(ENOMEM);
             goto fail;
         }
         tmpv = thread_avctx->priv_data;
+        avci = thread_avctx->internal;
         *thread_avctx = *avctx;
         thread_avctx->priv_data = tmpv;
-        thread_avctx->internal = NULL;
+        thread_avctx->internal = avci;
         thread_avctx->hw_frames_ctx = NULL;
         ret = av_opt_copy(thread_avctx, avctx);
         if (ret < 0)
diff --git a/libavcodec/internal.h b/libavcodec/internal.h
index a283c52e01..f21101752d 100644
--- a/libavcodec/internal.h
+++ b/libavcodec/internal.h
@@ -163,6 +163,11 @@ typedef struct AVCodecInternal {
 #if CONFIG_LCMS2
     FFIccContext icc; /* used to read and write embedded ICC profiles */
 #endif
+
+    /**
+     * a boolean to describe whether context is opened or not.
+     */
+    unsigned int ctx_opened;
 } AVCodecInternal;
 
 /**
diff --git a/libavcodec/options.c b/libavcodec/options.c
index a9b35ee1c3..f8fab164fb 100644
--- a/libavcodec/options.c
+++ b/libavcodec/options.c
@@ -28,6 +28,7 @@
 
 #include "avcodec.h"
 #include "codec_internal.h"
+#include "internal.h"
 #include "libavutil/avassert.h"
 #include "libavutil/internal.h"
 #include "libavutil/mem.h"
@@ -89,6 +90,7 @@ static const AVClass av_codec_context_class = {
 static int init_context_defaults(AVCodecContext *s, const AVCodec *codec)
 {
     const FFCodec *const codec2 = ffcodec(codec);
+    AVCodecInternal *avci = NULL;
     int flags=0;
     memset(s, 0, sizeof(AVCodecContext));
 
@@ -132,7 +134,7 @@ FF_ENABLE_DEPRECATION_WARNINGS
     if(codec && codec2->priv_data_size){
         s->priv_data = av_mallocz(codec2->priv_data_size);
         if (!s->priv_data)
-            return AVERROR(ENOMEM);
+            goto alloc_fail;
         if(codec->priv_class){
             *(const AVClass**)s->priv_data = codec->priv_class;
             av_opt_set_defaults(s->priv_data);
@@ -147,7 +149,21 @@ FF_ENABLE_DEPRECATION_WARNINGS
             d++;
         }
     }
+
+    avci = av_mallocz(sizeof(*avci));
+    if (!avci)
+        goto alloc_fail;
+
+    s->internal = avci;
+
     return 0;
+
+alloc_fail:
+    av_freep(&s->internal);
+
+    av_freep(&s->priv_data);
+
+    return AVERROR(ENOMEM);
 }
 
 AVCodecContext *avcodec_alloc_context3(const AVCodec *codec)
@@ -174,6 +190,8 @@ void avcodec_free_context(AVCodecContext **pavctx)
 
     avcodec_close(avctx);
 
+    av_freep(&avctx->internal);
+
     av_freep(&avctx->extradata);
     av_freep(&avctx->subtitle_header);
     av_freep(&avctx->intra_matrix);
-- 
2.39.2

_______________________________________________
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 v0 03/14] 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 5b58c14ac3..45024c2a03 100644
--- a/libavutil/frame.h
+++ b/libavutil/frame.h
@@ -241,6 +241,14 @@ typedef struct AVFrameSideData {
     AVBufferRef *buf;
 } AVFrameSideData;
 
+/**
+ * Structure to hold a set of AVFrameSideData
+ */
+typedef struct AVFrameSideDataSet {
+    AVFrameSideData **side_data;
+    int            nb_side_data;
+} AVFrameSideDataSet;
+
 /**
  * Structure describing a single Region Of Interest.
  *
-- 
2.39.2

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

[Jan Ekström]

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

diff --git a/libavutil/frame.c b/libavutil/frame.c
index c905e8d611..019613e4d2 100644
--- a/libavutil/frame.c
+++ b/libavutil/frame.c
@@ -76,14 +76,18 @@ 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 wipe_side_data_from_frame(AVFrame *frame) {
+    wipe_side_data(&frame->side_data, &frame->nb_side_data);
 }
 
 AVFrame *av_frame_alloc(void)
@@ -326,7 +330,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);
+                wipe_side_data_from_frame(dst);
                 return AVERROR(ENOMEM);
             }
             memcpy(sd_dst->data, sd_src->data, sd_src->size);
@@ -335,7 +339,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);
+                wipe_side_data_from_frame(dst);
                 return AVERROR(ENOMEM);
             }
         }
@@ -486,7 +490,7 @@ void av_frame_unref(AVFrame *frame)
     if (!frame)
         return;
 
-    wipe_side_data(frame);
+    wipe_side_data_from_frame(frame);
 
     for (int i = 0; i < FF_ARRAY_ELEMS(frame->buf); i++)
         av_buffer_unref(&frame->buf[i]);
-- 
2.39.2

_______________________________________________
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 v0 05/14] avutil/frame: add helper for clearing out side data sets

[Jan Ekström]

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

diff --git a/libavutil/frame.c b/libavutil/frame.c
index 019613e4d2..24038cc0fa 100644
--- a/libavutil/frame.c
+++ b/libavutil/frame.c
@@ -90,6 +90,11 @@ static void wipe_side_data_from_frame(AVFrame *frame) {
     wipe_side_data(&frame->side_data, &frame->nb_side_data);
 }
 
+void av_side_data_set_wipe(AVFrameSideDataSet *set)
+{
+    wipe_side_data(&set->side_data, &set->nb_side_data);
+}
+
 AVFrame *av_frame_alloc(void)
 {
     AVFrame *frame = av_malloc(sizeof(*frame));
diff --git a/libavutil/frame.h b/libavutil/frame.h
index 45024c2a03..734ac3fe75 100644
--- a/libavutil/frame.h
+++ b/libavutil/frame.h
@@ -991,6 +991,13 @@ int av_frame_apply_cropping(AVFrame *frame, int flags);
  */
 const char *av_frame_side_data_name(enum AVFrameSideDataType type);
 
+/**
+ * Clear all side data from a side data set
+ *
+ * @param set the set which should be cleared
+ */
+void av_side_data_set_wipe(AVFrameSideDataSet *set);
+
 /**
  * @}
  */
-- 
2.39.2

_______________________________________________
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 v0 06/14] avutil/frame: split side_data_from_buf to base and AVFrame func

[Jan Ekström]

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

diff --git a/libavutil/frame.c b/libavutil/frame.c
index 24038cc0fa..ab1a4e7f6a 100644
--- a/libavutil/frame.c
+++ b/libavutil/frame.c
@@ -648,23 +648,23 @@ FF_ENABLE_DEPRECATION_WARNINGS
     return NULL;
 }
 
-AVFrameSideData *av_frame_new_side_data_from_buf(AVFrame *frame,
-                                                 enum AVFrameSideDataType type,
-                                                 AVBufferRef *buf)
+static AVFrameSideData *add_side_data_to_set_from_buf(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_side_data > INT_MAX / sizeof(*set->side_data) - 1)
         return NULL;
 
-    tmp = av_realloc(frame->side_data,
-                     (frame->nb_side_data + 1) * sizeof(*frame->side_data));
+    tmp = av_realloc(set->side_data,
+                     (set->nb_side_data + 1) * sizeof(*set->side_data));
     if (!tmp)
         return NULL;
-    frame->side_data = tmp;
+    set->side_data = tmp;
 
     ret = av_mallocz(sizeof(*ret));
     if (!ret)
@@ -675,7 +675,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->side_data[set->nb_side_data++] = ret;
+
+    return ret;
+}
+
+AVFrameSideData *av_frame_new_side_data_from_buf(AVFrame *frame,
+                                                 enum AVFrameSideDataType type,
+                                                 AVBufferRef *buf)
+{
+    AVFrameSideDataSet set = {
+        .side_data    = frame->side_data,
+        .nb_side_data = frame->nb_side_data,
+    };
+    AVFrameSideData *ret = add_side_data_to_set_from_buf(&set, type, buf);
+
+    frame->side_data = set.side_data;
+    frame->nb_side_data = set.nb_side_data;
 
     return ret;
 }
-- 
2.39.2

_______________________________________________
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 v0 07/14] avutil/frame: add helper for adding side data to set

[Jan Ekström]

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

diff --git a/libavutil/frame.c b/libavutil/frame.c
index ab1a4e7f6a..29e9b631f8 100644
--- a/libavutil/frame.c
+++ b/libavutil/frame.c
@@ -708,6 +708,18 @@ AVFrameSideData *av_frame_new_side_data(AVFrame *frame,
     return ret;
 }
 
+
+AVFrameSideData *av_new_side_data_to_set(AVFrameSideDataSet *set,
+                                         enum AVFrameSideDataType type,
+                                         size_t size)
+{
+    AVBufferRef     *buf = av_buffer_alloc(size);
+    AVFrameSideData *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 734ac3fe75..167a8f0ff6 100644
--- a/libavutil/frame.h
+++ b/libavutil/frame.h
@@ -998,6 +998,19 @@ const char *av_frame_side_data_name(enum AVFrameSideDataType type);
  */
 void av_side_data_set_wipe(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
+ *
+ * @return newly added side data on success, NULL on error
+ */
+AVFrameSideData *av_new_side_data_to_set(AVFrameSideDataSet *set,
+                                         enum AVFrameSideDataType type,
+                                         size_t size);
+
 /**
  * @}
  */
-- 
2.39.2

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

[Jan Ekström]

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

diff --git a/libavutil/frame.c b/libavutil/frame.c
index 29e9b631f8..3386cda627 100644
--- a/libavutil/frame.c
+++ b/libavutil/frame.c
@@ -720,16 +720,28 @@ AVFrameSideData *av_new_side_data_to_set(AVFrameSideDataSet *set,
     return ret;
 }
 
-AVFrameSideData *av_frame_get_side_data(const AVFrame *frame,
-                                        enum AVFrameSideDataType type)
+AVFrameSideData *av_get_side_data_from_set(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_side_data; i++) {
+        if (set.side_data[i]->type == type)
+            return set.side_data[i];
     }
     return NULL;
 }
 
+AVFrameSideData *av_frame_get_side_data(const AVFrame *frame,
+                                        enum AVFrameSideDataType type)
+{
+    return av_get_side_data_from_set(
+        (const AVFrameSideDataSet){
+            .side_data    = frame->side_data,
+            .nb_side_data = 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 167a8f0ff6..8aa50e3ad8 100644
--- a/libavutil/frame.h
+++ b/libavutil/frame.h
@@ -1011,6 +1011,16 @@ AVFrameSideData *av_new_side_data_to_set(AVFrameSideDataSet *set,
                                          enum AVFrameSideDataType type,
                                          size_t size);
 
+/**
+ * @param set a set to which the side data should be added
+ * @param type type of the added side data
+ *
+ * @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_get_side_data_from_set(const AVFrameSideDataSet set,
+                                           enum AVFrameSideDataType type);
+
 /**
  * @}
  */
-- 
2.39.2

_______________________________________________
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 v0 09/14] avcodec: add private side data set to AVCodecInternal

[Jan Ekström]

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

diff --git a/libavcodec/avcodec.c b/libavcodec/avcodec.c
index c110b19e08..3faabe77d1 100644
--- a/libavcodec/avcodec.c
+++ b/libavcodec/avcodec.c
@@ -403,6 +403,7 @@ void avcodec_flush_buffers(AVCodecContext *avctx)
     avci->nb_draining_errors = 0;
     av_frame_unref(avci->buffer_frame);
     av_packet_unref(avci->buffer_pkt);
+    av_side_data_set_wipe(&avci->side_data_set);
 
     if (HAVE_THREADS && avctx->active_thread_type & FF_THREAD_FRAME)
         ff_thread_flush(avctx);
diff --git a/libavcodec/internal.h b/libavcodec/internal.h
index f21101752d..c658e97313 100644
--- a/libavcodec/internal.h
+++ b/libavcodec/internal.h
@@ -168,6 +168,13 @@ typedef struct AVCodecInternal {
      * a boolean to describe whether context is opened or not.
      */
     unsigned int ctx_opened;
+
+    /**
+     * Set holding static side data, such as HDR10 CLL / MDCV structures.
+     * - encoding: set by user
+     * - decoding: unused
+     */
+    AVFrameSideDataSet side_data_set;
 } AVCodecInternal;
 
 /**
diff --git a/libavcodec/options.c b/libavcodec/options.c
index f8fab164fb..acd3472fde 100644
--- a/libavcodec/options.c
+++ b/libavcodec/options.c
@@ -184,12 +184,17 @@ AVCodecContext *avcodec_alloc_context3(const AVCodec *codec)
 void avcodec_free_context(AVCodecContext **pavctx)
 {
     AVCodecContext *avctx = *pavctx;
+    AVCodecInternal *avci = NULL;
 
     if (!avctx)
         return;
 
     avcodec_close(avctx);
 
+    avci = avctx->internal;
+    if (avci)
+        av_side_data_set_wipe(&avci->side_data_set);
+
     av_freep(&avctx->internal);
 
     av_freep(&avctx->extradata);
-- 
2.39.2

_______________________________________________
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 v0 10/14] avcodec: add function for setting avctx side data

[Jan Ekström]

---
 libavcodec/avcodec.c | 30 ++++++++++++++++++++++++++++++
 libavcodec/avcodec.h | 12 ++++++++++++
 2 files changed, 42 insertions(+)

diff --git a/libavcodec/avcodec.c b/libavcodec/avcodec.c
index 3faabe77d1..9ffff28d70 100644
--- a/libavcodec/avcodec.c
+++ b/libavcodec/avcodec.c
@@ -29,6 +29,7 @@
 #include "libavutil/bprint.h"
 #include "libavutil/channel_layout.h"
 #include "libavutil/fifo.h"
+#include "libavutil/frame.h"
 #include "libavutil/imgutils.h"
 #include "libavutil/mem.h"
 #include "libavutil/opt.h"
@@ -719,3 +720,32 @@ int attribute_align_arg avcodec_receive_frame(AVCodecContext *avctx, AVFrame *fr
         return ff_decode_receive_frame(avctx, frame);
     return ff_encode_receive_frame(avctx, frame);
 }
+
+int avcodec_configure_side_data(AVCodecContext *avctx,
+                                const AVFrameSideDataSet set)
+{
+    if (!avctx || !avctx->internal || !av_codec_is_encoder(avctx->codec))
+        return AVERROR(EINVAL);
+
+    {
+        AVCodecInternal    *avci = avctx->internal;
+        AVFrameSideDataSet *dst  = &avci->side_data_set;
+
+        for (int i = 0; i < set.nb_side_data; i++) {
+            const AVFrameSideData *sd_src = set.side_data[i];
+            AVFrameSideData *sd_dst =
+                av_new_side_data_to_set(dst, sd_src->type,
+                                        sd_src->size);
+            if (!sd_dst) {
+                av_side_data_set_wipe(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;
+    }
+}
diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
index 30f1d312f4..8f535a0cc8 100644
--- a/libavcodec/avcodec.h
+++ b/libavcodec/avcodec.h
@@ -3223,6 +3223,18 @@ void av_fast_padded_mallocz(void *ptr, unsigned int *size, size_t min_size);
  */
 int avcodec_is_open(AVCodecContext *s);
 
+/**
+ * Configure a side data set to an encoder AVCodecContext. With multiple
+ * calls new side data gets added in addition to the existing set of side data.
+ *
+ * @param avctx codec context to which to add side data
+ * @param set   set of side data to add
+ *
+* @return negative error code on failure, >=0 on success.
+ */
+int avcodec_configure_side_data(AVCodecContext *avctx,
+                                const AVFrameSideDataSet set);
+
 /**
  * @}
  */
-- 
2.39.2

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

[Jan Ekström]

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

diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
index d721a5e721..e12d6ce25d 100644
--- a/fftools/ffmpeg.c
+++ b/fftools/ffmpeg.c
@@ -3113,11 +3113,23 @@ static int init_output_stream_encode(OutputStream *ost, AVFrame *frame)
                                                  av_pix_fmt_desc_get(enc_ctx->pix_fmt)->comp[0].depth);
 
         if (frame) {
+            int ret = AVERROR_BUG;
+
             enc_ctx->color_range            = frame->color_range;
             enc_ctx->color_primaries        = frame->color_primaries;
             enc_ctx->color_trc              = frame->color_trc;
             enc_ctx->colorspace             = frame->colorspace;
             enc_ctx->chroma_sample_location = frame->chroma_location;
+
+            if ((ret = avcodec_configure_side_data(enc_ctx,
+                                                   (const AVFrameSideDataSet){
+                                                       .side_data    = frame->side_data,
+                                                       .nb_side_data = frame->nb_side_data
+                                                   })) < 0) {
+                av_log(NULL, AV_LOG_ERROR, "failed to configure video encoder: %s!\n",
+                       av_err2str(ret));
+                return ret;
+            }
         }
 
         enc_ctx->framerate = ost->frame_rate;
-- 
2.39.2

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

[Jan Ekström]

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

In other words, no additional version limiting conditions seem
to be required.
---
 libavcodec/libsvtav1.c | 70 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 70 insertions(+)

diff --git a/libavcodec/libsvtav1.c b/libavcodec/libsvtav1.c
index 9174e2753c..a3fb3535c9 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->internal->side_data_set;
+    const AVFrameSideData *cll_sd =
+        av_get_side_data_from_set(set, AV_FRAME_DATA_CONTENT_LIGHT_LEVEL);
+    const AVFrameSideData *mdcv_sd =
+        av_get_side_data_from_set(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)
 {
@@ -256,6 +324,8 @@ static int config_enc_params(EbSvtAv1EncConfiguration *param,
     /* 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);
-- 
2.39.2

_______________________________________________
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 v0 13/14] avcodec/libx264: add support for writing out CLL and MDCV

[Jan Ekström]

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

Keep handle_side_data available in all cases as this way X264_init
does not require additional version based conditions within it.
---
 libavcodec/libx264.c | 79 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 79 insertions(+)

diff --git a/libavcodec/libx264.c b/libavcodec/libx264.c
index 92828fabc3..83c870609a 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"
@@ -737,6 +738,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->internal->side_data_set;
+    const AVFrameSideData *cll_sd =
+        av_get_side_data_from_set(set, AV_FRAME_DATA_CONTENT_LIGHT_LEVEL);
+    const AVFrameSideData *mdcv_sd =
+        av_get_side_data_from_set(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;
@@ -1031,6 +1108,8 @@ static av_cold int X264_init(AVCodecContext *avctx)
     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;
 
-- 
2.39.2

_______________________________________________
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 v0 14/14] avcodec/libx265: add support for writing out CLL and MDCV

[Jan Ekström]

The newer of these two are the separate integers for content light
level, introduced in 3952bf3e98c76c31594529a3fe34e056d3e3e2ea ,
with X265_BUILD 75. As we already require X265_BUILD of at least
89, no further conditions are required.
---
 libavcodec/libx265.c | 82 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 82 insertions(+)

diff --git a/libavcodec/libx265.c b/libavcodec/libx265.c
index 420d0953af..497f31aa34 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->internal->side_data_set;
+    const AVFrameSideData *cll_sd =
+        av_get_side_data_from_set(set, AV_FRAME_DATA_CONTENT_LIGHT_LEVEL);
+    const AVFrameSideData *mdcv_sd =
+        av_get_side_data_from_set(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;
@@ -333,6 +408,13 @@ static av_cold int libx265_encode_init(AVCodecContext *avctx)
         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];
 
-- 
2.39.2

_______________________________________________
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 v0 01/14] avutil/frame: move counters utilized in loops to their scope

[Anton Khirnov]

Quoting Jan Ekström (2023-03-21 00:33:55)
> This way we can clean up separate definitions in functions with
> just a single loop, as well as have no reuse between different
> loops' counters in functions with multiple.
> ---
>  libavutil/frame.c | 76 ++++++++++++++++++++---------------------------
>  1 file changed, 32 insertions(+), 44 deletions(-)

Looks ok

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

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

Re: [FFmpeg-devel] [PATCH v0 02/14] avcodec: move AVCodecInternal allocation to avcodec_alloc_context3

[Anton Khirnov]

Quoting Jan Ekström (2023-03-21 00:33:56)
> This allows for private values to be stored before the {de,en}coder
> has been opened and initialized.
> 
> Add a new unsigned boolean entry to specifically note that a
> context has been opened instead of just depending on the internal
> pointer.
> ---
>  libavcodec/avcodec.c              | 18 +++++++++++-------
>  libavcodec/frame_thread_encoder.c |  7 ++++---
>  libavcodec/internal.h             |  5 +++++
>  libavcodec/options.c              | 20 +++++++++++++++++++-
>  4 files changed, 39 insertions(+), 11 deletions(-)
> 
> diff --git a/libavcodec/avcodec.c b/libavcodec/avcodec.c
> index fb1362290f..c110b19e08 100644
> --- a/libavcodec/avcodec.c
> +++ b/libavcodec/avcodec.c
> @@ -115,7 +115,7 @@ static int64_t get_bit_rate(AVCodecContext *ctx)
>  int attribute_align_arg avcodec_open2(AVCodecContext *avctx, const AVCodec *codec, AVDictionary **options)
>  {
>      int ret = 0;
> -    AVCodecInternal *avci;
> +    AVCodecInternal *avci = NULL;

Just initialize it to avctx->avci.

>      const FFCodec *codec2;
>  
>      if (avcodec_is_open(avctx))
> @@ -147,12 +147,13 @@ int attribute_align_arg avcodec_open2(AVCodecContext *avctx, const AVCodec *code
>      if (avctx->extradata_size < 0 || avctx->extradata_size >= FF_MAX_EXTRADATA_SIZE)
>          return AVERROR(EINVAL);
>  
> -    avci = av_mallocz(sizeof(*avci));
> +    avci = avctx->internal;
>      if (!avci) {
> -        ret = AVERROR(ENOMEM);
> -        goto end;
> +        av_log(avctx, AV_LOG_ERROR,
> +               "This AVCodecContext was not properly allocated! Please utilize "
> +               "avcodec_alloc_context3!\n");
> +        return AVERROR(EINVAL);

This should be an assert.

> @@ -147,7 +149,21 @@ FF_ENABLE_DEPRECATION_WARNINGS
>              d++;
>          }
>      }
> +
> +    avci = av_mallocz(sizeof(*avci));
> +    if (!avci)
> +        goto alloc_fail;
> +
> +    s->internal = avci;
> +
>      return 0;
> +
> +alloc_fail:
> +    av_freep(&s->internal);
> +
> +    av_freep(&s->priv_data);
> +
> +    return AVERROR(ENOMEM);

A bit overdoing it with empty lines.

Otherwise looks ok.

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

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

Re: [FFmpeg-devel] [PATCH v0 10/14] avcodec: add function for setting avctx side data

[Anton Khirnov]

Quoting Jan Ekström (2023-03-21 00:34:04)
> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> index 30f1d312f4..8f535a0cc8 100644
> --- a/libavcodec/avcodec.h
> +++ b/libavcodec/avcodec.h
> @@ -3223,6 +3223,18 @@ void av_fast_padded_mallocz(void *ptr, unsigned int *size, size_t min_size);
>   */
>  int avcodec_is_open(AVCodecContext *s);
>  
> +/**
> + * Configure a side data set to an encoder AVCodecContext. With multiple
> + * calls new side data gets added in addition to the existing set of side data.

I have no idea what does "configure" mean in this context and what
effect will this actually have.

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

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

Re: [FFmpeg-devel] [PATCH v0 09/14] avcodec: add private side data set to AVCodecInternal

[Anton Khirnov]

Quoting Jan Ekström (2023-03-21 00:34:03)
> This allows configuring an encoder by using AVFrameSideData.
> ---
>  libavcodec/avcodec.c  | 1 +
>  libavcodec/internal.h | 7 +++++++
>  libavcodec/options.c  | 5 +++++
>  3 files changed, 13 insertions(+)
> 
> diff --git a/libavcodec/avcodec.c b/libavcodec/avcodec.c
> index c110b19e08..3faabe77d1 100644
> --- a/libavcodec/avcodec.c
> +++ b/libavcodec/avcodec.c
> @@ -403,6 +403,7 @@ void avcodec_flush_buffers(AVCodecContext *avctx)
>      avci->nb_draining_errors = 0;
>      av_frame_unref(avci->buffer_frame);
>      av_packet_unref(avci->buffer_pkt);
> +    av_side_data_set_wipe(&avci->side_data_set);
>  
>      if (HAVE_THREADS && avctx->active_thread_type & FF_THREAD_FRAME)
>          ff_thread_flush(avctx);
> diff --git a/libavcodec/internal.h b/libavcodec/internal.h
> index f21101752d..c658e97313 100644
> --- a/libavcodec/internal.h
> +++ b/libavcodec/internal.h
> @@ -168,6 +168,13 @@ typedef struct AVCodecInternal {
>       * a boolean to describe whether context is opened or not.
>       */
>      unsigned int ctx_opened;
> +
> +    /**
> +     * Set holding static side data, such as HDR10 CLL / MDCV structures.
> +     * - encoding: set by user
> +     * - decoding: unused
> +     */
> +    AVFrameSideDataSet side_data_set;

Why put it here and not in the public struct? It seems way more natural
there.

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

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

Re: [FFmpeg-devel] [PATCH v0 01/14] avutil/frame: move counters utilized in loops to their scope

[Jan Ekström]

On Fri, Mar 24, 2023 at 12:33 PM Anton Khirnov <anton@khirnov.net> wrote:
>
> Quoting Jan Ekström (2023-03-21 00:33:55)
> > This way we can clean up separate definitions in functions with
> > just a single loop, as well as have no reuse between different
> > loops' counters in functions with multiple.
> > ---
> >  libavutil/frame.c | 76 ++++++++++++++++++++---------------------------
> >  1 file changed, 32 insertions(+), 44 deletions(-)
>
> Looks ok
>

Thanks. Applied as 90488e14408119f6c3a2061a6c26ee8bcfaddbce , as this
was just posted as part of this set in order to make it applicable, as
it was posted earlier on the ML and received no ACKs :D .

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 v0 02/14] avcodec: move AVCodecInternal allocation to avcodec_alloc_context3

[Andreas Rheinhardt]

Jan Ekström:
> This allows for private values to be stored before the {de,en}coder
> has been opened and initialized.
> 
> Add a new unsigned boolean entry to specifically note that a
> context has been opened instead of just depending on the internal
> pointer.
> ---
>  libavcodec/avcodec.c              | 18 +++++++++++-------
>  libavcodec/frame_thread_encoder.c |  7 ++++---
>  libavcodec/internal.h             |  5 +++++
>  libavcodec/options.c              | 20 +++++++++++++++++++-
>  4 files changed, 39 insertions(+), 11 deletions(-)
> 
> diff --git a/libavcodec/avcodec.c b/libavcodec/avcodec.c
> index fb1362290f..c110b19e08 100644
> --- a/libavcodec/avcodec.c
> +++ b/libavcodec/avcodec.c
> @@ -115,7 +115,7 @@ static int64_t get_bit_rate(AVCodecContext *ctx)
>  int attribute_align_arg avcodec_open2(AVCodecContext *avctx, const AVCodec *codec, AVDictionary **options)
>  {
>      int ret = 0;
> -    AVCodecInternal *avci;
> +    AVCodecInternal *avci = NULL;
>      const FFCodec *codec2;
>  
>      if (avcodec_is_open(avctx))
> @@ -147,12 +147,13 @@ int attribute_align_arg avcodec_open2(AVCodecContext *avctx, const AVCodec *code
>      if (avctx->extradata_size < 0 || avctx->extradata_size >= FF_MAX_EXTRADATA_SIZE)
>          return AVERROR(EINVAL);
>  
> -    avci = av_mallocz(sizeof(*avci));
> +    avci = avctx->internal;
>      if (!avci) {
> -        ret = AVERROR(ENOMEM);
> -        goto end;
> +        av_log(avctx, AV_LOG_ERROR,
> +               "This AVCodecContext was not properly allocated! Please utilize "
> +               "avcodec_alloc_context3!\n");
> +        return AVERROR(EINVAL);
>      }
> -    avctx->internal = avci;
>  
>      avci->buffer_frame = av_frame_alloc();
>      avci->buffer_pkt = av_packet_alloc();
> @@ -360,6 +361,9 @@ FF_ENABLE_DEPRECATION_WARNINGS
>  
>  end:
>  
> +    if (ret >= 0 && avci)
> +        avci->ctx_opened = 1;

This should be moved before end: in order to save the checks.

> +
>      return ret;
>  free_and_end:
>      avcodec_close(avctx);
> @@ -470,7 +474,7 @@ av_cold int avcodec_close(AVCodecContext *avctx)
>          ff_icc_context_uninit(&avci->icc);
>  #endif
>  
> -        av_freep(&avctx->internal);
> +        avci->ctx_opened = 0;
>      }
>  
>      for (i = 0; i < avctx->nb_coded_side_data; i++)
> @@ -703,7 +707,7 @@ void avcodec_string(char *buf, int buf_size, AVCodecContext *enc, int encode)
>  
>  int avcodec_is_open(AVCodecContext *s)
>  {
> -    return !!s->internal;
> +    return s->internal && s->internal->ctx_opened;
>  }
>  
>  int attribute_align_arg avcodec_receive_frame(AVCodecContext *avctx, AVFrame *frame)
> diff --git a/libavcodec/frame_thread_encoder.c b/libavcodec/frame_thread_encoder.c
> index 62d9580ad4..683ba52608 100644
> --- a/libavcodec/frame_thread_encoder.c
> +++ b/libavcodec/frame_thread_encoder.c
> @@ -110,8 +110,7 @@ static void * attribute_align_arg worker(void *v){
>          pthread_mutex_unlock(&c->finished_task_mutex);
>      }
>  end:
> -    avcodec_close(avctx);
> -    av_freep(&avctx);
> +    avcodec_free_context(&avctx);

This will also free the other stuff in avcodec_free_context() and might
therefore cause double-frees.

>      return NULL;
>  }
>  
> @@ -195,15 +194,17 @@ av_cold int ff_frame_thread_encoder_init(AVCodecContext *avctx)
>  
>      for(i=0; i<avctx->thread_count ; i++){
>          void *tmpv;
> +        AVCodecInternal *avci;
>          thread_avctx = avcodec_alloc_context3(avctx->codec);
>          if (!thread_avctx) {
>              ret = AVERROR(ENOMEM);
>              goto fail;
>          }
>          tmpv = thread_avctx->priv_data;
> +        avci = thread_avctx->internal;
>          *thread_avctx = *avctx;
>          thread_avctx->priv_data = tmpv;
> -        thread_avctx->internal = NULL;
> +        thread_avctx->internal = avci;
>          thread_avctx->hw_frames_ctx = NULL;
>          ret = av_opt_copy(thread_avctx, avctx);
>          if (ret < 0)
> diff --git a/libavcodec/internal.h b/libavcodec/internal.h
> index a283c52e01..f21101752d 100644
> --- a/libavcodec/internal.h
> +++ b/libavcodec/internal.h
> @@ -163,6 +163,11 @@ typedef struct AVCodecInternal {
>  #if CONFIG_LCMS2
>      FFIccContext icc; /* used to read and write embedded ICC profiles */
>  #endif
> +
> +    /**
> +     * a boolean to describe whether context is opened or not.
> +     */
> +    unsigned int ctx_opened;
>  } AVCodecInternal;
>  
>  /**
> diff --git a/libavcodec/options.c b/libavcodec/options.c
> index a9b35ee1c3..f8fab164fb 100644
> --- a/libavcodec/options.c
> +++ b/libavcodec/options.c
> @@ -28,6 +28,7 @@
>  
>  #include "avcodec.h"
>  #include "codec_internal.h"
> +#include "internal.h"
>  #include "libavutil/avassert.h"
>  #include "libavutil/internal.h"
>  #include "libavutil/mem.h"
> @@ -89,6 +90,7 @@ static const AVClass av_codec_context_class = {
>  static int init_context_defaults(AVCodecContext *s, const AVCodec *codec)
>  {
>      const FFCodec *const codec2 = ffcodec(codec);
> +    AVCodecInternal *avci = NULL;
>      int flags=0;
>      memset(s, 0, sizeof(AVCodecContext));
>  
> @@ -132,7 +134,7 @@ FF_ENABLE_DEPRECATION_WARNINGS
>      if(codec && codec2->priv_data_size){
>          s->priv_data = av_mallocz(codec2->priv_data_size);
>          if (!s->priv_data)
> -            return AVERROR(ENOMEM);
> +            goto alloc_fail;
>          if(codec->priv_class){
>              *(const AVClass**)s->priv_data = codec->priv_class;
>              av_opt_set_defaults(s->priv_data);
> @@ -147,7 +149,21 @@ FF_ENABLE_DEPRECATION_WARNINGS
>              d++;
>          }
>      }
> +
> +    avci = av_mallocz(sizeof(*avci));
> +    if (!avci)
> +        goto alloc_fail;
> +
> +    s->internal = avci;
> +
>      return 0;
> +
> +alloc_fail:
> +    av_freep(&s->internal);
> +
> +    av_freep(&s->priv_data);
> +
> +    return AVERROR(ENOMEM);
>  }
>  
>  AVCodecContext *avcodec_alloc_context3(const AVCodec *codec)
> @@ -174,6 +190,8 @@ void avcodec_free_context(AVCodecContext **pavctx)
>  
>      avcodec_close(avctx);
>  
> +    av_freep(&avctx->internal);

Moving this to avcodec_free_context() creates a leak when using
avcodec_close()+av_free(). This can be fixed by allocating the
AVCodecContext and the AVCodecInternal jointly.

> +
>      av_freep(&avctx->extradata);
>      av_freep(&avctx->subtitle_header);
>      av_freep(&avctx->intra_matrix);

_______________________________________________
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 v0 02/14] avcodec: move AVCodecInternal allocation to avcodec_alloc_context3

[James Almer]

On 3/24/2023 9:07 AM, Andreas Rheinhardt wrote:
>> @@ -174,6 +190,8 @@ void avcodec_free_context(AVCodecContext **pavctx)
>>   
>>       avcodec_close(avctx);
>>   
>> +    av_freep(&avctx->internal);
> Moving this to avcodec_free_context() creates a leak when using
> avcodec_close()+av_free(). This can be fixed by allocating the
> AVCodecContext and the AVCodecInternal jointly.

Can't we just declare that doing av_free() on a AVCodecContext is not a 
valid API usage? Every other struct with an specific free function is 
very clear about it being the only way to free them.

> 
>> +
>>       av_freep(&avctx->extradata);
>>       av_freep(&avctx->subtitle_header);
>>       av_freep(&avctx->intra_matrix);
_______________________________________________
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 v0 02/14] avcodec: move AVCodecInternal allocation to avcodec_alloc_context3

[Anton Khirnov]

Quoting James Almer (2023-03-24 14:02:40)
> On 3/24/2023 9:07 AM, Andreas Rheinhardt wrote:
> >> @@ -174,6 +190,8 @@ void avcodec_free_context(AVCodecContext **pavctx)
> >>   
> >>       avcodec_close(avctx);
> >>   
> >> +    av_freep(&avctx->internal);
> > Moving this to avcodec_free_context() creates a leak when using
> > avcodec_close()+av_free(). This can be fixed by allocating the
> > AVCodecContext and the AVCodecInternal jointly.
> 
> Can't we just declare that doing av_free() on a AVCodecContext is not a 
> valid API usage? Every other struct with an specific free function is 
> very clear about it being the only way to free them.

Sadly I expect many callers still do this, even though
avcodec_free_context() has existed since 2014. The proper solution is to
deprecated avcodec_close(), but that needs a new parser API.

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

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

Re: [FFmpeg-devel] [PATCH v0 02/14] avcodec: move AVCodecInternal allocation to avcodec_alloc_context3

[Michael Niedermayer]

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

On Tue, Mar 21, 2023 at 01:33:56AM +0200, Jan Ekström wrote:
> This allows for private values to be stored before the {de,en}coder
> has been opened and initialized.
> 
> Add a new unsigned boolean entry to specifically note that a
> context has been opened instead of just depending on the internal
> pointer.
> ---
>  libavcodec/avcodec.c              | 18 +++++++++++-------
>  libavcodec/frame_thread_encoder.c |  7 ++++---
>  libavcodec/internal.h             |  5 +++++
>  libavcodec/options.c              | 20 +++++++++++++++++++-
>  4 files changed, 39 insertions(+), 11 deletions(-)

this causes memory corruption with mjpeg
./ffmpeg_g -i lena.pnm -qscale 4 -intra_matrix 10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10 -chroma_intra_matrix 400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400 -bitexact file-custommatrix10,400.jpg

i see

video:19kB audio:0kB subtitle:0kB other streams:0kB global headers:0kB muxing overhead: unknown
corrupted size vs. prev_size
Aborted (core dumped)

i can provide more details if it isnt reproduceable

thx

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

If you think the mosad wants you dead since a long time then you are either
wrong or dead since a long time.

[-- 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 v0 02/14] avcodec: move AVCodecInternal allocation to avcodec_alloc_context3

[Andreas Rheinhardt]

Michael Niedermayer:
> On Tue, Mar 21, 2023 at 01:33:56AM +0200, Jan Ekström wrote:
>> This allows for private values to be stored before the {de,en}coder
>> has been opened and initialized.
>>
>> Add a new unsigned boolean entry to specifically note that a
>> context has been opened instead of just depending on the internal
>> pointer.
>> ---
>>  libavcodec/avcodec.c              | 18 +++++++++++-------
>>  libavcodec/frame_thread_encoder.c |  7 ++++---
>>  libavcodec/internal.h             |  5 +++++
>>  libavcodec/options.c              | 20 +++++++++++++++++++-
>>  4 files changed, 39 insertions(+), 11 deletions(-)
> 
> this causes memory corruption with mjpeg
> ./ffmpeg_g -i lena.pnm -qscale 4 -intra_matrix 10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10 -chroma_intra_matrix 400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400,400 -bitexact file-custommatrix10,400.jpg
> 
> i see
> 
> video:19kB audio:0kB subtitle:0kB other streams:0kB global headers:0kB muxing overhead: unknown
> corrupted size vs. prev_size
> Aborted (core dumped)
> 
> i can provide more details if it isnt reproduceable
> 

This is an example of the double-frees due to using
avcodec_free_context() to free the worker threads that I mentioned in my
reply.

- 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 v0 09/14] avcodec: add private side data set to AVCodecInternal

[Jan Ekström]

On Fri, Mar 24, 2023 at 12:51 PM Anton Khirnov <anton@khirnov.net> wrote:
>
> Quoting Jan Ekström (2023-03-21 00:34:03)
> > This allows configuring an encoder by using AVFrameSideData.
> > ---
> >  libavcodec/avcodec.c  | 1 +
> >  libavcodec/internal.h | 7 +++++++
> >  libavcodec/options.c  | 5 +++++
> >  3 files changed, 13 insertions(+)
> >
> > diff --git a/libavcodec/avcodec.c b/libavcodec/avcodec.c
> > index c110b19e08..3faabe77d1 100644
> > --- a/libavcodec/avcodec.c
> > +++ b/libavcodec/avcodec.c
> > @@ -403,6 +403,7 @@ void avcodec_flush_buffers(AVCodecContext *avctx)
> >      avci->nb_draining_errors = 0;
> >      av_frame_unref(avci->buffer_frame);
> >      av_packet_unref(avci->buffer_pkt);
> > +    av_side_data_set_wipe(&avci->side_data_set);
> >
> >      if (HAVE_THREADS && avctx->active_thread_type & FF_THREAD_FRAME)
> >          ff_thread_flush(avctx);
> > diff --git a/libavcodec/internal.h b/libavcodec/internal.h
> > index f21101752d..c658e97313 100644
> > --- a/libavcodec/internal.h
> > +++ b/libavcodec/internal.h
> > @@ -168,6 +168,13 @@ typedef struct AVCodecInternal {
> >       * a boolean to describe whether context is opened or not.
> >       */
> >      unsigned int ctx_opened;
> > +
> > +    /**
> > +     * Set holding static side data, such as HDR10 CLL / MDCV structures.
> > +     * - encoding: set by user
> > +     * - decoding: unused
> > +     */
> > +    AVFrameSideDataSet side_data_set;
>
> Why put it here and not in the public struct? It seems way more natural
> there.
>

The general idea was that if you want to make people utilize helpers
and not touch entries willy-nilly, you put it outside of the API
client's view. Or in other words, "only making something public when
it's absolutely required means that no unnecessary things are part of
the ABI structs"

Of course looking at the fun times with avci, I might just end up
doing what you note... I just wanted to give it a try to have a
private structure which was available before init() was called :) .

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 v0 10/14] avcodec: add function for setting avctx side data

[Jan Ekström]

On Fri, Mar 24, 2023 at 12:49 PM Anton Khirnov <anton@khirnov.net> wrote:
>
> Quoting Jan Ekström (2023-03-21 00:34:04)
> > diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> > index 30f1d312f4..8f535a0cc8 100644
> > --- a/libavcodec/avcodec.h
> > +++ b/libavcodec/avcodec.h
> > @@ -3223,6 +3223,18 @@ void av_fast_padded_mallocz(void *ptr, unsigned int *size, size_t min_size);
> >   */
> >  int avcodec_is_open(AVCodecContext *s);
> >
> > +/**
> > + * Configure a side data set to an encoder AVCodecContext. With multiple
> > + * calls new side data gets added in addition to the existing set of side data.
>
> I have no idea what does "configure" mean in this context and what
> effect will this actually have.
>

It becomes available to any encoder which utilizes it for
configuration of itself, basically.

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 v0 09/14] avcodec: add private side data set to AVCodecInternal

[Anton Khirnov]

Quoting Jan Ekström (2023-03-24 18:34:28)
> On Fri, Mar 24, 2023 at 12:51 PM Anton Khirnov <anton@khirnov.net> wrote:
> >
> > Quoting Jan Ekström (2023-03-21 00:34:03)
> > > This allows configuring an encoder by using AVFrameSideData.
> > > ---
> > >  libavcodec/avcodec.c  | 1 +
> > >  libavcodec/internal.h | 7 +++++++
> > >  libavcodec/options.c  | 5 +++++
> > >  3 files changed, 13 insertions(+)
> > >
> > > diff --git a/libavcodec/avcodec.c b/libavcodec/avcodec.c
> > > index c110b19e08..3faabe77d1 100644
> > > --- a/libavcodec/avcodec.c
> > > +++ b/libavcodec/avcodec.c
> > > @@ -403,6 +403,7 @@ void avcodec_flush_buffers(AVCodecContext *avctx)
> > >      avci->nb_draining_errors = 0;
> > >      av_frame_unref(avci->buffer_frame);
> > >      av_packet_unref(avci->buffer_pkt);
> > > +    av_side_data_set_wipe(&avci->side_data_set);
> > >
> > >      if (HAVE_THREADS && avctx->active_thread_type & FF_THREAD_FRAME)
> > >          ff_thread_flush(avctx);
> > > diff --git a/libavcodec/internal.h b/libavcodec/internal.h
> > > index f21101752d..c658e97313 100644
> > > --- a/libavcodec/internal.h
> > > +++ b/libavcodec/internal.h
> > > @@ -168,6 +168,13 @@ typedef struct AVCodecInternal {
> > >       * a boolean to describe whether context is opened or not.
> > >       */
> > >      unsigned int ctx_opened;
> > > +
> > > +    /**
> > > +     * Set holding static side data, such as HDR10 CLL / MDCV structures.
> > > +     * - encoding: set by user
> > > +     * - decoding: unused
> > > +     */
> > > +    AVFrameSideDataSet side_data_set;
> >
> > Why put it here and not in the public struct? It seems way more natural
> > there.
> >
> 
> The general idea was that if you want to make people utilize helpers
> and not touch entries willy-nilly,

But do we? Why?

In this case I see no advantage to having a public function over having
two public fields. The function is strictly worse because it cannot be
extended, and enlarges the symbol table.

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

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

Re: [FFmpeg-devel] [PATCH v0 09/14] avcodec: add private side data set to AVCodecInternal

[James Almer]

On 3/26/2023 4:00 PM, Anton Khirnov wrote:
> Quoting Jan Ekström (2023-03-24 18:34:28)
>> On Fri, Mar 24, 2023 at 12:51 PM Anton Khirnov <anton@khirnov.net> wrote:
>>>
>>> Quoting Jan Ekström (2023-03-21 00:34:03)
>>>> This allows configuring an encoder by using AVFrameSideData.
>>>> ---
>>>>   libavcodec/avcodec.c  | 1 +
>>>>   libavcodec/internal.h | 7 +++++++
>>>>   libavcodec/options.c  | 5 +++++
>>>>   3 files changed, 13 insertions(+)
>>>>
>>>> diff --git a/libavcodec/avcodec.c b/libavcodec/avcodec.c
>>>> index c110b19e08..3faabe77d1 100644
>>>> --- a/libavcodec/avcodec.c
>>>> +++ b/libavcodec/avcodec.c
>>>> @@ -403,6 +403,7 @@ void avcodec_flush_buffers(AVCodecContext *avctx)
>>>>       avci->nb_draining_errors = 0;
>>>>       av_frame_unref(avci->buffer_frame);
>>>>       av_packet_unref(avci->buffer_pkt);
>>>> +    av_side_data_set_wipe(&avci->side_data_set);
>>>>
>>>>       if (HAVE_THREADS && avctx->active_thread_type & FF_THREAD_FRAME)
>>>>           ff_thread_flush(avctx);
>>>> diff --git a/libavcodec/internal.h b/libavcodec/internal.h
>>>> index f21101752d..c658e97313 100644
>>>> --- a/libavcodec/internal.h
>>>> +++ b/libavcodec/internal.h
>>>> @@ -168,6 +168,13 @@ typedef struct AVCodecInternal {
>>>>        * a boolean to describe whether context is opened or not.
>>>>        */
>>>>       unsigned int ctx_opened;
>>>> +
>>>> +    /**
>>>> +     * Set holding static side data, such as HDR10 CLL / MDCV structures.
>>>> +     * - encoding: set by user
>>>> +     * - decoding: unused
>>>> +     */
>>>> +    AVFrameSideDataSet side_data_set;
>>>
>>> Why put it here and not in the public struct? It seems way more natural
>>> there.
>>>
>>
>> The general idea was that if you want to make people utilize helpers
>> and not touch entries willy-nilly,
> 
> But do we? Why?
> 
> In this case I see no advantage to having a public function over having
> two public fields. The function is strictly worse because it cannot be
> extended, and enlarges the symbol table.

Also, is this new AVFrameSideDataSet struct necessary? This looks sort 
of like the opposite of coded_side_data.
_______________________________________________
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 v0 09/14] avcodec: add private side data set to AVCodecInternal

[Jan Ekström]

On Sun, Mar 26, 2023 at 10:00 PM Anton Khirnov <anton@khirnov.net> wrote:
>
> Quoting Jan Ekström (2023-03-24 18:34:28)
> > On Fri, Mar 24, 2023 at 12:51 PM Anton Khirnov <anton@khirnov.net> wrote:
> > >
> > > Quoting Jan Ekström (2023-03-21 00:34:03)
> > > > This allows configuring an encoder by using AVFrameSideData.
> > > > ---
> > > >  libavcodec/avcodec.c  | 1 +
> > > >  libavcodec/internal.h | 7 +++++++
> > > >  libavcodec/options.c  | 5 +++++
> > > >  3 files changed, 13 insertions(+)
> > > >
> > > > diff --git a/libavcodec/avcodec.c b/libavcodec/avcodec.c
> > > > index c110b19e08..3faabe77d1 100644
> > > > --- a/libavcodec/avcodec.c
> > > > +++ b/libavcodec/avcodec.c
> > > > @@ -403,6 +403,7 @@ void avcodec_flush_buffers(AVCodecContext *avctx)
> > > >      avci->nb_draining_errors = 0;
> > > >      av_frame_unref(avci->buffer_frame);
> > > >      av_packet_unref(avci->buffer_pkt);
> > > > +    av_side_data_set_wipe(&avci->side_data_set);
> > > >
> > > >      if (HAVE_THREADS && avctx->active_thread_type & FF_THREAD_FRAME)
> > > >          ff_thread_flush(avctx);
> > > > diff --git a/libavcodec/internal.h b/libavcodec/internal.h
> > > > index f21101752d..c658e97313 100644
> > > > --- a/libavcodec/internal.h
> > > > +++ b/libavcodec/internal.h
> > > > @@ -168,6 +168,13 @@ typedef struct AVCodecInternal {
> > > >       * a boolean to describe whether context is opened or not.
> > > >       */
> > > >      unsigned int ctx_opened;
> > > > +
> > > > +    /**
> > > > +     * Set holding static side data, such as HDR10 CLL / MDCV structures.
> > > > +     * - encoding: set by user
> > > > +     * - decoding: unused
> > > > +     */
> > > > +    AVFrameSideDataSet side_data_set;
> > >
> > > Why put it here and not in the public struct? It seems way more natural
> > > there.
> > >
> >
> > The general idea was that if you want to make people utilize helpers
> > and not touch entries willy-nilly,
>
> But do we? Why?
>
> In this case I see no advantage to having a public function over having
> two public fields. The function is strictly worse because it cannot be
> extended, and enlarges the symbol table.

The overarching idea has been that looking at the recent changes of
adding private internal structs and deprecating previously public
stuff from the public structs was to be careful.

1. Post a patch with things added to the relevant private struct.
2. Allow specific access with a helper
3. See if anyone requires the information to be part of the public
struct. At which point it may be moved there.

Additionally, only having things available via helpers means that you
know how exactly the fields were being poked (instead of "we don't
know how API users utilize them, and thus we cannot do X" (see the
avcodec context related helpers where we still have to support close &
av_freep which popped up in another patch), and given that the
argument is a struct, the function can technically be extended
(although a struct being in the middle of a struct might stop from it
being extended).

I hope this opens up my thoughts and that I'm not a religious zealot
about this selection. Just that I wanted to start where it's not
public, so that hopefully someone would explain why it should be
public if they would wish it to be such.

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 v0 09/14] avcodec: add private side data set to AVCodecInternal

[Jan Ekström]

On Sun, Mar 26, 2023 at 10:03 PM James Almer <jamrial@gmail.com> wrote:
>
>
>
> On 3/26/2023 4:00 PM, Anton Khirnov wrote:
> > Quoting Jan Ekström (2023-03-24 18:34:28)
> >> On Fri, Mar 24, 2023 at 12:51 PM Anton Khirnov <anton@khirnov.net> wrote:
> >>>
> >>> Quoting Jan Ekström (2023-03-21 00:34:03)
> >>>> This allows configuring an encoder by using AVFrameSideData.
> >>>> ---
> >>>>   libavcodec/avcodec.c  | 1 +
> >>>>   libavcodec/internal.h | 7 +++++++
> >>>>   libavcodec/options.c  | 5 +++++
> >>>>   3 files changed, 13 insertions(+)
> >>>>
> >>>> diff --git a/libavcodec/avcodec.c b/libavcodec/avcodec.c
> >>>> index c110b19e08..3faabe77d1 100644
> >>>> --- a/libavcodec/avcodec.c
> >>>> +++ b/libavcodec/avcodec.c
> >>>> @@ -403,6 +403,7 @@ void avcodec_flush_buffers(AVCodecContext *avctx)
> >>>>       avci->nb_draining_errors = 0;
> >>>>       av_frame_unref(avci->buffer_frame);
> >>>>       av_packet_unref(avci->buffer_pkt);
> >>>> +    av_side_data_set_wipe(&avci->side_data_set);
> >>>>
> >>>>       if (HAVE_THREADS && avctx->active_thread_type & FF_THREAD_FRAME)
> >>>>           ff_thread_flush(avctx);
> >>>> diff --git a/libavcodec/internal.h b/libavcodec/internal.h
> >>>> index f21101752d..c658e97313 100644
> >>>> --- a/libavcodec/internal.h
> >>>> +++ b/libavcodec/internal.h
> >>>> @@ -168,6 +168,13 @@ typedef struct AVCodecInternal {
> >>>>        * a boolean to describe whether context is opened or not.
> >>>>        */
> >>>>       unsigned int ctx_opened;
> >>>> +
> >>>> +    /**
> >>>> +     * Set holding static side data, such as HDR10 CLL / MDCV structures.
> >>>> +     * - encoding: set by user
> >>>> +     * - decoding: unused
> >>>> +     */
> >>>> +    AVFrameSideDataSet side_data_set;
> >>>
> >>> Why put it here and not in the public struct? It seems way more natural
> >>> there.
> >>>
> >>
> >> The general idea was that if you want to make people utilize helpers
> >> and not touch entries willy-nilly,
> >
> > But do we? Why?
> >
> > In this case I see no advantage to having a public function over having
> > two public fields. The function is strictly worse because it cannot be
> > extended, and enlarges the symbol table.
>
> Also, is this new AVFrameSideDataSet struct necessary? This looks sort
> of like the opposite of coded_side_data.

It indeed is the other side of the coded_side_data, which is something
that gets added after avctx init.

And the main reason was because the addition function being a pointer
to a struct, instead of (AVFrameSideData ***sd, int *nb_side_data).

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 v0 09/14] avcodec: add private side data set to AVCodecInternal

[Anton Khirnov]

Quoting Jan Ekström (2023-03-27 08:37:07)
> On Sun, Mar 26, 2023 at 10:00 PM Anton Khirnov <anton@khirnov.net> wrote:
> > Quoting Jan Ekström (2023-03-24 18:34:28)
> > > The general idea was that if you want to make people utilize helpers
> > > and not touch entries willy-nilly,
> >
> > But do we? Why?
> >
> > In this case I see no advantage to having a public function over having
> > two public fields. The function is strictly worse because it cannot be
> > extended, and enlarges the symbol table.
> 
> The overarching idea has been that looking at the recent changes of
> adding private internal structs and deprecating previously public
> stuff from the public structs was to be careful.

The fields being removed from public structs are actually private,
that's the only reason they are being hidden.

The thing you are adding is not private, it is public API. Having a
setter for it rather than making it a field in a public struct is merely
a different way of making it public, which
* is inconsistent with other similar APIs
* has the disadvantages I mentioned in the previous email

> 1. Post a patch with things added to the relevant private struct.
> 2. Allow specific access with a helper
> 3. See if anyone requires the information to be part of the public
> struct. At which point it may be moved there.
> 
> Additionally, only having things available via helpers means that you
> know how exactly the fields were being poked (instead of "we don't
> know how API users utilize them, and thus we cannot do X" (see the
> avcodec context related helpers where we still have to support close &
> av_freep which popped up in another patch), and given that the
> argument is a struct, the function can technically be extended
> (although a struct being in the middle of a struct might stop from it
> being extended).

There is no constructor for the struct, so it cannot be extended without
a major bump.

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

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