| Message ID | 20220624073528.26670-5-naush@raspberrypi.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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@ideasonboard.com> > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > --- > 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 >
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@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@ideasonboard.com> > > > > > Signed-off-by: Naushir Patuck <naush@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@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 > >
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@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@ideasonboard.com> > > > > > > > > Signed-off-by: Naushir Patuck <naush@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@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 > > >
Hi Kieran and David, On Tue, 28 Jun 2022 at 10:36, Kieran Bingham < kieran.bingham@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@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@ideasonboard.com> > > > > > > > > > > > Signed-off-by: Naushir Patuck <naush@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@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 > > > > >
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;
Switch the aperture, lens_position, and flash_intensity fields in the DeviceStatus structure to use std::optional instead of using invalid default values. Signed-off-by: Naushir Patuck <naush@raspberrypi.com> --- 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(-)