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

[Marton Balint]

On Fri, 10 Dec 2021, Hendrik Leppkes wrote:

> On Fri, Dec 10, 2021 at 12:06 AM Marton Balint <cus at passwd.hu> wrote:
>>
>>
>>
>> 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.
>>
>
> Pointers add a lot of trouble with ownership and memory management.
> Wouldn't be so bad if its just the pointer without all that management
> baggage.

Sure, but since you are dynamically allocating the 
custom map AVChannelLayout->u.map, you are already managing memory...
E.g. you could also store an array of AVDictionaries in AVChannelLayout.

Regards,
Marton