From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org [79.124.17.100]) by master.gitmailbox.com (Postfix) with ESMTP id 2869B45865 for ; Fri, 26 May 2023 20:02:47 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id C4B1A68C1D4; Fri, 26 May 2023 23:02:44 +0300 (EEST) Received: from relay9-d.mail.gandi.net (relay9-d.mail.gandi.net [217.70.183.199]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 681FD68BEDF for ; Fri, 26 May 2023 23:02:38 +0300 (EEST) X-GND-Sasl: michael@niedermayer.cc Received: by mail.gandi.net (Postfix) with ESMTPSA id AA810FF807 for ; Fri, 26 May 2023 20:02:37 +0000 (UTC) Date: Fri, 26 May 2023 22:02:36 +0200 From: Michael Niedermayer To: FFmpeg development discussions and patches Message-ID: <20230526200236.GO1391451@pb2> References: <20230501114456.13898-1-epirat07@gmail.com> <20230521235235.GC14958@mariano> <8CA999C7-1502-4FAB-ACD9-9BC02ED68058@gmail.com> <20230526060544.GB3241@mariano> MIME-Version: 1.0 In-Reply-To: Subject: Re: [FFmpeg-devel] [PATCH 1/3] avutil/dict: add av_dict_pop X-BeenThere: ffmpeg-devel@ffmpeg.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: FFmpeg development discussions and patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: FFmpeg development discussions and patches Content-Type: multipart/mixed; boundary="===============2587014425602000963==" Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" Archived-At: List-Archive: List-Post: --===============2587014425602000963== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="0Fhs++QIS2o6H55x" Content-Disposition: inline --0Fhs++QIS2o6H55x Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, May 26, 2023 at 11:11:48AM +0200, Marvin Scholz wrote: >=20 >=20 > On 26 May 2023, at 8:05, Stefano Sabatini wrote: >=20 > > 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 ch= ar *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 =3D *pm; > >>>> + AVDictionaryEntry *entry =3D NULL; > >>>> + entry =3D (AVDictionaryEntry *)av_dict_get(m, key, NULL, flags); > >>>> + if (!entry) > >>>> + return AVERROR(ENOENT); > >>>> + > >>>> + if (out_key) > >>>> + *out_key =3D entry->key; > >>>> + else > >>>> + av_free(entry->key); > >>>> + > >>>> + if (out_value) > >>>> + *out_value =3D entry->value; > >>>> + else > >>>> + av_free(entry->value); > >>>> + > >>>> + *entry =3D 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. Opti= onally > >>> > >>> 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). > > >=20 > Ok I will submit a new patch with it removed. >=20 > > [...] > > > > Should we also support the case with multiple same-key values? >=20 > I don't see what could be improved there. You just call it multiple times, > or what do you mean? >=20 > > > > 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). >=20 > 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. >=20 > > > > Another general question, since I see that dict.h is deprecated, do > > you think it might be possible to switch to tree.h? >=20 > 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 preser= ved > in most cases=E2=80=A6 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 >=20 > 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 effici= ent 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 [...] --=20 Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB No snowflake in an avalanche ever feels responsible. -- Voltaire --0Fhs++QIS2o6H55x Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iF0EABEIAB0WIQSf8hKLFH72cwut8TNhHseHBAsPqwUCZHEQWAAKCRBhHseHBAsP q640AKCIiE04UbbG/aTZLfJ0IS8zLrRgegCfYoWtXJfJm9UR+30niX318dlROwk= =4HoY -----END PGP SIGNATURE----- --0Fhs++QIS2o6H55x-- --===============2587014425602000963== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ 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". --===============2587014425602000963==--