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

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

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

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

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

[Paul B Mahol]

NAK, as full codebase have usages of designated initializers.
_______________________________________________
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".

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

[James Almer]

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

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

[Paul B Mahol]

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

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

[James Almer]

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

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

[Zhao Zhili]

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

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

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

[Tomas Härdin]

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

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

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

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


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

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

[Tomas Härdin]

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

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

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

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


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

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

[Tomas Härdin]

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