<div dir="ltr"><div dir="ltr">Hi Kieran,<div><br></div><div>Thank you both for your review feedback.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, 21 Jan 2021 at 12:58, Kieran Bingham <<a href="mailto:kieran.bingham@ideasonboard.com">kieran.bingham@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On 21/01/2021 12:52, Laurent Pinchart wrote:<br>
> On Thu, Jan 21, 2021 at 12:45:17PM +0000, Naushir Patuck wrote:<br>
>> On Thu, 21 Jan 2021, 12:42 pm Kieran Bingham, wrote:<br>
>>> On 21/01/2021 11:58, Naushir Patuck wrote:<br>
>>>> At startup, ControlInfoMap::generateIdmap() threw a log message warning<br>
>>>> that the controls::FrameDurations had a type mismatch based on the<br>
>>>> min/max values provided in libcamera::RPi::Controls initialiser.<br>
>>>><br>
>>>> Fix this warning by adding and explicit int64_t postfix to the const<br>
>>>> values for min and max.<br>
>>>><br>
>>>> Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
>>>> ---<br>
>>>>  include/libcamera/ipa/raspberrypi.h | 2 +-<br>
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)<br>
>>>><br>
>>>> diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h<br>
>>>> index 1de36039cee0..8e704d922ce6 100644<br>
>>>> --- a/include/libcamera/ipa/raspberrypi.h<br>
>>>> +++ b/include/libcamera/ipa/raspberrypi.h<br>
>>>> @@ -65,7 +65,7 @@ static const ControlInfoMap Controls = {<br>
>>>>       { &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },<br>
>>>>       { &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) },<br>
>>>>       { &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },<br>
>>>> -     { &controls::FrameDurations, ControlInfo(1000, 1000000000) },<br>
>>>> +     { &controls::FrameDurations, ControlInfo(INT64_C(1000), INT64_C(1000000000)) },<br>
>>><br>
>>> Is this casting a boost-ism?<br>
>><br>
>> I thought it was part of stdint, but admittedly never checked to confirm.<br>
>> Will do so in a bit.<br>
> <br>
> It seems standard (and I've just learnt about it :-)). An alternative<br>
> could be<br>
> <br>
>       { &controls::FrameDurations, ControlInfo(1000LL, 1000000000LL) },<br>
> <br>
> It however seems that we should improve creation of the ControlInfoMap<br>
> to make these casts automatic. An exercise left to the reader :-) For<br>
> now we can have a simple fix. Please include stdint.h if you want to use<br>
> INT64_C, or use LL instead.<br></blockquote><div><br></div><div>I knew this would come up :-)</div><div><br></div><div>My original implementation did use LL (I did not know about INT64_C either).  However, by using "LL" i get a curious compile error:</div><div><br></div><div>In file included from ../src/libcamera/pipeline/raspberrypi/raspberrypi.cpp:20:<br>../include/libcamera/ipa/raspberrypi.h:68:63: error: no matching function for call to ‘libcamera::ControlInfo::ControlInfo(long long int, long long int)’<br>   68 |  { &controls::FrameDurations, ControlInfo(1000LL, 1000000000LL) },<br></div><div><br></div><div>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.  </div><div><br></div><div>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?</div><div><br></div><div>Regards,</div><div>Naush</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">> <br>
> With this fixed,<br>
> <br>
> Reviewed-by: Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com" target="_blank">laurent.pinchart@ideasonboard.com</a>><br>
<br>
I'm surprised that it generates a warning putting a long into a long<br>
long, but I can see that it's a type mismatch indeed.<br>
<br>
Personally, the LL style would be more what I'm used to (hence not<br>
knowing about the INT64_C()) But either way:<br>
<br>
With that and an "ipa:" prefix in the subject to match the other patches:<br>
<br>
Reviewed-by: Kieran Bingham <<a href="mailto:kieran.bingham@ideaasonboard.com" target="_blank">kieran.bingham@ideaasonboard.com</a>><br>
<br>
<br>
<br>
> <br>
>>>>  };<br>
>>>><br>
>>>>  } /* namespace RPi */<br>
> <br>
<br>
-- <br>
Regards<br>
--<br>
Kieran<br>
</blockquote></div></div>