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

[Laurent Pinchart]

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

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

Laurent Pinchart