On 12/14/2021 5:27 AM, Marton Balint wrote:
On Mon, 13 Dec 2021, James Almer wrote:
On 12/13/2021 10:13 PM, Marton Balint wrote:
On Tue, 7 Dec 2021, James Almer wrote:
From: Anton Khirnov <an...@khirnov.net>
[...]
-static const char *get_channel_name(int channel_id)
+static const char *get_channel_name(enum AVChannel channel_id)
{
- if (channel_id < 0 || channel_id >=
FF_ARRAY_ELEMS(channel_names))
- return NULL;
+ if ((unsigned) channel_id >= FF_ARRAY_ELEMS(channel_names) ||
+ !channel_names[channel_id].name)
+ return "?";
I'd rather return NULL here instead of "?". Is this allowed to
happen by
the way?
Yes, even in a native layout, several AVChannel values have no name.
The idea is to print a string like "FL|FR|?" when the name of a
channel is unknown. See the test in patch 2.
As far as I see this function is used by av_channel_layout_describe. If
get_channel_name returned NULL, then av_channel_layout_describe could
print Ch%d as well, instead of a "?".
It's also used by the legacy API now that you mention it, and returning
"?" is a behavior change, so thanks for noticing it.
return channel_names[channel_id].name;
}
-static const struct {
+static inline void get_channel_str(AVBPrint *bp, const char *str,
+ enum AVChannel channel_id)
+{
+ if (str)
+ av_bprintf(bp, "%s", str);
+ else
+ av_bprintf(bp, "?");
If this is not allowed, then you should propagate back
AVERROR(EINVAL) if
it does. If it is allowed, because the user can use some custom
channel_id-s, then something like "USER%d" should be returned IMHO.
How about Ch%d? It would also further improve the usefulness of
av_channel_layout_from_string() by looking for such strings.
I find "Ch%d" a bit confusing, beacuse "Ch%d" would normally mean the
n-th channel in a layout, not a channel with the n-th ID.
What do you suggest? USER%d is IMO ugly. Maybe ChID%d? Although both are
a bit long.
I would like to attach some extendable, possibly per-channel
metadata to
the channel layout. I'd rather put it into AVChannelLayout, so native
layouts could also have metadata. This must be dynamically allocated to
make it extendable and to not consume too many bytes. I can accept
that it
will be slow. But I don't see it unmanagable, because AVChannelLayout
already have functions for allocation/free/copy. I also think that
most of
the time it will not be used (e.g. if metadata is NULL, that can
mean no
metadata for all the channels).
If we can't decide what this should be, array of AVDictionaries, some
side-data like approach but in the channel layout, or a new dynamically
allocated AVChannelLayoutMetadata struct, then maybe just reserve a
void*
in the end of the AVChannelLayout, and we can work it out later.
How about a "const uint8_t *name" in AVChannelCustom? You can then
point to some string that will not be owned by the layout, nor
duplicated on copy, and that will be used by the helpers
(av_channel_layout_channel_from_string,
av_channel_layout_index_from_string, av_channel_layout_describe) to
identify the channel.
This avoids doing dynamic allocations per channel.
But this kind of limits the names to compile time string constants. I'd
rather not add something so limited.
You could attach it dynamically allocated strings too, and to prevent
the need for the module allocating them to outlive the layout, we could
add an opaque pointer to AVChannelLayout (not AVChannelCustom) and a
user set free() call back to pass that pointer to, on uninit().
Regards,
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".
