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

[James Almer]

On 5/26/2023 6:11 AM, Marvin Scholz wrote:
> 
> 
> 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.

I found it pretty powerful and useful. It's currently used in the 
dts2pts bsf, and i wrote a dynamic size AVBufferPool implementation with 
it that was never upstreamed.