Re: [FFmpeg-devel] [PATCH v2 5/6] libavutil: Add AVMap

[Michael Niedermayer <michael@niedermayer.cc>]

[-- Attachment #1.1: Type: text/plain, Size: 1378 bytes --]

Hi Nicolas

On Tue, Apr 15, 2025 at 08:45:27PM +0200, Nicolas George wrote:
> Michael Niedermayer (HE12025-04-15):
> > +AVMap *av_map_new(AVMapCompareFunc cmp_keyvalue, AVMapCopyFunc copy, AVMapFreeFunc freef)
> > +{
> > +    AVMap *s = av_mallocz(sizeof(*s));
> > +    if (!s)
> > +        return NULL;
> 
> Please no.
> 
> The ability to allocate on stack including room for a few values is more
> important than any performance enhancement for large maps.

Where exactly would that benefit FFmpeg ?
Dictionaries generally are used to move stuff aorund, like metadata or
options.
Or may be used in the future to keep track of some mappings like
timestamps to file positions during demuxing or muxing
Or maybe codecs/formats lookup from name

None of these are confined to a single local function


> 
> > +        AVMap *set = av_map_new(our_cmp[settype], NULL, NULL);
> 
> … should be :
> 
> 	AVMap set = av_map_create(cmp, NULL, NULL);
> 
> or :
> 
> 	AVMap set;
> 	av_map_init(&set, sizeof(set), cmp, NULL, NULL);

i like av_map_new() because "new" is short
and sizeof(AVMap) is not public API

thx

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Those who would give up essential Liberty, to purchase a little
temporary Safety, deserve neither Liberty nor Safety -- Benjamin Franklin

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
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".