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
signature.asc
Description: PGP signature
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
