[FFmpeg-devel] [PR] fftools/textformat/avtextformat: Separate mutable and immutable data (PR #21372)

[FFmpeg-devel] [PR] fftools/textformat/avtextformat: Separate mutable and immutable data (PR #21372)

[mkver via ffmpeg-devel]

PR #21372 opened by mkver
URL: https://code.ffmpeg.org/FFmpeg/FFmpeg/pulls/21372
Patch URL: https://code.ffmpeg.org/FFmpeg/FFmpeg/pulls/21372.patch

(Part of this PR has already been sent to the ML before: See https://ffmpeg.org/pipermail/ffmpeg-devel/2025-June/344435.html)


>From e6c88d12ac3ee7a8795f76695658d32396648749 Mon Sep 17 00:00:00 2001
From: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
Date: Sun, 1 Jun 2025 00:13:54 +0200
Subject: [PATCH 1/4] fftools/textformat/avtextformat: Separate mutable and
 immutable data

Only two fields of AVTextFormatSection are ever modified:
entries_to_show and show_all_entries (they are only used
by ffprobe; the graph printing code always prints everything).
These fields do not belong into AVTextFormatSection, they are
more ffprobe-internal (and if the graph printing code ever
made use of them, these fields could very well be
per GraphPrintContext).

This commit therefore moves them out of AVTextFormatSection
and adds a callback to AVTextFormatContext to decide which
elements to discard. This also allows to make the AVTextFormatSections
const.

This also fixes a race when initializing the sections
for graphprint.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 fftools/ffprobe.c                 | 38 +++++++++++++++++++++++--------
 fftools/graph/graphprint.c        |  9 +-------
 fftools/textformat/avtextformat.c |  9 +++-----
 fftools/textformat/avtextformat.h | 13 +++++++++--
 4 files changed, 44 insertions(+), 25 deletions(-)

diff --git a/fftools/ffprobe.c b/fftools/ffprobe.c
index ddd5654796..814e591e3c 100644
--- a/fftools/ffprobe.c
+++ b/fftools/ffprobe.c
@@ -252,7 +252,7 @@ static const char *get_stream_group_type(const void *data)
     return av_x_if_null(avformat_stream_group_name(stg->type), "unknown");
 }
 
-static struct AVTextFormatSection sections[] = {
+static const AVTextFormatSection sections[] = {
     [SECTION_ID_CHAPTERS] =           { SECTION_ID_CHAPTERS, "chapters", AV_TEXTFORMAT_SECTION_FLAG_IS_ARRAY, { SECTION_ID_CHAPTER, -1 } },
     [SECTION_ID_CHAPTER] =            { SECTION_ID_CHAPTER, "chapter", 0, { SECTION_ID_CHAPTER_TAGS, -1 } },
     [SECTION_ID_CHAPTER_TAGS] =       { SECTION_ID_CHAPTER_TAGS, "tags", AV_TEXTFORMAT_SECTION_FLAG_HAS_VARIABLE_FIELDS, { -1 }, .element_name = "tag", .unique_name = "chapter_tags" },
@@ -324,6 +324,13 @@ static struct AVTextFormatSection sections[] = {
     [SECTION_ID_SUBTITLE] =           { SECTION_ID_SUBTITLE, "subtitle", 0, { -1 } },
 };
 
+typedef struct EntrySelection {
+    int show_all_entries;
+    AVDictionary *entries_to_show;
+} EntrySelection;
+
+static EntrySelection selected_entries[FF_ARRAY_ELEMS(sections)] = { 0 };
+
 static const OptionDef *options;
 
 /* FFprobe context */
@@ -358,6 +365,14 @@ typedef struct LogBuffer {
 static LogBuffer *log_buffer;
 static int log_buffer_size;
 
+static int is_key_selected_callback(AVTextFormatContext *tctx, const char *key)
+{
+    const AVTextFormatSection *section = tctx->section[tctx->level];
+    const EntrySelection *selection = &selected_entries[section - sections];
+
+    return selection->show_all_entries || av_dict_get(selection->entries_to_show, key, NULL, 0);
+}
+
 static void log_callback(void *ptr, int level, const char *fmt, va_list vl)
 {
     AVClass* avc = ptr ? *(AVClass **) ptr : NULL;
@@ -2737,14 +2752,15 @@ static int opt_format(void *optctx, const char *opt, const char *arg)
 static inline void mark_section_show_entries(SectionID section_id,
                                              int show_all_entries, AVDictionary *entries)
 {
-    struct AVTextFormatSection *section = &sections[section_id];
+    EntrySelection *selection = &selected_entries[section_id];
 
-    section->show_all_entries = show_all_entries;
+    selection->show_all_entries = show_all_entries;
     if (show_all_entries) {
+        const AVTextFormatSection *section = &sections[section_id];
         for (const int *id = section->children_ids; *id != -1; id++)
             mark_section_show_entries(*id, show_all_entries, entries);
     } else {
-        av_dict_copy(&section->entries_to_show, entries, 0);
+        av_dict_copy(&selection->entries_to_show, entries, 0);
     }
 }
 
@@ -3167,9 +3183,12 @@ static const OptionDef real_options[] = {
 
 static inline int check_section_show_entries(int section_id)
 {
-    struct AVTextFormatSection *section = &sections[section_id];
-    if (sections[section_id].show_all_entries || sections[section_id].entries_to_show)
+    const EntrySelection *selection = &selected_entries[section_id];
+
+    if (selection->show_all_entries || selection->entries_to_show)
         return 1;
+
+    const AVTextFormatSection *section = &sections[section_id];
     for (const int *id = section->children_ids; *id != -1; id++)
         if (check_section_show_entries(*id))
             return 1;
@@ -3188,7 +3207,7 @@ int main(int argc, char **argv)
     AVTextWriterContext *wctx;
     char *buf;
     char *f_name = NULL, *f_args = NULL;
-    int ret, input_ret, i;
+    int ret, input_ret;
 
     init_dynload();
 
@@ -3282,6 +3301,7 @@ int main(int argc, char **argv)
         goto end;
 
     AVTextFormatOptions tf_options = {
+        .is_key_selected              = is_key_selected_callback,
         .show_optional_fields = show_optional_fields,
         .show_value_unit = show_value_unit,
         .use_value_prefix = use_value_prefix,
@@ -3338,8 +3358,8 @@ end:
     av_freep(&read_intervals);
 
     uninit_opts();
-    for (i = 0; i < FF_ARRAY_ELEMS(sections); i++)
-        av_dict_free(&(sections[i].entries_to_show));
+    for (size_t i = 0; i < FF_ARRAY_ELEMS(selected_entries); ++i)
+        av_dict_free(&selected_entries[i].entries_to_show);
 
     avformat_network_deinit();
 
diff --git a/fftools/graph/graphprint.c b/fftools/graph/graphprint.c
index 242eaf8ba1..502632acb3 100644
--- a/fftools/graph/graphprint.c
+++ b/fftools/graph/graphprint.c
@@ -74,7 +74,7 @@ typedef enum {
     SECTION_ID_ENCODER,
 } SectionID;
 
-static struct AVTextFormatSection sections[] = {
+static const AVTextFormatSection sections[] = {
     [SECTION_ID_ROOT]            = { SECTION_ID_ROOT, "root", AV_TEXTFORMAT_SECTION_FLAG_IS_WRAPPER, { SECTION_ID_FILTERGRAPHS, SECTION_ID_INPUTFILES, SECTION_ID_OUTPUTFILES, SECTION_ID_DECODERS, SECTION_ID_ENCODERS, SECTION_ID_STREAMLINKS, -1 } },
 
     [SECTION_ID_FILTERGRAPHS]    = { SECTION_ID_FILTERGRAPHS, "graphs", AV_TEXTFORMAT_SECTION_FLAG_IS_ARRAY, { SECTION_ID_FILTERGRAPH, -1 } },
@@ -470,12 +470,6 @@ static void print_filter(GraphPrintContext *gpc, const AVFilterContext *filter,
     avtext_print_section_footer(tfc); // SECTION_ID_FILTER
 }
 
-static void init_sections(void)
-{
-    for (unsigned i = 0; i < FF_ARRAY_ELEMS(sections); i++)
-        sections[i].show_all_entries = 1;
-}
-
 static void print_filtergraph_single(GraphPrintContext *gpc, FilterGraph *fg, AVFilterGraph *graph)
 {
     AVTextFormatContext *tfc = gpc->tfc;
@@ -876,7 +870,6 @@ static int init_graphprint(GraphPrintContext **pgpc, AVBPrint *target_buf)
     GraphPrintContext *gpc = NULL;
     int ret;
 
-    init_sections();
     *pgpc = NULL;
 
     av_bprint_init(target_buf, 0, AV_BPRINT_SIZE_UNLIMITED);
diff --git a/fftools/textformat/avtextformat.c b/fftools/textformat/avtextformat.c
index 651a84578c..ac067a2045 100644
--- a/fftools/textformat/avtextformat.c
+++ b/fftools/textformat/avtextformat.c
@@ -147,6 +147,7 @@ int avtext_context_open(AVTextFormatContext **ptctx, const AVTextFormatter *form
         goto fail;
     }
 
+    tctx->is_key_selected = options.is_key_selected;
     tctx->show_value_unit = options.show_value_unit;
     tctx->use_value_prefix = options.use_value_prefix;
     tctx->use_byte_value_binary_prefix = options.use_byte_value_binary_prefix;
@@ -289,8 +290,6 @@ void avtext_print_section_footer(AVTextFormatContext *tctx)
 
 void avtext_print_integer(AVTextFormatContext *tctx, const char *key, int64_t val, int flags)
 {
-    const AVTextFormatSection *section;
-
     av_assert0(tctx);
 
     if (tctx->show_optional_fields == SHOW_OPTIONAL_FIELDS_NEVER)
@@ -303,9 +302,7 @@ void avtext_print_integer(AVTextFormatContext *tctx, const char *key, int64_t va
 
     av_assert0(key && tctx->level >= 0 && tctx->level < SECTION_MAX_NB_LEVELS);
 
-    section = tctx->section[tctx->level];
-
-    if (section->show_all_entries || av_dict_get(section->entries_to_show, key, NULL, 0)) {
+    if (!tctx->is_key_selected || tctx->is_key_selected(tctx, key)) {
         tctx->formatter->print_integer(tctx, key, val);
         tctx->nb_item[tctx->level]++;
     }
@@ -460,7 +457,7 @@ int avtext_print_string(AVTextFormatContext *tctx, const char *key, const char *
         && !(tctx->formatter->flags & AV_TEXTFORMAT_FLAG_SUPPORTS_OPTIONAL_FIELDS))
         return 0;
 
-    if (section->show_all_entries || av_dict_get(section->entries_to_show, key, NULL, 0)) {
+    if (!tctx->is_key_selected || tctx->is_key_selected(tctx, key)) {
         if (flags & AV_TEXTFORMAT_PRINT_STRING_VALIDATE) {
             char *key1 = NULL, *val1 = NULL;
             ret = validate_string(tctx, &key1, key);
diff --git a/fftools/textformat/avtextformat.h b/fftools/textformat/avtextformat.h
index d9c14069eb..33b06ca360 100644
--- a/fftools/textformat/avtextformat.h
+++ b/fftools/textformat/avtextformat.h
@@ -57,9 +57,7 @@ typedef struct AVTextFormatSection {
     const int children_ids[SECTION_MAX_NB_CHILDREN + 1]; ///< list of children section IDS, terminated by -1
     const char *element_name; ///< name of the contained element, if provided
     const char *unique_name;  ///< unique section name, in case the name is ambiguous
-    AVDictionary *entries_to_show;
     const char *(*get_type)(const void *data); ///< function returning a type if defined, must be defined when SECTION_FLAG_HAS_TYPE is defined
-    int show_all_entries;
     const char *id_key;          ///< name of the key to be used as the id
     const char *src_id_key;     ///< name of the key to be used as the source id for diagram connections
     const char *dest_id_key;   ///< name of the key to be used as the target id for diagram connections
@@ -131,6 +129,16 @@ struct AVTextFormatContext {
     AVBPrint section_pbuf[SECTION_MAX_NB_LEVELS]; ///< generic print buffer dedicated to each section,
                                                   ///  used by various formatters
 
+    /**
+     * Callback to discard certain elements based upon the key used.
+     * It is called before any element with a key is printed.
+     * If this callback is unset, all elements are printed.
+     *
+     * @retval 1 if the element is supposed to be printed
+     * @retval 0 if the element is supposed to be discarded
+     */
+    int (*is_key_selected)(struct AVTextFormatContext *tctx, const char *key);
+
     int show_optional_fields;
     int show_value_unit;
     int use_value_prefix;
@@ -145,6 +153,7 @@ struct AVTextFormatContext {
 };
 
 typedef struct AVTextFormatOptions {
+    int (*is_key_selected)(struct AVTextFormatContext *tctx, const char *key);
     int show_optional_fields;
     int show_value_unit;
     int use_value_prefix;
-- 
2.49.1


>From dc1d0edf636359bcee82cbc651fb5ea74cf3573a Mon Sep 17 00:00:00 2001
From: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
Date: Sun, 1 Jun 2025 05:09:32 +0200
Subject: [PATCH 2/4] fftools/textformat/avtextformat: Move
 avtext_print_integers to ffprobe.c

This is its only user and because this function is so specialised
it is very likely to stay that way. So move it back to ffprobe.c
(where it already was before d7a3f68feae0b1c3718f9d2671c6d41c60a40680).

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 fftools/ffprobe.c                 | 26 +++++++++++++++++++++++++-
 fftools/textformat/avtextformat.c | 28 ----------------------------
 fftools/textformat/avtextformat.h |  3 ---
 3 files changed, 25 insertions(+), 32 deletions(-)

diff --git a/fftools/ffprobe.c b/fftools/ffprobe.c
index 814e591e3c..67111bc31f 100644
--- a/fftools/ffprobe.c
+++ b/fftools/ffprobe.c
@@ -452,6 +452,30 @@ static void log_callback(void *ptr, int level, const char *fmt, va_list vl)
 #define print_duration_ts(k, v)       avtext_print_ts(tfc, k, v, 1)
 #define print_val(k, v, u)      avtext_print_unit_integer(tfc, k, v, u)
 
+static void print_integers(AVTextFormatContext *tfc, const char *key,
+                           const void *data, int size, const char *format,
+                           int columns, int bytes, int offset_add)
+{
+    AVBPrint bp;
+    unsigned offset = 0;
+
+    av_bprint_init(&bp, 0, AV_BPRINT_SIZE_AUTOMATIC);
+    av_bprint_chars(&bp, '\n', 1);
+    while (size) {
+        av_bprintf(&bp, "%08x: ", offset);
+        for (int i = 0, l = FFMIN(size, columns); i < l; i++) {
+            if      (bytes == 1) av_bprintf(&bp, format, *(const uint8_t*)data);
+            else if (bytes == 2) av_bprintf(&bp, format, AV_RN16(data));
+            else if (bytes == 4) av_bprintf(&bp, format, AV_RN32(data));
+            data = (const char*)data + bytes;
+            size--;
+        }
+        av_bprint_chars(&bp, '\n', 1);
+        offset += offset_add;
+    }
+    avtext_print_string(tfc, key, bp.str, 0);
+}
+
 #define REALLOCZ_ARRAY_STREAM(ptr, cur_n, new_n)                        \
 {                                                                       \
     ret = av_reallocp_array(&(ptr), (new_n), sizeof(*(ptr)));           \
@@ -483,7 +507,7 @@ static void print_displaymatrix(AVTextFormatContext *tfc, const int32_t matrix[9
     double rotation = av_display_rotation_get(matrix);
     if (isnan(rotation))
         rotation = 0;
-    avtext_print_integers(tfc, "displaymatrix", (void*)matrix, 9, " %11d", 3, 4, 1);
+    print_integers(tfc, "displaymatrix", matrix, 9, " %11d", 3, 4, 1);
     print_int("rotation", rotation);
 }
 
diff --git a/fftools/textformat/avtextformat.c b/fftools/textformat/avtextformat.c
index ac067a2045..04da8e7d3a 100644
--- a/fftools/textformat/avtextformat.c
+++ b/fftools/textformat/avtextformat.c
@@ -558,34 +558,6 @@ void avtext_print_data_hash(AVTextFormatContext *tctx, const char *key,
     avtext_print_string(tctx, key, buf, 0);
 }
 
-void avtext_print_integers(AVTextFormatContext *tctx, const char *key,
-                           uint8_t *data, int size, const char *format,
-                           int columns, int bytes, int offset_add)
-{
-    AVBPrint bp;
-    unsigned offset = 0;
-
-    if (!key || !data || !format || columns <= 0 || bytes <= 0)
-        return;
-
-    av_bprint_init(&bp, 0, AV_BPRINT_SIZE_UNLIMITED);
-    av_bprintf(&bp, "\n");
-    while (size) {
-        av_bprintf(&bp, "%08x: ", offset);
-        for (int i = 0, l = FFMIN(size, columns); i < l; i++) {
-            if      (bytes == 1) av_bprintf(&bp, format, *data);
-            else if (bytes == 2) av_bprintf(&bp, format, AV_RN16(data));
-            else if (bytes == 4) av_bprintf(&bp, format, AV_RN32(data));
-            data += bytes;
-            size--;
-        }
-        av_bprintf(&bp, "\n");
-        offset += offset_add;
-    }
-    avtext_print_string(tctx, key, bp.str, 0);
-    av_bprint_finalize(&bp, NULL);
-}
-
 static const char *writercontext_get_writer_name(void *p)
 {
     AVTextWriterContext *wctx = p;
diff --git a/fftools/textformat/avtextformat.h b/fftools/textformat/avtextformat.h
index 33b06ca360..d3ed437b7e 100644
--- a/fftools/textformat/avtextformat.h
+++ b/fftools/textformat/avtextformat.h
@@ -190,9 +190,6 @@ void avtext_print_data(AVTextFormatContext *tctx, const char *key, const uint8_t
 
 void avtext_print_data_hash(AVTextFormatContext *tctx, const char *key, const uint8_t *data, int size);
 
-void avtext_print_integers(AVTextFormatContext *tctx, const char *key, uint8_t *data, int size,
-                           const char *format, int columns, int bytes, int offset_add);
-
 const AVTextFormatter *avtext_get_formatter_by_name(const char *name);
 
 extern const AVTextFormatter avtextformatter_default;
-- 
2.49.1


>From d4dd6c8c03410c5e6b96cf44330a8c923d6f091c Mon Sep 17 00:00:00 2001
From: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
Date: Sun, 4 Jan 2026 11:10:41 +0100
Subject: [PATCH 3/4] fftools/graph/graphprint: Remove always-false checks

init_graphprint() already returns an error upon allocation
failure.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 fftools/graph/graphprint.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/fftools/graph/graphprint.c b/fftools/graph/graphprint.c
index 502632acb3..26d2fd381f 100644
--- a/fftools/graph/graphprint.c
+++ b/fftools/graph/graphprint.c
@@ -963,11 +963,6 @@ int print_filtergraph(FilterGraph *fg, AVFilterGraph *graph)
     if (ret)
         return ret;
 
-    if (!gpc) {
-        av_log(NULL, AV_LOG_ERROR, "Failed to initialize graph print context\n");
-        return AVERROR(ENOMEM);
-    }
-
     tfc = gpc->tfc;
 
     // Due to the threading model each graph needs to print itself into a buffer
@@ -1004,11 +999,6 @@ static int print_filtergraphs_priv(FilterGraph **graphs, int nb_graphs, InputFil
     if (ret)
         goto cleanup;
 
-    if (!gpc) {
-        ret = AVERROR(ENOMEM);
-        goto cleanup;
-    }
-
     tfc = gpc->tfc;
 
     avtext_print_section_header(tfc, NULL, SECTION_ID_ROOT);
-- 
2.49.1


>From 2fa2d360620a90e8909f8362b1e37a67802d3887 Mon Sep 17 00:00:00 2001
From: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
Date: Sun, 4 Jan 2026 11:14:03 +0100
Subject: [PATCH 4/4] fftools/graph/graphprint: Replace always-false check by
 assert

This check makes no sense, as the pointer arithmetic involved
in &fg->graph_print_buf would be UB if fg were NULL.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 fftools/graph/graphprint.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/fftools/graph/graphprint.c b/fftools/graph/graphprint.c
index 26d2fd381f..9a8c4c4230 100644
--- a/fftools/graph/graphprint.c
+++ b/fftools/graph/graphprint.c
@@ -946,16 +946,13 @@ fail:
 
 int print_filtergraph(FilterGraph *fg, AVFilterGraph *graph)
 {
+    av_assert2(fg);
+
     GraphPrintContext *gpc = NULL;
     AVTextFormatContext *tfc;
     AVBPrint *target_buf = &fg->graph_print_buf;
     int ret;
 
-    if (!fg) {
-        av_log(NULL, AV_LOG_ERROR, "Invalid filter graph provided\n");
-        return AVERROR(EINVAL);
-    }
-
     if (target_buf->len)
         av_bprint_finalize(target_buf, NULL);
 
-- 
2.49.1

_______________________________________________
ffmpeg-devel mailing list -- ffmpeg-devel@ffmpeg.org
To unsubscribe send an email to ffmpeg-devel-leave@ffmpeg.org