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-requ...@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-requ...@ffmpeg.org with subject "unsubscribe".
