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

[Hendrik Leppkes]

On Fri, Dec 10, 2021 at 12:06 AM Marton Balint <cus at passwd.hu> wrote:
>
>
>
> On Thu, 9 Dec 2021, Anton Khirnov wrote:
>
> > Quoting Lynne (2021-12-08 13:57:45)
> >> 8 Dec 2021, 13:16 by anton at khirnov.net:
> >>
> >>> Quoting Lynne (2021-12-08 10:02:34)
> >>>
> >>>> 8 Dec 2021, 02:06 by jamrial at gmail.com:
> >>>>
> >>>>> From: Anton Khirnov <anton at 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.giovara at gmail.com>
> >>>>> and James Almer <jamrial at gmail.com>.
> >>>>> Signed-off-by: Vittorio Giovara <vittorio.giovara at gmail.com>
> >>>>> Signed-off-by: James Almer <jamrial at gmail.com>
> >>>>> ---
> >>>>>  libavutil/channel_layout.c | 498 ++++++++++++++++++++++++++++++------
> >>>>>  libavutil/channel_layout.h | 510 ++++++++++++++++++++++++++++++++++---
> >>>>>  libavutil/version.h        |   1 +
> >>>>>  3 files changed, 906 insertions(+), 103 deletions(-)
> >>>>>
> >>>>>  /**
> >>>>>  * @}
> >>>>> @@ -128,6 +199,157 @@ enum AVMatrixEncoding {
> >>>>>  AV_MATRIX_ENCODING_NB
> >>>>>  };
> >>>>>
> >>>>> +/**
> >>>>> + * @}
> >>>>> + */
> >>>>> +
> >>>>> +/**
> >>>>> + * An AVChannelCustom defines a single channel within a custom order layout
> >>>>> + *
> >>>>> + * Unlike most structures in FFmpeg, sizeof(AVChannelCustom) is a part of the
> >>>>> + * public ABI.
> >>>>> + *
> >>>>> + * No new fields may be added to it without a major version bump.
> >>>>> + */
> >>>>> +typedef struct AVChannelCustom {
> >>>>> +    enum AVChannel id;
> >>>>> +} AVChannelCustom;
> >>>>>
> >>>>
> >>>> Consider adding a 25-byte or so of a description field string here that
> >>>> would also be copied if the frame/layout is copied?
> >>>> That way, users can assign a description label to each channel,
> >>>> for example labeling each channel in a 255 channel custom layout
> >>>> Opus stream used in production at a venue somewhere.
> >>>> Also, consider additionally reserving 16-bytes here, just in case.
> >>>>
> >>>
> >>> That would enlarge the layout size by a significant amount. Keep in mind
> >>> that this is all per frame. And for something that very few people would
> >>> want to use.
> >>>
> >>> These descriptors, if anyone really needs them, can live in side data.
> >>>
> >>
> >> That's fine with me.
> >> Can we at least have an opaque uint64_t? I'd accept a uintptr_t,
> >> or an int too, or even a single-byte uint8_t.
> >
> > like this?
> >
> > diff --git a/libavutil/channel_layout.h b/libavutil/channel_layout.h
> > index 7b77a74b61..7b2ba12532 100644
> > --- a/libavutil/channel_layout.h
> > +++ b/libavutil/channel_layout.h
> > @@ -252,7 +252,8 @@ enum AVMatrixEncoding {
> >  * No new fields may be added to it without a major version bump.
> >  */
> > typedef struct AVChannelCustom {
> > -    enum AVChannel id;
> > +    enum AVChannel id:16;
> > +    int      reserved:16;
> > } AVChannelCustom;
>
> A dynamically allocated field is so bad here? E.g.
>
> AVDictionary *metadata
>
> You can store labels. You can store group names. You can store language.
> You can store sound positions, GPS coordiantes, whatever.
>
> Yes, it will be slow. But it is rarely used for in normal cases, and it
> can be NULL usually.
>

Pointers add a lot of trouble with ownership and memory management.
Wouldn't be so bad if its just the pointer without all that management
baggage.

- Hendrik