Re: [libav-devel] [PATCH 01/25] Add a new channel layout API

[Anton Khirnov]

Quoting Vittorio Giovara (2017-06-29 00:10:45)
> +
> +enum AVChannelOrder {
> +/**
> + * The native channel order, i.e. the channels are in the same order in
> + * which they are defined in the AVChannel enum. This supports up to 63
> + * different channels.
> + */
> +AV_CHANNEL_ORDER_NATIVE,
> +/**
> + * The channel order does not correspond to any other predefined order 
> and
> + * is stored as an explicit map. For example, this could be used to 
> support
> + * layouts with 64 or more channels, or with channels that could be 
> skipped.
> + */
> +AV_CHANNEL_ORDER_CUSTOM,
> +/**
> + * Only the channel count is specified, without any further information
> + * about the channel order.

This used to be just 'about the channels', and I think that was more
correct -- here we lack any informations about what the channels are,
not just their ordering. Alternatively 'about the channel order or
semantics'.

> +
> +/**
> + * Initialize a channel layout from a given string description.
> + * The input string can be represented by:
> + *  - the formal channel layout name (returned by 
> av_channel_layout_describe())
> + *  - single or multiple channel names (returned by av_channel_name()
> + *or concatenated with "|")

Shouldn't this be
 - single or multiple channel names (returned by av_channel_name())
   concatenated with "|"

-- 
Anton Khirnov
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH 01/25] Add a new channel layout API

[Vittorio Giovara]

On Thu, Jun 29, 2017 at 5:14 AM, wm4  wrote:
> On Wed, 28 Jun 2017 18:10:45 -0400
> Vittorio Giovara  wrote:
>>  /**
>> + * 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.
>
> You should specify how it has to be handled. You can:
> - default initialize it with {0} or by setting all used fields correctly
> - using a predefined layout as initializer (AV_CHANNEL_LAYOUT_STEREO
>   etc.)
> - initialize it with a constructor function
> - must be uninitialized with av_channel_layout_uninit() (at least in
>   some situations, which is weird)
> - copy via assigning is forbidden (probably?), av_channel_layout_copy()
>   must be used instead

Ok i'll add a verbatim of this description

>> + * No new fields may be added to it without a major version bump.
>
> I think it's still intended that you can add new fields to the union?
> So you will add new fields. You just won't change the size, or add
> new fields which are mandatory for already defined layout types.

ok I took it for granted, but mentioning it won't harm

>> + * An AVChannelLayout can be constructed using the convenience function
>> + * av_channel_layout_from_mask() / av_channel_layout_from_string(), or it 
>> can be
>> + * built manually by the caller.
>> + */
>> +typedef struct AVChannelLayout {
>> +/**
>> + * Channel order used in this layout.
>> + */
>
> ("Mandatory field.")

It is but it defaults to NATIVE order. I'll mention it anyway.

>> +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 presend of the AVChannel with the
>> + * corresponding value.
>> + *
>> + * I.e. when map[i] is equal to AV_CHAN_FOO, then AV_CH_FOO is the 
>> i-th
>> + * channel in the audio data.
>> + */
>> +enum AVChannel *map;
>
> Even if the channel map identifier is AV_CHAN_SILENCE? What does the
> data contain then, actual silence or undefined contents?

I suppose so, it will simply mean that the channel at position `i`
will be SILENCE.
I documented that channel as "empty", which is a little vague, do have
any suggestion?

>> +/**
>> + * @return a string describing a given channel.
>> + */
>> +const char *av_channel_name(enum AVChannel channel);
>
> What does it return for invalid channels?

it returns "?", I'll mention it in the docs

>> +
>> +/**
>> + * @return a channel described by the given string.
>> + */
>> +int av_channel_from_string(const char *name);
>
> Return what exactly? I guess AVChannel or negative error code. Could
> also say it's the inverse of av_channel_name().

okay

>> +
>> +/**
>> + * Initialize a native channel layout from a bitmask indicating which 
>> channels
>> + * are present.
>> + */
>> +void av_channel_layout_from_mask(AVChannelLayout *channel_layout, uint64_t 
>> mask);
>
> What does it do if *channel_layout is not set to all 0 bytes?

Unpredictable. I'll mention that the input layout should be properly
initialized.
(same for the questions below)

>> +
>> +/**
>> + * Free any allocated data in the channel layout and reset the channel
>> + * count to 0.
>> + */
>> +void av_channel_layout_uninit(AVChannelLayout *channel_layout);
>
> Can the user assume that for defined channel orders, which do not use
> allocated memory (like channel mask), this 

Re: [libav-devel] [PATCH 01/25] Add a new channel layout API

[wm4]

On Wed, 28 Jun 2017 18:10:45 -0400
Vittorio Giovara  wrote:

> From: Anton Khirnov 
> 
> The new API is more extensible and allows for custom layouts.
> More accurate information is exported, eg for decoders that do not
> set a channel layout, lavc will not make one up for them.
> 
> Deprecate the old API working with just uint64_t bitmasks.
> 
> Expanded and completed by Vittorio Giovara .
> Signed-off-by: Vittorio Giovara 
> ---

I guess this is essentially the final version, so I'm bikeshedding a
little bit harder. We'll probably have to live forever with this API,
anyway.

>  /**
> + * 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.

You should specify how it has to be handled. You can:
- default initialize it with {0} or by setting all used fields correctly
- using a predefined layout as initializer (AV_CHANNEL_LAYOUT_STEREO
  etc.)
- initialize it with a constructor function
- must be uninitialized with av_channel_layout_uninit() (at least in
  some situations, which is weird)
- copy via assigning is forbidden (probably?), av_channel_layout_copy()
  must be used instead

> + * No new fields may be added to it without a major version bump.

I think it's still intended that you can add new fields to the union?
So you will add new fields. You just won't change the size, or add
new fields which are mandatory for already defined layout types.

> + *
> + * An AVChannelLayout can be constructed using the convenience function
> + * av_channel_layout_from_mask() / av_channel_layout_from_string(), or it 
> can be
> + * built manually by the caller.
> + */
> +typedef struct AVChannelLayout {
> +/**
> + * Channel order used in this layout.
> + */

("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 presend of the AVChannel with the
> + * corresponding value.
> + *
> + * I.e. when map[i] is equal to AV_CHAN_FOO, then AV_CH_FOO is the 
> i-th
> + * channel in the audio data.
> + */
> +enum AVChannel *map;

Even if the channel map identifier is AV_CHAN_SILENCE? What does the
data contain then, actual silence or undefined contents?

> +} u;
> +} AVChannelLayout;
> +
> +#define AV_CHANNEL_LAYOUT_MONO \
> +{ .order = AV_CHANNEL_ORDER_NATIVE, .nb_channels = 1,  .u = { .mask = 
> AV_CH_LAYOUT_MONO }}
> +#define AV_CHANNEL_LAYOUT_STEREO \
> +{ .order = AV_CHANNEL_ORDER_NATIVE, .nb_channels = 2,  .u = { .mask = 
> AV_CH_LAYOUT_STEREO }}
> +#define AV_CHANNEL_LAYOUT_2POINT1 \
> +{ .order = AV_CHANNEL_ORDER_NATIVE, .nb_channels = 3,  .u = { .mask = 
> AV_CH_LAYOUT_2POINT1 }}
> +#define AV_CHANNEL_LAYOUT_2_1 \
> +{ .order = AV_CHANNEL_ORDER_NATIVE, .nb_channels = 3,  .u = { .mask = 
> AV_CH_LAYOUT_2_1 }}
> +#define AV_CHANNEL_LAYOUT_SURROUND \
> +{ .order = AV_CHANNEL_ORDER_NATIVE, .nb_channels = 3,  .u = { .mask = 
> AV_CH_LAYOUT_SURROUND }}
> +#define AV_CHANNEL_LAYOUT_3POINT1 \
> +{ .order = AV_CHANNEL_ORDER_NATIVE, .nb_channels = 4,  .u = { .mask = 
> AV_CH_LAYOUT_3POINT1 }}
> +#define AV_CHANNEL_LAYOUT_4POINT0 \
> +{ .order = AV_CHANNEL_ORDER_NATIVE, .nb_channels = 4,  .u = { .mask = 
> AV_CH_LAYOUT_4POINT0 }}
> +#define AV_CHANNEL_LAYOUT_4POINT1 \
> +{ .order =