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

12793 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>
---
 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(-)

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
>

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