On Thu, May 18, 2017 at 11:16 AM, wm4 <nfx...@googlemail.com> wrote:
> On Wed, 17 May 2017 13:46:51 -0400
> Vittorio Giovara <vittorio.giov...@gmail.com> wrote:
>
>> From: Anton Khirnov <an...@khirnov.net>
>>
>> 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 <vittorio.giov...@gmail.com>.
>> Signed-off-by: Vittorio Giovara <vittorio.giov...@gmail.com>
>> ---
>
>
>> +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.
>> + */
>> + AV_CHANNEL_ORDER_UNSPEC,
>
> Wouldn't it be better to make the value 0 something special like maybe
> AV_CHANNEL_ORDER_INVALID? Otherwise it could lead to awkward error
> messages etc. if something doesn't set the channel layout at all (it
> will look like an invalid channel mask).
well, if we use AV_CHANNEL_ORDER_INVALID as value 0 we make it a
little more cumbersome to API users as they have to manually set this
order every time. Right now this is done only for UNSPEC which is
relatively rare, but I would preserve the 0 = ok philosophy for the
most common case (native). Also in the upcoming series every
channel_layout will be properly checked with av_channel_layout_check()
which will be the only source for correctness of the layout.
>> +};
>> +
>
>> + /**
>> + * 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.
>> + */
>> + uint8_t *map;
>
> I think I'd still like some better documentation on this one. For
> example, what does it mean if a channel label appears twice in the same
> map. Would it contain the same data? An alternative version of the data
> (and different how)? What value does it even take?
Yes that could be improved but I'm not sure how (I'm open to suggestions).
If a new channel in a different position would simply mean it's a
completely new channel.
Eg. 16 mono channels to recreate a surround field could be expressed
as a map of 16 elements all initialized to AV_CHAN_FRONT_CENTER. Since
the layout is custom, it's up to the container or codec decide what to
do with them, such as read additional signalling describing how they
are positioned.
> Also, why is this only 1 byte per channel? Seems a little short-sighted.
Probably because even in the long run there won't be more than 255
different channels so 1 byte should be enough.
> Maybe it would be better not to include ORDER_CUSTOM for now? And
> ambisonics could be a different AV_CHANNEL_ORDER.
I'd rather keep it since I find it helpful to have the necessary
checks in the various av_channel_layout_* functions already in place.
Plus API users can start fiddling with it in way we haven't thought of
yet. On your second point, yes, ambisonic will used a different order
entirely, and use a mask only for non diegetic channels if present.
> What's the rationale for making sizeof(AVChannelLayout) part of the
> ABI? It seems generally would need to carefully use special operations
> for copying and uninitializing the struct anyway. Why not make it an
> opaque object?
I find that this made it simpler to use and more similar to what is
currently done now, eg setting the channels and layout directly.
Also I'm not a fan of getters/setters or opaque objects. I think that
sizeof(AVChannelLayout) part of the ABI works of for this particular
struct as we can just expand the union for future orders.
--
Vittorio
_______________________________________________
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel
