[libcamera-devel,v5,2/8] ipa: raspberrypi: Add a constructor struct DeviceStatus

12873 diff mbox series

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>
Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
---
 src/ipa/raspberrypi/controller/device_status.h | 6 ++++++
 src/ipa/raspberrypi/controller/rpi/agc.cpp     | 1 -
 src/ipa/raspberrypi/controller/rpi/lux.cpp     | 9 +--------
 3 files changed, 7 insertions(+), 9 deletions(-)

Hi Naush,

On 09/07/2021 10:56, Naushir Patuck 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.

I guess this makes everything a bit more explicit.

Is it to fix coverity warnings? or just a general improvement?

> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
> ---
>  src/ipa/raspberrypi/controller/device_status.h | 6 ++++++
>  src/ipa/raspberrypi/controller/rpi/agc.cpp     | 1 -
>  src/ipa/raspberrypi/controller/rpi/lux.cpp     | 9 +--------
>  3 files changed, 7 insertions(+), 9 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..f57783f821ec 100644
> --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp
> +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp
> @@ -172,7 +172,6 @@ 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_));
>  }
>  
>  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..6367b17dc7f4 100644
> --- a/src/ipa/raspberrypi/controller/rpi/lux.cpp
> +++ b/src/ipa/raspberrypi/controller/rpi/lux.cpp
> @@ -60,14 +60,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,

These are now going to be set to 0, I assume that's not an issue if
previously they're just default values to satisfy the compiler...

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> -		.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;
>

Hi Kieran,

Thank you for your review feedback.

On Fri, 9 Jul 2021 at 14:00, Kieran Bingham <kieran.bingham@ideasonboard.com>
wrote:

> Hi Naush,
>
> On 09/07/2021 10:56, Naushir Patuck 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.
>
> I guess this makes everything a bit more explicit.
>
> Is it to fix coverity warnings? or just a general improvement?
>

It's just general improvements, mostly to allow the fixups below.



> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
> > ---
> >  src/ipa/raspberrypi/controller/device_status.h | 6 ++++++
> >  src/ipa/raspberrypi/controller/rpi/agc.cpp     | 1 -
> >  src/ipa/raspberrypi/controller/rpi/lux.cpp     | 9 +--------
> >  3 files changed, 7 insertions(+), 9 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..f57783f821ec 100644
> > --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp
> > +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp
> > @@ -172,7 +172,6 @@ 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_));
> >  }
> >
> >  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..6367b17dc7f4 100644
> > --- a/src/ipa/raspberrypi/controller/rpi/lux.cpp
> > +++ b/src/ipa/raspberrypi/controller/rpi/lux.cpp
> > @@ -60,14 +60,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,
>
> These are now going to be set to 0, I assume that's not an issue if
> previously they're just default values to satisfy the compiler...
>

Correct, we now have the explicit constructor so no need for this
ugliness!


> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>
> > -             .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;
> >
>