On Thu, Jun 29, 2017 at 5:14 AM, wm4 <nfx...@googlemail.com> wrote:
> On Wed, 28 Jun 2017 18:10:45 -0400
> Vittorio Giovara <vittorio.giov...@gmail.com> 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 function can be skipped?
Yes, I'll add a @note
>> +
>> +/**
>> + * Make a copy of a channel layout. This differs from just assigning src to
>> dst
>> + * in that it allocates and copies the map for AV_CHANNEL_ORDER_CUSTOM.
>> + *
>> + * @param dst destination channel layout
>> + * @param src source channel layout
>> + * @return 0 on success, a negative AVERROR on error.
>> + */
>> +int av_channel_layout_copy(AVChannelLayout *dst, const AVChannelLayout
>> *src);
>
> Same question.
I don't think it applies here? I'll mention this function uninits dst.
>> +/**
>> + * @return a string describing channel_layout or NULL on failure in the same
>> + * format that is accepted by av_channel_layout_from_string(). The
>> + * returned string allocated with av_malloc() and must be freed by
>> the
>> + * caller with av_free().
>> + */
>> +char *av_channel_layout_describe(const AVChannelLayout *channel_layout);
>> +
>> +/**
>> + * Get the channel with the given index in a channel layout.
>> + *
>> + * @return channel with the index idx in channel_layout on success or a
>> negative
>> + * AVERROR on failure (if idx is not valid or the channel
>> order
>> + * is unspecified)
>> + */
>> +int av_channel_layout_get_channel(const AVChannelLayout *channel_layout,
>> int idx);
>> +
>> +/**
>> + * Get the index of a given channel in a channel layout.
>> + *
>> + * @return index of channel in channel_layout on success or a negative
>> number if
>> + * channel is not present in channel_layout.
>> + */
>> +int av_channel_layout_channel_index(const AVChannelLayout *channel_layout,
>> + enum AVChannel channel);
>
> Returns the first index if there are multiple channels like this, I
> suppose.
correct, I'll mentione it
>> +
>> +/**
>> + * Find out what channels from a given set are present in a channel layout,
>> + * without regard for their positions.
>> + *
>> + * @param mask a combination of AV_CH_* representing a set of channels
>> + * @return a bitfield representing all the channels from mask that are
>> present
>> + * in channel_layout
>> + */
>> +uint64_t av_channel_layout_subset(const AVChannelLayout *channel_layout,
>> + uint64_t mask);
>
> No idea what's this for or what happens to AVChannel values that are
> grater than 63.
It used in some filters to extract channel layouts from more complex structures.
If it goes over 63, it will return 0, which matches the documentation
Thanks for the review(s) on this set.
--
Vittorio
_______________________________________________
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel
