libcamera: fix maybe-uninitialized error
diff mbox series

Message ID 20240627133730.2554717-1-stefan.klug@ideasonboard.com
State Superseded
Headers show
Series
  • libcamera: fix maybe-uninitialized error
Related show

Commit Message

Stefan Klug June 27, 2024, 1:31 p.m. UTC
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(-)

Comments

Naushir Patuck June 28, 2024, 7:02 a.m. UTC | #1
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 &params)
                }
                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 &params)
>  {
> -       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
>
Jacopo Mondi June 28, 2024, 7:17 a.m. UTC | #2
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 &params)
>  {
> -	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
>
Stefan Klug June 28, 2024, 9:25 a.m. UTC | #3
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 &params)
> >  {
> > -	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
> >

Patch
diff mbox series

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 &params)
 {
-	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;