On Fri, May 19, 2017 at 6:40 AM, wm4 <nfx...@googlemail.com> wrote: >> >> + /** >> >> + * 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.
Not necessarily, some demuxers for example only set channels instead of layout, leaving avconv to "guess" the corresponding layout. One of the benefits of this API is that it allows lavc not to lie any more about it, and only provide a compat layer in few well-known places (eg lavr). >> >> + /** >> >> + * 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. Well it depends on how much semantic overload we want this fields to carry. UNSPEC means that you only know the the layout has N channels. CUSTOM means that you know that the layout has N channels *and* they are of type X, Y, Z as defined by the map. While my example could maybe have been expressed in some way with UNSPEC too, it seems safe to assume that there are combinations that could not. >> > 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 :) Do you think changing it to (enum AVChannel *) is enough? > 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. IMO it does seem a little bit complicated, but maybe you might be right too. I leave the decision to the audience. >> > 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. Well this particular case will be well documented. > (Also, it returns an AVERROR as AVChannel enum, which won't work > because a compiler could e.g. make the underlying type uint8_t.) Yes I'll fix it thanks. -- Vittorio _______________________________________________ libav-devel mailing list libav-devel@libav.org https://lists.libav.org/mailman/listinfo/libav-devel
