| Message ID | 20211208010649.381-2-jamrial@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | New channel layout API | expand |
| Context | Check | Description |
|---|---|---|
| andriy/commit_msg_x86 | warning | The first line of the commit message must start with a context terminated by a colon and a space, for example "lavu/opt: " or "doc: ". |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
| andriy/makex86 | warning | New warnings during build |
| andriy/commit_msg_ppc | warning | The first line of the commit message must start with a context terminated by a colon and a space, for example "lavu/opt: " or "doc: ". |
| andriy/make_ppc | success | Make finished |
| andriy/make_fate_ppc | success | Make fate finished |
| andriy/makeppc | warning | New warnings during build |
8 Dec 2021, 02:06 by jamrial@gmail.com: > From: Anton Khirnov <anton@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@gmail.com> > and James Almer <jamrial@gmail.com>. > Signed-off-by: Vittorio Giovara <vittorio.giovara@gmail.com> > Signed-off-by: James Almer <jamrial@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. Rest looks fine.
8 Dec 2021, 02:06 by jamrial@gmail.com: > > +enum AVChannel { > + ///< Invalid channel index > + AV_CHAN_NONE = -1, > + AV_CHAN_FRONT_LEFT, > No, not the pixfmt mistake again. Set AV_CHAN_NONE to 0, the rest can follow. Or keep AV_CHAN_NONE to -1 and add a new AV_CHAN_UNSPECIFIED as 0. > + AV_CHAN_FRONT_RIGHT, > + AV_CHAN_FRONT_CENTER, > + AV_CHAN_LOW_FREQUENCY, > + AV_CHAN_BACK_LEFT, > + AV_CHAN_BACK_RIGHT, > + AV_CHAN_FRONT_LEFT_OF_CENTER, > + AV_CHAN_FRONT_RIGHT_OF_CENTER, > + AV_CHAN_BACK_CENTER, > + AV_CHAN_SIDE_LEFT, > + AV_CHAN_SIDE_RIGHT, > + AV_CHAN_TOP_CENTER, > + AV_CHAN_TOP_FRONT_LEFT, > + AV_CHAN_TOP_FRONT_CENTER, > + AV_CHAN_TOP_FRONT_RIGHT, > + AV_CHAN_TOP_BACK_LEFT, > + AV_CHAN_TOP_BACK_CENTER, > + AV_CHAN_TOP_BACK_RIGHT, > + /** Stereo downmix. */ > + AV_CHAN_STEREO_LEFT = 29, > + /** See above. */ > + AV_CHAN_STEREO_RIGHT, > + AV_CHAN_WIDE_LEFT, > + AV_CHAN_WIDE_RIGHT, > + AV_CHAN_SURROUND_DIRECT_LEFT, > + AV_CHAN_SURROUND_DIRECT_RIGHT, > + AV_CHAN_LOW_FREQUENCY_2, > + AV_CHAN_TOP_SIDE_LEFT, > + AV_CHAN_TOP_SIDE_RIGHT, > + AV_CHAN_BOTTOM_FRONT_CENTER, > + AV_CHAN_BOTTOM_FRONT_LEFT, > + AV_CHAN_BOTTOM_FRONT_RIGHT, > + > + /** Channel is empty can be safely skipped. */ > + AV_CHAN_SILENCE = 64, > +}; > Why is AV_CHAN_SILENCE set to 64? If it's special, set it to follow just after AV_CHAN_NONE or AV_CHAN_UNSPECIFIED. Finally, consider adding an AV_CHAN_NB at the end, not part of the API, for jut in case.
On Wed, Dec 8, 2021 at 10:14 AM Lynne <dev@lynne.ee> wrote: > > 8 Dec 2021, 02:06 by jamrial@gmail.com: > > > > > +enum AVChannel { > > + ///< Invalid channel index > > + AV_CHAN_NONE = -1, > > + AV_CHAN_FRONT_LEFT, > > > > No, not the pixfmt mistake again. Set AV_CHAN_NONE to 0, > the rest can follow. Or keep AV_CHAN_NONE to -1 > and add a new AV_CHAN_UNSPECIFIED as 0. > Care to elaborate on the reasons of this opinion? Using -1 as invalid and 0...x as valid entries seems quite reasonable to me. - Hendrik
8 Dec 2021, 10:22 by h.leppkes@gmail.com: > On Wed, Dec 8, 2021 at 10:14 AM Lynne <dev@lynne.ee> wrote: > >> >> 8 Dec 2021, 02:06 by jamrial@gmail.com: >> >> > >> > +enum AVChannel { >> > + ///< Invalid channel index >> > + AV_CHAN_NONE = -1, >> > + AV_CHAN_FRONT_LEFT, >> > >> >> No, not the pixfmt mistake again. Set AV_CHAN_NONE to 0, >> the rest can follow. Or keep AV_CHAN_NONE to -1 >> and add a new AV_CHAN_UNSPECIFIED as 0. >> > > Care to elaborate on the reasons of this opinion? Using -1 as invalid > and 0...x as valid entries seems quite reasonable to me. > Zero-initialization. I've had issues in the past telling YUV420P from <uninitialized>.
On Wed, 8 Dec 2021, Lynne wrote: > 8 Dec 2021, 10:22 by h.leppkes@gmail.com: > >> On Wed, Dec 8, 2021 at 10:14 AM Lynne <dev@lynne.ee> wrote: >> >>> >>> 8 Dec 2021, 02:06 by jamrial@gmail.com: >>> >>>> >>>> +enum AVChannel { >>>> + ///< Invalid channel index >>>> + AV_CHAN_NONE = -1, >>>> + AV_CHAN_FRONT_LEFT, >>>> >>> >>> No, not the pixfmt mistake again. Set AV_CHAN_NONE to 0, >>> the rest can follow. Or keep AV_CHAN_NONE to -1 >>> and add a new AV_CHAN_UNSPECIFIED as 0. >>> >> >> Care to elaborate on the reasons of this opinion? Using -1 as invalid >> and 0...x as valid entries seems quite reasonable to me. >> > > Zero-initialization. I've had issues in the past telling > YUV420P from <uninitialized>. It is convenient to be able to set the channel layout mask by a simple byte shift of the ID-s. AV_CH_FRONT_LEFT = 1 << AV_CHAN_FRONT_LEFT. So I'd say that is the reason that it is designed that way, because this way existing channel layout masks can be kept without breaking ABI. Regards, Marton
On Wed, 8 Dec 2021, Lynne wrote: > 8 Dec 2021, 02:06 by jamrial@gmail.com: > >> >> +enum AVChannel { >> + ///< Invalid channel index >> + AV_CHAN_NONE = -1, >> + AV_CHAN_FRONT_LEFT, >> > > No, not the pixfmt mistake again. Set AV_CHAN_NONE to 0, > the rest can follow. Or keep AV_CHAN_NONE to -1 > and add a new AV_CHAN_UNSPECIFIED as 0. > > >> + AV_CHAN_FRONT_RIGHT, >> + AV_CHAN_FRONT_CENTER, >> + AV_CHAN_LOW_FREQUENCY, >> + AV_CHAN_BACK_LEFT, >> + AV_CHAN_BACK_RIGHT, >> + AV_CHAN_FRONT_LEFT_OF_CENTER, >> + AV_CHAN_FRONT_RIGHT_OF_CENTER, >> + AV_CHAN_BACK_CENTER, >> + AV_CHAN_SIDE_LEFT, >> + AV_CHAN_SIDE_RIGHT, >> + AV_CHAN_TOP_CENTER, >> + AV_CHAN_TOP_FRONT_LEFT, >> + AV_CHAN_TOP_FRONT_CENTER, >> + AV_CHAN_TOP_FRONT_RIGHT, >> + AV_CHAN_TOP_BACK_LEFT, >> + AV_CHAN_TOP_BACK_CENTER, >> + AV_CHAN_TOP_BACK_RIGHT, >> + /** Stereo downmix. */ >> + AV_CHAN_STEREO_LEFT = 29, >> + /** See above. */ >> + AV_CHAN_STEREO_RIGHT, >> + AV_CHAN_WIDE_LEFT, >> + AV_CHAN_WIDE_RIGHT, >> + AV_CHAN_SURROUND_DIRECT_LEFT, >> + AV_CHAN_SURROUND_DIRECT_RIGHT, >> + AV_CHAN_LOW_FREQUENCY_2, >> + AV_CHAN_TOP_SIDE_LEFT, >> + AV_CHAN_TOP_SIDE_RIGHT, >> + AV_CHAN_BOTTOM_FRONT_CENTER, >> + AV_CHAN_BOTTOM_FRONT_LEFT, >> + AV_CHAN_BOTTOM_FRONT_RIGHT, >> + >> + /** Channel is empty can be safely skipped. */ >> + AV_CHAN_SILENCE = 64, >> +}; >> > > Why is AV_CHAN_SILENCE set to 64? If it's special, > set it to follow just after AV_CHAN_NONE or > AV_CHAN_UNSPECIFIED. Because of the channel layout bitmask representation and its relation to the channel ID. We might want to add real channel designations which can be represented using the bitmask layout, and using SILENCE for that does not make a lot of sense. E.g. there can be more than one silent channel. And if somebody wants sparse channel layout then making the extended representation a hard requirement seems also a good idea. Regards, Marton
8 Dec 2021, 11:13 by cus@passwd.hu: > > > On Wed, 8 Dec 2021, Lynne wrote: > >> 8 Dec 2021, 02:06 by jamrial@gmail.com: >> >>> >>> +enum AVChannel { >>> + ///< Invalid channel index >>> + AV_CHAN_NONE = -1, >>> + AV_CHAN_FRONT_LEFT, >>> >> >> No, not the pixfmt mistake again. Set AV_CHAN_NONE to 0, >> the rest can follow. Or keep AV_CHAN_NONE to -1 >> and add a new AV_CHAN_UNSPECIFIED as 0. >> >> >>> + AV_CHAN_FRONT_RIGHT, >>> + AV_CHAN_FRONT_CENTER, >>> + AV_CHAN_LOW_FREQUENCY, >>> + AV_CHAN_BACK_LEFT, >>> + AV_CHAN_BACK_RIGHT, >>> + AV_CHAN_FRONT_LEFT_OF_CENTER, >>> + AV_CHAN_FRONT_RIGHT_OF_CENTER, >>> + AV_CHAN_BACK_CENTER, >>> + AV_CHAN_SIDE_LEFT, >>> + AV_CHAN_SIDE_RIGHT, >>> + AV_CHAN_TOP_CENTER, >>> + AV_CHAN_TOP_FRONT_LEFT, >>> + AV_CHAN_TOP_FRONT_CENTER, >>> + AV_CHAN_TOP_FRONT_RIGHT, >>> + AV_CHAN_TOP_BACK_LEFT, >>> + AV_CHAN_TOP_BACK_CENTER, >>> + AV_CHAN_TOP_BACK_RIGHT, >>> + /** Stereo downmix. */ >>> + AV_CHAN_STEREO_LEFT = 29, >>> + /** See above. */ >>> + AV_CHAN_STEREO_RIGHT, >>> + AV_CHAN_WIDE_LEFT, >>> + AV_CHAN_WIDE_RIGHT, >>> + AV_CHAN_SURROUND_DIRECT_LEFT, >>> + AV_CHAN_SURROUND_DIRECT_RIGHT, >>> + AV_CHAN_LOW_FREQUENCY_2, >>> + AV_CHAN_TOP_SIDE_LEFT, >>> + AV_CHAN_TOP_SIDE_RIGHT, >>> + AV_CHAN_BOTTOM_FRONT_CENTER, >>> + AV_CHAN_BOTTOM_FRONT_LEFT, >>> + AV_CHAN_BOTTOM_FRONT_RIGHT, >>> + >>> + /** Channel is empty can be safely skipped. */ >>> + AV_CHAN_SILENCE = 64, >>> +}; >>> >> >> Why is AV_CHAN_SILENCE set to 64? If it's special, >> set it to follow just after AV_CHAN_NONE or >> AV_CHAN_UNSPECIFIED. >> > > Because of the channel layout bitmask representation and its relation to the channel ID. We might want to add real channel designations which can be represented using the bitmask layout, and using SILENCE for that does not make a lot of sense. E.g. there can be more than one silent channel. And if somebody wants sparse channel layout then making the extended representation a hard requirement seems also a good idea. > > Regards, > Marton > If it's a flag, then I'd like for it to be specified with a hint, either AV_CHAN_FLAG_SILENCE, or a comment, or using AV_CHAN_SILENCE = 1 << 16.
8 Dec 2021, 11:06 by cus@passwd.hu: > > > On Wed, 8 Dec 2021, Lynne wrote: > >> 8 Dec 2021, 10:22 by h.leppkes@gmail.com: >> >>> On Wed, Dec 8, 2021 at 10:14 AM Lynne <dev@lynne.ee> wrote: >>> >>>> >>>> 8 Dec 2021, 02:06 by jamrial@gmail.com: >>>> >>>>> >>>>> +enum AVChannel { >>>>> + ///< Invalid channel index >>>>> + AV_CHAN_NONE = -1, >>>>> + AV_CHAN_FRONT_LEFT, >>>>> >>>> >>>> No, not the pixfmt mistake again. Set AV_CHAN_NONE to 0, >>>> the rest can follow. Or keep AV_CHAN_NONE to -1 >>>> and add a new AV_CHAN_UNSPECIFIED as 0. >>>> >>> >>> Care to elaborate on the reasons of this opinion? Using -1 as invalid >>> and 0...x as valid entries seems quite reasonable to me. >>> >> >> Zero-initialization. I've had issues in the past telling >> YUV420P from <uninitialized>. >> > > It is convenient to be able to set the channel layout mask by a simple byte shift of the ID-s. > > AV_CH_FRONT_LEFT = 1 << AV_CHAN_FRONT_LEFT. > > So I'd say that is the reason that it is designed that way, because this way existing channel layout masks can be kept without breaking ABI. > Those can be set with offsets, e.g. AV_CH_FRONT_LEFT = 1 << (AV_CHAN_FRONT_LEFT - 2). I'd like to not have weird values in enums because the old API, just being deprecated, must have its way and can't deal with an offset.
On Wed, 8 Dec 2021, Lynne wrote: > 8 Dec 2021, 11:06 by cus@passwd.hu: > >> >> >> On Wed, 8 Dec 2021, Lynne wrote: >> >>> 8 Dec 2021, 10:22 by h.leppkes@gmail.com: >>> >>>> On Wed, Dec 8, 2021 at 10:14 AM Lynne <dev@lynne.ee> wrote: >>>> >>>>> >>>>> 8 Dec 2021, 02:06 by jamrial@gmail.com: >>>>> >>>>>> >>>>>> +enum AVChannel { >>>>>> + ///< Invalid channel index >>>>>> + AV_CHAN_NONE = -1, >>>>>> + AV_CHAN_FRONT_LEFT, >>>>>> >>>>> >>>>> No, not the pixfmt mistake again. Set AV_CHAN_NONE to 0, >>>>> the rest can follow. Or keep AV_CHAN_NONE to -1 >>>>> and add a new AV_CHAN_UNSPECIFIED as 0. >>>>> >>>> >>>> Care to elaborate on the reasons of this opinion? Using -1 as invalid >>>> and 0...x as valid entries seems quite reasonable to me. >>>> >>> >>> Zero-initialization. I've had issues in the past telling >>> YUV420P from <uninitialized>. >>> >> >> It is convenient to be able to set the channel layout mask by a simple byte shift of the ID-s. >> >> AV_CH_FRONT_LEFT = 1 << AV_CHAN_FRONT_LEFT. >> >> So I'd say that is the reason that it is designed that way, because this way existing channel layout masks can be kept without breaking ABI. >> > > Those can be set with offsets, e.g. > AV_CH_FRONT_LEFT = 1 << (AV_CHAN_FRONT_LEFT - 2). Well, I find this less fortunate then the need to initialize NONE to -1... > I'd like to not have weird values in enums because the old > API, just being deprecated, must have its way and can't deal > with an offset. AV_CH_FRONT_LEFT and similar is not deprecated, these are still used for the native channel order which is still using a mask. Regards, Marton
Lynne (12021-12-08): > 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. I posted a remark with the same concern. But if we add it, or if we make the labels dynamically allocated, the structure will become large and/or slow: I suspect we will need to make it refcounted. Long ago, I posted a template to implement refcounting in structure without the syntactic and memory overhead that buffers require, it was in part meant for this. Regards,
Quoting Lynne (2021-12-08 10:02:34) > 8 Dec 2021, 02:06 by jamrial@gmail.com: > > > From: Anton Khirnov <anton@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@gmail.com> > > and James Almer <jamrial@gmail.com>. > > Signed-off-by: Vittorio Giovara <vittorio.giovara@gmail.com> > > Signed-off-by: James Almer <jamrial@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.
Quoting Marton Balint (2021-12-08 11:55:37) > > > On Wed, 8 Dec 2021, Lynne wrote: > > > 8 Dec 2021, 11:06 by cus@passwd.hu: > > > >> > >> > >> On Wed, 8 Dec 2021, Lynne wrote: > >> > >>> 8 Dec 2021, 10:22 by h.leppkes@gmail.com: > >>> > >>>> On Wed, Dec 8, 2021 at 10:14 AM Lynne <dev@lynne.ee> wrote: > >>>> > >>>>> > >>>>> 8 Dec 2021, 02:06 by jamrial@gmail.com: > >>>>> > >>>>>> > >>>>>> +enum AVChannel { > >>>>>> + ///< Invalid channel index > >>>>>> + AV_CHAN_NONE = -1, > >>>>>> + AV_CHAN_FRONT_LEFT, > >>>>>> > >>>>> > >>>>> No, not the pixfmt mistake again. Set AV_CHAN_NONE to 0, > >>>>> the rest can follow. Or keep AV_CHAN_NONE to -1 > >>>>> and add a new AV_CHAN_UNSPECIFIED as 0. > >>>>> > >>>> > >>>> Care to elaborate on the reasons of this opinion? Using -1 as invalid > >>>> and 0...x as valid entries seems quite reasonable to me. > >>>> > >>> > >>> Zero-initialization. I've had issues in the past telling > >>> YUV420P from <uninitialized>. > >>> > >> > >> It is convenient to be able to set the channel layout mask by a simple byte shift of the ID-s. > >> > >> AV_CH_FRONT_LEFT = 1 << AV_CHAN_FRONT_LEFT. > >> > >> So I'd say that is the reason that it is designed that way, because this way existing channel layout masks can be kept without breaking ABI. > >> > > > > Those can be set with offsets, e.g. > > AV_CH_FRONT_LEFT = 1 << (AV_CHAN_FRONT_LEFT - 2). > > Well, I find this less fortunate then the need to initialize NONE to -1... > > > I'd like to not have weird values in enums because the old > > API, just being deprecated, must have its way and can't deal > > with an offset. > > AV_CH_FRONT_LEFT and similar is not deprecated, these are still used for > the native channel order which is still using a mask. Yes, the direct mapping between channel indices and WAVEFORMATEXTENSIBLE values is a design goal here, just as it was in the old API. I also belive that the initialization issue of pixel/sample formats is less of a problem here, since you rarely need to store actual channel ids. So IMO retaining channel index compatibility is more important.
8 Dec 2021, 13:19 by anton@khirnov.net: > Quoting Marton Balint (2021-12-08 11:55:37) > >> >> >> On Wed, 8 Dec 2021, Lynne wrote: >> >> > 8 Dec 2021, 11:06 by cus@passwd.hu: >> > >> >> >> >> >> >> On Wed, 8 Dec 2021, Lynne wrote: >> >> >> >>> 8 Dec 2021, 10:22 by h.leppkes@gmail.com: >> >>> >> >>>> On Wed, Dec 8, 2021 at 10:14 AM Lynne <dev@lynne.ee> wrote: >> >>>> >> >>>>> >> >>>>> 8 Dec 2021, 02:06 by jamrial@gmail.com: >> >>>>> >> >>>>>> >> >>>>>> +enum AVChannel { >> >>>>>> + ///< Invalid channel index >> >>>>>> + AV_CHAN_NONE = -1, >> >>>>>> + AV_CHAN_FRONT_LEFT, >> >>>>>> >> >>>>> >> >>>>> No, not the pixfmt mistake again. Set AV_CHAN_NONE to 0, >> >>>>> the rest can follow. Or keep AV_CHAN_NONE to -1 >> >>>>> and add a new AV_CHAN_UNSPECIFIED as 0. >> >>>>> >> >>>> >> >>>> Care to elaborate on the reasons of this opinion? Using -1 as invalid >> >>>> and 0...x as valid entries seems quite reasonable to me. >> >>>> >> >>> >> >>> Zero-initialization. I've had issues in the past telling >> >>> YUV420P from <uninitialized>. >> >>> >> >> >> >> It is convenient to be able to set the channel layout mask by a simple byte shift of the ID-s. >> >> >> >> AV_CH_FRONT_LEFT = 1 << AV_CHAN_FRONT_LEFT. >> >> >> >> So I'd say that is the reason that it is designed that way, because this way existing channel layout masks can be kept without breaking ABI. >> >> >> > >> > Those can be set with offsets, e.g. >> > AV_CH_FRONT_LEFT = 1 << (AV_CHAN_FRONT_LEFT - 2). >> >> Well, I find this less fortunate then the need to initialize NONE to -1... >> >> > I'd like to not have weird values in enums because the old >> > API, just being deprecated, must have its way and can't deal >> > with an offset. >> >> AV_CH_FRONT_LEFT and similar is not deprecated, these are still used for >> the native channel order which is still using a mask. >> > > Yes, the direct mapping between channel indices and WAVEFORMATEXTENSIBLE > values is a design goal here, just as it was in the old API. > > I also belive that the initialization issue of pixel/sample formats is > less of a problem here, since you rarely need to store actual channel > ids. So IMO retaining channel index compatibility is more important. > That's not a goal, it's anti-goal, and a cause for hysterical raisin picking in the future. Having an instantly debuggable structure rather than one that makes you question your sanity is much more important than some compatibility few care about, apart from some users who don't even use it but think it's 'neat' and nothing more.
8 Dec 2021, 13:16 by anton@khirnov.net: > Quoting Lynne (2021-12-08 10:02:34) > >> 8 Dec 2021, 02:06 by jamrial@gmail.com: >> >> > From: Anton Khirnov <anton@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@gmail.com> >> > and James Almer <jamrial@gmail.com>. >> > Signed-off-by: Vittorio Giovara <vittorio.giovara@gmail.com> >> > Signed-off-by: James Almer <jamrial@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.
On Wed, Dec 8, 2021 at 1:54 PM Lynne <dev@lynne.ee> wrote: > > 8 Dec 2021, 13:19 by anton@khirnov.net: > > > Quoting Marton Balint (2021-12-08 11:55:37) > > > >> > >> > >> On Wed, 8 Dec 2021, Lynne wrote: > >> > >> > 8 Dec 2021, 11:06 by cus@passwd.hu: > >> > > >> >> > >> >> > >> >> On Wed, 8 Dec 2021, Lynne wrote: > >> >> > >> >>> 8 Dec 2021, 10:22 by h.leppkes@gmail.com: > >> >>> > >> >>>> On Wed, Dec 8, 2021 at 10:14 AM Lynne <dev@lynne.ee> wrote: > >> >>>> > >> >>>>> > >> >>>>> 8 Dec 2021, 02:06 by jamrial@gmail.com: > >> >>>>> > >> >>>>>> > >> >>>>>> +enum AVChannel { > >> >>>>>> + ///< Invalid channel index > >> >>>>>> + AV_CHAN_NONE = -1, > >> >>>>>> + AV_CHAN_FRONT_LEFT, > >> >>>>>> > >> >>>>> > >> >>>>> No, not the pixfmt mistake again. Set AV_CHAN_NONE to 0, > >> >>>>> the rest can follow. Or keep AV_CHAN_NONE to -1 > >> >>>>> and add a new AV_CHAN_UNSPECIFIED as 0. > >> >>>>> > >> >>>> > >> >>>> Care to elaborate on the reasons of this opinion? Using -1 as invalid > >> >>>> and 0...x as valid entries seems quite reasonable to me. > >> >>>> > >> >>> > >> >>> Zero-initialization. I've had issues in the past telling > >> >>> YUV420P from <uninitialized>. > >> >>> > >> >> > >> >> It is convenient to be able to set the channel layout mask by a simple byte shift of the ID-s. > >> >> > >> >> AV_CH_FRONT_LEFT = 1 << AV_CHAN_FRONT_LEFT. > >> >> > >> >> So I'd say that is the reason that it is designed that way, because this way existing channel layout masks can be kept without breaking ABI. > >> >> > >> > > >> > Those can be set with offsets, e.g. > >> > AV_CH_FRONT_LEFT = 1 << (AV_CHAN_FRONT_LEFT - 2). > >> > >> Well, I find this less fortunate then the need to initialize NONE to -1... > >> > >> > I'd like to not have weird values in enums because the old > >> > API, just being deprecated, must have its way and can't deal > >> > with an offset. > >> > >> AV_CH_FRONT_LEFT and similar is not deprecated, these are still used for > >> the native channel order which is still using a mask. > >> > > > > Yes, the direct mapping between channel indices and WAVEFORMATEXTENSIBLE > > values is a design goal here, just as it was in the old API. > > > > I also belive that the initialization issue of pixel/sample formats is > > less of a problem here, since you rarely need to store actual channel > > ids. So IMO retaining channel index compatibility is more important. > > > > That's not a goal, it's anti-goal, and a cause for hysterical raisin > picking in the future. The old AV_CH_* values are not chosen randomly, they match the WAVEFORMAT channel ordering and bit position, which a lot of formats and system use to identify channel layouts - and will continue to do so in the future. So its hardly just compatibility with FFmpeg from yester-year thats being considered here, but the multimedia infrastructure at large. - Hendrik
8 Dec 2021, 14:10 by h.leppkes@gmail.com: > On Wed, Dec 8, 2021 at 1:54 PM Lynne <dev@lynne.ee> wrote: > >> >> 8 Dec 2021, 13:19 by anton@khirnov.net: >> >> > Quoting Marton Balint (2021-12-08 11:55:37) >> > >> >> >> >> >> >> On Wed, 8 Dec 2021, Lynne wrote: >> >> >> >> > 8 Dec 2021, 11:06 by cus@passwd.hu: >> >> > >> >> >> >> >> >> >> >> >> On Wed, 8 Dec 2021, Lynne wrote: >> >> >> >> >> >>> 8 Dec 2021, 10:22 by h.leppkes@gmail.com: >> >> >>> >> >> >>>> On Wed, Dec 8, 2021 at 10:14 AM Lynne <dev@lynne.ee> wrote: >> >> >>>> >> >> >>>>> >> >> >>>>> 8 Dec 2021, 02:06 by jamrial@gmail.com: >> >> >>>>> >> >> >>>>>> >> >> >>>>>> +enum AVChannel { >> >> >>>>>> + ///< Invalid channel index >> >> >>>>>> + AV_CHAN_NONE = -1, >> >> >>>>>> + AV_CHAN_FRONT_LEFT, >> >> >>>>>> >> >> >>>>> >> >> >>>>> No, not the pixfmt mistake again. Set AV_CHAN_NONE to 0, >> >> >>>>> the rest can follow. Or keep AV_CHAN_NONE to -1 >> >> >>>>> and add a new AV_CHAN_UNSPECIFIED as 0. >> >> >>>>> >> >> >>>> >> >> >>>> Care to elaborate on the reasons of this opinion? Using -1 as invalid >> >> >>>> and 0...x as valid entries seems quite reasonable to me. >> >> >>>> >> >> >>> >> >> >>> Zero-initialization. I've had issues in the past telling >> >> >>> YUV420P from <uninitialized>. >> >> >>> >> >> >> >> >> >> It is convenient to be able to set the channel layout mask by a simple byte shift of the ID-s. >> >> >> >> >> >> AV_CH_FRONT_LEFT = 1 << AV_CHAN_FRONT_LEFT. >> >> >> >> >> >> So I'd say that is the reason that it is designed that way, because this way existing channel layout masks can be kept without breaking ABI. >> >> >> >> >> > >> >> > Those can be set with offsets, e.g. >> >> > AV_CH_FRONT_LEFT = 1 << (AV_CHAN_FRONT_LEFT - 2). >> >> >> >> Well, I find this less fortunate then the need to initialize NONE to -1... >> >> >> >> > I'd like to not have weird values in enums because the old >> >> > API, just being deprecated, must have its way and can't deal >> >> > with an offset. >> >> >> >> AV_CH_FRONT_LEFT and similar is not deprecated, these are still used for >> >> the native channel order which is still using a mask. >> >> >> > >> > Yes, the direct mapping between channel indices and WAVEFORMATEXTENSIBLE >> > values is a design goal here, just as it was in the old API. >> > >> > I also belive that the initialization issue of pixel/sample formats is >> > less of a problem here, since you rarely need to store actual channel >> > ids. So IMO retaining channel index compatibility is more important. >> > >> >> That's not a goal, it's anti-goal, and a cause for hysterical raisin >> picking in the future. >> > > The old AV_CH_* values are not chosen randomly, they match the > WAVEFORMAT channel ordering and bit position, which a lot of formats > and system use to identify channel layouts - and will continue to do > so in the future. > So its hardly just compatibility with FFmpeg from yester-year thats > being considered here, but the multimedia infrastructure at large. > I don't mind AV_CH_ flags using WAVEFORMATEXTENSIBLE, but I do mind enum AVChannel following those one to one with no offset. The two are independent, and having an offset of one or two won't do anything to the flags as long as that offset is accounted for when the flags are set.
Quoting Lynne (2021-12-08 13:53:52) > That's not a goal, it's anti-goal, and a cause for hysterical raisin > picking in the future. > Having an instantly debuggable structure rather than one that Instantly debuggable structure? Again: very very little code needs to store actual AVChannels. In the submitted tree this is just af_join. So the gain from adding an offset would simplify very little code, while complicating all the callers dealing with masks, of which there are quite many.
9 Dec 2021, 11:14 by anton@khirnov.net: > Quoting Lynne (2021-12-08 13:53:52) > >> That's not a goal, it's anti-goal, and a cause for hysterical raisin >> picking in the future. >> Having an instantly debuggable structure rather than one that >> > > Instantly debuggable structure? > > Again: very very little code needs to store actual AVChannels. In the > submitted tree this is just af_join. So the gain from adding an offset > would simplify very little code, while complicating all the callers > dealing with masks, of which there are quite many. > It would make debugging much less hazardous, and any code that deals with masks uses the existing #defines. And even if it dealt directly, it's an offset that can be #defined so users could directly map wavetableext flags with a shift.
On 12/9/2021 8:21 AM, Lynne wrote: > 9 Dec 2021, 11:14 by anton@khirnov.net: > >> Quoting Lynne (2021-12-08 13:53:52) >> >>> That's not a goal, it's anti-goal, and a cause for hysterical raisin >>> picking in the future. >>> Having an instantly debuggable structure rather than one that >>> >> >> Instantly debuggable structure? >> >> Again: very very little code needs to store actual AVChannels. In the >> submitted tree this is just af_join. So the gain from adding an offset >> would simplify very little code, while complicating all the callers >> dealing with masks, of which there are quite many. >> > > It would make debugging much less hazardous, > and any code that deals with masks uses the existing > #defines. And even if it dealt directly, it's an offset that > can be #defined so users could directly map wavetableext > flags with a shift. You're asking to make code uglier and harder to use, and to stray away from decades old standard mask conventions across software, for some supposed easier time debugging. I'm with Anton and Hendrik, and i don't agree with this approach at all. > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Quoting Lynne (2021-12-08 13:57:45) > 8 Dec 2021, 13:16 by anton@khirnov.net: > > > Quoting Lynne (2021-12-08 10:02:34) > > > >> 8 Dec 2021, 02:06 by jamrial@gmail.com: > >> > >> > From: Anton Khirnov <anton@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@gmail.com> > >> > and James Almer <jamrial@gmail.com>. > >> > Signed-off-by: Vittorio Giovara <vittorio.giovara@gmail.com> > >> > Signed-off-by: James Almer <jamrial@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; /**
9 Dec 2021, 14:12 by anton@khirnov.net: > Quoting Lynne (2021-12-08 13:57:45) > >> 8 Dec 2021, 13:16 by anton@khirnov.net: >> >> > Quoting Lynne (2021-12-08 10:02:34) >> > >> >> 8 Dec 2021, 02:06 by jamrial@gmail.com: >> >> >> >> > From: Anton Khirnov <anton@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@gmail.com> >> >> > and James Almer <jamrial@gmail.com>. >> >> > Signed-off-by: Vittorio Giovara <vittorio.giovara@gmail.com> >> >> > Signed-off-by: James Almer <jamrial@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; > "reserved" rather than "opaque"? 16 bits for both? Could you keep the id as a plain (32-bit, on a sane compiler) enum, since we may need to add flags, one never knows, and make the opaque field a plain uint32_t? 64-bits per channel overall isn't too bad.
Quoting Lynne (2021-12-09 12:21:14) > 9 Dec 2021, 11:14 by anton@khirnov.net: > > > Quoting Lynne (2021-12-08 13:53:52) > > > >> That's not a goal, it's anti-goal, and a cause for hysterical raisin > >> picking in the future. > >> Having an instantly debuggable structure rather than one that > >> > > > > Instantly debuggable structure? > > > > Again: very very little code needs to store actual AVChannels. In the > > submitted tree this is just af_join. So the gain from adding an offset > > would simplify very little code, while complicating all the callers > > dealing with masks, of which there are quite many. > > > > It would make debugging much less hazardous, I fail to see how, since almost no code stores AVChannel directly.
Lynne (12021-12-09): > "reserved" rather than "opaque"? > 16 bits for both? Could you keep the id as a plain (32-bit, on > a sane compiler) enum, since we may need to add flags, one > never knows, and make the opaque field a plain uint32_t? > 64-bits per channel overall isn't too bad. You wanted a real string, please do not beg for 16 measly bits! I strongly support your demand of a real string. Regards,
On Thu, 9 Dec 2021, Anton Khirnov wrote: > Quoting Lynne (2021-12-08 13:57:45) >> 8 Dec 2021, 13:16 by anton@khirnov.net: >> >>> Quoting Lynne (2021-12-08 10:02:34) >>> >>>> 8 Dec 2021, 02:06 by jamrial@gmail.com: >>>> >>>>> From: Anton Khirnov <anton@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@gmail.com> >>>>> and James Almer <jamrial@gmail.com>. >>>>> Signed-off-by: Vittorio Giovara <vittorio.giovara@gmail.com> >>>>> Signed-off-by: James Almer <jamrial@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. Regards, Marton
On Fri, Dec 10, 2021 at 12:06 AM Marton Balint <cus@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@khirnov.net: > >> > >>> Quoting Lynne (2021-12-08 10:02:34) > >>> > >>>> 8 Dec 2021, 02:06 by jamrial@gmail.com: > >>>> > >>>>> From: Anton Khirnov <anton@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@gmail.com> > >>>>> and James Almer <jamrial@gmail.com>. > >>>>> Signed-off-by: Vittorio Giovara <vittorio.giovara@gmail.com> > >>>>> Signed-off-by: James Almer <jamrial@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
On Fri, 10 Dec 2021, Hendrik Leppkes wrote: > On Fri, Dec 10, 2021 at 12:06 AM Marton Balint <cus@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@khirnov.net: >>>> >>>>> Quoting Lynne (2021-12-08 10:02:34) >>>>> >>>>>> 8 Dec 2021, 02:06 by jamrial@gmail.com: >>>>>> >>>>>>> From: Anton Khirnov <anton@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@gmail.com> >>>>>>> and James Almer <jamrial@gmail.com>. >>>>>>> Signed-off-by: Vittorio Giovara <vittorio.giovara@gmail.com> >>>>>>> Signed-off-by: James Almer <jamrial@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. Sure, but since you are dynamically allocating the custom map AVChannelLayout->u.map, you are already managing memory... E.g. you could also store an array of AVDictionaries in AVChannelLayout. Regards, Marton
On 12/9/2021 8:31 PM, Marton Balint wrote: > > > On Fri, 10 Dec 2021, Hendrik Leppkes wrote: > >> On Fri, Dec 10, 2021 at 12:06 AM Marton Balint <cus@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@khirnov.net: >>>>> >>>>>> Quoting Lynne (2021-12-08 10:02:34) >>>>>> >>>>>>> 8 Dec 2021, 02:06 by jamrial@gmail.com: >>>>>>> >>>>>>>> From: Anton Khirnov <anton@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@gmail.com> >>>>>>>> and James Almer <jamrial@gmail.com>. >>>>>>>> Signed-off-by: Vittorio Giovara <vittorio.giovara@gmail.com> >>>>>>>> Signed-off-by: James Almer <jamrial@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. > > Sure, but since you are dynamically allocating the custom map > AVChannelLayout->u.map, you are already managing memory... > E.g. you could also store an array of AVDictionaries in AVChannelLayout. That's one per layout, whereas your suggestion is one per channel. > > Regards, > Marton > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
On Tue, 7 Dec 2021, James Almer wrote: > From: Anton Khirnov <anton@khirnov.net> > [...] > -static const char *get_channel_name(int channel_id) > +static const char *get_channel_name(enum AVChannel channel_id) > { > - if (channel_id < 0 || channel_id >= FF_ARRAY_ELEMS(channel_names)) > - return NULL; > + if ((unsigned) channel_id >= FF_ARRAY_ELEMS(channel_names) || > + !channel_names[channel_id].name) > + return "?"; I'd rather return NULL here instead of "?". Is this allowed to happen by the way? > return channel_names[channel_id].name; > } > > -static const struct { > +static inline void get_channel_str(AVBPrint *bp, const char *str, > + enum AVChannel channel_id) > +{ > + if (str) > + av_bprintf(bp, "%s", str); > + else > + av_bprintf(bp, "?"); If this is not allowed, then you should propagate back AVERROR(EINVAL) if it does. If it is allowed, because the user can use some custom channel_id-s, then something like "USER%d" should be returned IMHO. > + errno = 0; > + channels = strtol(str, &end, 10); > + > + /* number of channels */ > + if (!errno && *end == 'c' && !*(end + 1) && channels >= 0) { > + av_channel_layout_default(channel_layout, channels); > + return 0; > + } > + > + /* number of unordered channels */ > + if (!errno && (!*end || av_strnstr(str, "channels", strlen(str))) && channels >= 0) { This is missing the syntax used in av_get_extended_channel_layout(), 1C to 63C for unspecified channel layout with that many number of channels. [...] > +int av_channel_layout_describe(const AVChannelLayout *channel_layout, > + char *buf, size_t buf_size) > +{ Can you make a function variant of this which directly gets a AVBPrint buffer? Similar to av_bprint_channel_layout. [...] > +int av_channel_layout_check(const AVChannelLayout *channel_layout) av_channel_layout_is_valid() seems like a better name. [...] > diff --git a/libavutil/channel_layout.h b/libavutil/channel_layout.h > index d39ae1177a..018e87ff0b 100644 > --- a/libavutil/channel_layout.h > +++ b/libavutil/channel_layout.h > @@ -23,6 +23,10 @@ > #define AVUTIL_CHANNEL_LAYOUT_H > > #include <stdint.h> > +#include <stdlib.h> > + > +#include "version.h" > +#include "attributes.h" > > /** > * @file > @@ -34,6 +38,68 @@ > * @{ > */ > > +enum AVChannel { > + ///< Invalid channel index > + AV_CHAN_NONE = -1, > + AV_CHAN_FRONT_LEFT, > + AV_CHAN_FRONT_RIGHT, > + AV_CHAN_FRONT_CENTER, > + AV_CHAN_LOW_FREQUENCY, > + AV_CHAN_BACK_LEFT, > + AV_CHAN_BACK_RIGHT, > + AV_CHAN_FRONT_LEFT_OF_CENTER, > + AV_CHAN_FRONT_RIGHT_OF_CENTER, > + AV_CHAN_BACK_CENTER, > + AV_CHAN_SIDE_LEFT, > + AV_CHAN_SIDE_RIGHT, > + AV_CHAN_TOP_CENTER, > + AV_CHAN_TOP_FRONT_LEFT, > + AV_CHAN_TOP_FRONT_CENTER, > + AV_CHAN_TOP_FRONT_RIGHT, > + AV_CHAN_TOP_BACK_LEFT, > + AV_CHAN_TOP_BACK_CENTER, > + AV_CHAN_TOP_BACK_RIGHT, > + /** Stereo downmix. */ > + AV_CHAN_STEREO_LEFT = 29, > + /** See above. */ > + AV_CHAN_STEREO_RIGHT, > + AV_CHAN_WIDE_LEFT, > + AV_CHAN_WIDE_RIGHT, > + AV_CHAN_SURROUND_DIRECT_LEFT, > + AV_CHAN_SURROUND_DIRECT_RIGHT, > + AV_CHAN_LOW_FREQUENCY_2, > + AV_CHAN_TOP_SIDE_LEFT, > + AV_CHAN_TOP_SIDE_RIGHT, > + AV_CHAN_BOTTOM_FRONT_CENTER, > + AV_CHAN_BOTTOM_FRONT_LEFT, > + AV_CHAN_BOTTOM_FRONT_RIGHT, > + > + /** Channel is empty can be safely skipped. */ > + AV_CHAN_SILENCE = 64, I'd rather name this AV_CHAN_GARBAGE or AV_CHAN_UNUSED instead. Silence could be useful, and a silent channel could be FRONT LEFT at the same time, but AV_CHAN_SILENCE is not a flag. But based on the comment this is simply a channel which should be ignored, because it does not contain anything playable/presentable. Other already said, but an unknown designation (AV_CHAN_UNKNOWN) should be added. Which is a useful channel, but with unknown or unsupported designation. [...] > + * 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; > + > +/** > + * An AVChannelLayout holds information about the channel layout of audio data. > + * > + * A channel layout here is defined as a set of channels ordered in a specific > + * way (unless the channel order is AV_CHANNEL_ORDER_UNSPEC, in which case an > + * AVChannelLayout carries only the channel count). > + * > + * 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 > + * or embedded in caller-defined structs. > + * > + * AVChannelLayout can be initialized as follows: > + * - default initialization with {0}, followed by by setting all used fields > + * correctly; > + * - by assigning one of the predefined AV_CHANNEL_LAYOUT_* initializers; > + * - with a constructor function, such as av_channel_layout_default(), > + * av_channel_layout_from_mask() or av_channel_layout_from_string(). > + * > + * The channel layout must be unitialized with av_channel_layout_uninit() > + * (strictly speaking this is not necessary for AV_CHANNEL_ORDER_NATIVE and > + * AV_CHANNEL_ORDER_UNSPEC, but we recommend always calling > + * av_channel_layout_uninit() regardless of the channel order used). I'd remove this comment, that it might not be necessary to uninit. New API, please always uninit(). > + * > + * Copying an AVChannelLayout via assigning is forbidden, > + * av_channel_layout_copy() must be used. instead (and its return value should > + * be checked) > + * > + * 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). > + */ > +typedef struct AVChannelLayout { > + /** > + * Channel order used in this layout. > + * This is a mandatory field. > + */ > + enum AVChannelOrder order; > + > + /** > + * Number of channels in this layout. Mandatory field. > + */ > + int nb_channels; > + > + /** > + * 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 presence of the AVChannel with the > + * corresponding value in map[i].id. > + * > + * I.e. when map[i].id is equal to AV_CHAN_FOO, then AV_CH_FOO is the > + * i-th channel in the audio data. > + */ > + AVChannelCustom *map; > + } u; I would like to attach some extendable, possibly per-channel metadata to the channel layout. I'd rather put it into AVChannelLayout, so native layouts could also have metadata. This must be dynamically allocated to make it extendable and to not consume too many bytes. I can accept that it will be slow. But I don't see it unmanagable, because AVChannelLayout already have functions for allocation/free/copy. I also think that most of the time it will not be used (e.g. if metadata is NULL, that can mean no metadata for all the channels). If we can't decide what this should be, array of AVDictionaries, some side-data like approach but in the channel layout, or a new dynamically allocated AVChannelLayoutMetadata struct, then maybe just reserve a void* in the end of the AVChannelLayout, and we can work it out later. Regards, Marton
On 12/13/2021 10:13 PM, Marton Balint wrote: > > > On Tue, 7 Dec 2021, James Almer wrote: > >> From: Anton Khirnov <anton@khirnov.net> >> > > [...] > >> -static const char *get_channel_name(int channel_id) >> +static const char *get_channel_name(enum AVChannel channel_id) >> { >> - if (channel_id < 0 || channel_id >= FF_ARRAY_ELEMS(channel_names)) >> - return NULL; >> + if ((unsigned) channel_id >= FF_ARRAY_ELEMS(channel_names) || >> + !channel_names[channel_id].name) >> + return "?"; > > I'd rather return NULL here instead of "?". Is this allowed to happen by > the way? Yes, even in a native layout, several AVChannel values have no name. The idea is to print a string like "FL|FR|?" when the name of a channel is unknown. See the test in patch 2. > >> return channel_names[channel_id].name; >> } >> >> -static const struct { >> +static inline void get_channel_str(AVBPrint *bp, const char *str, >> + enum AVChannel channel_id) >> +{ >> + if (str) >> + av_bprintf(bp, "%s", str); >> + else >> + av_bprintf(bp, "?"); > > If this is not allowed, then you should propagate back AVERROR(EINVAL) > if it does. If it is allowed, because the user can use some custom > channel_id-s, then something like "USER%d" should be returned IMHO. How about Ch%d? It would also further improve the usefulness of av_channel_layout_from_string() by looking for such strings. > >> + errno = 0; >> + channels = strtol(str, &end, 10); >> + >> + /* number of channels */ >> + if (!errno && *end == 'c' && !*(end + 1) && channels >= 0) { >> + av_channel_layout_default(channel_layout, channels); >> + return 0; >> + } >> + >> + /* number of unordered channels */ >> + if (!errno && (!*end || av_strnstr(str, "channels", strlen(str))) >> && channels >= 0) { > > This is missing the syntax used in av_get_extended_channel_layout(), 1C > to 63C for unspecified channel layout with that many number of channels. Ok. > > [...] > > >> +int av_channel_layout_describe(const AVChannelLayout *channel_layout, >> + char *buf, size_t buf_size) >> +{ > > Can you make a function variant of this which directly gets a AVBPrint > buffer? Similar to av_bprint_channel_layout. Yes, i was going to do that, following the other discussion. > > [...] > >> +int av_channel_layout_check(const AVChannelLayout *channel_layout) > > av_channel_layout_is_valid() seems like a better name. > > [...] > > >> diff --git a/libavutil/channel_layout.h b/libavutil/channel_layout.h >> index d39ae1177a..018e87ff0b 100644 >> --- a/libavutil/channel_layout.h >> +++ b/libavutil/channel_layout.h >> @@ -23,6 +23,10 @@ >> #define AVUTIL_CHANNEL_LAYOUT_H >> >> #include <stdint.h> >> +#include <stdlib.h> >> + >> +#include "version.h" >> +#include "attributes.h" >> >> /** >> * @file >> @@ -34,6 +38,68 @@ >> * @{ >> */ >> >> +enum AVChannel { >> + ///< Invalid channel index >> + AV_CHAN_NONE = -1, >> + AV_CHAN_FRONT_LEFT, >> + AV_CHAN_FRONT_RIGHT, >> + AV_CHAN_FRONT_CENTER, >> + AV_CHAN_LOW_FREQUENCY, >> + AV_CHAN_BACK_LEFT, >> + AV_CHAN_BACK_RIGHT, >> + AV_CHAN_FRONT_LEFT_OF_CENTER, >> + AV_CHAN_FRONT_RIGHT_OF_CENTER, >> + AV_CHAN_BACK_CENTER, >> + AV_CHAN_SIDE_LEFT, >> + AV_CHAN_SIDE_RIGHT, >> + AV_CHAN_TOP_CENTER, >> + AV_CHAN_TOP_FRONT_LEFT, >> + AV_CHAN_TOP_FRONT_CENTER, >> + AV_CHAN_TOP_FRONT_RIGHT, >> + AV_CHAN_TOP_BACK_LEFT, >> + AV_CHAN_TOP_BACK_CENTER, >> + AV_CHAN_TOP_BACK_RIGHT, >> + /** Stereo downmix. */ >> + AV_CHAN_STEREO_LEFT = 29, >> + /** See above. */ >> + AV_CHAN_STEREO_RIGHT, >> + AV_CHAN_WIDE_LEFT, >> + AV_CHAN_WIDE_RIGHT, >> + AV_CHAN_SURROUND_DIRECT_LEFT, >> + AV_CHAN_SURROUND_DIRECT_RIGHT, >> + AV_CHAN_LOW_FREQUENCY_2, >> + AV_CHAN_TOP_SIDE_LEFT, >> + AV_CHAN_TOP_SIDE_RIGHT, >> + AV_CHAN_BOTTOM_FRONT_CENTER, >> + AV_CHAN_BOTTOM_FRONT_LEFT, >> + AV_CHAN_BOTTOM_FRONT_RIGHT, >> + >> + /** Channel is empty can be safely skipped. */ >> + AV_CHAN_SILENCE = 64, > > I'd rather name this AV_CHAN_GARBAGE or AV_CHAN_UNUSED instead. Silence > could be useful, and a silent channel could be FRONT LEFT at the same > time, but AV_CHAN_SILENCE is not a flag. But based on the comment this > is simply a channel which should be ignored, because it does not contain > anything playable/presentable. > > Other already said, but an unknown designation (AV_CHAN_UNKNOWN) should > be added. Which is a useful channel, but with unknown or unsupported > designation. > > [...] > >> + * 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; >> + >> +/** >> + * An AVChannelLayout holds information about the channel layout of >> audio data. >> + * >> + * A channel layout here is defined as a set of channels ordered in a >> specific >> + * way (unless the channel order is AV_CHANNEL_ORDER_UNSPEC, in which >> case an >> + * AVChannelLayout carries only the channel count). >> + * >> + * 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 >> + * or embedded in caller-defined structs. >> + * >> + * AVChannelLayout can be initialized as follows: >> + * - default initialization with {0}, followed by by setting all used >> fields >> + * correctly; >> + * - by assigning one of the predefined AV_CHANNEL_LAYOUT_* >> initializers; >> + * - with a constructor function, such as av_channel_layout_default(), >> + * av_channel_layout_from_mask() or av_channel_layout_from_string(). >> + * >> + * The channel layout must be unitialized with >> av_channel_layout_uninit() >> + * (strictly speaking this is not necessary for >> AV_CHANNEL_ORDER_NATIVE and >> + * AV_CHANNEL_ORDER_UNSPEC, but we recommend always calling >> + * av_channel_layout_uninit() regardless of the channel order used). > > I'd remove this comment, that it might not be necessary to uninit. New > API, please always uninit(). Ok. > >> + * >> + * Copying an AVChannelLayout via assigning is forbidden, >> + * av_channel_layout_copy() must be used. instead (and its return >> value should >> + * be checked) >> + * >> + * 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). >> + */ >> +typedef struct AVChannelLayout { >> + /** >> + * Channel order used in this layout. >> + * This is a mandatory field. >> + */ >> + enum AVChannelOrder order; >> + >> + /** >> + * Number of channels in this layout. Mandatory field. >> + */ >> + int nb_channels; >> + >> + /** >> + * 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 presence of the AVChannel with the >> + * corresponding value in map[i].id. >> + * >> + * I.e. when map[i].id is equal to AV_CHAN_FOO, then >> AV_CH_FOO is the >> + * i-th channel in the audio data. >> + */ >> + AVChannelCustom *map; >> + } u; > > I would like to attach some extendable, possibly per-channel metadata to > the channel layout. I'd rather put it into AVChannelLayout, so native > layouts could also have metadata. This must be dynamically allocated to > make it extendable and to not consume too many bytes. I can accept that > it will be slow. But I don't see it unmanagable, because AVChannelLayout > already have functions for allocation/free/copy. I also think that most > of the time it will not be used (e.g. if metadata is NULL, that can mean > no metadata for all the channels). > > If we can't decide what this should be, array of AVDictionaries, some > side-data like approach but in the channel layout, or a new dynamically > allocated AVChannelLayoutMetadata struct, then maybe just reserve a > void* in the end of the AVChannelLayout, and we can work it out later. How about a "const uint8_t *name" in AVChannelCustom? You can then point to some string that will not be owned by the layout, nor duplicated on copy, and that will be used by the helpers (av_channel_layout_channel_from_string, av_channel_layout_index_from_string, av_channel_layout_describe) to identify the channel. This avoids doing dynamic allocations per channel. > > Regards, > Marton > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
14 Dec 2021, 02:13 by cus@passwd.hu: > > > On Tue, 7 Dec 2021, James Almer wrote: > > I would like to attach some extendable, possibly per-channel metadata to the channel layout. I'd rather put it into AVChannelLayout, so native layouts could also have metadata. This must be dynamically allocated to make it extendable and to not consume too many bytes. I can accept that it will be slow. But I don't see it unmanagable, because AVChannelLayout already have functions for allocation/free/copy. I also think that most of the time it will not be used (e.g. if metadata is NULL, that can mean no metadata for all the channels). > > If we can't decide what this should be, array of AVDictionaries, some side-data like approach but in the channel layout, or a new dynamically allocated AVChannelLayoutMetadata struct, then maybe just reserve a void* in the end of the AVChannelLayout, and we can work it out later. > The idea to use opaque indices is IMO better. You can store any metadata you want into the frame->opaque(_buf) field, and it'll be completely copied of av_frame_ref(). If we go for an opaque pointer, I'd like for it to be copied (by value, not data) into any ref'd frames. I'd prefer an opaque field over an opaque metadata field.
On Mon, 13 Dec 2021, James Almer wrote: > > > On 12/13/2021 10:13 PM, Marton Balint wrote: >> >> >> On Tue, 7 Dec 2021, James Almer wrote: >> >>> From: Anton Khirnov <anton@khirnov.net> >>> >> >> [...] >> >>> -static const char *get_channel_name(int channel_id) >>> +static const char *get_channel_name(enum AVChannel channel_id) >>> { >>> - if (channel_id < 0 || channel_id >= FF_ARRAY_ELEMS(channel_names)) >>> - return NULL; >>> + if ((unsigned) channel_id >= FF_ARRAY_ELEMS(channel_names) || >>> + !channel_names[channel_id].name) >>> + return "?"; >> >> I'd rather return NULL here instead of "?". Is this allowed to happen by >> the way? > > Yes, even in a native layout, several AVChannel values have no name. The idea > is to print a string like "FL|FR|?" when the name of a channel is unknown. > See the test in patch 2. As far as I see this function is used by av_channel_layout_describe. If get_channel_name returned NULL, then av_channel_layout_describe could print Ch%d as well, instead of a "?". > >> >>> return channel_names[channel_id].name; >>> } >>> >>> -static const struct { >>> +static inline void get_channel_str(AVBPrint *bp, const char *str, >>> + enum AVChannel channel_id) >>> +{ >>> + if (str) >>> + av_bprintf(bp, "%s", str); >>> + else >>> + av_bprintf(bp, "?"); >> >> If this is not allowed, then you should propagate back AVERROR(EINVAL) if >> it does. If it is allowed, because the user can use some custom >> channel_id-s, then something like "USER%d" should be returned IMHO. > > How about Ch%d? It would also further improve the usefulness of > av_channel_layout_from_string() by looking for such strings. I find "Ch%d" a bit confusing, beacuse "Ch%d" would normally mean the n-th channel in a layout, not a channel with the n-th ID. >> >> I would like to attach some extendable, possibly per-channel metadata to >> the channel layout. I'd rather put it into AVChannelLayout, so native >> layouts could also have metadata. This must be dynamically allocated to >> make it extendable and to not consume too many bytes. I can accept that it >> will be slow. But I don't see it unmanagable, because AVChannelLayout >> already have functions for allocation/free/copy. I also think that most of >> the time it will not be used (e.g. if metadata is NULL, that can mean no >> metadata for all the channels). >> >> If we can't decide what this should be, array of AVDictionaries, some >> side-data like approach but in the channel layout, or a new dynamically >> allocated AVChannelLayoutMetadata struct, then maybe just reserve a void* >> in the end of the AVChannelLayout, and we can work it out later. > > How about a "const uint8_t *name" in AVChannelCustom? You can then point to > some string that will not be owned by the layout, nor duplicated on copy, and > that will be used by the helpers (av_channel_layout_channel_from_string, > av_channel_layout_index_from_string, av_channel_layout_describe) to identify > the channel. > This avoids doing dynamic allocations per channel. But this kind of limits the names to compile time string constants. I'd rather not add something so limited. Regards, Marton
On Tue, 14 Dec 2021, Lynne wrote: > 14 Dec 2021, 02:13 by cus@passwd.hu: > >> >> >> On Tue, 7 Dec 2021, James Almer wrote: >> >> I would like to attach some extendable, possibly per-channel metadata to the channel layout. I'd rather put it into AVChannelLayout, so native layouts could also have metadata. This must be dynamically allocated to make it extendable and to not consume too many bytes. I can accept that it will be slow. But I don't see it unmanagable, because AVChannelLayout already have functions for allocation/free/copy. I also think that most of the time it will not be used (e.g. if metadata is NULL, that can mean no metadata for all the channels). >> >> If we can't decide what this should be, array of AVDictionaries, some side-data like approach but in the channel layout, or a new dynamically allocated AVChannelLayoutMetadata struct, then maybe just reserve a void* in the end of the AVChannelLayout, and we can work it out later. >> > > The idea to use opaque indices is IMO better. You can store any > metadata you want into the frame->opaque(_buf) field, and it'll > be completely copied of av_frame_ref(). > If we go for an opaque pointer, I'd like for it to be copied (by > value, not data) into any ref'd frames. I'd prefer an opaque > field over an opaque metadata field. It's not just frames which have channel layouts, but AVCodecParameters, AVStreams or AVFilterLinks. It is not the user defined metadata we have a problem storing/getting, but framework (demuxer, filter) defined metadata. Regards, Marton
Quoting Marton Balint (2021-12-14 02:13:00) > > + * > > + * Copying an AVChannelLayout via assigning is forbidden, > > + * av_channel_layout_copy() must be used. instead (and its return value should > > + * be checked) > > + * > > + * 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). > > + */ > > +typedef struct AVChannelLayout { > > + /** > > + * Channel order used in this layout. > > + * This is a mandatory field. > > + */ > > + enum AVChannelOrder order; > > + > > + /** > > + * Number of channels in this layout. Mandatory field. > > + */ > > + int nb_channels; > > + > > + /** > > + * 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 presence of the AVChannel with the > > + * corresponding value in map[i].id. > > + * > > + * I.e. when map[i].id is equal to AV_CHAN_FOO, then AV_CH_FOO is the > > + * i-th channel in the audio data. > > + */ > > + AVChannelCustom *map; > > + } u; > > I would like to attach some extendable, possibly per-channel metadata to > the channel layout. I'd rather put it into AVChannelLayout, so native > layouts could also have metadata. This must be dynamically allocated to > make it extendable and to not consume too many bytes. I can accept that it > will be slow. But I don't see it unmanagable, because AVChannelLayout > already have functions for allocation/free/copy. I also think that most of > the time it will not be used (e.g. if metadata is NULL, that can mean no > metadata for all the channels). > > If we can't decide what this should be, array of AVDictionaries, some > side-data like approach but in the channel layout, or a new dynamically > allocated AVChannelLayoutMetadata struct, then maybe just reserve a void* > in the end of the AVChannelLayout, and we can work it out later. I would much prefer not to have any extra dynamically allocated complexity here, especially not AVDictionaries. E.g. the AVFrame dictionary has been a horrible blight on avfilter - now countless filters use it export structured values as strings, instead of defining a proper API through side data. The same thing happening to channel layouts is something we very much do not want IMO. Opaque IDs referring to higher-layer side data allows implementing the same capabilities without stuffing complexity in places it doesn't belong.
On 12/14/2021 5:27 AM, Marton Balint wrote: > > > On Mon, 13 Dec 2021, James Almer wrote: > >> >> >> On 12/13/2021 10:13 PM, Marton Balint wrote: >>> >>> >>> On Tue, 7 Dec 2021, James Almer wrote: >>> >>>> From: Anton Khirnov <anton@khirnov.net> >>>> >>> >>> [...] >>> >>>> -static const char *get_channel_name(int channel_id) >>>> +static const char *get_channel_name(enum AVChannel channel_id) >>>> { >>>> - if (channel_id < 0 || channel_id >= >>>> FF_ARRAY_ELEMS(channel_names)) >>>> - return NULL; >>>> + if ((unsigned) channel_id >= FF_ARRAY_ELEMS(channel_names) || >>>> + !channel_names[channel_id].name) >>>> + return "?"; >>> >>> I'd rather return NULL here instead of "?". Is this allowed to >>> happen by >>> the way? >> >> Yes, even in a native layout, several AVChannel values have no name. >> The idea is to print a string like "FL|FR|?" when the name of a >> channel is unknown. See the test in patch 2. > > As far as I see this function is used by av_channel_layout_describe. If > get_channel_name returned NULL, then av_channel_layout_describe could > print Ch%d as well, instead of a "?". It's also used by the legacy API now that you mention it, and returning "?" is a behavior change, so thanks for noticing it. > >> >>> >>>> return channel_names[channel_id].name; >>>> } >>>> >>>> -static const struct { >>>> +static inline void get_channel_str(AVBPrint *bp, const char *str, >>>> + enum AVChannel channel_id) >>>> +{ >>>> + if (str) >>>> + av_bprintf(bp, "%s", str); >>>> + else >>>> + av_bprintf(bp, "?"); >>> >>> If this is not allowed, then you should propagate back >>> AVERROR(EINVAL) if >>> it does. If it is allowed, because the user can use some custom >>> channel_id-s, then something like "USER%d" should be returned IMHO. >> >> How about Ch%d? It would also further improve the usefulness of >> av_channel_layout_from_string() by looking for such strings. > > I find "Ch%d" a bit confusing, beacuse "Ch%d" would normally mean the > n-th channel in a layout, not a channel with the n-th ID. What do you suggest? USER%d is IMO ugly. Maybe ChID%d? Although both are a bit long. > >>> >>> I would like to attach some extendable, possibly per-channel >>> metadata to >>> the channel layout. I'd rather put it into AVChannelLayout, so native >>> layouts could also have metadata. This must be dynamically allocated to >>> make it extendable and to not consume too many bytes. I can accept >>> that it >>> will be slow. But I don't see it unmanagable, because AVChannelLayout >>> already have functions for allocation/free/copy. I also think that >>> most of >>> the time it will not be used (e.g. if metadata is NULL, that can >>> mean no >>> metadata for all the channels). >>> >>> If we can't decide what this should be, array of AVDictionaries, some >>> side-data like approach but in the channel layout, or a new dynamically >>> allocated AVChannelLayoutMetadata struct, then maybe just reserve a >>> void* >>> in the end of the AVChannelLayout, and we can work it out later. >> >> How about a "const uint8_t *name" in AVChannelCustom? You can then >> point to some string that will not be owned by the layout, nor >> duplicated on copy, and that will be used by the helpers >> (av_channel_layout_channel_from_string, >> av_channel_layout_index_from_string, av_channel_layout_describe) to >> identify the channel. >> This avoids doing dynamic allocations per channel. > > But this kind of limits the names to compile time string constants. I'd > rather not add something so limited. You could attach it dynamically allocated strings too, and to prevent the need for the module allocating them to outlive the layout, we could add an opaque pointer to AVChannelLayout (not AVChannelCustom) and a user set free() call back to pass that pointer to, on uninit(). > > Regards, > Marton > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
James Almer (12021-12-14): > You could attach it dynamically allocated strings too, and to prevent the > need for the module allocating them to outlive the layout, we could add an > opaque pointer to AVChannelLayout (not AVChannelCustom) and a user set > free() call back to pass that pointer to, on uninit(). So you want to shift the responsibility to worry about pointer lifetime and ownership to the API user. Sure, we can do that. We can always shift responsibility to API users, but that is not very nice to them. I think the best solution would be to make this new structure ref-counted. Remember that I proposed a template to make ref-counted structures easily: https://ffmpeg.org/pipermail/ffmpeg-devel/2020-June/265227.html (Notice that it was soon after the first version of this API was posted: I was thinking of it already then.) Regards,
Anton Khirnov (12021-12-14): > The same thing happening to channel layouts is something we very much do > not want IMO. Opaque IDs referring to higher-layer side data allows > implementing the same capabilities without stuffing complexity in places > it doesn't belong. Opaque IDs only work for applications that were designed to make use of them and have logic to map them onto something the user can understand. We need something that can work with a generic application, starting with ffmpeg itself. Something that will work if only a few components (muxers and demuxers maybe) know about these strange layouts, and most components only look at the standard part of the layout and behave as they do now or with the straightforward extension to the more powerful API.
Quoting Nicolas George (2021-12-14 14:15:05) > Anton Khirnov (12021-12-14): > > The same thing happening to channel layouts is something we very much do > > not want IMO. Opaque IDs referring to higher-layer side data allows > > implementing the same capabilities without stuffing complexity in places > > it doesn't belong. > > Opaque IDs only work for applications that were designed to make use of > them and have logic to map them onto something the user can understand. > We need something that can work with a generic application, starting > with ffmpeg itself. Something that will work if only a few components > (muxers and demuxers maybe) know about these strange layouts, and most > components only look at the standard part of the layout and behave as > they do now or with the straightforward extension to the more powerful > API. A transcoder that is not aware of opaque IDs can just blindly copy them as the data travels along the chain. Same with side data those IDs refer to. Callers who want to do something nontrivial with them will of course have to be aware of them, but that's the case either way.
14 Dec 2021, 09:32 by cus@passwd.hu: > > > On Tue, 14 Dec 2021, Lynne wrote: > >> 14 Dec 2021, 02:13 by cus@passwd.hu: >> >>> >>> >>> On Tue, 7 Dec 2021, James Almer wrote: >>> >>> I would like to attach some extendable, possibly per-channel metadata to the channel layout. I'd rather put it into AVChannelLayout, so native layouts could also have metadata. This must be dynamically allocated to make it extendable and to not consume too many bytes. I can accept that it will be slow. But I don't see it unmanagable, because AVChannelLayout already have functions for allocation/free/copy. I also think that most of the time it will not be used (e.g. if metadata is NULL, that can mean no metadata for all the channels). >>> >>> If we can't decide what this should be, array of AVDictionaries, some side-data like approach but in the channel layout, or a new dynamically allocated AVChannelLayoutMetadata struct, then maybe just reserve a void* in the end of the AVChannelLayout, and we can work it out later. >>> >> >> The idea to use opaque indices is IMO better. You can store any >> metadata you want into the frame->opaque(_buf) field, and it'll >> be completely copied of av_frame_ref(). >> If we go for an opaque pointer, I'd like for it to be copied (by >> value, not data) into any ref'd frames. I'd prefer an opaque >> field over an opaque metadata field. >> > > It's not just frames which have channel layouts, but AVCodecParameters, AVStreams or AVFilterLinks. It is not the user defined metadata we have a problem storing/getting, but framework (demuxer, filter) defined metadata. > In that case, I think it's fine to have a dictionary metadata field in the per-channel struct, It'll increase the struct size slightly, but I think it's acceptable. I think this discussion would go a lot quicker if we could discuss this on IRC, would you and Nicolas mind joining and pinging me and elenril so we could settle the details?
Lynne (12021-12-14): > I think this discussion would go a lot quicker if we could discuss this > on IRC, would you and Nicolas mind joining and pinging me and elenril > so we could settle the details? Yes, I would mind; more: I cannot; I have other things claiming my time. And I believe people who come a little late deserve to know what the arguments were and even to weigh in. IRC is a terrible medium for this kind of discussion. Regards,
Anton Khirnov (12021-12-14): > A transcoder that is not aware of opaque IDs can just blindly copy them > as the data travels along the chain. Same with side data those IDs refer > to. Callers who want to do something nontrivial with them will of course > have to be aware of them, but that's the case either way. You are still neglecting the user interface side of things. Please try to consider actual scenarios with the example Marton and I gave from the point of view of users using the generic command-line tool.
14 Dec 2021, 15:23 by george@nsup.org: > Lynne (12021-12-14): > >> I think this discussion would go a lot quicker if we could discuss this >> on IRC, would you and Nicolas mind joining and pinging me and elenril >> so we could settle the details? >> > > Yes, I would mind; more: I cannot; I have other things claiming my time. > > And I believe people who come a little late deserve to know what the > arguments were and even to weigh in. IRC is a terrible medium for this > kind of discussion. > Ah, okay, I understand. Does this mean you'd be okay with letting us choose, as long as your requirements are satisfied?
Lynne (12021-12-14): > Ah, okay, I understand. > Does this mean you'd be okay with letting us choose, as long as your > requirements are satisfied? Of course I am okay with a solution where my requirements are satisfied. But I cannot say if they are before seeing the solution you propose, and other people are allowed to weigh in when it is posted here, too. Regards,
On 12/14/2021 9:53 AM, Nicolas George wrote: > James Almer (12021-12-14): >> You could attach it dynamically allocated strings too, and to prevent the >> need for the module allocating them to outlive the layout, we could add an >> opaque pointer to AVChannelLayout (not AVChannelCustom) and a user set >> free() call back to pass that pointer to, on uninit(). > > So you want to shift the responsibility to worry about pointer lifetime > and ownership to the API user. Sure, we can do that. We can always shift > responsibility to API users, but that is not very nice to them. We add a const uint8_t* field to AVChannelCustom. If the user wants to allocate the strings instead, and not worry about their lifetime, they can provide the layout with a custom free() function that will take care of cleaning anything they came up with on uninit(). > > I think the best solution would be to make this new structure > ref-counted. Having something like this called on every copy sounds inefficient. And what will you refcount? The layout? Each channel's string? Can you be more specific about how do you imagine implementing this? > > Remember that I proposed a template to make ref-counted structures > easily: > https://ffmpeg.org/pipermail/ffmpeg-devel/2020-June/265227.html > (Notice that it was soon after the first version of this API was posted: > I was thinking of it already then.) > > Regards, > > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
James Almer (12021-12-14): > We add a const uint8_t* field to AVChannelCustom. If the user wants to > allocate the strings instead, and not worry about their lifetime, they can > provide the layout with a custom free() function that will take care of > cleaning anything they came up with on uninit(). I understood what you suggested, and it amounts to letting API users fend for themselves. We can do that, but I would prefer if we only did on last resort, if we do not find a more convenient solution. > Having something like this called on every copy sounds inefficient. And what What??? Ref-counting makes operations more efficient by avoiding new dynamic allocations. I grant you, the code currently tries to avoid dynamic allocations, but I do not think it is actually manageable without seriously limiting the features of the API. And you know me, I try to avoid dynamic allocations as much as possible. > will you refcount? The layout? Each channel's string? Can you be more > specific about how do you imagine implementing this? The whole layout structure, with all the possible custom layouts, dictionaries and strings. I do not think that sharing strings between layouts that are similar but not equal is worth the added complexity. (We may consider a ref-counted string type at some point, maybe, but that is another story.) Regards,
On 12/14/2021 3:54 PM, Nicolas George wrote: > James Almer (12021-12-14): >> We add a const uint8_t* field to AVChannelCustom. If the user wants to >> allocate the strings instead, and not worry about their lifetime, they can >> provide the layout with a custom free() function that will take care of >> cleaning anything they came up with on uninit(). > > I understood what you suggested, and it amounts to letting API users > fend for themselves. We can do that, but I would prefer if we only did > on last resort, if we do not find a more convenient solution. There's "char name[16]". Bigger than a pointer (Could be 8 bytes instead, but then it's kinda small). The user will not have to worry about the lifetime of anything then. I'm attaching a diff that goes on top of the last patch of this set implementing this. It also adds support for these custom names to av_channel_layout_describe(), av_channel_layout_index_from_string() and av_channel_layout_channel_from_string(), including tests. > >> Having something like this called on every copy sounds inefficient. And what > > What??? Ref-counting makes operations more efficient by avoiding new > dynamic allocations. > > I grant you, the code currently tries to avoid dynamic allocations, but > I do not think it is actually manageable without seriously limiting the > features of the API. And you know me, I try to avoid dynamic allocations > as much as possible. > >> will you refcount? The layout? Each channel's string? Can you be more >> specific about how do you imagine implementing this? > > The whole layout structure, with all the possible custom layouts, > dictionaries and strings. > > I do not think that sharing strings between layouts that are similar but > not equal is worth the added complexity. > > (We may consider a ref-counted string type at some point, maybe, but > that is another story.) > > Regards, > > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". diff --git a/libavutil/channel_layout.c b/libavutil/channel_layout.c index a51af95fcf..09f8fbe4b5 100644 --- a/libavutil/channel_layout.c +++ b/libavutil/channel_layout.c @@ -548,8 +548,8 @@ static int try_describe_ambisonic(AVBPrint *bp, const AVChannelLayout *channel_l if (!extra.u.map) return AVERROR(ENOMEM); - for (i = 0; i < extra.nb_channels; i++) - extra.u.map[i].id = map[highest_ambi + 1 + i].id; + memcpy(extra.u.map, &map[highest_ambi + 1], + sizeof(*extra.u.map) * extra.nb_channels); av_channel_layout_describe(&extra, buf, sizeof(buf)); av_channel_layout_uninit(&extra); @@ -587,8 +587,15 @@ int av_channel_layout_describe(const AVChannelLayout *channel_layout, } for (i = 0; i < channel_layout->nb_channels; i++) { - enum AVChannel ch = av_channel_layout_channel_from_index(channel_layout, i); - const char *ch_name = get_channel_name(ch); + const char *ch_name = NULL; + + if (channel_layout->order == AV_CHANNEL_ORDER_CUSTOM && + channel_layout->u.map[i].name[0]) + ch_name = channel_layout->u.map[i].name; + if (channel_layout->order == AV_CHANNEL_ORDER_NATIVE || !ch_name) { + enum AVChannel ch = av_channel_layout_channel_from_index(channel_layout, i); + ch_name = get_channel_name(ch); + } if (i) av_bprintf(&bp, "|"); @@ -641,6 +648,11 @@ av_channel_layout_channel_from_string(const AVChannelLayout *channel_layout, switch (channel_layout->order) { case AV_CHANNEL_ORDER_CUSTOM: + for (int i = 0; i < channel_layout->nb_channels; i++) { + if (channel_layout->u.map[i].name[0] && !strcmp(name, channel_layout->u.map[i].name)) + return channel_layout->u.map[i].id; + } + // fall-through case AV_CHANNEL_ORDER_NATIVE: channel = av_channel_from_string(name); if (channel < 0) @@ -687,13 +699,22 @@ int av_channel_layout_index_from_string(const AVChannelLayout *channel_layout, { int ret; - if (channel_layout->order == AV_CHANNEL_ORDER_UNSPEC) - return AVERROR(EINVAL); + switch (channel_layout->order) { + case AV_CHANNEL_ORDER_CUSTOM: + for (int i = 0; i < channel_layout->nb_channels; i++) { + if (channel_layout->u.map[i].name[0] && !strcmp(name, channel_layout->u.map[i].name)) + return i; + } + // fall-through + case AV_CHANNEL_ORDER_NATIVE: + case AV_CHANNEL_ORDER_AMBISONIC: + ret = av_channel_from_string(name); + if (ret < 0) + return ret; + return av_channel_layout_index_from_channel(channel_layout, ret); + } - ret = av_channel_from_string(name); - if (ret < 0) - return ret; - return av_channel_layout_index_from_channel(channel_layout, ret); + return AVERROR(EINVAL); } int av_channel_layout_check(const AVChannelLayout *channel_layout) diff --git a/libavutil/channel_layout.h b/libavutil/channel_layout.h index 7b77a74b61..996c389f5d 100644 --- a/libavutil/channel_layout.h +++ b/libavutil/channel_layout.h @@ -253,6 +253,7 @@ enum AVMatrixEncoding { */ typedef struct AVChannelCustom { enum AVChannel id; + char name[16]; } AVChannelCustom; /** @@ -330,6 +331,10 @@ typedef struct AVChannelLayout { * AV_CHAN_AMBISONIC_END (inclusive), the channel contains an ambisonic * component with ACN index (as defined above) * n = map[i].id - AV_CHAN_AMBISONIC_BASE. + * + * map[i].name may be filled with a 0-terminated string, in which case + * it will be used for the purpose of identifying the channel with the + * convenience functions below. Otherise it must be zeroed. */ AVChannelCustom *map; } u; diff --git a/libavutil/tests/channel_layout.c b/libavutil/tests/channel_layout.c index e4b42b1574..3c5b3c3320 100644 --- a/libavutil/tests/channel_layout.c +++ b/libavutil/tests/channel_layout.c @@ -183,6 +183,7 @@ int main(void) custom.u.map[0].id = AV_CHAN_FRONT_RIGHT; custom.u.map[1].id = AV_CHAN_FRONT_LEFT; custom.u.map[2].id = 63; + av_strlcpy(custom.u.map[2].name, "Ch63", sizeof(custom.u.map[2].name)); buf[0] = 0; printf("\nTesting av_channel_layout_describe\n"); av_channel_layout_describe(&custom, buf, sizeof(buf)); @@ -193,6 +194,8 @@ int main(void) printf("On \"FR|FL|Ch63\" layout with \"FR\": %18d\n", ret); CHANNEL_LAYOUT_INDEX_FROM_STRING(custom, "FL"); printf("On \"FR|FL|Ch63\" layout with \"FL\": %18d\n", ret); + CHANNEL_LAYOUT_INDEX_FROM_STRING(custom, "Ch63"); + printf("On \"FR|FL|Ch63\" layout with \"Ch63\": %16d\n", ret); CHANNEL_LAYOUT_INDEX_FROM_STRING(custom, "BC"); printf("On \"FR|FL|Ch63\" layout with \"BC\": %18d\n", ret); @@ -201,6 +204,8 @@ int main(void) printf("On \"FR|FL|Ch63\" layout with \"FR\": %18d\n", ret); CHANNEL_LAYOUT_CHANNEL_FROM_STRING(custom, "FL"); printf("On \"FR|FL|Ch63\" layout with \"FL\": %18d\n", ret); + CHANNEL_LAYOUT_CHANNEL_FROM_STRING(custom, "Ch63"); + printf("On \"FR|FL|Ch63\" layout with \"Ch63\": %16d\n", ret); CHANNEL_LAYOUT_CHANNEL_FROM_STRING(custom, "BC"); printf("On \"FR|FL|Ch63\" layout with \"BC\": %18d\n", ret); diff --git a/tests/ref/fate/channel_layout b/tests/ref/fate/channel_layout index 1a74216125..a1cb9b7e04 100644 --- a/tests/ref/fate/channel_layout +++ b/tests/ref/fate/channel_layout @@ -69,16 +69,18 @@ On 5.1(side) layout with "BC": -1 ==Custom layouts== Testing av_channel_layout_describe -On "FR|FL|Ch63" layout: FR|FL|? +On "FR|FL|Ch63" layout: FR|FL|Ch63 Testing av_channel_layout_index_from_string On "FR|FL|Ch63" layout with "FR": 0 On "FR|FL|Ch63" layout with "FL": 1 +On "FR|FL|Ch63" layout with "Ch63": 2 On "FR|FL|Ch63" layout with "BC": -1 Testing av_channel_layout_channel_from_string On "FR|FL|Ch63" layout with "FR": 1 On "FR|FL|Ch63" layout with "FL": 0 +On "FR|FL|Ch63" layout with "Ch63": 63 On "FR|FL|Ch63" layout with "BC": -1 Testing av_channel_layout_index_from_channel
On Tue, Dec 14, 2021 at 04:30:04PM -0300, James Almer wrote: > On 12/14/2021 3:54 PM, Nicolas George wrote: > > James Almer (12021-12-14): > > > We add a const uint8_t* field to AVChannelCustom. If the user wants to > > > allocate the strings instead, and not worry about their lifetime, they can > > > provide the layout with a custom free() function that will take care of > > > cleaning anything they came up with on uninit(). > > > > I understood what you suggested, and it amounts to letting API users > > fend for themselves. We can do that, but I would prefer if we only did > > on last resort, if we do not find a more convenient solution. > > There's "char name[16]". Bigger than a pointer (Could be 8 bytes instead, > but then it's kinda small). The user will not have to worry about the > lifetime of anything then. the advantages of a fixed length array are * no dynamic allocation (overhead, complexity, error handling) * no refcounting * easier to cleanly display in user interfaces than arbitrary length strings * no question about what to do with overly long strings (mb, gb?) thx [...]
On Tue, 14 Dec 2021, James Almer wrote: >> >>> >>>> >>>>> return channel_names[channel_id].name; >>>>> } >>>>> >>>>> -static const struct { >>>>> +static inline void get_channel_str(AVBPrint *bp, const char *str, >>>>> + enum AVChannel channel_id) >>>>> +{ >>>>> + if (str) >>>>> + av_bprintf(bp, "%s", str); >>>>> + else >>>>> + av_bprintf(bp, "?"); >>>> >>>> If this is not allowed, then you should propagate back AVERROR(EINVAL) >>>> if >>>> it does. If it is allowed, because the user can use some custom >>>> channel_id-s, then something like "USER%d" should be returned IMHO. >>> >>> How about Ch%d? It would also further improve the usefulness of >>> av_channel_layout_from_string() by looking for such strings. >> >> I find "Ch%d" a bit confusing, beacuse "Ch%d" would normally mean the n-th >> channel in a layout, not a channel with the n-th ID. > > What do you suggest? USER%d is IMO ugly. Maybe ChID%d? Although both are a > bit long. Can be Usr%d, Dsg%d if you like these better. If not then, I don't mind too much Ch%d, I just find it a bit confusing. Regards, Marton
On Wed, 15 Dec 2021, Marton Balint wrote: > > > On Tue, 14 Dec 2021, James Almer wrote: > >>> >>>> >>>>> >>>>>> return channel_names[channel_id].name; >>>>>> } >>>>>> >>>>>> -static const struct { >>>>>> +static inline void get_channel_str(AVBPrint *bp, const char *str, >>>>>> + enum AVChannel channel_id) >>>>>> +{ >>>>>> + if (str) >>>>>> + av_bprintf(bp, "%s", str); >>>>>> + else >>>>>> + av_bprintf(bp, "?"); >>>>> >>>>> If this is not allowed, then you should propagate back >>>>> AVERROR(EINVAL) >>>>> if >>>>> it does. If it is allowed, because the user can use some custom >>>>> channel_id-s, then something like "USER%d" should be returned IMHO. >>>> >>>> How about Ch%d? It would also further improve the usefulness of >>>> av_channel_layout_from_string() by looking for such strings. >>> >>> I find "Ch%d" a bit confusing, beacuse "Ch%d" would normally mean the >>> n-th >>> channel in a layout, not a channel with the n-th ID. >> >> What do you suggest? USER%d is IMO ugly. Maybe ChID%d? Although both are a >> bit long. > > Can be Usr%d, Dsg%d if you like these better. If not then, I don't mind too > much Ch%d, I just find it a bit confusing. One more thing, don't we want to follow existing convention? E.g. for channel name a abbriviated name with all capital letters, (that is why I suggested USER%d as a first), and for channel desription, a longer one, with lowercase, e.g. "user %d". I noticed the same inconsistency for ambisonic, because as far as I see for that now both the short name is "ambisonic %d" and the description is also. It would be more consistent to have "AMBI%d" and "amibisonic %d" for name and description. Regards, Marton
On Tue, 14 Dec 2021, James Almer wrote: > On 12/14/2021 3:54 PM, Nicolas George wrote: >> James Almer (12021-12-14): >>> We add a const uint8_t* field to AVChannelCustom. If the user wants to >>> allocate the strings instead, and not worry about their lifetime, they >>> can >>> provide the layout with a custom free() function that will take care of >>> cleaning anything they came up with on uninit(). >> >> I understood what you suggested, and it amounts to letting API users >> fend for themselves. We can do that, but I would prefer if we only did >> on last resort, if we do not find a more convenient solution. > > There's "char name[16]". Bigger than a pointer (Could be 8 bytes instead, but > then it's kinda small). The user will not have to worry about the lifetime of > anything then. > > I'm attaching a diff that goes on top of the last patch of this set > implementing this. It also adds support for these custom names to > av_channel_layout_describe(), av_channel_layout_index_from_string() and > av_channel_layout_channel_from_string(), including tests. I'd rather not mix custom labels with our fixed names for channels. E.g. what if a label conflicts with our existing channel names? If the user wants to specify a channel based on its label, that should be a separate syntax IMHO. Overall, having a char name[16] is still kind of limiting, e.g. a label read from a muxed file will be truncated, I'd rather not have anything. Here is one more idea, kind of a mix of what I read so far: Let's refcount only the dynamically allocated part of AVChannelLayout. Something like: typedef struct AVChannelLayout { enum AVChannelOrder order; int nb_channels; uint64_t mask; AVBufferRef *custom; } AVChannelLayout; And the reference counted data could point to an array of AVChannelCustom entries. And AVChannelCustom can have AVDictionary *metadata, because it is refcounted. Regards, Marton
James Almer (12021-12-14): > There's "char name[16]". Bigger than a pointer (Could be 8 bytes instead, > but then it's kinda small). The user will not have to worry about the > lifetime of anything then. I find the fixed-size limiting. Please seriously consider the alternative: AVChannelLayout is dynamically allocated and ref-counted, and manages all the memory. If you consider this seriously (as a complete redesign of the API, not as a small change), and think it is not worth it, I will not insist and resign myself to fixed-size label + opaque id. But note that if Marton still wants more than a single fixed-size string, I support. > I'm attaching a diff that goes on top of the last patch of this set > implementing this. It also adds support for these custom names to > av_channel_layout_describe(), av_channel_layout_index_from_string() and > av_channel_layout_channel_from_string(), including tests. Thanks. Now we need the user interface: how does the user say “the left channel with label "oboe"”? Probably something like "FL.oboe". The parsing functions need to be adapted. Regards,
On 12/15/2021 7:24 AM, Marton Balint wrote: > > > On Tue, 14 Dec 2021, James Almer wrote: > >> On 12/14/2021 3:54 PM, Nicolas George wrote: >>> James Almer (12021-12-14): >>>> We add a const uint8_t* field to AVChannelCustom. If the user wants to >>>> allocate the strings instead, and not worry about their lifetime, they >>>> can >>>> provide the layout with a custom free() function that will take >>>> care of >>>> cleaning anything they came up with on uninit(). >>> >>> I understood what you suggested, and it amounts to letting API users >>> fend for themselves. We can do that, but I would prefer if we only did >>> on last resort, if we do not find a more convenient solution. >> >> There's "char name[16]". Bigger than a pointer (Could be 8 bytes >> instead, but then it's kinda small). The user will not have to worry >> about the lifetime of anything then. >> >> I'm attaching a diff that goes on top of the last patch of this set >> implementing this. It also adds support for these custom names to >> av_channel_layout_describe(), av_channel_layout_index_from_string() >> and av_channel_layout_channel_from_string(), including tests. > > I'd rather not mix custom labels with our fixed names for channels. E.g. > what if a label conflicts with our existing channel names? If the user > wants to specify a channel based on its label, that should be a separate > syntax IMHO. > > Overall, having a char name[16] is still kind of limiting, e.g. a label > read from a muxed file will be truncated, I'd rather not have anything. Container metadata is typically be exported as stream metadata or side data. > > Here is one more idea, kind of a mix of what I read so far: Let's > refcount only the dynamically allocated part of AVChannelLayout. > Something like: > > typedef struct AVChannelLayout { > enum AVChannelOrder order; > int nb_channels; > uint64_t mask; > AVBufferRef *custom; > } AVChannelLayout; > > And the reference counted data could point to an array of > AVChannelCustom entries. And AVChannelCustom can have AVDictionary > *metadata, because it is refcounted. AVBufferRef is meant to hold flat arrays, though. And where would you store the custom channel names? As a fixed array like in the above, or in the dictionary? The latter sounds like a nightmare for both the helpers and as an API user. Also, since AVChannelCustom would have a dictionary, the AVBufferRef holding it needs a custom free() function. This makes creating layouts manually a pain. And what about the buffer being writable or not? You need to consider both copy and ref scenarios. It's a lot of added complexity for what should be a simple "this stream with six channels has the channels arranged like this in your room: This is front left, this is front right, this is 'under the carpet', etc". People want this to automatically handle very specific filtering scenarios to remove one entry in their chain when it's not its job. > > Regards, > Marton > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
On Wed, 15 Dec 2021, James Almer wrote: > > > On 12/15/2021 7:24 AM, Marton Balint wrote: >> >> >> On Tue, 14 Dec 2021, James Almer wrote: >> >>> On 12/14/2021 3:54 PM, Nicolas George wrote: >>>> James Almer (12021-12-14): >>>>> We add a const uint8_t* field to AVChannelCustom. If the user wants to >>>>> allocate the strings instead, and not worry about their lifetime, they >>>>> can >>>>> provide the layout with a custom free() function that will take care >>>>> of >>>>> cleaning anything they came up with on uninit(). >>>> >>>> I understood what you suggested, and it amounts to letting API users >>>> fend for themselves. We can do that, but I would prefer if we only did >>>> on last resort, if we do not find a more convenient solution. >>> >>> There's "char name[16]". Bigger than a pointer (Could be 8 bytes instead, >>> but then it's kinda small). The user will not have to worry about the >>> lifetime of anything then. >>> >>> I'm attaching a diff that goes on top of the last patch of this set >>> implementing this. It also adds support for these custom names to >>> av_channel_layout_describe(), av_channel_layout_index_from_string() and >>> av_channel_layout_channel_from_string(), including tests. >> >> I'd rather not mix custom labels with our fixed names for channels. E.g. >> what if a label conflicts with our existing channel names? If the user >> wants to specify a channel based on its label, that should be a separate >> syntax IMHO. >> >> Overall, having a char name[16] is still kind of limiting, e.g. a label >> read from a muxed file will be truncated, I'd rather not have anything. > > Container metadata is typically be exported as stream metadata or side data. That is always a possibility, but if you want some data per-channel, and not per-stream, (e.g. variable size labels) then side data becomes difficult to work with. > >> >> Here is one more idea, kind of a mix of what I read so far: Let's refcount >> only the dynamically allocated part of AVChannelLayout. Something like: >> >> typedef struct AVChannelLayout { >> enum AVChannelOrder order; >> int nb_channels; >> uint64_t mask; >> AVBufferRef *custom; >> } AVChannelLayout; >> >> And the reference counted data could point to an array of AVChannelCustom >> entries. And AVChannelCustom can have AVDictionary *metadata, because it >> is refcounted. > > AVBufferRef is meant to hold flat arrays, though. Since sizeof(AVChannelCustom) is fixed, I don't really see the difference. > the custom channel names? As a fixed array like in the above, or in the > dictionary? The latter sounds like a nightmare for both the helpers and as an > API user. Personally I think it can be put in the metadata dictionary, because if we work out some syntax to select channel based on their labels, then it also make sense to select channels based on other metadata, and syntax might be extended with it. E.g. FL.label.Oboe. or FL.language.eng. But if for some reason that is frowned upon, we can extend AVChannelCustom with a char *label, and free it as well on free(). > Also, since AVChannelCustom would have a dictionary, the AVBufferRef holding > it needs a custom free() function. This makes creating layouts manually a > pain. You are already initalizing the dynamically allocated part of AVChannelLayout: layout.u.map = av_mallocz_array(nb_channels, sizeof()); If AVBufferRef is used for that, then a helper function can be added: layout.custom = av_channel_layout_custom_new(nb_channels); No difference really. > And what about the buffer being writable or not? You need to consider > both copy and ref scenarios. av_channel_layout_copy() can simply ref. If a deep copy is needed because the user wants to change anything in AVChannelCustom, then helper function can be added. > > It's a lot of added complexity for what should be a simple "this stream with > six channels has the channels arranged like this in your room: This is front > left, this is front right, this is 'under the carpet', etc". See the attached proof-of-concept patch, to be used on top of your channel_layout branch. Is it that bad? Thanks, Marton
On 12/15/2021 6:32 PM, Marton Balint wrote: > > > On Wed, 15 Dec 2021, James Almer wrote: > >> >> >> On 12/15/2021 7:24 AM, Marton Balint wrote: >>> >>> >>> On Tue, 14 Dec 2021, James Almer wrote: >>> >>>> On 12/14/2021 3:54 PM, Nicolas George wrote: >>>>> James Almer (12021-12-14): >>>>>> We add a const uint8_t* field to AVChannelCustom. If the user >>>>>> wants to >>>>>> allocate the strings instead, and not worry about their >>>>>> lifetime, they >>>>>> can >>>>>> provide the layout with a custom free() function that will take >>>>>> care >>>>>> of >>>>>> cleaning anything they came up with on uninit(). >>>>> >>>>> I understood what you suggested, and it amounts to letting API users >>>>> fend for themselves. We can do that, but I would prefer if we >>>>> only did >>>>> on last resort, if we do not find a more convenient solution. >>>> >>>> There's "char name[16]". Bigger than a pointer (Could be 8 bytes >>>> instead, >>>> but then it's kinda small). The user will not have to worry about the >>>> lifetime of anything then. >>>> >>>> I'm attaching a diff that goes on top of the last patch of this set >>>> implementing this. It also adds support for these custom names to >>>> av_channel_layout_describe(), av_channel_layout_index_from_string() >>>> and >>>> av_channel_layout_channel_from_string(), including tests. >>> >>> I'd rather not mix custom labels with our fixed names for channels. >>> E.g. >>> what if a label conflicts with our existing channel names? If the user >>> wants to specify a channel based on its label, that should be a >>> separate >>> syntax IMHO. >>> >>> Overall, having a char name[16] is still kind of limiting, e.g. a label >>> read from a muxed file will be truncated, I'd rather not have anything. >> >> Container metadata is typically be exported as stream metadata or side >> data. > > That is always a possibility, but if you want some data per-channel, and > not per-stream, (e.g. variable size labels) then side data becomes > difficult to work with. > >> >>> >>> Here is one more idea, kind of a mix of what I read so far: Let's >>> refcount >>> only the dynamically allocated part of AVChannelLayout. Something like: >>> >>> typedef struct AVChannelLayout { >>> enum AVChannelOrder order; >>> int nb_channels; >>> uint64_t mask; >>> AVBufferRef *custom; >>> } AVChannelLayout; >>> >>> And the reference counted data could point to an array of >>> AVChannelCustom >>> entries. And AVChannelCustom can have AVDictionary *metadata, >>> because it >>> is refcounted. >> >> AVBufferRef is meant to hold flat arrays, though. > > Since sizeof(AVChannelCustom) is fixed, I don't really see the difference. A flat array of bytes. If you do a copy of the contents of the buffer (av_buffer_make_writable), and there's a pointer in it, you're copying it but not what it points to. The copy is not a reference, so when the last actual reference is freed, the custom free() function is called, it frees the dictionary, and the copy now has a dangling pointer to a non existent dictionary. It's the reason we used to have split side data to handle non flat array structures. > >> the custom channel names? As a fixed array like in the above, or in >> the dictionary? The latter sounds like a nightmare for both the >> helpers and as an API user. > > Personally I think it can be put in the metadata dictionary, because if > we work out some syntax to select channel based on their labels, then it > also make sense to select channels based on other metadata, and syntax > might be extended with it. E.g. FL.label.Oboe. or FL.language.eng. > > But if for some reason that is frowned upon, we can extend > AVChannelCustom with a char *label, and free it as well on free(). Same situation as above. A fixed array would be ok, though. > > >> Also, since AVChannelCustom would have a dictionary, the AVBufferRef >> holding it needs a custom free() function. This makes creating layouts >> manually a pain. > > You are already initalizing the dynamically allocated part of > AVChannelLayout: > > layout.u.map = av_mallocz_array(nb_channels, sizeof()); > > If AVBufferRef is used for that, then a helper function can be added: > > layout.custom = av_channel_layout_custom_new(nb_channels); > > No difference really. > > >> And what about the buffer being writable or not? You need to consider >> both copy and ref scenarios. > > av_channel_layout_copy() can simply ref. If a deep copy is needed > because the user wants to change anything in AVChannelCustom, then > helper function can be added. > >> >> It's a lot of added complexity for what should be a simple "this >> stream with six channels has the channels arranged like this in your >> room: This is front left, this is front right, this is 'under the >> carpet', etc". > > See the attached proof-of-concept patch, to be used on top of your > channel_layout branch. Is it that bad? It's fragile by misusing the AVBufferRef API. Also, why remove the pointer from the union? I'll send a version with a flat name array plus an opaque pointer. Then an email listing all the proposed solutions, to be discussed in a call where we hopefully reach an agreement. > > Thanks, > Marton > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
On Wed, 15 Dec 2021, James Almer wrote: > > > On 12/15/2021 6:32 PM, Marton Balint wrote: >> >> >> On Wed, 15 Dec 2021, James Almer wrote: >> >>> >>> >>> On 12/15/2021 7:24 AM, Marton Balint wrote: >>>> >>>> >>>> On Tue, 14 Dec 2021, James Almer wrote: >>>> >>>>> On 12/14/2021 3:54 PM, Nicolas George wrote: >>>>>> James Almer (12021-12-14): >>>>>>> We add a const uint8_t* field to AVChannelCustom. If the user wants >>>>>>> to >>>>>>> allocate the strings instead, and not worry about their lifetime, >>>>>>> they >>>>>>> can >>>>>>> provide the layout with a custom free() function that will take >>>>>>> care >>>>>>> of >>>>>>> cleaning anything they came up with on uninit(). >>>>>> >>>>>> I understood what you suggested, and it amounts to letting API users >>>>>> fend for themselves. We can do that, but I would prefer if we only >>>>>> did >>>>>> on last resort, if we do not find a more convenient solution. >>>>> >>>>> There's "char name[16]". Bigger than a pointer (Could be 8 bytes >>>>> instead, >>>>> but then it's kinda small). The user will not have to worry about the >>>>> lifetime of anything then. >>>>> >>>>> I'm attaching a diff that goes on top of the last patch of this set >>>>> implementing this. It also adds support for these custom names to >>>>> av_channel_layout_describe(), av_channel_layout_index_from_string() >>>>> and >>>>> av_channel_layout_channel_from_string(), including tests. >>>> >>>> I'd rather not mix custom labels with our fixed names for channels. >>>> E.g. >>>> what if a label conflicts with our existing channel names? If the user >>>> wants to specify a channel based on its label, that should be a >>>> separate >>>> syntax IMHO. >>>> >>>> Overall, having a char name[16] is still kind of limiting, e.g. a label >>>> read from a muxed file will be truncated, I'd rather not have anything. >>> >>> Container metadata is typically be exported as stream metadata or side >>> data. >> >> That is always a possibility, but if you want some data per-channel, and >> not per-stream, (e.g. variable size labels) then side data becomes >> difficult to work with. >> >>> >>>> >>>> Here is one more idea, kind of a mix of what I read so far: Let's >>>> refcount >>>> only the dynamically allocated part of AVChannelLayout. Something like: >>>> >>>> typedef struct AVChannelLayout { >>>> enum AVChannelOrder order; >>>> int nb_channels; >>>> uint64_t mask; >>>> AVBufferRef *custom; >>>> } AVChannelLayout; >>>> >>>> And the reference counted data could point to an array of >>>> AVChannelCustom >>>> entries. And AVChannelCustom can have AVDictionary *metadata, because >>>> it >>>> is refcounted. >>> >>> AVBufferRef is meant to hold flat arrays, though. >> >> Since sizeof(AVChannelCustom) is fixed, I don't really see the difference. > > A flat array of bytes. If you do a copy of the contents of the buffer > (av_buffer_make_writable), and there's a pointer in it, you're copying it but > not what it points to. The copy is not a reference, so when the last actual > reference is freed, the custom free() function is called, it frees the > dictionary, and the copy now has a dangling pointer to a non existent > dictionary. OK, I understand. Wrapped avframe misuses AVBufferRef similarly however, and that also should be fixed then sometime... Maybe with an optional copy() function for AVBuffer? >>> And what about the buffer being writable or not? You need to consider >>> both copy and ref scenarios. >> >> av_channel_layout_copy() can simply ref. If a deep copy is needed because >> the user wants to change anything in AVChannelCustom, then helper function >> can be added. >> >>> >>> It's a lot of added complexity for what should be a simple "this stream >>> with six channels has the channels arranged like this in your room: This >>> is front left, this is front right, this is 'under the carpet', etc". >> >> See the attached proof-of-concept patch, to be used on top of your >> channel_layout branch. Is it that bad? > > It's fragile by misusing the AVBufferRef API. Also, why remove the pointer > from the union? Because you may want additional metadata without having a custom channel layout. > > I'll send a version with a flat name array plus an opaque pointer. Then an > email listing all the proposed solutions, to be discussed in a call where we > hopefully reach an agreement. OK. Thanks, Marton
On 12/15/2021 7:55 PM, Marton Balint wrote: > > > On Wed, 15 Dec 2021, James Almer wrote: > >> >> >> On 12/15/2021 6:32 PM, Marton Balint wrote: >>> >>> >>> On Wed, 15 Dec 2021, James Almer wrote: >>> >>>> >>>> >>>> On 12/15/2021 7:24 AM, Marton Balint wrote: >>>>> >>>>> >>>>> On Tue, 14 Dec 2021, James Almer wrote: >>>>> >>>>>> On 12/14/2021 3:54 PM, Nicolas George wrote: >>>>>>> James Almer (12021-12-14): >>>>>>>> We add a const uint8_t* field to AVChannelCustom. If the user >>>>>>>> wants >>>>>>>> to >>>>>>>> allocate the strings instead, and not worry about their >>>>>>>> lifetime, >>>>>>>> they >>>>>>>> can >>>>>>>> provide the layout with a custom free() function that will take >>>>>>>> care >>>>>>>> of >>>>>>>> cleaning anything they came up with on uninit(). >>>>>>> >>>>>>> I understood what you suggested, and it amounts to letting API >>>>>>> users >>>>>>> fend for themselves. We can do that, but I would prefer if we >>>>>>> only >>>>>>> did >>>>>>> on last resort, if we do not find a more convenient solution. >>>>>> >>>>>> There's "char name[16]". Bigger than a pointer (Could be 8 bytes >>>>>> instead, >>>>>> but then it's kinda small). The user will not have to worry >>>>>> about the >>>>>> lifetime of anything then. >>>>>> >>>>>> I'm attaching a diff that goes on top of the last patch of this set >>>>>> implementing this. It also adds support for these custom names to >>>>>> av_channel_layout_describe(), av_channel_layout_index_from_string() >>>>>> and >>>>>> av_channel_layout_channel_from_string(), including tests. >>>>> >>>>> I'd rather not mix custom labels with our fixed names for channels. >>>>> E.g. >>>>> what if a label conflicts with our existing channel names? If the >>>>> user >>>>> wants to specify a channel based on its label, that should be a >>>>> separate >>>>> syntax IMHO. >>>>> >>>>> Overall, having a char name[16] is still kind of limiting, e.g. a >>>>> label >>>>> read from a muxed file will be truncated, I'd rather not have >>>>> anything. >>>> >>>> Container metadata is typically be exported as stream metadata or side >>>> data. >>> >>> That is always a possibility, but if you want some data per-channel, >>> and >>> not per-stream, (e.g. variable size labels) then side data becomes >>> difficult to work with. >>> >>>> >>>>> >>>>> Here is one more idea, kind of a mix of what I read so far: Let's >>>>> refcount >>>>> only the dynamically allocated part of AVChannelLayout. Something >>>>> like: >>>>> >>>>> typedef struct AVChannelLayout { >>>>> enum AVChannelOrder order; >>>>> int nb_channels; >>>>> uint64_t mask; >>>>> AVBufferRef *custom; >>>>> } AVChannelLayout; >>>>> >>>>> And the reference counted data could point to an array of >>>>> AVChannelCustom >>>>> entries. And AVChannelCustom can have AVDictionary *metadata, >>>>> because >>>>> it >>>>> is refcounted. >>>> >>>> AVBufferRef is meant to hold flat arrays, though. >>> >>> Since sizeof(AVChannelCustom) is fixed, I don't really see the >>> difference. >> >> A flat array of bytes. If you do a copy of the contents of the buffer >> (av_buffer_make_writable), and there's a pointer in it, you're copying >> it but not what it points to. The copy is not a reference, so when the >> last actual reference is freed, the custom free() function is called, >> it frees the dictionary, and the copy now has a dangling pointer to a >> non existent dictionary. > > OK, I understand. Wrapped avframe misuses AVBufferRef similarly however, > and that also should be fixed then sometime... Maybe with an optional > copy() function for AVBuffer? Wrapped avframe is an abomination i tried to fix but my patch was rejected. I also tried to add a copy() callback to AVBufferRef some time ago, and it was also rejected, as the API was meant to be for flat arrays and it simply became the defactor refcounter because there was nothing else. So other uses should be done with a different, more adequate and extensible kind of refcounting API. Anton and Nicolas both suggested two different approaches for it. > >>>> And what about the buffer being writable or not? You need to consider >>>> both copy and ref scenarios. >>> >>> av_channel_layout_copy() can simply ref. If a deep copy is needed >>> because >>> the user wants to change anything in AVChannelCustom, then helper >>> function >>> can be added. >>> >>>> >>>> It's a lot of added complexity for what should be a simple "this >>>> stream >>>> with six channels has the channels arranged like this in your room: >>>> This >>>> is front left, this is front right, this is 'under the carpet', etc". >>> >>> See the attached proof-of-concept patch, to be used on top of your >>> channel_layout branch. Is it that bad? >> >> It's fragile by misusing the AVBufferRef API. Also, why remove the >> pointer from the union? > > Because you may want additional metadata without having a custom channel > layout. Then it's no longer a struct meant for the Custom order. If you allow using it for native, unspec or ambisonic, what will the id field represent? If you say "make it be undefined", then it becomes wasted space. > >> >> I'll send a version with a flat name array plus an opaque pointer. >> Then an email listing all the proposed solutions, to be discussed in a >> call where we hopefully reach an agreement. > > OK. > > Thanks, > Marton > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
diff --git a/libavutil/channel_layout.c b/libavutil/channel_layout.c index ac773a9e63..984255bfca 100644 --- a/libavutil/channel_layout.c +++ b/libavutil/channel_layout.c @@ -37,81 +37,132 @@ struct channel_name { }; static const struct channel_name channel_names[] = { - [0] = { "FL", "front left" }, - [1] = { "FR", "front right" }, - [2] = { "FC", "front center" }, - [3] = { "LFE", "low frequency" }, - [4] = { "BL", "back left" }, - [5] = { "BR", "back right" }, - [6] = { "FLC", "front left-of-center" }, - [7] = { "FRC", "front right-of-center" }, - [8] = { "BC", "back center" }, - [9] = { "SL", "side left" }, - [10] = { "SR", "side right" }, - [11] = { "TC", "top center" }, - [12] = { "TFL", "top front left" }, - [13] = { "TFC", "top front center" }, - [14] = { "TFR", "top front right" }, - [15] = { "TBL", "top back left" }, - [16] = { "TBC", "top back center" }, - [17] = { "TBR", "top back right" }, - [29] = { "DL", "downmix left" }, - [30] = { "DR", "downmix right" }, - [31] = { "WL", "wide left" }, - [32] = { "WR", "wide right" }, - [33] = { "SDL", "surround direct left" }, - [34] = { "SDR", "surround direct right" }, - [35] = { "LFE2", "low frequency 2" }, - [36] = { "TSL", "top side left" }, - [37] = { "TSR", "top side right" }, - [38] = { "BFC", "bottom front center" }, - [39] = { "BFL", "bottom front left" }, - [40] = { "BFR", "bottom front right" }, + [AV_CHAN_FRONT_LEFT ] = { "FL", "front left" }, + [AV_CHAN_FRONT_RIGHT ] = { "FR", "front right" }, + [AV_CHAN_FRONT_CENTER ] = { "FC", "front center" }, + [AV_CHAN_LOW_FREQUENCY ] = { "LFE", "low frequency" }, + [AV_CHAN_BACK_LEFT ] = { "BL", "back left" }, + [AV_CHAN_BACK_RIGHT ] = { "BR", "back right" }, + [AV_CHAN_FRONT_LEFT_OF_CENTER ] = { "FLC", "front left-of-center" }, + [AV_CHAN_FRONT_RIGHT_OF_CENTER] = { "FRC", "front right-of-center" }, + [AV_CHAN_BACK_CENTER ] = { "BC", "back center" }, + [AV_CHAN_SIDE_LEFT ] = { "SL", "side left" }, + [AV_CHAN_SIDE_RIGHT ] = { "SR", "side right" }, + [AV_CHAN_TOP_CENTER ] = { "TC", "top center" }, + [AV_CHAN_TOP_FRONT_LEFT ] = { "TFL", "top front left" }, + [AV_CHAN_TOP_FRONT_CENTER ] = { "TFC", "top front center" }, + [AV_CHAN_TOP_FRONT_RIGHT ] = { "TFR", "top front right" }, + [AV_CHAN_TOP_BACK_LEFT ] = { "TBL", "top back left" }, + [AV_CHAN_TOP_BACK_CENTER ] = { "TBC", "top back center" }, + [AV_CHAN_TOP_BACK_RIGHT ] = { "TBR", "top back right" }, + [AV_CHAN_STEREO_LEFT ] = { "DL", "downmix left" }, + [AV_CHAN_STEREO_RIGHT ] = { "DR", "downmix right" }, + [AV_CHAN_WIDE_LEFT ] = { "WL", "wide left" }, + [AV_CHAN_WIDE_RIGHT ] = { "WR", "wide right" }, + [AV_CHAN_SURROUND_DIRECT_LEFT ] = { "SDL", "surround direct left" }, + [AV_CHAN_SURROUND_DIRECT_RIGHT] = { "SDR", "surround direct right" }, + [AV_CHAN_LOW_FREQUENCY_2 ] = { "LFE2", "low frequency 2" }, + [AV_CHAN_TOP_SIDE_LEFT ] = { "TSL", "top side left" }, + [AV_CHAN_TOP_SIDE_RIGHT ] = { "TSR", "top side right" }, + [AV_CHAN_BOTTOM_FRONT_CENTER ] = { "BFC", "bottom front center" }, + [AV_CHAN_BOTTOM_FRONT_LEFT ] = { "BFL", "bottom front left" }, + [AV_CHAN_BOTTOM_FRONT_RIGHT ] = { "BFR", "bottom front right" }, }; -static const char *get_channel_name(int channel_id) +static const char *get_channel_name(enum AVChannel channel_id) { - if (channel_id < 0 || channel_id >= FF_ARRAY_ELEMS(channel_names)) - return NULL; + if ((unsigned) channel_id >= FF_ARRAY_ELEMS(channel_names) || + !channel_names[channel_id].name) + return "?"; return channel_names[channel_id].name; } -static const struct { +static inline void get_channel_str(AVBPrint *bp, const char *str, + enum AVChannel channel_id) +{ + if (str) + av_bprintf(bp, "%s", str); + else + av_bprintf(bp, "?"); +} + +int av_channel_name(char *buf, size_t buf_size, enum AVChannel channel_id) +{ + AVBPrint bp; + + if (!buf && buf_size) + return AVERROR(EINVAL); + + av_bprint_init_for_buffer(&bp, buf, buf_size); + get_channel_str(&bp, (unsigned)channel_id < FF_ARRAY_ELEMS(channel_names) ? + channel_names[channel_id].name : NULL, channel_id); + + return bp.len; +} +int av_channel_description(char *buf, size_t buf_size, enum AVChannel channel_id) +{ + AVBPrint bp; + + if (!buf && buf_size) + return AVERROR(EINVAL); + + av_bprint_init_for_buffer(&bp, buf, buf_size); + get_channel_str(&bp, (unsigned)channel_id < FF_ARRAY_ELEMS(channel_names) ? + channel_names[channel_id].description : NULL, channel_id); + + return bp.len; +} + +enum AVChannel av_channel_from_string(const char *str) +{ + int i; + for (i = 0; i < FF_ARRAY_ELEMS(channel_names); i++) { + if (channel_names[i].name && !strcmp(str, channel_names[i].name)) + return i; + } + return AV_CHAN_NONE; +} + +struct channel_layout_name { const char *name; - int nb_channels; - uint64_t layout; -} channel_layout_map[] = { - { "mono", 1, AV_CH_LAYOUT_MONO }, - { "stereo", 2, AV_CH_LAYOUT_STEREO }, - { "2.1", 3, AV_CH_LAYOUT_2POINT1 }, - { "3.0", 3, AV_CH_LAYOUT_SURROUND }, - { "3.0(back)", 3, AV_CH_LAYOUT_2_1 }, - { "4.0", 4, AV_CH_LAYOUT_4POINT0 }, - { "quad", 4, AV_CH_LAYOUT_QUAD }, - { "quad(side)", 4, AV_CH_LAYOUT_2_2 }, - { "3.1", 4, AV_CH_LAYOUT_3POINT1 }, - { "5.0", 5, AV_CH_LAYOUT_5POINT0_BACK }, - { "5.0(side)", 5, AV_CH_LAYOUT_5POINT0 }, - { "4.1", 5, AV_CH_LAYOUT_4POINT1 }, - { "5.1", 6, AV_CH_LAYOUT_5POINT1_BACK }, - { "5.1(side)", 6, AV_CH_LAYOUT_5POINT1 }, - { "6.0", 6, AV_CH_LAYOUT_6POINT0 }, - { "6.0(front)", 6, AV_CH_LAYOUT_6POINT0_FRONT }, - { "hexagonal", 6, AV_CH_LAYOUT_HEXAGONAL }, - { "6.1", 7, AV_CH_LAYOUT_6POINT1 }, - { "6.1(back)", 7, AV_CH_LAYOUT_6POINT1_BACK }, - { "6.1(front)", 7, AV_CH_LAYOUT_6POINT1_FRONT }, - { "7.0", 7, AV_CH_LAYOUT_7POINT0 }, - { "7.0(front)", 7, AV_CH_LAYOUT_7POINT0_FRONT }, - { "7.1", 8, AV_CH_LAYOUT_7POINT1 }, - { "7.1(wide)", 8, AV_CH_LAYOUT_7POINT1_WIDE_BACK }, - { "7.1(wide-side)", 8, AV_CH_LAYOUT_7POINT1_WIDE }, - { "octagonal", 8, AV_CH_LAYOUT_OCTAGONAL }, - { "hexadecagonal", 16, AV_CH_LAYOUT_HEXADECAGONAL }, - { "downmix", 2, AV_CH_LAYOUT_STEREO_DOWNMIX, }, - { "22.2", 24, AV_CH_LAYOUT_22POINT2, }, + AVChannelLayout layout; }; +static const struct channel_layout_name channel_layout_map[] = { + { "mono", AV_CHANNEL_LAYOUT_MONO }, + { "stereo", AV_CHANNEL_LAYOUT_STEREO }, + { "stereo", AV_CHANNEL_LAYOUT_STEREO_DOWNMIX }, + { "2.1", AV_CHANNEL_LAYOUT_2POINT1 }, + { "3.0", AV_CHANNEL_LAYOUT_SURROUND }, + { "3.0(back)", AV_CHANNEL_LAYOUT_2_1 }, + { "4.0", AV_CHANNEL_LAYOUT_4POINT0 }, + { "quad", AV_CHANNEL_LAYOUT_QUAD }, + { "quad(side)", AV_CHANNEL_LAYOUT_2_2 }, + { "3.1", AV_CHANNEL_LAYOUT_3POINT1 }, + { "5.0", AV_CHANNEL_LAYOUT_5POINT0_BACK }, + { "5.0(side)", AV_CHANNEL_LAYOUT_5POINT0 }, + { "4.1", AV_CHANNEL_LAYOUT_4POINT1 }, + { "5.1", AV_CHANNEL_LAYOUT_5POINT1_BACK }, + { "5.1(side)", AV_CHANNEL_LAYOUT_5POINT1 }, + { "6.0", AV_CHANNEL_LAYOUT_6POINT0 }, + { "6.0(front)", AV_CHANNEL_LAYOUT_6POINT0_FRONT }, + { "hexagonal", AV_CHANNEL_LAYOUT_HEXAGONAL }, + { "6.1", AV_CHANNEL_LAYOUT_6POINT1 }, + { "6.1(back)", AV_CHANNEL_LAYOUT_6POINT1_BACK }, + { "6.1(front)", AV_CHANNEL_LAYOUT_6POINT1_FRONT }, + { "7.0", AV_CHANNEL_LAYOUT_7POINT0 }, + { "7.0(front)", AV_CHANNEL_LAYOUT_7POINT0_FRONT }, + { "7.1", AV_CHANNEL_LAYOUT_7POINT1 }, + { "7.1(wide)", AV_CHANNEL_LAYOUT_7POINT1_WIDE_BACK }, + { "7.1(wide-side)", AV_CHANNEL_LAYOUT_7POINT1_WIDE }, + { "octagonal", AV_CHANNEL_LAYOUT_OCTAGONAL }, + { "hexadecagonal", AV_CHANNEL_LAYOUT_HEXADECAGONAL }, + { "downmix", AV_CHANNEL_LAYOUT_STEREO_DOWNMIX, }, + { "22.2", AV_CHANNEL_LAYOUT_22POINT2, }, +}; + +#if FF_API_OLD_CHANNEL_LAYOUT +FF_DISABLE_DEPRECATION_WARNINGS static uint64_t get_channel_layout_single(const char *name, int name_len) { int i; @@ -121,7 +172,7 @@ static uint64_t get_channel_layout_single(const char *name, int name_len) for (i = 0; i < FF_ARRAY_ELEMS(channel_layout_map); i++) { if (strlen(channel_layout_map[i].name) == name_len && !memcmp(channel_layout_map[i].name, name, name_len)) - return channel_layout_map[i].layout; + return channel_layout_map[i].layout.u.mask; } for (i = 0; i < FF_ARRAY_ELEMS(channel_names); i++) if (channel_names[i].name && @@ -189,8 +240,8 @@ void av_bprint_channel_layout(struct AVBPrint *bp, nb_channels = av_get_channel_layout_nb_channels(channel_layout); for (i = 0; i < FF_ARRAY_ELEMS(channel_layout_map); i++) - if (nb_channels == channel_layout_map[i].nb_channels && - channel_layout == channel_layout_map[i].layout) { + if (nb_channels == channel_layout_map[i].layout.nb_channels && + channel_layout == channel_layout_map[i].layout.u.mask) { av_bprintf(bp, "%s", channel_layout_map[i].name); return; } @@ -231,8 +282,8 @@ int av_get_channel_layout_nb_channels(uint64_t channel_layout) int64_t av_get_default_channel_layout(int nb_channels) { int i; for (i = 0; i < FF_ARRAY_ELEMS(channel_layout_map); i++) - if (nb_channels == channel_layout_map[i].nb_channels) - return channel_layout_map[i].layout; + if (nb_channels == channel_layout_map[i].layout.nb_channels) + return channel_layout_map[i].layout.u.mask; return 0; } @@ -287,7 +338,310 @@ int av_get_standard_channel_layout(unsigned index, uint64_t *layout, { if (index >= FF_ARRAY_ELEMS(channel_layout_map)) return AVERROR_EOF; - if (layout) *layout = channel_layout_map[index].layout; + if (layout) *layout = channel_layout_map[index].layout.u.mask; if (name) *name = channel_layout_map[index].name; return 0; } +FF_ENABLE_DEPRECATION_WARNINGS +#endif + +int av_channel_layout_from_mask(AVChannelLayout *channel_layout, + uint64_t mask) +{ + if (!mask) + return AVERROR(EINVAL); + + channel_layout->order = AV_CHANNEL_ORDER_NATIVE; + channel_layout->nb_channels = av_popcount64(mask); + channel_layout->u.mask = mask; + + return 0; +} + +int av_channel_layout_from_string(AVChannelLayout *channel_layout, + const char *str) +{ + int i, channels; + const char *dup = str; + char *end; + uint64_t mask = 0; + + /* channel layout names */ + for (i = 0; i < FF_ARRAY_ELEMS(channel_layout_map); i++) { + if (channel_layout_map[i].name && !strcmp(str, channel_layout_map[i].name)) { + *channel_layout = channel_layout_map[i].layout; + return 0; + } + } + + /* channel names */ + while (*dup) { + char *chname = av_get_token(&dup, "|"); + if (!chname) + return AVERROR(ENOMEM); + if (*dup) + dup++; // skip separator + for (i = 0; i < FF_ARRAY_ELEMS(channel_names); i++) { + if (channel_names[i].name && !strcmp(chname, channel_names[i].name)) { + mask |= 1ULL << i; + } + } + av_free(chname); + } + if (mask) { + av_channel_layout_from_mask(channel_layout, mask); + return 0; + } + + /* channel layout mask */ + if (!strncmp(str, "0x", 2) && sscanf(str + 2, "%"SCNx64, &mask) == 1) { + av_channel_layout_from_mask(channel_layout, mask); + return 0; + } + + errno = 0; + channels = strtol(str, &end, 10); + + /* number of channels */ + if (!errno && *end == 'c' && !*(end + 1) && channels >= 0) { + av_channel_layout_default(channel_layout, channels); + return 0; + } + + /* number of unordered channels */ + if (!errno && (!*end || av_strnstr(str, "channels", strlen(str))) && channels >= 0) { + channel_layout->order = AV_CHANNEL_ORDER_UNSPEC; + channel_layout->nb_channels = channels; + return 0; + } + + return AVERROR_INVALIDDATA; +} + +void av_channel_layout_uninit(AVChannelLayout *channel_layout) +{ + if (channel_layout->order == AV_CHANNEL_ORDER_CUSTOM) + av_freep(&channel_layout->u.map); + memset(channel_layout, 0, sizeof(*channel_layout)); +} + +int av_channel_layout_copy(AVChannelLayout *dst, const AVChannelLayout *src) +{ + av_channel_layout_uninit(dst); + *dst = *src; + if (src->order == AV_CHANNEL_ORDER_CUSTOM) { + dst->u.map = av_malloc(src->nb_channels * sizeof(*dst->u.map)); + if (!dst->u.map) + return AVERROR(ENOMEM); + memcpy(dst->u.map, src->u.map, src->nb_channels * sizeof(*src->u.map)); + } + return 0; +} + +int av_channel_layout_describe(const AVChannelLayout *channel_layout, + char *buf, size_t buf_size) +{ + AVBPrint bp; + int i; + + if (!buf && buf_size) + return AVERROR(EINVAL); + + av_bprint_init_for_buffer(&bp, buf, buf_size); + + switch (channel_layout->order) { + case AV_CHANNEL_ORDER_NATIVE: + for (i = 0; i < FF_ARRAY_ELEMS(channel_layout_map); i++) + if (channel_layout->u.mask == channel_layout_map[i].layout.u.mask) { + av_bprintf(&bp, "%s", channel_layout_map[i].name); + return bp.len; + } + // fall-through + case AV_CHANNEL_ORDER_CUSTOM: + for (i = 0; i < channel_layout->nb_channels; i++) { + enum AVChannel ch = av_channel_layout_channel_from_index(channel_layout, i); + const char *ch_name = get_channel_name(ch); + + if (i) + av_bprintf(&bp, "|"); + av_bprintf(&bp, "%s", ch_name); + } + if (channel_layout->nb_channels) + return bp.len; + // fall-through + case AV_CHANNEL_ORDER_UNSPEC: + av_bprintf(&bp, "%d channels", channel_layout->nb_channels); + return bp.len; + default: + return AVERROR(EINVAL); + } +} + +enum AVChannel +av_channel_layout_channel_from_index(const AVChannelLayout *channel_layout, + unsigned int idx) +{ + int i; + + if (idx >= channel_layout->nb_channels) + return AV_CHAN_NONE; + + switch (channel_layout->order) { + case AV_CHANNEL_ORDER_CUSTOM: + return channel_layout->u.map[idx].id; + case AV_CHANNEL_ORDER_NATIVE: + for (i = 0; i < 64; i++) { + if ((1ULL << i) & channel_layout->u.mask && !idx--) + return i; + } + default: + return AV_CHAN_NONE; + } +} + +enum AVChannel +av_channel_layout_channel_from_string(const AVChannelLayout *channel_layout, + const char *name) +{ + enum AVChannel; + int channel, ret; + + switch (channel_layout->order) { + case AV_CHANNEL_ORDER_CUSTOM: + case AV_CHANNEL_ORDER_NATIVE: + channel = av_channel_from_string(name); + if (channel < 0) + return channel; + ret = av_channel_layout_index_from_channel(channel_layout, channel); + if (ret < 0) + return ret; + return channel; + } + + return AV_CHAN_NONE; +} + +int av_channel_layout_index_from_channel(const AVChannelLayout *channel_layout, + enum AVChannel channel) +{ + int i; + + switch (channel_layout->order) { + case AV_CHANNEL_ORDER_CUSTOM: + for (i = 0; i < channel_layout->nb_channels; i++) + if (channel_layout->u.map[i].id == channel) + return i; + return AVERROR(EINVAL); + case AV_CHANNEL_ORDER_NATIVE: { + uint64_t mask = channel_layout->u.mask; + if (!(mask & (1ULL << channel))) + return AVERROR(EINVAL); + mask &= (1ULL << channel) - 1; + return av_popcount64(mask); + } + default: + return AVERROR(EINVAL); + } +} + +int av_channel_layout_index_from_string(const AVChannelLayout *channel_layout, + const char *name) +{ + int ret; + + if (channel_layout->order == AV_CHANNEL_ORDER_UNSPEC) + return AVERROR(EINVAL); + + ret = av_channel_from_string(name); + if (ret < 0) + return ret; + return av_channel_layout_index_from_channel(channel_layout, ret); +} + +int av_channel_layout_check(const AVChannelLayout *channel_layout) +{ + if (channel_layout->nb_channels <= 0) + return 0; + + switch (channel_layout->order) { + case AV_CHANNEL_ORDER_NATIVE: + return av_popcount64(channel_layout->u.mask) == channel_layout->nb_channels; + case AV_CHANNEL_ORDER_CUSTOM: + return !!channel_layout->u.map; + case AV_CHANNEL_ORDER_UNSPEC: + return 1; + default: + return 0; + } +} + +int av_channel_layout_compare(const AVChannelLayout *chl, const AVChannelLayout *chl1) +{ + int i; + + /* different channel counts -> not equal */ + if (chl->nb_channels != chl1->nb_channels) + return 1; + + /* if only one is unspecified -> not equal */ + if ((chl->order == AV_CHANNEL_ORDER_UNSPEC) != + (chl1->order == AV_CHANNEL_ORDER_UNSPEC)) + return 1; + /* both are unspecified -> equal */ + else if (chl->order == AV_CHANNEL_ORDER_UNSPEC) + return 0; + + /* can compare masks directly */ + if (chl->order != AV_CHANNEL_ORDER_CUSTOM && + chl->order == chl1->order) + return chl->u.mask != chl1->u.mask; + + /* compare channel by channel */ + for (i = 0; i < chl->nb_channels; i++) + if (av_channel_layout_channel_from_index(chl, i) != + av_channel_layout_channel_from_index(chl1, i)) + return 1; + return 0; +} + +void av_channel_layout_default(AVChannelLayout *ch_layout, int nb_channels) +{ + int i; + for (i = 0; i < FF_ARRAY_ELEMS(channel_layout_map); i++) + if (nb_channels == channel_layout_map[i].layout.nb_channels) { + *ch_layout = channel_layout_map[i].layout; + return; + } + + ch_layout->order = AV_CHANNEL_ORDER_UNSPEC; + ch_layout->nb_channels = nb_channels; +} + +const AVChannelLayout *av_channel_layout_standard(void **opaque) +{ + uintptr_t i = (uintptr_t)*opaque; + const AVChannelLayout *ch_layout = NULL; + + if (i < FF_ARRAY_ELEMS(channel_layout_map)) { + ch_layout = &channel_layout_map[i].layout; + *opaque = (void*)(i + 1); + } + + return ch_layout; +} + +uint64_t av_channel_layout_subset(const AVChannelLayout *channel_layout, + uint64_t mask) +{ + uint64_t ret = 0; + int i; + + if (channel_layout->order == AV_CHANNEL_ORDER_NATIVE) + return channel_layout->u.mask & mask; + + for (i = 0; i < 64; i++) + if (mask & (1ULL << i) && av_channel_layout_index_from_channel(channel_layout, i) >= 0) + ret |= (1ULL << i); + + return ret; +} diff --git a/libavutil/channel_layout.h b/libavutil/channel_layout.h index d39ae1177a..018e87ff0b 100644 --- a/libavutil/channel_layout.h +++ b/libavutil/channel_layout.h @@ -23,6 +23,10 @@ #define AVUTIL_CHANNEL_LAYOUT_H #include <stdint.h> +#include <stdlib.h> + +#include "version.h" +#include "attributes.h" /** * @file @@ -34,6 +38,68 @@ * @{ */ +enum AVChannel { + ///< Invalid channel index + AV_CHAN_NONE = -1, + AV_CHAN_FRONT_LEFT, + AV_CHAN_FRONT_RIGHT, + AV_CHAN_FRONT_CENTER, + AV_CHAN_LOW_FREQUENCY, + AV_CHAN_BACK_LEFT, + AV_CHAN_BACK_RIGHT, + AV_CHAN_FRONT_LEFT_OF_CENTER, + AV_CHAN_FRONT_RIGHT_OF_CENTER, + AV_CHAN_BACK_CENTER, + AV_CHAN_SIDE_LEFT, + AV_CHAN_SIDE_RIGHT, + AV_CHAN_TOP_CENTER, + AV_CHAN_TOP_FRONT_LEFT, + AV_CHAN_TOP_FRONT_CENTER, + AV_CHAN_TOP_FRONT_RIGHT, + AV_CHAN_TOP_BACK_LEFT, + AV_CHAN_TOP_BACK_CENTER, + AV_CHAN_TOP_BACK_RIGHT, + /** Stereo downmix. */ + AV_CHAN_STEREO_LEFT = 29, + /** See above. */ + AV_CHAN_STEREO_RIGHT, + AV_CHAN_WIDE_LEFT, + AV_CHAN_WIDE_RIGHT, + AV_CHAN_SURROUND_DIRECT_LEFT, + AV_CHAN_SURROUND_DIRECT_RIGHT, + AV_CHAN_LOW_FREQUENCY_2, + AV_CHAN_TOP_SIDE_LEFT, + AV_CHAN_TOP_SIDE_RIGHT, + AV_CHAN_BOTTOM_FRONT_CENTER, + AV_CHAN_BOTTOM_FRONT_LEFT, + AV_CHAN_BOTTOM_FRONT_RIGHT, + + /** Channel is empty can be safely skipped. */ + AV_CHAN_SILENCE = 64, +}; + +enum AVChannelOrder { + /** + * Only the channel count is specified, without any further information + * about the channel order. + */ + AV_CHANNEL_ORDER_UNSPEC, + /** + * 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 empty/skipped (AV_CHAN_SILENCE) + * channels at arbitrary positions. + */ + AV_CHANNEL_ORDER_CUSTOM, +}; + + /** * @defgroup channel_masks Audio channel masks * @@ -46,41 +112,46 @@ * * @{ */ -#define AV_CH_FRONT_LEFT 0x00000001 -#define AV_CH_FRONT_RIGHT 0x00000002 -#define AV_CH_FRONT_CENTER 0x00000004 -#define AV_CH_LOW_FREQUENCY 0x00000008 -#define AV_CH_BACK_LEFT 0x00000010 -#define AV_CH_BACK_RIGHT 0x00000020 -#define AV_CH_FRONT_LEFT_OF_CENTER 0x00000040 -#define AV_CH_FRONT_RIGHT_OF_CENTER 0x00000080 -#define AV_CH_BACK_CENTER 0x00000100 -#define AV_CH_SIDE_LEFT 0x00000200 -#define AV_CH_SIDE_RIGHT 0x00000400 -#define AV_CH_TOP_CENTER 0x00000800 -#define AV_CH_TOP_FRONT_LEFT 0x00001000 -#define AV_CH_TOP_FRONT_CENTER 0x00002000 -#define AV_CH_TOP_FRONT_RIGHT 0x00004000 -#define AV_CH_TOP_BACK_LEFT 0x00008000 -#define AV_CH_TOP_BACK_CENTER 0x00010000 -#define AV_CH_TOP_BACK_RIGHT 0x00020000 -#define AV_CH_STEREO_LEFT 0x20000000 ///< Stereo downmix. -#define AV_CH_STEREO_RIGHT 0x40000000 ///< See AV_CH_STEREO_LEFT. -#define AV_CH_WIDE_LEFT 0x0000000080000000ULL -#define AV_CH_WIDE_RIGHT 0x0000000100000000ULL -#define AV_CH_SURROUND_DIRECT_LEFT 0x0000000200000000ULL -#define AV_CH_SURROUND_DIRECT_RIGHT 0x0000000400000000ULL -#define AV_CH_LOW_FREQUENCY_2 0x0000000800000000ULL -#define AV_CH_TOP_SIDE_LEFT 0x0000001000000000ULL -#define AV_CH_TOP_SIDE_RIGHT 0x0000002000000000ULL -#define AV_CH_BOTTOM_FRONT_CENTER 0x0000004000000000ULL -#define AV_CH_BOTTOM_FRONT_LEFT 0x0000008000000000ULL -#define AV_CH_BOTTOM_FRONT_RIGHT 0x0000010000000000ULL +#define AV_CH_FRONT_LEFT (1ULL << AV_CHAN_FRONT_LEFT ) +#define AV_CH_FRONT_RIGHT (1ULL << AV_CHAN_FRONT_RIGHT ) +#define AV_CH_FRONT_CENTER (1ULL << AV_CHAN_FRONT_CENTER ) +#define AV_CH_LOW_FREQUENCY (1ULL << AV_CHAN_LOW_FREQUENCY ) +#define AV_CH_BACK_LEFT (1ULL << AV_CHAN_BACK_LEFT ) +#define AV_CH_BACK_RIGHT (1ULL << AV_CHAN_BACK_RIGHT ) +#define AV_CH_FRONT_LEFT_OF_CENTER (1ULL << AV_CHAN_FRONT_LEFT_OF_CENTER ) +#define AV_CH_FRONT_RIGHT_OF_CENTER (1ULL << AV_CHAN_FRONT_RIGHT_OF_CENTER) +#define AV_CH_BACK_CENTER (1ULL << AV_CHAN_BACK_CENTER ) +#define AV_CH_SIDE_LEFT (1ULL << AV_CHAN_SIDE_LEFT ) +#define AV_CH_SIDE_RIGHT (1ULL << AV_CHAN_SIDE_RIGHT ) +#define AV_CH_TOP_CENTER (1ULL << AV_CHAN_TOP_CENTER ) +#define AV_CH_TOP_FRONT_LEFT (1ULL << AV_CHAN_TOP_FRONT_LEFT ) +#define AV_CH_TOP_FRONT_CENTER (1ULL << AV_CHAN_TOP_FRONT_CENTER ) +#define AV_CH_TOP_FRONT_RIGHT (1ULL << AV_CHAN_TOP_FRONT_RIGHT ) +#define AV_CH_TOP_BACK_LEFT (1ULL << AV_CHAN_TOP_BACK_LEFT ) +#define AV_CH_TOP_BACK_CENTER (1ULL << AV_CHAN_TOP_BACK_CENTER ) +#define AV_CH_TOP_BACK_RIGHT (1ULL << AV_CHAN_TOP_BACK_RIGHT ) +#define AV_CH_STEREO_LEFT (1ULL << AV_CHAN_STEREO_LEFT ) +#define AV_CH_STEREO_RIGHT (1ULL << AV_CHAN_STEREO_RIGHT ) +#define AV_CH_WIDE_LEFT (1ULL << AV_CHAN_WIDE_LEFT ) +#define AV_CH_WIDE_RIGHT (1ULL << AV_CHAN_WIDE_RIGHT ) +#define AV_CH_SURROUND_DIRECT_LEFT (1ULL << AV_CHAN_SURROUND_DIRECT_LEFT ) +#define AV_CH_SURROUND_DIRECT_RIGHT (1ULL << AV_CHAN_SURROUND_DIRECT_RIGHT) +#define AV_CH_LOW_FREQUENCY_2 (1ULL << AV_CHAN_LOW_FREQUENCY_2 ) +#define AV_CH_TOP_SIDE_LEFT (1ULL << AV_CHAN_TOP_SIDE_LEFT ) +#define AV_CH_TOP_SIDE_RIGHT (1ULL << AV_CHAN_TOP_SIDE_RIGHT ) +#define AV_CH_BOTTOM_FRONT_CENTER (1ULL << AV_CHAN_BOTTOM_FRONT_CENTER ) +#define AV_CH_BOTTOM_FRONT_LEFT (1ULL << AV_CHAN_BOTTOM_FRONT_LEFT ) +#define AV_CH_BOTTOM_FRONT_RIGHT (1ULL << AV_CHAN_BOTTOM_FRONT_RIGHT ) +#if FF_API_OLD_CHANNEL_LAYOUT /** Channel mask value used for AVCodecContext.request_channel_layout to indicate that the user requests the channel order of the decoder output - to be the native codec channel order. */ + to be the native codec channel order. + @deprecated channel order is now indicated in a special field in + AVChannelLayout + */ #define AV_CH_LAYOUT_NATIVE 0x8000000000000000ULL +#endif /** * @} @@ -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; + +/** + * An AVChannelLayout holds information about the channel layout of audio data. + * + * A channel layout here is defined as a set of channels ordered in a specific + * way (unless the channel order is AV_CHANNEL_ORDER_UNSPEC, in which case an + * AVChannelLayout carries only the channel count). + * + * 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 + * or embedded in caller-defined structs. + * + * AVChannelLayout can be initialized as follows: + * - default initialization with {0}, followed by by setting all used fields + * correctly; + * - by assigning one of the predefined AV_CHANNEL_LAYOUT_* initializers; + * - with a constructor function, such as av_channel_layout_default(), + * av_channel_layout_from_mask() or av_channel_layout_from_string(). + * + * The channel layout must be unitialized with av_channel_layout_uninit() + * (strictly speaking this is not necessary for AV_CHANNEL_ORDER_NATIVE and + * AV_CHANNEL_ORDER_UNSPEC, but we recommend always calling + * av_channel_layout_uninit() regardless of the channel order used). + * + * Copying an AVChannelLayout via assigning is forbidden, + * av_channel_layout_copy() must be used. instead (and its return value should + * be checked) + * + * 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). + */ +typedef struct AVChannelLayout { + /** + * Channel order used in this layout. + * This is a mandatory field. + */ + enum AVChannelOrder order; + + /** + * Number of channels in this layout. Mandatory field. + */ + int nb_channels; + + /** + * 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 presence of the AVChannel with the + * corresponding value in map[i].id. + * + * I.e. when map[i].id is equal to AV_CHAN_FOO, then AV_CH_FOO is the + * i-th channel in the audio data. + */ + AVChannelCustom *map; + } u; +} AVChannelLayout; + +#define AV_CHANNEL_LAYOUT_MONO \ + { .order = AV_CHANNEL_ORDER_NATIVE, .nb_channels = 1, .u = { .mask = AV_CH_LAYOUT_MONO }} +#define AV_CHANNEL_LAYOUT_STEREO \ + { .order = AV_CHANNEL_ORDER_NATIVE, .nb_channels = 2, .u = { .mask = AV_CH_LAYOUT_STEREO }} +#define AV_CHANNEL_LAYOUT_2POINT1 \ + { .order = AV_CHANNEL_ORDER_NATIVE, .nb_channels = 3, .u = { .mask = AV_CH_LAYOUT_2POINT1 }} +#define AV_CHANNEL_LAYOUT_2_1 \ + { .order = AV_CHANNEL_ORDER_NATIVE, .nb_channels = 3, .u = { .mask = AV_CH_LAYOUT_2_1 }} +#define AV_CHANNEL_LAYOUT_SURROUND \ + { .order = AV_CHANNEL_ORDER_NATIVE, .nb_channels = 3, .u = { .mask = AV_CH_LAYOUT_SURROUND }} +#define AV_CHANNEL_LAYOUT_3POINT1 \ + { .order = AV_CHANNEL_ORDER_NATIVE, .nb_channels = 4, .u = { .mask = AV_CH_LAYOUT_3POINT1 }} +#define AV_CHANNEL_LAYOUT_4POINT0 \ + { .order = AV_CHANNEL_ORDER_NATIVE, .nb_channels = 4, .u = { .mask = AV_CH_LAYOUT_4POINT0 }} +#define AV_CHANNEL_LAYOUT_4POINT1 \ + { .order = AV_CHANNEL_ORDER_NATIVE, .nb_channels = 5, .u = { .mask = AV_CH_LAYOUT_4POINT1 }} +#define AV_CHANNEL_LAYOUT_2_2 \ + { .order = AV_CHANNEL_ORDER_NATIVE, .nb_channels = 4, .u = { .mask = AV_CH_LAYOUT_2_2 }} +#define AV_CHANNEL_LAYOUT_QUAD \ + { .order = AV_CHANNEL_ORDER_NATIVE, .nb_channels = 4, .u = { .mask = AV_CH_LAYOUT_QUAD }} +#define AV_CHANNEL_LAYOUT_5POINT0 \ + { .order = AV_CHANNEL_ORDER_NATIVE, .nb_channels = 5, .u = { .mask = AV_CH_LAYOUT_5POINT0 }} +#define AV_CHANNEL_LAYOUT_5POINT1 \ + { .order = AV_CHANNEL_ORDER_NATIVE, .nb_channels = 6, .u = { .mask = AV_CH_LAYOUT_5POINT1 }} +#define AV_CHANNEL_LAYOUT_5POINT0_BACK \ + { .order = AV_CHANNEL_ORDER_NATIVE, .nb_channels = 5, .u = { .mask = AV_CH_LAYOUT_5POINT0_BACK }} +#define AV_CHANNEL_LAYOUT_5POINT1_BACK \ + { .order = AV_CHANNEL_ORDER_NATIVE, .nb_channels = 6, .u = { .mask = AV_CH_LAYOUT_5POINT1_BACK }} +#define AV_CHANNEL_LAYOUT_6POINT0 \ + { .order = AV_CHANNEL_ORDER_NATIVE, .nb_channels = 6, .u = { .mask = AV_CH_LAYOUT_6POINT0 }} +#define AV_CHANNEL_LAYOUT_6POINT0_FRONT \ + { .order = AV_CHANNEL_ORDER_NATIVE, .nb_channels = 6, .u = { .mask = AV_CH_LAYOUT_6POINT0_FRONT }} +#define AV_CHANNEL_LAYOUT_HEXAGONAL \ + { .order = AV_CHANNEL_ORDER_NATIVE, .nb_channels = 6, .u = { .mask = AV_CH_LAYOUT_HEXAGONAL }} +#define AV_CHANNEL_LAYOUT_6POINT1 \ + { .order = AV_CHANNEL_ORDER_NATIVE, .nb_channels = 7, .u = { .mask = AV_CH_LAYOUT_6POINT1 }} +#define AV_CHANNEL_LAYOUT_6POINT1_BACK \ + { .order = AV_CHANNEL_ORDER_NATIVE, .nb_channels = 7, .u = { .mask = AV_CH_LAYOUT_6POINT1_BACK }} +#define AV_CHANNEL_LAYOUT_6POINT1_FRONT \ + { .order = AV_CHANNEL_ORDER_NATIVE, .nb_channels = 7, .u = { .mask = AV_CH_LAYOUT_6POINT1_FRONT }} +#define AV_CHANNEL_LAYOUT_7POINT0 \ + { .order = AV_CHANNEL_ORDER_NATIVE, .nb_channels = 7, .u = { .mask = AV_CH_LAYOUT_7POINT0 }} +#define AV_CHANNEL_LAYOUT_7POINT0_FRONT \ + { .order = AV_CHANNEL_ORDER_NATIVE, .nb_channels = 7, .u = { .mask = AV_CH_LAYOUT_7POINT0_FRONT }} +#define AV_CHANNEL_LAYOUT_7POINT1 \ + { .order = AV_CHANNEL_ORDER_NATIVE, .nb_channels = 8, .u = { .mask = AV_CH_LAYOUT_7POINT1 }} +#define AV_CHANNEL_LAYOUT_7POINT1_WIDE \ + { .order = AV_CHANNEL_ORDER_NATIVE, .nb_channels = 8, .u = { .mask = AV_CH_LAYOUT_7POINT1_WIDE }} +#define AV_CHANNEL_LAYOUT_7POINT1_WIDE_BACK \ + { .order = AV_CHANNEL_ORDER_NATIVE, .nb_channels = 8, .u = { .mask = AV_CH_LAYOUT_7POINT1_WIDE_BACK }} +#define AV_CHANNEL_LAYOUT_OCTAGONAL \ + { .order = AV_CHANNEL_ORDER_NATIVE, .nb_channels = 8, .u = { .mask = AV_CH_LAYOUT_OCTAGONAL }} +#define AV_CHANNEL_LAYOUT_HEXADECAGONAL \ + { .order = AV_CHANNEL_ORDER_NATIVE, .nb_channels = 16, .u = { .mask = AV_CH_LAYOUT_HEXAGONAL }} +#define AV_CHANNEL_LAYOUT_STEREO_DOWNMIX \ + { .order = AV_CHANNEL_ORDER_NATIVE, .nb_channels = 2, .u = { .mask = AV_CH_LAYOUT_STEREO_DOWNMIX }} +#define AV_CHANNEL_LAYOUT_22POINT2 \ + { .order = AV_CHANNEL_ORDER_NATIVE, .nb_channels = 24, .u = { .mask = AV_CH_LAYOUT_22POINT2 }} + +#if FF_API_OLD_CHANNEL_LAYOUT /** * Return a channel layout id that matches name, or 0 if no match is found. * @@ -144,7 +366,10 @@ enum AVMatrixEncoding { * AV_CH_* macros). * * Example: "stereo+FC" = "2c+FC" = "2c+1c" = "0x7" + * + * @deprecated use av_channel_layout_from_string() */ +attribute_deprecated uint64_t av_get_channel_layout(const char *name); /** @@ -158,7 +383,9 @@ uint64_t av_get_channel_layout(const char *name); * @param[out] nb_channels number of channels * * @return 0 on success, AVERROR(EINVAL) if the parsing fails. + * @deprecated use av_channel_layout_from_string() */ +attribute_deprecated int av_get_extended_channel_layout(const char *name, uint64_t* channel_layout, int* nb_channels); /** @@ -167,23 +394,32 @@ int av_get_extended_channel_layout(const char *name, uint64_t* channel_layout, i * * @param buf put here the string containing the channel layout * @param buf_size size in bytes of the buffer + * @deprecated use av_channel_layout_describe() */ +attribute_deprecated void av_get_channel_layout_string(char *buf, int buf_size, int nb_channels, uint64_t channel_layout); struct AVBPrint; /** * Append a description of a channel layout to a bprint buffer. + * @deprecated use av_channel_layout_describe() */ +attribute_deprecated void av_bprint_channel_layout(struct AVBPrint *bp, int nb_channels, uint64_t channel_layout); /** * Return the number of channels in the channel layout. + * @deprecated use AVChannelLayout.nb_channels */ +attribute_deprecated int av_get_channel_layout_nb_channels(uint64_t channel_layout); /** * Return default channel layout for a given number of channels. + * + * @deprecated use av_channel_layout_default() */ +attribute_deprecated int64_t av_get_default_channel_layout(int nb_channels); /** @@ -194,20 +430,28 @@ int64_t av_get_default_channel_layout(int nb_channels); * * @return index of channel in channel_layout on success, a negative AVERROR * on error. + * + * @deprecated use av_channel_layout_index_from_channel() */ +attribute_deprecated int av_get_channel_layout_channel_index(uint64_t channel_layout, uint64_t channel); /** * Get the channel with the given index in channel_layout. + * @deprecated use av_channel_layout_channel_from_index() */ +attribute_deprecated uint64_t av_channel_layout_extract_channel(uint64_t channel_layout, int index); /** * Get the name of a given channel. * * @return channel name on success, NULL on error. + * + * @deprecated use av_channel_name() */ +attribute_deprecated const char *av_get_channel_name(uint64_t channel); /** @@ -215,7 +459,9 @@ const char *av_get_channel_name(uint64_t channel); * * @param channel a channel layout with a single channel * @return channel description on success, NULL on error + * @deprecated use av_channel_description() */ +attribute_deprecated const char *av_get_channel_description(uint64_t channel); /** @@ -226,9 +472,211 @@ const char *av_get_channel_description(uint64_t channel); * @param[out] name name of the layout * @return 0 if the layout exists, * <0 if index is beyond the limits + * @deprecated use av_channel_layout_standard() */ +attribute_deprecated int av_get_standard_channel_layout(unsigned index, uint64_t *layout, const char **name); +#endif + +/** + * Get a human readable string in an abbreviated form describing a given channel, + * or "?" if not found. + * This is the inverse function of @ref av_channel_from_string(). + * + * @param buf pre-allocated buffer where to put the generated string + * @param buf_size size in bytes of the buffer. + * @return amount of bytes needed to hold the output string, or a negative AVERROR + * on failure. If the returned value is bigger than buf_size, then the + * string was truncated. + */ +int av_channel_name(char *buf, size_t buf_size, enum AVChannel channel); + +/** + * Get a human readable string describing a given channel, or "?" if not found. + * + * @param buf pre-allocated buffer where to put the generated string + * @param buf_size size in bytes of the buffer. + * @return amount of bytes needed to hold the output string, or a negative AVERROR + * on failure. If the returned value is bigger than buf_size, then the + * string was truncated. + */ +int av_channel_description(char *buf, size_t buf_size, enum AVChannel channel); + +/** + * This is the inverse function of @ref av_channel_name(). + * + * @return the channel with the given name + * AV_CHAN_NONE when name does not identify a known channel + */ +enum AVChannel av_channel_from_string(const char *name); + +/** + * Initialize a native channel layout from a bitmask indicating which channels + * are present. + * + * @param channel_layout the layout structure to be initialized + * @param mask bitmask describing the channel layout + * + * @return 0 on success + * AVERROR(EINVAL) for invalid mask values + */ +int av_channel_layout_from_mask(AVChannelLayout *channel_layout, uint64_t mask); + +/** + * Initialize a channel layout from a given string description. + * The input string can be represented by: + * - the formal channel layout name (returned by av_channel_layout_describe()) + * - single or multiple channel names (returned by av_channel_name() + * or concatenated with "|") + * - a hexadecimal value of a channel layout (eg. "0x4") + * - the number of channels with default layout (eg. "5c") + * - the number of unordered channels (eg. "4" or "4 channels") + * + * @param channel_layout input channel layout + * @param str string describing the channel layout + * @return 0 channel layout was detected, AVERROR_INVALIDATATA otherwise + */ +int av_channel_layout_from_string(AVChannelLayout *channel_layout, + const char *str); + +/** + * Get the default channel layout for a given number of channels. + * + * @param channel_layout the layout structure to be initialized + * @param nb_channels number of channels + */ +void av_channel_layout_default(AVChannelLayout *ch_layout, int nb_channels); + +/** + * Iterate over all standard channel layouts. + * + * @param opaque a pointer where libavutil will store the iteration state. Must + * point to NULL to start the iteration. + * + * @return the standard channel layout or NULL when the iteration is + * finished + */ +const AVChannelLayout *av_channel_layout_standard(void **opaque); + +/** + * Free any allocated data in the channel layout and reset the channel + * count to 0. + * + * @param channel_layout the layout structure to be uninitialized + */ +void av_channel_layout_uninit(AVChannelLayout *channel_layout); + +/** + * Make a copy of a channel layout. This differs from just assigning src to dst + * in that it allocates and copies the map for AV_CHANNEL_ORDER_CUSTOM. + * + * @note the destination channel_layout will be always uninitialized before copy. + * + * @param dst destination channel layout + * @param src source channel layout + * @return 0 on success, a negative AVERROR on error. + */ +int av_channel_layout_copy(AVChannelLayout *dst, const AVChannelLayout *src); + +/** + * Get a human-readable string describing the channel layout properties. + * The string will be in the same format that is accepted by + * @ref av_channel_layout_from_string(). + * + * @param channel_layout channel layout to be described + * @param buf pre-allocated buffer where to put the generated string + * @param buf_size size in bytes of the buffer. + * @return amount of bytes needed to hold the output string, or a negative AVERROR + * on failure. If the returned value is bigger than buf_size, then the + * string was truncated. + */ +int av_channel_layout_describe(const AVChannelLayout *channel_layout, + char *buf, size_t buf_size); + +/** + * Get the channel with the given index in a channel layout. + * + * @param channel_layout input channel layout + * @return channel with the index idx in channel_layout on success or + * AV_CHAN_NONE on failure (if idx is not valid or the channel order is + * unspecified) + */ +enum AVChannel +av_channel_layout_channel_from_index(const AVChannelLayout *channel_layout, unsigned int idx); + +/** + * Get the index of a given channel in a channel layout. In case multiple + * channels are found, only the first match will be returned. + * + * @param channel_layout input channel layout + * @return index of channel in channel_layout on success or a negative number if + * channel is not present in channel_layout. + */ +int av_channel_layout_index_from_channel(const AVChannelLayout *channel_layout, + enum AVChannel channel); + +/** + * Get the index in a channel layout of a channel described by the given string. + * In case multiple channels are found, only the first match will be returned. + * + * This function accepts channel names in the same format as + * @ref av_channel_from_string(). + * + * @param channel_layout input channel layout + * @return a channel index described by the given string, or a negative AVERROR + * value. + */ +int av_channel_layout_index_from_string(const AVChannelLayout *channel_layout, + const char *name); + +/** + * Get a channel described by the given string. + * + * This function accepts channel names in the same format as + * @ref av_channel_from_string(). + * + * @param channel_layout input channel layout + * @return a channel described by the given string, or a negative AVERROR value. + */ +int av_channel_layout_channel_from_string(const AVChannelLayout *channel_layout, + const char *name); + +/** + * Find out what channels from a given set are present in a channel layout, + * without regard for their positions. + * + * @param channel_layout input channel layout + * @param mask a combination of AV_CH_* representing a set of channels + * @return a bitfield representing all the channels from mask that are present + * in channel_layout + */ +uint64_t av_channel_layout_subset(const AVChannelLayout *channel_layout, + uint64_t mask); + +/** + * Check whether a channel layout is valid, i.e. can possibly describe audio + * data. + * + * @param channel_layout input channel layout + * @return 1 if channel_layout is valid, 0 otherwise. + */ +int av_channel_layout_check(const AVChannelLayout *channel_layout); + +/** + * Check whether two channel layouts are semantically the same, i.e. the same + * channels are present on the same positions in both. + * + * If one of the channel layouts is AV_CHANNEL_ORDER_UNSPEC, while the other is + * not, they are considered to be unequal. If both are AV_CHANNEL_ORDER_UNSPEC, + * they are considered equal iff the channel counts are the same in both. + * + * @param chl input channel layout + * @param chl1 input channel layout + * @return 0 if chl and chl1 are equal, 1 if they are not equal. A negative + * AVERROR code if one or both are invalid. + */ +int av_channel_layout_compare(const AVChannelLayout *chl, const AVChannelLayout *chl1); /** * @} diff --git a/libavutil/version.h b/libavutil/version.h index 017fc277a6..d7179e1fdd 100644 --- a/libavutil/version.h +++ b/libavutil/version.h @@ -109,6 +109,7 @@ #define FF_API_DECLARE_ALIGNED (LIBAVUTIL_VERSION_MAJOR < 58) #define FF_API_COLORSPACE_NAME (LIBAVUTIL_VERSION_MAJOR < 58) #define FF_API_AV_MALLOCZ_ARRAY (LIBAVUTIL_VERSION_MAJOR < 58) +#define FF_API_OLD_CHANNEL_LAYOUT (LIBAVUTIL_VERSION_MAJOR < 58) /** * @}