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

[Marton Balint]

On Tue, 7 Dec 2021, James Almer wrote:

> From: Anton Khirnov <anton at khirnov.net>
>

[...]

> -static const char *get_channel_name(int channel_id)
> +static const char *get_channel_name(enum AVChannel channel_id)
> {
> -    if (channel_id < 0 || channel_id >= FF_ARRAY_ELEMS(channel_names))
> -        return NULL;
> +    if ((unsigned) channel_id >= FF_ARRAY_ELEMS(channel_names) ||
> +        !channel_names[channel_id].name)
> +        return "?";

I'd rather return NULL here instead of "?". Is this allowed to happen by 
the way?

>     return channel_names[channel_id].name;
> }
>
> -static const struct {
> +static inline void get_channel_str(AVBPrint *bp, const char *str,
> +                                   enum AVChannel channel_id)
> +{
> +    if (str)
> +        av_bprintf(bp, "%s", str);
> +    else
> +        av_bprintf(bp, "?");

If this is not allowed, then you should propagate back AVERROR(EINVAL) 
if it does. If it is allowed, because the user can use some custom 
channel_id-s, then something like "USER%d" should be returned IMHO.

> +    errno = 0;
> +    channels = strtol(str, &end, 10);
> +
> +    /* number of channels */
> +    if (!errno && *end == 'c' && !*(end + 1) && channels >= 0) {
> +        av_channel_layout_default(channel_layout, channels);
> +        return 0;
> +    }
> +
> +    /* number of unordered channels */
> +    if (!errno && (!*end || av_strnstr(str, "channels", strlen(str))) && channels >= 0) {

This is missing the syntax used in av_get_extended_channel_layout(), 1C to 
63C for unspecified channel layout with that many number of channels.

[...]


> +int av_channel_layout_describe(const AVChannelLayout *channel_layout,
> +                               char *buf, size_t buf_size)
> +{

Can you make a function variant of this which directly gets a AVBPrint 
buffer? Similar to av_bprint_channel_layout.

[...]

> +int av_channel_layout_check(const AVChannelLayout *channel_layout)

av_channel_layout_is_valid() seems like a better name.

[...]


> diff --git a/libavutil/channel_layout.h b/libavutil/channel_layout.h
> index d39ae1177a..018e87ff0b 100644
> --- a/libavutil/channel_layout.h
> +++ b/libavutil/channel_layout.h
> @@ -23,6 +23,10 @@
> #define AVUTIL_CHANNEL_LAYOUT_H
>
> #include <stdint.h>
> +#include <stdlib.h>
> +
> +#include "version.h"
> +#include "attributes.h"
>
> /**
>  * @file
> @@ -34,6 +38,68 @@
>  * @{
>  */
>
> +enum AVChannel {
> +    ///< Invalid channel index
> +    AV_CHAN_NONE = -1,
> +    AV_CHAN_FRONT_LEFT,
> +    AV_CHAN_FRONT_RIGHT,
> +    AV_CHAN_FRONT_CENTER,
> +    AV_CHAN_LOW_FREQUENCY,
> +    AV_CHAN_BACK_LEFT,
> +    AV_CHAN_BACK_RIGHT,
> +    AV_CHAN_FRONT_LEFT_OF_CENTER,
> +    AV_CHAN_FRONT_RIGHT_OF_CENTER,
> +    AV_CHAN_BACK_CENTER,
> +    AV_CHAN_SIDE_LEFT,
> +    AV_CHAN_SIDE_RIGHT,
> +    AV_CHAN_TOP_CENTER,
> +    AV_CHAN_TOP_FRONT_LEFT,
> +    AV_CHAN_TOP_FRONT_CENTER,
> +    AV_CHAN_TOP_FRONT_RIGHT,
> +    AV_CHAN_TOP_BACK_LEFT,
> +    AV_CHAN_TOP_BACK_CENTER,
> +    AV_CHAN_TOP_BACK_RIGHT,
> +    /** Stereo downmix. */
> +    AV_CHAN_STEREO_LEFT = 29,
> +    /** See above. */
> +    AV_CHAN_STEREO_RIGHT,
> +    AV_CHAN_WIDE_LEFT,
> +    AV_CHAN_WIDE_RIGHT,
> +    AV_CHAN_SURROUND_DIRECT_LEFT,
> +    AV_CHAN_SURROUND_DIRECT_RIGHT,
> +    AV_CHAN_LOW_FREQUENCY_2,
> +    AV_CHAN_TOP_SIDE_LEFT,
> +    AV_CHAN_TOP_SIDE_RIGHT,
> +    AV_CHAN_BOTTOM_FRONT_CENTER,
> +    AV_CHAN_BOTTOM_FRONT_LEFT,
> +    AV_CHAN_BOTTOM_FRONT_RIGHT,
> +
> +    /** Channel is empty can be safely skipped. */
> +    AV_CHAN_SILENCE = 64,

I'd rather name this AV_CHAN_GARBAGE or AV_CHAN_UNUSED instead. Silence 
could be useful, and a silent channel could be FRONT LEFT at the same 
time, but AV_CHAN_SILENCE is not a flag. But based on the comment this is 
simply a channel which should be ignored, because it does not contain 
anything playable/presentable.

Other already said, but an unknown designation (AV_CHAN_UNKNOWN) should be 
added. Which is a useful channel, but with unknown or unsupported 
designation.

[...]

> + * An AVChannelCustom defines a single channel within a custom order layout
> + *
> + * Unlike most structures in FFmpeg, sizeof(AVChannelCustom) is a part of the
> + * public ABI.
> + *
> + * No new fields may be added to it without a major version bump.
> + */
> +typedef struct AVChannelCustom {
> +    enum AVChannel id;
> +} AVChannelCustom;
> +
> +/**
> + * An AVChannelLayout holds information about the channel layout of audio data.
> + *
> + * A channel layout here is defined as a set of channels ordered in a specific
> + * way (unless the channel order is AV_CHANNEL_ORDER_UNSPEC, in which case an
> + * AVChannelLayout carries only the channel count).
> + *
> + * Unlike most structures in Libav, sizeof(AVChannelLayout) is a part of the
> + * public ABI and may be used by the caller. E.g. it may be allocated on stack
> + * or embedded in caller-defined structs.
> + *
> + * AVChannelLayout can be initialized as follows:
> + * - default initialization with {0}, followed by by setting all used fields
> + *   correctly;
> + * - by assigning one of the predefined AV_CHANNEL_LAYOUT_* initializers;
> + * - with a constructor function, such as av_channel_layout_default(),
> + *   av_channel_layout_from_mask() or av_channel_layout_from_string().
> + *
> + * The channel layout must be unitialized with av_channel_layout_uninit()
> + * (strictly speaking this is not necessary for AV_CHANNEL_ORDER_NATIVE and
> + * AV_CHANNEL_ORDER_UNSPEC, but we recommend always calling
> + * av_channel_layout_uninit() regardless of the channel order used).

I'd remove this comment, that it might not be necessary to uninit. New 
API, please always uninit().

> + *
> + * 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.

Regards,
Marton