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

From: "Tomas Härdin" <git@haerdin.se>
To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
Subject: Re: [FFmpeg-devel] [PATCH] avutil/channel_layout: make pre-defined channel layouts C++ friendly
Date: Sun, 20 Aug 2023 14:53:57 +0200
Message-ID: <562b641f7acc57140511dd6f4eb315b22bbf29dc.camel@haerdin.se> (raw)
In-Reply-To: <tencent_D915B52C6ADE003569B67CDAAF0A38A5C30A@qq.com>

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 =
{...};

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