[libcamera-devel] [PATCH 2/4] ipa: raspberrypi: fix missing initialize of status_

[David Plowman]

Hi Tomi

Thank you for this patch. You are indeed correct, it is much tidier.

However, I'd quite like to hold off with this patch (and the related
patch 4/4) as I'm currently doing some maintenance on the AGC and
would like to avoid a conflict! As part of that maintenance I'll be
sure to grab the memset here, though I may leave the other
initialisations here (rather than move them to the header file) as
that seems to be the current style.

So please be sure to check I've done the right thing when I submit my
set of AGC patches, hopefully early next week.

Thanks and best regards
David

On Wed, 7 Oct 2020 at 13:49, Tomi Valkeinen <tomi.valkeinen at iki.fi> wrote:
>
> On 07/10/2020 15:48, Kieran Bingham wrote:
> > On 07/10/2020 13:35, Kieran Bingham wrote:
> >> Hi Tomi,
> >>
> >> On 07/10/2020 12:07, Tomi Valkeinen 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.
> >>>
> >>
> >> I think David/Naush' input will be important on this one.
> >>
> >>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen at iki.fi>
> >>> ---
> >>>  src/ipa/raspberrypi/controller/rpi/agc.cpp | 8 +-------
> >>>  1 file changed, 1 insertion(+), 7 deletions(-)
> >>>
> >>> diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/raspberrypi/controller/rpi/agc.cpp
> >>> index df4d364..9e75178 100644
> >>> --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp
> >>> +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp
> >>> @@ -148,14 +148,8 @@ Agc::Agc(Controller *controller)
> >>>       exposure_mode_(nullptr), constraint_mode_(nullptr),
> >>>       frame_count_(0), lock_count_(0)
> >>>  {
> >>> +   memset(&status_, 0, sizeof(status_));
> >>>     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
> >>
> >> Is there any benefit to retaining this comment in some adjusted form?
> >>
> >> Otherwise, I think generally clearing/initialising the whole status
> >> sounds good.
> >>
> >> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> >
> > Ahem, coming back to correct myself - This patch introduces a defect.
> > However you fix it in 4/4 ... so ... well ... There's a problem. But
> > you've fixed it ;-)
>
> Oh, indeed. I was a bit too hasty =).
>
>  Tomi