[libcamera-devel] [PATCH 1/4] raspberrypi: Add the correct integer const postfix for FrameDurations

[Kieran Bingham]

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 at ideasonboard.com
> <mailto:kieran.bingham at 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 at raspberrypi.com
>     <mailto:naush at 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 at ideasonboard.com
>     <mailto:laurent.pinchart at 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 at ideaasonboard.com
>     <mailto:kieran.bingham at ideaasonboard.com>>
> 
> 
> 
>     >
>     >>>>  };
>     >>>>
>     >>>>  } /* namespace RPi */
>     >
> 
>     -- 
>     Regards
>     --
>     Kieran
> 

-- 
Regards
--
Kieran