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

[Marton Balint]

On Wed, 15 Dec 2021, James Almer wrote:

>
>
> On 12/15/2021 7:24 AM, Marton Balint wrote:
>>
>>
>>  On Tue, 14 Dec 2021, James Almer wrote:
>>
>>>  On 12/14/2021 3:54 PM, Nicolas George wrote:
>>>>   James Almer (12021-12-14):
>>>>>   We add a const uint8_t* field to AVChannelCustom. If the user wants to
>>>>>   allocate the strings instead, and not worry about their lifetime, they
>>>>>   can
>>>>>   provide the layout with a custom free() function that will take care
>>>>>  of
>>>>>   cleaning anything they came up with on uninit().
>>>>
>>>>   I understood what you suggested, and it amounts to letting API users
>>>>   fend for themselves. We can do that, but I would prefer if we only did
>>>>   on last resort, if we do not find a more convenient solution.
>>>
>>>  There's "char name[16]". Bigger than a pointer (Could be 8 bytes instead,
>>>  but then it's kinda small). The user will not have to worry about the
>>>  lifetime of anything then.
>>>
>>>  I'm attaching a diff that goes on top of the last patch of this set
>>>  implementing this. It also adds support for these custom names to
>>>  av_channel_layout_describe(), av_channel_layout_index_from_string() and
>>>  av_channel_layout_channel_from_string(), including tests.
>>
>>  I'd rather not mix custom labels with our fixed names for channels. E.g.
>>  what if a label conflicts with our existing channel names? If the user
>>  wants to specify a channel based on its label, that should be a separate
>>  syntax IMHO.
>>
>>  Overall, having a char name[16] is still kind of limiting, e.g. a label
>>  read from a muxed file will be truncated, I'd rather not have anything.
>
> Container metadata is typically be exported as stream metadata or side data.

That is always a possibility, but if you want some data per-channel, and 
not per-stream, (e.g. variable size labels) then side data becomes 
difficult to work with.

>
>>
>>  Here is one more idea, kind of a mix of what I read so far: Let's refcount
>>  only the dynamically allocated part of AVChannelLayout. Something like:
>>
>>  typedef struct AVChannelLayout {
>>       enum AVChannelOrder order;
>>       int nb_channels;
>>       uint64_t mask;
>>       AVBufferRef *custom;
>>  } AVChannelLayout;
>>
>>  And the reference counted data could point to an array of AVChannelCustom
>>  entries. And AVChannelCustom can have AVDictionary *metadata, because it
>>  is refcounted.
>
> AVBufferRef is meant to hold flat arrays, though.

Since sizeof(AVChannelCustom) is fixed, I don't really see the difference.

> the custom channel names? As a fixed array like in the above, or in the 
> dictionary? The latter sounds like a nightmare for both the helpers and as an 
> API user.

Personally I think it can be put in the metadata dictionary, because if 
we work out some syntax to select channel based on their labels, then it 
also make sense to select channels based on other metadata, and syntax 
might be extended with it. E.g. FL.label.Oboe. or FL.language.eng.

But if for some reason that is frowned upon, we can extend AVChannelCustom 
with a char *label, and free it as well on free().


> Also, since AVChannelCustom would have a dictionary, the AVBufferRef holding 
> it needs a custom free() function. This makes creating layouts manually a 
> pain.

You are already initalizing the dynamically allocated part of 
AVChannelLayout:

layout.u.map = av_mallocz_array(nb_channels, sizeof());

If AVBufferRef is used for that, then a helper function can be added:

layout.custom = av_channel_layout_custom_new(nb_channels);

No difference really.


> And what about the buffer being writable or not? You need to consider 
> both copy and ref scenarios.

av_channel_layout_copy() can simply ref. If a deep copy is needed because 
the user wants to change anything in AVChannelCustom, then helper function 
can be added.

>
> It's a lot of added complexity for what should be a simple "this stream with 
> six channels has the channels arranged like this in your room: This is front 
> left, this is front right, this is 'under the carpet', etc".

See the attached proof-of-concept patch, to be used on top of your 
channel_layout branch. Is it that bad?

Thanks,
Marton
-------------- next part --------------
A non-text attachment was scrubbed...
Name: channellayout-bufferref-poc.patch
Type: text/x-patch
Size: 12123 bytes
Desc: 
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20211215/098148d6/attachment.bin>