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

[Stefano Sabatini]

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

[...]

Should we also support the case with multiple same-key values?

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

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