| Message ID | 20230501114456.13898-1-epirat07@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [FFmpeg-devel,1/3] avutil/dict: add av_dict_pop | expand |
| Context | Check | Description |
|---|---|---|
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
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. > --- > 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? > + return 0; > +} > + > static int parse_key_value_pair(AVDictionary **pm, const char **buf, > const char *key_val_sep, const char *pairs_sep, > int flags) > diff --git a/libavutil/dict.h b/libavutil/dict.h > index 713c9e361a..b2ab55a026 100644 > --- a/libavutil/dict.h > +++ b/libavutil/dict.h > @@ -172,6 +172,26 @@ int av_dict_set(AVDictionary **pm, const char *key, const char *value, int flags > */ > int av_dict_set_int(AVDictionary **pm, const char *key, int64_t value, int flags); > > +/** > + * 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). > + * the found key and/or value can be returned using the `out_key`/`out_value` > + * arguments.
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. >> + return 0; >> +} >> + >> static int parse_key_value_pair(AVDictionary **pm, const char **buf, >> const char *key_val_sep, const char *pairs_sep, >> int flags) >> diff --git a/libavutil/dict.h b/libavutil/dict.h >> index 713c9e361a..b2ab55a026 100644 >> --- a/libavutil/dict.h >> +++ b/libavutil/dict.h >> @@ -172,6 +172,26 @@ int av_dict_set(AVDictionary **pm, const char *key, const char *value, int flags >> */ >> int av_dict_set_int(AVDictionary **pm, const char *key, int64_t value, int flags); >> >> +/** >> + * 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. >> + * the found key and/or value can be returned using the `out_key`/`out_value` >> + * arguments. > > _______________________________________________ > 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".
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?
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".
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 [...]
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.
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".
On date Friday 2023-05-26 11:11:48 +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: [...] > > 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. OK, anyway this is pretty unrelated to the current patch (might be done as a followup). I was checking the current documentation and it's missing some important information (I'll try to send a patch to fix that later). [...] About your patch, please mention that the pop operation is destructive. We probably want to make that behavior configurable through flags, but I'm fine to keep the current behavior for consistency with the set(NULL) operation. [...] Thanks (sorry again for the slow reply).
On 4 Jun 2023, at 16:25, Stefano Sabatini wrote: > On date Friday 2023-05-26 11:11:48 +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: > [...] >>> 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. > > OK, anyway this is pretty unrelated to the current patch (might be > done as a followup). I was checking the current documentation and it's > missing some important information (I'll try to send a patch to fix > that later). > > [...] > > About your patch, please mention that the pop operation is > destructive. We probably want to make that behavior configurable > through flags, but I'm fine to keep the current behavior for > consistency with the set(NULL) operation. What do you mean by "destructive"? > > [...] > > Thanks (sorry again for the slow reply). > _______________________________________________ > 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".
Il dom 4 giu 2023, 16:34 Marvin Scholz <epirat07@gmail.com> ha scritto: > On 4 Jun 2023, at 16:25, Stefano Sabatini wrote: > [...] > > About your patch, please mention that the pop operation is > > destructive. We probably want to make that behavior configurable > > through flags, but I'm fine to keep the current behavior for > > consistency with the set(NULL) operation. > > What do you mean by "destructive"? > I mean that the dictionary structure will be freed when the last element is popped.
Quoting Marvin Scholz (2023-05-01 13:44:54) > diff --git a/libavutil/dict.h b/libavutil/dict.h > index 713c9e361a..b2ab55a026 100644 > --- a/libavutil/dict.h > +++ b/libavutil/dict.h > @@ -172,6 +172,26 @@ int av_dict_set(AVDictionary **pm, const char *key, const char *value, int flags > */ > int av_dict_set_int(AVDictionary **pm, const char *key, int64_t value, int flags); > > +/** > + * Remove the entry with the given key from the dictionary. > + * > + * Search for an entry matching `key` and remove it, if found. Optionally > + * the found key and/or value can be returned using the `out_key`/`out_value` > + * arguments. > + * > + * If more than one entry matches, only one entry is removed and returned > + * on each call. Which entry is returned first in that case is undefined. > + * > + * @param pm Pointer to a pointer to a dictionary struct. > + * @param key Entry key to match. > + * @param out_key Pointer whose pointee will be set to the matched > + * entry key. Must be freed by the caller. May be NULL. > + * @param out_value Pointer whose pointee will be set to the matched > + * entry value. Must be freed by the caller. May be NULL. freed using av_free() Should also mention what the function's return value is. > diff --git a/tests/ref/fate/dict b/tests/ref/fate/dict > index 7205e4c845..bdb097cb03 100644 > --- a/tests/ref/fate/dict > +++ b/tests/ref/fate/dict > @@ -48,3 +48,15 @@ Testing av_dict_set_int() > Testing av_dict_set() with existing AVDictionaryEntry.key as key > new val OK > new val OK > + > +Testing av_dict_pop() with existing AVDictionaryEntry.key as key > +test-key: test-value (Return code: 0) > +(null) > + > +Testing av_dict_pop() with nonexistent key > +(null): (null) (Return code: -2) I don't think anything guarantees that ENOENT has to be -2 on all platforms we support.
Quoting Stefano Sabatini (2023-06-05 10:08:07) > Il dom 4 giu 2023, 16:34 Marvin Scholz <epirat07@gmail.com> ha scritto: > > > On 4 Jun 2023, at 16:25, Stefano Sabatini wrote: > > [...] > > > About your patch, please mention that the pop operation is > > > destructive. We probably want to make that behavior configurable > > > through flags, but I'm fine to keep the current behavior for > > > consistency with the set(NULL) operation. > > > > What do you mean by "destructive"? > > > > I mean that the dictionary structure will be freed when the last element > is popped. It is explicitly mentioned in the documentation that NULL is a valid representation of an empty dictionary, so I don't think there's anything destructive here.
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); + } + return 0; +} + static int parse_key_value_pair(AVDictionary **pm, const char **buf, const char *key_val_sep, const char *pairs_sep, int flags) diff --git a/libavutil/dict.h b/libavutil/dict.h index 713c9e361a..b2ab55a026 100644 --- a/libavutil/dict.h +++ b/libavutil/dict.h @@ -172,6 +172,26 @@ int av_dict_set(AVDictionary **pm, const char *key, const char *value, int flags */ int av_dict_set_int(AVDictionary **pm, const char *key, int64_t value, int flags); +/** + * Remove the entry with the given key from the dictionary. + * + * Search for an entry matching `key` and remove it, if found. Optionally + * the found key and/or value can be returned using the `out_key`/`out_value` + * arguments. + * + * If more than one entry matches, only one entry is removed and returned + * on each call. Which entry is returned first in that case is undefined. + * + * @param pm Pointer to a pointer to a dictionary struct. + * @param key Entry key to match. + * @param out_key Pointer whose pointee will be set to the matched + * entry key. Must be freed by the caller. May be NULL. + * @param out_value Pointer whose pointee will be set to the matched + * entry value. Must be freed by the caller. May be NULL. + */ +int av_dict_pop(AVDictionary **pm, const char *key, + char **out_key, char **out_value, int flags); + /** * Parse the key/value pairs list and add the parsed entries to a dictionary. * diff --git a/libavutil/tests/dict.c b/libavutil/tests/dict.c index bececefb31..0652794b97 100644 --- a/libavutil/tests/dict.c +++ b/libavutil/tests/dict.c @@ -158,5 +158,39 @@ int main(void) printf("%s\n", e->value); av_dict_free(&dict); + char *key, *val = NULL; + int ret; + printf("\nTesting av_dict_pop() with existing AVDictionaryEntry.key as key\n"); + av_dict_set(&dict, "test-key", "test-value", 0); + ret = av_dict_pop(&dict, "test-key", &key, &val, 0); + printf("%s: %s (Return code: %i)\n", + (key) ? key : "(null)", + (val) ? val : "(null)", ret); + e = av_dict_get(dict, "test-key", NULL, 0); + printf("%s\n", (e) ? e->value : "(null)"); + av_freep(&key); + av_freep(&val); + + printf("\nTesting av_dict_pop() with nonexistent key\n"); + ret = av_dict_pop(&dict, "test-key", &key, &val, 0); + printf("%s: %s (Return code: %i)\n", + (key) ? key : "(null)", + (val) ? val : "(null)", ret); + e = av_dict_get(dict, "test-key", NULL, 0); + printf("%s\n", (e) ? e->value : "(null)"); + av_freep(&key); + av_freep(&val); + + printf("\nTesting av_dict_pop() with prefix key match\n"); + av_dict_set(&dict, "prefix-test-key", "test-value", 0); + ret = av_dict_pop(&dict, "prefix-test", &key, &val, AV_DICT_IGNORE_SUFFIX); + printf("%s: %s (Return code: %i)\n", + (key) ? key : "(null)", + (val) ? val : "(null)", ret); + e = av_dict_get(dict, "prefix-test", NULL, AV_DICT_IGNORE_SUFFIX); + printf("%s\n", (e) ? e->value : "(null)"); + av_freep(&key); + av_freep(&val); + return 0; } diff --git a/libavutil/version.h b/libavutil/version.h index 40f92af055..b8d1ef06a8 100644 --- a/libavutil/version.h +++ b/libavutil/version.h @@ -79,7 +79,7 @@ */ #define LIBAVUTIL_VERSION_MAJOR 58 -#define LIBAVUTIL_VERSION_MINOR 6 +#define LIBAVUTIL_VERSION_MINOR 7 #define LIBAVUTIL_VERSION_MICRO 100 #define LIBAVUTIL_VERSION_INT AV_VERSION_INT(LIBAVUTIL_VERSION_MAJOR, \ diff --git a/tests/ref/fate/dict b/tests/ref/fate/dict index 7205e4c845..bdb097cb03 100644 --- a/tests/ref/fate/dict +++ b/tests/ref/fate/dict @@ -48,3 +48,15 @@ Testing av_dict_set_int() Testing av_dict_set() with existing AVDictionaryEntry.key as key new val OK new val OK + +Testing av_dict_pop() with existing AVDictionaryEntry.key as key +test-key: test-value (Return code: 0) +(null) + +Testing av_dict_pop() with nonexistent key +(null): (null) (Return code: -2) +(null) + +Testing av_dict_pop() with prefix key match +prefix-test-key: test-value (Return code: 0) +(null)