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

[Marvin Scholz <epirat07@gmail.com>]

On 26 May 2023, at 22:02, Michael Niedermayer wrote:

> On Fri, May 26, 2023 at 11:11:48AM +0200, 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…
>
> There is no gurantee on insertion order preservation. And even with the
> current implementation any code depening on that is broken.

Good to know

> It may be a good idea to allow randomizing the order for fate tests though
> independant of any change to AVDictionary
>

Good idea

>
>>
>> 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 think AVDictionary should be made to internally use something more efficient
> like tree.c/h if possible.
>
> Only if its not possible within the API of AVDictionary would a new API be
> needed. That new API must be similarly easy to use as AVDictionary
>

I agree with that, I just meant it is not possible to replace usages of
AVDictionary with tree.h functions directly easily.

Regarding if all use-cases of AVDictionary can be covered if we use the
tree functions internally, I would probably need to discuss on IRC with
you.

Anyway though this discussion seems to derail a bit from the patch
at hand which is unrelated to a possible change of AVDictionary internals
in the future.

> thx
>
> [...]
> -- 
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> No snowflake in an avalanche ever feels responsible. -- Voltaire
> _______________________________________________
> 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".