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] avutil/map: [WIP] Introduction
@ 2025-04-20  2:29 Michael Niedermayer
  2025-04-21 15:01 ` Nicolas George
  2025-04-21 19:55 ` Marton Balint
  0 siblings, 2 replies; 8+ messages in thread
From: Michael Niedermayer @ 2025-04-20  2:29 UTC (permalink / raw)
  To: FFmpeg development discussions and patches

Note, help is welcome.
Time i spend on this, i cannot spend on other things

Note2: i intend to push AVMap after the release unless the release
ends up delayed alot for other reasons, theres no real reason
to hurry here except that i seem to keep workig on it when
people ask for some non trivial changes/improvments :)
so dont ask, send patch yourself if its not a trivial change :))

Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavutil/map.h | 86 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 86 insertions(+)

diff --git a/libavutil/map.h b/libavutil/map.h
index 8211a05ec8d..0d3f7eab9ac 100644
--- a/libavutil/map.h
+++ b/libavutil/map.h
@@ -31,6 +31,92 @@
 #include "tree.h"
 
 /**
+ * @file
+ *
+ * AVMap is a simple and fast key -> value map.
+ *
+ * ---------- Creating AVMaps ------------------
+ *
+ * AVMap *map = av_map_alloc(strcmp, AV_MAP_CMP_CASE_SENSITIVE + AV_MAP_CMP_KEY, NULL, NULL);
+ *
+ * This creates a case sensitve string based map using strcmp(). It will not allow
+ * multiple entries with the same key.
+ * or
+ *
+ * AVMap *map = av_map_alloc(av_map_strcmp_keyvalue, AV_MAP_CMP_CASE_SENSITIVE + AV_MAP_CMP_KEYVALUE, NULL, NULL);
+ *
+ * This is like the previous, but it will allow multiple entries with the same key
+ * the difference here is that the compare function compares the value too when
+ * the key is equal.
+ * All entries in a map must always be different. So by comparing the value
+ * too we can have multiple entries with the same key
+ *
+ * The remaining 2 pointers in av_map_alloc() are for a function copying an element
+ * and one for freeing it. That is only needed for complex objects, not for strings.
+ *
+ *
+ * ----------- Adding entries -----------------
+ *
+ * av_map_add_strings(map, "cat", "neko", 0); // add new entry or do nothing
+ *
+ * av_map_add_strings(map, "cat", "neko", AV_MAP_REPLACE); // add new entry or replace existing
+ *
+ *
+ * ----------- Removing entries -----------------
+ *
+ * Removing entries does by default not rebuild the map. That is, while access will always
+ * be O(log n) when n becomes smaller, memory consumption will not decrease until
+ * AV_SET_ALLOW_REBUILD is used. Note if you use AV_SET_ALLOW_REBUILD, all previously
+ * returned elements become invalid.
+ *
+ * av_map_del(map, "cat", 0); // remove one entry matching "the key"
+ *
+ * av_map_del(map, "cat", AV_SET_ALLOW_REBUILD); // remove one entry matching "the key" and rebuild the map to re
+ *
+ *
+ * ----------- Retrieving an entry --------------
+ *
+ * AVMapEntry *e = av_map_get(map, "cat", AV_MAP_CMP_KEY); //Find an entry with the key = "cat"
+ *
+ * AVMapEntry *e = av_map_get(map, "cat", AV_MAP_CMP_KEY+AV_MAP_CMP_CASE_INSENSITIVE); //Find an entry with the key = "cat", "Cat", "cAt", ...
+ * // this will only work if one of the set compare functions is case insensitive
+ *
+ *
+ * ----------- Iterating over all elements ------
+ *
+ * const AVMapEntry *t = NULL;
+ * while ((t = av_map_iterate(s, t)))
+ *     printf("%s=%s %zu,%zu   ", t->key, t->value, t->keylen, t->valuelen);
+ *
+ *
+ * ----------- copying all elements of a mep into another map
+ *
+ * av_map_copy(dst, src);
+ *
+ *
+ * ----------- freeing a map ---------------------
+ *
+ * av_map_free(&map);
+ *
+ *
+ * ----------- multiple compare function in a single map -----------
+ *
+ * Each map has a primary compare function, which is used for ordering elements.
+ * Additional (compatible) compare functions can be added with av_map_add_cmp_func()
+ *
+ * What "compaibility" means here is that every added function returns the same value
+ * as the primary function or 0.
+ *
+ * An example, Imagine we have "cat", "dog", "Dog", "fox"
+ * a function that treats "dog" and "Dog" as equal is compatible to this ordering
+ * OTOH
+ * if we have have strcmp() as primary function we would order like this:
+ * "Dog", "cat", "dog", "fox"
+ * and here we could not treat "dog" and "Dog" as equal, and thus case insensitive
+ * compare would not be possible
+ *
+ * ----------- compared to AVDictionary -----------
+ *
  * compared to AVDictionary this has
  * clone is O(n) instead of O(n²)
  * copy is O(n*log n) instead of O(n²)
-- 
2.49.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] 8+ messages in thread

* Re: [FFmpeg-devel] [PATCH] avutil/map: [WIP] Introduction
  2025-04-20  2:29 [FFmpeg-devel] [PATCH] avutil/map: [WIP] Introduction Michael Niedermayer
@ 2025-04-21 15:01 ` Nicolas George
  2025-04-23 20:46   ` Michael Niedermayer
  2025-04-21 19:55 ` Marton Balint
  1 sibling, 1 reply; 8+ messages in thread
From: Nicolas George @ 2025-04-21 15:01 UTC (permalink / raw)
  To: FFmpeg development discussions and patches

Michael Niedermayer (HE12025-04-20):
>					  theres no real reason
> to hurry here

(This is me applauding.)

>		except that i seem to keep workig on it when
> people ask for some non trivial changes/improvments :)

As long as you have fun.

> so dont ask, send patch yourself if its not a trivial change :))
> 
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavutil/map.h | 86 +++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 86 insertions(+)
> 
> diff --git a/libavutil/map.h b/libavutil/map.h
> index 8211a05ec8d..0d3f7eab9ac 100644
> --- a/libavutil/map.h
> +++ b/libavutil/map.h
> @@ -31,6 +31,92 @@
>  #include "tree.h"
>  
>  /**
> + * @file
> + *
> + * AVMap is a simple and fast key -> value map.
> + *
> + * ---------- Creating AVMaps ------------------
> + *
> + * AVMap *map = av_map_alloc(strcmp, AV_MAP_CMP_CASE_SENSITIVE + AV_MAP_CMP_KEY, NULL, NULL);
> + *
> + * This creates a case sensitve string based map using strcmp(). It will not allow
> + * multiple entries with the same key.
> + * or
> + *
> + * AVMap *map = av_map_alloc(av_map_strcmp_keyvalue, AV_MAP_CMP_CASE_SENSITIVE + AV_MAP_CMP_KEYVALUE, NULL, NULL);
> + *

> + * This is like the previous, but it will allow multiple entries with the same key
> + * the difference here is that the compare function compares the value too when
> + * the key is equal.
> + * All entries in a map must always be different. So by comparing the value
> + * too we can have multiple entries with the same key

Maybe insist: “If the same (key, value) pair is added multiple times, it
will be returned only once when iterating and removing it once will
completely remove it.”

And that raises the question: should this API not also allow entries
with multiplicity?

Also: what happens if:

AVMap *map = av_map_alloc(av_map_strcmp_keyvalue, AV_MAP_CMP_CASE_SENSITIVE + AV_MAP_CMP_KEY, NULL, NULL);
AVMap *map = av_map_alloc(strcmp, AV_MAP_CMP_CASE_SENSITIVE + AV_MAP_CMP_KEYVALUE, NULL, NULL);

(I swapped the compare functions)?


> + * The remaining 2 pointers in av_map_alloc() are for a function copying an element
> + * and one for freeing it. That is only needed for complex objects, not for strings.

Ok for now I guess, but eventually a complete introduction should
include sample code, for example if key is
struct { const char *family_name, *given_name; }
and the value is
struct { int type; const char *text; }.

> + *
> + *
> + * ----------- Adding entries -----------------
> + *
> + * av_map_add_strings(map, "cat", "neko", 0); // add new entry or do nothing
> + *
> + * av_map_add_strings(map, "cat", "neko", AV_MAP_REPLACE); // add new entry or replace existing
> + *
> + *
> + * ----------- Removing entries -----------------
> + *
> + * Removing entries does by default not rebuild the map. That is, while access will always
> + * be O(log n) when n becomes smaller, memory consumption will not decrease until
> + * AV_SET_ALLOW_REBUILD is used. Note if you use AV_SET_ALLOW_REBUILD, all previously
> + * returned elements become invalid.
> + *

> + * av_map_del(map, "cat", 0); // remove one entry matching "the key"
> + *
> + * av_map_del(map, "cat", AV_SET_ALLOW_REBUILD); // remove one entry matching "the key" and rebuild the map to re

(It seems you sentence got interrup)

If I did “av_map_add("animal\0cat", 11, "neko", 5, 0)”, how can I del or
get it?

> + *
> + *
> + * ----------- Retrieving an entry --------------
> + *
> + * AVMapEntry *e = av_map_get(map, "cat", AV_MAP_CMP_KEY); //Find an entry with the key = "cat"
> + *
> + * AVMapEntry *e = av_map_get(map, "cat", AV_MAP_CMP_KEY+AV_MAP_CMP_CASE_INSENSITIVE); //Find an entry with the key = "cat", "Cat", "cAt", ...

> + * // this will only work if one of the set compare functions is case insensitive

This is crucial: how? (This is redundant with my next remark.)

> + *
> + *
> + * ----------- Iterating over all elements ------
> + *
> + * const AVMapEntry *t = NULL;
> + * while ((t = av_map_iterate(s, t)))
> + *     printf("%s=%s %zu,%zu   ", t->key, t->value, t->keylen, t->valuelen);
> + *
> + *
> + * ----------- copying all elements of a mep into another map
> + *
> + * av_map_copy(dst, src);
> + *
> + *
> + * ----------- freeing a map ---------------------
> + *
> + * av_map_free(&map);
> + *
> + *
> + * ----------- multiple compare function in a single map -----------
> + *

> + * Each map has a primary compare function, which is used for ordering elements.
> + * Additional (compatible) compare functions can be added with av_map_add_cmp_func()
> + *
> + * What "compaibility" means here is that every added function returns the same value
> + * as the primary function or 0.
> + *
> + * An example, Imagine we have "cat", "dog", "Dog", "fox"
> + * a function that treats "dog" and "Dog" as equal is compatible to this ordering
> + * OTOH
> + * if we have have strcmp() as primary function we would order like this:
> + * "Dog", "cat", "dog", "fox"
> + * and here we could not treat "dog" and "Dog" as equal, and thus case insensitive
> + * compare would not be possible

Code examples needed. For example: a map where key = name, value = phone
number, with AV_MAP_CMP_KEYVALUE. How do we set it up to allow
case-insensitive lookups and name only lookups, and name only
case-insensitive lookups?

I would help, but I still cannot wrap my head around the way your flags
system work.

> + *
> + * ----------- compared to AVDictionary -----------
> + *
>   * compared to AVDictionary this has
>   * clone is O(n) instead of O(n²)
>   * copy is O(n*log n) instead of O(n²)

Thanks.

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] 8+ messages in thread

* Re: [FFmpeg-devel] [PATCH] avutil/map: [WIP] Introduction
  2025-04-20  2:29 [FFmpeg-devel] [PATCH] avutil/map: [WIP] Introduction Michael Niedermayer
  2025-04-21 15:01 ` Nicolas George
@ 2025-04-21 19:55 ` Marton Balint
  2025-04-23  0:31   ` Michael Niedermayer
  1 sibling, 1 reply; 8+ messages in thread
From: Marton Balint @ 2025-04-21 19:55 UTC (permalink / raw)
  To: FFmpeg development discussions and patches



On Sun, 20 Apr 2025, Michael Niedermayer wrote:

> Note, help is welcome.
> Time i spend on this, i cannot spend on other things
>
> Note2: i intend to push AVMap after the release unless the release
> ends up delayed alot for other reasons, theres no real reason
> to hurry here except that i seem to keep workig on it when
> people ask for some non trivial changes/improvments :)
> so dont ask, send patch yourself if its not a trivial change :))
>
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
> libavutil/map.h | 86 +++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 86 insertions(+)
>
> diff --git a/libavutil/map.h b/libavutil/map.h
> index 8211a05ec8d..0d3f7eab9ac 100644
> --- a/libavutil/map.h
> +++ b/libavutil/map.h
> @@ -31,6 +31,92 @@
> #include "tree.h"
> 
> /**
> + * @file
> + *
> + * AVMap is a simple and fast key -> value map.

Is this intended to be an AVDictionary replacement? Because as far as I 
remember AVDictionary keeps key insertion order, and we even rely on this 
behaviour sometimes, so an ordered-by-compare-function list is likely not 
going to work as an instant drop-in replacement...

> + *
> + * ---------- Creating AVMaps ------------------
> + *
> + * AVMap *map = av_map_alloc(strcmp, AV_MAP_CMP_CASE_SENSITIVE + AV_MAP_CMP_KEY, NULL, NULL);
> + *
> + * This creates a case sensitve string based map using strcmp(). It will not allow
> + * multiple entries with the same key.
> + * or
> + *
> + * AVMap *map = av_map_alloc(av_map_strcmp_keyvalue, AV_MAP_CMP_CASE_SENSITIVE + AV_MAP_CMP_KEYVALUE, NULL, NULL);
> + *
> + * This is like the previous, but it will allow multiple entries with the same key
> + * the difference here is that the compare function compares the value too when
> + * the key is equal.
> + * All entries in a map must always be different. So by comparing the value
> + * too we can have multiple entries with the same key
> + *
> + * The remaining 2 pointers in av_map_alloc() are for a function copying an element
> + * and one for freeing it. That is only needed for complex objects, not for strings.
> + *
> + *
> + * ----------- Adding entries -----------------
> + *
> + * av_map_add_strings(map, "cat", "neko", 0); // add new entry or do nothing

What "or do nothing" means here? That it will not overwrite a key by 
default? This is a different semantics than AVDictionary, where you need 
to explicitly set DONT_OVERWRITE flag for such.

I think we should use function names and flags similar to what we have in 
AVDictionary. Like av_map_set_strings() instead of av_map_add_strings(), 
or AV_MAP_DONT_OVERWRITE. So our users won't have to use different 
mindset for similar stuff.

> + *
> + * av_map_add_strings(map, "cat", "neko", AV_MAP_REPLACE); // add new entry or replace existing
> + *
> + *
> + * ----------- Removing entries -----------------
> + *
> + * Removing entries does by default not rebuild the map. That is, while access will always
> + * be O(log n) when n becomes smaller, memory consumption will not decrease until
> + * AV_SET_ALLOW_REBUILD is used. Note if you use AV_SET_ALLOW_REBUILD, all previously
> + * returned elements become invalid.
> + *
> + * av_map_del(map, "cat", 0); // remove one entry matching "the key"
> + *
> + * av_map_del(map, "cat", AV_SET_ALLOW_REBUILD); // remove one entry matching "the key" and rebuild the map to re

Do you specify a key, or a concatenated key + \0 + value? Or you can 
specify both?

In general I believe the public API should not use const char * for 
keyvalue types, that would be very fragile. A string constant is 
not a valid concatenated keyvalue for example, and the compiler will not 
catch such isses. Therefore public functions should always use separate 
key and separate value parameters.

> + *
> + *
> + * ----------- Retrieving an entry --------------
> + *
> + * AVMapEntry *e = av_map_get(map, "cat", AV_MAP_CMP_KEY); //Find an entry with the key = "cat"
> + *
> + * AVMapEntry *e = av_map_get(map, "cat", AV_MAP_CMP_KEY+AV_MAP_CMP_CASE_INSENSITIVE); //Find an entry with the key = "cat", "Cat", "cAt", ...
> + * // this will only work if one of the set compare functions is case insensitive
> + *
> + *
> + * ----------- Iterating over all elements ------
> + *
> + * const AVMapEntry *t = NULL;
> + * while ((t = av_map_iterate(s, t)))
> + *     printf("%s=%s %zu,%zu   ", t->key, t->value, t->keylen, t->valuelen);
> + *
> + *
> + * ----------- copying all elements of a mep into another map
> + *
> + * av_map_copy(dst, src);
> + *
> + *
> + * ----------- freeing a map ---------------------
> + *
> + * av_map_free(&map);
> + *
> + *
> + * ----------- multiple compare function in a single map -----------
> + *
> + * Each map has a primary compare function, which is used for ordering elements.
> + * Additional (compatible) compare functions can be added with av_map_add_cmp_func()
> + *
> + * What "compaibility" means here is that every added function returns the same value
> + * as the primary function or 0.
> + *
> + * An example, Imagine we have "cat", "dog", "Dog", "fox"
> + * a function that treats "dog" and "Dog" as equal is compatible to this ordering
> + * OTOH
> + * if we have have strcmp() as primary function we would order like this:
> + * "Dog", "cat", "dog", "fox"
> + * and here we could not treat "dog" and "Dog" as equal, and thus case insensitive
> + * compare would not be possible
> + *
> + * ----------- compared to AVDictionary -----------
> + *
>  * compared to AVDictionary this has
>  * clone is O(n) instead of O(n²)
>  * copy is O(n*log n) instead of O(n²)

You should also mention the differences (e.g. ordering).

Thanks,
Marton
_______________________________________________
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] 8+ messages in thread

* Re: [FFmpeg-devel] [PATCH] avutil/map: [WIP] Introduction
  2025-04-21 19:55 ` Marton Balint
@ 2025-04-23  0:31   ` Michael Niedermayer
  2025-04-23 21:16     ` Marton Balint
  0 siblings, 1 reply; 8+ messages in thread
From: Michael Niedermayer @ 2025-04-23  0:31 UTC (permalink / raw)
  To: FFmpeg development discussions and patches


[-- Attachment #1.1: Type: text/plain, Size: 5778 bytes --]

Hi

On Mon, Apr 21, 2025 at 09:55:33PM +0200, Marton Balint wrote:
> 
> 
> On Sun, 20 Apr 2025, Michael Niedermayer wrote:
> 
> > Note, help is welcome.
> > Time i spend on this, i cannot spend on other things
> > 
> > Note2: i intend to push AVMap after the release unless the release
> > ends up delayed alot for other reasons, theres no real reason
> > to hurry here except that i seem to keep workig on it when
> > people ask for some non trivial changes/improvments :)
> > so dont ask, send patch yourself if its not a trivial change :))
> > 
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> > libavutil/map.h | 86 +++++++++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 86 insertions(+)
> > 
> > diff --git a/libavutil/map.h b/libavutil/map.h
> > index 8211a05ec8d..0d3f7eab9ac 100644
> > --- a/libavutil/map.h
> > +++ b/libavutil/map.h
> > @@ -31,6 +31,92 @@
> > #include "tree.h"
> > 
> > /**
> > + * @file
> > + *
> > + * AVMap is a simple and fast key -> value map.
> 
> Is this intended to be an AVDictionary replacement?

Yes.
In how far a "automatic" replacement is possible iam not sure


> Because as far as I
> remember AVDictionary keeps key insertion order, and we even rely on this
> behaviour sometimes, so an ordered-by-compare-function list is likely not
> going to work as an instant drop-in replacement...

What do you mean by "keeps key insertion order" ?
this isnt documented, or is it ?
In fact i dont think thats supposed to be guranteed by AVDictionary

I think the order coming out of av_map_iterate() will match the
order elements where added.
Is that what you meant ?



> 
> > + *
> > + * ---------- Creating AVMaps ------------------
> > + *
> > + * AVMap *map = av_map_alloc(strcmp, AV_MAP_CMP_CASE_SENSITIVE + AV_MAP_CMP_KEY, NULL, NULL);
> > + *
> > + * This creates a case sensitve string based map using strcmp(). It will not allow
> > + * multiple entries with the same key.
> > + * or
> > + *
> > + * AVMap *map = av_map_alloc(av_map_strcmp_keyvalue, AV_MAP_CMP_CASE_SENSITIVE + AV_MAP_CMP_KEYVALUE, NULL, NULL);
> > + *
> > + * This is like the previous, but it will allow multiple entries with the same key
> > + * the difference here is that the compare function compares the value too when
> > + * the key is equal.
> > + * All entries in a map must always be different. So by comparing the value
> > + * too we can have multiple entries with the same key
> > + *
> > + * The remaining 2 pointers in av_map_alloc() are for a function copying an element
> > + * and one for freeing it. That is only needed for complex objects, not for strings.
> > + *
> > + *
> > + * ----------- Adding entries -----------------
> > + *
> > + * av_map_add_strings(map, "cat", "neko", 0); // add new entry or do nothing
> 
> What "or do nothing" means here? That it will not overwrite a key by
> default? This is a different semantics than AVDictionary, where you need to
> explicitly set DONT_OVERWRITE flag for such.

yes, we can flip the default around if people prefer
I really picked this solely because i felt negated flags with "DONT" in their
name are suboptimal design


> 
> I think we should use function names and flags similar to what we have in
> AVDictionary. Like av_map_set_strings() instead of av_map_add_strings(), or
> AV_MAP_DONT_OVERWRITE. So our users won't have to use different mindset for
> similar stuff.

like i said i dont like the "DONT" flag, i think we should avoid such names
in new designs. Maybe AV_MAP_PRESERVE would be an alternative ?

av_map_set_strings() implies setting a key to a value. When this in reality is
more flexibl, depending on flags and setup


> 
> > + *
> > + * av_map_add_strings(map, "cat", "neko", AV_MAP_REPLACE); // add new entry or replace existing
> > + *
> > + *
> > + * ----------- Removing entries -----------------
> > + *
> > + * Removing entries does by default not rebuild the map. That is, while access will always
> > + * be O(log n) when n becomes smaller, memory consumption will not decrease until
> > + * AV_SET_ALLOW_REBUILD is used. Note if you use AV_SET_ALLOW_REBUILD, all previously
> > + * returned elements become invalid.
> > + *
> > + * av_map_del(map, "cat", 0); // remove one entry matching "the key"
> > + *
> > + * av_map_del(map, "cat", AV_SET_ALLOW_REBUILD); // remove one entry matching "the key" and rebuild the map to re
> 
> Do you specify a key, or a concatenated key + \0 + value? Or you can specify
> both?

it depends on the flags (which select the compare function)
In fact it can be an arbitrary struct instead of a string, if the compare
function  during setup compares the specific struct.

av_map_del(map, "cat\0katze", AV_MAP_CMP_KEYVALUE);
av_map_del(map, "cat", AV_MAP_CMP_KEY);


> 
> In general I believe the public API should not use const char * for keyvalue
> types, that would be very fragile. A string constant is not a valid
> concatenated keyvalue for example, and the compiler will not catch such
> isses. Therefore public functions should always use separate key and
> separate value parameters.

about "const char *", we could use a typedef or struct or something

completely droping keyvalue from public API is not an option because
AVMapEntry always provides you with a keyvalue already and requiring
to copy it on every use is dumb.

so we need to find a smarter solution here

thx

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Into a blind darkness they enter who follow after the Ignorance,
they as if into a greater darkness enter who devote themselves
to the Knowledge alone. -- Isha Upanishad

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 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] 8+ messages in thread

* Re: [FFmpeg-devel] [PATCH] avutil/map: [WIP] Introduction
  2025-04-21 15:01 ` Nicolas George
@ 2025-04-23 20:46   ` Michael Niedermayer
  2025-04-23 21:25     ` Michael Niedermayer
  0 siblings, 1 reply; 8+ messages in thread
From: Michael Niedermayer @ 2025-04-23 20:46 UTC (permalink / raw)
  To: FFmpeg development discussions and patches


[-- Attachment #1.1: Type: text/plain, Size: 6701 bytes --]

Hi Nicolas

On Mon, Apr 21, 2025 at 05:01:19PM +0200, Nicolas George wrote:
> Michael Niedermayer (HE12025-04-20):
> >					  theres no real reason
> > to hurry here
> 
> (This is me applauding.)
> 
> >		except that i seem to keep workig on it when
> > people ask for some non trivial changes/improvments :)
> 
> As long as you have fun.
> 
> > so dont ask, send patch yourself if its not a trivial change :))
> > 
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >  libavutil/map.h | 86 +++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 86 insertions(+)
> > 
> > diff --git a/libavutil/map.h b/libavutil/map.h
> > index 8211a05ec8d..0d3f7eab9ac 100644
> > --- a/libavutil/map.h
> > +++ b/libavutil/map.h
> > @@ -31,6 +31,92 @@
> >  #include "tree.h"
> >  
> >  /**
> > + * @file
> > + *
> > + * AVMap is a simple and fast key -> value map.
> > + *
> > + * ---------- Creating AVMaps ------------------
> > + *
> > + * AVMap *map = av_map_alloc(strcmp, AV_MAP_CMP_CASE_SENSITIVE + AV_MAP_CMP_KEY, NULL, NULL);
> > + *
> > + * This creates a case sensitve string based map using strcmp(). It will not allow
> > + * multiple entries with the same key.
> > + * or
> > + *
> > + * AVMap *map = av_map_alloc(av_map_strcmp_keyvalue, AV_MAP_CMP_CASE_SENSITIVE + AV_MAP_CMP_KEYVALUE, NULL, NULL);
> > + *
> 
> > + * This is like the previous, but it will allow multiple entries with the same key
> > + * the difference here is that the compare function compares the value too when
> > + * the key is equal.
> > + * All entries in a map must always be different. So by comparing the value
> > + * too we can have multiple entries with the same key
> 
> Maybe insist: “If the same (key, value) pair is added multiple times, it
> will be returned only once when iterating and removing it once will
> completely remove it.”
> 
> And that raises the question: should this API not also allow entries
> with multiplicity?

you can have some map where each element maps to an integer, where
that integer represents a "count"


> 
> Also: what happens if:
> 
> AVMap *map = av_map_alloc(av_map_strcmp_keyvalue, AV_MAP_CMP_CASE_SENSITIVE + AV_MAP_CMP_KEY, NULL, NULL);
> AVMap *map = av_map_alloc(strcmp, AV_MAP_CMP_CASE_SENSITIVE + AV_MAP_CMP_KEYVALUE, NULL, NULL);
> 
> (I swapped the compare functions)?

crash probably, but technically, undefined behavior


[...]

> > + *
> > + *
> > + * ----------- Adding entries -----------------
> > + *
> > + * av_map_add_strings(map, "cat", "neko", 0); // add new entry or do nothing
> > + *
> > + * av_map_add_strings(map, "cat", "neko", AV_MAP_REPLACE); // add new entry or replace existing
> > + *
> > + *
> > + * ----------- Removing entries -----------------
> > + *
> > + * Removing entries does by default not rebuild the map. That is, while access will always
> > + * be O(log n) when n becomes smaller, memory consumption will not decrease until
> > + * AV_SET_ALLOW_REBUILD is used. Note if you use AV_SET_ALLOW_REBUILD, all previously
> > + * returned elements become invalid.
> > + *
> 
> > + * av_map_del(map, "cat", 0); // remove one entry matching "the key"
> > + *
> > + * av_map_del(map, "cat", AV_SET_ALLOW_REBUILD); // remove one entry matching "the key" and rebuild the map to re
> 
> (It seems you sentence got interrup)
> 

> If I did “av_map_add("animal\0cat", 11, "neko", 5, 0)”, how can I del or
> get it?

depends on the compare function, the map was setup with.
If you created your own function that works with double or tripple zero
terminated strings. This will work perfectly fine.

Otherwise if you mix the passed object and the function so they are
not compatible then you get undefined behavior

same with qsort() or printf() or AVDictionary


> 
> > + *
> > + *
> > + * ----------- Retrieving an entry --------------
> > + *
> > + * AVMapEntry *e = av_map_get(map, "cat", AV_MAP_CMP_KEY); //Find an entry with the key = "cat"
> > + *
> > + * AVMapEntry *e = av_map_get(map, "cat", AV_MAP_CMP_KEY+AV_MAP_CMP_CASE_INSENSITIVE); //Find an entry with the key = "cat", "Cat", "cAt", ...
> 
> > + * // this will only work if one of the set compare functions is case insensitive
> 
> This is crucial: how? (This is redundant with my next remark.)
> 
> > + *
> > + *
> > + * ----------- Iterating over all elements ------
> > + *
> > + * const AVMapEntry *t = NULL;
> > + * while ((t = av_map_iterate(s, t)))
> > + *     printf("%s=%s %zu,%zu   ", t->key, t->value, t->keylen, t->valuelen);
> > + *
> > + *
> > + * ----------- copying all elements of a mep into another map
> > + *
> > + * av_map_copy(dst, src);
> > + *
> > + *
> > + * ----------- freeing a map ---------------------
> > + *
> > + * av_map_free(&map);
> > + *
> > + *
> > + * ----------- multiple compare function in a single map -----------
> > + *
> 
> > + * Each map has a primary compare function, which is used for ordering elements.
> > + * Additional (compatible) compare functions can be added with av_map_add_cmp_func()
> > + *
> > + * What "compaibility" means here is that every added function returns the same value
> > + * as the primary function or 0.
> > + *
> > + * An example, Imagine we have "cat", "dog", "Dog", "fox"
> > + * a function that treats "dog" and "Dog" as equal is compatible to this ordering
> > + * OTOH
> > + * if we have have strcmp() as primary function we would order like this:
> > + * "Dog", "cat", "dog", "fox"
> > + * and here we could not treat "dog" and "Dog" as equal, and thus case insensitive
> > + * compare would not be possible
> 
> Code examples needed. For example: a map where key = name, value = phone
> number, with AV_MAP_CMP_KEYVALUE. How do we set it up to allow
> case-insensitive lookups and name only lookups, and name only
> case-insensitive lookups?

The phone numbers i have seen are not affected by case sensitivity.
So case sensitivity only matters for the name (which is in key)

This should work:

AVMap *map = av_map_alloc(av_map_supercmp_keyvalue, AV_MAP_CMP_KEYVALUE + AV_MAP_CMP_CASE_SENSITIVE, NULL, NULL);
av_map_add_cmp_func(map,       av_map_supercmp_key, AV_MAP_CMP_KEY      + AV_MAP_CMP_CASE_SENSITIVE);
av_map_add_cmp_func(map,             av_strcasecmp, AV_MAP_CMP_KEY      + AV_MAP_CMP_CASE_INSENSITIVE);

But the point you seem to try to raise, is correct,
not every combination is possible

thx

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The worst form of inequality is to try to make unequal things equal.
-- Aristotle

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 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] 8+ messages in thread

* Re: [FFmpeg-devel] [PATCH] avutil/map: [WIP] Introduction
  2025-04-23  0:31   ` Michael Niedermayer
@ 2025-04-23 21:16     ` Marton Balint
  2025-04-24 11:52       ` Michael Niedermayer
  0 siblings, 1 reply; 8+ messages in thread
From: Marton Balint @ 2025-04-23 21:16 UTC (permalink / raw)
  To: FFmpeg development discussions and patches



On Wed, 23 Apr 2025, Michael Niedermayer wrote:

> Hi
>
> On Mon, Apr 21, 2025 at 09:55:33PM +0200, Marton Balint wrote:
>>
>>
>> On Sun, 20 Apr 2025, Michael Niedermayer wrote:
>>
>>> Note, help is welcome.
>>> Time i spend on this, i cannot spend on other things
>>>
>>> Note2: i intend to push AVMap after the release unless the release
>>> ends up delayed alot for other reasons, theres no real reason
>>> to hurry here except that i seem to keep workig on it when
>>> people ask for some non trivial changes/improvments :)
>>> so dont ask, send patch yourself if its not a trivial change :))
>>>
>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>>> ---
>>> libavutil/map.h | 86 +++++++++++++++++++++++++++++++++++++++++++++++++
>>> 1 file changed, 86 insertions(+)
>>>
>>> diff --git a/libavutil/map.h b/libavutil/map.h
>>> index 8211a05ec8d..0d3f7eab9ac 100644
>>> --- a/libavutil/map.h
>>> +++ b/libavutil/map.h
>>> @@ -31,6 +31,92 @@
>>> #include "tree.h"
>>>
>>> /**
>>> + * @file
>>> + *
>>> + * AVMap is a simple and fast key -> value map.
>>
>> Is this intended to be an AVDictionary replacement?
>
> Yes.
> In how far a "automatic" replacement is possible iam not sure
>
>
>> Because as far as I
>> remember AVDictionary keeps key insertion order, and we even rely on this
>> behaviour sometimes, so an ordered-by-compare-function list is likely not
>> going to work as an instant drop-in replacement...
>
> What do you mean by "keeps key insertion order" ?
> this isnt documented, or is it ?
> In fact i dont think thats supposed to be guranteed by AVDictionary
>
> I think the order coming out of av_map_iterate() will match the
> order elements where added.
> Is that what you meant ?

Yes, exactly. Admittedly not documented behaviour... Another thing is that 
this does not support AV_DICT_MULTIKEY if the same value is used multiple 
times. I am just saying that we should document these fundamental 
differences to avoid any suprises...

>
>
>
>>
>>> + *
>>> + * ---------- Creating AVMaps ------------------
>>> + *
>>> + * AVMap *map = av_map_alloc(strcmp, AV_MAP_CMP_CASE_SENSITIVE + AV_MAP_CMP_KEY, NULL, NULL);
>>> + *
>>> + * This creates a case sensitve string based map using strcmp(). It will not allow
>>> + * multiple entries with the same key.
>>> + * or
>>> + *
>>> + * AVMap *map = av_map_alloc(av_map_strcmp_keyvalue, AV_MAP_CMP_CASE_SENSITIVE + AV_MAP_CMP_KEYVALUE, NULL, NULL);
>>> + *
>>> + * This is like the previous, but it will allow multiple entries with the same key
>>> + * the difference here is that the compare function compares the value too when
>>> + * the key is equal.
>>> + * All entries in a map must always be different. So by comparing the value
>>> + * too we can have multiple entries with the same key
>>> + *
>>> + * The remaining 2 pointers in av_map_alloc() are for a function copying an element
>>> + * and one for freeing it. That is only needed for complex objects, not for strings.
>>> + *
>>> + *
>>> + * ----------- Adding entries -----------------
>>> + *
>>> + * av_map_add_strings(map, "cat", "neko", 0); // add new entry or do nothing
>>
>> What "or do nothing" means here? That it will not overwrite a key by
>> default? This is a different semantics than AVDictionary, where you need to
>> explicitly set DONT_OVERWRITE flag for such.
>
> yes, we can flip the default around if people prefer
> I really picked this solely because i felt negated flags with "DONT" in their
> name are suboptimal design
>
>
>>
>> I think we should use function names and flags similar to what we have in
>> AVDictionary. Like av_map_set_strings() instead of av_map_add_strings(), or
>> AV_MAP_DONT_OVERWRITE. So our users won't have to use different mindset for
>> similar stuff.
>
> like i said i dont like the "DONT" flag, i think we should avoid such names
> in new designs. Maybe AV_MAP_PRESERVE would be an alternative ?

Flag names should be easy to understand and remember, I don't really mind 
if it is is negated. And "dont overwrite" or "no overwrite" is the most 
expressive IMHO.

>
> av_map_set_strings() implies setting a key to a value. When this in reality is
> more flexibl, depending on flags and setup

But if it depends on flags if "add" or "set" make more sense, then I'd 
rather keep set, considering the primary goal for this is being the 
dictionary replacement...

>
>
>>
>>> + *
>>> + * av_map_add_strings(map, "cat", "neko", AV_MAP_REPLACE); // add new entry or replace existing
>>> + *
>>> + *
>>> + * ----------- Removing entries -----------------
>>> + *
>>> + * Removing entries does by default not rebuild the map. That is, while access will always
>>> + * be O(log n) when n becomes smaller, memory consumption will not decrease until
>>> + * AV_SET_ALLOW_REBUILD is used. Note if you use AV_SET_ALLOW_REBUILD, all previously
>>> + * returned elements become invalid.
>>> + *
>>> + * av_map_del(map, "cat", 0); // remove one entry matching "the key"
>>> + *
>>> + * av_map_del(map, "cat", AV_SET_ALLOW_REBUILD); // remove one entry matching "the key" and rebuild the map to re
>>
>> Do you specify a key, or a concatenated key + \0 + value? Or you can specify
>> both?
>
> it depends on the flags (which select the compare function)
> In fact it can be an arbitrary struct instead of a string, if the compare
> function  during setup compares the specific struct.
>
> av_map_del(map, "cat\0katze", AV_MAP_CMP_KEYVALUE);
> av_map_del(map, "cat", AV_MAP_CMP_KEY);
>
>
>>
>> In general I believe the public API should not use const char * for keyvalue
>> types, that would be very fragile. A string constant is not a valid
>> concatenated keyvalue for example, and the compiler will not catch such
>> isses. Therefore public functions should always use separate key and
>> separate value parameters.
>
> about "const char *", we could use a typedef or struct or something
>
> completely droping keyvalue from public API is not an option because
> AVMapEntry always provides you with a keyvalue already and requiring
> to copy it on every use is dumb.

But AVMapEntry has separete key and value pointers, even sizes, so I am 
not sure where you need to copy.

Regards,
Marton
_______________________________________________
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] 8+ messages in thread

* Re: [FFmpeg-devel] [PATCH] avutil/map: [WIP] Introduction
  2025-04-23 20:46   ` Michael Niedermayer
@ 2025-04-23 21:25     ` Michael Niedermayer
  0 siblings, 0 replies; 8+ messages in thread
From: Michael Niedermayer @ 2025-04-23 21:25 UTC (permalink / raw)
  To: FFmpeg development discussions and patches


[-- Attachment #1.1: Type: text/plain, Size: 4056 bytes --]

Hi Nicolas

On Wed, Apr 23, 2025 at 10:46:17PM +0200, Michael Niedermayer wrote:
> Hi Nicolas
> 
> On Mon, Apr 21, 2025 at 05:01:19PM +0200, Nicolas George wrote:
> > Michael Niedermayer (HE12025-04-20):
[...]
> > > + *
> > > + *
> > > + * ----------- Retrieving an entry --------------
> > > + *
> > > + * AVMapEntry *e = av_map_get(map, "cat", AV_MAP_CMP_KEY); //Find an entry with the key = "cat"
> > > + *
> > > + * AVMapEntry *e = av_map_get(map, "cat", AV_MAP_CMP_KEY+AV_MAP_CMP_CASE_INSENSITIVE); //Find an entry with the key = "cat", "Cat", "cAt", ...
> > 
> > > + * // this will only work if one of the set compare functions is case insensitive
> > 
> > This is crucial: how? (This is redundant with my next remark.)
> > 
> > > + *
> > > + *
> > > + * ----------- Iterating over all elements ------
> > > + *
> > > + * const AVMapEntry *t = NULL;
> > > + * while ((t = av_map_iterate(s, t)))
> > > + *     printf("%s=%s %zu,%zu   ", t->key, t->value, t->keylen, t->valuelen);
> > > + *
> > > + *
> > > + * ----------- copying all elements of a mep into another map
> > > + *
> > > + * av_map_copy(dst, src);
> > > + *
> > > + *
> > > + * ----------- freeing a map ---------------------
> > > + *
> > > + * av_map_free(&map);
> > > + *
> > > + *
> > > + * ----------- multiple compare function in a single map -----------
> > > + *
> > 
> > > + * Each map has a primary compare function, which is used for ordering elements.
> > > + * Additional (compatible) compare functions can be added with av_map_add_cmp_func()
> > > + *
> > > + * What "compaibility" means here is that every added function returns the same value
> > > + * as the primary function or 0.
> > > + *
> > > + * An example, Imagine we have "cat", "dog", "Dog", "fox"
> > > + * a function that treats "dog" and "Dog" as equal is compatible to this ordering
> > > + * OTOH
> > > + * if we have have strcmp() as primary function we would order like this:
> > > + * "Dog", "cat", "dog", "fox"
> > > + * and here we could not treat "dog" and "Dog" as equal, and thus case insensitive
> > > + * compare would not be possible
> > 
> > Code examples needed. For example: a map where key = name, value = phone
> > number, with AV_MAP_CMP_KEYVALUE. How do we set it up to allow
> > case-insensitive lookups and name only lookups, and name only
> > case-insensitive lookups?
> 
> The phone numbers i have seen are not affected by case sensitivity.
> So case sensitivity only matters for the name (which is in key)
> 
> This should work:
> 
> AVMap *map = av_map_alloc(av_map_supercmp_keyvalue, AV_MAP_CMP_KEYVALUE + AV_MAP_CMP_CASE_SENSITIVE, NULL, NULL);
> av_map_add_cmp_func(map,       av_map_supercmp_key, AV_MAP_CMP_KEY      + AV_MAP_CMP_CASE_SENSITIVE);
> av_map_add_cmp_func(map,             av_strcasecmp, AV_MAP_CMP_KEY      + AV_MAP_CMP_CASE_INSENSITIVE);
> 
> But the point you seem to try to raise, is correct,
> not every combination is possible

To elaborate on this

If we consider key_value and key searches and case sensitivity
then we have 6 possible things a user might want to search

for only the key there are case sensitive and case insensitive
for key_value there are 4 because both key and value have their own case sensitivity

If we use the syntax or I and S for the key only, and II,SS,IS,SI for the
key_value ones, then we can have for example the following in the same map

I ,S ,SI,SS
I ,II,IS,SS
I ,II,SI,SS

If these combination of 4 out of 6 isnt good enough, the user can use 2 maps
or we can implement the remaining by iteration over the closest subset we can
efficiently search for.
Currently we iterate over all entries with AVDictionary, if for a key value
both case insensitive match we have to iterate over all values from a insensitive
matching key. Its still alot less asymptotically

thx

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

He who knows, does not speak. He who speaks, does not know. -- Lao Tsu

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 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] 8+ messages in thread

* Re: [FFmpeg-devel] [PATCH] avutil/map: [WIP] Introduction
  2025-04-23 21:16     ` Marton Balint
@ 2025-04-24 11:52       ` Michael Niedermayer
  0 siblings, 0 replies; 8+ messages in thread
From: Michael Niedermayer @ 2025-04-24 11:52 UTC (permalink / raw)
  To: FFmpeg development discussions and patches


[-- Attachment #1.1: Type: text/plain, Size: 7095 bytes --]

Hi

On Wed, Apr 23, 2025 at 11:16:13PM +0200, Marton Balint wrote:
> 
> 
> On Wed, 23 Apr 2025, Michael Niedermayer wrote:
> 
> > Hi
> > 
> > On Mon, Apr 21, 2025 at 09:55:33PM +0200, Marton Balint wrote:
> > > 
> > > 
> > > On Sun, 20 Apr 2025, Michael Niedermayer wrote:
[...]
> > > Because as far as I
> > > remember AVDictionary keeps key insertion order, and we even rely on this
> > > behaviour sometimes, so an ordered-by-compare-function list is likely not
> > > going to work as an instant drop-in replacement...
> > 
> > What do you mean by "keeps key insertion order" ?
> > this isnt documented, or is it ?
> > In fact i dont think thats supposed to be guranteed by AVDictionary
> > 
> > I think the order coming out of av_map_iterate() will match the
> > order elements where added.
> > Is that what you meant ?
> 
> Yes, exactly. Admittedly not documented behaviour... Another thing is that
> this does not support AV_DICT_MULTIKEY if the same value is used multiple
> times. I am just saying that we should document these fundamental
> differences to avoid any suprises...

will document this difference


> 
> > 
> > 
> > 
> > > 
> > > > + *
> > > > + * ---------- Creating AVMaps ------------------
> > > > + *
> > > > + * AVMap *map = av_map_alloc(strcmp, AV_MAP_CMP_CASE_SENSITIVE + AV_MAP_CMP_KEY, NULL, NULL);
> > > > + *
> > > > + * This creates a case sensitve string based map using strcmp(). It will not allow
> > > > + * multiple entries with the same key.
> > > > + * or
> > > > + *
> > > > + * AVMap *map = av_map_alloc(av_map_strcmp_keyvalue, AV_MAP_CMP_CASE_SENSITIVE + AV_MAP_CMP_KEYVALUE, NULL, NULL);
> > > > + *
> > > > + * This is like the previous, but it will allow multiple entries with the same key
> > > > + * the difference here is that the compare function compares the value too when
> > > > + * the key is equal.
> > > > + * All entries in a map must always be different. So by comparing the value
> > > > + * too we can have multiple entries with the same key
> > > > + *
> > > > + * The remaining 2 pointers in av_map_alloc() are for a function copying an element
> > > > + * and one for freeing it. That is only needed for complex objects, not for strings.
> > > > + *
> > > > + *
> > > > + * ----------- Adding entries -----------------
> > > > + *
> > > > + * av_map_add_strings(map, "cat", "neko", 0); // add new entry or do nothing
> > > 
> > > What "or do nothing" means here? That it will not overwrite a key by
> > > default? This is a different semantics than AVDictionary, where you need to
> > > explicitly set DONT_OVERWRITE flag for such.
> > 
> > yes, we can flip the default around if people prefer
> > I really picked this solely because i felt negated flags with "DONT" in their
> > name are suboptimal design
> > 
> > 
> > > 
> > > I think we should use function names and flags similar to what we have in
> > > AVDictionary. Like av_map_set_strings() instead of av_map_add_strings(), or
> > > AV_MAP_DONT_OVERWRITE. So our users won't have to use different mindset for
> > > similar stuff.
> > 
> > like i said i dont like the "DONT" flag, i think we should avoid such names
> > in new designs. Maybe AV_MAP_PRESERVE would be an alternative ?
> 
> Flag names should be easy to understand and remember,

yes


> I don't really mind if
> it is is negated. And "dont overwrite" or "no overwrite" is the most
> expressive IMHO.
> 
> > 
> > av_map_set_strings() implies setting a key to a value. When this in reality is
> > more flexibl, depending on flags and setup
> 
> But if it depends on flags if "add" or "set" make more sense, then I'd
> rather keep set, considering the primary goal for this is being the
> dictionary replacement...

"set" has a double meaning, As in Lists, Maps, Sets, ...

map_set, set_map, to me fall in the group of list_map, map_list set_list, list_set, ...

so iam a bit hesitant to change it to that

Ill sleep over these things, maybe i or someone else has some more genius idea


> 
> > 
> > 
> > > 
> > > > + *
> > > > + * av_map_add_strings(map, "cat", "neko", AV_MAP_REPLACE); // add new entry or replace existing
> > > > + *
> > > > + *
> > > > + * ----------- Removing entries -----------------
> > > > + *
> > > > + * Removing entries does by default not rebuild the map. That is, while access will always
> > > > + * be O(log n) when n becomes smaller, memory consumption will not decrease until
> > > > + * AV_SET_ALLOW_REBUILD is used. Note if you use AV_SET_ALLOW_REBUILD, all previously
> > > > + * returned elements become invalid.
> > > > + *
> > > > + * av_map_del(map, "cat", 0); // remove one entry matching "the key"
> > > > + *
> > > > + * av_map_del(map, "cat", AV_SET_ALLOW_REBUILD); // remove one entry matching "the key" and rebuild the map to re
> > > 
> > > Do you specify a key, or a concatenated key + \0 + value? Or you can specify
> > > both?
> > 
> > it depends on the flags (which select the compare function)
> > In fact it can be an arbitrary struct instead of a string, if the compare
> > function  during setup compares the specific struct.
> > 
> > av_map_del(map, "cat\0katze", AV_MAP_CMP_KEYVALUE);
> > av_map_del(map, "cat", AV_MAP_CMP_KEY);
> > 
> > 
> > > 
> > > In general I believe the public API should not use const char * for keyvalue
> > > types, that would be very fragile. A string constant is not a valid
> > > concatenated keyvalue for example, and the compiler will not catch such
> > > isses. Therefore public functions should always use separate key and
> > > separate value parameters.
> > 
> > about "const char *", we could use a typedef or struct or something
> > 
> > completely droping keyvalue from public API is not an option because
> > AVMapEntry always provides you with a keyvalue already and requiring
> > to copy it on every use is dumb.
> 
> But AVMapEntry has separete key and value pointers, even sizes, so I am not
> sure where you need to copy.

current case
here you have a valid keyvalue pointer IF the user specifies that its a keyvalue
and not key.
no check, passing 1 pointer, no copy

what you suggest
here you have key pointer, value pointer, keylen and valuelen
now the user needs to strlen() his strings or keep track of their
length (easy to get that wrong btw)
pass 2 pointers and 2 length into functions. (thats 4 times the stuff)
and then the functions would do

if (key + keylen != value) {
    copy into temporary storage
} // an additional check and potential copy

What is the gain here?
its slower, its more complex, and there are more places to mess up

Whats the point in having an efficient format in teh map when we encourage
teh user to use a incompatible one that needs to be checked and converted on each use.

thx

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

it is not once nor twice but times without number that the same ideas make
their appearance in the world. -- Aristotle

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 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] 8+ messages in thread

end of thread, other threads:[~2025-04-24 11:53 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2025-04-20  2:29 [FFmpeg-devel] [PATCH] avutil/map: [WIP] Introduction Michael Niedermayer
2025-04-21 15:01 ` Nicolas George
2025-04-23 20:46   ` Michael Niedermayer
2025-04-23 21:25     ` Michael Niedermayer
2025-04-21 19:55 ` Marton Balint
2025-04-23  0:31   ` Michael Niedermayer
2025-04-23 21:16     ` Marton Balint
2025-04-24 11:52       ` Michael Niedermayer

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