Message ID | 20210702150940.226941-3-naush@raspberrypi.com |
---|---|
State | Superseded |
Headers | show |
Series |
|
Related | show |
Hi Naush Thanks for tidying this up! On Fri, 2 Jul 2021 at 16:09, Naushir Patuck <naush@raspberrypi.com> wrote: > > The constructor sets all fields to 0. This replaces the memset(0) and default > value initialisation usage in the agc and lux controllers respectively. > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > --- > src/ipa/raspberrypi/controller/device_status.h | 6 ++++++ > src/ipa/raspberrypi/controller/rpi/agc.cpp | 2 +- > src/ipa/raspberrypi/controller/rpi/lux.cpp | 8 +------- > 3 files changed, 8 insertions(+), 8 deletions(-) > > diff --git a/src/ipa/raspberrypi/controller/device_status.h b/src/ipa/raspberrypi/controller/device_status.h > index 733378dbfa27..73df7ce228dd 100644 > --- a/src/ipa/raspberrypi/controller/device_status.h > +++ b/src/ipa/raspberrypi/controller/device_status.h > @@ -14,6 +14,12 @@ > */ > > struct DeviceStatus { > + DeviceStatus() > + : shutter_speed(std::chrono::seconds(0)), analogue_gain(0.0), > + lens_position(0.0), aperture(0.0), flash_intensity(0.0) > + { > + } > + > /* time shutter is open */ > libcamera::utils::Duration shutter_speed; > double analogue_gain; > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/raspberrypi/controller/rpi/agc.cpp > index 6c3a4eb2a04d..393cfacea025 100644 > --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp > +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp > @@ -172,7 +172,7 @@ Agc::Agc(Controller *controller) > // it's not been calculated yet (i.e. Process hasn't yet run). > memset(&status_, 0, sizeof(status_)); > status_.ev = ev_; > - memset(&last_device_status_, 0, sizeof(last_device_status_)); > + last_device_status_ = {}; I only wondered slightly whether this could be left out and we rely on the new constructor iniitialising last_device_stats_ automatically? > } > > char const *Agc::Name() const > diff --git a/src/ipa/raspberrypi/controller/rpi/lux.cpp b/src/ipa/raspberrypi/controller/rpi/lux.cpp > index f58d69397e1c..a3ae633b867a 100644 > --- a/src/ipa/raspberrypi/controller/rpi/lux.cpp > +++ b/src/ipa/raspberrypi/controller/rpi/lux.cpp > @@ -61,13 +61,7 @@ void Lux::Prepare(Metadata *image_metadata) > void Lux::Process(StatisticsPtr &stats, Metadata *image_metadata) > { > // set some initial values to shut the compiler up > - DeviceStatus device_status = { > - .shutter_speed = 1.0ms, > - .analogue_gain = 1.0, > - .lens_position = 0.0, > - .aperture = 0.0, > - .flash_intensity = 0.0 > - }; > + DeviceStatus device_status; Maybe we can remove my slightly off-hand remark about shutting up the compiler too? Notwithstanding those very minor things: Reviewed-by: David Plowman <david.plowman@raspberrypi.com> Thanks David > if (image_metadata->Get("device.status", device_status) == 0) { > double current_gain = device_status.analogue_gain; > double current_aperture = device_status.aperture; > -- > 2.25.1 >
Hi David, Thank you for your review feedback. On Fri, 2 Jul 2021 at 16:45, David Plowman <david.plowman@raspberrypi.com> wrote: > Hi Naush > > Thanks for tidying this up! > > On Fri, 2 Jul 2021 at 16:09, Naushir Patuck <naush@raspberrypi.com> wrote: > > > > The constructor sets all fields to 0. This replaces the memset(0) and > default > > value initialisation usage in the agc and lux controllers respectively. > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > --- > > src/ipa/raspberrypi/controller/device_status.h | 6 ++++++ > > src/ipa/raspberrypi/controller/rpi/agc.cpp | 2 +- > > src/ipa/raspberrypi/controller/rpi/lux.cpp | 8 +------- > > 3 files changed, 8 insertions(+), 8 deletions(-) > > > > diff --git a/src/ipa/raspberrypi/controller/device_status.h > b/src/ipa/raspberrypi/controller/device_status.h > > index 733378dbfa27..73df7ce228dd 100644 > > --- a/src/ipa/raspberrypi/controller/device_status.h > > +++ b/src/ipa/raspberrypi/controller/device_status.h > > @@ -14,6 +14,12 @@ > > */ > > > > struct DeviceStatus { > > + DeviceStatus() > > + : shutter_speed(std::chrono::seconds(0)), > analogue_gain(0.0), > > + lens_position(0.0), aperture(0.0), flash_intensity(0.0) > > + { > > + } > > + > > /* time shutter is open */ > > libcamera::utils::Duration shutter_speed; > > double analogue_gain; > > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp > b/src/ipa/raspberrypi/controller/rpi/agc.cpp > > index 6c3a4eb2a04d..393cfacea025 100644 > > --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp > > +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp > > @@ -172,7 +172,7 @@ Agc::Agc(Controller *controller) > > // it's not been calculated yet (i.e. Process hasn't yet run). > > memset(&status_, 0, sizeof(status_)); > > status_.ev = ev_; > > - memset(&last_device_status_, 0, sizeof(last_device_status_)); > > + last_device_status_ = {}; > > I only wondered slightly whether this could be left out and we rely on > the new constructor iniitialising last_device_stats_ automatically? > Oops, yes we can! > > > } > > > > char const *Agc::Name() const > > diff --git a/src/ipa/raspberrypi/controller/rpi/lux.cpp > b/src/ipa/raspberrypi/controller/rpi/lux.cpp > > index f58d69397e1c..a3ae633b867a 100644 > > --- a/src/ipa/raspberrypi/controller/rpi/lux.cpp > > +++ b/src/ipa/raspberrypi/controller/rpi/lux.cpp > > @@ -61,13 +61,7 @@ void Lux::Prepare(Metadata *image_metadata) > > void Lux::Process(StatisticsPtr &stats, Metadata *image_metadata) > > { > > // set some initial values to shut the compiler up > > - DeviceStatus device_status = { > > - .shutter_speed = 1.0ms, > > - .analogue_gain = 1.0, > > - .lens_position = 0.0, > > - .aperture = 0.0, > > - .flash_intensity = 0.0 > > - }; > > + DeviceStatus device_status; > > Maybe we can remove my slightly off-hand remark about shutting up the > compiler too? > Will remove the comment in the next revision. Regards, Naush > > Notwithstanding those very minor things: > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> > > Thanks > David > > > if (image_metadata->Get("device.status", device_status) == 0) { > > double current_gain = device_status.analogue_gain; > > double current_aperture = device_status.aperture; > > -- > > 2.25.1 > > >
diff --git a/src/ipa/raspberrypi/controller/device_status.h b/src/ipa/raspberrypi/controller/device_status.h index 733378dbfa27..73df7ce228dd 100644 --- a/src/ipa/raspberrypi/controller/device_status.h +++ b/src/ipa/raspberrypi/controller/device_status.h @@ -14,6 +14,12 @@ */ struct DeviceStatus { + DeviceStatus() + : shutter_speed(std::chrono::seconds(0)), analogue_gain(0.0), + lens_position(0.0), aperture(0.0), flash_intensity(0.0) + { + } + /* time shutter is open */ libcamera::utils::Duration shutter_speed; double analogue_gain; diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/raspberrypi/controller/rpi/agc.cpp index 6c3a4eb2a04d..393cfacea025 100644 --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp @@ -172,7 +172,7 @@ Agc::Agc(Controller *controller) // it's not been calculated yet (i.e. Process hasn't yet run). memset(&status_, 0, sizeof(status_)); status_.ev = ev_; - memset(&last_device_status_, 0, sizeof(last_device_status_)); + last_device_status_ = {}; } char const *Agc::Name() const diff --git a/src/ipa/raspberrypi/controller/rpi/lux.cpp b/src/ipa/raspberrypi/controller/rpi/lux.cpp index f58d69397e1c..a3ae633b867a 100644 --- a/src/ipa/raspberrypi/controller/rpi/lux.cpp +++ b/src/ipa/raspberrypi/controller/rpi/lux.cpp @@ -61,13 +61,7 @@ void Lux::Prepare(Metadata *image_metadata) void Lux::Process(StatisticsPtr &stats, Metadata *image_metadata) { // set some initial values to shut the compiler up - DeviceStatus device_status = { - .shutter_speed = 1.0ms, - .analogue_gain = 1.0, - .lens_position = 0.0, - .aperture = 0.0, - .flash_intensity = 0.0 - }; + DeviceStatus device_status; if (image_metadata->Get("device.status", device_status) == 0) { double current_gain = device_status.analogue_gain; double current_aperture = device_status.aperture;
The constructor sets all fields to 0. This replaces the memset(0) and default value initialisation usage in the agc and lux controllers respectively. Signed-off-by: Naushir Patuck <naush@raspberrypi.com> --- src/ipa/raspberrypi/controller/device_status.h | 6 ++++++ src/ipa/raspberrypi/controller/rpi/agc.cpp | 2 +- src/ipa/raspberrypi/controller/rpi/lux.cpp | 8 +------- 3 files changed, 8 insertions(+), 8 deletions(-)