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

[Michael Niedermayer]

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.
It may be a good idea to allow randomizing the order for fate tests though
independant of any change to AVDictionary


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

thx

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

No snowflake in an avalanche ever feels responsible. -- Voltaire
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20230526/0e9ebc04/attachment.sig>