[libcamera-devel,v3,2/8] ipa: raspberrypi: Add a constructor struct DeviceStatus
diff mbox series

Message ID 20210702150940.226941-3-naush@raspberrypi.com
State Superseded
Headers show
Series
  • Raspberry Pi: Enable imx477 long exposure modes
Related show

Commit Message

Naushir Patuck July 2, 2021, 3:09 p.m. UTC
The constructor sets all fields to 0. This replaces the memset(0) and default
value initialisation usage in the agc and lux controllers respectively.

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
---
 src/ipa/raspberrypi/controller/device_status.h | 6 ++++++
 src/ipa/raspberrypi/controller/rpi/agc.cpp     | 2 +-
 src/ipa/raspberrypi/controller/rpi/lux.cpp     | 8 +-------
 3 files changed, 8 insertions(+), 8 deletions(-)

Comments

David Plowman July 2, 2021, 3:44 p.m. UTC | #1
Hi Naush

Thanks for tidying this up!

On Fri, 2 Jul 2021 at 16:09, Naushir Patuck <naush@raspberrypi.com> wrote:
>
> The constructor sets all fields to 0. This replaces the memset(0) and default
> value initialisation usage in the agc and lux controllers respectively.
>
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> ---
>  src/ipa/raspberrypi/controller/device_status.h | 6 ++++++
>  src/ipa/raspberrypi/controller/rpi/agc.cpp     | 2 +-
>  src/ipa/raspberrypi/controller/rpi/lux.cpp     | 8 +-------
>  3 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/src/ipa/raspberrypi/controller/device_status.h b/src/ipa/raspberrypi/controller/device_status.h
> index 733378dbfa27..73df7ce228dd 100644
> --- a/src/ipa/raspberrypi/controller/device_status.h
> +++ b/src/ipa/raspberrypi/controller/device_status.h
> @@ -14,6 +14,12 @@
>   */
>
>  struct DeviceStatus {
> +       DeviceStatus()
> +               : shutter_speed(std::chrono::seconds(0)), analogue_gain(0.0),
> +                 lens_position(0.0), aperture(0.0), flash_intensity(0.0)
> +       {
> +       }
> +
>         /* time shutter is open */
>         libcamera::utils::Duration shutter_speed;
>         double analogue_gain;
> diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/raspberrypi/controller/rpi/agc.cpp
> index 6c3a4eb2a04d..393cfacea025 100644
> --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp
> +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp
> @@ -172,7 +172,7 @@ Agc::Agc(Controller *controller)
>         // it's not been calculated yet (i.e. Process hasn't yet run).
>         memset(&status_, 0, sizeof(status_));
>         status_.ev = ev_;
> -       memset(&last_device_status_, 0, sizeof(last_device_status_));
> +       last_device_status_ = {};

I only wondered slightly whether this could be left out and we rely on
the new constructor iniitialising last_device_stats_ automatically?

>  }
>
>  char const *Agc::Name() const
> diff --git a/src/ipa/raspberrypi/controller/rpi/lux.cpp b/src/ipa/raspberrypi/controller/rpi/lux.cpp
> index f58d69397e1c..a3ae633b867a 100644
> --- a/src/ipa/raspberrypi/controller/rpi/lux.cpp
> +++ b/src/ipa/raspberrypi/controller/rpi/lux.cpp
> @@ -61,13 +61,7 @@ void Lux::Prepare(Metadata *image_metadata)
>  void Lux::Process(StatisticsPtr &stats, Metadata *image_metadata)
>  {
>         // set some initial values to shut the compiler up
> -       DeviceStatus device_status = {
> -               .shutter_speed = 1.0ms,
> -               .analogue_gain = 1.0,
> -               .lens_position = 0.0,
> -               .aperture = 0.0,
> -               .flash_intensity = 0.0
> -       };
> +       DeviceStatus device_status;

Maybe we can remove my slightly off-hand remark about shutting up the
compiler too?

Notwithstanding those very minor things:

Reviewed-by: David Plowman <david.plowman@raspberrypi.com>

Thanks
David

>         if (image_metadata->Get("device.status", device_status) == 0) {
>                 double current_gain = device_status.analogue_gain;
>                 double current_aperture = device_status.aperture;
> --
> 2.25.1
>
Naushir Patuck July 2, 2021, 3:59 p.m. UTC | #2
Hi David,

Thank you for your review feedback.

On Fri, 2 Jul 2021 at 16:45, David Plowman <david.plowman@raspberrypi.com>
wrote:

> Hi Naush
>
> Thanks for tidying this up!
>
> On Fri, 2 Jul 2021 at 16:09, Naushir Patuck <naush@raspberrypi.com> wrote:
> >
> > The constructor sets all fields to 0. This replaces the memset(0) and
> default
> > value initialisation usage in the agc and lux controllers respectively.
> >
> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > ---
> >  src/ipa/raspberrypi/controller/device_status.h | 6 ++++++
> >  src/ipa/raspberrypi/controller/rpi/agc.cpp     | 2 +-
> >  src/ipa/raspberrypi/controller/rpi/lux.cpp     | 8 +-------
> >  3 files changed, 8 insertions(+), 8 deletions(-)
> >
> > diff --git a/src/ipa/raspberrypi/controller/device_status.h
> b/src/ipa/raspberrypi/controller/device_status.h
> > index 733378dbfa27..73df7ce228dd 100644
> > --- a/src/ipa/raspberrypi/controller/device_status.h
> > +++ b/src/ipa/raspberrypi/controller/device_status.h
> > @@ -14,6 +14,12 @@
> >   */
> >
> >  struct DeviceStatus {
> > +       DeviceStatus()
> > +               : shutter_speed(std::chrono::seconds(0)),
> analogue_gain(0.0),
> > +                 lens_position(0.0), aperture(0.0), flash_intensity(0.0)
> > +       {
> > +       }
> > +
> >         /* time shutter is open */
> >         libcamera::utils::Duration shutter_speed;
> >         double analogue_gain;
> > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp
> b/src/ipa/raspberrypi/controller/rpi/agc.cpp
> > index 6c3a4eb2a04d..393cfacea025 100644
> > --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp
> > +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp
> > @@ -172,7 +172,7 @@ Agc::Agc(Controller *controller)
> >         // it's not been calculated yet (i.e. Process hasn't yet run).
> >         memset(&status_, 0, sizeof(status_));
> >         status_.ev = ev_;
> > -       memset(&last_device_status_, 0, sizeof(last_device_status_));
> > +       last_device_status_ = {};
>
> I only wondered slightly whether this could be left out and we rely on
> the new constructor iniitialising last_device_stats_ automatically?
>

Oops, yes we can!


>
> >  }
> >
> >  char const *Agc::Name() const
> > diff --git a/src/ipa/raspberrypi/controller/rpi/lux.cpp
> b/src/ipa/raspberrypi/controller/rpi/lux.cpp
> > index f58d69397e1c..a3ae633b867a 100644
> > --- a/src/ipa/raspberrypi/controller/rpi/lux.cpp
> > +++ b/src/ipa/raspberrypi/controller/rpi/lux.cpp
> > @@ -61,13 +61,7 @@ void Lux::Prepare(Metadata *image_metadata)
> >  void Lux::Process(StatisticsPtr &stats, Metadata *image_metadata)
> >  {
> >         // set some initial values to shut the compiler up
> > -       DeviceStatus device_status = {
> > -               .shutter_speed = 1.0ms,
> > -               .analogue_gain = 1.0,
> > -               .lens_position = 0.0,
> > -               .aperture = 0.0,
> > -               .flash_intensity = 0.0
> > -       };
> > +       DeviceStatus device_status;
>
> Maybe we can remove my slightly off-hand remark about shutting up the
> compiler too?
>

Will remove the comment in the next revision.

Regards,
Naush


>
> Notwithstanding those very minor things:
>
> Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
>
> Thanks
> David
>
> >         if (image_metadata->Get("device.status", device_status) == 0) {
> >                 double current_gain = device_status.analogue_gain;
> >                 double current_aperture = device_status.aperture;
> > --
> > 2.25.1
> >
>

Patch
diff mbox series

diff --git a/src/ipa/raspberrypi/controller/device_status.h b/src/ipa/raspberrypi/controller/device_status.h
index 733378dbfa27..73df7ce228dd 100644
--- a/src/ipa/raspberrypi/controller/device_status.h
+++ b/src/ipa/raspberrypi/controller/device_status.h
@@ -14,6 +14,12 @@ 
  */
 
 struct DeviceStatus {
+	DeviceStatus()
+		: shutter_speed(std::chrono::seconds(0)), analogue_gain(0.0),
+		  lens_position(0.0), aperture(0.0), flash_intensity(0.0)
+	{
+	}
+
 	/* time shutter is open */
 	libcamera::utils::Duration shutter_speed;
 	double analogue_gain;
diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/raspberrypi/controller/rpi/agc.cpp
index 6c3a4eb2a04d..393cfacea025 100644
--- a/src/ipa/raspberrypi/controller/rpi/agc.cpp
+++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp
@@ -172,7 +172,7 @@  Agc::Agc(Controller *controller)
 	// it's not been calculated yet (i.e. Process hasn't yet run).
 	memset(&status_, 0, sizeof(status_));
 	status_.ev = ev_;
-	memset(&last_device_status_, 0, sizeof(last_device_status_));
+	last_device_status_ = {};
 }
 
 char const *Agc::Name() const
diff --git a/src/ipa/raspberrypi/controller/rpi/lux.cpp b/src/ipa/raspberrypi/controller/rpi/lux.cpp
index f58d69397e1c..a3ae633b867a 100644
--- a/src/ipa/raspberrypi/controller/rpi/lux.cpp
+++ b/src/ipa/raspberrypi/controller/rpi/lux.cpp
@@ -61,13 +61,7 @@  void Lux::Prepare(Metadata *image_metadata)
 void Lux::Process(StatisticsPtr &stats, Metadata *image_metadata)
 {
 	// set some initial values to shut the compiler up
-	DeviceStatus device_status = {
-		.shutter_speed = 1.0ms,
-		.analogue_gain = 1.0,
-		.lens_position = 0.0,
-		.aperture = 0.0,
-		.flash_intensity = 0.0
-	};
+	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;