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

[Marton Balint]

On Mon, 13 Dec 2021, James Almer wrote:

>
>
> On 12/13/2021 10:13 PM, Marton Balint wrote:
>>
>>
>>  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?
>
> Yes, even in a native layout, several AVChannel values have no name. The idea 
> is to print a string like "FL|FR|?" when the name of a channel is unknown. 
> See the test in patch 2.

As far as I see this function is used by av_channel_layout_describe. If 
get_channel_name returned NULL, then av_channel_layout_describe could 
print Ch%d as well, instead of a "?".

>
>>
>>>      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.
>
> How about Ch%d? It would also further improve the usefulness of 
> av_channel_layout_from_string() by looking for such strings.

I find "Ch%d" a bit confusing, beacuse "Ch%d" would normally mean the n-th 
channel in a layout, not a channel with the n-th ID.

>>
>>  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.
>
> How about a "const uint8_t *name" in AVChannelCustom? You can then point to 
> some string that will not be owned by the layout, nor duplicated on copy, and 
> that will be used by the helpers (av_channel_layout_channel_from_string, 
> av_channel_layout_index_from_string, av_channel_layout_describe) to identify 
> the channel.
> This avoids doing dynamic allocations per channel.

But this kind of limits the names to compile time string constants. I'd 
rather not add something so limited.

Regards,
Marton