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

[Naushir Patuck]

Hi Kieran,

Thank you both for your review feedback.

On Thu, 21 Jan 2021 at 12:58, Kieran Bingham <
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>
> >>>> ---
> >>>>  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 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210121/acf6df07/attachment-0001.htm>