Hi
On Thu, Apr 17, 2025 at 07:57:46PM +0200, Nicolas George wrote:
> Michael Niedermayer (HE12025-04-17):
> > +AVMap *av_map_new(AVMapCompareFunc cmp_keyvalue, int cmp_flags,
> > AVMapCopyFunc copy, AVMapFreeFunc freef)
> > +{
> > + AVMap *s = av_mallocz(sizeof(*s));
> > + if (!s)
> > + return NULL;
> > +
> > + s->copy = copy;
> > + s->freef = freef;
> > +
> > + av_map_add_cmp_func(s, cmp_keyvalue, cmp_flags);
> > +
> > + return s;
> > +}
>
> If you do not want to do the version where dynamic allocation can be
> avoided, would you at least consider using _alloc() for this? It is only
> to letters more, it is consistent with the rest of the API and it would
> leave _new() available if somebody adds the lighter version.
ok, i will change teh name
>
> I really do not understand why you do not want to do this: it is a very
> useful low-hanging fruit, and it is quite elegant and fun to implement.
You can implement an av_map_new() variant thats on the stack
if thats what you want to have / to use / to do
once the code is in git
or maybe i will do it but there are many more interresting things
> Alas, you did not reply to my last round of arguments.
yes, mails take alot of time to read and reply to.
>
> > + * @param keyvalue_cmp compare function, will be passed the key + value
> > concatenated.
>
> Please no. Pass the key and the value separately to the compare
> function.
thats neither efficient nor does it fit the existing APIs.
The value is most of the time not used and when used it is known exactly
where it is. after a string compare of the key thats equal the value location
is known
or with arbitrary binary data the compare function knows the size of the data.
Passing redudant pointers just wastes cpu cycles
also existing APIs from av_tree strcmp() av_strcasecmp() and so on none of
which take 4 pointers from which they would ignore 2.
>
> > +/**
> > + * Add a compatible compare function to the map.
> > + * The function will later be selected by using AV_MAP_CMP_* flags
> > + *
> > + * Functions must be added from most specific to least specific, that is
> > if a AVMap is build
> > + * with a case insensitive compare no case sensitive compare functions can
> > be added. This is
> > + * to avoid building non functional AVMaps.
> > + *
> > + *
> > + * @see av_map_new
> > + *
> > + * @param cmp compare function to be added.
> > + * cmp(a,b) must return 0 or be equal to the previously added
> > compare function for (a,b), if it returns 0 it also must do so for all
> > + * elements between a and b
> > + *
> > + * @param cmp_flags a combination of AV_MAP_CMP_*, note key/keyvalue and
> > truncated vs non truncated
> > + * are mandatory to be specified
> > + *
> > + * @return a negative error code if the cmp_flags are illegal or
> > unsupported for this AVMap
> > + * If you know your flags are valid, then you dont need to check
> > the return
> > + */
> > +int av_map_add_cmp_func(AVMap *m, AVMapCompareFunc cmp, int cmp_flags);
>
> This whole thing with multiple compare functions makes the code quite
> hard to understand. And not just the code: it makes the API itself hard
> to understand.
>
> I think the practical goal it server could be achieved in a simpler way.
>
> If I understand correctly, it servers to allow the equivalent of
> AV_DICT_IGNORE_SUFFIX and such. It would be simpler to have a flag to
> av_map_find() to let it return the first key >= to the searched key.
There is
AV_MAP_CMP_CASE_SENSITIVE
AV_MAP_CMP_CASE_INSENSITIVE
AV_MAP_CMP_KEY
AV_MAP_CMP_KEYVALUE
AV_MAP_CMP_TRUNCATED
AV_MAP_CMP_NON_TRUNCATED
and these are passed to av_map_get() no compare function.
the compare functions are needed purely for setup. And there
they are needed because of arbitrary data support. av_map cannot
know how to compare your data. (if its not strings)
but something like a av_map_find() that returns the closest
previous and next matching and mismatching entries is a good idea too.
But the existing code is much simpler:
AVMapEntry e = NULL;
while (e = av_map_get_multiple(set, e, "author",
AV_MAP_CMP_CASE_INSENSITIVE | AV_MAP_CMP_KEY) {
printf("Tag key:%s value:%s\n", e->key, e->value);
}
vs:
/**
*
* @param surroundings these are the 2 closest non matching and 2 farthest
matching entries
*/
const AVMapEntry *av_map_get_find(const AVMap *s, AVMapEntry
*surroundings[4], const char *keyvalue, int flags);
you would probably need something like this:
(which is alot uglier and probably contains bugs)
AVMapEntry *e4[4] = {NULL};
AVMapEntry *e = av_map_find(set, e4, "author", AV_MAP_CMP_KEY); //We hope
that "author" is the first in the ordering of case insensitive strings
if (e4[2]) // we compare just the key so we can have multiple matches and
need to take the first not any
e = e4[2];
if (!e)
e = e4[1]; //if we have no exact match we start with the next non match
which may be matching with case insensitive compare
while(e) {
if (!av_strcasecmp(e->key, "author")) {
printf("Tag key:%s value:%s\n", e->key, e->value);
} else {
break;
}
memset(e4, 0, sizeof(e4));
av_map_find(set, e4, e->key, 0);
e = e4[1];
}
Also the code above will just give wrong results with no warning if the compare
function the map is setup with is incompatible with
thx
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Concerning the gods, I have no means of knowing whether they exist or not
or of what sort they may be, because of the obscurity of the subject, and
the brevity of human life -- Protagoras
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".
