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-requ...@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-requ...@ffmpeg.org with subject "unsubscribe".