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

[James Almer <jamrial@gmail.com>]

On 12/15/2021 7:55 PM, Marton Balint wrote:
> 
> 
> 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?

Wrapped avframe is an abomination i tried to fix but my patch was rejected.

I also tried to add a copy() callback to AVBufferRef some time ago, and 
it was also rejected, as the API was meant to be for flat arrays and it 
simply became the defactor refcounter because there was nothing else.
So other uses should be done with a different, more adequate and 
extensible kind of refcounting API. Anton and Nicolas both suggested two 
different approaches for it.

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

Then it's no longer a struct meant for the Custom order. If you allow 
using it for native, unspec or ambisonic, what will the id field 
represent? If you say "make it be undefined", then it becomes wasted space.

> 
>>
>> 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
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".