[libcamera-devel] [PATCH v2 3/3] ipa: raspberrypi: fix missing initialize of status_

[David Plowman]

Hi Tomi

Thanks for this patch. Actually I'm OK for this to go in subject to a
couple of very minor nitpicks... (sorry!)

On Wed, 7 Oct 2020 at 14:18, Tomi Valkeinen <tomi.valkeinen at iki.fi> wrote:
>
> Many fields in status_ are not initialized, causing use of uninitialized
> memory.
>
> Drop the code that clears some of the individual fields, and instead
> just memset the whole thing.
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen at iki.fi>
> ---
>  src/ipa/raspberrypi/controller/rpi/agc.cpp | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/raspberrypi/controller/rpi/agc.cpp
> index df4d364..6ae774d 100644
> --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp
> +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp
> @@ -148,14 +148,14 @@ Agc::Agc(Controller *controller)
>           exposure_mode_(nullptr), constraint_mode_(nullptr),
>           frame_count_(0), lock_count_(0)
>  {
> -       ev_ = status_.ev = 1.0;
> -       flicker_period_ = status_.flicker_period = 0.0;
> -       fixed_shutter_ = status_.fixed_shutter = 0;
> -       fixed_analogue_gain_ = status_.fixed_analogue_gain = 0.0;
> -       // set to zero initially, so we can tell it's not been calculated

We could keep the comment here, perhaps. Maybe

// set status_.total_exposure_value to zero initially, so we can tell
it's not been calculated

> -       status_.total_exposure_value = 0.0;
> -       status_.target_exposure_value = 0.0;
> -       status_.locked = false;
> +       memset(&status_, 0, sizeof(status_));
> +       status_.ev = 1.0;
> +
> +       ev_ = status_.ev;
> +       flicker_period_ = status_.flicker_period;
> +       fixed_shutter_ = status_.fixed_shutter;
> +       fixed_analogue_gain_ = status_.fixed_analogue_gain;

I'd prefer to set the values explicitly here, i.e. 1.0 or 0.0 rather
than to copy them. I know it makes no difference, but the intent is
that, for example, "fixed_shutter_" should be zero, not "whatever
status_.fixed-shutter is". I know I'm splitting hairs, for which I
apologise. But I don't feel that strongly whether we change this or
not, so

Reviewed-by: David Plowman <david.plowman at raspberrypi.com>

Thanks very much for the fixes!

Best regards
David

> +
>         output_status_ = status_;
>  }
>
> --
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
>