Git Inbox Mirror of the ffmpeg-devel mailing list - see https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
 help / color / mirror / Atom feed
* [FFmpeg-devel] [PATCH 1/7] avutil/avutil: move some definitions to a new header
@ 2025-01-22  2:53 James Almer
  2025-01-22  2:53 ` [FFmpeg-devel] [PATCH 2/7] avcodec/packet: move AVSideDataParamChangeFlags to libavutil/defs.h James Almer
                   ` (6 more replies)
  0 siblings, 7 replies; 21+ messages in thread
From: James Almer @ 2025-01-22  2:53 UTC (permalink / raw)
  To: ffmpeg-devel

They don't belong anywhere in particular, so move them to a new generic header
called defs.h, following the same idea as the one from lavc.

Signed-off-by: James Almer <jamrial@gmail.com>
---
 libavutil/avutil.h |  94 +++-------------------------
 libavutil/defs.h   | 149 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 159 insertions(+), 84 deletions(-)
 create mode 100644 libavutil/defs.h

diff --git a/libavutil/avutil.h b/libavutil/avutil.h
index ee709fbb2a..1cf562c73f 100644
--- a/libavutil/avutil.h
+++ b/libavutil/avutil.h
@@ -191,100 +191,36 @@ const char *avutil_license(void);
  * @}
  */
 
+#include "common.h"
+#include "defs.h"
+#include "rational.h"
+#include "version.h"
+#include "macros.h"
+#include "mathematics.h"
+#include "log.h"
+#include "pixfmt.h"
+
 /**
  * @addtogroup lavu_media Media Type
  * @brief Media Type
+ * @{
  */
 
-enum AVMediaType {
-    AVMEDIA_TYPE_UNKNOWN = -1,  ///< Usually treated as AVMEDIA_TYPE_DATA
-    AVMEDIA_TYPE_VIDEO,
-    AVMEDIA_TYPE_AUDIO,
-    AVMEDIA_TYPE_DATA,          ///< Opaque data information usually continuous
-    AVMEDIA_TYPE_SUBTITLE,
-    AVMEDIA_TYPE_ATTACHMENT,    ///< Opaque data information usually sparse
-    AVMEDIA_TYPE_NB
-};
-
 /**
  * Return a string describing the media_type enum, NULL if media_type
  * is unknown.
  */
 const char *av_get_media_type_string(enum AVMediaType media_type);
 
-/**
- * @defgroup lavu_const Constants
- * @{
- *
- * @defgroup lavu_enc Encoding specific
- *
- * @note those definition should move to avcodec
- * @{
- */
-
-#define FF_LAMBDA_SHIFT 7
-#define FF_LAMBDA_SCALE (1<<FF_LAMBDA_SHIFT)
-#define FF_QP2LAMBDA 118 ///< factor to convert from H.263 QP to lambda
-#define FF_LAMBDA_MAX (256*128-1)
-
-#define FF_QUALITY_SCALE FF_LAMBDA_SCALE //FIXME maybe remove
-
 /**
  * @}
- * @defgroup lavu_time Timestamp specific
- *
- * FFmpeg internal timebase and timestamp definitions
- *
- * @{
- */
-
-/**
- * @brief Undefined timestamp value
- *
- * Usually reported by demuxer that work on containers that do not provide
- * either pts or dts.
  */
 
-#define AV_NOPTS_VALUE          ((int64_t)UINT64_C(0x8000000000000000))
-
 /**
- * Internal time base represented as integer
- */
-
-#define AV_TIME_BASE            1000000
-
-/**
- * Internal time base represented as fractional value
- */
-
-#ifdef __cplusplus
-/* ISO C++ forbids compound-literals. */
-#define AV_TIME_BASE_Q          av_make_q(1, AV_TIME_BASE)
-#else
-#define AV_TIME_BASE_Q          (AVRational){1, AV_TIME_BASE}
-#endif
-
-/**
- * @}
- * @}
  * @defgroup lavu_picture Image related
- *
- * AVPicture types, pixel formats and basic image planes manipulation.
- *
  * @{
  */
 
-enum AVPictureType {
-    AV_PICTURE_TYPE_NONE = 0, ///< Undefined
-    AV_PICTURE_TYPE_I,     ///< Intra
-    AV_PICTURE_TYPE_P,     ///< Predicted
-    AV_PICTURE_TYPE_B,     ///< Bi-dir predicted
-    AV_PICTURE_TYPE_S,     ///< S(GMC)-VOP MPEG-4
-    AV_PICTURE_TYPE_SI,    ///< Switching Intra
-    AV_PICTURE_TYPE_SP,    ///< Switching Predicted
-    AV_PICTURE_TYPE_BI,    ///< BI type
-};
-
 /**
  * Return a single letter to describe the given picture type
  * pict_type.
@@ -298,14 +234,6 @@ char av_get_picture_type_char(enum AVPictureType pict_type);
  * @}
  */
 
-#include "common.h"
-#include "rational.h"
-#include "version.h"
-#include "macros.h"
-#include "mathematics.h"
-#include "log.h"
-#include "pixfmt.h"
-
 /**
  * Return x default pointer in case p is NULL.
  */
@@ -343,8 +271,6 @@ unsigned av_int_list_length_for_size(unsigned elsize,
  */
 AVRational av_get_time_base_q(void);
 
-#define AV_FOURCC_MAX_STRING_SIZE 32
-
 #define av_fourcc2str(fourcc) av_fourcc_make_string((char[AV_FOURCC_MAX_STRING_SIZE]){0}, fourcc)
 
 /**
diff --git a/libavutil/defs.h b/libavutil/defs.h
new file mode 100644
index 0000000000..9e9bb8bcc5
--- /dev/null
+++ b/libavutil/defs.h
@@ -0,0 +1,149 @@
+/*
+ * 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
+ */
+
+#ifndef AVUTIL_DEFS_H
+#define AVUTIL_DEFS_H
+
+/**
+ * @file
+ * @ingroup lavu
+ * Misc types and constants that do not belong anywhere else.
+ */
+
+#include <stdint.h>
+#include <stdlib.h>
+
+/**
+ * @addtogroup lavu_misc Other
+ * @{
+ *
+ * @defgroup preproc_misc Preprocessor String Macros
+ *
+ * @{
+ *
+ * @}
+ *
+ * @defgroup version_utils Library Version Macros
+ *
+ * @{
+ *
+ * @}
+ */
+
+/**
+ * @addtogroup lavu_media Media Type
+ * @{
+ */
+enum AVMediaType {
+    AVMEDIA_TYPE_UNKNOWN = -1,  ///< Usually treated as AVMEDIA_TYPE_DATA
+    AVMEDIA_TYPE_VIDEO,
+    AVMEDIA_TYPE_AUDIO,
+    AVMEDIA_TYPE_DATA,          ///< Opaque data information usually continuous
+    AVMEDIA_TYPE_SUBTITLE,
+    AVMEDIA_TYPE_ATTACHMENT,    ///< Opaque data information usually sparse
+    AVMEDIA_TYPE_NB
+};
+
+/**
+ * @}
+ */
+
+/**
+ * @defgroup lavu_const Constants
+ * @{
+ *
+ * @defgroup lavu_enc Encoding specific
+ *
+ * @note those definition should move to avcodec
+ * @{
+ */
+
+#define FF_LAMBDA_SHIFT 7
+#define FF_LAMBDA_SCALE (1<<FF_LAMBDA_SHIFT)
+#define FF_QP2LAMBDA 118 ///< factor to convert from H.263 QP to lambda
+#define FF_LAMBDA_MAX (256*128-1)
+
+#define FF_QUALITY_SCALE FF_LAMBDA_SCALE //FIXME maybe remove
+
+#define AV_FOURCC_MAX_STRING_SIZE 32
+
+/**
+ * @}
+ * @defgroup lavu_time Timestamp specific
+ *
+ * FFmpeg internal timebase and timestamp definitions
+ *
+ * @{
+ */
+
+/**
+ * @brief Undefined timestamp value
+ *
+ * Usually reported by demuxer that work on containers that do not provide
+ * either pts or dts.
+ */
+
+#define AV_NOPTS_VALUE          ((int64_t)UINT64_C(0x8000000000000000))
+
+/**
+ * Internal time base represented as integer
+ */
+
+#define AV_TIME_BASE            1000000
+
+/**
+ * Internal time base represented as fractional value
+ */
+
+#ifdef __cplusplus
+/* ISO C++ forbids compound-literals. */
+#define AV_TIME_BASE_Q          av_make_q(1, AV_TIME_BASE)
+#else
+#define AV_TIME_BASE_Q          (AVRational){1, AV_TIME_BASE}
+#endif
+
+/**
+ * @}
+ * @}
+ */
+
+/**
+ * @addtogroup lavu_picture Image related
+ *
+ * AVPicture types, pixel formats and basic image planes manipulation.
+ *
+ * @{
+ */
+
+enum AVPictureType {
+    AV_PICTURE_TYPE_NONE = 0, ///< Undefined
+    AV_PICTURE_TYPE_I,     ///< Intra
+    AV_PICTURE_TYPE_P,     ///< Predicted
+    AV_PICTURE_TYPE_B,     ///< Bi-dir predicted
+    AV_PICTURE_TYPE_S,     ///< S(GMC)-VOP MPEG-4
+    AV_PICTURE_TYPE_SI,    ///< Switching Intra
+    AV_PICTURE_TYPE_SP,    ///< Switching Predicted
+    AV_PICTURE_TYPE_BI,    ///< BI type
+};
+
+/**
+ * @}
+ * @}
+ */
+
+#endif // AVUTIL_DEFS_H
-- 
2.48.1

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [FFmpeg-devel] [PATCH 2/7] avcodec/packet: move AVSideDataParamChangeFlags to libavutil/defs.h
  2025-01-22  2:53 [FFmpeg-devel] [PATCH 1/7] avutil/avutil: move some definitions to a new header James Almer
@ 2025-01-22  2:53 ` James Almer
  2025-01-22  2:53 ` [FFmpeg-devel] [PATCH 3/7] avutil/frame: add a param change side data type James Almer
                   ` (5 subsequent siblings)
  6 siblings, 0 replies; 21+ messages in thread
From: James Almer @ 2025-01-22  2:53 UTC (permalink / raw)
  To: ffmpeg-devel

Its namespace is not packet specific, and will be useful in a follow commit.

Signed-off-by: James Almer <jamrial@gmail.com>
---
 libavcodec/packet.h | 5 -----
 libavutil/defs.h    | 5 +++++
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/libavcodec/packet.h b/libavcodec/packet.h
index c1f1ad7b43..7ccfcd1249 100644
--- a/libavcodec/packet.h
+++ b/libavcodec/packet.h
@@ -612,11 +612,6 @@ typedef struct AVPacketList {
  */
 #define AV_PKT_FLAG_DISPOSABLE 0x0010
 
-enum AVSideDataParamChangeFlags {
-    AV_SIDE_DATA_PARAM_CHANGE_SAMPLE_RATE    = 0x0004,
-    AV_SIDE_DATA_PARAM_CHANGE_DIMENSIONS     = 0x0008,
-};
-
 /**
  * Allocate an AVPacket and set its fields to default values.  The resulting
  * struct must be freed using av_packet_free().
diff --git a/libavutil/defs.h b/libavutil/defs.h
index 9e9bb8bcc5..f09fb5efd2 100644
--- a/libavutil/defs.h
+++ b/libavutil/defs.h
@@ -122,6 +122,11 @@ enum AVMediaType {
  * @}
  */
 
+enum AVSideDataParamChangeFlags {
+    AV_SIDE_DATA_PARAM_CHANGE_SAMPLE_RATE    = 0x0004,
+    AV_SIDE_DATA_PARAM_CHANGE_DIMENSIONS     = 0x0008,
+};
+
 /**
  * @addtogroup lavu_picture Image related
  *
-- 
2.48.1

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [FFmpeg-devel] [PATCH 3/7] avutil/frame: add a param change side data type
  2025-01-22  2:53 [FFmpeg-devel] [PATCH 1/7] avutil/avutil: move some definitions to a new header James Almer
  2025-01-22  2:53 ` [FFmpeg-devel] [PATCH 2/7] avcodec/packet: move AVSideDataParamChangeFlags to libavutil/defs.h James Almer
@ 2025-01-22  2:53 ` James Almer
  2025-01-22  3:13   ` Lynne
  2025-01-22  2:54 ` [FFmpeg-devel] [PATCH 4/7] avcodec/encode: add support for param change frame side data James Almer
                   ` (4 subsequent siblings)
  6 siblings, 1 reply; 21+ messages in thread
From: James Almer @ 2025-01-22  2:53 UTC (permalink / raw)
  To: ffmpeg-devel

Equivalent to the one from packet side data using AVSideDataParamChangeFlags,
and will be used for the same purpose with encoders.

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

diff --git a/libavutil/frame.c b/libavutil/frame.c
index 10b59545f0..f3575afc31 100644
--- a/libavutil/frame.c
+++ b/libavutil/frame.c
@@ -48,6 +48,7 @@ static const AVSideDataDescriptor sd_props[] = {
     [AV_FRAME_DATA_DOVI_METADATA]               = { "Dolby Vision Metadata",                        AV_SIDE_DATA_PROP_COLOR_DEPENDENT },
     [AV_FRAME_DATA_LCEVC]                       = { "LCEVC NAL data",                               AV_SIDE_DATA_PROP_SIZE_DEPENDENT },
     [AV_FRAME_DATA_VIEW_ID]                     = { "View ID" },
+    [AV_FRAME_DATA_PARAM_CHANGE]                = { "Param Change" },
     [AV_FRAME_DATA_STEREO3D]                    = { "Stereo 3D",                                    AV_SIDE_DATA_PROP_GLOBAL },
     [AV_FRAME_DATA_REPLAYGAIN]                  = { "AVReplayGain",                                 AV_SIDE_DATA_PROP_GLOBAL },
     [AV_FRAME_DATA_DISPLAYMATRIX]               = { "3x3 displaymatrix",                            AV_SIDE_DATA_PROP_GLOBAL },
diff --git a/libavutil/frame.h b/libavutil/frame.h
index 628f2b5b3a..de7a8e9eab 100644
--- a/libavutil/frame.h
+++ b/libavutil/frame.h
@@ -243,6 +243,19 @@ enum AVFrameSideDataType {
      * The data is an int storing the view ID.
      */
     AV_FRAME_DATA_VIEW_ID,
+
+    /**
+     * An AV_FRAME_DATA_PARAM_CHANGE side data packet is laid out as follows:
+     * @code
+     * u32le param_flags
+     * if (param_flags & AV_SIDE_DATA_PARAM_CHANGE_SAMPLE_RATE)
+     *     s32le sample_rate
+     * if (param_flags & AV_SIDE_DATA_PARAM_CHANGE_DIMENSIONS)
+     *     s32le width
+     *     s32le height
+     * @endcode
+     */
+    AV_FRAME_DATA_PARAM_CHANGE,
 };
 
 enum AVActiveFormatDescription {
-- 
2.48.1

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [FFmpeg-devel] [PATCH 4/7] avcodec/encode: add support for param change frame side data
  2025-01-22  2:53 [FFmpeg-devel] [PATCH 1/7] avutil/avutil: move some definitions to a new header James Almer
  2025-01-22  2:53 ` [FFmpeg-devel] [PATCH 2/7] avcodec/packet: move AVSideDataParamChangeFlags to libavutil/defs.h James Almer
  2025-01-22  2:53 ` [FFmpeg-devel] [PATCH 3/7] avutil/frame: add a param change side data type James Almer
@ 2025-01-22  2:54 ` James Almer
  2025-01-27 12:48   ` Andreas Rheinhardt
  2025-01-22  2:54 ` [FFmpeg-devel] [PATCH 5/7] avcodec/encode: add a new param change side data value that takes a dictionary James Almer
                   ` (3 subsequent siblings)
  6 siblings, 1 reply; 21+ messages in thread
From: James Almer @ 2025-01-22  2:54 UTC (permalink / raw)
  To: ffmpeg-devel

With this, callers can signal supported encoders to reinitialize using certain
parameters, which should allow for more graceful (but potentially more limited
in scope) reinitialization than closing and reopening the encoder, or even
simply on-the-fly changes of trivial values that would not require any kind
or flushing or reinitialization by the encoder.

Signed-off-by: James Almer <jamrial@gmail.com>
---
 libavcodec/codec_internal.h |  2 ++
 libavcodec/encode.c         | 69 +++++++++++++++++++++++++++++++++++++
 2 files changed, 71 insertions(+)

diff --git a/libavcodec/codec_internal.h b/libavcodec/codec_internal.h
index 5b2db74590..e38402d2f5 100644
--- a/libavcodec/codec_internal.h
+++ b/libavcodec/codec_internal.h
@@ -243,6 +243,8 @@ typedef struct FFCodec {
      */
     void (*flush)(struct AVCodecContext *);
 
+    int (*reconf)(struct AVCodecContext *);
+
     /**
      * Decoding only, a comma-separated list of bitstream filters to apply to
      * packets before decoding.
diff --git a/libavcodec/encode.c b/libavcodec/encode.c
index 3baf5b8103..187b4015f1 100644
--- a/libavcodec/encode.c
+++ b/libavcodec/encode.c
@@ -31,6 +31,7 @@
 
 #include "avcodec.h"
 #include "avcodec_internal.h"
+#include "bytestream.h"
 #include "codec_desc.h"
 #include "codec_internal.h"
 #include "encode.h"
@@ -59,6 +60,69 @@ static EncodeContext *encode_ctx(AVCodecInternal *avci)
     return (EncodeContext*)avci;
 }
 
+static int apply_param_change(AVCodecContext *avctx, const AVFrame *frame)
+{
+    int ret = AVERROR_BUG;
+    const AVFrameSideData *sd;
+    GetByteContext gbc;
+    uint32_t flags;
+
+    sd = av_frame_get_side_data(frame, AV_FRAME_DATA_PARAM_CHANGE);
+    if (!sd)
+        return 0;
+
+    if (!(avctx->codec->capabilities & AV_CODEC_CAP_PARAM_CHANGE)) {
+        av_log(avctx, AV_LOG_ERROR, "This encoder does not support parameter "
+               "changes, but PARAM_CHANGE side data was sent to it.\n");
+        ret = AVERROR(EINVAL);
+        goto fail2;
+    }
+
+    if (sd->size < 4)
+        goto fail;
+
+    bytestream2_init(&gbc, sd->data, sd->size);
+    flags = bytestream2_get_le32(&gbc);
+
+    if (flags & AV_SIDE_DATA_PARAM_CHANGE_SAMPLE_RATE) {
+        if (frame->sample_rate <= 0 || frame->sample_rate > INT_MAX) {
+            av_log(avctx, AV_LOG_ERROR, "Invalid sample rate");
+            ret = AVERROR_INVALIDDATA;
+            goto fail2;
+        }
+        avctx->sample_rate = frame->sample_rate;
+    }
+    if (flags & AV_SIDE_DATA_PARAM_CHANGE_DIMENSIONS) {
+        av_image_check_size2(frame->width, frame->height, avctx->max_pixels, avctx->pix_fmt, 0, avctx);
+        if (ret < 0) {
+            av_log(avctx, AV_LOG_ERROR, "Invalid dimensions");
+            goto fail2;
+        }
+        avctx->width  = frame->width;
+        avctx->height = frame->height;
+    }
+
+    if (flags) {
+        ret = 0;
+        if (ffcodec(avctx->codec)->reconf)
+            ret = ffcodec(avctx->codec)->reconf(avctx);
+        if (ret < 0)
+            goto fail2;
+    }
+
+    return 0;
+fail:
+    av_log(avctx, AV_LOG_ERROR, "PARAM_CHANGE side data too small.\n");
+    ret = AVERROR_INVALIDDATA;
+fail2:
+    if (ret < 0) {
+        av_log(avctx, AV_LOG_ERROR, "Error applying parameter changes.\n");
+        if (avctx->err_recognition & AV_EF_EXPLODE)
+            return ret;
+    }
+    return 0;
+}
+
 int ff_alloc_packet(AVCodecContext *avctx, AVPacket *avpkt, int64_t size)
 {
     if (size < 0 || size > INT_MAX - AV_INPUT_BUFFER_PADDING_SIZE) {
@@ -205,6 +269,7 @@ int avcodec_encode_subtitle(AVCodecContext *avctx, uint8_t *buf, int buf_size,
 int ff_encode_get_frame(AVCodecContext *avctx, AVFrame *frame)
 {
     AVCodecInternal *avci = avctx->internal;
+    int ret;
 
     if (avci->draining)
         return AVERROR_EOF;
@@ -229,6 +294,10 @@ FF_DISABLE_DEPRECATION_WARNINGS
 FF_ENABLE_DEPRECATION_WARNINGS
 #endif
 
+    ret = apply_param_change(avctx, frame);
+    if (ret < 0)
+        return ret;
+
     return 0;
 }
 
-- 
2.48.1

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [FFmpeg-devel] [PATCH 5/7] avcodec/encode: add a new param change side data value that takes a dictionary
  2025-01-22  2:53 [FFmpeg-devel] [PATCH 1/7] avutil/avutil: move some definitions to a new header James Almer
                   ` (2 preceding siblings ...)
  2025-01-22  2:54 ` [FFmpeg-devel] [PATCH 4/7] avcodec/encode: add support for param change frame side data James Almer
@ 2025-01-22  2:54 ` James Almer
  2025-01-27 13:26   ` Andreas Rheinhardt
  2025-01-22  2:54 ` [FFmpeg-devel] [PATCH 6/7] avcodec/libaomenc: add support for param change frame side data James Almer
                   ` (2 subsequent siblings)
  6 siblings, 1 reply; 21+ messages in thread
From: James Almer @ 2025-01-22  2:54 UTC (permalink / raw)
  To: ffmpeg-devel

Allow the caller to pass a dictionary with key/value pairs to set encoder
private options for reinitialization.
AVCodecContext global options are not affected. For that, specific
AVSideDataParamChangeFlags should be used, as is currently the case for
dimensions and sample rate.

Signed-off-by: James Almer <jamrial@gmail.com>
---
 libavcodec/encode.c | 21 +++++++++++++++++++++
 libavutil/defs.h    |  1 +
 2 files changed, 22 insertions(+)

diff --git a/libavcodec/encode.c b/libavcodec/encode.c
index 187b4015f1..c14ab65509 100644
--- a/libavcodec/encode.c
+++ b/libavcodec/encode.c
@@ -20,12 +20,14 @@
 
 #include "libavutil/attributes.h"
 #include "libavutil/avassert.h"
+#include "libavutil/avstring.h"
 #include "libavutil/channel_layout.h"
 #include "libavutil/emms.h"
 #include "libavutil/frame.h"
 #include "libavutil/imgutils.h"
 #include "libavutil/internal.h"
 #include "libavutil/mem.h"
+#include "libavutil/opt.h"
 #include "libavutil/pixdesc.h"
 #include "libavutil/samplefmt.h"
 
@@ -101,6 +103,25 @@ static int apply_param_change(AVCodecContext *avctx, const AVFrame *frame)
         avctx->width  = frame->width;
         avctx->height = frame->height;
     }
+    if (flags & AV_SIDE_DATA_PARAM_CHANGE_DICT) {
+        AVDictionary *dict = NULL;
+        int left = av_strnlen(gbc.buffer, bytestream2_get_bytes_left(&gbc));
+        if (!left)
+            goto fail;
+        ret = av_dict_parse_string(&dict, gbc.buffer, "=", ":", 0);
+        if (ret < 0) {
+            av_log(avctx, AV_LOG_ERROR, "Invalid string");
+            goto fail2;
+        }
+        bytestream2_skip(&gbc, left + 1);
+
+        if (avctx->codec->priv_class) {
+            ret = av_opt_set_dict(avctx->priv_data, &dict);
+            if (ret < 0)
+                goto fail2;
+        }
+        av_dict_free(&dict);
+    }
 
     if (flags) {
         ret = 0;
diff --git a/libavutil/defs.h b/libavutil/defs.h
index f09fb5efd2..e1de680f2f 100644
--- a/libavutil/defs.h
+++ b/libavutil/defs.h
@@ -125,6 +125,7 @@ enum AVMediaType {
 enum AVSideDataParamChangeFlags {
     AV_SIDE_DATA_PARAM_CHANGE_SAMPLE_RATE    = 0x0004,
     AV_SIDE_DATA_PARAM_CHANGE_DIMENSIONS     = 0x0008,
+    AV_SIDE_DATA_PARAM_CHANGE_DICT           = 0x0016,
 };
 
 /**
-- 
2.48.1

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [FFmpeg-devel] [PATCH 6/7] avcodec/libaomenc: add support for param change frame side data
  2025-01-22  2:53 [FFmpeg-devel] [PATCH 1/7] avutil/avutil: move some definitions to a new header James Almer
                   ` (3 preceding siblings ...)
  2025-01-22  2:54 ` [FFmpeg-devel] [PATCH 5/7] avcodec/encode: add a new param change side data value that takes a dictionary James Almer
@ 2025-01-22  2:54 ` James Almer
  2025-01-22  2:54 ` [FFmpeg-devel] [PATCH 7/7] avcodec/libx264: " James Almer
  2025-01-27 12:43 ` [FFmpeg-devel] [PATCH 1/7] avutil/avutil: move some definitions to a new header Andreas Rheinhardt
  6 siblings, 0 replies; 21+ messages in thread
From: James Almer @ 2025-01-22  2:54 UTC (permalink / raw)
  To: ffmpeg-devel

Signed-off-by: James Almer <jamrial@gmail.com>
---
 libavcodec/libaomenc.c | 266 ++++++++++++++++++++++++-----------------
 1 file changed, 159 insertions(+), 107 deletions(-)

diff --git a/libavcodec/libaomenc.c b/libavcodec/libaomenc.c
index 0f7571ee7a..45d3fe862a 100644
--- a/libavcodec/libaomenc.c
+++ b/libavcodec/libaomenc.c
@@ -73,7 +73,10 @@ typedef struct AOMEncoderContext {
     AVBSFContext *bsf;
     DOVIContext dovi;
     struct aom_codec_ctx encoder;
+    struct aom_codec_enc_cfg enccfg;
     struct aom_image rawimg;
+    aom_codec_flags_t flags;
+    aom_img_fmt_t img_fmt;
     struct aom_fixed_buf twopass_stats;
     unsigned twopass_stats_size;
     struct FrameListData *coded_frame_list;
@@ -342,9 +345,6 @@ static av_cold int codecctl_intp(AVCodecContext *avctx,
     int width = -30;
     int res;
 
-    snprintf(buf, sizeof(buf), "%s:", ctlidstr[id]);
-    av_log(avctx, AV_LOG_DEBUG, "  %*s%d\n", width, buf, *ptr);
-
     res = aom_codec_control(&ctx->encoder, id, ptr);
     if (res != AOM_CODEC_OK) {
         snprintf(buf, sizeof(buf), "Failed to set %s codec control",
@@ -353,6 +353,9 @@ static av_cold int codecctl_intp(AVCodecContext *avctx,
         return AVERROR(EINVAL);
     }
 
+    snprintf(buf, sizeof(buf), "%s:", ctlidstr[id]);
+    av_log(avctx, AV_LOG_DEBUG, "  %*s%d\n", width, buf, *ptr);
+
     return 0;
 }
 #endif
@@ -673,29 +676,22 @@ static int choose_tiling(AVCodecContext *avctx,
     return 0;
 }
 
-static av_cold int aom_init(AVCodecContext *avctx,
-                            const struct aom_codec_iface *iface)
+static av_cold int aom_config(AVCodecContext *avctx,
+                              const struct aom_codec_iface *iface)
 {
     AOMContext *ctx = avctx->priv_data;
-    const AVPixFmtDescriptor *desc = av_pix_fmt_desc_get(avctx->pix_fmt);
-    struct aom_codec_enc_cfg enccfg = { 0 };
-    aom_codec_flags_t flags =
-        (avctx->flags & AV_CODEC_FLAG_PSNR) ? AOM_CODEC_USE_PSNR : 0;
-    AVCPBProperties *cpb_props;
     int res;
-    aom_img_fmt_t img_fmt;
     aom_codec_caps_t codec_caps = aom_codec_get_caps(iface);
 
-    av_log(avctx, AV_LOG_INFO, "%s\n", aom_codec_version_str());
-    av_log(avctx, AV_LOG_VERBOSE, "%s\n", aom_codec_build_config());
+    ctx->flags = (avctx->flags & AV_CODEC_FLAG_PSNR) ? AOM_CODEC_USE_PSNR : 0;
 
-    if ((res = aom_codec_enc_config_default(iface, &enccfg, ctx->usage)) != AOM_CODEC_OK) {
+    if ((res = aom_codec_enc_config_default(iface, &ctx->enccfg, ctx->usage)) != AOM_CODEC_OK) {
         av_log(avctx, AV_LOG_ERROR, "Failed to get config: %s\n",
                aom_codec_err_to_string(res));
         return AVERROR(EINVAL);
     }
 
-    if (set_pix_fmt(avctx, codec_caps, &enccfg, &flags, &img_fmt))
+    if (set_pix_fmt(avctx, codec_caps, &ctx->enccfg, &ctx->flags, &ctx->img_fmt))
         return AVERROR(EINVAL);
 
     if(!avctx->bit_rate)
@@ -704,39 +700,30 @@ static av_cold int aom_init(AVCodecContext *avctx,
             return AVERROR(EINVAL);
         }
 
-    dump_enc_cfg(avctx, &enccfg, AV_LOG_DEBUG);
-
-    enccfg.g_w            = avctx->width;
-    enccfg.g_h            = avctx->height;
-    enccfg.g_timebase.num = avctx->time_base.num;
-    enccfg.g_timebase.den = avctx->time_base.den;
-    enccfg.g_threads      =
+    ctx->enccfg.g_w            = avctx->width;
+    ctx->enccfg.g_h            = avctx->height;
+    ctx->enccfg.g_timebase.num = avctx->time_base.num;
+    ctx->enccfg.g_timebase.den = avctx->time_base.den;
+    ctx->enccfg.g_threads      =
         FFMIN(avctx->thread_count ? avctx->thread_count : av_cpu_count(), 64);
 
     if (ctx->lag_in_frames >= 0)
-        enccfg.g_lag_in_frames = ctx->lag_in_frames;
-
-    if (avctx->flags & AV_CODEC_FLAG_PASS1)
-        enccfg.g_pass = AOM_RC_FIRST_PASS;
-    else if (avctx->flags & AV_CODEC_FLAG_PASS2)
-        enccfg.g_pass = AOM_RC_LAST_PASS;
-    else
-        enccfg.g_pass = AOM_RC_ONE_PASS;
+        ctx->enccfg.g_lag_in_frames = ctx->lag_in_frames;
 
     if (avctx->rc_min_rate == avctx->rc_max_rate &&
         avctx->rc_min_rate == avctx->bit_rate && avctx->bit_rate) {
-        enccfg.rc_end_usage = AOM_CBR;
+        ctx->enccfg.rc_end_usage = AOM_CBR;
     } else if (ctx->crf >= 0) {
-        enccfg.rc_end_usage = AOM_CQ;
+        ctx->enccfg.rc_end_usage = AOM_CQ;
         if (!avctx->bit_rate)
-            enccfg.rc_end_usage = AOM_Q;
+            ctx->enccfg.rc_end_usage = AOM_Q;
     }
 
     if (avctx->bit_rate) {
-        enccfg.rc_target_bitrate = av_rescale_rnd(avctx->bit_rate, 1, 1000,
+        ctx->enccfg.rc_target_bitrate = av_rescale_rnd(avctx->bit_rate, 1, 1000,
                                                   AV_ROUND_NEAR_INF);
-    } else if (enccfg.rc_end_usage != AOM_Q) {
-        enccfg.rc_end_usage = AOM_Q;
+    } else if (ctx->enccfg.rc_end_usage != AOM_Q) {
+        ctx->enccfg.rc_end_usage = AOM_Q;
         ctx->crf = 32;
         av_log(avctx, AV_LOG_WARNING,
                "Neither bitrate nor constrained quality specified, using default CRF of %d\n",
@@ -744,95 +731,65 @@ static av_cold int aom_init(AVCodecContext *avctx,
     }
 
     if (avctx->qmin >= 0)
-        enccfg.rc_min_quantizer = avctx->qmin;
+        ctx->enccfg.rc_min_quantizer = avctx->qmin;
     if (avctx->qmax >= 0) {
-        enccfg.rc_max_quantizer = avctx->qmax;
+        ctx->enccfg.rc_max_quantizer = avctx->qmax;
     } else if (!ctx->crf) {
-        enccfg.rc_max_quantizer = 0;
+        ctx->enccfg.rc_max_quantizer = 0;
     }
 
-    if (enccfg.rc_end_usage == AOM_CQ || enccfg.rc_end_usage == AOM_Q) {
-        if (ctx->crf < enccfg.rc_min_quantizer || ctx->crf > enccfg.rc_max_quantizer) {
+    if (ctx->enccfg.rc_end_usage == AOM_CQ || ctx->enccfg.rc_end_usage == AOM_Q) {
+        if (ctx->crf < ctx->enccfg.rc_min_quantizer || ctx->crf > ctx->enccfg.rc_max_quantizer) {
             av_log(avctx, AV_LOG_ERROR,
                    "CQ level %d must be between minimum and maximum quantizer value (%d-%d)\n",
-                   ctx->crf, enccfg.rc_min_quantizer, enccfg.rc_max_quantizer);
+                   ctx->crf, ctx->enccfg.rc_min_quantizer, ctx->enccfg.rc_max_quantizer);
             return AVERROR(EINVAL);
         }
     }
 
-    enccfg.rc_dropframe_thresh = ctx->drop_threshold;
+    ctx->enccfg.rc_dropframe_thresh = ctx->drop_threshold;
 
     // 0-100 (0 => CBR, 100 => VBR)
-    enccfg.rc_2pass_vbr_bias_pct       = round(avctx->qcompress * 100);
+    ctx->enccfg.rc_2pass_vbr_bias_pct       = round(avctx->qcompress * 100);
     if (ctx->minsection_pct >= 0)
-        enccfg.rc_2pass_vbr_minsection_pct = ctx->minsection_pct;
+        ctx->enccfg.rc_2pass_vbr_minsection_pct = ctx->minsection_pct;
     else if (avctx->bit_rate)
-        enccfg.rc_2pass_vbr_minsection_pct =
+        ctx->enccfg.rc_2pass_vbr_minsection_pct =
             avctx->rc_min_rate * 100LL / avctx->bit_rate;
     if (ctx->maxsection_pct >= 0)
-        enccfg.rc_2pass_vbr_maxsection_pct = ctx->maxsection_pct;
+        ctx->enccfg.rc_2pass_vbr_maxsection_pct = ctx->maxsection_pct;
     else if (avctx->rc_max_rate)
-        enccfg.rc_2pass_vbr_maxsection_pct =
+        ctx->enccfg.rc_2pass_vbr_maxsection_pct =
             avctx->rc_max_rate * 100LL / avctx->bit_rate;
 
     if (avctx->rc_buffer_size)
-        enccfg.rc_buf_sz =
+        ctx->enccfg.rc_buf_sz =
             avctx->rc_buffer_size * 1000LL / avctx->bit_rate;
     if (avctx->rc_initial_buffer_occupancy)
-        enccfg.rc_buf_initial_sz =
+        ctx->enccfg.rc_buf_initial_sz =
             avctx->rc_initial_buffer_occupancy * 1000LL / avctx->bit_rate;
-    enccfg.rc_buf_optimal_sz = enccfg.rc_buf_sz * 5 / 6;
+    ctx->enccfg.rc_buf_optimal_sz = ctx->enccfg.rc_buf_sz * 5 / 6;
 
     if (ctx->rc_undershoot_pct >= 0)
-        enccfg.rc_undershoot_pct = ctx->rc_undershoot_pct;
+        ctx->enccfg.rc_undershoot_pct = ctx->rc_undershoot_pct;
     if (ctx->rc_overshoot_pct >= 0)
-        enccfg.rc_overshoot_pct = ctx->rc_overshoot_pct;
+        ctx->enccfg.rc_overshoot_pct = ctx->rc_overshoot_pct;
 
     // _enc_init() will balk if kf_min_dist differs from max w/AOM_KF_AUTO
     if (avctx->keyint_min >= 0 && avctx->keyint_min == avctx->gop_size)
-        enccfg.kf_min_dist = avctx->keyint_min;
+        ctx->enccfg.kf_min_dist = avctx->keyint_min;
     if (avctx->gop_size >= 0)
-        enccfg.kf_max_dist = avctx->gop_size;
-
-    if (enccfg.g_pass == AOM_RC_FIRST_PASS)
-        enccfg.g_lag_in_frames = 0;
-    else if (enccfg.g_pass == AOM_RC_LAST_PASS) {
-        int decode_size, ret;
-
-        if (!avctx->stats_in) {
-            av_log(avctx, AV_LOG_ERROR, "No stats file for second pass\n");
-            return AVERROR_INVALIDDATA;
-        }
-
-        ctx->twopass_stats.sz = strlen(avctx->stats_in) * 3 / 4;
-        ret                   = av_reallocp(&ctx->twopass_stats.buf, ctx->twopass_stats.sz);
-        if (ret < 0) {
-            av_log(avctx, AV_LOG_ERROR,
-                   "Stat buffer alloc (%"SIZE_SPECIFIER" bytes) failed\n",
-                   ctx->twopass_stats.sz);
-            ctx->twopass_stats.sz = 0;
-            return ret;
-        }
-        decode_size = av_base64_decode(ctx->twopass_stats.buf, avctx->stats_in,
-                                       ctx->twopass_stats.sz);
-        if (decode_size < 0) {
-            av_log(avctx, AV_LOG_ERROR, "Stat buffer decode failed\n");
-            return AVERROR_INVALIDDATA;
-        }
-
-        ctx->twopass_stats.sz      = decode_size;
-        enccfg.rc_twopass_stats_in = ctx->twopass_stats;
-    }
+        ctx->enccfg.kf_max_dist = avctx->gop_size;
 
     /* 0-3: For non-zero values the encoder increasingly optimizes for reduced
      * complexity playback on low powered devices at the expense of encode
      * quality. */
     if (avctx->profile != AV_PROFILE_UNKNOWN)
-        enccfg.g_profile = avctx->profile;
+        ctx->enccfg.g_profile = avctx->profile;
 
-    enccfg.g_error_resilient = ctx->error_resilient;
+    ctx->enccfg.g_error_resilient = ctx->error_resilient;
 
-    res = choose_tiling(avctx, &enccfg);
+    res = choose_tiling(avctx, &ctx->enccfg);
     if (res < 0)
         return res;
 
@@ -840,22 +797,22 @@ static av_cold int aom_init(AVCodecContext *avctx,
         // Set the maximum number of frames to 1. This will let libaom set
         // still_picture and reduced_still_picture_header to 1 in the Sequence
         // Header as required by AVIF still images.
-        enccfg.g_limit = 1;
+        ctx->enccfg.g_limit = 1;
         // Reduce memory usage for still images.
-        enccfg.g_lag_in_frames = 0;
+        ctx->enccfg.g_lag_in_frames = 0;
         // All frames will be key frames.
-        enccfg.kf_max_dist = 0;
-        enccfg.kf_mode = AOM_KF_DISABLED;
+        ctx->enccfg.kf_max_dist = 0;
+        ctx->enccfg.kf_mode = AOM_KF_DISABLED;
     }
 
-    /* Construct Encoder Context */
-    res = aom_codec_enc_init(&ctx->encoder, iface, &enccfg, flags);
-    if (res != AOM_CODEC_OK) {
-        dump_enc_cfg(avctx, &enccfg, AV_LOG_WARNING);
-        log_encoder_error(avctx, "Failed to initialize encoder");
-        return AVERROR(EINVAL);
-    }
-    dump_enc_cfg(avctx, &enccfg, AV_LOG_DEBUG);
+    return 0;
+}
+
+static av_cold int aom_codecctl(AVCodecContext *avctx)
+{
+    AOMContext *ctx = avctx->priv_data;
+    const AVPixFmtDescriptor *desc = av_pix_fmt_desc_get(avctx->pix_fmt);
+    aom_codec_caps_t codec_caps = aom_codec_get_caps(ctx->encoder.iface);
 
     // codec control failures are currently treated only as warnings
     av_log(avctx, AV_LOG_DEBUG, "aom_codec_control\n");
@@ -983,20 +940,87 @@ static av_cold int aom_init(AVCodecContext *avctx,
 #endif
 
     // provide dummy value to initialize wrapper, values will be updated each _encode()
-    aom_img_wrap(&ctx->rawimg, img_fmt, avctx->width, avctx->height, 1,
+    aom_img_wrap(&ctx->rawimg, ctx->img_fmt, avctx->width, avctx->height, 1,
                  (unsigned char*)1);
 
     if (codec_caps & AOM_CODEC_CAP_HIGHBITDEPTH)
-        ctx->rawimg.bit_depth = enccfg.g_bit_depth;
+        ctx->rawimg.bit_depth = ctx->enccfg.g_bit_depth;
 
-    cpb_props = ff_encode_add_cpb_side_data(avctx);
-    if (!cpb_props)
-        return AVERROR(ENOMEM);
+    return 0;
+}
+
+static av_cold int aom_init(AVCodecContext *avctx,
+                            const struct aom_codec_iface *iface)
+{
+    AOMContext *ctx = avctx->priv_data;
+    AVCPBProperties *cpb_props;
+    int res;
+
+    av_log(avctx, AV_LOG_INFO, "%s\n", aom_codec_version_str());
+    av_log(avctx, AV_LOG_VERBOSE, "%s\n", aom_codec_build_config());
+
+    res = aom_config(avctx, iface);
+    if (res < 0)
+        return res;
+
+    if (avctx->flags & AV_CODEC_FLAG_PASS1)
+        ctx->enccfg.g_pass = AOM_RC_FIRST_PASS;
+    else if (avctx->flags & AV_CODEC_FLAG_PASS2)
+        ctx->enccfg.g_pass = AOM_RC_LAST_PASS;
+    else
+        ctx->enccfg.g_pass = AOM_RC_ONE_PASS;
+
+    if (ctx->enccfg.g_pass == AOM_RC_FIRST_PASS)
+        ctx->enccfg.g_lag_in_frames = 0;
+    else if (ctx->enccfg.g_pass == AOM_RC_LAST_PASS) {
+        int decode_size, ret;
+
+        if (!avctx->stats_in) {
+            av_log(avctx, AV_LOG_ERROR, "No stats file for second pass\n");
+            return AVERROR_INVALIDDATA;
+        }
+
+        ctx->twopass_stats.sz = strlen(avctx->stats_in) * 3 / 4;
+        ret                   = av_reallocp(&ctx->twopass_stats.buf, ctx->twopass_stats.sz);
+        if (ret < 0) {
+            av_log(avctx, AV_LOG_ERROR,
+                   "Stat buffer alloc (%"SIZE_SPECIFIER" bytes) failed\n",
+                   ctx->twopass_stats.sz);
+            ctx->twopass_stats.sz = 0;
+            return ret;
+        }
+        decode_size = av_base64_decode(ctx->twopass_stats.buf, avctx->stats_in,
+                                       ctx->twopass_stats.sz);
+        if (decode_size < 0) {
+            av_log(avctx, AV_LOG_ERROR, "Stat buffer decode failed\n");
+            return AVERROR_INVALIDDATA;
+        }
+
+        ctx->twopass_stats.sz      = decode_size;
+        ctx->enccfg.rc_twopass_stats_in = ctx->twopass_stats;
+    }
+
+    /* Construct Encoder Context */
+    res = aom_codec_enc_init(&ctx->encoder, iface, &ctx->enccfg, ctx->flags);
+    if (res != AOM_CODEC_OK) {
+        dump_enc_cfg(avctx, &ctx->enccfg, AV_LOG_WARNING);
+        log_encoder_error(avctx, "Failed to initialize encoder");
+        return AVERROR(EINVAL);
+    }
+    dump_enc_cfg(avctx, &ctx->enccfg, AV_LOG_DEBUG);
+
+    res = aom_codecctl(avctx);
+    if (res < 0)
+        return res;
 
     ctx->dovi.logctx = avctx;
     if ((res = ff_dovi_configure(&ctx->dovi, avctx)) < 0)
         return res;
 
+    cpb_props = ff_encode_add_cpb_side_data(avctx);
+    if (!cpb_props)
+        return AVERROR(ENOMEM);
+
     if (avctx->flags & AV_CODEC_FLAG_GLOBAL_HEADER) {
         const AVBitStreamFilter *filter = av_bsf_get_by_name("extract_extradata");
         int ret;
@@ -1019,8 +1043,8 @@ static av_cold int aom_init(AVCodecContext *avctx,
            return ret;
     }
 
-    if (enccfg.rc_end_usage == AOM_CBR ||
-        enccfg.g_pass != AOM_RC_ONE_PASS) {
+    if (ctx->enccfg.rc_end_usage == AOM_CBR ||
+        ctx->enccfg.g_pass != AOM_RC_ONE_PASS) {
         cpb_props->max_bitrate = avctx->rc_max_rate;
         cpb_props->min_bitrate = avctx->rc_min_rate;
         cpb_props->avg_bitrate = avctx->bit_rate;
@@ -1464,6 +1488,32 @@ static av_cold int av1_init(AVCodecContext *avctx)
     return aom_init(avctx, aom_codec_av1_cx());
 }
 
+static av_cold int av1_reconf(AVCodecContext *avctx)
+{
+    AOMContext *ctx = avctx->priv_data;
+    int loglevel;
+    int res;
+
+    res = aom_config(avctx, ctx->encoder.iface);
+    if (res < 0)
+        return res;
+
+    res = aom_codec_enc_config_set(&ctx->encoder, &ctx->enccfg);
+    loglevel = res != AOM_CODEC_OK ? AV_LOG_WARNING : AV_LOG_DEBUG;
+    av_log(avctx, loglevel, "Reconfigure options:\n");
+    dump_enc_cfg(avctx, &ctx->enccfg, loglevel);
+    if (res != AOM_CODEC_OK) {
+        log_encoder_error(avctx, "Failed to reconfigure encoder");
+        return AVERROR(EINVAL);
+    }
+
+    res = aom_codecctl(avctx);
+    if (res < 0)
+        return res;
+
+    return 0;
+}
+
 #define OFFSET(x) offsetof(AOMContext, x)
 #define VE AV_OPT_FLAG_VIDEO_PARAM | AV_OPT_FLAG_ENCODING_PARAM
 static const AVOption options[] = {
@@ -1567,6 +1617,7 @@ FFCodec ff_libaom_av1_encoder = {
     .p.id           = AV_CODEC_ID_AV1,
     .p.capabilities = AV_CODEC_CAP_DR1 | AV_CODEC_CAP_DELAY |
                       AV_CODEC_CAP_ENCODER_RECON_FRAME |
+                      AV_CODEC_CAP_PARAM_CHANGE |
                       AV_CODEC_CAP_OTHER_THREADS,
     .color_ranges   = AVCOL_RANGE_MPEG | AVCOL_RANGE_JPEG,
     .p.profiles     = NULL_IF_CONFIG_SMALL(ff_av1_profiles),
@@ -1574,6 +1625,7 @@ FFCodec ff_libaom_av1_encoder = {
     .p.wrapper_name = "libaom",
     .priv_data_size = sizeof(AOMContext),
     .init           = av1_init,
+    .reconf         = av1_reconf,
     FF_CODEC_ENCODE_CB(aom_encode),
     .close          = aom_free,
     .caps_internal  = FF_CODEC_CAP_NOT_INIT_THREADSAFE |
-- 
2.48.1

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [FFmpeg-devel] [PATCH 7/7] avcodec/libx264: add support for param change frame side data
  2025-01-22  2:53 [FFmpeg-devel] [PATCH 1/7] avutil/avutil: move some definitions to a new header James Almer
                   ` (4 preceding siblings ...)
  2025-01-22  2:54 ` [FFmpeg-devel] [PATCH 6/7] avcodec/libaomenc: add support for param change frame side data James Almer
@ 2025-01-22  2:54 ` James Almer
  2025-01-27 12:43 ` [FFmpeg-devel] [PATCH 1/7] avutil/avutil: move some definitions to a new header Andreas Rheinhardt
  6 siblings, 0 replies; 21+ messages in thread
From: James Almer @ 2025-01-22  2:54 UTC (permalink / raw)
  To: ffmpeg-devel

Signed-off-by: James Almer <jamrial@gmail.com>
---
 libavcodec/libx264.c | 98 ++++++++++++++++++++++++++------------------
 1 file changed, 59 insertions(+), 39 deletions(-)

diff --git a/libavcodec/libx264.c b/libavcodec/libx264.c
index 409f45fc7d..c64c2a28b0 100644
--- a/libavcodec/libx264.c
+++ b/libavcodec/libx264.c
@@ -194,54 +194,64 @@ static int encode_nals(AVCodecContext *ctx, AVPacket *pkt,
     return 1;
 }
 
-static void reconfig_encoder(AVCodecContext *ctx, const AVFrame *frame)
+static void reconfig_encoder(AVCodecContext *ctx)
 {
     X264Context *x4 = ctx->priv_data;
-    AVFrameSideData *side_data;
 
+    if (x4->avcintra_class >= 0)
+        return;
 
-    if (x4->avcintra_class < 0) {
-        if (x4->params.b_interlaced && x4->params.b_tff != !!(frame->flags & AV_FRAME_FLAG_TOP_FIELD_FIRST)) {
+    if (x4->params.vui.i_sar_height*ctx->sample_aspect_ratio.num != ctx->sample_aspect_ratio.den * x4->params.vui.i_sar_width) {
+        x4->params.vui.i_sar_height = ctx->sample_aspect_ratio.den;
+        x4->params.vui.i_sar_width  = ctx->sample_aspect_ratio.num;
+        x264_encoder_reconfig(x4->enc, &x4->params);
+    }
 
-            x4->params.b_tff = !!(frame->flags & AV_FRAME_FLAG_TOP_FIELD_FIRST);
-            x264_encoder_reconfig(x4->enc, &x4->params);
-        }
-        if (x4->params.vui.i_sar_height*ctx->sample_aspect_ratio.num != ctx->sample_aspect_ratio.den * x4->params.vui.i_sar_width) {
-            x4->params.vui.i_sar_height = ctx->sample_aspect_ratio.den;
-            x4->params.vui.i_sar_width  = ctx->sample_aspect_ratio.num;
-            x264_encoder_reconfig(x4->enc, &x4->params);
-        }
+    if (x4->params.rc.i_vbv_buffer_size != ctx->rc_buffer_size / 1000 ||
+        x4->params.rc.i_vbv_max_bitrate != ctx->rc_max_rate    / 1000) {
+        x4->params.rc.i_vbv_buffer_size = ctx->rc_buffer_size / 1000;
+        x4->params.rc.i_vbv_max_bitrate = ctx->rc_max_rate    / 1000;
+        x264_encoder_reconfig(x4->enc, &x4->params);
+    }
 
-        if (x4->params.rc.i_vbv_buffer_size != ctx->rc_buffer_size / 1000 ||
-            x4->params.rc.i_vbv_max_bitrate != ctx->rc_max_rate    / 1000) {
-            x4->params.rc.i_vbv_buffer_size = ctx->rc_buffer_size / 1000;
-            x4->params.rc.i_vbv_max_bitrate = ctx->rc_max_rate    / 1000;
-            x264_encoder_reconfig(x4->enc, &x4->params);
-        }
+    if (x4->params.rc.i_rc_method == X264_RC_ABR &&
+        x4->params.rc.i_bitrate != ctx->bit_rate / 1000) {
+        x4->params.rc.i_bitrate = ctx->bit_rate / 1000;
+        x264_encoder_reconfig(x4->enc, &x4->params);
+    }
 
-        if (x4->params.rc.i_rc_method == X264_RC_ABR &&
-            x4->params.rc.i_bitrate != ctx->bit_rate / 1000) {
-            x4->params.rc.i_bitrate = ctx->bit_rate / 1000;
-            x264_encoder_reconfig(x4->enc, &x4->params);
-        }
+    if (x4->crf >= 0 &&
+        x4->params.rc.i_rc_method == X264_RC_CRF &&
+        x4->params.rc.f_rf_constant != x4->crf) {
+        x4->params.rc.f_rf_constant = x4->crf;
+        x264_encoder_reconfig(x4->enc, &x4->params);
+    }
 
-        if (x4->crf >= 0 &&
-            x4->params.rc.i_rc_method == X264_RC_CRF &&
-            x4->params.rc.f_rf_constant != x4->crf) {
-            x4->params.rc.f_rf_constant = x4->crf;
-            x264_encoder_reconfig(x4->enc, &x4->params);
-        }
+    if (x4->params.rc.i_rc_method == X264_RC_CQP &&
+        x4->cqp >= 0 &&
+        x4->params.rc.i_qp_constant != x4->cqp) {
+        x4->params.rc.i_qp_constant = x4->cqp;
+        x264_encoder_reconfig(x4->enc, &x4->params);
+    }
 
-        if (x4->params.rc.i_rc_method == X264_RC_CQP &&
-            x4->cqp >= 0 &&
-            x4->params.rc.i_qp_constant != x4->cqp) {
-            x4->params.rc.i_qp_constant = x4->cqp;
-            x264_encoder_reconfig(x4->enc, &x4->params);
-        }
+    if (x4->crf_max >= 0 &&
+        x4->params.rc.f_rf_constant_max != x4->crf_max) {
+        x4->params.rc.f_rf_constant_max = x4->crf_max;
+        x264_encoder_reconfig(x4->enc, &x4->params);
+    }
+}
 
-        if (x4->crf_max >= 0 &&
-            x4->params.rc.f_rf_constant_max != x4->crf_max) {
-            x4->params.rc.f_rf_constant_max = x4->crf_max;
+static void reconfig_encoder_from_frame(AVCodecContext *ctx, const AVFrame *frame)
+{
+    X264Context *x4 = ctx->priv_data;
+    AVFrameSideData *side_data;
+
+    reconfig_encoder(ctx);
+
+    if (x4->avcintra_class < 0) {
+        if (x4->params.b_interlaced && x4->params.b_tff != !!(frame->flags & AV_FRAME_FLAG_TOP_FIELD_FIRST)) {
+
+            x4->params.b_tff = !!(frame->flags & AV_FRAME_FLAG_TOP_FIELD_FIRST);
             x264_encoder_reconfig(x4->enc, &x4->params);
         }
     }
@@ -528,7 +538,7 @@ static int setup_frame(AVCodecContext *ctx, const AVFrame *frame,
         pic->i_type = X264_TYPE_AUTO;
         break;
     }
-    reconfig_encoder(ctx, frame);
+    reconfig_encoder_from_frame(ctx, frame);
 
     if (x4->a53_cc) {
         void *sei_data;
@@ -759,6 +769,14 @@ static void X264_flush(AVCodecContext *avctx)
         x4->sei_size = -x4->sei_size;
 }
 
+static av_cold int X264_reconf(AVCodecContext *avctx)
+{
+    X264Context *x4 = avctx->priv_data;
+
+    reconfig_encoder(avctx);
+    return 0;
+}
+
 static av_cold int X264_close(AVCodecContext *avctx)
 {
     X264Context *x4 = avctx->priv_data;
@@ -1624,6 +1642,7 @@ const FFCodec ff_libx264_encoder = {
                         AV_CODEC_CAP_OTHER_THREADS |
                         AV_CODEC_CAP_ENCODER_REORDERED_OPAQUE |
                         AV_CODEC_CAP_ENCODER_FLUSH |
+                        AV_CODEC_CAP_PARAM_CHANGE |
                         AV_CODEC_CAP_ENCODER_RECON_FRAME,
     .p.priv_class     = &x264_class,
     .p.wrapper_name   = "libx264",
@@ -1631,6 +1650,7 @@ const FFCodec ff_libx264_encoder = {
     .init             = X264_init,
     FF_CODEC_ENCODE_CB(X264_frame),
     .flush            = X264_flush,
+    .reconf           = X264_reconf,
     .close            = X264_close,
     .defaults         = x264_defaults,
     .p.pix_fmts       = pix_fmts_all,
-- 
2.48.1

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [FFmpeg-devel] [PATCH 3/7] avutil/frame: add a param change side data type
  2025-01-22  2:53 ` [FFmpeg-devel] [PATCH 3/7] avutil/frame: add a param change side data type James Almer
@ 2025-01-22  3:13   ` Lynne
  2025-01-22 13:40     ` James Almer
  0 siblings, 1 reply; 21+ messages in thread
From: Lynne @ 2025-01-22  3:13 UTC (permalink / raw)
  To: ffmpeg-devel


[-- Attachment #1.1.1.1: Type: text/plain, Size: 2443 bytes --]



On 22/01/2025 11:53, James Almer wrote:
> Equivalent to the one from packet side data using AVSideDataParamChangeFlags,
> and will be used for the same purpose with encoders.
> 
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
>   libavutil/frame.c |  1 +
>   libavutil/frame.h | 13 +++++++++++++
>   2 files changed, 14 insertions(+)
> 
> diff --git a/libavutil/frame.c b/libavutil/frame.c
> index 10b59545f0..f3575afc31 100644
> --- a/libavutil/frame.c
> +++ b/libavutil/frame.c
> @@ -48,6 +48,7 @@ static const AVSideDataDescriptor sd_props[] = {
>       [AV_FRAME_DATA_DOVI_METADATA]               = { "Dolby Vision Metadata",                        AV_SIDE_DATA_PROP_COLOR_DEPENDENT },
>       [AV_FRAME_DATA_LCEVC]                       = { "LCEVC NAL data",                               AV_SIDE_DATA_PROP_SIZE_DEPENDENT },
>       [AV_FRAME_DATA_VIEW_ID]                     = { "View ID" },
> +    [AV_FRAME_DATA_PARAM_CHANGE]                = { "Param Change" },
>       [AV_FRAME_DATA_STEREO3D]                    = { "Stereo 3D",                                    AV_SIDE_DATA_PROP_GLOBAL },
>       [AV_FRAME_DATA_REPLAYGAIN]                  = { "AVReplayGain",                                 AV_SIDE_DATA_PROP_GLOBAL },
>       [AV_FRAME_DATA_DISPLAYMATRIX]               = { "3x3 displaymatrix",                            AV_SIDE_DATA_PROP_GLOBAL },
> diff --git a/libavutil/frame.h b/libavutil/frame.h
> index 628f2b5b3a..de7a8e9eab 100644
> --- a/libavutil/frame.h
> +++ b/libavutil/frame.h
> @@ -243,6 +243,19 @@ enum AVFrameSideDataType {
>        * The data is an int storing the view ID.
>        */
>       AV_FRAME_DATA_VIEW_ID,
> +
> +    /**
> +     * An AV_FRAME_DATA_PARAM_CHANGE side data packet is laid out as follows:
> +     * @code
> +     * u32le param_flags
> +     * if (param_flags & AV_SIDE_DATA_PARAM_CHANGE_SAMPLE_RATE)
> +     *     s32le sample_rate
> +     * if (param_flags & AV_SIDE_DATA_PARAM_CHANGE_DIMENSIONS)
> +     *     s32le width
> +     *     s32le height
> +     * @endcode
> +     */
> +    AV_FRAME_DATA_PARAM_CHANGE,

Could you make the struct defined, and allocated by libavutil, like with 
encode data and film grain?
That way new fields could be added in the future.

I'd also like for the FPS and timebase to be signaled. Encoders can 
sometimes handle them gracefully, as its just a rate control parameter 
change.

[-- Attachment #1.1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 637 bytes --]

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 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".

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [FFmpeg-devel] [PATCH 3/7] avutil/frame: add a param change side data type
  2025-01-22  3:13   ` Lynne
@ 2025-01-22 13:40     ` James Almer
  2025-01-22 19:49       ` Nicolas George
  0 siblings, 1 reply; 21+ messages in thread
From: James Almer @ 2025-01-22 13:40 UTC (permalink / raw)
  To: ffmpeg-devel


[-- Attachment #1.1.1: Type: text/plain, Size: 3420 bytes --]

On 1/22/2025 12:13 AM, Lynne wrote:
> 
> 
> On 22/01/2025 11:53, James Almer wrote:
>> Equivalent to the one from packet side data using 
>> AVSideDataParamChangeFlags,
>> and will be used for the same purpose with encoders.
>>
>> Signed-off-by: James Almer <jamrial@gmail.com>
>> ---
>>   libavutil/frame.c |  1 +
>>   libavutil/frame.h | 13 +++++++++++++
>>   2 files changed, 14 insertions(+)
>>
>> diff --git a/libavutil/frame.c b/libavutil/frame.c
>> index 10b59545f0..f3575afc31 100644
>> --- a/libavutil/frame.c
>> +++ b/libavutil/frame.c
>> @@ -48,6 +48,7 @@ static const AVSideDataDescriptor sd_props[] = {
>>       [AV_FRAME_DATA_DOVI_METADATA]               = { "Dolby Vision 
>> Metadata",                        AV_SIDE_DATA_PROP_COLOR_DEPENDENT },
>>       [AV_FRAME_DATA_LCEVC]                       = { "LCEVC NAL 
>> data",                               AV_SIDE_DATA_PROP_SIZE_DEPENDENT },
>>       [AV_FRAME_DATA_VIEW_ID]                     = { "View ID" },
>> +    [AV_FRAME_DATA_PARAM_CHANGE]                = { "Param Change" },
>>       [AV_FRAME_DATA_STEREO3D]                    = { "Stereo 
>> 3D",                                    AV_SIDE_DATA_PROP_GLOBAL },
>>       [AV_FRAME_DATA_REPLAYGAIN]                  = 
>> { "AVReplayGain",                                 
>> AV_SIDE_DATA_PROP_GLOBAL },
>>       [AV_FRAME_DATA_DISPLAYMATRIX]               = { "3x3 
>> displaymatrix",                            AV_SIDE_DATA_PROP_GLOBAL },
>> diff --git a/libavutil/frame.h b/libavutil/frame.h
>> index 628f2b5b3a..de7a8e9eab 100644
>> --- a/libavutil/frame.h
>> +++ b/libavutil/frame.h
>> @@ -243,6 +243,19 @@ enum AVFrameSideDataType {
>>        * The data is an int storing the view ID.
>>        */
>>       AV_FRAME_DATA_VIEW_ID,
>> +
>> +    /**
>> +     * An AV_FRAME_DATA_PARAM_CHANGE side data packet is laid out as 
>> follows:
>> +     * @code
>> +     * u32le param_flags
>> +     * if (param_flags & AV_SIDE_DATA_PARAM_CHANGE_SAMPLE_RATE)
>> +     *     s32le sample_rate
>> +     * if (param_flags & AV_SIDE_DATA_PARAM_CHANGE_DIMENSIONS)
>> +     *     s32le width
>> +     *     s32le height
>> +     * @endcode
>> +     */
>> +    AV_FRAME_DATA_PARAM_CHANGE,
> 
> Could you make the struct defined, and allocated by libavutil, like with 
> encode data and film grain?
> That way new fields could be added in the future.

New fields can be added this way too. The reason i didn't make it a 
struct is that i wanted to introduce the least amount of new public 
defines/symbols, thus reusing the packet side data implementation.

> 
> I'd also like for the FPS and timebase to be signaled. Encoders can 
> sometimes handle them gracefully, as its just a rate control parameter 
> change.
> 
> _______________________________________________
> 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".


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 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".

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [FFmpeg-devel] [PATCH 3/7] avutil/frame: add a param change side data type
  2025-01-22 13:40     ` James Almer
@ 2025-01-22 19:49       ` Nicolas George
  2025-01-22 19:58         ` James Almer
  0 siblings, 1 reply; 21+ messages in thread
From: Nicolas George @ 2025-01-22 19:49 UTC (permalink / raw)
  To: FFmpeg development discussions and patches

James Almer (12025-01-22):
> New fields can be added this way too. The reason i didn't make it a struct
> is that i wanted to introduce the least amount of new public
> defines/symbols, thus reusing the packet side data implementation.

Let us make the packet side data a struct too. This version is terrible
design that puts a lot of burden on all sides of the code and wastes the
ability of the compiler to check the structure, this is terrible
design¹.

Since the first field is a flag, we can easily make a compatibility
layer by adding a AV_SIDE_DATA_PARAM_CHANGE_STRUCT_API flag.

And we can dispense with all the _alloc() and _free() API in libavutil and
let the applications allocate as they want, including on the stack, the
code just needs to check size before accessing a field.

Later let us fix the other side data that use this pattern.

1: One `git blame` later… yeah, not surprised.

Regards,

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [FFmpeg-devel] [PATCH 3/7] avutil/frame: add a param change side data type
  2025-01-22 19:49       ` Nicolas George
@ 2025-01-22 19:58         ` James Almer
  2025-01-22 20:18           ` Nicolas George
  0 siblings, 1 reply; 21+ messages in thread
From: James Almer @ 2025-01-22 19:58 UTC (permalink / raw)
  To: ffmpeg-devel


[-- Attachment #1.1.1: Type: text/plain, Size: 1102 bytes --]

On 1/22/2025 4:49 PM, Nicolas George wrote:
> James Almer (12025-01-22):
>> New fields can be added this way too. The reason i didn't make it a struct
>> is that i wanted to introduce the least amount of new public
>> defines/symbols, thus reusing the packet side data implementation.
> 
> Let us make the packet side data a struct too. This version is terrible
> design that puts a lot of burden on all sides of the code and wastes the
> ability of the compiler to check the structure, this is terrible
> design¹.

I agree, it's not a good design.

> 
> Since the first field is a flag, we can easily make a compatibility
> layer by adding a AV_SIDE_DATA_PARAM_CHANGE_STRUCT_API flag.

Ok, will implement a new struct.

> 
> And we can dispense with all the _alloc() and _free() API in libavutil and
> let the applications allocate as they want, including on the stack, the
> code just needs to check size before accessing a field.
> 
> Later let us fix the other side data that use this pattern.
> 
> 1: One `git blame` later… yeah, not surprised.
> 
> Regards,
> 


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 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".

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [FFmpeg-devel] [PATCH 3/7] avutil/frame: add a param change side data type
  2025-01-22 19:58         ` James Almer
@ 2025-01-22 20:18           ` Nicolas George
  2025-01-22 20:52             ` James Almer
  2025-01-23 21:49             ` Alexander Strasser via ffmpeg-devel
  0 siblings, 2 replies; 21+ messages in thread
From: Nicolas George @ 2025-01-22 20:18 UTC (permalink / raw)
  To: FFmpeg development discussions and patches

James Almer (12025-01-22):
> Ok, will implement a new struct.

Thanks.

I think this code would work:

typedef struct AVSideDataParamChange
    …
    intmax_t end_padding;
} AVSideDataSomethingType;

static const AVSideDataParamChange default = { ... };


    AVSideDataParamChange pc;
    av_assert0(sd->size > sizeof(pc.end_padding));
    size_t s = FFMIN(sd->size - sizeof(pc.end_padding), sizeof(pc));
    memcpy(&pc, sd->data, s);
    memcpy((char *)&pc + s, (char *)&default + s, sizeof(pc) - s);

After this code, pc contains the fields from the side data if they are
present in the version known by the caller and the fields from default
otherwise.

I am rather sure this is valid C unless there we add a field with
individual size greater than end_padding, hence intmax_t.

If I can at some time start spending efforts on FFmpeg again, I intend
to use this pattern on things that need to be lightweight (no dynamic
allocations) but extensible.

Regards,

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [FFmpeg-devel] [PATCH 3/7] avutil/frame: add a param change side data type
  2025-01-22 20:18           ` Nicolas George
@ 2025-01-22 20:52             ` James Almer
  2025-01-22 21:14               ` Nicolas George
  2025-01-23 21:49             ` Alexander Strasser via ffmpeg-devel
  1 sibling, 1 reply; 21+ messages in thread
From: James Almer @ 2025-01-22 20:52 UTC (permalink / raw)
  To: ffmpeg-devel


[-- Attachment #1.1.1: Type: text/plain, Size: 484 bytes --]

On 1/22/2025 5:18 PM, Nicolas George wrote:
> James Almer (12025-01-22):
>> Ok, will implement a new struct.
> 
> Thanks.
> 
> I think this code would work:
> 
> typedef struct AVSideDataParamChange
>      …
>      intmax_t end_padding;
> } AVSideDataSomethingType;

Actually, i just realized i can't store an AVDictionary or a string in a 
struct like this that's meant to be stored as side data. It has a to be 
a flat array with no pointers to separate arrays.


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 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".

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [FFmpeg-devel] [PATCH 3/7] avutil/frame: add a param change side data type
  2025-01-22 20:52             ` James Almer
@ 2025-01-22 21:14               ` Nicolas George
  2025-01-22 21:21                 ` Nicolas George
  0 siblings, 1 reply; 21+ messages in thread
From: Nicolas George @ 2025-01-22 21:14 UTC (permalink / raw)
  To: FFmpeg development discussions and patches

James Almer (12025-01-22):
> Actually, i just realized i can't store an AVDictionary or a string in a
> struct like this that's meant to be stored as side data. It has a to be a
> flat array with no pointers to separate arrays.

Hum, good point.

That limitation of side data is very annoying. We should get rid of it,
especially since it is not that hard.

The issue is that side data has to be flat because its refcounting is
built on top of AVBufferRef when AVBufferRef if only good for flat data
(if it is even good for that).

So, plan:

- Add to AVSideDataDescriptor function pointer for free() and clone()
  and ref().

- For existing side data types, these three fields are set to an
  implementation working with AVBufferRef.

- For new side data types and side data types we want to de-flatten, we
  populate these fields with an adequate function.

That would be a tremendous enhancement to the side data system.

(Having a structure for every type in FFmpeg that contains function
pointers to free, ref, clone, display, parse, hash, compare, etc.,
elements of that type was a project of mine. It went nowhere for the
obvious reason it requires an efficient strings API.)

Regards,

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [FFmpeg-devel] [PATCH 3/7] avutil/frame: add a param change side data type
  2025-01-22 21:14               ` Nicolas George
@ 2025-01-22 21:21                 ` Nicolas George
  0 siblings, 0 replies; 21+ messages in thread
From: Nicolas George @ 2025-01-22 21:21 UTC (permalink / raw)
  To: FFmpeg development discussions and patches

Nicolas George (12025-01-22):
> That limitation of side data is very annoying. We should get rid of it,
> especially since it is not that hard.
> 
> The issue is that side data has to be flat because its refcounting is
> built on top of AVBufferRef when AVBufferRef if only good for flat data
> (if it is even good for that).
> 
> So, plan:
> 
> 
> (Having a structure for every type in FFmpeg that contains function
> pointers to free, ref, clone, display, parse, hash, compare, etc.,
> elements of that type was a project of mine. It went nowhere for the
> obvious reason it requires an efficient strings API.)

Now that I think of refcounting, I remember that a long time ago I
proposed a system of templates and macros to easily add refcounting to
any structure. It was argued against and went nowhere, but later
somebody told me they realized my proposal was actually good, and I
think I remember it was you, was it not?

Regards,

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [FFmpeg-devel] [PATCH 3/7] avutil/frame: add a param change side data type
  2025-01-22 20:18           ` Nicolas George
  2025-01-22 20:52             ` James Almer
@ 2025-01-23 21:49             ` Alexander Strasser via ffmpeg-devel
  1 sibling, 0 replies; 21+ messages in thread
From: Alexander Strasser via ffmpeg-devel @ 2025-01-23 21:49 UTC (permalink / raw)
  To: FFmpeg development discussions and patches; +Cc: Alexander Strasser

On 2025-01-22 21:18 +0100, Nicolas George wrote:
> James Almer (12025-01-22):
> > Ok, will implement a new struct.
>
> Thanks.
>
> I think this code would work:
>
> typedef struct AVSideDataParamChange
>     …
>     intmax_t end_padding;
> } AVSideDataSomethingType;
>
> static const AVSideDataParamChange default = { ... };
>
>
>     AVSideDataParamChange pc;
>     av_assert0(sd->size > sizeof(pc.end_padding));
>     size_t s = FFMIN(sd->size - sizeof(pc.end_padding), sizeof(pc));
>     memcpy(&pc, sd->data, s);
>     memcpy((char *)&pc + s, (char *)&default + s, sizeof(pc) - s);
>
> After this code, pc contains the fields from the side data if they are
> present in the version known by the caller and the fields from default
> otherwise.
>
> I am rather sure this is valid C unless there we add a field with
> individual size greater than end_padding, hence intmax_t.

Sorry, couldn't really follow the problem and this solution idea yet.

Just think we shouldn't use intmax_t as I think it wasn't a good idea
to have it in the C standard at all.

Is it essential for your idea?


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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [FFmpeg-devel] [PATCH 1/7] avutil/avutil: move some definitions to a new header
  2025-01-22  2:53 [FFmpeg-devel] [PATCH 1/7] avutil/avutil: move some definitions to a new header James Almer
                   ` (5 preceding siblings ...)
  2025-01-22  2:54 ` [FFmpeg-devel] [PATCH 7/7] avcodec/libx264: " James Almer
@ 2025-01-27 12:43 ` Andreas Rheinhardt
  6 siblings, 0 replies; 21+ messages in thread
From: Andreas Rheinhardt @ 2025-01-27 12:43 UTC (permalink / raw)
  To: ffmpeg-devel

James Almer:
> They don't belong anywhere in particular, so move them to a new generic header
> called defs.h, following the same idea as the one from lavc.
> 
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
>  libavutil/avutil.h |  94 +++-------------------------
>  libavutil/defs.h   | 149 +++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 159 insertions(+), 84 deletions(-)
>  create mode 100644 libavutil/defs.h
> 
> diff --git a/libavutil/avutil.h b/libavutil/avutil.h
> index ee709fbb2a..1cf562c73f 100644
> --- a/libavutil/avutil.h
> +++ b/libavutil/avutil.h
> @@ -191,100 +191,36 @@ const char *avutil_license(void);
>   * @}
>   */
>  
> +#include "common.h"
> +#include "defs.h"
> +#include "rational.h"
> +#include "version.h"
> +#include "macros.h"
> +#include "mathematics.h"
> +#include "log.h"
> +#include "pixfmt.h"
> +
>  /**
>   * @addtogroup lavu_media Media Type
>   * @brief Media Type
> + * @{
>   */
>  
> -enum AVMediaType {
> -    AVMEDIA_TYPE_UNKNOWN = -1,  ///< Usually treated as AVMEDIA_TYPE_DATA
> -    AVMEDIA_TYPE_VIDEO,
> -    AVMEDIA_TYPE_AUDIO,
> -    AVMEDIA_TYPE_DATA,          ///< Opaque data information usually continuous
> -    AVMEDIA_TYPE_SUBTITLE,
> -    AVMEDIA_TYPE_ATTACHMENT,    ///< Opaque data information usually sparse
> -    AVMEDIA_TYPE_NB
> -};
> -
>  /**
>   * Return a string describing the media_type enum, NULL if media_type
>   * is unknown.
>   */
>  const char *av_get_media_type_string(enum AVMediaType media_type);
>  
> -/**
> - * @defgroup lavu_const Constants
> - * @{
> - *
> - * @defgroup lavu_enc Encoding specific
> - *
> - * @note those definition should move to avcodec
> - * @{
> - */
> -
> -#define FF_LAMBDA_SHIFT 7
> -#define FF_LAMBDA_SCALE (1<<FF_LAMBDA_SHIFT)
> -#define FF_QP2LAMBDA 118 ///< factor to convert from H.263 QP to lambda
> -#define FF_LAMBDA_MAX (256*128-1)
> -
> -#define FF_QUALITY_SCALE FF_LAMBDA_SCALE //FIXME maybe remove
> -
>  /**
>   * @}
> - * @defgroup lavu_time Timestamp specific
> - *
> - * FFmpeg internal timebase and timestamp definitions
> - *
> - * @{
> - */
> -
> -/**
> - * @brief Undefined timestamp value
> - *
> - * Usually reported by demuxer that work on containers that do not provide
> - * either pts or dts.
>   */
>  
> -#define AV_NOPTS_VALUE          ((int64_t)UINT64_C(0x8000000000000000))
> -
>  /**
> - * Internal time base represented as integer
> - */
> -
> -#define AV_TIME_BASE            1000000
> -
> -/**
> - * Internal time base represented as fractional value
> - */
> -
> -#ifdef __cplusplus
> -/* ISO C++ forbids compound-literals. */
> -#define AV_TIME_BASE_Q          av_make_q(1, AV_TIME_BASE)
> -#else
> -#define AV_TIME_BASE_Q          (AVRational){1, AV_TIME_BASE}
> -#endif
> -
> -/**
> - * @}
> - * @}
>   * @defgroup lavu_picture Image related
> - *
> - * AVPicture types, pixel formats and basic image planes manipulation.
> - *
>   * @{
>   */
>  
> -enum AVPictureType {
> -    AV_PICTURE_TYPE_NONE = 0, ///< Undefined
> -    AV_PICTURE_TYPE_I,     ///< Intra
> -    AV_PICTURE_TYPE_P,     ///< Predicted
> -    AV_PICTURE_TYPE_B,     ///< Bi-dir predicted
> -    AV_PICTURE_TYPE_S,     ///< S(GMC)-VOP MPEG-4
> -    AV_PICTURE_TYPE_SI,    ///< Switching Intra
> -    AV_PICTURE_TYPE_SP,    ///< Switching Predicted
> -    AV_PICTURE_TYPE_BI,    ///< BI type
> -};
> -
>  /**
>   * Return a single letter to describe the given picture type
>   * pict_type.
> @@ -298,14 +234,6 @@ char av_get_picture_type_char(enum AVPictureType pict_type);
>   * @}
>   */
>  
> -#include "common.h"
> -#include "rational.h"
> -#include "version.h"
> -#include "macros.h"
> -#include "mathematics.h"
> -#include "log.h"
> -#include "pixfmt.h"
> -
>  /**
>   * Return x default pointer in case p is NULL.
>   */
> @@ -343,8 +271,6 @@ unsigned av_int_list_length_for_size(unsigned elsize,
>   */
>  AVRational av_get_time_base_q(void);
>  
> -#define AV_FOURCC_MAX_STRING_SIZE 32
> -

Splitting AV_FOURCC_MAX_STRING_SIZE from av_fourcc2str() makes no sense.

>  #define av_fourcc2str(fourcc) av_fourcc_make_string((char[AV_FOURCC_MAX_STRING_SIZE]){0}, fourcc)
>  
>  /**
> diff --git a/libavutil/defs.h b/libavutil/defs.h
> new file mode 100644
> index 0000000000..9e9bb8bcc5
> --- /dev/null
> +++ b/libavutil/defs.h
> @@ -0,0 +1,149 @@
> +/*
> + * 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
> + */
> +
> +#ifndef AVUTIL_DEFS_H
> +#define AVUTIL_DEFS_H
> +
> +/**
> + * @file
> + * @ingroup lavu
> + * Misc types and constants that do not belong anywhere else.
> + */
> +
> +#include <stdint.h>
> +#include <stdlib.h>

stdlib for what?

> +
> +/**
> + * @addtogroup lavu_misc Other
> + * @{
> + *
> + * @defgroup preproc_misc Preprocessor String Macros
> + *
> + * @{
> + *
> + * @}
> + *
> + * @defgroup version_utils Library Version Macros
> + *
> + * @{
> + *
> + * @}
> + */
> +
> +/**
> + * @addtogroup lavu_media Media Type
> + * @{
> + */
> +enum AVMediaType {
> +    AVMEDIA_TYPE_UNKNOWN = -1,  ///< Usually treated as AVMEDIA_TYPE_DATA
> +    AVMEDIA_TYPE_VIDEO,
> +    AVMEDIA_TYPE_AUDIO,
> +    AVMEDIA_TYPE_DATA,          ///< Opaque data information usually continuous
> +    AVMEDIA_TYPE_SUBTITLE,
> +    AVMEDIA_TYPE_ATTACHMENT,    ///< Opaque data information usually sparse
> +    AVMEDIA_TYPE_NB
> +};
> +
> +/**
> + * @}
> + */
> +
> +/**
> + * @defgroup lavu_const Constants
> + * @{
> + *
> + * @defgroup lavu_enc Encoding specific
> + *
> + * @note those definition should move to avcodec
> + * @{
> + */
> +
> +#define FF_LAMBDA_SHIFT 7
> +#define FF_LAMBDA_SCALE (1<<FF_LAMBDA_SHIFT)
> +#define FF_QP2LAMBDA 118 ///< factor to convert from H.263 QP to lambda
> +#define FF_LAMBDA_MAX (256*128-1)
> +
> +#define FF_QUALITY_SCALE FF_LAMBDA_SCALE //FIXME maybe remove
> +

There is a comment that this stuff should be moved to avcodec (which you
move, too). Maybe it should be moved to avcodec instead of to this new
header? Do all of these constants even need to be public?

> +#define AV_FOURCC_MAX_STRING_SIZE 32
> +
> +/**
> + * @}
> + * @defgroup lavu_time Timestamp specific
> + *
> + * FFmpeg internal timebase and timestamp definitions
> + *
> + * @{
> + */
> +
> +/**
> + * @brief Undefined timestamp value
> + *
> + * Usually reported by demuxer that work on containers that do not provide
> + * either pts or dts.
> + */
> +
> +#define AV_NOPTS_VALUE          ((int64_t)UINT64_C(0x8000000000000000))
> +
> +/**
> + * Internal time base represented as integer
> + */
> +
> +#define AV_TIME_BASE            1000000
> +
> +/**
> + * Internal time base represented as fractional value
> + */
> +
> +#ifdef __cplusplus
> +/* ISO C++ forbids compound-literals. */
> +#define AV_TIME_BASE_Q          av_make_q(1, AV_TIME_BASE)
> +#else
> +#define AV_TIME_BASE_Q          (AVRational){1, AV_TIME_BASE}

This would need an inclusion of rational.h for the user to use it. Is it
intended for public macros to not be immediately usable?

> +#endif
> +
> +/**
> + * @}
> + * @}
> + */
> +
> +/**
> + * @addtogroup lavu_picture Image related
> + *
> + * AVPicture types, pixel formats and basic image planes manipulation.

It is only AVPicture type.

> + *
> + * @{
> + */
> +
> +enum AVPictureType {
> +    AV_PICTURE_TYPE_NONE = 0, ///< Undefined
> +    AV_PICTURE_TYPE_I,     ///< Intra
> +    AV_PICTURE_TYPE_P,     ///< Predicted
> +    AV_PICTURE_TYPE_B,     ///< Bi-dir predicted
> +    AV_PICTURE_TYPE_S,     ///< S(GMC)-VOP MPEG-4
> +    AV_PICTURE_TYPE_SI,    ///< Switching Intra
> +    AV_PICTURE_TYPE_SP,    ///< Switching Predicted
> +    AV_PICTURE_TYPE_BI,    ///< BI type
> +};

This splits AVPictureType from av_get_picture_type_char() which makes
little sense.

> +
> +/**
> + * @}
> + * @}
> + */
> +
> +#endif // AVUTIL_DEFS_H

Apart from that: This patch as-is would also need to add defs.h to the
list of installed header in the Makefile.

But actually, I don't really see the point of this patch. The problem of
avutil.h is not that it contains too much, but that it includes so much.
And fixing this runs into the problem that avutil.h is advertised to be
a "convenience header that includes libavutil's core" (which means:
users are allowed to rely on implicit inclusions). Which IMO should be
common.h, with avutil.h being a normal header.

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [FFmpeg-devel] [PATCH 4/7] avcodec/encode: add support for param change frame side data
  2025-01-22  2:54 ` [FFmpeg-devel] [PATCH 4/7] avcodec/encode: add support for param change frame side data James Almer
@ 2025-01-27 12:48   ` Andreas Rheinhardt
  2025-01-27 12:54     ` James Almer
  0 siblings, 1 reply; 21+ messages in thread
From: Andreas Rheinhardt @ 2025-01-27 12:48 UTC (permalink / raw)
  To: ffmpeg-devel

James Almer:
> With this, callers can signal supported encoders to reinitialize using certain
> parameters, which should allow for more graceful (but potentially more limited
> in scope) reinitialization than closing and reopening the encoder, or even
> simply on-the-fly changes of trivial values that would not require any kind
> or flushing or reinitialization by the encoder.
> 
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---

I don't really see the big improvement over closing+reopening the encoder.

>  libavcodec/codec_internal.h |  2 ++
>  libavcodec/encode.c         | 69 +++++++++++++++++++++++++++++++++++++
>  2 files changed, 71 insertions(+)
> 
> diff --git a/libavcodec/codec_internal.h b/libavcodec/codec_internal.h
> index 5b2db74590..e38402d2f5 100644
> --- a/libavcodec/codec_internal.h
> +++ b/libavcodec/codec_internal.h
> @@ -243,6 +243,8 @@ typedef struct FFCodec {
>       */
>      void (*flush)(struct AVCodecContext *);
>  
> +    int (*reconf)(struct AVCodecContext *);
> +
>      /**
>       * Decoding only, a comma-separated list of bitstream filters to apply to
>       * packets before decoding.
> diff --git a/libavcodec/encode.c b/libavcodec/encode.c
> index 3baf5b8103..187b4015f1 100644
> --- a/libavcodec/encode.c
> +++ b/libavcodec/encode.c
> @@ -31,6 +31,7 @@
>  
>  #include "avcodec.h"
>  #include "avcodec_internal.h"
> +#include "bytestream.h"
>  #include "codec_desc.h"
>  #include "codec_internal.h"
>  #include "encode.h"
> @@ -59,6 +60,69 @@ static EncodeContext *encode_ctx(AVCodecInternal *avci)
>      return (EncodeContext*)avci;
>  }
>  
> +static int apply_param_change(AVCodecContext *avctx, const AVFrame *frame)
> +{
> +    int ret = AVERROR_BUG;
> +    const AVFrameSideData *sd;
> +    GetByteContext gbc;
> +    uint32_t flags;
> +
> +    sd = av_frame_get_side_data(frame, AV_FRAME_DATA_PARAM_CHANGE);
> +    if (!sd)
> +        return 0;
> +
> +    if (!(avctx->codec->capabilities & AV_CODEC_CAP_PARAM_CHANGE)) {
> +        av_log(avctx, AV_LOG_ERROR, "This encoder does not support parameter "
> +               "changes, but PARAM_CHANGE side data was sent to it.\n");
> +        ret = AVERROR(EINVAL);
> +        goto fail2;
> +    }
> +
> +    if (sd->size < 4)
> +        goto fail;
> +
> +    bytestream2_init(&gbc, sd->data, sd->size);
> +    flags = bytestream2_get_le32(&gbc);
> +
> +    if (flags & AV_SIDE_DATA_PARAM_CHANGE_SAMPLE_RATE) {
> +        if (frame->sample_rate <= 0 || frame->sample_rate > INT_MAX) {
> +            av_log(avctx, AV_LOG_ERROR, "Invalid sample rate");
> +            ret = AVERROR_INVALIDDATA;
> +            goto fail2;
> +        }
> +        avctx->sample_rate = frame->sample_rate;
> +    }
> +    if (flags & AV_SIDE_DATA_PARAM_CHANGE_DIMENSIONS) {
> +        av_image_check_size2(frame->width, frame->height, avctx->max_pixels, avctx->pix_fmt, 0, avctx);
> +        if (ret < 0) {
> +            av_log(avctx, AV_LOG_ERROR, "Invalid dimensions");
> +            goto fail2;
> +        }
> +        avctx->width  = frame->width;
> +        avctx->height = frame->height;
> +    }
> +
> +    if (flags) {
> +        ret = 0;
> +        if (ffcodec(avctx->codec)->reconf)
> +            ret = ffcodec(avctx->codec)->reconf(avctx);
> +        if (ret < 0)
> +            goto fail2;
> +    }
> +
> +    return 0;
> +fail:
> +    av_log(avctx, AV_LOG_ERROR, "PARAM_CHANGE side data too small.\n");
> +    ret = AVERROR_INVALIDDATA;
> +fail2:
> +    if (ret < 0) {
> +        av_log(avctx, AV_LOG_ERROR, "Error applying parameter changes.\n");
> +        if (avctx->err_recognition & AV_EF_EXPLODE)
> +            return ret;
> +    }
> +    return 0;
> +}
> +
>  int ff_alloc_packet(AVCodecContext *avctx, AVPacket *avpkt, int64_t size)
>  {
>      if (size < 0 || size > INT_MAX - AV_INPUT_BUFFER_PADDING_SIZE) {
> @@ -205,6 +269,7 @@ int avcodec_encode_subtitle(AVCodecContext *avctx, uint8_t *buf, int buf_size,
>  int ff_encode_get_frame(AVCodecContext *avctx, AVFrame *frame)
>  {
>      AVCodecInternal *avci = avctx->internal;
> +    int ret;
>  
>      if (avci->draining)
>          return AVERROR_EOF;
> @@ -229,6 +294,10 @@ FF_DISABLE_DEPRECATION_WARNINGS
>  FF_ENABLE_DEPRECATION_WARNINGS
>  #endif
>  
> +    ret = apply_param_change(avctx, frame);
> +    if (ret < 0)
> +        return ret;
> +
>      return 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".

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [FFmpeg-devel] [PATCH 4/7] avcodec/encode: add support for param change frame side data
  2025-01-27 12:48   ` Andreas Rheinhardt
@ 2025-01-27 12:54     ` James Almer
  0 siblings, 0 replies; 21+ messages in thread
From: James Almer @ 2025-01-27 12:54 UTC (permalink / raw)
  To: ffmpeg-devel


[-- Attachment #1.1.1: Type: text/plain, Size: 949 bytes --]

On 1/27/2025 9:48 AM, Andreas Rheinhardt wrote:
> James Almer:
>> With this, callers can signal supported encoders to reinitialize using certain
>> parameters, which should allow for more graceful (but potentially more limited
>> in scope) reinitialization than closing and reopening the encoder, or even
>> simply on-the-fly changes of trivial values that would not require any kind
>> or flushing or reinitialization by the encoder.
>>
>> Signed-off-by: James Almer <jamrial@gmail.com>
>> ---
> 
> I don't really see the big improvement over closing+reopening the encoder.

libx264 and libaom for example provides x264_encoder_reconfig() and 
aom_codec_enc_config_set() respectively, which lets you change some 
parameters on the fly, which although limited in scope (Neither will 
accept dimension changes, even if within the "max" set during init) is 
much more efficient compression-wise than closing+reopening the encoder.


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 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".

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [FFmpeg-devel] [PATCH 5/7] avcodec/encode: add a new param change side data value that takes a dictionary
  2025-01-22  2:54 ` [FFmpeg-devel] [PATCH 5/7] avcodec/encode: add a new param change side data value that takes a dictionary James Almer
@ 2025-01-27 13:26   ` Andreas Rheinhardt
  2025-01-27 13:33     ` James Almer
  0 siblings, 1 reply; 21+ messages in thread
From: Andreas Rheinhardt @ 2025-01-27 13:26 UTC (permalink / raw)
  To: ffmpeg-devel

James Almer:
> Allow the caller to pass a dictionary with key/value pairs to set encoder
> private options for reinitialization.
> AVCodecContext global options are not affected. For that, specific
> AVSideDataParamChangeFlags should be used, as is currently the case for
> dimensions and sample rate.
> 
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
>  libavcodec/encode.c | 21 +++++++++++++++++++++
>  libavutil/defs.h    |  1 +
>  2 files changed, 22 insertions(+)
> 
> diff --git a/libavcodec/encode.c b/libavcodec/encode.c
> index 187b4015f1..c14ab65509 100644
> --- a/libavcodec/encode.c
> +++ b/libavcodec/encode.c
> @@ -20,12 +20,14 @@
>  
>  #include "libavutil/attributes.h"
>  #include "libavutil/avassert.h"
> +#include "libavutil/avstring.h"
>  #include "libavutil/channel_layout.h"
>  #include "libavutil/emms.h"
>  #include "libavutil/frame.h"
>  #include "libavutil/imgutils.h"
>  #include "libavutil/internal.h"
>  #include "libavutil/mem.h"
> +#include "libavutil/opt.h"
>  #include "libavutil/pixdesc.h"
>  #include "libavutil/samplefmt.h"
>  
> @@ -101,6 +103,25 @@ static int apply_param_change(AVCodecContext *avctx, const AVFrame *frame)
>          avctx->width  = frame->width;
>          avctx->height = frame->height;
>      }
> +    if (flags & AV_SIDE_DATA_PARAM_CHANGE_DICT) {
> +        AVDictionary *dict = NULL;
> +        int left = av_strnlen(gbc.buffer, bytestream2_get_bytes_left(&gbc));
> +        if (!left)> +            goto fail;
> +        ret = av_dict_parse_string(&dict, gbc.buffer, "=", ":", 0);
> +        if (ret < 0) {
> +            av_log(avctx, AV_LOG_ERROR, "Invalid string");
> +            goto fail2;

Potential leak of dict here and below.

> +        }
> +        bytestream2_skip(&gbc, left + 1);
> +
> +        if (avctx->codec->priv_class) {
> +            ret = av_opt_set_dict(avctx->priv_data, &dict);
> +            if (ret < 0)
> +                goto fail2;

1. You are simply setting the options directly, without letting the
codec know what the changed options are. This might make implementing
this easy for the users you add in this patchset, but this may come back
to bite us in the future.
2. What happens in case of error? The encoder state may be inconsistent.
Is the user allowed to send more frames or is the user just supposed to
free this AVCodecContext?

> +        }
> +        av_dict_free(&dict);
> +    }
>  
>      if (flags) {
>          ret = 0;
> diff --git a/libavutil/defs.h b/libavutil/defs.h
> index f09fb5efd2..e1de680f2f 100644
> --- a/libavutil/defs.h
> +++ b/libavutil/defs.h
> @@ -125,6 +125,7 @@ enum AVMediaType {
>  enum AVSideDataParamChangeFlags {
>      AV_SIDE_DATA_PARAM_CHANGE_SAMPLE_RATE    = 0x0004,
>      AV_SIDE_DATA_PARAM_CHANGE_DIMENSIONS     = 0x0008,
> +    AV_SIDE_DATA_PARAM_CHANGE_DICT           = 0x0016,

This does not explain the layout of this 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".

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [FFmpeg-devel] [PATCH 5/7] avcodec/encode: add a new param change side data value that takes a dictionary
  2025-01-27 13:26   ` Andreas Rheinhardt
@ 2025-01-27 13:33     ` James Almer
  0 siblings, 0 replies; 21+ messages in thread
From: James Almer @ 2025-01-27 13:33 UTC (permalink / raw)
  To: ffmpeg-devel


[-- Attachment #1.1.1: Type: text/plain, Size: 3513 bytes --]

On 1/27/2025 10:26 AM, Andreas Rheinhardt wrote:
> James Almer:
>> Allow the caller to pass a dictionary with key/value pairs to set encoder
>> private options for reinitialization.
>> AVCodecContext global options are not affected. For that, specific
>> AVSideDataParamChangeFlags should be used, as is currently the case for
>> dimensions and sample rate.
>>
>> Signed-off-by: James Almer <jamrial@gmail.com>
>> ---
>>   libavcodec/encode.c | 21 +++++++++++++++++++++
>>   libavutil/defs.h    |  1 +
>>   2 files changed, 22 insertions(+)
>>
>> diff --git a/libavcodec/encode.c b/libavcodec/encode.c
>> index 187b4015f1..c14ab65509 100644
>> --- a/libavcodec/encode.c
>> +++ b/libavcodec/encode.c
>> @@ -20,12 +20,14 @@
>>   
>>   #include "libavutil/attributes.h"
>>   #include "libavutil/avassert.h"
>> +#include "libavutil/avstring.h"
>>   #include "libavutil/channel_layout.h"
>>   #include "libavutil/emms.h"
>>   #include "libavutil/frame.h"
>>   #include "libavutil/imgutils.h"
>>   #include "libavutil/internal.h"
>>   #include "libavutil/mem.h"
>> +#include "libavutil/opt.h"
>>   #include "libavutil/pixdesc.h"
>>   #include "libavutil/samplefmt.h"
>>   
>> @@ -101,6 +103,25 @@ static int apply_param_change(AVCodecContext *avctx, const AVFrame *frame)
>>           avctx->width  = frame->width;
>>           avctx->height = frame->height;
>>       }
>> +    if (flags & AV_SIDE_DATA_PARAM_CHANGE_DICT) {
>> +        AVDictionary *dict = NULL;
>> +        int left = av_strnlen(gbc.buffer, bytestream2_get_bytes_left(&gbc));
>> +        if (!left)> +            goto fail;
>> +        ret = av_dict_parse_string(&dict, gbc.buffer, "=", ":", 0);
>> +        if (ret < 0) {
>> +            av_log(avctx, AV_LOG_ERROR, "Invalid string");
>> +            goto fail2;
> 
> Potential leak of dict here and below.
> 
>> +        }
>> +        bytestream2_skip(&gbc, left + 1);
>> +
>> +        if (avctx->codec->priv_class) {
>> +            ret = av_opt_set_dict(avctx->priv_data, &dict);
>> +            if (ret < 0)
>> +                goto fail2;
> 
> 1. You are simply setting the options directly, without letting the
> codec know what the changed options are. This might make implementing
> this easy for the users you add in this patchset, but this may come back
> to bite us in the future.

I can make the dict an argument to FFCodec.reconf() to let the encoder 
handle it if you prefer.

> 2. What happens in case of error? The encoder state may be inconsistent.
> Is the user allowed to send more frames or is the user just supposed to
> free this AVCodecContext?
> 
>> +        }
>> +        av_dict_free(&dict);
>> +    }
>>   
>>       if (flags) {
>>           ret = 0;
>> diff --git a/libavutil/defs.h b/libavutil/defs.h
>> index f09fb5efd2..e1de680f2f 100644
>> --- a/libavutil/defs.h
>> +++ b/libavutil/defs.h
>> @@ -125,6 +125,7 @@ enum AVMediaType {
>>   enum AVSideDataParamChangeFlags {
>>       AV_SIDE_DATA_PARAM_CHANGE_SAMPLE_RATE    = 0x0004,
>>       AV_SIDE_DATA_PARAM_CHANGE_DIMENSIONS     = 0x0008,
>> +    AV_SIDE_DATA_PARAM_CHANGE_DICT           = 0x0016,
> 
> This does not explain the layout of this side data.

That's done in the side data type entry, like the two other flags.

fwiw: This approach (reusing the host-agnostic bitstream param change 
side data implementation from AVPacketSideData) was disliked by Lynne 
and Nicolas, so i sent a different one in a new patchset.


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 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".

^ permalink raw reply	[flat|nested] 21+ messages in thread

end of thread, other threads:[~2025-01-27 13:33 UTC | newest]

Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2025-01-22  2:53 [FFmpeg-devel] [PATCH 1/7] avutil/avutil: move some definitions to a new header James Almer
2025-01-22  2:53 ` [FFmpeg-devel] [PATCH 2/7] avcodec/packet: move AVSideDataParamChangeFlags to libavutil/defs.h James Almer
2025-01-22  2:53 ` [FFmpeg-devel] [PATCH 3/7] avutil/frame: add a param change side data type James Almer
2025-01-22  3:13   ` Lynne
2025-01-22 13:40     ` James Almer
2025-01-22 19:49       ` Nicolas George
2025-01-22 19:58         ` James Almer
2025-01-22 20:18           ` Nicolas George
2025-01-22 20:52             ` James Almer
2025-01-22 21:14               ` Nicolas George
2025-01-22 21:21                 ` Nicolas George
2025-01-23 21:49             ` Alexander Strasser via ffmpeg-devel
2025-01-22  2:54 ` [FFmpeg-devel] [PATCH 4/7] avcodec/encode: add support for param change frame side data James Almer
2025-01-27 12:48   ` Andreas Rheinhardt
2025-01-27 12:54     ` James Almer
2025-01-22  2:54 ` [FFmpeg-devel] [PATCH 5/7] avcodec/encode: add a new param change side data value that takes a dictionary James Almer
2025-01-27 13:26   ` Andreas Rheinhardt
2025-01-27 13:33     ` James Almer
2025-01-22  2:54 ` [FFmpeg-devel] [PATCH 6/7] avcodec/libaomenc: add support for param change frame side data James Almer
2025-01-22  2:54 ` [FFmpeg-devel] [PATCH 7/7] avcodec/libx264: " James Almer
2025-01-27 12:43 ` [FFmpeg-devel] [PATCH 1/7] avutil/avutil: move some definitions to a new header Andreas Rheinhardt

Git Inbox Mirror of the ffmpeg-devel mailing list - see https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://master.gitmailbox.com/ffmpegdev/0 ffmpegdev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 ffmpegdev ffmpegdev/ https://master.gitmailbox.com/ffmpegdev \
		ffmpegdev@gitmailbox.com
	public-inbox-index ffmpegdev

Example config snippet for mirrors.


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git