[FFmpeg-devel] [PATCH 001/279] Add a new channel layout API
James Almer
jamrial at gmail.com
Tue Dec 14 03:27:42 EET 2021
On 12/13/2021 10:13 PM, Marton Balint wrote:
>
>
> On Tue, 7 Dec 2021, James Almer wrote:
>
>> From: Anton Khirnov <anton at 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 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".
More information about the ffmpeg-devel
mailing list