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

[James Almer]

On 1/16/2022 7:54 PM, Marton Balint wrote:
> 
> 
> On Sun, 16 Jan 2022, Nicolas George wrote:
> 
>> James Almer (12022-01-12):
>>> From: Anton Khirnov <anton at khirnov.net>
>>>
>>> The new API is more extensible and allows for custom layouts.
>>> More accurate information is exported, eg for decoders that do not
>>> set a channel layout, lavc will not make one up for them.
>>>
>>> Deprecate the old API working with just uint64_t bitmasks.
>>>
>>> Expanded and completed by Vittorio Giovara <vittorio.giovara at gmail.com>
>>> and James Almer <jamrial at gmail.com>.
>>> Signed-off-by: Vittorio Giovara <vittorio.giovara at gmail.com>
>>> Signed-off-by: James Almer <jamrial at gmail.com>
>>> ---
>>>  libavutil/channel_layout.c | 629 ++++++++++++++++++++++++++++++++-----
>>>  libavutil/channel_layout.h | 542 ++++++++++++++++++++++++++++++--
>>>  libavutil/version.h        |   1 +
>>>  3 files changed, 1069 insertions(+), 103 deletions(-)
>>
>> Thank you. I have no fundamental objection to the design of the API as
>> it is, but the user interface and documentation is still missing, that
>> needs to be addressed before the patch goes in.
>>
>> (But IIRC, Marton had other requirements, so let us wait for him to
>> weigh in.)
> 
> My extensible metadata idea was not popular, so I am willing to let it go.
> 
>>
>>> +/**
>>> + * Initialize a channel layout from a given string description.
>>> + * The input string can be represented by:
>>> + *  - the formal channel layout name (returned by 
>>> av_channel_layout_describe())
>>> + *  - single or multiple channel names (returned by av_channel_name()
>>> + *    or concatenated with "+")
>>> + *  - a hexadecimal value of a channel layout (eg. "0x4")
>>> + *  - the number of channels with default layout (eg. "5c")
>>> + *  - the number of unordered channels (eg. "4", "4C", or "4 channels")
>>> + *
>>> + * @param channel_layout input channel layout
>>> + * @param str string describing the channel layout
>>> + * @return 0 channel layout was detected, AVERROR_INVALIDATATA 
>>> otherwise
>>> + */
>>> +int av_channel_layout_from_string(AVChannelLayout *channel_layout,
>>> +                                  const char *str);
>>
>> The documentation for the syntax needs to be in the user documentation,
>> with examples, not just in the API documentation.
>>
>>> +/**
>>> + * This is the inverse function of @ref av_channel_name().
>>> + *
>>> + * @return the channel with the given name
>>> + *         AV_CHAN_NONE when name does not identify a known channel
>>> + */
>>> +enum AVChannel av_channel_from_string(const char *name);
>>
>>> +/**
>>> + * Get a channel described by the given string.
>>> + *
>>> + * This function accepts channel names in the same format as
>>> + * @ref av_channel_from_string().
>>> + *
>>> + * @param channel_layout input channel layout
>>> + * @return a channel described by the given string, or a negative 
>>> AVERROR value.
>>> + */
>>> +int av_channel_layout_channel_from_string(const AVChannelLayout 
>>> *channel_layout,
>>> +                                          const char *name);
>>
>> This looks to be the preferred function for when the user will specify a
>> channel in a layout.
>>
>> First, this fact should be stated clearly in the introduction of the
>> documentation. Otherwise, people will likely use other functions,
>> probably av_channel_layout_channel_from_index().
>>
>> Second, the "name" argument cannot be just a name argument: the user
>> must be able to say "the third FC channel" or "the FC channel with name
>> 'piano'". And probably both at once.
>>
>> idx = av_channel_layout_channel_from_string(layout, "FC");
>> idx = av_channel_layout_channel_from_string(layout, "FC#2");
>> idx = av_channel_layout_channel_from_string(layout, "FC[piano]");
>> idx = av_channel_layout_channel_from_string(layout, "FC[piano]#2");
>>
>> (I think it would be acceptable to limit the name, for example "names
>> with non-alphanumeric ASCII characters are not supported.)
>>
>> And this need to go in the user documentation.
>>
>> I am not sure if we also need a function to extract "all the FL channels
>> with name 'piano'".
> 
> Before discussing channel selection syntax, can we discuss serialization?
> 
> Should the serializaton and unserialization functions store/parse both 
> the channel label and the channel designation? As far as I see right now 
> it is kind of inconsistent: av_channel_layout_describe_print() prints 
> the label (if exists), not the designation, but 
> av_channel_layout_from_string() expects the designations only, never the 
> custom label.

You're still thinking there's a distinction, when i already told you 
that there is none. I added the name field because people wanted to give 
non standard channel names, and maybe also change the standard ones too. 
It's not a label to go alongside a designation, it's *a* name.
There are about 20 channels that have a standard name from waveformat 
and extensions, while the rest lack one. You can obviously have a non 
standard speaker setup with 50 channels, and all those extra speakers 
can surely have a name based on their position (Say, RTFC, to refer to 
"right of top front center"), so the field lets you give it to them if 
that's convenient for you and you want a pretty print output of the 
layout without seeing things like USR49. That's it.

Yes, av_channel_layout_from_string() will not be able to parse the 
output of av_channel_layout_from_describe() if you gave channels a 
custom name, since they are specifically from that other layout. There's 
no way around that, as we can't make describe() output some string that 
from_string() can interpret for those because then describe() will be 
useless for printing the layout for human readability purposes.
It is in fact a good reason to either remove the name field or stop 
making these helpers look at it, since describe() is meant to create 
strings from_string() can parse.

I personally would do just that and keep the opaque fields alone. 
Otherwise I'll make the helpers stop looking at it, since after your 
request, non standard channels can be addressed as USR#, so you can 
create layout from strings with them just fine.
The opaque field is more than enough to give channels custom names and 
even implement all kinds of crazy unconventional inter-filter usage 
within libavfilter like Nicolas wants to without overdesigning the 
helpers for virtually unused scenarios outside of our own libraries.

> 
> There is still confusion what custom channel labels should be used for. 
> Is it only used for string labels for user channel designations or is it 
> used by the end user to tag its channels unrelated to their designation? 
> "Piano" example suggest the latter, but API is designed for the former...

It's what i said above. The piano stuff is Nicolas personal usecase for 
libavfilter internal behavior that's beyond the scope of these helpers.

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