[libcamera-devel] [PATCH v2 4/4] ipa: raspberrypi: Use std::optional in DeviceStatus

[Kieran Bingham]

Hi Naush,

Quoting David Plowman (2022-06-28 09:35:00)
> Hi Naush
> 
> Thanks for this, I think it's a good improvement!
> 
> On Fri, 24 Jun 2022 at 13:34, Kieran Bingham via libcamera-devel
> <libcamera-devel at lists.libcamera.org> wrote:
> >
> > Quoting Naushir Patuck via libcamera-devel (2022-06-24 08:35:28)
> > > Switch the aperture, lens_position, and flash_intensity fields in the
> > > DeviceStatus structure to use std::optional instead of using invalid default
> > > values.
> >
> > \o/
> >
> >
> > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> >
> > >
> > > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> 
> I suppose the only thing I wonder about is that once upon a time we
> had a convention that .h files could be included in a C program, and
> .hpp was for C++ only headers. But obviously libcamera doesn't do that
> so I guess it is indeed time to consign that one to the dustbin of
> history. Is it a bit weird now that we have both? I don't think I'm
> bothered. So it's a \o/ from me too.

Oh ... we have .hpp files ? - Oh - we do in controller indeed.

We should probably make these more consitent, but that can be a patch on
top anyway.

Another comment below too...

> 
> Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
> 
> Thanks!
> David
> 
> > > ---
> > >  src/ipa/raspberrypi/controller/device_status.cpp | 14 ++++++++++----
> > >  src/ipa/raspberrypi/controller/device_status.h   |  9 ++++-----
> > >  src/ipa/raspberrypi/controller/rpi/lux.cpp       |  5 ++---
> > >  3 files changed, 16 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/src/ipa/raspberrypi/controller/device_status.cpp b/src/ipa/raspberrypi/controller/device_status.cpp
> > > index 05897fc15b50..a389c40dafed 100644
> > > --- a/src/ipa/raspberrypi/controller/device_status.cpp
> > > +++ b/src/ipa/raspberrypi/controller/device_status.cpp
> > > @@ -12,10 +12,16 @@ std::ostream &operator<<(std::ostream &out, const DeviceStatus &d)
> > >  {
> > >         out << "Exposure: " << d.shutter_speed
> > >             << " Frame length: " << d.frame_length
> > > -           << " Gain: " << d.analogue_gain
> > > -           << " Aperture: " << d.aperture
> > > -           << " Lens: " << d.lens_position
> > > -           << " Flash: " << d.flash_intensity;
> > > +           << " Gain: " << d.analogue_gain;
> > > +
> > > +       if (d.aperture)
> > > +               out << " Aperture: " << *d.aperture;
> > > +
> > > +       if (d.lens_position)
> > > +               out << " Lens: " << *d.lens_position;
> > > +
> > > +       if (d.flash_intensity)
> > > +               out << " Flash: " << *d.flash_intensity;
> > >
> > >         if (d.sensor_temperature)
> > >                 out << " Temperature: " << *d.sensor_temperature;
> > > diff --git a/src/ipa/raspberrypi/controller/device_status.h b/src/ipa/raspberrypi/controller/device_status.h
> > > index eca3bf4b042e..b33f0d093ff3 100644
> > > --- a/src/ipa/raspberrypi/controller/device_status.h
> > > +++ b/src/ipa/raspberrypi/controller/device_status.h
> > > @@ -19,8 +19,7 @@
> > >  struct DeviceStatus {
> > >         DeviceStatus()
> > >                 : shutter_speed(std::chrono::seconds(0)), frame_length(0),
> > > -                 analogue_gain(0.0), lens_position(0.0), aperture(0.0),
> > > -                 flash_intensity(0.0)
> > > +                 analogue_gain(0.0)
> > >         {
> > >         }
> > >
> > > @@ -32,11 +31,11 @@ struct DeviceStatus {
> > >         uint32_t frame_length;
> > >         double analogue_gain;
> > >         /* 1.0/distance-in-metres, or 0 if unknown */
> > > -       double lens_position;
> > > +       std::optional<double> lens_position;
> > >         /* 1/f so that brightness quadruples when this doubles, or 0 if unknown */
> > > -       double aperture;
> > > +       std::optional<double> aperture;
> > >         /* proportional to brightness with 0 = no flash, 1 = maximum flash */
> > > -       double flash_intensity;
> > > +       std::optional<double> flash_intensity;
> > >         /* Sensor reported temperature value (in degrees) */
> > >         std::optional<double> sensor_temperature;
> > >  };
> > > diff --git a/src/ipa/raspberrypi/controller/rpi/lux.cpp b/src/ipa/raspberrypi/controller/rpi/lux.cpp
> > > index f77e9140ac10..643fb8fbaac6 100644
> > > --- a/src/ipa/raspberrypi/controller/rpi/lux.cpp
> > > +++ b/src/ipa/raspberrypi/controller/rpi/lux.cpp
> > > @@ -63,9 +63,8 @@ void Lux::Process(StatisticsPtr &stats, Metadata *image_metadata)
> > >         DeviceStatus device_status;
> > >         if (image_metadata->Get("device.status", device_status) == 0) {
> > >                 double current_gain = device_status.analogue_gain;
> > > -               double current_aperture = device_status.aperture;
> > > -               if (current_aperture == 0)
> > > -                       current_aperture = current_aperture_;
> > > +               double current_aperture = device_status.aperture ? *device_status.aperture
> > > +                                                                : current_aperture_;

Aha, there's actually one small thing here.

With std::optional, we have value_or(), [0] so this could be:
		double current_aperture = device_status.aperture.value_or(current_aperture_);

though that comes in at 94 characters long...
I don't mind either way though, it's only really syntactic sugar.

[0] https://en.cppreference.com/w/cpp/utility/optional/value_or

Let me know what you think/prefer and I'll get these through the matrix
and merged soon now we have David's review.

--
Kieran

> > >                 uint64_t sum = 0;
> > >                 uint32_t num = 0;
> > >                 uint32_t *bin = stats->hist[0].g_hist;
> > > --
> > > 2.25.1
> > >