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

[Marton Balint]

On Thu, 16 Dec 2021, James Almer wrote:

> - Added a 16 byte fixed array to AVChannelCustom to give custom
>  labels to channels in Custom order layouts. These labes will
>  be used by the helpers when querying channels by name or
>  describing the layout.

I don't think this is a good idea to use the labels directly in helpers 
instead of the channel designation names. See more below.

It is also not great that if the user wants to label a channel, he has to 
use a custom layout. So I'd rather remove the name field from 
AVChannelCustom, I find it limited anyway.

[..]

> +enum AVChannel av_channel_from_string(const char *str)
> +{
> +    int i;
> +    char *endptr = (char *)str;
> +    enum AVChannel id = AV_CHAN_NONE;
> +    for (i = 0; i < FF_ARRAY_ELEMS(channel_names); i++) {
> +        if (channel_names[i].name && !strcmp(str, channel_names[i].name))
> +            return i;
> +    }
> +    if (!strncmp(str, "USR", 3)) {
> +        const char *p = str + 3;
> +        id = strtol(p, &endptr, 0);
> +    }
> +    if (id > 0 && !*endptr)

id >= 0

[..]


> +int av_channel_layout_from_mask(AVChannelLayout *channel_layout,
> +                                uint64_t mask)
> +{
> +    if (!mask)
> +        return AVERROR(EINVAL);
> +
> +    channel_layout->order       = AV_CHANNEL_ORDER_NATIVE;
> +    channel_layout->nb_channels = av_popcount64(mask);
> +    channel_layout->u.mask      = mask;
> +
> +    return 0;
> +}

Probably a constructor for custom layout would also make sense:

int av_channel_layout_custom(AVChannelLayout *channel_layout, int nb_channels)

[...]

> +
> +int av_channel_layout_copy(AVChannelLayout *dst, const AVChannelLayout *src)
> +{
> +    av_channel_layout_uninit(dst);
> +    *dst = *src;
> +    if (src->order == AV_CHANNEL_ORDER_CUSTOM) {
> +        dst->u.map = av_malloc(src->nb_channels * sizeof(*dst->u.map));

av_malloc_array()

> +int av_channel_layout_describe_bprint(const AVChannelLayout *channel_layout,
> +                                      AVBPrint *bp)
> +{
> +    int i;
> +
> +    av_bprint_clear(bp);
> +
> +    switch (channel_layout->order) {
> +    case AV_CHANNEL_ORDER_NATIVE:
> +        for (i = 0; i < FF_ARRAY_ELEMS(channel_layout_map); i++)
> +            if (channel_layout->u.mask == channel_layout_map[i].layout.u.mask) {
> +                av_bprintf(bp, "%s", channel_layout_map[i].name);
> +                return 0;
> +            }
> +        // fall-through
> +    case AV_CHANNEL_ORDER_CUSTOM:
> +        for (i = 0; i < channel_layout->nb_channels; i++) {
> +            const char *ch_name = NULL;
> +            enum AVChannel ch = AV_CHAN_NONE;
> +
> +            if (channel_layout->order == AV_CHANNEL_ORDER_CUSTOM &&
> +                channel_layout->u.map[i].name[0])
> +                ch_name = channel_layout->u.map[i].name;
> +            if (channel_layout->order == AV_CHANNEL_ORDER_NATIVE || !ch_name) {
> +                ch = av_channel_layout_channel_from_index(channel_layout, i);
> +                ch_name = get_channel_name(ch);
> +            }

You are mixing channel labels and channel designations, and it can be the 
source of confusion. I guess the main problem is that the label of the 
channel can be totally unrelated to the channel designation.

You could show it as some combination. E.g. FL at label. This way there 
will be no confusion, that part before @ is the designation, part after 
the @ is the channel label.

[..]

> +enum AVChannel
> +av_channel_layout_channel_from_string(const AVChannelLayout *channel_layout,
> +                                      const char *name)
> +{
> +    int channel, ret;
> +
> +    switch (channel_layout->order) {
> +    case AV_CHANNEL_ORDER_CUSTOM:
> +        for (int i = 0; i < channel_layout->nb_channels; i++) {
> +            if (channel_layout->u.map[i].name[0] && !strcmp(name, channel_layout->u.map[i].name))
> +                return channel_layout->u.map[i].id;

This is the reverse case, I also find it confusing that name can be a 
label and a designation at the same time and it depends on the channel 
layout type which one is it... Yet again, some syntax could help. E.g. a 
string without @ is a designation, @label is a label without any filter 
for channel designation, designation at label filters both.

[..]

> +int av_channel_layout_index_from_string(const AVChannelLayout *channel_layout,
> +                                        const char *name)
> +{
> +    enum AVChannel ch;
> +
> +    switch (channel_layout->order) {
> +    case AV_CHANNEL_ORDER_CUSTOM:
> +        for (int i = 0; i < channel_layout->nb_channels; i++) {
> +            if (channel_layout->u.map[i].name[0] && !strcmp(name, channel_layout->u.map[i].name))
> +                return i;

Same here.

[..]

> +int av_channel_layout_check(const AVChannelLayout *channel_layout)

av_channel_layout_valid would be more readable.

Thanks,
Marton