On Thu, 18 May 2017 15:01:54 -0400
Vittorio Giovara <vittorio.giov...@gmail.com> wrote:
> 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.
Won't they just use av_channel_layout_from_mask()? They have to set the
channel count too, which the function does.
> >> +};
> >> +
> >
> >> + /**
> >> + * 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.
Well, that's the thing - the map us _supposed_ to inform the user about
the semantics of each channel, otherwise it's worthless. So in the
example you mentioned, it'd be better to use an UNSPEC layout in the
first place.
When I did something similar in mpv, I just strictly defined that no
channel can appear more than once (except zero-padding channels).
> > 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.
Well that seems short-sighted :)
I'm starting to wonder whether it should probably be a permutation map
instead. The meaning of each channel would be strictly defined by the
AV_CHANNEL_ORDER enum value and channel count. _Except_ you could
permutate the order of the channels with an extra map.
Maybe I'm going off a tangent here.
But I'm still trying to gauge how useful the map array is going to be.
> > 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.
Dunno...
At this point I have to say that av_channel_layout_get_channel() looks
dangerous to me, because it might lead the user to assume the list of
AVChannels defines the channel layout completely. Which is probably not
true.
(Also, it returns an AVERROR as AVChannel enum, which won't work
because a compiler could e.g. make the underlying type uint8_t.)
> > 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.
I think setter/getters are much safer for a stable ABI in the long run.
You can bet all libav* users hate the constant API changes.
If we had setters/getters for the channel layout (designed in the right
way), it might have been possible to extend the channel layout cleanly
without ABI break.
_______________________________________________
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel
