[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 at haerdin.se> wrote:
> 
> tor 2023-08-17 klockan 22:03 +0800 skrev zhilizhao(赵志立):
>> 
>> 
>>> On Aug 17, 2023, at 20:57, Tomas Härdin <git at haerdin.se> wrote:
>>> 
>>> ons 2023-08-16 klockan 23:44 +0800 skrev Zhao Zhili:
>>>> From: Zhao Zhili <zhilizhao at 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 at 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 at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".