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

[Marton Balint <cus@passwd.hu>]

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