| Message ID | 20201007110743.55561-3-tomi.valkeinen@iki.fi |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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@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@ideasonboard.com> > - status_.total_exposure_value = 0.0; > - status_.target_exposure_value = 0.0; > - status_.locked = false; > output_status_ = status_; > } > >
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@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@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 ;-) Kieran > > >> - status_.total_exposure_value = 0.0; >> - status_.target_exposure_value = 0.0; >> - status_.locked = false; >> output_status_ = status_; >> } >> >> >
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@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@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
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@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@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@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
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 - status_.total_exposure_value = 0.0; - status_.target_exposure_value = 0.0; - status_.locked = false; output_status_ = status_; }
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@iki.fi> --- src/ipa/raspberrypi/controller/rpi/agc.cpp | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-)