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

[Marvin Scholz <epirat07@gmail.com>]

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