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

[Marton Balint]

On Thu, 9 Dec 2021, Anton Khirnov wrote:

> Quoting Lynne (2021-12-08 13:57:45)
>> 8 Dec 2021, 13:16 by anton at khirnov.net:
>>
>>> Quoting Lynne (2021-12-08 10:02:34)
>>>
>>>> 8 Dec 2021, 02:06 by jamrial at gmail.com:
>>>>
>>>>> 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 | 498 ++++++++++++++++++++++++++++++------
>>>>>  libavutil/channel_layout.h | 510 ++++++++++++++++++++++++++++++++++---
>>>>>  libavutil/version.h        |   1 +
>>>>>  3 files changed, 906 insertions(+), 103 deletions(-)
>>>>>
>>>>>  /**
>>>>>  * @}
>>>>> @@ -128,6 +199,157 @@ enum AVMatrixEncoding {
>>>>>  AV_MATRIX_ENCODING_NB
>>>>>  };
>>>>>
>>>>> +/**
>>>>> + * @}
>>>>> + */
>>>>> +
>>>>> +/**
>>>>> + * An AVChannelCustom defines a single channel within a custom order layout
>>>>> + *
>>>>> + * Unlike most structures in FFmpeg, sizeof(AVChannelCustom) is a part of the
>>>>> + * public ABI.
>>>>> + *
>>>>> + * No new fields may be added to it without a major version bump.
>>>>> + */
>>>>> +typedef struct AVChannelCustom {
>>>>> +    enum AVChannel id;
>>>>> +} AVChannelCustom;
>>>>>
>>>>
>>>> Consider adding a 25-byte or so of a description field string here that
>>>> would also be copied if the frame/layout is copied?
>>>> That way, users can assign a description label to each channel,
>>>> for example labeling each channel in a 255 channel custom layout
>>>> Opus stream used in production at a venue somewhere.
>>>> Also, consider additionally reserving 16-bytes here, just in case.
>>>>
>>>
>>> That would enlarge the layout size by a significant amount. Keep in mind
>>> that this is all per frame. And for something that very few people would
>>> want to use.
>>>
>>> These descriptors, if anyone really needs them, can live in side data.
>>>
>>
>> That's fine with me.
>> Can we at least have an opaque uint64_t? I'd accept a uintptr_t,
>> or an int too, or even a single-byte uint8_t.
>
> like this?
>
> diff --git a/libavutil/channel_layout.h b/libavutil/channel_layout.h
> index 7b77a74b61..7b2ba12532 100644
> --- a/libavutil/channel_layout.h
> +++ b/libavutil/channel_layout.h
> @@ -252,7 +252,8 @@ enum AVMatrixEncoding {
>  * No new fields may be added to it without a major version bump.
>  */
> typedef struct AVChannelCustom {
> -    enum AVChannel id;
> +    enum AVChannel id:16;
> +    int      reserved:16;
> } AVChannelCustom;

A dynamically allocated field is so bad here? E.g.

AVDictionary *metadata

You can store labels. You can store group names. You can store language. 
You can store sound positions, GPS coordiantes, whatever.

Yes, it will be slow. But it is rarely used for in normal cases, and it 
can be NULL usually.

Regards,
Marton