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

[James Almer]

On 12/16/2021 8:27 PM, Marton Balint wrote:
> 
> 
> 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.

The custom names are meant for channels without a known name. If you 
want to call FL something else you can, and address it by it or FL, but 
it's not the main objective.

> 
> [..]
> 
>> +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.

Again, the custom names are not meant to be something separate from the 
standard names. The latter exists for like 25 ids, so adding custom 
names will let you address the rest in some form.

> 
> [..]
> 
>> +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.

Ok.

> 
> Thanks,
> Marton
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".