<div dir="ltr"><div dir="ltr">Hi David,<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, 2 Jul 2021 at 16:45, David Plowman <<a href="mailto:david.plowman@raspberrypi.com">david.plowman@raspberrypi.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>
Thanks for tidying this up!<br>
<br>
On Fri, 2 Jul 2021 at 16:09, Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>> wrote:<br>
><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>
> Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
> ---<br>
>  src/ipa/raspberrypi/controller/device_status.h | 6 ++++++<br>
>  src/ipa/raspberrypi/controller/rpi/agc.cpp     | 2 +-<br>
>  src/ipa/raspberrypi/controller/rpi/lux.cpp     | 8 +-------<br>
>  3 files changed, 8 insertions(+), 8 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..393cfacea025 100644<br>
> --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp<br>
> +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp<br>
> @@ -172,7 +172,7 @@ 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>
> +       last_device_status_ = {};<br>
<br>
I only wondered slightly whether this could be left out and we rely on<br>
the new constructor iniitialising last_device_stats_ automatically?<br></blockquote><div><br></div><div>Oops, yes we can!</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">
<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..a3ae633b867a 100644<br>
> --- a/src/ipa/raspberrypi/controller/rpi/lux.cpp<br>
> +++ b/src/ipa/raspberrypi/controller/rpi/lux.cpp<br>
> @@ -61,13 +61,7 @@ void Lux::Prepare(Metadata *image_metadata)<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>
> -               .lens_position = 0.0,<br>
> -               .aperture = 0.0,<br>
> -               .flash_intensity = 0.0<br>
> -       };<br>
> +       DeviceStatus device_status;<br>
<br>
Maybe we can remove my slightly off-hand remark about shutting up the<br>
compiler too?<br></blockquote><div><br></div><div>Will remove the comment in the next revision.</div><div><br></div><div>Regards,</div><div>Naush</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">
<br>
Notwithstanding those very minor things:<br>
<br>
Reviewed-by: David Plowman <<a href="mailto:david.plowman@raspberrypi.com" target="_blank">david.plowman@raspberrypi.com</a>><br>
<br>
Thanks<br>
David<br>
<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>
> 2.25.1<br>
><br>
</blockquote></div></div>