diff mbox series

[FFmpeg-devel,2/2] avfilter/af_astats: Remove fraction part of integer metadata entries

Message ID 1616763487-10025-2-git-send-email-t.rapp@noa-archive.com
State New
Headers show
Series [FFmpeg-devel,1/2] avfilter/af_astats: Only print header lines when values are to be printed | expand

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate success Make fate finished

Commit Message

Tobias Rapp March 26, 2021, 12:58 p.m. UTC 
Using integer format makes the metadata display similar to the log output.

Fix missing "Overall" metadata key name prefix where necessary. Also
replace whitespace in some metadata key names with underscore character
for consistency.

Signed-off-by: Tobias Rapp <t.rapp@noa-archive.com>
---
 libavfilter/af_astats.c | 56 +++++++++++++++++++++++++++----------------------
 1 file changed, 31 insertions(+), 25 deletions(-)

Comments

Anton Khirnov April 8, 2021, 9:23 a.m. UTC | #1 
Quoting Tobias Rapp (2021-03-26 13:58:07)
> Using integer format makes the metadata display similar to the log output.
> 
> Fix missing "Overall" metadata key name prefix where necessary. Also
> replace whitespace in some metadata key names with underscore character
> for consistency.
> 
> Signed-off-by: Tobias Rapp <t.rapp@noa-archive.com>
> ---
>  libavfilter/af_astats.c | 56 +++++++++++++++++++++++++++----------------------
>  1 file changed, 31 insertions(+), 25 deletions(-)

Does this mean that there are no stability guarantees for metadata
exported by filters?
Nicolas George April 8, 2021, 9:34 a.m. UTC | #2 
Anton Khirnov (12021-04-08):
> Does this mean that there are no stability guarantees for metadata
> exported by filters?

We can have stability for the components that are good enough to be
stable, and no stability yet for components that need enhancing.

Regards,
Tobias Rapp April 9, 2021, 7:58 a.m. UTC | #3 
On 08.04.2021 11:34, Nicolas George wrote:
> Anton Khirnov (12021-04-08):
>> Does this mean that there are no stability guarantees for metadata
>> exported by filters?
> 
> We can have stability for the components that are good enough to be
> stable, and no stability yet for components that need enhancing.

Indeed I should at least increment the minor (or micro?) version for 
libavfilter. And for the metadata changes my goal was to make some 
obvious things like "lavfi.astats.Bit_depth=24.000000" less weird. I 
didn't dare to touch other things. As the astats filter metadata is not 
tested by FATE my guess was that stability is not yet a high priority.

Regards,
Tobias
Tobias Rapp April 16, 2021, 1:50 p.m. UTC | #4 
On 09.04.2021 09:58, Tobias Rapp wrote:
> On 08.04.2021 11:34, Nicolas George wrote:
>> Anton Khirnov (12021-04-08):
>>> Does this mean that there are no stability guarantees for metadata
>>> exported by filters?
>>
>> We can have stability for the components that are good enough to be
>> stable, and no stability yet for components that need enhancing.
> 
> Indeed I should at least increment the minor (or micro?) version for 
> libavfilter. And for the metadata changes my goal was to make some 
> obvious things like "lavfi.astats.Bit_depth=24.000000" less weird. I 
> didn't dare to touch other things. As the astats filter metadata is not 
> tested by FATE my guess was that stability is not yet a high priority.

BTW: After the change metadata key names match what is documented in
http://ffmpeg.org/ffmpeg-filters.html#astats-1 regarding underscores and 
the "Overall" prefix.

Have added an increment of the minor version in libavfilter/version.h 
locally. So I guess this patch should be fine now?

Regards,
Tobias
Anton Khirnov April 17, 2021, 1:23 p.m. UTC | #5 
Quoting Tobias Rapp (2021-04-16 15:50:44)
> On 09.04.2021 09:58, Tobias Rapp wrote:
> > On 08.04.2021 11:34, Nicolas George wrote:
> >> Anton Khirnov (12021-04-08):
> >>> Does this mean that there are no stability guarantees for metadata
> >>> exported by filters?
> >>
> >> We can have stability for the components that are good enough to be
> >> stable, and no stability yet for components that need enhancing.
> > 
> > Indeed I should at least increment the minor (or micro?) version for 
> > libavfilter. And for the metadata changes my goal was to make some 
> > obvious things like "lavfi.astats.Bit_depth=24.000000" less weird. I 
> > didn't dare to touch other things. As the astats filter metadata is not 
> > tested by FATE my guess was that stability is not yet a high priority.
> 
> BTW: After the change metadata key names match what is documented in
> http://ffmpeg.org/ffmpeg-filters.html#astats-1 regarding underscores and 
> the "Overall" prefix.
> 
> Have added an increment of the minor version in libavfilter/version.h 
> locally. So I guess this patch should be fine now?

If it makes thing more in line with documenation then I guess it's less
bad. But from the users' perspective it sure be nice to be able to tell
which of these things are guaranteed to remain stable.

And more generally I believe side data should be used for information
exported by filters, not metadata.

(do not take this as an objection to the patch, just general rambling)
diff mbox series

Patch

diff --git a/libavfilter/af_astats.c b/libavfilter/af_astats.c
index c13df62..3b248b9 100644
--- a/libavfilter/af_astats.c
+++ b/libavfilter/af_astats.c
@@ -390,18 +390,24 @@  static inline void update_double_stat(AudioStatsContext *s, ChannelStats *p, dou
     p->nb_denormals += type == FP_SUBNORMAL;
 }
 
-static void set_meta(AVDictionary **metadata, int chan, const char *key,
-                     const char *fmt, double val)
+static av_printf_format(4, 5) void set_meta(AVDictionary **metadata, int chan,
+        const char *key, const char *value, ...)
 {
-    uint8_t value[128];
-    uint8_t key2[128];
-
-    snprintf(value, sizeof(value), fmt, val);
-    if (chan)
-        snprintf(key2, sizeof(key2), "lavfi.astats.%d.%s", chan, key);
-    else
-        snprintf(key2, sizeof(key2), "lavfi.astats.%s", key);
-    av_dict_set(metadata, key2, value, 0);
+    va_list argument_list;
+
+    va_start(argument_list, value);
+    if (value) {
+        char keybuf[128];
+        char valbuf[128];
+        if (chan) {
+            snprintf(keybuf, sizeof(keybuf), "lavfi.astats.%d.%s", chan, key);
+        } else {
+            snprintf(keybuf, sizeof(keybuf), "lavfi.astats.%s", key);
+        }
+        vsnprintf(valbuf, sizeof(valbuf), value, argument_list);
+        av_dict_set(metadata, keybuf, valbuf, 0);
+    }
+    va_end(argument_list);
 }
 
 #define LINEAR_TO_DB(x) (log10(x) * 20)
@@ -484,28 +490,28 @@  static void set_metadata(AudioStatsContext *s, AVDictionary **metadata)
         if (s->measure_perchannel & MEASURE_FLAT_FACTOR)
             set_meta(metadata, c + 1, "Flat_factor", "%f", LINEAR_TO_DB((p->min_runs + p->max_runs) / (p->min_count + p->max_count)));
         if (s->measure_perchannel & MEASURE_PEAK_COUNT)
-            set_meta(metadata, c + 1, "Peak_count", "%f", (float)(p->min_count + p->max_count));
+            set_meta(metadata, c + 1, "Peak_count", "%"PRId64, p->min_count + p->max_count);
         if (s->measure_perchannel & MEASURE_NOISE_FLOOR)
             set_meta(metadata, c + 1, "Noise_floor", "%f", LINEAR_TO_DB(p->noise_floor));
         if (s->measure_perchannel & MEASURE_NOISE_FLOOR_COUNT)
-            set_meta(metadata, c + 1, "Noise_floor_count", "%f", p->noise_floor_count);
+            set_meta(metadata, c + 1, "Noise_floor_count", "%"PRId64, p->noise_floor_count);
         if (s->measure_perchannel & MEASURE_BIT_DEPTH) {
             bit_depth(s, p->mask, p->imask, &depth);
-            set_meta(metadata, c + 1, "Bit_depth", "%f", depth.num);
-            set_meta(metadata, c + 1, "Bit_depth2", "%f", depth.den);
+            set_meta(metadata, c + 1, "Bit_depth", "%u", depth.num);
+            set_meta(metadata, c + 1, "Bit_depth2", "%u", depth.den);
         }
         if (s->measure_perchannel & MEASURE_DYNAMIC_RANGE)
             set_meta(metadata, c + 1, "Dynamic_range", "%f", LINEAR_TO_DB(2 * FFMAX(FFABS(p->min), FFABS(p->max))/ p->min_non_zero));
         if (s->measure_perchannel & MEASURE_ZERO_CROSSINGS)
-            set_meta(metadata, c + 1, "Zero_crossings", "%f", p->zero_runs);
+            set_meta(metadata, c + 1, "Zero_crossings", "%"PRId64, p->zero_runs);
         if (s->measure_perchannel & MEASURE_ZERO_CROSSINGS_RATE)
             set_meta(metadata, c + 1, "Zero_crossings_rate", "%f", p->zero_runs/(double)p->nb_samples);
         if ((s->is_float || s->is_double) && s->measure_perchannel & MEASURE_NUMBER_OF_NANS)
-            set_meta(metadata, c + 1, "Number of NaNs", "%f", p->nb_nans);
+            set_meta(metadata, c + 1, "Number_of_NaNs", "%"PRId64, p->nb_nans);
         if ((s->is_float || s->is_double) && s->measure_perchannel & MEASURE_NUMBER_OF_INFS)
-            set_meta(metadata, c + 1, "Number of Infs", "%f", p->nb_infs);
+            set_meta(metadata, c + 1, "Number_of_Infs", "%"PRId64, p->nb_infs);
         if ((s->is_float || s->is_double) && s->measure_perchannel & MEASURE_NUMBER_OF_DENORMALS)
-            set_meta(metadata, c + 1, "Number of denormals", "%f", p->nb_denormals);
+            set_meta(metadata, c + 1, "Number_of_denormals", "%"PRId64, p->nb_denormals);
     }
 
     if (s->measure_overall & MEASURE_DC_OFFSET)
@@ -540,17 +546,17 @@  static void set_metadata(AudioStatsContext *s, AVDictionary **metadata)
         set_meta(metadata, 0, "Overall.Noise_floor_count", "%f", noise_floor_count / (double)s->nb_channels);
     if (s->measure_overall & MEASURE_BIT_DEPTH) {
         bit_depth(s, mask, imask, &depth);
-        set_meta(metadata, 0, "Overall.Bit_depth", "%f", depth.num);
-        set_meta(metadata, 0, "Overall.Bit_depth2", "%f", depth.den);
+        set_meta(metadata, 0, "Overall.Bit_depth", "%u", depth.num);
+        set_meta(metadata, 0, "Overall.Bit_depth2", "%u", depth.den);
     }
     if (s->measure_overall & MEASURE_NUMBER_OF_SAMPLES)
-        set_meta(metadata, 0, "Overall.Number_of_samples", "%f", nb_samples / s->nb_channels);
+        set_meta(metadata, 0, "Overall.Number_of_samples", "%"PRId64, nb_samples / s->nb_channels);
     if ((s->is_float || s->is_double) && s->measure_overall & MEASURE_NUMBER_OF_NANS)
-        set_meta(metadata, 0, "Number of NaNs", "%f", nb_nans / (float)s->nb_channels);
+        set_meta(metadata, 0, "Overall.Number_of_NaNs", "%f", nb_nans / (float)s->nb_channels);
     if ((s->is_float || s->is_double) && s->measure_overall & MEASURE_NUMBER_OF_INFS)
-        set_meta(metadata, 0, "Number of Infs", "%f", nb_infs / (float)s->nb_channels);
+        set_meta(metadata, 0, "Overall.Number_of_Infs", "%f", nb_infs / (float)s->nb_channels);
     if ((s->is_float || s->is_double) && s->measure_overall & MEASURE_NUMBER_OF_DENORMALS)
-        set_meta(metadata, 0, "Number of denormals", "%f", nb_denormals / (float)s->nb_channels);
+        set_meta(metadata, 0, "Overall.Number_of_denormals", "%f", nb_denormals / (float)s->nb_channels);
 }
 
 #define UPDATE_STATS_P(type, update_func, update_float, channel_func)           \