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

[Kieran Bingham]

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



> 
>>>>  };
>>>>
>>>>  } /* namespace RPi */
> 

-- 
Regards
--
Kieran