On 12/16/2021 8:27 PM, Marton Balint wrote:
On Thu, 16 Dec 2021, James Almer wrote:
- Added a 16 byte fixed array to AVChannelCustom to give custom
labels to channels in Custom order layouts. These labes will
be used by the helpers when querying channels by name or
describing the layout.
I don't think this is a good idea to use the labels directly in helpers
instead of the channel designation names. See more below.
It is also not great that if the user wants to label a channel, he has
to use a custom layout. So I'd rather remove the name field from
AVChannelCustom, I find it limited anyway.
[..]
+enum AVChannel av_channel_from_string(const char *str)
+{
+ int i;
+ char *endptr = (char *)str;
+ enum AVChannel id = AV_CHAN_NONE;
+ for (i = 0; i < FF_ARRAY_ELEMS(channel_names); i++) {
+ if (channel_names[i].name && !strcmp(str,
channel_names[i].name))
+ return i;
+ }
+ if (!strncmp(str, "USR", 3)) {
+ const char *p = str + 3;
+ id = strtol(p, &endptr, 0);
+ }
+ if (id > 0 && !*endptr)
id >= 0
[..]
+int av_channel_layout_from_mask(AVChannelLayout *channel_layout,
+ uint64_t mask)
+{
+ if (!mask)
+ return AVERROR(EINVAL);
+
+ channel_layout->order = AV_CHANNEL_ORDER_NATIVE;
+ channel_layout->nb_channels = av_popcount64(mask);
+ channel_layout->u.mask = mask;
+
+ return 0;
+}
Probably a constructor for custom layout would also make sense:
int av_channel_layout_custom(AVChannelLayout *channel_layout, int
nb_channels)
Doesn't seem too useful to me. It will basically just call av_malloc for
u.map for you. You still need to fill the array with the channels in the
order you want it, unlike av_channel_layout_from_mask() which gives you
a usable layout directly on return.
You can use av_channel_layout_from_string() for this instead, thanks to
your suggestion to use generic USR%d designations for no-name ids. In
the tests from patch 2 i have a couple examples of it.
[...]
+
+int av_channel_layout_copy(AVChannelLayout *dst, const
AVChannelLayout *src)
+{
+ av_channel_layout_uninit(dst);
+ *dst = *src;
+ if (src->order == AV_CHANNEL_ORDER_CUSTOM) {
+ dst->u.map = av_malloc(src->nb_channels * sizeof(*dst->u.map));
av_malloc_array()
+int av_channel_layout_describe_bprint(const AVChannelLayout
*channel_layout,
+ AVBPrint *bp)
+{
+ int i;
+
+ av_bprint_clear(bp);
+
+ switch (channel_layout->order) {
+ case AV_CHANNEL_ORDER_NATIVE:
+ for (i = 0; i < FF_ARRAY_ELEMS(channel_layout_map); i++)
+ if (channel_layout->u.mask ==
channel_layout_map[i].layout.u.mask) {
+ av_bprintf(bp, "%s", channel_layout_map[i].name);
+ return 0;
+ }
+ // fall-through
+ case AV_CHANNEL_ORDER_CUSTOM:
+ for (i = 0; i < channel_layout->nb_channels; i++) {
+ const char *ch_name = NULL;
+ enum AVChannel ch = AV_CHAN_NONE;
+
+ if (channel_layout->order == AV_CHANNEL_ORDER_CUSTOM &&
+ channel_layout->u.map[i].name[0])
+ ch_name = channel_layout->u.map[i].name;
+ if (channel_layout->order == AV_CHANNEL_ORDER_NATIVE ||
!ch_name) {
+ ch =
av_channel_layout_channel_from_index(channel_layout, i);
+ ch_name = get_channel_name(ch);
+ }
You are mixing channel labels and channel designations, and it can be
the source of confusion. I guess the main problem is that the label of
the channel can be totally unrelated to the channel designation.
You could show it as some combination. E.g. FL@label. This way there
will be no confusion, that part before @ is the designation, part after
the @ is the channel label.
[..]
+enum AVChannel
+av_channel_layout_channel_from_string(const AVChannelLayout
*channel_layout,
+ const char *name)
+{
+ int channel, ret;
+
+ switch (channel_layout->order) {
+ case AV_CHANNEL_ORDER_CUSTOM:
+ for (int i = 0; i < channel_layout->nb_channels; i++) {
+ if (channel_layout->u.map[i].name[0] && !strcmp(name,
channel_layout->u.map[i].name))
+ return channel_layout->u.map[i].id;
This is the reverse case, I also find it confusing that name can be a
label and a designation at the same time and it depends on the channel
layout type which one is it... Yet again, some syntax could help. E.g. a
string without @ is a designation, @label is a label without any filter
for channel designation, designation@label filters both.
[..]
+int av_channel_layout_index_from_string(const AVChannelLayout
*channel_layout,
+ const char *name)
+{
+ enum AVChannel ch;
+
+ switch (channel_layout->order) {
+ case AV_CHANNEL_ORDER_CUSTOM:
+ for (int i = 0; i < channel_layout->nb_channels; i++) {
+ if (channel_layout->u.map[i].name[0] && !strcmp(name,
channel_layout->u.map[i].name))
+ return i;
Same here.
[..]
+int av_channel_layout_check(const AVChannelLayout *channel_layout)
av_channel_layout_valid would be more readable.
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".
