diff mbox series

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

Message ID tencent_280E4EA7054B7318E85C93E2537C15DF0905@qq.com
State New
Headers show
Series [FFmpeg-devel] avutil/channel_layout: make pre-defined channel layouts C++ friendly | expand

Checks

Context Check Description
yinshiyou/make_loongarch64 success Make finished
yinshiyou/make_fate_loongarch64 success Make fate finished
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished

Commit Message

Zhao Zhili Aug. 16, 2023, 3:44 p.m. UTC 
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(-)

Comments

Paul B Mahol Aug. 16, 2023, 4 p.m. UTC | #1 
NAK, as full codebase have usages of designated initializers.
James Almer Aug. 16, 2023, 4:21 p.m. UTC | #2 
On 8/16/2023 12:44 PM, Zhao Zhili wrote:
> 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 }

Comment out the field names instead of removing them altogether, and add 
a comment about how this is done for the sake of c++ projects including 
this header.

>   
>   /**
>    * @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 }
>   /** @} */
>   
>   struct AVBPrint;

This needs a minor bump IMO. Something said c++ projects can look for 
before using these defines.
Paul B Mahol Aug. 16, 2023, 4:52 p.m. UTC | #3 
On Wed, Aug 16, 2023 at 6:21 PM James Almer <jamrial@gmail.com> wrote:

> On 8/16/2023 12:44 PM, Zhao Zhili wrote:
> > 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 }
>
> Comment out the field names instead of removing them altogether, and add
> a comment about how this is done for the sake of c++ projects including
> this header.
>
> >
> >   /**
> >    * @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 }
> >   /** @} */
> >
> >   struct AVBPrint;
>
> This needs a minor bump IMO. Something said c++ projects can look for
> before using these defines.
>

OK,  I had enough, I'm leaving FFmpeg once and forever.


> _______________________________________________
> 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".
>
Zhao Zhili Aug. 16, 2023, 5 p.m. UTC | #4 
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of James Almer
> Sent: 2023年8月17日 0:22
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH] avutil/channel_layout: make pre-defined channel layouts C++ friendly
> 
> On 8/16/2023 12:44 PM, Zhao Zhili wrote:
> > 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 }
> 
> Comment out the field names instead of removing them altogether, and add
> a comment about how this is done for the sake of c++ projects including
> this header.

OK. So it won't be reverted by accident.

> 
> >
> >   /**
> >    * @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 }
> >   /** @} */
> >
> >   struct AVBPrint;
> 
> This needs a minor bump IMO. Something said c++ projects can look for
> before using these defines.

Good idea.

> _______________________________________________
> 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 Aug. 16, 2023, 5:04 p.m. UTC | #5 
On 8/16/2023 1:52 PM, Paul B Mahol wrote:
> On Wed, Aug 16, 2023 at 6:21 PM James Almer <jamrial@gmail.com> wrote:
> 
>> On 8/16/2023 12:44 PM, Zhao Zhili wrote:
>>> 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 }
>>
>> Comment out the field names instead of removing them altogether, and add
>> a comment about how this is done for the sake of c++ projects including
>> this header.
>>
>>>
>>>    /**
>>>     * @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 }
>>>    /** @} */
>>>
>>>    struct AVBPrint;
>>
>> This needs a minor bump IMO. Something said c++ projects can look for
>> before using these defines.
>>
> 
> OK,  I had enough, I'm leaving FFmpeg once and forever.

We use designated initializers in source files and internal headers, but 
not in public installed headers, so c++ projects can include them.

It's the same reason we avoid bitfields in those too.
Tomas Härdin Aug. 17, 2023, 12:57 p.m. UTC | #6 
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.

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.

/Tomas
Zhao Zhili Aug. 17, 2023, 2:03 p.m. UTC | #7 
> 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.

> 
> /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".
Tomas Härdin Aug. 20, 2023, 12:53 p.m. UTC | #8 
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
Zhao Zhili Aug. 28, 2023, 8:13 a.m. UTC | #9 
> 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.  

> 
> /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".
Tomas Härdin Aug. 28, 2023, 9:31 a.m. UTC | #10 
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
diff mbox series

Patch

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 }
 /** @} */
 
 struct AVBPrint;