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

[Naushir Patuck]

Hi Kieran,

Thank you for your review feedback.

On Fri, 9 Jul 2021 at 14:00, Kieran Bingham <kieran.bingham at 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 at raspberrypi.com>
> > Reviewed-by: David Plowman <david.plowman at 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 at 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;
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210709/d7f0ddea/attachment-0001.htm>