Re: [FFmpeg-devel] [PATCH v3] libavutil: Add AVMap

[Nicolas George <george@nsup.org>]

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.

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.
Alas, you did not reply to my last round of arguments.

> + * @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.

> +/**
> + * 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.

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".