[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)

Doesn't seem too useful to me. It will basically just call av_malloc for 
u.map for you. You still need to fill the array with the channels in the 
order you want it, unlike av_channel_layout_from_mask() which gives you 
a usable layout directly on return.

You can use av_channel_layout_from_string() for this instead, thanks to 
your suggestion to use generic USR%d designations for no-name ids. In 
the tests from patch 2 i have a couple examples of it.

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