| Message ID | 20201007131812.2688973-4-tomi.valkeinen@iki.fi |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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@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@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@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 >
Hi David, On 07/10/2020 16:48, David Plowman wrote: > Hi Tomi > > Thanks for this patch. Actually I'm OK for this to go in subject to a > couple of very minor nitpicks... (sorry!) Well, you could consider these patches more like bug reports than real patches, as I have no idea what the code is doing =). If you're already working on agc.cpp, I'm fine with you fixing this in your series however you see best. I just wanted to get rid of all the valgrind warnings so that I could see if my code causes any. Tomi
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 - 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; + 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 | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)