[FFmpeg-devel] [PATCH] libavutil/dict: extend the list of convienience functions for storing different data types

[Kevin Wheatley]

Hi,

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.

I'm also conscious that to provide for rewriting the .mov metadata
types, I'll need something different to store the actual types and
that might mean these functions would end up not being used at all.

Kevin
From 34fedb67d58402b519a7c91bff7623469802c4c4 Mon Sep 17 00:00:00 2001
From: Kevin Wheatley 
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 
---
 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;
+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;
+
 printf("Testing av_dict_get_string() and av_dict_parse_string()\n");
 av_dict_get_string(dict, , '=', ',');
 printf("%s\n", buffer);
@@ -356,6 +387,52 @@ int main(void)
 printf("%s\n", e->value);
 av_dict_free();
 
+printf("\nTesting av_dict_set_int()\n");
+un_signed_int.ui = 0U;
+av_dict_set_int(, "0", un_signed_int.i, 0);
+un_signed_int.ui = 0x8000UL;
+av_dict_set_int(, "-max", un_signed_int.i, 0);
+un_signed_int.ui = 0x7fffUL;
+av_dict_set_int(, "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();
+
+printf("\nTesting av_dict_set_uint()\n");
+av_dict_set_uint(, "0", 0, 0);
+av_dict_set_uint(, "max", 0xUL, 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();
+
+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(, "f32", f32, 0);
+e = av_dict_get(dict, "f32", NULL, 0);
+printf("%.30g => %s\n", f32, e->value);
+av_dict_set_float64(, "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(, "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(, "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" : 

Re: [FFmpeg-devel] [PATCH] libavutil/dict: extend the list of convienience functions for storing different data types

[wm4]

On Fri, 4 Sep 2015 14:38:54 +0100
Kevin Wheatley  wrote:

> Hi,
> 
> 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.

I don't understand it either. Why not just use float and double,
instead of obfuscating the names further? (It'd be a difference if this
function e.g. serialized the floats to binary - but it literally just
passes them as-is to libc. Having the C data type in the argument seems
advantageous.

Also, what's the use in having a float function at all?

I'd call av_dict_set_uint av_dict_set_uint64.

> I'm also conscious that to provide for rewriting the .mov metadata
> types, I'll need something different to store the actual types and
> that might mean these functions would end up not being used at all.
> 
> Kevin

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] libavutil/dict: extend the list of convienience functions for storing different data types

[Kevin Wheatley]

On Fri, Sep 4, 2015 at 4:18 PM, wm4  wrote:
> On Fri, 4 Sep 2015 14:38:54 +0100
> Kevin Wheatley  wrote:
>
>> Hi,
>>
>> 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.
>
> I don't understand it either. Why not just use float and double,
> instead of obfuscating the names further? (It'd be a difference if this
> function e.g. serialized the floats to binary - but it literally just
> passes them as-is to libc. Having the C data type in the argument seems
> advantageous.
>
> Also, what's the use in having a float function at all?
>
> I'd call av_dict_set_uint av_dict_set_uint64.

I'll explain my POV...

Essentially these helpers do the same as the existing
av_dict_set_int() does for int64_t so I've essentially just copied
that and substituted the required conversions from  to char
array. The floating point is needed to correctly decode those formats.

Using the bit length in the name of the floating point variation of
the functions is because they will only correctly produce the textual
representation sufficient to transform back into the same binary
representation based on the bit lengths of the floating point
representation (they assume IEEE single and double precision in the
lengths of the formatting options and thus buffer size).

Mentally I was in the middle of the Apple QuickTime metadata
representations where they also use the floatXX terminology - not a
great excuse I know :-)


Re Nicholas' comments on the

flags &= ~AV_DICT_DONT_STRDUP_VAL;

I was simply replicating the existing form of the function av_dict_set_int().

As to portability in the tests - yes I agree - happy for somebody to
point me towards a good solution for that.


My alternative to these would have been to embed the formatting in the
libavformat/mov.c code which obviously would have less impact.

Kevin
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] libavutil/dict: extend the list of convienience functions for storing different data types

[Nicolas George]

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 
> 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 
> ---
>  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, , '=', ',');
>  printf("%s\n", buffer);
> @@ -356,6 +387,52 @@ int main(void)
>  printf("%s\n", e->value);
>  av_dict_free();
>  
> +printf("\nTesting av_dict_set_int()\n");
> +un_signed_int.ui = 0U;
> +av_dict_set_int(, "0", un_signed_int.i, 0);
> +un_signed_int.ui = 0x8000UL;
> +av_dict_set_int(, "-max", un_signed_int.i, 0);
> +un_signed_int.ui = 0x7fffUL;
> +av_dict_set_int(, "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();
> +
> +printf("\nTesting av_dict_set_uint()\n");
> +av_dict_set_uint(, "0", 0, 0);
> +av_dict_set_uint(, "max", 0xUL, 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();
> +
> +printf("\nTesting av_dict_set_float*()\n");
> +// float and double representation of a given number should give the 
> same value
> +f32 = -1.20786635e-05f;
>