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.
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.
+ errno = 0;
+ channels = strtol(str, &end, 10);
+
+ /* number of channels */
+ if (!errno && *end == 'c' && !*(end + 1) && channels >= 0) {
+ av_channel_layout_default(channel_layout, channels);
+ return 0;
+ }
+
+ /* number of unordered channels */
+ if (!errno && (!*end || av_strnstr(str, "channels", strlen(str)))
&& channels >= 0) {
This is missing the syntax used in av_get_extended_channel_layout(), 1C
to 63C for unspecified channel layout with that many number of channels.
Ok.
[...]
+int av_channel_layout_describe(const AVChannelLayout *channel_layout,
+ char *buf, size_t buf_size)
+{
Can you make a function variant of this which directly gets a AVBPrint
buffer? Similar to av_bprint_channel_layout.
Yes, i was going to do that, following the other discussion.
[...]
+int av_channel_layout_check(const AVChannelLayout *channel_layout)
av_channel_layout_is_valid() seems like a better name.
[...]
diff --git a/libavutil/channel_layout.h b/libavutil/channel_layout.h
index d39ae1177a..018e87ff0b 100644
--- a/libavutil/channel_layout.h
+++ b/libavutil/channel_layout.h
@@ -23,6 +23,10 @@
#define AVUTIL_CHANNEL_LAYOUT_H
#include <stdint.h>
+#include <stdlib.h>
+
+#include "version.h"
+#include "attributes.h"
/**
* @file
@@ -34,6 +38,68 @@
* @{
*/
+enum AVChannel {
+ ///< Invalid channel index
+ AV_CHAN_NONE = -1,
+ AV_CHAN_FRONT_LEFT,
+ AV_CHAN_FRONT_RIGHT,
+ AV_CHAN_FRONT_CENTER,
+ AV_CHAN_LOW_FREQUENCY,
+ AV_CHAN_BACK_LEFT,
+ AV_CHAN_BACK_RIGHT,
+ AV_CHAN_FRONT_LEFT_OF_CENTER,
+ AV_CHAN_FRONT_RIGHT_OF_CENTER,
+ AV_CHAN_BACK_CENTER,
+ AV_CHAN_SIDE_LEFT,
+ AV_CHAN_SIDE_RIGHT,
+ AV_CHAN_TOP_CENTER,
+ AV_CHAN_TOP_FRONT_LEFT,
+ AV_CHAN_TOP_FRONT_CENTER,
+ AV_CHAN_TOP_FRONT_RIGHT,
+ AV_CHAN_TOP_BACK_LEFT,
+ AV_CHAN_TOP_BACK_CENTER,
+ AV_CHAN_TOP_BACK_RIGHT,
+ /** Stereo downmix. */
+ AV_CHAN_STEREO_LEFT = 29,
+ /** See above. */
+ AV_CHAN_STEREO_RIGHT,
+ AV_CHAN_WIDE_LEFT,
+ AV_CHAN_WIDE_RIGHT,
+ AV_CHAN_SURROUND_DIRECT_LEFT,
+ AV_CHAN_SURROUND_DIRECT_RIGHT,
+ AV_CHAN_LOW_FREQUENCY_2,
+ AV_CHAN_TOP_SIDE_LEFT,
+ AV_CHAN_TOP_SIDE_RIGHT,
+ AV_CHAN_BOTTOM_FRONT_CENTER,
+ AV_CHAN_BOTTOM_FRONT_LEFT,
+ AV_CHAN_BOTTOM_FRONT_RIGHT,
+
+ /** Channel is empty can be safely skipped. */
+ AV_CHAN_SILENCE = 64,
I'd rather name this AV_CHAN_GARBAGE or AV_CHAN_UNUSED instead. Silence
could be useful, and a silent channel could be FRONT LEFT at the same
time, but AV_CHAN_SILENCE is not a flag. But based on the comment this
is simply a channel which should be ignored, because it does not contain
anything playable/presentable.
Other already said, but an unknown designation (AV_CHAN_UNKNOWN) should
be added. Which is a useful channel, but with unknown or unsupported
designation.
[...]
+ * An AVChannelCustom defines a single channel within a custom order
layout
+ *
+ * Unlike most structures in FFmpeg, sizeof(AVChannelCustom) is a
part of the
+ * public ABI.
+ *
+ * No new fields may be added to it without a major version bump.
+ */
+typedef struct AVChannelCustom {
+ enum AVChannel id;
+} AVChannelCustom;
+
+/**
+ * An AVChannelLayout holds information about the channel layout of
audio data.
+ *
+ * A channel layout here is defined as a set of channels ordered in a
specific
+ * way (unless the channel order is AV_CHANNEL_ORDER_UNSPEC, in which
case an
+ * AVChannelLayout carries only the channel count).
+ *
+ * Unlike most structures in Libav, sizeof(AVChannelLayout) is a part
of the
+ * public ABI and may be used by the caller. E.g. it may be allocated
on stack
+ * or embedded in caller-defined structs.
+ *
+ * AVChannelLayout can be initialized as follows:
+ * - default initialization with {0}, followed by by setting all used
fields
+ * correctly;
+ * - by assigning one of the predefined AV_CHANNEL_LAYOUT_*
initializers;
+ * - with a constructor function, such as av_channel_layout_default(),
+ * av_channel_layout_from_mask() or av_channel_layout_from_string().
+ *
+ * The channel layout must be unitialized with
av_channel_layout_uninit()
+ * (strictly speaking this is not necessary for
AV_CHANNEL_ORDER_NATIVE and
+ * AV_CHANNEL_ORDER_UNSPEC, but we recommend always calling
+ * av_channel_layout_uninit() regardless of the channel order used).
I'd remove this comment, that it might not be necessary to uninit. New
API, please always uninit().
Ok.
+ *
+ * Copying an AVChannelLayout via assigning is forbidden,
+ * av_channel_layout_copy() must be used. instead (and its return
value should
+ * be checked)
+ *
+ * No new fields may be added to it without a major version bump,
except for
+ * new elements of the union fitting in sizeof(uint64_t).
+ */
+typedef struct AVChannelLayout {
+ /**
+ * Channel order used in this layout.
+ * This is a mandatory field.
+ */
+ enum AVChannelOrder order;
+
+ /**
+ * Number of channels in this layout. Mandatory field.
+ */
+ int nb_channels;
+
+ /**
+ * Details about which channels are present in this layout.
+ * For AV_CHANNEL_ORDER_UNSPEC, this field is undefined and must
not be
+ * used.
+ */
+ union {
+ /**
+ * This member must be used for AV_CHANNEL_ORDER_NATIVE.
+ * It is a bitmask, where the position of each set bit means
that the
+ * AVChannel with the corresponding value is present.
+ *
+ * I.e. when (mask & (1 << AV_CHAN_FOO)) is non-zero, then
AV_CHAN_FOO
+ * is present in the layout. Otherwise it is not present.
+ *
+ * @note when a channel layout using a bitmask is constructed or
+ * modified manually (i.e. not using any of the
av_channel_layout_*
+ * functions), the code doing it must ensure that the number
of set bits
+ * is equal to nb_channels.
+ */
+ uint64_t mask;
+ /**
+ * This member must be used when the channel order is
+ * AV_CHANNEL_ORDER_CUSTOM. It is a nb_channels-sized array,
with each
+ * element signalling the presence of the AVChannel with the
+ * corresponding value in map[i].id.
+ *
+ * I.e. when map[i].id is equal to AV_CHAN_FOO, then
AV_CH_FOO is the
+ * i-th channel in the audio data.
+ */
+ AVChannelCustom *map;
+ } u;
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.
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".
