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 after
> > all used entries
>
> 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
>
> > + unsigned internal_entries_len;
> > + AVTreeNode *tree_root;
> > + AVMapInternalEntry *internal_entries;
> > +};
> > +
> > +static uint8_t deleted_entry;
>
> 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] = {
> > + {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,18},{1,6,18},{2,6,18},};
> > + int case_sensitive = sensitivity[cmp_flags][0];
> > + int keyvalue_sensitive = sensitivity[cmp_flags][1];
> > + int truncated_sensitive = sensitivity[cmp_flags][2];
> > +
> > + if (!keyvalue_sensitive || !truncated_sensitive || cmp_flags >= 27U)
> > + return AVERROR(EINVAL);
>
> Need to check for cmp_flags >= 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 crash
[...]
> > +AVMap *av_map_new(AVMapCompareFunc cmp_keyvalue, int cmp_flags,
> > AVMapCopyFunc copy, AVMapFreeFunc freef)
> > +{
> > + AVMap *s = av_mallocz(sizeof(*s));
> > + if (!s)
> > + return NULL;
> > +
> > + s->copy = copy;
> > + s->freef = freef;
> > +
> > + av_map_add_cmp_func(s, cmp_keyvalue, cmp_flags);
>
> 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 *value,
> > int valuelen, int flags)
> > +{
> > + av_assert2(keylen || valuelen); // patch welcome but how would the
> > compare function compare a len=0 element without knowing it is a len 0
> > element
> > +
>
> 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 = NULL;
> > + AVMap *bak = av_memdup(dst, sizeof(*dst));
> > + if (!bak)
> > + return AVERROR(ENOMEM);
> > +
> > + AVMapInternalEntry *new_internal_entries =
> > av_memdup(bak->internal_entries, bak->internal_entries_len);
> > + AVMapInternalEntry *old_internal_entries = dst->internal_entries;
>
> We don't allow mixed delcarations and statements. Hoist the defintion above
> if (!bak) and then put the assignment below it.
commit 890b8da1ce27fd365eaffefc7efcbadae9f01f2a
configure: Allow mixing declarations and statements
>
> > +
> > + while ((t = av_map_iterate(src, t))) {
> > + int ret = av_map_add(dst, t->key, t->keylen, t->value,
> > t->valuelen, 0);
> > +
> > + if (ret < 0) {
> > + update_pointers(bak, new_internal_entries,
> > old_internal_entries);
> > + av_free(dst->internal_entries);
> > + memcpy(dst, bak, sizeof(*dst));
> > + return ret;
>
> You leak bak here. Possibly new_internal_entries as well.
fixed
[...]
> > +typedef struct AVMapEntry {
> > + uint8_t *key;
> > + uint8_t *value;
>
> 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
>
> > + int keylen;
> > + int valuelen;
>
> size_t for lengths
>
> > +} 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 their
> > 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 their
> > 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);
>
>
> 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 thinking
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
> > +1111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111100000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
> > +the key=the value 8,10 n=n 2,2 �=� 2,2 "=" 2,2 ]=] 2,2 �=� 2,2
> > y=y 2,2 *=* 2,2 5=5 2,2 ~=~ 2,2 �=� 2,2 �=� 2,2 �=� 2,2 �=�
> > 2,2 )=) 2,2 �=� 2,2 e=e 2,2 �=� 2,2 A=A 2,2 B=B 2,2 �=� 2,2
> > �=� 2,2 �=� 2,2 J=J 2,2 �=� 2,2 �=� 2,2 �=� 2,2 �=� 2,2 �=�
> > 2,2 �=� 2,2 �=� 2,2 �=� 2,2 �=� 2,2 �=� 2,2 �=� 2,2 b=b 2,2
> > = 2,2 �=� 2,2 9=9 2,2 j=j 2,2 �=� 2,2 �=� 2,2 Q=Q 2,2 �=�
> > 2,2 M=M 2,2 = 2,2 �=� 2,2 �=� 2,2 %=% 2,2 �=� 2,2 = 2,2
> > �=� 2,2 }=} 2,2 = 2,2 �=� 2,2 �=� 2,2 U=U 2,2 �=� 2,2 �=�
> > 2,2 = 2,2 �=� 2,2 &=& 2,2 I=I 2,2 = 2,2 �=� 2,2 �=� 2,2
> > a=a 2,2 �=� 2,2 �=� 2,2 6=6 2,2 �=� 2,2 �=� 2,2 �=� 2,2 �=�
> > 2,2 = 2,2 2=2 2,2
> > =
> > 2,2 F=F 2,2 �=� 2,2 :=: 2,2 �=� 2,2 = 2,2 �=� 2,2 �=� 2,2
> > === 2,2 V=V 2,2 Y=Y 2,2 �=� 2,2 = 2,2 = 2,2 q=q 2,2 R=R
> > 2,2 m=m 2,2 f=f 2,2 = 2,2 Z=Z 2,2 E=E 2,2 .=. 2,2
> > !=! 2,2 �=� 2,2 �=� 2,2 v=v 2,2 �=� 2,2 �=� 2,2 u=u 2,2 >=>
> > 2,2 �=� 2,2 r=r 2,2 �=� 2,2 �=� 2,2 i=i 2,2 z=z 2,2 �=� 2,2
> > N=N 2,2 �=� 2,2 = 2,2 �=� 2,2 �=� 2,2 = 2,2
> > +=
> > + 2,2 �=� 2,2 ^=^ 2,2 1=1 2,2 �=� 2,2 -=- 2,2 �=� 2,2 �=� 2,2
> > �=� 2,2 = 2,2
>
>
> 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
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
The educated differ from the uneducated as much as the living from the
dead. -- Aristotle
signature.asc
Description: PGP signature
_______________________________________________ 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".
