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

[James Almer <jamrial@gmail.com>]

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.

It's the reason we used to have split side data to handle non flat array 
structures.

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

Same situation as above. A fixed array would be ok, though.

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

It's fragile by misusing the AVBufferRef API. Also, why remove the 
pointer from the union?

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.

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