| Message ID | 20210121115849.682130-2-naush@raspberrypi.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Naush, On 21/01/2021 11:58, Naushir Patuck wrote: > At startup, ControlInfoMap::generateIdmap() threw a log message warning > that the controls::FrameDurations had a type mismatch based on the > min/max values provided in libcamera::RPi::Controls initialiser. > > Fix this warning by adding and explicit int64_t postfix to the const > values for min and max. > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > --- > include/libcamera/ipa/raspberrypi.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h > index 1de36039cee0..8e704d922ce6 100644 > --- a/include/libcamera/ipa/raspberrypi.h > +++ b/include/libcamera/ipa/raspberrypi.h > @@ -65,7 +65,7 @@ static const ControlInfoMap Controls = { > { &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) }, > { &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) }, > { &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) }, > - { &controls::FrameDurations, ControlInfo(1000, 1000000000) }, > + { &controls::FrameDurations, ControlInfo(INT64_C(1000), INT64_C(1000000000)) }, Is this casting a boost-ism? > }; > > } /* namespace RPi */ >
Hi Kieran, On Thu, 21 Jan 2021, 12:42 pm Kieran Bingham, < kieran.bingham@ideasonboard.com> wrote: > Hi Naush, > > On 21/01/2021 11:58, Naushir Patuck wrote: > > At startup, ControlInfoMap::generateIdmap() threw a log message warning > > that the controls::FrameDurations had a type mismatch based on the > > min/max values provided in libcamera::RPi::Controls initialiser. > > > > Fix this warning by adding and explicit int64_t postfix to the const > > values for min and max. > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > --- > > include/libcamera/ipa/raspberrypi.h | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/include/libcamera/ipa/raspberrypi.h > b/include/libcamera/ipa/raspberrypi.h > > index 1de36039cee0..8e704d922ce6 100644 > > --- a/include/libcamera/ipa/raspberrypi.h > > +++ b/include/libcamera/ipa/raspberrypi.h > > @@ -65,7 +65,7 @@ static const ControlInfoMap Controls = { > > { &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) }, > > { &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) }, > > { &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, > 65535, 65535, 65535), Rectangle{}) }, > > - { &controls::FrameDurations, ControlInfo(1000, 1000000000) }, > > + { &controls::FrameDurations, ControlInfo(INT64_C(1000), > INT64_C(1000000000)) }, > > Is this casting a boost-ism? > I thought it was part of stdint, but admittedly never checked to confirm. Will do so in a bit. Regards, Naush > > > > }; > > > > } /* namespace RPi */ > > > > -- > Regards > -- > Kieran >
On Thu, Jan 21, 2021 at 12:45:17PM +0000, Naushir Patuck wrote: > On Thu, 21 Jan 2021, 12:42 pm Kieran Bingham, wrote: > > On 21/01/2021 11:58, Naushir Patuck wrote: > > > At startup, ControlInfoMap::generateIdmap() threw a log message warning > > > that the controls::FrameDurations had a type mismatch based on the > > > min/max values provided in libcamera::RPi::Controls initialiser. > > > > > > Fix this warning by adding and explicit int64_t postfix to the const > > > values for min and max. > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > --- > > > include/libcamera/ipa/raspberrypi.h | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h > > > index 1de36039cee0..8e704d922ce6 100644 > > > --- a/include/libcamera/ipa/raspberrypi.h > > > +++ b/include/libcamera/ipa/raspberrypi.h > > > @@ -65,7 +65,7 @@ static const ControlInfoMap Controls = { > > > { &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) }, > > > { &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) }, > > > { &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) }, > > > - { &controls::FrameDurations, ControlInfo(1000, 1000000000) }, > > > + { &controls::FrameDurations, ControlInfo(INT64_C(1000), INT64_C(1000000000)) }, > > > > Is this casting a boost-ism? > > I thought it was part of stdint, but admittedly never checked to confirm. > Will do so in a bit. It seems standard (and I've just learnt about it :-)). An alternative could be { &controls::FrameDurations, ControlInfo(1000LL, 1000000000LL) }, It however seems that we should improve creation of the ControlInfoMap to make these casts automatic. An exercise left to the reader :-) For now we can have a simple fix. Please include stdint.h if you want to use INT64_C, or use LL instead. With this fixed, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > }; > > > > > > } /* namespace RPi */
On 21/01/2021 12:52, Laurent Pinchart wrote: > On Thu, Jan 21, 2021 at 12:45:17PM +0000, Naushir Patuck wrote: >> On Thu, 21 Jan 2021, 12:42 pm Kieran Bingham, wrote: >>> On 21/01/2021 11:58, Naushir Patuck wrote: >>>> At startup, ControlInfoMap::generateIdmap() threw a log message warning >>>> that the controls::FrameDurations had a type mismatch based on the >>>> min/max values provided in libcamera::RPi::Controls initialiser. >>>> >>>> Fix this warning by adding and explicit int64_t postfix to the const >>>> values for min and max. >>>> >>>> Signed-off-by: Naushir Patuck <naush@raspberrypi.com> >>>> --- >>>> include/libcamera/ipa/raspberrypi.h | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h >>>> index 1de36039cee0..8e704d922ce6 100644 >>>> --- a/include/libcamera/ipa/raspberrypi.h >>>> +++ b/include/libcamera/ipa/raspberrypi.h >>>> @@ -65,7 +65,7 @@ static const ControlInfoMap Controls = { >>>> { &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) }, >>>> { &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) }, >>>> { &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) }, >>>> - { &controls::FrameDurations, ControlInfo(1000, 1000000000) }, >>>> + { &controls::FrameDurations, ControlInfo(INT64_C(1000), INT64_C(1000000000)) }, >>> >>> Is this casting a boost-ism? >> >> I thought it was part of stdint, but admittedly never checked to confirm. >> Will do so in a bit. > > It seems standard (and I've just learnt about it :-)). An alternative > could be > > { &controls::FrameDurations, ControlInfo(1000LL, 1000000000LL) }, > > It however seems that we should improve creation of the ControlInfoMap > to make these casts automatic. An exercise left to the reader :-) For > now we can have a simple fix. Please include stdint.h if you want to use > INT64_C, or use LL instead. > > With this fixed, > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> I'm surprised that it generates a warning putting a long into a long long, but I can see that it's a type mismatch indeed. Personally, the LL style would be more what I'm used to (hence not knowing about the INT64_C()) But either way: With that and an "ipa:" prefix in the subject to match the other patches: Reviewed-by: Kieran Bingham <kieran.bingham@ideaasonboard.com> > >>>> }; >>>> >>>> } /* namespace RPi */ >
Hi Kieran, Thank you both for your review feedback. On Thu, 21 Jan 2021 at 12:58, Kieran Bingham < kieran.bingham@ideasonboard.com> wrote: > On 21/01/2021 12:52, Laurent Pinchart wrote: > > On Thu, Jan 21, 2021 at 12:45:17PM +0000, Naushir Patuck wrote: > >> On Thu, 21 Jan 2021, 12:42 pm Kieran Bingham, wrote: > >>> On 21/01/2021 11:58, Naushir Patuck wrote: > >>>> At startup, ControlInfoMap::generateIdmap() threw a log message > warning > >>>> that the controls::FrameDurations had a type mismatch based on the > >>>> min/max values provided in libcamera::RPi::Controls initialiser. > >>>> > >>>> Fix this warning by adding and explicit int64_t postfix to the const > >>>> values for min and max. > >>>> > >>>> Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > >>>> --- > >>>> include/libcamera/ipa/raspberrypi.h | 2 +- > >>>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>>> > >>>> diff --git a/include/libcamera/ipa/raspberrypi.h > b/include/libcamera/ipa/raspberrypi.h > >>>> index 1de36039cee0..8e704d922ce6 100644 > >>>> --- a/include/libcamera/ipa/raspberrypi.h > >>>> +++ b/include/libcamera/ipa/raspberrypi.h > >>>> @@ -65,7 +65,7 @@ static const ControlInfoMap Controls = { > >>>> { &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) }, > >>>> { &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) > }, > >>>> { &controls::ScalerCrop, ControlInfo(Rectangle{}, > Rectangle(65535, 65535, 65535, 65535), Rectangle{}) }, > >>>> - { &controls::FrameDurations, ControlInfo(1000, 1000000000) }, > >>>> + { &controls::FrameDurations, ControlInfo(INT64_C(1000), > INT64_C(1000000000)) }, > >>> > >>> Is this casting a boost-ism? > >> > >> I thought it was part of stdint, but admittedly never checked to > confirm. > >> Will do so in a bit. > > > > It seems standard (and I've just learnt about it :-)). An alternative > > could be > > > > { &controls::FrameDurations, ControlInfo(1000LL, 1000000000LL) }, > > > > It however seems that we should improve creation of the ControlInfoMap > > to make these casts automatic. An exercise left to the reader :-) For > > now we can have a simple fix. Please include stdint.h if you want to use > > INT64_C, or use LL instead. > I knew this would come up :-) My original implementation did use LL (I did not know about INT64_C either). However, by using "LL" i get a curious compile error: In file included from ../src/libcamera/pipeline/raspberrypi/raspberrypi.cpp:20: ../include/libcamera/ipa/raspberrypi.h:68:63: error: no matching function for call to ‘libcamera::ControlInfo::ControlInfo(long long int, long long int)’ 68 | { &controls::FrameDurations, ControlInfo(1000LL, 1000000000LL) }, The compilation fails in gcc version 9.30 and 10.2.0 and clang version 10 on my Linux machine. Curiously, I just tried it on my Raspberry Pi and it compiles fine with gcc v 8.30. This is why I originally switched to INT64_C which works everywhere. It slipped my mind, and I did not get a chance to investigate further. I can only assume from this result that INT64_C macro uses the L postfix, which does compile fine. Anyone have any ideas as to what is going on? Regards, Naush > > > With this fixed, > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > I'm surprised that it generates a warning putting a long into a long > long, but I can see that it's a type mismatch indeed. > > Personally, the LL style would be more what I'm used to (hence not > knowing about the INT64_C()) But either way: > > With that and an "ipa:" prefix in the subject to match the other patches: > > Reviewed-by: Kieran Bingham <kieran.bingham@ideaasonboard.com> > > > > > > >>>> }; > >>>> > >>>> } /* namespace RPi */ > > > > -- > Regards > -- > Kieran >
Hi Naush, On 21/01/2021 16:11, Naushir Patuck wrote: > Hi Kieran, > > Thank you both for your review feedback. > > On Thu, 21 Jan 2021 at 12:58, Kieran Bingham > <kieran.bingham@ideasonboard.com > <mailto:kieran.bingham@ideasonboard.com>> wrote: > > On 21/01/2021 12:52, Laurent Pinchart wrote: > > On Thu, Jan 21, 2021 at 12:45:17PM +0000, Naushir Patuck wrote: > >> On Thu, 21 Jan 2021, 12:42 pm Kieran Bingham, wrote: > >>> On 21/01/2021 11:58, Naushir Patuck wrote: > >>>> At startup, ControlInfoMap::generateIdmap() threw a log message > warning > >>>> that the controls::FrameDurations had a type mismatch based on the > >>>> min/max values provided in libcamera::RPi::Controls initialiser. > >>>> > >>>> Fix this warning by adding and explicit int64_t postfix to the > const > >>>> values for min and max. > >>>> > >>>> Signed-off-by: Naushir Patuck <naush@raspberrypi.com > <mailto:naush@raspberrypi.com>> > >>>> --- > >>>> include/libcamera/ipa/raspberrypi.h | 2 +- > >>>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>>> > >>>> diff --git a/include/libcamera/ipa/raspberrypi.h > b/include/libcamera/ipa/raspberrypi.h > >>>> index 1de36039cee0..8e704d922ce6 100644 > >>>> --- a/include/libcamera/ipa/raspberrypi.h > >>>> +++ b/include/libcamera/ipa/raspberrypi.h > >>>> @@ -65,7 +65,7 @@ static const ControlInfoMap Controls = { > >>>> { &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) }, > >>>> { &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, > 16.0f) }, > >>>> { &controls::ScalerCrop, ControlInfo(Rectangle{}, > Rectangle(65535, 65535, 65535, 65535), Rectangle{}) }, > >>>> - { &controls::FrameDurations, ControlInfo(1000, 1000000000) }, > >>>> + { &controls::FrameDurations, ControlInfo(INT64_C(1000), > INT64_C(1000000000)) }, > >>> > >>> Is this casting a boost-ism? > >> > >> I thought it was part of stdint, but admittedly never checked to > confirm. > >> Will do so in a bit. > > > > It seems standard (and I've just learnt about it :-)). An alternative > > could be > > > > { &controls::FrameDurations, ControlInfo(1000LL, > 1000000000LL) }, > > > > It however seems that we should improve creation of the ControlInfoMap > > to make these casts automatic. An exercise left to the reader :-) For > > now we can have a simple fix. Please include stdint.h if you want > to use > > INT64_C, or use LL instead. > > > I knew this would come up :-) > > My original implementation did use LL (I did not know about INT64_C > either). However, by using "LL" i get a curious compile error: > > In file included from > ../src/libcamera/pipeline/raspberrypi/raspberrypi.cpp:20: > ../include/libcamera/ipa/raspberrypi.h:68:63: error: no matching > function for call to ‘libcamera::ControlInfo::ControlInfo(long long int, > long long int)’ > 68 | { &controls::FrameDurations, ControlInfo(1000LL, 1000000000LL) }, > > The compilation fails in gcc version 9.30 and 10.2.0 and clang version > 10 on my Linux machine. Curiously, I just tried it on my Raspberry Pi > and it compiles fine with gcc v 8.30. > > This is why I originally switched to INT64_C which works everywhere. It > slipped my mind, and I did not get a chance to investigate further. I > can only assume from this result that INT64_C macro uses the L postfix, > which does compile fine. Anyone have any ideas as to what is going on? Oh isn't C/C++ wonderful. I only raised the INT64_C() as it stood out as something I'd not seen before. If it's part of stdint.h and works I wouldn't object to using that. -- Kieran > Regards, > Naush > > > > > With this fixed, > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com > <mailto:laurent.pinchart@ideasonboard.com>> > > I'm surprised that it generates a warning putting a long into a long > long, but I can see that it's a type mismatch indeed. > > Personally, the LL style would be more what I'm used to (hence not > knowing about the INT64_C()) But either way: > > With that and an "ipa:" prefix in the subject to match the other > patches: > > Reviewed-by: Kieran Bingham <kieran.bingham@ideaasonboard.com > <mailto:kieran.bingham@ideaasonboard.com>> > > > > > > >>>> }; > >>>> > >>>> } /* namespace RPi */ > > > > -- > Regards > -- > Kieran >
Hi Naush, On Thu, Jan 21, 2021 at 04:11:36PM +0000, Naushir Patuck wrote: > On Thu, 21 Jan 2021 at 12:58, Kieran Bingham wrote: > > On 21/01/2021 12:52, Laurent Pinchart wrote: > > > On Thu, Jan 21, 2021 at 12:45:17PM +0000, Naushir Patuck wrote: > > >> On Thu, 21 Jan 2021, 12:42 pm Kieran Bingham, wrote: > > >>> On 21/01/2021 11:58, Naushir Patuck wrote: > > >>>> At startup, ControlInfoMap::generateIdmap() threw a log message warning > > >>>> that the controls::FrameDurations had a type mismatch based on the > > >>>> min/max values provided in libcamera::RPi::Controls initialiser. > > >>>> > > >>>> Fix this warning by adding and explicit int64_t postfix to the const > > >>>> values for min and max. > > >>>> > > >>>> Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > >>>> --- > > >>>> include/libcamera/ipa/raspberrypi.h | 2 +- > > >>>> 1 file changed, 1 insertion(+), 1 deletion(-) > > >>>> > > >>>> diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h > > >>>> index 1de36039cee0..8e704d922ce6 100644 > > >>>> --- a/include/libcamera/ipa/raspberrypi.h > > >>>> +++ b/include/libcamera/ipa/raspberrypi.h > > >>>> @@ -65,7 +65,7 @@ static const ControlInfoMap Controls = { > > >>>> { &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) }, > > >>>> { &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) }, > > >>>> { &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) }, > > >>>> - { &controls::FrameDurations, ControlInfo(1000, 1000000000) }, > > >>>> + { &controls::FrameDurations, ControlInfo(INT64_C(1000), INT64_C(1000000000)) }, > > >>> > > >>> Is this casting a boost-ism? > > >> > > >> I thought it was part of stdint, but admittedly never checked to confirm. > > >> Will do so in a bit. > > > > > > It seems standard (and I've just learnt about it :-)). An alternative > > > could be > > > > > > { &controls::FrameDurations, ControlInfo(1000LL, 1000000000LL) }, > > > > > > It however seems that we should improve creation of the ControlInfoMap > > > to make these casts automatic. An exercise left to the reader :-) For > > > now we can have a simple fix. Please include stdint.h if you want to use > > > INT64_C, or use LL instead. > > I knew this would come up :-) > > My original implementation did use LL (I did not know about INT64_C > either). However, by using "LL" i get a curious compile error: > > In file included from > ../src/libcamera/pipeline/raspberrypi/raspberrypi.cpp:20: > ../include/libcamera/ipa/raspberrypi.h:68:63: error: no matching function > for call to ‘libcamera::ControlInfo::ControlInfo(long long int, long long > int)’ > 68 | { &controls::FrameDurations, ControlInfo(1000LL, 1000000000LL) }, > > The compilation fails in gcc version 9.30 and 10.2.0 and clang version 10 > on my Linux machine. Curiously, I just tried it on my Raspberry Pi and it > compiles fine with gcc v 8.30. As RPi is a 32-bit platform, int64_t maps to long long, so the LL suffix matches. On a 64-bit platform, int64_t maps to long, so LL isn't the same type (even if the two types are equivalent). > This is why I originally switched to INT64_C which works everywhere. It > slipped my mind, and I did not get a chance to investigate further. I can > only assume from this result that INT64_C macro uses the L postfix, which > does compile fine. Anyone have any ideas as to what is going on? INT64_C is the right way to go. You only need to include stdint.h. > > > With this fixed, > > > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > I'm surprised that it generates a warning putting a long into a long > > long, but I can see that it's a type mismatch indeed. > > > > Personally, the LL style would be more what I'm used to (hence not > > knowing about the INT64_C()) But either way: > > > > With that and an "ipa:" prefix in the subject to match the other patches: > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideaasonboard.com> > > > > >>>> }; > > >>>> > > >>>> } /* namespace RPi */
diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h index 1de36039cee0..8e704d922ce6 100644 --- a/include/libcamera/ipa/raspberrypi.h +++ b/include/libcamera/ipa/raspberrypi.h @@ -65,7 +65,7 @@ static const ControlInfoMap Controls = { { &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) }, { &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) }, { &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) }, - { &controls::FrameDurations, ControlInfo(1000, 1000000000) }, + { &controls::FrameDurations, ControlInfo(INT64_C(1000), INT64_C(1000000000)) }, }; } /* namespace RPi */
At startup, ControlInfoMap::generateIdmap() threw a log message warning that the controls::FrameDurations had a type mismatch based on the min/max values provided in libcamera::RPi::Controls initialiser. Fix this warning by adding and explicit int64_t postfix to the const values for min and max. Signed-off-by: Naushir Patuck <naush@raspberrypi.com> --- include/libcamera/ipa/raspberrypi.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)