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

[David Plowman]

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.

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_;
> >                 uint64_t sum = 0;
> >                 uint32_t num = 0;
> >                 uint32_t *bin = stats->hist[0].g_hist;
> > --
> > 2.25.1
> >