<div dir="ltr"><div dir="ltr">Hi Kieran,<div><br></div><div>Thank you for your review feedback.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, 9 Jul 2021 at 14:00, Kieran Bingham <<a href="mailto:kieran.bingham@ideasonboard.com">kieran.bingham@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Naush,<br>
<br>
On 09/07/2021 10:56, Naushir Patuck wrote:<br>
> The constructor sets all fields to 0. This replaces the memset(0) and default<br>
> value initialisation usage in the agc and lux controllers respectively.<br>
<br>
I guess this makes everything a bit more explicit.<br>
<br>
Is it to fix coverity warnings? or just a general improvement?<br></blockquote><div><br></div><div>It's just general improvements, mostly to allow the fixups below.</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
> Reviewed-by: David Plowman <<a href="mailto:david.plowman@raspberrypi.com" target="_blank">david.plowman@raspberrypi.com</a>><br>
> ---<br>
>  src/ipa/raspberrypi/controller/device_status.h | 6 ++++++<br>
>  src/ipa/raspberrypi/controller/rpi/agc.cpp     | 1 -<br>
>  src/ipa/raspberrypi/controller/rpi/lux.cpp     | 9 +--------<br>
>  3 files changed, 7 insertions(+), 9 deletions(-)<br>
> <br>
> diff --git a/src/ipa/raspberrypi/controller/device_status.h b/src/ipa/raspberrypi/controller/device_status.h<br>
> index 733378dbfa27..73df7ce228dd 100644<br>
> --- a/src/ipa/raspberrypi/controller/device_status.h<br>
> +++ b/src/ipa/raspberrypi/controller/device_status.h<br>
> @@ -14,6 +14,12 @@<br>
>   */<br>
>  <br>
>  struct DeviceStatus {<br>
> +     DeviceStatus()<br>
> +             : shutter_speed(std::chrono::seconds(0)), analogue_gain(0.0),<br>
> +               lens_position(0.0), aperture(0.0), flash_intensity(0.0)<br>
> +     {<br>
> +     }<br>
> +<br>
>       /* time shutter is open */<br>
>       libcamera::utils::Duration shutter_speed;<br>
>       double analogue_gain;<br>
> diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/raspberrypi/controller/rpi/agc.cpp<br>
> index 6c3a4eb2a04d..f57783f821ec 100644<br>
> --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp<br>
> +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp<br>
> @@ -172,7 +172,6 @@ Agc::Agc(Controller *controller)<br>
>       // it's not been calculated yet (i.e. Process hasn't yet run).<br>
>       memset(&status_, 0, sizeof(status_));<br>
>       status_.ev = ev_;<br>
> -     memset(&last_device_status_, 0, sizeof(last_device_status_));<br>
>  }<br>
>  <br>
>  char const *Agc::Name() const<br>
> diff --git a/src/ipa/raspberrypi/controller/rpi/lux.cpp b/src/ipa/raspberrypi/controller/rpi/lux.cpp<br>
> index f58d69397e1c..6367b17dc7f4 100644<br>
> --- a/src/ipa/raspberrypi/controller/rpi/lux.cpp<br>
> +++ b/src/ipa/raspberrypi/controller/rpi/lux.cpp<br>
> @@ -60,14 +60,7 @@ void Lux::Prepare(Metadata *image_metadata)<br>
>  <br>
>  void Lux::Process(StatisticsPtr &stats, Metadata *image_metadata)<br>
>  {<br>
> -     // set some initial values to shut the compiler up<br>
> -     DeviceStatus device_status = {<br>
> -             .shutter_speed = 1.0ms,<br>
> -             .analogue_gain = 1.0,<br>
<br>
These are now going to be set to 0, I assume that's not an issue if<br>
previously they're just default values to satisfy the compiler...<br></blockquote><div><br></div><div>Correct, we now have the explicit constructor so no need for this</div><div>ugliness!</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Reviewed-by: Kieran Bingham <<a href="mailto:kieran.bingham@ideasonboard.com" target="_blank">kieran.bingham@ideasonboard.com</a>><br>
<br>
> -             .lens_position = 0.0,<br>
> -             .aperture = 0.0,<br>
> -             .flash_intensity = 0.0<br>
> -     };<br>
> +     DeviceStatus device_status;<br>
>       if (image_metadata->Get("device.status", device_status) == 0) {<br>
>               double current_gain = device_status.analogue_gain;<br>
>               double current_aperture = device_status.aperture;<br>
> <br>
</blockquote></div></div>