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

[Marvin Scholz <epirat07@gmail.com>]

On 26 May 2023, at 8:05, Stefano Sabatini wrote:

> On date Monday 2023-05-22 11:23:24 +0200, Marvin Scholz wrote:
>> On 22 May 2023, at 1:52, Stefano Sabatini wrote:
>>
>>> On date Monday 2023-05-01 13:44:54 +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.
>>
>> Thanks for the review!
>>
>>>> ---
>>>>  doc/APIchanges         |  4 ++++
>>>>  libavutil/dict.c       | 27 +++++++++++++++++++++++++++
>>>>  libavutil/dict.h       | 20 ++++++++++++++++++++
>>>>  libavutil/tests/dict.c | 34 ++++++++++++++++++++++++++++++++++
>>>>  libavutil/version.h    |  2 +-
>>>>  tests/ref/fate/dict    | 12 ++++++++++++
>>>>  6 files changed, 98 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/doc/APIchanges b/doc/APIchanges
>>>> index 0b609e3d3b..5b807873b7 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-04-29 - xxxxxxxxxx - lavu 58.7.100 - dict.c
>>>> +  Add av_dict_pop() to remove an entry from a dict
>>>> +  and get ownership of the removed key/value.
>>>> +
>>>>  2023-04-10 - xxxxxxxxxx - lavu 58.6.100 - frame.h
>>>>    av_frame_get_plane_buffer() now accepts const AVFrame*.
>>>>
>>>> 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);
>>>> +    }
>>>
>>> I'm not sure this is the right behavior. Should we clear the
>>> dictionary when it is empty? What if you need to refill it later?
>>>
>>
>
>> Thats the same behaviour as if you use av_dict_set to remove all items
>> and IMO this should be consistent.
>
>> Additionally NULL means an empty AVDictionary, suddenly
>> having a non-NULL but empty dictionary seems like a very bad idea.
>
> Sorry for the slow reply, I see.
>
> [...]
>>>> +/**
>>>> + * Remove the entry with the given key from the dictionary.
>>>> + *
>>>
>>>> + * Search for an entry matching `key` and remove it, if found. Optionally
>>>
>>> Not sure the `foo` syntax is supported by doxygen (and probably we
>>> should eschew it for consistency with the other doxys).
>>>
>>
>> I tested it locally and it works fine and its much more readable than the
>> alternatives.
>>
>> However if you feel it should be removed I am happy to do that, I have no
>> strong opinions there.
>
> Please let's avoid to add more syntax variance (also I'm not sure when
> the `var` syntax was introduced).
>

Ok I will submit a new patch with it removed.

> [...]
>
> Should we also support the case with multiple same-key values?

I don't see what could be improved there. You just call it multiple times,
or what do you mean?

>
> Also maybe we should mention that this operation might alterate the
> order of the entries (unless we add a new flag to shift the
> trailing data when an entry is removed).

We currently IIRC nowhere give guarantees on the order of items in the
dict, which we probably should keep that way especially in regards to
your next point.

>
> Another general question, since I see that dict.h is deprecated, do
> you think it might be possible to switch to tree.h?

To internally use more efficient ways to handle entries would require
some big changes and lots of tests with all users to ensure they do not
rely on current undocumented behaviours like insertion order being preserved
in most cases…

Generally completely deprecating AVDictionary does not sound feasible at all
and the tree API is way too cumbersome and low-level right now to use it
as a replacement IMO.

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