Re: [libav-devel] [PATCH 001/200] Add a new channel layout API

[Diego Biurrun]

On Mon, May 22, 2017 at 04:08:53PM -0400, Vittorio Giovara wrote:
> On Sat, May 20, 2017 at 7:18 AM, wm4  wrote:
> > On Fri, 19 May 2017 11:25:59 -0400
> > Vittorio Giovara  wrote:
> >
> >> >> > 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?
> >
> > Using enum in ABIs is somewhat risky, because some ABIs might make the
> > underlying type as small as possible. (Clarification on whether C
> > allows this and/or whether any platforms really do it would be welcome.)
> 
> I'm ok with int* as well.

C99 6.7.2.2 Enumeration specifiers:

  Each enumerated type shall be compatible with char, a signed integer type,
  or an unsigned integer type. The choice of type is implementation-defined,
  but shall be capable of representing the values of all the members of the
  enumeration.

I'd still go with the more type-safe enum type.

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

Re: [libav-devel] [PATCH 001/200] Add a new channel layout API

[Luca Barbato]

On 5/17/17 7:46 PM, Vittorio Giovara wrote:
> +enum AVChannel av_channel_from_string(const char *str)

Either return an int, return an int and store the enum in a field or
return an INVALID_CHANNEL enum value.

Same for av_channel_layout_get_channel

The rest looks fine.

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

Re: [libav-devel] [PATCH 001/200] Add a new channel layout API

[Vittorio Giovara]

On Sat, May 20, 2017 at 7:18 AM, wm4  wrote:
> On Fri, 19 May 2017 11:25:59 -0400
> Vittorio Giovara  wrote:
>
>> >> > 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?
>
> Using enum in ABIs is somewhat risky, because some ABIs might make the
> underlying type as small as possible. (Clarification on whether C
> allows this and/or whether any platforms really do it would be welcome.)

I'm ok with int* as well.
-- 
Vittorio
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH 001/200] Add a new channel layout API

[wm4]

On Fri, 19 May 2017 11:25:59 -0400
Vittorio Giovara  wrote:

> >> > 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?

Using enum in ABIs is somewhat risky, because some ABIs might make the
underlying type as small as possible. (Clarification on whether C
allows this and/or whether any platforms really do it would be welcome.)
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH 001/200] Add a new channel layout API

[Vittorio Giovara]

On Fri, May 19, 2017 at 6:40 AM, wm4  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 

Re: [libav-devel] [PATCH 001/200] Add a new channel layout API

[wm4]

On Thu, 18 May 2017 15:01:54 -0400
Vittorio Giovara  wrote:

> On Thu, May 18, 2017 at 11:16 AM, wm4  wrote:
> > On Wed, 17 May 2017 13:46:51 -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 
> >> ---  
> >
> >  
> >> +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 

Re: [libav-devel] [PATCH 001/200] Add a new channel layout API

[Vittorio Giovara]

On Thu, May 18, 2017 at 11:16 AM, wm4  wrote:
> On Wed, 17 May 2017 13:46:51 -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 
>> ---
>
>
>> +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 

Re: [libav-devel] [PATCH 001/200] Add a new channel layout API

[wm4]

On Wed, 17 May 2017 13:46:51 -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 
> ---


> +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).

> +};
> +

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

Also, why is this only 1 byte per channel? Seems a little short-sighted.

Maybe it would be better not to include ORDER_CUSTOM for now? And
ambisonics could be a different AV_CHANNEL_ORDER.

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?
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel