From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <ffmpeg-devel-bounces@ffmpeg.org>
Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org [79.124.17.100])
by master.gitmailbox.com (Postfix) with ESMTPS id 869774D6FD
for <ffmpegdev@gitmailbox.com>; Sat, 19 Apr 2025 14:39:25 +0000 (UTC)
Received: from [127.0.1.1] (localhost [127.0.0.1])
by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 1358A687D75;
Sat, 19 Apr 2025 17:39:20 +0300 (EEST)
Received: from relay6-d.mail.gandi.net (relay6-d.mail.gandi.net
[217.70.183.198])
by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 070BB680C85
for <ffmpeg-devel@ffmpeg.org>; Sat, 19 Apr 2025 17:39:12 +0300 (EEST)
Received: by mail.gandi.net (Postfix) with ESMTPSA id 0240C439A6
for <ffmpeg-devel@ffmpeg.org>; Sat, 19 Apr 2025 14:39:11 +0000 (UTC)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=niedermayer.cc;
s=gm1; t=1745073552;
h=from:from:reply-to:subject:subject:date:date:message-id:message-id:
to:to:cc:mime-version:mime-version:content-type:content-type:
in-reply-to:in-reply-to:references:references;
bh=JutfwR6ZaczI7wn2Z7MEhCWb/6GyX6t5FIl1D5Y9tNI=;
b=n37QCqfXmb2IILJtbltxMnF40AaHnyKmgf9NJ405/+bAlBmgZ4N8s6LWq3BJmTnsxXRHi6
hI/pct1OyQTqo35mJR/fYuCfaMXMccDT4Z+Sf89LeZ07FN3HaNjslOgsIY3FNltMZ4I9+j
hKuTZKnhgETLZWsq584347U2QGDm9VrWQfrbusGMS02cJ0/ISS4HvcX0K4WmNnTOJ8QzH1
3l3yJ/raP1piJGUW+QgL6p0MXvx1k9y4WaHj7XXUjsgsqAflq3u7jBBuJEi/kzOfXol4C7
avHPebQahmuym1pgZmmzM3JbRVEJnJty/jAM5NTPKvzSa/3wsRGvkb7k77BCFA==
Date: Sat, 19 Apr 2025 16:39:11 +0200
From: Michael Niedermayer <michael@niedermayer.cc>
To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
Message-ID: <20250419143911.GC4991@pb2>
References: <20250417165241.3266643-1-michael@niedermayer.cc>
<ef669c75-c548-4a04-8dca-147e59f67c08@gmail.com>
MIME-Version: 1.0
In-Reply-To: <ef669c75-c548-4a04-8dca-147e59f67c08@gmail.com>
X-GND-State: clean
X-GND-Score: -70
X-GND-Cause: gggruggvucftvghtrhhoucdtuddrgeefvddrtddtgddvfeehvdelucetufdoteggodetrfdotffvucfrrhhofhhilhgvmecuifetpfffkfdpucggtfgfnhhsuhgsshgtrhhisggvnecuuegrihhlohhuthemuceftddunecusecvtfgvtghiphhivghnthhsucdlqddutddtmdenfghrlhcuvffnffculdeftddmnecujfgurhepfffhvffukfhfgggtuggjsehgtderredttdejnecuhfhrohhmpefoihgthhgrvghlucfpihgvuggvrhhmrgihvghruceomhhitghhrggvlhesnhhivgguvghrmhgrhigvrhdrtggtqeenucggtffrrghtthgvrhhnpeelkeeggfffiedufeejueffjeduhedttdduledtheevveevtdeiueelhfdtuedtkeenucfkphepgedurdeiiedrieejrdduudefnecuvehluhhsthgvrhfuihiivgeptdenucfrrghrrghmpehinhgvthepgedurdeiiedrieejrdduudefpdhhvghloheplhhotggrlhhhohhsthdpmhgrihhlfhhrohhmpehmihgthhgrvghlsehnihgvuggvrhhmrgihvghrrdgttgdpnhgspghrtghpthhtohepuddprhgtphhtthhopehffhhmphgvghdquggvvhgvlhesfhhfmhhpvghgrdhorhhg
X-GND-Sasl: michael@niedermayer.cc
Subject: Re: [FFmpeg-devel] [PATCH v3] libavutil: Add AVMap
X-BeenThere: ffmpeg-devel@ffmpeg.org
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: FFmpeg development discussions and patches <ffmpeg-devel.ffmpeg.org>
List-Unsubscribe: <https://ffmpeg.org/mailman/options/ffmpeg-devel>,
<mailto:ffmpeg-devel-request@ffmpeg.org?subject=unsubscribe>
List-Archive: <https://ffmpeg.org/pipermail/ffmpeg-devel>
List-Post: <mailto:ffmpeg-devel@ffmpeg.org>
List-Help: <mailto:ffmpeg-devel-request@ffmpeg.org?subject=help>
List-Subscribe: <https://ffmpeg.org/mailman/listinfo/ffmpeg-devel>,
<mailto:ffmpeg-devel-request@ffmpeg.org?subject=subscribe>
Reply-To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
Content-Type: multipart/mixed; boundary="===============5512388279613546459=="
Errors-To: ffmpeg-devel-bounces@ffmpeg.org
Sender: "ffmpeg-devel" <ffmpeg-devel-bounces@ffmpeg.org>
Archived-At: <https://master.gitmailbox.com/ffmpegdev/20250419143911.GC4991@pb2/>
List-Archive: <https://master.gitmailbox.com/ffmpegdev/>
List-Post: <mailto:ffmpegdev@gitmailbox.com>
--===============5512388279613546459==
Content-Type: multipart/signed; micalg=pgp-sha512;
protocol="application/pgp-signature"; boundary="jcgSe0cjDtjjgkA5"
Content-Disposition: inline
--jcgSe0cjDtjjgkA5
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable
Hi Leo
On Sat, Apr 19, 2025 at 06:36:56AM -0400, Leo Izen wrote:
> On 4/17/25 12:52, Michael Niedermayer wrote:
[...]
> > +typedef struct{
> > + AVMapEntry map_entry;
> > + uint8_t treenode_and_keyvalue[0];
> > +} AVMapInternalEntry;
> > +
> > +struct AVMap{
> > +#define CMP_MASK 31
> > + AVMapCompareFunc cmp[27];
> > + AVMapCopyFunc copy;
> > + AVMapFreeFunc freef;
> > + int count;
> > + int deleted;
> > + int next; ///< index of entry in root array afte=
r all used entries
>=20
> Indices into arrays and counts and the like should be size_t.
I guess AVMap will be one of the first parts of FFmpeg doing that
git grep 'int count' | wc
354 2493 27289
git grep 'size_t count' | wc
2 8 97
git grep 'int [a-zA-Z_]*index' | wc
1279 7735 94963
git grep 'size_t [a-zA-Z_]*index' | wc
29 118 1888
also we use av_fast_realloc() which doesnt use size_t
and av_map_realloc() must return a signed value to allow error codes so it =
will use int64_t not size_t
>=20
> > + unsigned internal_entries_len;
> > + AVTreeNode *tree_root;
> > + AVMapInternalEntry *internal_entries;
> > +};
> > +
> > +static uint8_t deleted_entry;
>=20
> static const should allow the compiler to put it in a different section if
> it wishes. You can still address it even then.
I dont want to put it in a different section. Its value should be available
with a minimum of computation.
[...]
> > +int av_map_add_cmp_func(AVMap *m, AVMapCompareFunc cmp, int cmp_flags)
> > +{
> > + static const uint8_t sensitivity[27][3] =3D {
> > + {0,0, 0},{1,0, 0},{2,0, 0}, {0,3, 0},{1,3, 0},{2,3, 0}, {0,6, =
0},{1,6, 0},{2,6, 0},
> > + {0,0, 9},{1,0, 9},{2,0, 9}, {0,3, 9},{1,3, 9},{2,3, 9}, {0,6, =
9},{1,6, 9},{2,6, 9},
> > + {0,0,18},{1,0,18},{2,0,18}, {0,3,18},{1,3,18},{2,3,18}, {0,6,1=
8},{1,6,18},{2,6,18},};
> > + int case_sensitive =3D sensitivity[cmp_flags][0];
> > + int keyvalue_sensitive =3D sensitivity[cmp_flags][1];
> > + int truncated_sensitive =3D sensitivity[cmp_flags][2];
> > +
> > + if (!keyvalue_sensitive || !truncated_sensitive || cmp_flags >=3D =
27U)
> > + return AVERROR(EINVAL);
>=20
> Need to check for cmp_flags >=3D 27U before indexing into the array. The
ok
> compiler may pull this array off the stack cause it's static const so you
> risk hitting a dead page here.
printf("A"+5) also might hit a dead page, the user is not supposed to input
invalid data, and theres no gurantee that passing invalid data will not cra=
sh
[...]
> > +AVMap *av_map_new(AVMapCompareFunc cmp_keyvalue, int cmp_flags, AVMapC=
opyFunc copy, AVMapFreeFunc freef)
> > +{
> > + AVMap *s =3D av_mallocz(sizeof(*s));
> > + if (!s)
> > + return NULL;
> > +
> > + s->copy =3D copy;
> > + s->freef =3D freef;
> > +
> > + av_map_add_cmp_func(s, cmp_keyvalue, cmp_flags);
>=20
> No check for return value. av_map_add_cmp_func can return AVERROR(EINVAL)
> depending on cmp_flags.
indeed, i missed that, good find
[...]
> > + return advance;
> > +}
> > +
> > +int av_map_add(AVMap *s, const char *key, int keylen, const char *valu=
e, int valuelen, int flags)
> > +{
> > + av_assert2(keylen || valuelen); // patch welcome but how would the=
compare function compare a len=3D0 element without knowing it is a len 0 e=
lement
> > +
>=20
> This is a public function so we should be returning AVERROR(EINVAL) on
> invalid input rather than asserting. Since a library user could do that.
Its a speed relevent function
testing for the caller to do something stupid is not its job
The assert helps the user to find such a mistake
[...]
> > +int av_map_copy(AVMap *dst, const AVMap *src)
> > +{
> > + const AVMapEntry *t =3D NULL;
> > + AVMap *bak =3D av_memdup(dst, sizeof(*dst));
> > + if (!bak)
> > + return AVERROR(ENOMEM);
> > +
> > + AVMapInternalEntry *new_internal_entries =3D av_memdup(bak->intern=
al_entries, bak->internal_entries_len);
> > + AVMapInternalEntry *old_internal_entries =3D dst->internal_entries;
>=20
> We don't allow mixed delcarations and statements. Hoist the defintion abo=
ve
> if (!bak) and then put the assignment below it.
commit 890b8da1ce27fd365eaffefc7efcbadae9f01f2a
configure: Allow mixing declarations and statements
>=20
> > +
> > + while ((t =3D av_map_iterate(src, t))) {
> > + int ret =3D av_map_add(dst, t->key, t->keylen, t->value, t->va=
luelen, 0);
> > +
> > + if (ret < 0) {
> > + update_pointers(bak, new_internal_entries, old_internal_en=
tries);
> > + av_free(dst->internal_entries);
> > + memcpy(dst, bak, sizeof(*dst));
> > + return ret;
>=20
> You leak bak here. Possibly new_internal_entries as well.
fixed
[...]
> > +typedef struct AVMapEntry {
> > + uint8_t *key;
> > + uint8_t *value;
>=20
> Any particular reason to use unsigned char here rather than just char when
> working with strings? Most string stuff expects signed char.
changed to char*
AVMap is not limited to strings
>=20
> > + int keylen;
> > + int valuelen;
>=20
> size_t for lengths
>=20
> > +} AVMapEntry;
> > +
> > +typedef struct AVMap AVMap;
> > +typedef void (* AVMapFreeFunc)(AVMapEntry *c);
> > +typedef void (* AVMapCopyFunc)(AVMapEntry *dst, const AVMapEntry *src,=
size_t len);
> > +typedef int (* AVMapCompareFunc)(const void *keyvalue, const void *b);
> > +
> > +/**
> > + * like strcmp() but compares concatenated keyvalues.
> > + *
> > + * A map initialized with this will allow duplicate keys as long as th=
eir values differ.
> > + */
> > +int av_map_strcmp_keyvalue(const char *a, const char *b);
> > +
> > +/**
> > + * like av_map_strcmp_keyvalue() but is compatible with av_strcasecmp(=
) and av_map_supercmp_key.
> > + *
> > + * A map initialized with this will allow duplicate keys as long as th=
eir values differ.
> > + */
> > +int av_map_supercmp_keyvalue(const char *a, const char *b);
> > +
> > +/**
> > + * like strcmp() but is compatible with av_strcasecmp().
> > + *
> > + * A map initialized with this will not allow duplicate keys.
> > + */
> > +int av_map_supercmp_key(const char *a, const char *b);
>=20
>=20
> I don't believe it's clear if and when a user should call these functions=
or
> simply pass them by address. If so, we should use the typedef, or at least
> reference it in the doc comment.
the user can do whatever she wants with them
av_map_alloc() refers to them already
maybe we should simplify init to not require this at all, iam still thinkin=
g about that
[...]
> > index 00000000000..6d1699ef4b7
> > --- /dev/null
> > +++ b/tests/ref/fate/map
> > @@ -0,0 +1,27 @@
> > +testing empty set
> > +
> > +testing 1-set
> > +
> > +testing n-set
> > +1111111111111111111111111111111111111111111111111111111111111111111111=
111111111111111111111111111111111111111111111111111111111100000000000000000=
000000000000000000000000000000000000000000000000000000000000000000000000000=
000000000000000000000000000000000000000000000000000000000000000000000000000=
000000000000000000000000000000000000000000000000000000000000000000000000000=
000000000000000000000000000000000000000000000000000000000000000000000000000=
000000000000000000000000000000000000000000000000000000000000000000000000000=
000000000000000000000000000000000000000000000000000000000000000000000000000=
000000000000000000000000000000000000000000000000000000000000000000000000000=
000000000000000000000000000000000000000000000000000000000000000000000000000=
000000000000000000000000000000000000000000000000000000000000000000000000000=
000000000000000000000000000000000000000000000000000000000000000000000000000=
000000000000000000000000000000000000000000000000000000000000000000000000000=
000000000000000000000000000000
> > +the key=3Dthe value 8,10 n=3Dn 2,2 =EF=BF=BD=3D=EF=BF=BD 2,2 "=
=3D" 2,2 ]=3D] 2,2 =EF=BF=BD=3D=EF=BF=BD 2,2 y=3Dy 2,2 *=3D* 2,2 =
5=3D5 2,2 ~=3D~ 2,2 =EF=BF=BD=3D=EF=BF=BD 2,2 =EF=BF=BD=3D=EF=BF=BD 2=
,2 =EF=BF=BD=3D=EF=BF=BD 2,2 =EF=BF=BD=3D=EF=BF=BD 2,2 )=3D) 2,2 =
=EF=BF=BD=3D=EF=BF=BD 2,2 e=3De 2,2 =EF=BF=BD=3D=EF=BF=BD 2,2 A=3DA 2=
,2 B=3DB 2,2 =EF=BF=BD=3D=EF=BF=BD 2,2 =EF=BF=BD=3D=EF=BF=BD 2,2 =
=EF=BF=BD=3D=EF=BF=BD 2,2 J=3DJ 2,2 =EF=BF=BD=3D=EF=BF=BD 2,2 =EF=BF=
=BD=3D=EF=BF=BD 2,2 =EF=BF=BD=3D=EF=BF=BD 2,2 =EF=BF=BD=3D=EF=BF=BD 2,2=
=EF=BF=BD=3D=EF=BF=BD 2,2 =EF=BF=BD=3D=EF=BF=BD 2,2 =EF=BF=BD=3D=EF=
=BF=BD 2,2 =EF=BF=BD=3D=EF=BF=BD 2,2 =EF=BF=BD=3D=EF=BF=BD 2,2 =EF=BF=
=BD=3D=EF=BF=BD 2,2 =EF=BF=BD=3D=EF=BF=BD 2,2 b=3Db 2,2 =1D=3D=1D 2,2=
=EF=BF=BD=3D=EF=BF=BD 2,2 9=3D9 2,2 j=3Dj 2,2 =EF=BF=BD=3D=EF=BF=
=BD 2,2 =EF=BF=BD=3D=EF=BF=BD 2,2 Q=3DQ 2,2 =EF=BF=BD=3D=EF=BF=BD 2,2=
M=3DM 2,2 =06=3D=06 2,2 =EF=BF=BD=3D=EF=BF=BD 2,2 =EF=BF=BD=3D=EF=
=BF=BD 2,2 %=3D% 2,2 =EF=BF=BD=3D=EF=BF=BD 2,2 =01=3D=01 2,2 =EF=BF=
=BD=3D=EF=BF=BD 2,2 }=3D} 2,2 =16=3D=16 2,2 =EF=BF=BD=3D=EF=BF=BD 2,2=
=EF=BF=BD=3D=EF=BF=BD 2,2 U=3DU 2,2 =EF=BF=BD=3D=EF=BF=BD 2,2 =EF=
=BF=BD=3D=EF=BF=BD 2,2 =12=3D=12 2,2 =EF=BF=BD=3D=EF=BF=BD 2,2 &=3D& =
2,2 I=3DI 2,2 =1A=3D=1A 2,2 =EF=BF=BD=3D=EF=BF=BD 2,2 =EF=BF=BD=3D=
=EF=BF=BD 2,2 a=3Da 2,2 =EF=BF=BD=3D=EF=BF=BD 2,2 =EF=BF=BD=3D=EF=BF=
=BD 2,2 6=3D6 2,2 =EF=BF=BD=3D=EF=BF=BD 2,2 =EF=BF=BD=3D=EF=BF=BD 2,2=
=EF=BF=BD=3D=EF=BF=BD 2,2 =EF=BF=BD=3D=EF=BF=BD 2,2 =11=3D=11 2,2 =
2=3D2 2,2
> > =3D
> > 2,2 F=3DF 2,2 =EF=BF=BD=3D=EF=BF=BD 2,2 :=3D: 2,2 =EF=BF=BD=
=3D=EF=BF=BD 2,2 =0E=3D=0E 2,2 =EF=BF=BD=3D=EF=BF=BD 2,2 =EF=BF=BD=3D=
=EF=BF=BD 2,2 =3D=3D=3D 2,2 V=3DV 2,2 Y=3DY 2,2 =EF=BF=BD=3D=EF=BF=
=BD 2,2 =15=3D=15 2,2 =1E=3D=1E 2,2 q=3Dq 2,2 R=3DR 2,2 m=3Dm 2,2=
f=3Df 2,2 =3D 2,2 Z=3DZ 2,2 E=3DE 2,2 .=3D. 2,2 !=3D! 2,2 =
=EF=BF=BD=3D=EF=BF=BD 2,2 =EF=BF=BD=3D=EF=BF=BD 2,2 v=3Dv 2,2 =EF=BF=
=BD=3D=EF=BF=BD 2,2 =EF=BF=BD=3D=EF=BF=BD 2,2 u=3Du 2,2 >=3D> 2,2 =
=EF=BF=BD=3D=EF=BF=BD 2,2 r=3Dr 2,2 =EF=BF=BD=3D=EF=BF=BD 2,2 =EF=BF=
=BD=3D=EF=BF=BD 2,2 i=3Di 2,2 z=3Dz 2,2 =EF=BF=BD=3D=EF=BF=BD 2,2 N=
=3DN 2,2 =EF=BF=BD=3D=EF=BF=BD 2,2 =02=3D=02 2,2 =EF=BF=BD=3D=EF=BF=
=BD 2,2 =EF=BF=BD=3D=EF=BF=BD 2,2 =19=3D=19 2,2
> > +=3D
> > + 2,2 =EF=BF=BD=3D=EF=BF=BD 2,2 ^=3D^ 2,2 1=3D1 2,2 =EF=BF=BD=
=3D=EF=BF=BD 2,2 -=3D- 2,2 =EF=BF=BD=3D=EF=BF=BD 2,2 =EF=BF=BD=3D=EF=
=BF=BD 2,2 =EF=BF=BD=3D=EF=BF=BD 2,2 =05=3D=05 2,2
>=20
>=20
> This may just be my email client messing up, but many of these appear to =
be
> nonprintable. I don't know why.
Testing that covers non printable chars
thanks for the review
[...]
--=20
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
The educated differ from the uneducated as much as the living from the
dead. -- Aristotle=20
--jcgSe0cjDtjjgkA5
Content-Type: application/pgp-signature; name="signature.asc"
-----BEGIN PGP SIGNATURE-----
iF0EABEKAB0WIQSf8hKLFH72cwut8TNhHseHBAsPqwUCaAO1gQAKCRBhHseHBAsP
q8DBAJkBFiKZOFC+lXObXZmr1jFAf5zBfgCglcpFCcUKQczL7kAPa21dkiM7c0w=
=F1Lu
-----END PGP SIGNATURE-----
--jcgSe0cjDtjjgkA5--
--===============5512388279613546459==
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".
--===============5512388279613546459==--