| Message ID | 20240627133730.2554717-1-stefan.klug@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi Stefan, On Thu, 27 Jun 2024 at 14:37, Stefan Klug <stefan.klug@ideasonboard.com> wrote: > > The gcc used in my current buildroot (Version 12.3) errors out with > -Wmaybe-uninitialized. Fix that. > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > --- > > @Naush: Could you have a look at the rpi specific changes? Specifically > I'm not sure if the change in AwbConfig::read() is correct. It was > unclear to me if there are cases where line 125 should return 0 even > though a error got logged one line above. I think the original error handling is probably wrong and we need something like: diff --git a/src/ipa/rpi/controller/rpi/awb.cpp b/src/ipa/rpi/controller/rpi/awb.cpp index 5ae0c2fad6e9..81f700dba2c7 100644 --- a/src/ipa/rpi/controller/rpi/awb.cpp +++ b/src/ipa/rpi/controller/rpi/awb.cpp @@ -121,7 +121,7 @@ int AwbConfig::read(const libcamera::YamlObject ¶ms) } if (priors.empty()) { LOG(RPiAwb, Error) << "AwbConfig: no AWB priors configured"; - return ret; + return -EINVAL; } Feel free to make that change as part of this commt, or I can submit a separate patch. Your changes look good to me otherwise. Reviewed-by: Naushir Patuck <naush@raspberrypi.com> > > Regards, > Stefan > > > src/ipa/libipa/exposure_mode_helper.cpp | 15 ++++++--------- > src/ipa/rpi/controller/rpi/awb.cpp | 2 +- > src/libcamera/device_enumerator_sysfs.cpp | 2 +- > src/libcamera/pipeline/rpi/vc4/vc4.cpp | 2 +- > 4 files changed, 9 insertions(+), 12 deletions(-) > > diff --git a/src/ipa/libipa/exposure_mode_helper.cpp b/src/ipa/libipa/exposure_mode_helper.cpp > index 683a564a01c8..9d1387636d05 100644 > --- a/src/ipa/libipa/exposure_mode_helper.cpp > +++ b/src/ipa/libipa/exposure_mode_helper.cpp > @@ -166,7 +166,7 @@ ExposureModeHelper::splitExposure(utils::Duration exposure) const > return { minShutter_, minGain_, exposure / (minShutter_ * minGain_) }; > > utils::Duration shutter; > - double stageGain; > + double stageGain = 1.0; > double gain; > > for (unsigned int stage = 0; stage < gains_.size(); stage++) { > @@ -198,15 +198,12 @@ ExposureModeHelper::splitExposure(utils::Duration exposure) const > } > > /* > - * From here on all we can do is max out the shutter time, followed by > - * the analogue gain. If we still haven't achieved the target we send > - * the rest of the exposure time to digital gain. If we were given no > - * stages to use then set stageGain to 1.0 so that shutter time is maxed > - * before gain touched at all. > + * From here on all we can do is max out the shutter time, followed by the > + * analogue gain. If we still haven't achieved the target we send the rest > + * of the exposure time to digital gain. If we were given no stages to use > + * then the default stageGain of 1.0 is used so that shutter time is maxed > + * before gain is touched at all. > */ > - if (gains_.empty()) > - stageGain = 1.0; > - > shutter = clampShutter(exposure / clampGain(stageGain)); > gain = clampGain(exposure / shutter); > > diff --git a/src/ipa/rpi/controller/rpi/awb.cpp b/src/ipa/rpi/controller/rpi/awb.cpp > index 003c8fa137f3..3503a5ab9218 100644 > --- a/src/ipa/rpi/controller/rpi/awb.cpp > +++ b/src/ipa/rpi/controller/rpi/awb.cpp > @@ -91,7 +91,7 @@ static int readCtCurve(ipa::Pwl &ctR, ipa::Pwl &ctB, const libcamera::YamlObject > > int AwbConfig::read(const libcamera::YamlObject ¶ms) > { > - int ret; > + int ret = 0; > > bayes = params["bayes"].get<int>(1); > framePeriod = params["frame_period"].get<uint16_t>(10); > diff --git a/src/libcamera/device_enumerator_sysfs.cpp b/src/libcamera/device_enumerator_sysfs.cpp > index fc33ba52b813..7866885c0f73 100644 > --- a/src/libcamera/device_enumerator_sysfs.cpp > +++ b/src/libcamera/device_enumerator_sysfs.cpp > @@ -33,7 +33,7 @@ int DeviceEnumeratorSysfs::init() > int DeviceEnumeratorSysfs::enumerate() > { > struct dirent *ent; > - DIR *dir; > + DIR *dir = nullptr; > > static const char * const sysfs_dirs[] = { > "/sys/subsystem/media/devices", > diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp b/src/libcamera/pipeline/rpi/vc4/vc4.cpp > index 4a89e35f5d7b..e5b6ef2b37cd 100644 > --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp > +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp > @@ -802,7 +802,7 @@ void Vc4CameraData::ispInputDequeue(FrameBuffer *buffer) > void Vc4CameraData::ispOutputDequeue(FrameBuffer *buffer) > { > RPi::Stream *stream = nullptr; > - unsigned int index; > + unsigned int index = 0; > > if (!isRunning()) > return; > -- > 2.43.0 >
Hi Stefan On Thu, Jun 27, 2024 at 03:31:00PM GMT, Stefan Klug wrote: > The gcc used in my current buildroot (Version 12.3) errors out with > -Wmaybe-uninitialized. Fix that. > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > --- > > @Naush: Could you have a look at the rpi specific changes? Specifically > I'm not sure if the change in AwbConfig::read() is correct. It was > unclear to me if there are cases where line 125 should return 0 even > though a error got logged one line above. > > Regards, > Stefan > > > src/ipa/libipa/exposure_mode_helper.cpp | 15 ++++++--------- > src/ipa/rpi/controller/rpi/awb.cpp | 2 +- > src/libcamera/device_enumerator_sysfs.cpp | 2 +- > src/libcamera/pipeline/rpi/vc4/vc4.cpp | 2 +- > 4 files changed, 9 insertions(+), 12 deletions(-) > > diff --git a/src/ipa/libipa/exposure_mode_helper.cpp b/src/ipa/libipa/exposure_mode_helper.cpp > index 683a564a01c8..9d1387636d05 100644 > --- a/src/ipa/libipa/exposure_mode_helper.cpp > +++ b/src/ipa/libipa/exposure_mode_helper.cpp > @@ -166,7 +166,7 @@ ExposureModeHelper::splitExposure(utils::Duration exposure) const > return { minShutter_, minGain_, exposure / (minShutter_ * minGain_) }; > > utils::Duration shutter; > - double stageGain; > + double stageGain = 1.0; > double gain; > > for (unsigned int stage = 0; stage < gains_.size(); stage++) { > @@ -198,15 +198,12 @@ ExposureModeHelper::splitExposure(utils::Duration exposure) const > } > > /* > - * From here on all we can do is max out the shutter time, followed by > - * the analogue gain. If we still haven't achieved the target we send > - * the rest of the exposure time to digital gain. If we were given no > - * stages to use then set stageGain to 1.0 so that shutter time is maxed > - * before gain touched at all. > + * From here on all we can do is max out the shutter time, followed by the > + * analogue gain. If we still haven't achieved the target we send the rest > + * of the exposure time to digital gain. If we were given no stages to use > + * then the default stageGain of 1.0 is used so that shutter time is maxed > + * before gain is touched at all. No need to go over 80-cols > */ > - if (gains_.empty()) > - stageGain = 1.0; > - > shutter = clampShutter(exposure / clampGain(stageGain)); > gain = clampGain(exposure / shutter); > > diff --git a/src/ipa/rpi/controller/rpi/awb.cpp b/src/ipa/rpi/controller/rpi/awb.cpp > index 003c8fa137f3..3503a5ab9218 100644 > --- a/src/ipa/rpi/controller/rpi/awb.cpp > +++ b/src/ipa/rpi/controller/rpi/awb.cpp > @@ -91,7 +91,7 @@ static int readCtCurve(ipa::Pwl &ctR, ipa::Pwl &ctB, const libcamera::YamlObject > > int AwbConfig::read(const libcamera::YamlObject ¶ms) > { > - int ret; > + int ret = 0; > > bayes = params["bayes"].get<int>(1); > framePeriod = params["frame_period"].get<uint16_t>(10); There's one code path below were you could return 0 on an error condition if (params.contains("priors")) { for (const auto &p : params["priors"].asList()) { AwbPrior prior; ret = prior.read(p); if (ret) return ret; if (!priors.empty() && prior.lux <= priors.back().lux) { LOG(RPiAwb, Error) << "AwbConfig: Prior must be ordered in increasing lux value"; return -EINVAL; } priors.push_back(prior); } if (priors.empty()) { LOG(RPiAwb, Error) << "AwbConfig: no AWB priors configured"; return ret; <---- HERE } While at it, could you return -EINVAL here ? > diff --git a/src/libcamera/device_enumerator_sysfs.cpp b/src/libcamera/device_enumerator_sysfs.cpp > index fc33ba52b813..7866885c0f73 100644 > --- a/src/libcamera/device_enumerator_sysfs.cpp > +++ b/src/libcamera/device_enumerator_sysfs.cpp > @@ -33,7 +33,7 @@ int DeviceEnumeratorSysfs::init() > int DeviceEnumeratorSysfs::enumerate() > { > struct dirent *ent; > - DIR *dir; > + DIR *dir = nullptr; > > static const char * const sysfs_dirs[] = { > "/sys/subsystem/media/devices", > diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp b/src/libcamera/pipeline/rpi/vc4/vc4.cpp > index 4a89e35f5d7b..e5b6ef2b37cd 100644 > --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp > +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp > @@ -802,7 +802,7 @@ void Vc4CameraData::ispInputDequeue(FrameBuffer *buffer) > void Vc4CameraData::ispOutputDequeue(FrameBuffer *buffer) > { > RPi::Stream *stream = nullptr; > - unsigned int index; > + unsigned int index = 0; > > if (!isRunning()) > return; These two look good! Thanks j > -- > 2.43.0 >
Hi Jacopo, thanks for the review. On Fri, Jun 28, 2024 at 09:17:34AM +0200, Jacopo Mondi wrote: > Hi Stefan > > On Thu, Jun 27, 2024 at 03:31:00PM GMT, Stefan Klug wrote: > > The gcc used in my current buildroot (Version 12.3) errors out with > > -Wmaybe-uninitialized. Fix that. > > > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > > --- > > > > @Naush: Could you have a look at the rpi specific changes? Specifically > > I'm not sure if the change in AwbConfig::read() is correct. It was > > unclear to me if there are cases where line 125 should return 0 even > > though a error got logged one line above. > > > > Regards, > > Stefan > > > > > > src/ipa/libipa/exposure_mode_helper.cpp | 15 ++++++--------- > > src/ipa/rpi/controller/rpi/awb.cpp | 2 +- > > src/libcamera/device_enumerator_sysfs.cpp | 2 +- > > src/libcamera/pipeline/rpi/vc4/vc4.cpp | 2 +- > > 4 files changed, 9 insertions(+), 12 deletions(-) > > > > diff --git a/src/ipa/libipa/exposure_mode_helper.cpp b/src/ipa/libipa/exposure_mode_helper.cpp > > index 683a564a01c8..9d1387636d05 100644 > > --- a/src/ipa/libipa/exposure_mode_helper.cpp > > +++ b/src/ipa/libipa/exposure_mode_helper.cpp > > @@ -166,7 +166,7 @@ ExposureModeHelper::splitExposure(utils::Duration exposure) const > > return { minShutter_, minGain_, exposure / (minShutter_ * minGain_) }; > > > > utils::Duration shutter; > > - double stageGain; > > + double stageGain = 1.0; > > double gain; > > > > for (unsigned int stage = 0; stage < gains_.size(); stage++) { > > @@ -198,15 +198,12 @@ ExposureModeHelper::splitExposure(utils::Duration exposure) const > > } > > > > /* > > - * From here on all we can do is max out the shutter time, followed by > > - * the analogue gain. If we still haven't achieved the target we send > > - * the rest of the exposure time to digital gain. If we were given no > > - * stages to use then set stageGain to 1.0 so that shutter time is maxed > > - * before gain touched at all. > > + * From here on all we can do is max out the shutter time, followed by the > > + * analogue gain. If we still haven't achieved the target we send the rest > > + * of the exposure time to digital gain. If we were given no stages to use > > + * then the default stageGain of 1.0 is used so that shutter time is maxed > > + * before gain is touched at all. > > No need to go over 80-cols Arg, right. My tab width was incorrect, therfore the 80cols where different ones... will fix. > > > */ > > - if (gains_.empty()) > > - stageGain = 1.0; > > - > > shutter = clampShutter(exposure / clampGain(stageGain)); > > gain = clampGain(exposure / shutter); > > > > diff --git a/src/ipa/rpi/controller/rpi/awb.cpp b/src/ipa/rpi/controller/rpi/awb.cpp > > index 003c8fa137f3..3503a5ab9218 100644 > > --- a/src/ipa/rpi/controller/rpi/awb.cpp > > +++ b/src/ipa/rpi/controller/rpi/awb.cpp > > @@ -91,7 +91,7 @@ static int readCtCurve(ipa::Pwl &ctR, ipa::Pwl &ctB, const libcamera::YamlObject > > > > int AwbConfig::read(const libcamera::YamlObject ¶ms) > > { > > - int ret; > > + int ret = 0; > > > > bayes = params["bayes"].get<int>(1); > > framePeriod = params["frame_period"].get<uint16_t>(10); > > There's one code path below were you could return 0 on an error > condition > > if (params.contains("priors")) { > for (const auto &p : params["priors"].asList()) { > AwbPrior prior; > ret = prior.read(p); > if (ret) > return ret; > if (!priors.empty() && prior.lux <= priors.back().lux) { > LOG(RPiAwb, Error) << "AwbConfig: Prior must be ordered in increasing lux value"; > return -EINVAL; > } > priors.push_back(prior); > } > if (priors.empty()) { > LOG(RPiAwb, Error) << "AwbConfig: no AWB priors configured"; > return ret; <---- HERE > } > > While at it, could you return -EINVAL here ? That was the thing I asked Naushir about. I will modify it that way. Cheers, Stefan > > > diff --git a/src/libcamera/device_enumerator_sysfs.cpp b/src/libcamera/device_enumerator_sysfs.cpp > > index fc33ba52b813..7866885c0f73 100644 > > --- a/src/libcamera/device_enumerator_sysfs.cpp > > +++ b/src/libcamera/device_enumerator_sysfs.cpp > > @@ -33,7 +33,7 @@ int DeviceEnumeratorSysfs::init() > > int DeviceEnumeratorSysfs::enumerate() > > { > > struct dirent *ent; > > - DIR *dir; > > + DIR *dir = nullptr; > > > > static const char * const sysfs_dirs[] = { > > "/sys/subsystem/media/devices", > > diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp b/src/libcamera/pipeline/rpi/vc4/vc4.cpp > > index 4a89e35f5d7b..e5b6ef2b37cd 100644 > > --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp > > +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp > > @@ -802,7 +802,7 @@ void Vc4CameraData::ispInputDequeue(FrameBuffer *buffer) > > void Vc4CameraData::ispOutputDequeue(FrameBuffer *buffer) > > { > > RPi::Stream *stream = nullptr; > > - unsigned int index; > > + unsigned int index = 0; > > > > if (!isRunning()) > > return; > > These two look good! > > Thanks > j > > > -- > > 2.43.0 > >
diff --git a/src/ipa/libipa/exposure_mode_helper.cpp b/src/ipa/libipa/exposure_mode_helper.cpp index 683a564a01c8..9d1387636d05 100644 --- a/src/ipa/libipa/exposure_mode_helper.cpp +++ b/src/ipa/libipa/exposure_mode_helper.cpp @@ -166,7 +166,7 @@ ExposureModeHelper::splitExposure(utils::Duration exposure) const return { minShutter_, minGain_, exposure / (minShutter_ * minGain_) }; utils::Duration shutter; - double stageGain; + double stageGain = 1.0; double gain; for (unsigned int stage = 0; stage < gains_.size(); stage++) { @@ -198,15 +198,12 @@ ExposureModeHelper::splitExposure(utils::Duration exposure) const } /* - * From here on all we can do is max out the shutter time, followed by - * the analogue gain. If we still haven't achieved the target we send - * the rest of the exposure time to digital gain. If we were given no - * stages to use then set stageGain to 1.0 so that shutter time is maxed - * before gain touched at all. + * From here on all we can do is max out the shutter time, followed by the + * analogue gain. If we still haven't achieved the target we send the rest + * of the exposure time to digital gain. If we were given no stages to use + * then the default stageGain of 1.0 is used so that shutter time is maxed + * before gain is touched at all. */ - if (gains_.empty()) - stageGain = 1.0; - shutter = clampShutter(exposure / clampGain(stageGain)); gain = clampGain(exposure / shutter); diff --git a/src/ipa/rpi/controller/rpi/awb.cpp b/src/ipa/rpi/controller/rpi/awb.cpp index 003c8fa137f3..3503a5ab9218 100644 --- a/src/ipa/rpi/controller/rpi/awb.cpp +++ b/src/ipa/rpi/controller/rpi/awb.cpp @@ -91,7 +91,7 @@ static int readCtCurve(ipa::Pwl &ctR, ipa::Pwl &ctB, const libcamera::YamlObject int AwbConfig::read(const libcamera::YamlObject ¶ms) { - int ret; + int ret = 0; bayes = params["bayes"].get<int>(1); framePeriod = params["frame_period"].get<uint16_t>(10); diff --git a/src/libcamera/device_enumerator_sysfs.cpp b/src/libcamera/device_enumerator_sysfs.cpp index fc33ba52b813..7866885c0f73 100644 --- a/src/libcamera/device_enumerator_sysfs.cpp +++ b/src/libcamera/device_enumerator_sysfs.cpp @@ -33,7 +33,7 @@ int DeviceEnumeratorSysfs::init() int DeviceEnumeratorSysfs::enumerate() { struct dirent *ent; - DIR *dir; + DIR *dir = nullptr; static const char * const sysfs_dirs[] = { "/sys/subsystem/media/devices", diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp b/src/libcamera/pipeline/rpi/vc4/vc4.cpp index 4a89e35f5d7b..e5b6ef2b37cd 100644 --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp @@ -802,7 +802,7 @@ void Vc4CameraData::ispInputDequeue(FrameBuffer *buffer) void Vc4CameraData::ispOutputDequeue(FrameBuffer *buffer) { RPi::Stream *stream = nullptr; - unsigned int index; + unsigned int index = 0; if (!isRunning()) return;
The gcc used in my current buildroot (Version 12.3) errors out with -Wmaybe-uninitialized. Fix that. Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> --- @Naush: Could you have a look at the rpi specific changes? Specifically I'm not sure if the change in AwbConfig::read() is correct. It was unclear to me if there are cases where line 125 should return 0 even though a error got logged one line above. Regards, Stefan src/ipa/libipa/exposure_mode_helper.cpp | 15 ++++++--------- src/ipa/rpi/controller/rpi/awb.cpp | 2 +- src/libcamera/device_enumerator_sysfs.cpp | 2 +- src/libcamera/pipeline/rpi/vc4/vc4.cpp | 2 +- 4 files changed, 9 insertions(+), 12 deletions(-)