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

[James Almer]

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.

> 
> 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. And where would you 
store 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.
Also, since AVChannelCustom would have a dictionary, the AVBufferRef 
holding it needs a custom free() function. This makes creating layouts 
manually a pain. And what about the buffer being writable or not? You 
need to consider both copy and ref scenarios.

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".
People want this to automatically handle very specific filtering 
scenarios to remove one entry in their chain when it's not its job.

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