Re: [FFmpeg-devel] [PATCH] avutil/channel_layout: make pre-defined channel layouts C++ friendly

["Tomas Härdin" <git@haerdin.se>]

mån 2023-08-28 klockan 16:13 +0800 skrev zhilizhao(赵志立):
> 
> 
> > On Aug 20, 2023, at 20:53, Tomas Härdin <git@haerdin.se> wrote:
> > 
> > tor 2023-08-17 klockan 22:03 +0800 skrev zhilizhao(赵志立):
> > > 
> > > 
> > > > On Aug 17, 2023, at 20:57, Tomas Härdin <git@haerdin.se> wrote:
> > > > 
> > > > ons 2023-08-16 klockan 23:44 +0800 skrev Zhao Zhili:
> > > > > From: Zhao Zhili <zhilizhao@tencent.com>
> > > > > 
> > > > > C++ doesn't support designated initializers until C++20. We
> > > > > have
> > > > > a bunch of pre-defined channel layouts, the gains to make
> > > > > them
> > > > > usable in C++ exceed the losses.
> > > > > 
> > > > > Signed-off-by: Zhao Zhili <zhilizhao@tencent.com>
> > > > > ---
> > > > >  libavutil/channel_layout.h | 4 ++--
> > > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/libavutil/channel_layout.h
> > > > > b/libavutil/channel_layout.h
> > > > > index f345415c55..817a5ad370 100644
> > > > > --- a/libavutil/channel_layout.h
> > > > > +++ b/libavutil/channel_layout.h
> > > > > @@ -359,7 +359,7 @@ typedef struct AVChannelLayout {
> > > > >  } AVChannelLayout;
> > > > >  
> > > > >  #define AV_CHANNEL_LAYOUT_MASK(nb, m) \
> > > > > -    { .order = AV_CHANNEL_ORDER_NATIVE, .nb_channels = (nb),
> > > > > .u
> > > > > = {
> > > > > .mask = (m) }}
> > > > > +    { AV_CHANNEL_ORDER_NATIVE, (nb), { m }, NULL }
> > > > >  
> > > > >  /**
> > > > >   * @name Common pre-defined channel layouts
> > > > > @@ -397,7 +397,7 @@ typedef struct AVChannelLayout {
> > > > >  #define AV_CHANNEL_LAYOUT_STEREO_DOWNMIX   
> > > > > AV_CHANNEL_LAYOUT_MASK(2,  AV_CH_LAYOUT_STEREO_DOWNMIX)
> > > > >  #define AV_CHANNEL_LAYOUT_22POINT2         
> > > > > AV_CHANNEL_LAYOUT_MASK(24, AV_CH_LAYOUT_22POINT2)
> > > > >  #define AV_CHANNEL_LAYOUT_AMBISONIC_FIRST_ORDER \
> > > > > -    { .order = AV_CHANNEL_ORDER_AMBISONIC, .nb_channels = 4,
> > > > > .u
> > > > > = {
> > > > > .mask = 0 }}
> > > > > +    { AV_CHANNEL_ORDER_AMBISONIC, 4, { 0 }, NULL }
> > > > 
> > > > For C++ compat you could use some constructor magic instead,
> > > > and
> > > > turn
> > > > these into proper constants. Hidden behind #ifdef __cplusplus
> > > > of
> > > > course.
> > > 
> > > I’m trying to avoid more debate to not refer to __cplusplus on
> > > purpose.
> > > 
> > > > 
> > > > IMO having untyped #defines like this is mega ugly. At the very
> > > > least
> > > > it should be (AVChannelLayout){...}. It's likely cargo culted
> > > > from
> > > > when
> > > > channel layouts were bitmasks.
> > > 
> > > (AVChannelLayout){…} can be invalid in C++. AV_TIME_BASE_Q has
> > > the
> > > problem
> > > already.
> > 
> > We could turn them into proper constants, like so:
> > 
> > static const AVChannelLayout
> > AV_CHANNEL_LAYOUT_AMBISONIC_FIRST_ORDER =
> > {...};
> 
> I like this style, but it’s API, so we can’t just replace current
> implementation by static const variables. And I’m not sure about the
> coding style regarding to use static const variables in public
> headers.  

The only public header with static const outside of comments is mem.h,
and even then they're wrapped in macros (DECLARE_ALIGNED etc)

/Tomas
_______________________________________________
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".