| Message ID | 20240212211537.18468-2-cus@passwd.hu |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,1/5] avutil/channel_layout: change AV_CHAN_SILENCE to AV_CHAN_UNUSED in the docs | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | fail | Make fate failed |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | fail | Make fate failed |
On 2/12/2024 6:15 PM, Marton Balint wrote: > Signed-off-by: Marton Balint <cus@passwd.hu> > --- > libavutil/channel_layout.h | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/libavutil/channel_layout.h b/libavutil/channel_layout.h > index b8bff6f365..db0c005e87 100644 > --- a/libavutil/channel_layout.h > +++ b/libavutil/channel_layout.h > @@ -146,6 +146,10 @@ enum AVChannelOrder { > * as defined in AmbiX format $ 2.1. > */ > AV_CHANNEL_ORDER_AMBISONIC, > + /** > + * Number of channel orders, not part of ABI/API > + */ > + AV_CHANNEL_ORDER_NB > }; Is it worth adding this to a public header just to limit a loop in a test? A loop that fwiw still depends on an array that needs to be updated with more names if you add new orders. IMO, just do FF_ARRAY_ELEMS(channel_order_names) in the test.
On Tue, 13 Feb 2024, James Almer wrote: > On 2/12/2024 6:15 PM, Marton Balint wrote: >> Signed-off-by: Marton Balint <cus@passwd.hu> >> --- >> libavutil/channel_layout.h | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/libavutil/channel_layout.h b/libavutil/channel_layout.h >> index b8bff6f365..db0c005e87 100644 >> --- a/libavutil/channel_layout.h >> +++ b/libavutil/channel_layout.h >> @@ -146,6 +146,10 @@ enum AVChannelOrder { >> * as defined in AmbiX format $ 2.1. >> */ >> AV_CHANNEL_ORDER_AMBISONIC, >> + /** >> + * Number of channel orders, not part of ABI/API >> + */ >> + AV_CHANNEL_ORDER_NB >> }; > > Is it worth adding this to a public header just to limit a loop in a test? A > loop that fwiw still depends on an array that needs to be updated with more > names if you add new orders. Several other enums also have this. So API consistency can be considered a more important factor. > > IMO, just do FF_ARRAY_ELEMS(channel_order_names) in the test. Then adding a new channel order would not show up as breakage in the test. I have no strong preference though, and can change it if you still want me to. Regards, Marton
Quoting Marton Balint (2024-02-13 21:27:34) > > > On Tue, 13 Feb 2024, James Almer wrote: > > > On 2/12/2024 6:15 PM, Marton Balint wrote: > >> Signed-off-by: Marton Balint <cus@passwd.hu> > >> --- > >> libavutil/channel_layout.h | 4 ++++ > >> 1 file changed, 4 insertions(+) > >> > >> diff --git a/libavutil/channel_layout.h b/libavutil/channel_layout.h > >> index b8bff6f365..db0c005e87 100644 > >> --- a/libavutil/channel_layout.h > >> +++ b/libavutil/channel_layout.h > >> @@ -146,6 +146,10 @@ enum AVChannelOrder { > >> * as defined in AmbiX format $ 2.1. > >> */ > >> AV_CHANNEL_ORDER_AMBISONIC, > >> + /** > >> + * Number of channel orders, not part of ABI/API > >> + */ > >> + AV_CHANNEL_ORDER_NB > >> }; > > > > Is it worth adding this to a public header just to limit a loop in a test? A > > loop that fwiw still depends on an array that needs to be updated with more > > names if you add new orders. > > Several other enums also have this. So API consistency can be considered > a more important factor. I'd be concerned that many callers don't undertand the implications of "not part of the ABI". Maybe we should rename all of them to FF_ prefix to make it more clear callers should not use these?
On Thu, 15 Feb 2024, Anton Khirnov wrote: > Quoting Marton Balint (2024-02-13 21:27:34) >> >> >> On Tue, 13 Feb 2024, James Almer wrote: >> >>> On 2/12/2024 6:15 PM, Marton Balint wrote: >>>> Signed-off-by: Marton Balint <cus@passwd.hu> >>>> --- >>>> libavutil/channel_layout.h | 4 ++++ >>>> 1 file changed, 4 insertions(+) >>>> >>>> diff --git a/libavutil/channel_layout.h b/libavutil/channel_layout.h >>>> index b8bff6f365..db0c005e87 100644 >>>> --- a/libavutil/channel_layout.h >>>> +++ b/libavutil/channel_layout.h >>>> @@ -146,6 +146,10 @@ enum AVChannelOrder { >>>> * as defined in AmbiX format $ 2.1. >>>> */ >>>> AV_CHANNEL_ORDER_AMBISONIC, >>>> + /** >>>> + * Number of channel orders, not part of ABI/API >>>> + */ >>>> + AV_CHANNEL_ORDER_NB >>>> }; >>> >>> Is it worth adding this to a public header just to limit a loop in a test? A >>> loop that fwiw still depends on an array that needs to be updated with more >>> names if you add new orders. >> >> Several other enums also have this. So API consistency can be considered >> a more important factor. > > I'd be concerned that many callers don't undertand the implications of > "not part of the ABI". > > Maybe we should rename all of them to FF_ prefix to make it more clear > callers should not use these? I think this is a good idea. So is it OK to apply this if I change the prefix to FF? Thanks, Marton
On 2/16/2024 7:42 PM, Marton Balint wrote: > > > On Thu, 15 Feb 2024, Anton Khirnov wrote: > >> Quoting Marton Balint (2024-02-13 21:27:34) >>> >>> >>> On Tue, 13 Feb 2024, James Almer wrote: >>> >>>> On 2/12/2024 6:15 PM, Marton Balint wrote: >>>>> Signed-off-by: Marton Balint <cus@passwd.hu> >>>>> --- >>>>> libavutil/channel_layout.h | 4 ++++ >>>>> 1 file changed, 4 insertions(+) >>>>> >>>>> diff --git a/libavutil/channel_layout.h b/libavutil/channel_layout.h >>>>> index b8bff6f365..db0c005e87 100644 >>>>> --- a/libavutil/channel_layout.h >>>>> +++ b/libavutil/channel_layout.h >>>>> @@ -146,6 +146,10 @@ enum AVChannelOrder { >>>>> * as defined in AmbiX format $ 2.1. >>>>> */ >>>>> AV_CHANNEL_ORDER_AMBISONIC, >>>>> + /** >>>>> + * Number of channel orders, not part of ABI/API >>>>> + */ >>>>> + AV_CHANNEL_ORDER_NB >>>>> }; >>>> >>>> Is it worth adding this to a public header just to limit a loop in a >>>> test? A >>>> loop that fwiw still depends on an array that needs to be updated >>>> with more >>>> names if you add new orders. >>> >>> Several other enums also have this. So API consistency can be considered >>> a more important factor. >> >> I'd be concerned that many callers don't undertand the implications of >> "not part of the ABI". >> >> Maybe we should rename all of them to FF_ prefix to make it more clear >> callers should not use these? > > I think this is a good idea. So is it OK to apply this if I change the > prefix to FF? I wont oppose to it.
On Fri, 16 Feb 2024, James Almer wrote: > On 2/16/2024 7:42 PM, Marton Balint wrote: >> >> >> On Thu, 15 Feb 2024, Anton Khirnov wrote: >> >>> Quoting Marton Balint (2024-02-13 21:27:34) >>>> >>>> >>>> On Tue, 13 Feb 2024, James Almer wrote: >>>> >>>>> On 2/12/2024 6:15 PM, Marton Balint wrote: >>>>>> Signed-off-by: Marton Balint <cus@passwd.hu> >>>>>> --- >>>>>> libavutil/channel_layout.h | 4 ++++ >>>>>> 1 file changed, 4 insertions(+) >>>>>> >>>>>> diff --git a/libavutil/channel_layout.h b/libavutil/channel_layout.h >>>>>> index b8bff6f365..db0c005e87 100644 >>>>>> --- a/libavutil/channel_layout.h >>>>>> +++ b/libavutil/channel_layout.h >>>>>> @@ -146,6 +146,10 @@ enum AVChannelOrder { >>>>>> * as defined in AmbiX format $ 2.1. >>>>>> */ >>>>>> AV_CHANNEL_ORDER_AMBISONIC, >>>>>> + /** >>>>>> + * Number of channel orders, not part of ABI/API >>>>>> + */ >>>>>> + AV_CHANNEL_ORDER_NB >>>>>> }; >>>>> >>>>> Is it worth adding this to a public header just to limit a loop in a >>>>> test? A >>>>> loop that fwiw still depends on an array that needs to be updated with >>>>> more >>>>> names if you add new orders. >>>> >>>> Several other enums also have this. So API consistency can be considered >>>> a more important factor. >>> >>> I'd be concerned that many callers don't undertand the implications of >>> "not part of the ABI". >>> >>> Maybe we should rename all of them to FF_ prefix to make it more clear >>> callers should not use these? >> >> I think this is a good idea. So is it OK to apply this if I change the >> prefix to FF? > > I wont oppose to it. Ok, changed locally. Will apply the series soon. Regards, Marton
diff --git a/libavutil/channel_layout.h b/libavutil/channel_layout.h index b8bff6f365..db0c005e87 100644 --- a/libavutil/channel_layout.h +++ b/libavutil/channel_layout.h @@ -146,6 +146,10 @@ enum AVChannelOrder { * as defined in AmbiX format $ 2.1. */ AV_CHANNEL_ORDER_AMBISONIC, + /** + * Number of channel orders, not part of ABI/API + */ + AV_CHANNEL_ORDER_NB };
Signed-off-by: Marton Balint <cus@passwd.hu> --- libavutil/channel_layout.h | 4 ++++ 1 file changed, 4 insertions(+)