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

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(-)

On date Sunday 2023-06-25 12:49:05 +0200, Marvin Scholz wrote:
> 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;
> +    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) {
> +        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.

Note: I checked the code and we see that in some cases we use `param`
(e.g. in mem.h) but I prefer this format (although apparently not used
in other places) since it looks more consistent with the doxygen
syntax.

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

missing docs for flags, something like:
@param flags flags passed to av_dict_get to look for the entry

should be fine

[...]

Looks good otherwise, thanks.

On 2 Jul 2023, at 10:43, Stefano Sabatini wrote:

> On date Sunday 2023-06-25 12:49:05 +0200, Marvin Scholz wrote:
>> 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;
>> +    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) {
>> +        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.
>
> Note: I checked the code and we see that in some cases we use `param`
> (e.g. in mem.h) but I prefer this format (although apparently not used
> in other places) since it looks more consistent with the doxygen
> syntax.
>
>> + *
>> + * 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
>> + *                  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.
>
> missing docs for flags, something like:
> @param flags flags passed to av_dict_get to look for the entry
>
> should be fine
>

Can whoever ends up merging this maybe add that?
If not, I can send a new revision with this change next weekend or so.

> [...]
>
> Looks good otherwise, thanks.
>
> _______________________________________________
> 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".

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)

On 3 Jul 2023, at 2:18, Andreas Rheinhardt wrote:

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

Indeed, copy paste mistake, sorry.

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

I honestly can't think of a sensible API design for this,
if you have any idea feel free to share.

If the value is known before, using av_dict_pop is useless anyway
as there would be not need to obtain the value if it's already known?

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

I would assume whatever future changes we do, the dictionary would continue
to hold ownership of the items so a way to remove it from the dict
and transferring ownership seems useful to me.

However indeed there are not really any places in code where this is
done, but then again there was no official way to do it before, so
there might be more cases that benefit from this.

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

Marvin Scholz:
> 
> 
> On 3 Jul 2023, at 2:18, Andreas Rheinhardt wrote:
> 
>> 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.
> 
> Indeed, copy paste mistake, sorry.
> 
>>
>>> + *                  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).
> 
> I honestly can't think of a sensible API design for this,
> if you have any idea feel free to share.
> 

The only way I can think of that allows this is for the user to pass a
pointer to a (const) AVDictionaryEntry, namely the entry that the user
wishes to pop. This is of course cumbersome, because it would be two
function calls.

> If the value is known before, using av_dict_pop is useless anyway
> as there would be not need to obtain the value if it's already known?
> 

I don't really understand this. Having pointers to the strings is not
the same as owning the strings.

>>
>> Furthermore, while this may be used to avoid a few allocations with the
>> current implementation, said implementation is not guaranteed to stay
>> this way.
>>
> 
> I would assume whatever future changes we do, the dictionary would continue
> to hold ownership of the items so a way to remove it from the dict
> and transferring ownership seems useful to me.
> 

Of course the dict has ownership of the strings referred to by the
dictionary. But for this function to work as you wrote it, the strings
referred to by each entry need to be e.g. separately allocated; this may
or may not change (e.g. we may only allocate one key for all the
AVDictionaryEntries with the same key in the future).

> However indeed there are not really any places in code where this is
> done, but then again there was no official way to do it before, so
> there might be more cases that benefit from this.

Do you know whether there are library users that would benefit from this?

> 
>> 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)
>>

On 3 Jul 2023, at 20:02, Andreas Rheinhardt wrote:

> Marvin Scholz:
>>
>>
>> On 3 Jul 2023, at 2:18, Andreas Rheinhardt wrote:
>>
>>> 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.
>>
>> Indeed, copy paste mistake, sorry.
>>
>>>
>>>> + *                  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).
>>
>> I honestly can't think of a sensible API design for this,
>> if you have any idea feel free to share.
>>
>
> The only way I can think of that allows this is for the user to pass a
> pointer to a (const) AVDictionaryEntry, namely the entry that the user
> wishes to pop. This is of course cumbersome, because it would be two
> function calls.
>
>> If the value is known before, using av_dict_pop is useless anyway
>> as there would be not need to obtain the value if it's already known?
>>
>
> I don't really understand this. Having pointers to the strings is not
> the same as owning the strings.
>

Indeed, I get what you mean now, that looking up and popping could
be two separate steps, then it would make sense but somewhat annoying
to use API wise.

>>>
>>> Furthermore, while this may be used to avoid a few allocations with the
>>> current implementation, said implementation is not guaranteed to stay
>>> this way.
>>>
>>
>> I would assume whatever future changes we do, the dictionary would continue
>> to hold ownership of the items so a way to remove it from the dict
>> and transferring ownership seems useful to me.
>>
>
> Of course the dict has ownership of the strings referred to by the
> dictionary. But for this function to work as you wrote it, the strings
> referred to by each entry need to be e.g. separately allocated; this may
> or may not change (e.g. we may only allocate one key for all the
> AVDictionaryEntries with the same key in the future).

Thats a good point, the main reason I included the key at all
was that with some flag values it is possible to pop values
for which you don't match the whole key and might want to know
it.

>
>> However indeed there are not really any places in code where this is
>> done, but then again there was no official way to do it before, so
>> there might be more cases that benefit from this.
>
> Do you know whether there are library users that would benefit from this?
>

No,
main reason for proposing this API was so I can make the av_dict_get
return const as it should be. I can strdup instead, if you think thats
better. But given we have a way to hand over string values to the dictionary
without copy, it just seemed logical to have some API for the reverse.

So I am open for new API proposals/designs or can use strdup, it just would
be good if we can agree on something before I spend more time on this and have
fundamentally changed things several times.

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

Quoting Andreas Rheinhardt (2023-07-03 20:02:25)
> Marvin Scholz:
> > I honestly can't think of a sensible API design for this,
> > if you have any idea feel free to share.
> > 
> 
> The only way I can think of that allows this is for the user to pass a
> pointer to a (const) AVDictionaryEntry, namely the entry that the user
> wishes to pop. This is of course cumbersome, because it would be two
> function calls.

We could start guaranteeing that entries with the same key are always
returned in insertion order. Now that I think of it there's some code in
ffmpeg CLI that relies on that.

Anton Khirnov:
> Quoting Andreas Rheinhardt (2023-07-03 20:02:25)
>> Marvin Scholz:
>>> I honestly can't think of a sensible API design for this,
>>> if you have any idea feel free to share.
>>>
>>
>> The only way I can think of that allows this is for the user to pass a
>> pointer to a (const) AVDictionaryEntry, namely the entry that the user
>> wishes to pop. This is of course cumbersome, because it would be two
>> function calls.
> 
> We could start guaranteeing that entries with the same key are always
> returned in insertion order. Now that I think of it there's some code in
> ffmpeg CLI that relies on that.
> 

IIRC this is not true correctly, because removing an entry changes the
order of the remaining entries (we don't memmove the remaining entries
into the new position; instead we move the last entry to the now vacant
slot).

- Andreas

Quoting Andreas Rheinhardt (2023-10-20 16:00:45)
> Anton Khirnov:
> > Quoting Andreas Rheinhardt (2023-07-03 20:02:25)
> >> Marvin Scholz:
> >>> I honestly can't think of a sensible API design for this,
> >>> if you have any idea feel free to share.
> >>>
> >>
> >> The only way I can think of that allows this is for the user to pass a
> >> pointer to a (const) AVDictionaryEntry, namely the entry that the user
> >> wishes to pop. This is of course cumbersome, because it would be two
> >> function calls.
> > 
> > We could start guaranteeing that entries with the same key are always
> > returned in insertion order. Now that I think of it there's some code in
> > ffmpeg CLI that relies on that.
> > 
> 
> IIRC this is not true correctly, because removing an entry changes the
> order of the remaining entries (we don't memmove the remaining entries
> into the new position; instead we move the last entry to the now vacant
> slot).

I am aware - that code is strictly speaking incorrect. It happens to
work because nothing ever gets removed from the option dicts. We could
consider changing the internal representation somehow, so that all
values for a key are grouped together.

Anton Khirnov:
> Quoting Andreas Rheinhardt (2023-10-20 16:00:45)
>> Anton Khirnov:
>>> Quoting Andreas Rheinhardt (2023-07-03 20:02:25)
>>>> Marvin Scholz:
>>>>> I honestly can't think of a sensible API design for this,
>>>>> if you have any idea feel free to share.
>>>>>
>>>>
>>>> The only way I can think of that allows this is for the user to pass a
>>>> pointer to a (const) AVDictionaryEntry, namely the entry that the user
>>>> wishes to pop. This is of course cumbersome, because it would be two
>>>> function calls.
>>>
>>> We could start guaranteeing that entries with the same key are always
>>> returned in insertion order. Now that I think of it there's some code in
>>> ffmpeg CLI that relies on that.
>>>
>>
>> IIRC this is not true correctly, because removing an entry changes the
>> order of the remaining entries (we don't memmove the remaining entries
>> into the new position; instead we move the last entry to the now vacant
>> slot).
> 
> I am aware - that code is strictly speaking incorrect. It happens to
> work because nothing ever gets removed from the option dicts. We could
> consider changing the internal representation somehow, so that all
> values for a key are grouped together.
> 

We could also just memmove the remaining entries.

- Andreas