[libcamera-devel] [PATCH v2 4/4] ipa: raspberrypi: Use std::optional in DeviceStatus
Naushir Patuck
naush at raspberrypi.com
Tue Jun 28 11:42:14 CEST 2022
Hi Kieran and David,
On Tue, 28 Jun 2022 at 10:36, Kieran Bingham <
kieran.bingham at ideasonboard.com> wrote:
> 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.
>
Yes we do. There is a long (long) outstanding task of libcamerifying our
controller
source files to match the required style. This includes *.hpp -> *.h and
converting
from snake_case to camelCase. As always, it's on my to-do list :-)
>
> 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.
>
Happy to change to value_or() here, it does seem more concise.
Would you be able to change that while merging, or should I post an update?
Regards,
Naush
>
> --
> Kieran
>
> > > > uint64_t sum = 0;
> > > > uint32_t num = 0;
> > > > uint32_t *bin = stats->hist[0].g_hist;
> > > > --
> > > > 2.25.1
> > > >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20220628/ffae4ca8/attachment.htm>
More information about the libcamera-devel
mailing list