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 4325546B77 for ; Mon, 3 Jul 2023 22:41:22 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id EFCB768C5B4; Tue, 4 Jul 2023 01:41:19 +0300 (EEST) Received: from mail-ed1-f50.google.com (mail-ed1-f50.google.com [209.85.208.50]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 2B7A168C581 for ; Tue, 4 Jul 2023 01:41:14 +0300 (EEST) Received: by mail-ed1-f50.google.com with SMTP id 4fb4d7f45d1cf-51dfa0771cdso3370096a12.2 for ; Mon, 03 Jul 2023 15:41:14 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1688424073; x=1691016073; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:to:from:from:to:cc:subject:date:message-id :reply-to; bh=Sgz3kFX/yQel6gBePKP3Th2DPktwAry85oJPeiPdGLo=; b=iPlmJaW3D8gLfI4N8xcor1Xy40PjIYzVsdYMYSZf5RtT5oq2F8xn4wRy4hVoSAjod+ 9U7IYes61XTrKWlwHx4Exeidv0WS3QLYDyLusbOy1mu4JPBsiMgHo+60nNuAdFPVI4eL oUmMU55c8xQfvDavYokAKt4Riq0HaLbzvvMndlBbY4+KPll0zUdd2SsURcHDfLc8dTs5 5HtoJkGtkyNWDD1BTF49MU/qV7my8N2BnqLo+qAB6XhH7AOS9x/EYZp/2HUUTmw7+jxl O39TC1qr9f4E6OP96GndxdMvj8VFI9LO3nGU+YKGM/IR+q9Y5f86zufIOwMi2PBoBmfK 3BQQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1688424073; x=1691016073; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=Sgz3kFX/yQel6gBePKP3Th2DPktwAry85oJPeiPdGLo=; b=VpsSfPYSMnMNZ3Gp4jMuQK3eosMqC39M7bUh9McyyqYwkMGruL/Mubw5wzN03a739o xjTUvFflXMuNK8/ey/VjFOkJKYMSSvbQT+eQqEXT/B3dfHapKAeKJ6WpOdKfPUIZqGvE lIknXmCr7Z1RNazW4KmiNcvk9Vi0SPKZNDy7B1tRDjlco4WzBwGY58ZNYJ91bnncjGVh qH8f5qrFHbQCpd2j9thYqyp0C3X4Vz42ftqpcT9PkJZefK2p6lKw3KxPVXlIBDESQuT8 vuALkrmUIPtBTIV778yMAHtJavVrG2yMyM49UjfNmiEg0je9W2q8ji7r0us/zw/chVsV yFiw== X-Gm-Message-State: ABy/qLYxkFoMpe4WOAEJIYXDEpq3HVZkJF+JrKq6LuvX4eenXBRgxmKq nQOTfg81SZTWujZzNlIpSsd8nFpE4VUmjg== X-Google-Smtp-Source: APBJJlHz4tBN7rOnoOGPe567QE2EIeFxLYwFXqzCS3Wf9SNLkpGLyfEQJUZtfgHq2d7wIycpgtZplA== X-Received: by 2002:a17:906:fa18:b0:992:b3a3:8202 with SMTP id lo24-20020a170906fa1800b00992b3a38202mr8487009ejb.19.1688424073157; Mon, 03 Jul 2023 15:41:13 -0700 (PDT) Received: from [192.168.1.104] (84-112-104-25.cable.dynamic.surfer.at. [84.112.104.25]) by smtp.gmail.com with ESMTPSA id lr3-20020a170906fb8300b00973f1cd586fsm12587282ejb.1.2023.07.03.15.41.12 for (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Mon, 03 Jul 2023 15:41:12 -0700 (PDT) From: Marvin Scholz To: FFmpeg development discussions and patches Date: Tue, 04 Jul 2023 00:41:11 +0200 X-Mailer: MailMate (1.14r5964) Message-ID: <76D0F9DC-59E5-4903-A6E0-EA9EA01F9EE2@gmail.com> In-Reply-To: References: <20230501114456.13898-1-epirat07@gmail.com> <20230625104907.53071-1-epirat07@gmail.com> <7D4C10CB-A3F9-4ECD-916B-417F3F538913@gmail.com> MIME-Version: 1.0 Subject: Re: [FFmpeg-devel] [PATCH v2 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: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" Archived-At: List-Archive: List-Post: On 3 Jul 2023, at 20:02, Andreas Rheinhardt wrote: > Marvin Scholz: >> >> >> On 3 Jul 2023, at 2:18, Andreas Rheinhardt wrote: >> >>> Marvin Scholz: >>>> This new API allows to remove an entry and obtain ownership of the >>>> key/value that was associated with the removed entry. >>>> --- >>>> >>>> Changes since v1: >>>> - Clarify documentation about av_free having to be used. >>>> - Fix fate test to not rely on specific error code value >>>> >>>> doc/APIchanges | 4 ++++ >>>> libavutil/dict.c | 27 +++++++++++++++++++++++++++ >>>> libavutil/dict.h | 26 ++++++++++++++++++++++++++ >>>> libavutil/tests/dict.c | 38 ++++++++++++++++++++++++++++++++++++++ >>>> libavutil/version.h | 4 ++-- >>>> tests/ref/fate/dict | 12 ++++++++++++ >>>> 6 files changed, 109 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/doc/APIchanges b/doc/APIchanges >>>> index f040211f7d..d55821f682 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-06-02 - xxxxxxxxxx - lavu 58.14.100 - dict.h >>>> + Add av_dict_pop() to remove an entry from a dict >>>> + and get ownership of the removed key/value. >>>> + >>>> 2023-05-29 - xxxxxxxxxx - lavc 60.16.100 - avcodec.h codec_id.h >>>> Add AV_CODEC_ID_EVC, FF_PROFILE_EVC_BASELINE, and FF_PROFILE_EVC_MAIN. >>>> >>>> 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; >>> Why don't you merge this initialization and the assignment below? >>> >>>> + 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) { >>> >>> The check for m is completely unnecessary; Coverity will probably >>> complain about this (because this check implies that m can be NULL, but >>> then the line above the check would crash). >>> >>>> + 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..31d38dabec 100644 >>>> --- a/libavutil/dict.h >>>> +++ b/libavutil/dict.h >>>> @@ -172,6 +172,32 @@ 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 @p key and remove it, if found. Optionally >>>> + * the found key and/or value can be returned using the @p out_key and >>>> + * @p 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 using av_dict_free() by >>> >>> This is wrong: It must be freed using av_free() (or av_freep()); same below. >> >> Indeed, copy paste mistake, sorry. >> >>> >>>> + * the caller. May be NULL. >>>> + * @param out_value Pointer whose pointee will be set to the matched >>>> + * entry value. Must be freed using av_dict_free() by >>>> + * the caller. May be NULL. >>>> + * >>>> + * @retval 0 Success >>>> + * @retval AVERROR(ENOENT) No item for the given key found >>>> + * @retval "Other (negative) AVERROR" Other failure >>>> + */ >>>> +int av_dict_pop(AVDictionary **pm, const char *key, >>>> + char **out_key, char **out_value, int flags); >>> >>> It is possible to store multiple entries with the same value in an >>> AVDictionary. This function is not designed to handle this case, as one >>> can't tell it which one one wants to pop (IIRC the rest of the API does >>> not properly support this either, but that is not a reason to add a new >>> API with this limitation). >> >> I honestly can't think of a sensible API design for this, >> if you have any idea feel free to share. >> > > The only way I can think of that allows this is for the user to pass a > pointer to a (const) AVDictionaryEntry, namely the entry that the user > wishes to pop. This is of course cumbersome, because it would be two > function calls. > >> If the value is known before, using av_dict_pop is useless anyway >> as there would be not need to obtain the value if it's already known? >> > > I don't really understand this. Having pointers to the strings is not > the same as owning the strings. > Indeed, I get what you mean now, that looking up and popping could be two separate steps, then it would make sense but somewhat annoying to use API wise. >>> >>> Furthermore, while this may be used to avoid a few allocations with the >>> current implementation, said implementation is not guaranteed to stay >>> this way. >>> >> >> I would assume whatever future changes we do, the dictionary would continue >> to hold ownership of the items so a way to remove it from the dict >> and transferring ownership seems useful to me. >> > > Of course the dict has ownership of the strings referred to by the > dictionary. But for this function to work as you wrote it, the strings > referred to by each entry need to be e.g. separately allocated; this may > or may not change (e.g. we may only allocate one key for all the > AVDictionaryEntries with the same key in the future). Thats a good point, the main reason I included the key at all was that with some flag values it is possible to pop values for which you don't match the whole key and might want to know it. > >> However indeed there are not really any places in code where this is >> done, but then again there was no official way to do it before, so >> there might be more cases that benefit from this. > > Do you know whether there are library users that would benefit from this? > No, main reason for proposing this API was so I can make the av_dict_get return const as it should be. I can strdup instead, if you think thats better. But given we have a way to hand over string values to the dictionary without copy, it just seemed logical to have some API for the reverse. So I am open for new API proposals/designs or can use strdup, it just would be good if we can agree on something before I spend more time on this and have fundamentally changed things several times. >> >>> Finally, are there more intended uses than the one in the second patch? >>> If so, this could also be simply fixed by strduping the value without >>> any need for a further API addition. >>> >>>> + >>>> /** >>>> * 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..06d94ecc9a 100644 >>>> --- a/libavutil/tests/dict.c >>>> +++ b/libavutil/tests/dict.c >>>> @@ -158,5 +158,43 @@ 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 ", >>>> + (key) ? key : "(null)", >>>> + (val) ? val : "(null)"); >>>> + if (ret == AVERROR(ENOENT)) >>>> + printf("(Return code: ENOENT)\n"); >>>> + else >>>> + printf("(Return code: Unexpected error: %i)\n", 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 17a6d296a6..24af520e08 100644 >>>> --- a/libavutil/version.h >>>> +++ b/libavutil/version.h >>>> @@ -79,8 +79,8 @@ >>>> */ >>>> >>>> #define LIBAVUTIL_VERSION_MAJOR 58 >>>> -#define LIBAVUTIL_VERSION_MINOR 13 >>>> -#define LIBAVUTIL_VERSION_MICRO 101 >>>> +#define LIBAVUTIL_VERSION_MINOR 14 >>>> +#define LIBAVUTIL_VERSION_MICRO 100 >>>> >>>> #define LIBAVUTIL_VERSION_INT AV_VERSION_INT(LIBAVUTIL_VERSION_MAJOR, \ >>>> LIBAVUTIL_VERSION_MINOR, \ >>>> diff --git a/tests/ref/fate/dict b/tests/ref/fate/dict >>>> index 7205e4c845..afa87aca5f 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: ENOENT) >>>> +(null) >>>> + >>>> +Testing av_dict_pop() with prefix key match >>>> +prefix-test-key: test-value (Return code: 0) >>>> +(null) >>> > > _______________________________________________ > 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". _______________________________________________ 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".