[FFmpeg-devel] [PATCH] libavutil/dict: extend the list of convienience functions for storing different data types
Nicolas George
george at nsup.org
Fri Sep 4 17:07:48 CEST 2015
L'octidi 18 fructidor, an CCXXIII, Kevin Wheatley a écrit :
> as part of adding support for non-string data types to .mov metadata,
> I wondered about adding the following helper functions for storing
> numeric types into an AVDictionary.
>
> upfront I'll say I'm not 100% happy with the float32 and float64 named
> variants (vs float and double) as there is no clear preference in
> other areas of the code that I could see.
These function deal with CPU-ready values, not with the values serialized as
octets in memory; therefore, I am in favour of using the type names: float /
double. There is no guarantee that float is 32 bits and double 64. On
x86_32, double is sometimes 64 sometimes 80 depending on the FPU used.
> From 34fedb67d58402b519a7c91bff7623469802c4c4 Mon Sep 17 00:00:00 2001
> From: Kevin Wheatley <kevin.j.wheatley at gmail.com>
> Date: Fri, 4 Sep 2015 14:26:49 +0100
> Subject: [PATCH] libavutil/dict: extend the list of convienience functions for storing different data types
>
>
> Signed-off-by: Kevin Wheatley <kevin.j.wheatley at gmail.com>
> ---
> libavutil/dict.c | 81 +++++++++++++++++++++++++++++++++++++++++++++++++-
> libavutil/dict.h | 24 +++++++++++++++
> tests/ref/fate/dict | 17 +++++++++++
> 3 files changed, 120 insertions(+), 2 deletions(-)
>
> diff --git a/libavutil/dict.c b/libavutil/dict.c
> index 6ff1af5..4eaaf83 100644
> --- a/libavutil/dict.c
> +++ b/libavutil/dict.c
> @@ -149,6 +149,33 @@ int av_dict_set_int(AVDictionary **pm, const char *key, int64_t value,
> return av_dict_set(pm, key, valuestr, flags);
> }
>
> +int av_dict_set_uint(AVDictionary **pm, const char *key, uint64_t value,
> + int flags)
> +{
> + char valuestr[22];
> + snprintf(valuestr, sizeof(valuestr), "%"PRIu64, value);
> + flags &= ~AV_DICT_DONT_STRDUP_VAL;
I do not like that, it feels like defensive programming. API users should
not give random flags to functions and expect them to work. Therefore, I
would rather have the documentation state "AV_DICT_DONT_STRDUP_VAL must not
be set", implying that setting it is an undefined behaviour.
(And just to be nice, av_assert0() that it is not set: failing an assert is
the epitome of undefined behaviour, but nicer to debug than memory
corruption.)
> + return av_dict_set(pm, key, valuestr, flags);
> +}
> +
> +int av_dict_set_float32(AVDictionary **pm, const char *key, float value,
> + int flags)
> +{
> + char valuestr[17]; // sign + 1 + point + 8 + e + sign + 3 + term
> + snprintf(valuestr, sizeof(valuestr), "%.9g", value);
> + flags &= ~AV_DICT_DONT_STRDUP_VAL;
> + return av_dict_set(pm, key, valuestr, flags);
> +}
> +
> +int av_dict_set_float64(AVDictionary **pm, const char *key, double value,
> + int flags)
> +{
> + char valuestr[26]; // sign + 1 + point + 16 + e + sign + 4 + term
> + snprintf(valuestr, sizeof(valuestr), "%.17g", value);
> + flags &= ~AV_DICT_DONT_STRDUP_VAL;
> + return av_dict_set(pm, key, valuestr, flags);
> +}
> +
> static int parse_key_value_pair(AVDictionary **pm, const char **buf,
> const char *key_val_sep, const char *pairs_sep,
> int flags)
> @@ -276,9 +303,13 @@ static void test_separators(const AVDictionary *m, const char pair, const char v
> int main(void)
> {
> AVDictionary *dict = NULL;
> - AVDictionaryEntry *e;
> + AVDictionaryEntry *e, *e2;
> char *buffer = NULL;
> -
> + float f32 = 0.0f;
> + double f64 = 0.0f;
> + union { uint32_t i; float f; } intfloat;
> + union { uint64_t ui; int64_t i; } un_signed_int;
What you are doing with these unions is highly non-portable and should be
avoided in the current code IMHO.
> +
> printf("Testing av_dict_get_string() and av_dict_parse_string()\n");
> av_dict_get_string(dict, &buffer, '=', ',');
> printf("%s\n", buffer);
> @@ -356,6 +387,52 @@ int main(void)
> printf("%s\n", e->value);
> av_dict_free(&dict);
>
> + printf("\nTesting av_dict_set_int()\n");
> + un_signed_int.ui = 0U;
> + av_dict_set_int(&dict, "0", un_signed_int.i, 0);
> + un_signed_int.ui = 0x8000000000000000UL;
> + av_dict_set_int(&dict, "-max", un_signed_int.i, 0);
> + un_signed_int.ui = 0x7fffffffffffffffUL;
> + av_dict_set_int(&dict, "max", un_signed_int.i, 0);
> + e = NULL;
> + while ((e = av_dict_get(dict, "", e, AV_DICT_IGNORE_SUFFIX)))
> + printf("%s %s\n", e->key, e->value);
> + av_dict_free(&dict);
> +
> + printf("\nTesting av_dict_set_uint()\n");
> + av_dict_set_uint(&dict, "0", 0, 0);
> + av_dict_set_uint(&dict, "max", 0xffffffffffffffffUL, 0);
> + e = NULL;
> + while ((e = av_dict_get(dict, "", e, AV_DICT_IGNORE_SUFFIX)))
> + printf("%s %s\n", e->key, e->value);
> + av_dict_free(&dict);
> +
> + printf("\nTesting av_dict_set_float*()\n");
> + // float and double representation of a given number should give the same value
> + f32 = -1.20786635e-05f;
> + f64 = -1.20786635e-05;
> + av_dict_set_float32(&dict, "f32", f32, 0);
> + e = av_dict_get(dict, "f32", NULL, 0);
> + printf("%.30g => %s\n", f32, e->value);
> + av_dict_set_float64(&dict, "f64", f64, 0);
> + e2 = av_dict_get(dict, "f64", NULL, 0);
> + printf("%.30g => %s\n", f64, e2->value);
> + printf("%s == %s %s\n", e->value, e2->value, strcmp(e->value, e2->value) == 0 ? "OK" : "BAD");
> +
> + intfloat.i = 0xa647609;
> + av_dict_set_float32(&dict, "0xa647609", intfloat.f, 0);
> + e = av_dict_get(dict, "0xa647609", NULL, 0);
> + printf("%.30g => %s\n", intfloat.f, e->value);
> +
> + // Next available bit pattern should not match
> + ++intfloat.i;
> + av_dict_set_float32(&dict, "0xa64760a", intfloat.f, 0);
> + e2 = av_dict_get(dict, "0xa64760a", NULL, 0);
> + printf("%.30g => %s\n", intfloat.f, e2->value);
> +
> + printf("%s =! %s %s\n", e->value, e2->value, strcmp(e->value, e2->value) != 0 ? "OK" : "BAD");
> + av_dict_free(&dict);
> +
> return 0;
> }
> #endif
> diff --git a/libavutil/dict.h b/libavutil/dict.h
> index f2df687..0b056ee 100644
> --- a/libavutil/dict.h
> +++ b/libavutil/dict.h
> @@ -136,6 +136,30 @@ int av_dict_set(AVDictionary **pm, const char *key, const char *value, int flags
> int av_dict_set_int(AVDictionary **pm, const char *key, int64_t value, int flags);
>
> /**
> + * Convenience wrapper for av_dict_set that converts the value to a string
> + * and stores it.
> + *
> + * Note: If AV_DICT_DONT_STRDUP_KEY is set, key will be freed on error.
> + */
> +int av_dict_set_uint(AVDictionary **pm, const char *key, uint64_t value, int flags);
> +
> +/**
> + * Convenience wrapper for av_dict_set that converts the value to a string
> + * and stores it.
> + *
> + * Note: If AV_DICT_DONT_STRDUP_KEY is set, key will be freed on error.
> + */
> +int av_dict_set_float32(AVDictionary **pm, const char *key, float value, int flags);
> +
> +/**
> + * Convenience wrapper for av_dict_set that converts the value to a string
> + * and stores it.
> + *
> + * Note: If AV_DICT_DONT_STRDUP_KEY is set, key will be freed on error.
> + */
> +int av_dict_set_float64(AVDictionary **pm, const char *key, double value, int flags);
The documentation should make clear that it just converts normal double into
a hard-coded format, and does no effort to handle strange floating point
values.
> +
> +/**
> * Parse the key/value pairs list and add the parsed entries to a dictionary.
> *
> * In case of failure, all the successfully set entries are stored in
> diff --git a/tests/ref/fate/dict b/tests/ref/fate/dict
> index 837f7b0..224a370 100644
> --- a/tests/ref/fate/dict
> +++ b/tests/ref/fate/dict
> @@ -41,3 +41,20 @@ Testing av_dict_set_int()
> Testing av_dict_set() with existing AVDictionaryEntry.key as key
> new val OK
> new val OK
> +
> +Testing av_dict_set_int()
> +0 0
> +-max -9223372036854775808
> +max 9223372036854775807
> +
> +Testing av_dict_set_uint()
> +0 0
> +max 18446744073709551615
> +
> +Testing av_dict_set_float*()
> +-1.20786635307013057172298431396e-05 => -1.20786635e-05
> +-1.20786635000000002328028603227e-05 => -1.20786635e-05
> +-1.20786635e-05 == -1.20786635e-05 OK
> +1.10000006285064925259367563427e-32 => 1.10000006e-32
> +1.10000013631904617898664488232e-32 => 1.10000014e-32
> +1.10000006e-32 =! 1.10000014e-32 OK
Regards,
--
Nicolas George
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20150904/3217183e/attachment.sig>
More information about the ffmpeg-devel
mailing list