[FFmpeg-devel] [PATCH 001/244] Add a new channel layout API

[Michael Niedermayer]

On Sat, Dec 07, 2019 at 09:25:53PM +0100, Nicolas George wrote:
> Anton Khirnov (12019-12-06):
> > 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.giovara at gmail.com>.
> > Signed-off-by: Vittorio Giovara <vittorio.giovara at gmail.com>
> > ---
> >  libavutil/channel_layout.c | 393 +++++++++++++++++++++++++++++------
> >  libavutil/channel_layout.h | 415 ++++++++++++++++++++++++++++++++++---
> >  libavutil/version.h        |   3 +
> >  3 files changed, 719 insertions(+), 92 deletions(-)
> 
> Thanks for sending this here. I will make a few preliminary remarks
> below.
[...]
> > + * 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.
> > + * In particular, this structure can be initialized as follows:
> > + * - default initialization with {0} or by setting all used fields correctly
> > + * - with predefined layout as initializer (AV_CHANNEL_LAYOUT_STEREO, etc.)
> > + * - with a constructor function such as av_channel_layout_default()
> > + * On that note, this also applies:
> > + * - copy via assigning is forbidden, av_channel_layout_copy() must be used
> > + *   instead (and its return value should be checked)
> > + * - if order is AV_CHANNEL_ORDER_CUSTOM, then it must be uninitialized with
> > + *   av_channel_layout_uninit().
> > + *
> > + * No new fields may be added to it without a major version bump, except for
> > + * new elements of the union fitting in sizeof(uint64_t).
> 
> [ Already said on 2019-10-30 ]
> This is a significant difference from the other APIs, and we need to be
> really sure we can live with it.
> 
> I am rather for this version, but I am afraid we will find cases that
> need extending the structure.
> 
> I suggest we explicitly declare this API unstable for a few months,
> during which we consider acceptable to add fields.
> 

> > + *
> > + * 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.
> > +     * This is a mandatory field, will default to AV_CHANNEL_ORDER_NATIVE.
> > +     */
> > +    enum AVChannelOrder order;
> > +
> > +    /**
> > +     * Number of channels in this layout. Mandatory field.
> > +     */
> 
> > +    int nb_channels;
> 
> [ Already said on 2019-10-30 ]
> unsigned

One problem with unsigned is that in expressions you cannot have negative
values, nor can you compare to negative values without casting to signed.
That has some risk for producing unexpected behavior bugs

for example 
if (ch >= s->nb_channels) {
    ...
} else if (ch < 0)
    ...

would not work as expected

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

If you think the mosad wants you dead since a long time then you are either
wrong or dead since a long time.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20191209/0b05b45a/attachment.sig>