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

[James Almer]

On 12/14/2021 5:27 AM, Marton Balint wrote:
> 
> 
> 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 "?".

It's also used by the legacy API now that you mention it, and returning 
"?" is a behavior change, so thanks for noticing it.

> 
>>
>>>
>>>>      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.

What do you suggest? USER%d is IMO ugly. Maybe ChID%d? Although both are 
a bit long.

> 
>>>
>>>  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.

You could attach it dynamically allocated strings too, and to prevent 
the need for the module allocating them to outlive the layout, we could 
add an opaque pointer to AVChannelLayout (not AVChannelCustom) and a 
user set free() call back to pass that pointer to, on uninit().

> 
> Regards,
> 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".