Re: [FFmpeg-devel] [PATCH v2 1/3] avutil/dict: add av_dict_pop

[Andreas Rheinhardt <andreas.rheinhardt@outlook.com>]

Marvin Scholz:
> This new API allows to remove an entry and obtain ownership of the
> key/value that was associated with the removed entry.
> ---
> 
> Changes since v1:
> - Clarify documentation about av_free having to be used.
> - Fix fate test to not rely on specific error code value
> 
>  doc/APIchanges         |  4 ++++
>  libavutil/dict.c       | 27 +++++++++++++++++++++++++++
>  libavutil/dict.h       | 26 ++++++++++++++++++++++++++
>  libavutil/tests/dict.c | 38 ++++++++++++++++++++++++++++++++++++++
>  libavutil/version.h    |  4 ++--
>  tests/ref/fate/dict    | 12 ++++++++++++
>  6 files changed, 109 insertions(+), 2 deletions(-)
> 
> diff --git a/doc/APIchanges b/doc/APIchanges
> index f040211f7d..d55821f682 100644
> --- a/doc/APIchanges
> +++ b/doc/APIchanges
> @@ -2,6 +2,10 @@ The last version increases of all libraries were on 2023-02-09
>  
>  API changes, most recent first:
>  
> +2023-06-02 - xxxxxxxxxx - lavu 58.14.100 - dict.h
> +  Add av_dict_pop() to remove an entry from a dict
> +  and get ownership of the removed key/value.
> +
>  2023-05-29 - xxxxxxxxxx - lavc 60.16.100 - avcodec.h codec_id.h
>    Add AV_CODEC_ID_EVC, FF_PROFILE_EVC_BASELINE, and FF_PROFILE_EVC_MAIN.
>  
> diff --git a/libavutil/dict.c b/libavutil/dict.c
> index f673977a98..ac41771994 100644
> --- a/libavutil/dict.c
> +++ b/libavutil/dict.c
> @@ -173,6 +173,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_pop(AVDictionary **pm, const char *key,
> +                char **out_key, char **out_value, int flags)
> +{
> +    AVDictionary *m = *pm;
> +    AVDictionaryEntry *entry = NULL;
Why don't you merge this initialization and the assignment below?

> +    entry = (AVDictionaryEntry *)av_dict_get(m, key, NULL, flags);
> +    if (!entry)
> +        return AVERROR(ENOENT);
> +
> +    if (out_key)
> +        *out_key = entry->key;
> +    else
> +        av_free(entry->key);
> +
> +    if (out_value)
> +        *out_value = entry->value;
> +    else
> +        av_free(entry->value);
> +
> +    *entry = m->elems[--m->count];
> +    if (m && !m->count) {

The check for m is completely unnecessary; Coverity will probably
complain about this (because this check implies that m can be NULL, but
then the line above the check would crash).

> +        av_freep(&m->elems);
> +        av_freep(pm);
> +    }
> +    return 0;
> +}
> +
>  static int parse_key_value_pair(AVDictionary **pm, const char **buf,
>                                  const char *key_val_sep, const char *pairs_sep,
>                                  int flags)
> diff --git a/libavutil/dict.h b/libavutil/dict.h
> index 713c9e361a..31d38dabec 100644
> --- a/libavutil/dict.h
> +++ b/libavutil/dict.h
> @@ -172,6 +172,32 @@ int av_dict_set(AVDictionary **pm, const char *key, const char *value, int flags
>   */
>  int av_dict_set_int(AVDictionary **pm, const char *key, int64_t value, int flags);
>  
> +/**
> + * Remove the entry with the given key from the dictionary.
> + *
> + * Search for an entry matching @p key and remove it, if found. Optionally
> + * the found key and/or value can be returned using the @p out_key and
> + * @p out_value arguments.
> + *
> + * If more than one entry matches, only one entry is removed and returned
> + * on each call. Which entry is returned first in that case is undefined.
> + *
> + * @param pm        Pointer to a pointer to a dictionary struct.
> + * @param key       Entry key to match.
> + * @param out_key   Pointer whose pointee will be set to the matched
> + *                  entry key. Must be freed using av_dict_free() by

This is wrong: It must be freed using av_free() (or av_freep()); same below.

> + *                  the caller. May be NULL.
> + * @param out_value Pointer whose pointee will be set to the matched
> + *                  entry value. Must be freed using av_dict_free() by
> + *                  the caller. May be NULL.
> + *
> + * @retval 0                            Success
> + * @retval AVERROR(ENOENT)              No item for the given key found
> + * @retval "Other (negative) AVERROR"   Other failure
> + */
> +int av_dict_pop(AVDictionary **pm, const char *key,
> +                char **out_key, char **out_value, int flags);

It is possible to store multiple entries with the same value in an
AVDictionary. This function is not designed to handle this case, as one
can't tell it which one one wants to pop (IIRC the rest of the API does
not properly support this either, but that is not a reason to add a new
API with this limitation).

Furthermore, while this may be used to avoid a few allocations with the
current implementation, said implementation is not guaranteed to stay
this way.

Finally, are there more intended uses than the one in the second patch?
If so, this could also be simply fixed by strduping the value without
any need for a further API addition.

> +
>  /**
>   * Parse the key/value pairs list and add the parsed entries to a dictionary.
>   *
> diff --git a/libavutil/tests/dict.c b/libavutil/tests/dict.c
> index bececefb31..06d94ecc9a 100644
> --- a/libavutil/tests/dict.c
> +++ b/libavutil/tests/dict.c
> @@ -158,5 +158,43 @@ int main(void)
>      printf("%s\n", e->value);
>      av_dict_free(&dict);
>  
> +    char *key, *val = NULL;
> +    int ret;
> +    printf("\nTesting av_dict_pop() with existing AVDictionaryEntry.key as key\n");
> +    av_dict_set(&dict, "test-key", "test-value", 0);
> +    ret = av_dict_pop(&dict, "test-key", &key, &val, 0);
> +    printf("%s: %s (Return code: %i)\n",
> +        (key) ? key : "(null)",
> +        (val) ? val : "(null)", ret);
> +    e = av_dict_get(dict, "test-key", NULL, 0);
> +    printf("%s\n", (e) ? e->value : "(null)");
> +    av_freep(&key);
> +    av_freep(&val);
> +
> +    printf("\nTesting av_dict_pop() with nonexistent key\n");
> +    ret = av_dict_pop(&dict, "test-key", &key, &val, 0);
> +    printf("%s: %s ",
> +        (key) ? key : "(null)",
> +        (val) ? val : "(null)");
> +    if (ret == AVERROR(ENOENT))
> +        printf("(Return code: ENOENT)\n");
> +    else
> +        printf("(Return code: Unexpected error: %i)\n", ret);
> +    e = av_dict_get(dict, "test-key", NULL, 0);
> +    printf("%s\n", (e) ? e->value : "(null)");
> +    av_freep(&key);
> +    av_freep(&val);
> +
> +    printf("\nTesting av_dict_pop() with prefix key match\n");
> +    av_dict_set(&dict, "prefix-test-key", "test-value", 0);
> +    ret = av_dict_pop(&dict, "prefix-test", &key, &val, AV_DICT_IGNORE_SUFFIX);
> +    printf("%s: %s (Return code: %i)\n",
> +        (key) ? key : "(null)",
> +        (val) ? val : "(null)", ret);
> +    e = av_dict_get(dict, "prefix-test", NULL, AV_DICT_IGNORE_SUFFIX);
> +    printf("%s\n", (e) ? e->value : "(null)");
> +    av_freep(&key);
> +    av_freep(&val);
> +
>      return 0;
>  }
> diff --git a/libavutil/version.h b/libavutil/version.h
> index 17a6d296a6..24af520e08 100644
> --- a/libavutil/version.h
> +++ b/libavutil/version.h
> @@ -79,8 +79,8 @@
>   */
>  
>  #define LIBAVUTIL_VERSION_MAJOR  58
> -#define LIBAVUTIL_VERSION_MINOR  13
> -#define LIBAVUTIL_VERSION_MICRO 101
> +#define LIBAVUTIL_VERSION_MINOR  14
> +#define LIBAVUTIL_VERSION_MICRO 100
>  
>  #define LIBAVUTIL_VERSION_INT   AV_VERSION_INT(LIBAVUTIL_VERSION_MAJOR, \
>                                                 LIBAVUTIL_VERSION_MINOR, \
> diff --git a/tests/ref/fate/dict b/tests/ref/fate/dict
> index 7205e4c845..afa87aca5f 100644
> --- a/tests/ref/fate/dict
> +++ b/tests/ref/fate/dict
> @@ -48,3 +48,15 @@ Testing av_dict_set_int()
>  Testing av_dict_set() with existing AVDictionaryEntry.key as key
>  new val OK
>  new val OK
> +
> +Testing av_dict_pop() with existing AVDictionaryEntry.key as key
> +test-key: test-value (Return code: 0)
> +(null)
> +
> +Testing av_dict_pop() with nonexistent key
> +(null): (null) (Return code: ENOENT)
> +(null)
> +
> +Testing av_dict_pop() with prefix key match
> +prefix-test-key: test-value (Return code: 0)
> +(null)

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