[FFmpeg-devel] [PATCH 001/279] Add a new channel layout API

[Anton Khirnov]

Quoting Marton Balint (2021-12-14 02:13:00)
> > + *
> > + * Copying an AVChannelLayout via assigning is forbidden,
> > + * av_channel_layout_copy() must be used. instead (and its return value should
> > + * be checked)
> > + *
> > + * No new fields may be added to it without a major version bump, except for
> > + * new elements of the union fitting in sizeof(uint64_t).
> > + */
> > +typedef struct AVChannelLayout {
> > +    /**
> > +     * Channel order used in this layout.
> > +     * This is a mandatory field.
> > +     */
> > +    enum AVChannelOrder order;
> > +
> > +    /**
> > +     * Number of channels in this layout. Mandatory field.
> > +     */
> > +    int nb_channels;
> > +
> > +    /**
> > +     * Details about which channels are present in this layout.
> > +     * For AV_CHANNEL_ORDER_UNSPEC, this field is undefined and must not be
> > +     * used.
> > +     */
> > +    union {
> > +        /**
> > +         * This member must be used for AV_CHANNEL_ORDER_NATIVE.
> > +         * It is a bitmask, where the position of each set bit means that the
> > +         * AVChannel with the corresponding value is present.
> > +         *
> > +         * I.e. when (mask & (1 << AV_CHAN_FOO)) is non-zero, then AV_CHAN_FOO
> > +         * is present in the layout. Otherwise it is not present.
> > +         *
> > +         * @note when a channel layout using a bitmask is constructed or
> > +         * modified manually (i.e.  not using any of the av_channel_layout_*
> > +         * functions), the code doing it must ensure that the number of set bits
> > +         * is equal to nb_channels.
> > +         */
> > +        uint64_t mask;
> > +        /**
> > +         * This member must be used when the channel order is
> > +         * AV_CHANNEL_ORDER_CUSTOM. It is a nb_channels-sized array, with each
> > +         * element signalling the presence of the AVChannel with the
> > +         * corresponding value in map[i].id.
> > +         *
> > +         * I.e. when map[i].id is equal to AV_CHAN_FOO, then AV_CH_FOO is the
> > +         * i-th channel in the audio data.
> > +         */
> > +        AVChannelCustom *map;
> > +    } u;
> 
> I would like to attach some extendable, possibly per-channel metadata to 
> the channel layout. I'd rather put it into AVChannelLayout, so native 
> layouts could also have metadata. This must be dynamically allocated to 
> make it extendable and to not consume too many bytes. I can accept that it 
> will be slow. But I don't see it unmanagable, because AVChannelLayout 
> already have functions for allocation/free/copy. I also think that most of 
> the time it will not be used (e.g. if metadata is NULL, that can mean no 
> metadata for all the channels).
> 
> If we can't decide what this should be, array of AVDictionaries, some 
> side-data like approach but in the channel layout, or a new dynamically 
> allocated AVChannelLayoutMetadata struct, then maybe just reserve a void* 
> in the end of the AVChannelLayout, and we can work it out later.

I would much prefer not to have any extra dynamically allocated
complexity here, especially not AVDictionaries. E.g. the AVFrame
dictionary has been a horrible blight on avfilter - now countless
filters use it export structured values as strings, instead of defining
a proper API through side data.

The same thing happening to channel layouts is something we very much do
not want IMO. Opaque IDs referring to higher-layer side data allows
implementing the same capabilities without stuffing complexity in places
it doesn't belong.

-- 
Anton Khirnov