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 425CF46B17 for ; Mon, 3 Jul 2023 09:12:01 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 6979568C526; Mon, 3 Jul 2023 12:11:58 +0300 (EEST) Received: from mail-ed1-f42.google.com (mail-ed1-f42.google.com [209.85.208.42]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id D332868C4DE for ; Mon, 3 Jul 2023 12:11:51 +0300 (EEST) Received: by mail-ed1-f42.google.com with SMTP id 4fb4d7f45d1cf-51d7f350758so5458341a12.3 for ; Mon, 03 Jul 2023 02:11:51 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1688375511; x=1690967511; 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=Y82tbK5YpzxWq27w8zOML9VXaSAgh4EiyYLUuLg/NIc=; b=dt7dzXbTnkGDhmVal/5tXJ58Xm+mQP/DDk0F/xtzWyOq6FjKOqKhbqOhr+tcc1ShPA 2KeK0N39TbjL6jkdQJoKC/1NA3xkWcwzfby4APCrGjXmB6mcDHnVp22pdAr6eOGFb/H6 6CfNAhaAoNJ/7ySR89+TUkVaNjKkZCBJAOTYnRq7Nigy7Rsq+W4XKF+4Q2UqZaq7kzF+ mZxysaQirsqybR8g3u8lZVChS2gho9fJOdQoD1lkgg8GZokA5OXBkQRprfgRlQef1aOI rom5yGWegI/q1cI4zLazjTRUarrgij1ZJrpMBHqhNeQ+y/5r/oglZz7No/v0gh/EnXFe EVQg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1688375511; x=1690967511; 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=Y82tbK5YpzxWq27w8zOML9VXaSAgh4EiyYLUuLg/NIc=; b=bqkdtWZSy/jUVtheweioS3gQlpWE8n6foZqNVcn4u5JYGbCOz9FdxqCTfNep25tVCb Y7DqjXEMNd/kgJg68Dh4cDmphsM9srPIxK/qwUniHve03oxlZPqZRGxCdywWKpK1OAXD pzc5wUnKx+P+IG2qYgT700nYjymoA0TtBD7yOaEkF/W5YrJ0TXP6bvpSt8tEXUZ95Tmq MUD6YPWuIA4SC+MD6Jm18YDIGUYan1yz54A3FIkMm7hDjaFa6GifHkQStlQDsadBlI0n t5+jkOc0qrdpMy+fOBqGICOAycGLV3Oz/vrd0cz/mBQ54mnfp5ChWsZtBJIYPVModK77 cvcQ== X-Gm-Message-State: ABy/qLbP0ZF3/BOxOibO6f1rT/25Bs4YTChR0Gv3LBUKFD/zgTolCtU+ uG4rS4YB8LiVwCbccgwwmdBVzTJ71ao= X-Google-Smtp-Source: APBJJlFkQyUbU0y0hr6QZdET7JDTMovlKuntlaQFOjPk7hnvQw8ck8lhnde7Kd160mO0NdulY9BDOQ== X-Received: by 2002:aa7:d1d8:0:b0:51e:f20:cf38 with SMTP id g24-20020aa7d1d8000000b0051e0f20cf38mr1766925edp.27.1688375510898; Mon, 03 Jul 2023 02:11:50 -0700 (PDT) Received: from [172.16.250.1] (84-112-104-25.cable.dynamic.surfer.at. [84.112.104.25]) by smtp.gmail.com with ESMTPSA id h11-20020aa7c60b000000b0051bed60f333sm10217165edq.48.2023.07.03.02.11.49 for (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Mon, 03 Jul 2023 02:11:50 -0700 (PDT) From: Marvin Scholz To: FFmpeg development discussions and patches Date: Mon, 03 Jul 2023 11:11:48 +0200 X-Mailer: MailMate (1.14r5964) Message-ID: <7D4C10CB-A3F9-4ECD-916B-417F3F538913@gmail.com> In-Reply-To: References: <20230501114456.13898-1-epirat07@gmail.com> <20230625104907.53071-1-epirat07@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 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. 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? > > 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. 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. > 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".