[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 6:32 PM, Marton Balint wrote:
>>
>>
>>  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.
>
> A flat array of bytes. If you do a copy of the contents of the buffer 
> (av_buffer_make_writable), and there's a pointer in it, you're copying it but 
> not what it points to. The copy is not a reference, so when the last actual 
> reference is freed, the custom free() function is called, it frees the 
> dictionary, and the copy now has a dangling pointer to a non existent 
> dictionary.

OK, I understand. Wrapped avframe misuses AVBufferRef similarly however, 
and that also should be fixed then sometime... Maybe with an optional 
copy() function for AVBuffer?

>>>  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?
>
> It's fragile by misusing the AVBufferRef API. Also, why remove the pointer 
> from the union?

Because you may want additional metadata without having a custom 
channel layout.

>
> I'll send a version with a flat name array plus an opaque pointer. Then an 
> email listing all the proposed solutions, to be discussed in a call where we 
> hopefully reach an agreement.

OK.

Thanks,
Marton