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

[James Almer]

On 12/9/2021 8:31 PM, Marton Balint wrote:
> 
> 
> 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.

That's one per layout, whereas your suggestion is one per channel.

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