[v1] libcamera: camera: Fix clearing of stream associations before `validate()`
diff mbox series

Message ID 20241125000834.396815-1-pobrn@protonmail.com
State Superseded
Headers show
Series
  • [v1] libcamera: camera: Fix clearing of stream associations before `validate()`
Related show

Commit Message

Barnabás Pőcze Nov. 25, 2024, 12:08 a.m. UTC
A copy is made in the range-based for loop, and thus `setStream()`
operates on this copy, leading to the `stream_` member of the given
`StreamConfiguration` object in `*config` never being set to `nullptr`.

Fix that by taking a reference in the range-based for loop.

Also rename the variable from `it` since it is not an iterator.

Fixes: 4217c9f1aa863c ("libcamera: camera: Zero streams before validate()")
---
 src/libcamera/camera.cpp | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Kieran Bingham Nov. 25, 2024, 12:17 p.m. UTC | #1
Quoting Barnabás Pőcze (2024-11-25 00:08:37)
> A copy is made in the range-based for loop, and thus `setStream()`
> operates on this copy, leading to the `stream_` member of the given
> `StreamConfiguration` object in `*config` never being set to `nullptr`.
> 
> Fix that by taking a reference in the range-based for loop.
> 
> Also rename the variable from `it` since it is not an iterator.
> 
> Fixes: 4217c9f1aa863c ("libcamera: camera: Zero streams before validate()")

Ouch - I would not have spotted that!

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> ---
>  src/libcamera/camera.cpp | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> index 25135d46..82a5186a 100644
> --- a/src/libcamera/camera.cpp
> +++ b/src/libcamera/camera.cpp
> @@ -1178,8 +1178,8 @@ int Camera::configure(CameraConfiguration *config)
>         if (ret < 0)
>                 return ret;
>  
> -       for (auto it : *config)
> -               it.setStream(nullptr);
> +       for (auto &cfg : *config)
> +               cfg.setStream(nullptr);
>  
>         if (config->validate() != CameraConfiguration::Valid) {
>                 LOG(Camera, Error)
> -- 
> 2.47.0
> 
>
Laurent Pinchart Nov. 25, 2024, 10:48 p.m. UTC | #2
Hi Barnabás,

Thank you for the patch.

On Mon, Nov 25, 2024 at 12:08:37AM +0000, Barnabás Pőcze wrote:
> A copy is made in the range-based for loop, and thus `setStream()`
> operates on this copy, leading to the `stream_` member of the given
> `StreamConfiguration` object in `*config` never being set to `nullptr`.
> 
> Fix that by taking a reference in the range-based for loop.
> 
> Also rename the variable from `it` since it is not an iterator.
> 
> Fixes: 4217c9f1aa863c ("libcamera: camera: Zero streams before validate()")

Missing SoB line.

> ---
>  src/libcamera/camera.cpp | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> index 25135d46..82a5186a 100644
> --- a/src/libcamera/camera.cpp
> +++ b/src/libcamera/camera.cpp
> @@ -1178,8 +1178,8 @@ int Camera::configure(CameraConfiguration *config)
>  	if (ret < 0)
>  		return ret;
>  
> -	for (auto it : *config)
> -		it.setStream(nullptr);
> +	for (auto &cfg : *config)
> +		cfg.setStream(nullptr);

That's a worthy fix indeed, but I'm annoyed the compiler didn't tell us.
Or worse, that it won't tell us in the future if this occurs again.

It turns out we can do better by marking the copy constructor explicit:

diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h
index 071b71698acb..96b1ef36702e 100644
--- a/include/libcamera/stream.h
+++ b/include/libcamera/stream.h
@@ -41,6 +41,12 @@ struct StreamConfiguration {
 	StreamConfiguration();
 	StreamConfiguration(const StreamFormats &formats);

+	explicit StreamConfiguration(const StreamConfiguration &cfg) = default;
+	StreamConfiguration(StreamConfiguration &&cfg) = default;
+
+	StreamConfiguration &operator=(const StreamConfiguration &cfg) = default;
+	StreamConfiguration &operator=(StreamConfiguration &&cfg) = default;
+
 	PixelFormat pixelFormat;
 	Size size;
 	unsigned int stride;
diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp
index 1f75dbbc5b64..e34a112e7b01 100644
--- a/src/libcamera/stream.cpp
+++ b/src/libcamera/stream.cpp
@@ -294,6 +294,34 @@ StreamConfiguration::StreamConfiguration(const StreamFormats &formats)
 {
 }

+/**
+ * \fn StreamConfiguration::StreamConfiguration(const StreamConfiguration &other)
+ * \brief Copy constructor, create a StreamConfiguration from a copy of \a other
+ * \param[in] other The other StreamConfiguration
+ */
+
+/**
+ * \fn StreamConfiguration::StreamConfiguration(StreamConfiguration &&other)
+ * \brief Move constructor, create a StreamConfiguration by taking over \a other
+ * \param[in] other The other StreamConfiguration
+ */
+
+/**
+ * \fn StreamConfiguration::operator=(const StreamConfiguration &other)
+ * \brief Copy assignment operator, replace the StreamConfiguration with a copy
+ * of \a other
+ * \param[in] other The other StreamConfiguration
+ * \return This StreamConfiguration
+ */
+
+/**
+ * \fn StreamConfiguration::operator=(StreamConfiguration &&other)
+ * \brief Move assignment operator, replace the StreamConfiguration by taking
+ * over \a other
+ * \param[in] other The other StreamConfiguration
+ * \return This StreamConfiguration
+ */
+
 /**
  * \var StreamConfiguration::size
  * \brief Stream size in pixels

(We could possibly guard the functions with #ifndef __DOXYGEN__ in the
header file instead of documenting them.)

We will then need to make use of the explicit copy constructor or the
compiler won't be happy:

diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
index 1d68540d7e50..83b7249b88e5 100644
--- a/src/android/camera_stream.cpp
+++ b/src/android/camera_stream.cpp
@@ -87,7 +87,7 @@ int CameraStream::configure()
 	if (type_ == Type::Internal || type_ == Type::Mapped) {
 		const PixelFormat outFormat =
 			cameraDevice_->capabilities()->toPixelFormat(camera3Stream_->format);
-		StreamConfiguration output = configuration();
+		StreamConfiguration output{ configuration() };
 		output.pixelFormat = outFormat;
 		output.size.width = camera3Stream_->width;
 		output.size.height = camera3Stream_->height;
diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
index 0069d5e2558f..7ef18b00aee8 100644
--- a/src/libcamera/pipeline/ipu3/ipu3.cpp
+++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
@@ -280,7 +280,7 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()
 	bool mainOutputAvailable = true;
 	for (unsigned int i = 0; i < config_.size(); ++i) {
 		const PixelFormatInfo &info = PixelFormatInfo::info(config_[i].pixelFormat);
-		const StreamConfiguration originalCfg = config_[i];
+		const StreamConfiguration originalCfg{ config_[i] };
 		StreamConfiguration *cfg = &config_[i];

 		LOG(IPU3, Debug) << "Validating stream: " << config_[i].toString();
diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
index 6c6d711f91b3..6935018bec13 100644
--- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
+++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
@@ -562,7 +562,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()

 		/* Try to match stream without adjusting configuration. */
 		if (mainPathAvailable) {
-			StreamConfiguration tryCfg = cfg;
+			StreamConfiguration tryCfg{ cfg };
 			if (data_->mainPath_->validate(sensor, sensorConfig, &tryCfg) == Valid) {
 				mainPathAvailable = false;
 				cfg = tryCfg;
@@ -572,7 +572,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
 		}

 		if (selfPathAvailable) {
-			StreamConfiguration tryCfg = cfg;
+			StreamConfiguration tryCfg{ cfg };
 			if (data_->selfPath_->validate(sensor, sensorConfig, &tryCfg) == Valid) {
 				selfPathAvailable = false;
 				cfg = tryCfg;
@@ -583,7 +583,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()

 		/* Try to match stream allowing adjusting configuration. */
 		if (mainPathAvailable) {
-			StreamConfiguration tryCfg = cfg;
+			StreamConfiguration tryCfg{ cfg };
 			if (data_->mainPath_->validate(sensor, sensorConfig, &tryCfg) == Adjusted) {
 				mainPathAvailable = false;
 				cfg = tryCfg;
@@ -594,7 +594,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
 		}

 		if (selfPathAvailable) {
-			StreamConfiguration tryCfg = cfg;
+			StreamConfiguration tryCfg{ cfg };
 			if (data_->selfPath_->validate(sensor, sensorConfig, &tryCfg) == Adjusted) {
 				selfPathAvailable = false;
 				cfg = tryCfg;
diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
index 236d05af7a2f..120e3a0bc263 100644
--- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
+++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
@@ -259,7 +259,7 @@ RkISP1Path::validate(const CameraSensor *sensor,
 	const std::vector<unsigned int> &mbusCodes = sensor->mbusCodes();
 	Size resolution = filterSensorResolution(sensor);

-	const StreamConfiguration reqCfg = *cfg;
+	const StreamConfiguration reqCfg{ *cfg };
 	CameraConfiguration::Status status = CameraConfiguration::Valid;

 	/*

No functional change so far. Then the compiler will complain about the
code fixed by your patch:

diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
index 7507e9ddae77..4c865a46af53 100644
--- a/src/libcamera/camera.cpp
+++ b/src/libcamera/camera.cpp
@@ -1178,8 +1178,8 @@ int Camera::configure(CameraConfiguration *config)
 	if (ret < 0)
 		return ret;

-	for (auto it : *config)
-		it.setStream(nullptr);
+	for (auto &cfg : *config)
+		cfg.setStream(nullptr);

 	if (config->validate() != CameraConfiguration::Valid) {
 		LOG(Camera, Error)

But... surprise surprise, it complains somewhere else !

diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
index 9e2d9d23c527..6f278b29331a 100644
--- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
+++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
@@ -105,7 +105,7 @@ CameraConfiguration::Status RPiCameraConfiguration::validateColorSpaces([[maybe_
 	Status status = Valid;
 	yuvColorSpace_.reset();

-	for (auto cfg : config_) {
+	for (auto &cfg : config_) {
 		/* First fix up raw streams to have the "raw" colour space. */
 		if (PipelineHandlerBase::isRaw(cfg.pixelFormat)) {
 			/* If there was no value here, that doesn't count as "adjusted". */
@@ -130,7 +130,7 @@ CameraConfiguration::Status RPiCameraConfiguration::validateColorSpaces([[maybe_
 	rgbColorSpace_->range = ColorSpace::Range::Full;

 	/* Go through the streams again and force everyone to the same colour space. */
-	for (auto cfg : config_) {
+	for (auto &cfg : config_) {
 		if (cfg.colorSpace == ColorSpace::Raw)
 			continue;

Would you be able to respin this in a small series? I'd start invoking
copy constructor explicitly through the code, then fixing the bugs, and
finally marking the copy constructor as explicit in a third patch.

>  
>  	if (config->validate() != CameraConfiguration::Valid) {
>  		LOG(Camera, Error)
Barnabás Pőcze Nov. 26, 2024, 2:03 a.m. UTC | #3
Hi


2024. november 25., hétfő 23:48 keltezéssel, Laurent Pinchart <laurent.pinchart@ideasonboard.com> írta:

> Hi Barnabás,
> 
> Thank you for the patch.
> 
> On Mon, Nov 25, 2024 at 12:08:37AM +0000, Barnabás Pőcze wrote:
> > A copy is made in the range-based for loop, and thus `setStream()`
> > operates on this copy, leading to the `stream_` member of the given
> > `StreamConfiguration` object in `*config` never being set to `nullptr`.
> >
> > Fix that by taking a reference in the range-based for loop.
> >
> > Also rename the variable from `it` since it is not an iterator.
> >
> > Fixes: 4217c9f1aa863c ("libcamera: camera: Zero streams before validate()")
> 
> Missing SoB line.
> 
> > ---
> >  src/libcamera/camera.cpp | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > index 25135d46..82a5186a 100644
> > --- a/src/libcamera/camera.cpp
> > +++ b/src/libcamera/camera.cpp
> > @@ -1178,8 +1178,8 @@ int Camera::configure(CameraConfiguration *config)
> >  	if (ret < 0)
> >  		return ret;
> >
> > -	for (auto it : *config)
> > -		it.setStream(nullptr);
> > +	for (auto &cfg : *config)
> > +		cfg.setStream(nullptr);
> 
> That's a worthy fix indeed, but I'm annoyed the compiler didn't tell us.
> Or worse, that it won't tell us in the future if this occurs again.
> 
> It turns out we can do better by marking the copy constructor explicit:

I am not a fan of that for two reasons:
 - I like using `=` :-)
 - it is limited to just this type

`clang-tidy` has a check for a very close scenario:
https://clang.llvm.org/extra/clang-tidy/checks/performance/for-range-copy.html

I believe integrating that into the CI or something would be a more general solution.

Executing

  clang-tidy \
    -p ./build/compile_commands.json \
    --config="{ Checks: [ '-*', 'performance-for-range-copy' ], CheckOptions: [{key: 'performance-for-range-copy.WarnOnAllAutoCopies', value: true}] }" \
    $(find -type f -name '*.cpp')

reveals:

./src/libcamera/converter.cpp:328:13: warning: the loop variable's type is not a reference type; this creates a copy in each iteration; consider making this a reference [performance-for-range-copy]
  328 |                 for (auto alias : factory->compatibles())
      |                           ^
      |                      const  &
./src/libcamera/pipeline/rpi/common/pipeline_base.cpp:108:12: warning: the loop variable's type is not a reference type; this creates a copy in each iteration; consider making this a reference [performance-for-range-copy]
  108 |         for (auto cfg : config_) {
      |                   ^
      |              const  &
./src/libcamera/pipeline/rpi/common/pipeline_base.cpp:133:12: warning: the loop variable's type is not a reference type; this creates a copy in each iteration; consider making this a reference [performance-for-range-copy]
  133 |         for (auto cfg : config_) {
      |                   ^
      |              const  &
./src/libcamera/pipeline/virtual/image_frame_generator.cpp:42:29: warning: loop variable is copied but only used as const reference; consider making it a const reference [performance-for-range-copy]
   42 |         for (std::filesystem::path path : imageFrames.files) {
      |                                    ^
      |              const                &
./test/gstreamer/gstreamer_device_provider_test.cpp:60:14: warning: the loop variable's type is not a reference type; this creates a copy in each iteration; consider making this a reference [performance-for-range-copy]
   60 |                         for (auto name : cameraNames) {
      |                                   ^
      |                              const  &

Apart from the two you mention below, they all seem to be performance
issues that do not affect correctness.

I have just discovered that there is already a .clang-tidy file, and that meson
automatically generates a clang-tidy target if it is installed and a .clang-tidy
file is found, so I am suggesting the following:

diff --git a/.clang-tidy b/.clang-tidy
index 8056d7a8..6ef418d9 100644
--- a/.clang-tidy
+++ b/.clang-tidy
@@ -1,4 +1,10 @@
 # SPDX-License-Identifier: CC0-1.0
 
-Checks:                -clang-diagnostic-c99-designator
+Checks:
+  - '-clang-diagnostic-c99-designator'
+  - 'performance-*'
+CheckOptions:
+  - key: 'performance-for-range-copy.WarnOnAllAutoCopies'
+    value: true
+WarningsAsErrors: 'performance-for-range-copy'
 FormatStyle:   file

and adding `ninja -C build clang-tidy` to an appropriate CI job.

Admittedly, there are cases that this won't report, but I think
this is worthwhile nonetheless.

> 
> diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h
> index 071b71698acb..96b1ef36702e 100644
> --- a/include/libcamera/stream.h
> +++ b/include/libcamera/stream.h
> @@ -41,6 +41,12 @@ struct StreamConfiguration {
>  	StreamConfiguration();
>  	StreamConfiguration(const StreamFormats &formats);
> 
> +	explicit StreamConfiguration(const StreamConfiguration &cfg) = default;
> +	StreamConfiguration(StreamConfiguration &&cfg) = default;
> +
> +	StreamConfiguration &operator=(const StreamConfiguration &cfg) = default;
> +	StreamConfiguration &operator=(StreamConfiguration &&cfg) = default;
> +
>  	PixelFormat pixelFormat;
>  	Size size;
>  	unsigned int stride;
> [...]
> No functional change so far. Then the compiler will complain about the
> code fixed by your patch:
> 
> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> index 7507e9ddae77..4c865a46af53 100644
> --- a/src/libcamera/camera.cpp
> +++ b/src/libcamera/camera.cpp
> @@ -1178,8 +1178,8 @@ int Camera::configure(CameraConfiguration *config)
>  	if (ret < 0)
>  		return ret;
> 
> -	for (auto it : *config)
> -		it.setStream(nullptr);
> +	for (auto &cfg : *config)
> +		cfg.setStream(nullptr);
> 
>  	if (config->validate() != CameraConfiguration::Valid) {
>  		LOG(Camera, Error)
> 
> But... surprise surprise, it complains somewhere else !

Ahh... I should've looked further.


> 
> diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> index 9e2d9d23c527..6f278b29331a 100644
> --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> @@ -105,7 +105,7 @@ CameraConfiguration::Status RPiCameraConfiguration::validateColorSpaces([[maybe_
>  	Status status = Valid;
>  	yuvColorSpace_.reset();
> 
> -	for (auto cfg : config_) {
> +	for (auto &cfg : config_) {
>  		/* First fix up raw streams to have the "raw" colour space. */
>  		if (PipelineHandlerBase::isRaw(cfg.pixelFormat)) {
>  			/* If there was no value here, that doesn't count as "adjusted". */
> @@ -130,7 +130,7 @@ CameraConfiguration::Status RPiCameraConfiguration::validateColorSpaces([[maybe_
>  	rgbColorSpace_->range = ColorSpace::Range::Full;
> 
>  	/* Go through the streams again and force everyone to the same colour space. */
> -	for (auto cfg : config_) {
> +	for (auto &cfg : config_) {
>  		if (cfg.colorSpace == ColorSpace::Raw)
>  			continue;
> 
> Would you be able to respin this in a small series? I'd start invoking
> copy constructor explicitly through the code, then fixing the bugs, and
> finally marking the copy constructor as explicit in a third patch.
> 
> >
> >  	if (config->validate() != CameraConfiguration::Valid) {
> >  		LOG(Camera, Error)
> 
> --
> Regards,
> 
> Laurent Pinchart
> 


Regards,
Barnabás Pőcze
Laurent Pinchart Nov. 26, 2024, 12:08 p.m. UTC | #4
On Tue, Nov 26, 2024 at 02:03:09AM +0000, Barnabás Pőcze wrote:
> 2024. november 25., hétfő 23:48 keltezéssel, Laurent Pinchart írta:
> > On Mon, Nov 25, 2024 at 12:08:37AM +0000, Barnabás Pőcze wrote:
> > > A copy is made in the range-based for loop, and thus `setStream()`
> > > operates on this copy, leading to the `stream_` member of the given
> > > `StreamConfiguration` object in `*config` never being set to `nullptr`.
> > >
> > > Fix that by taking a reference in the range-based for loop.
> > >
> > > Also rename the variable from `it` since it is not an iterator.
> > >
> > > Fixes: 4217c9f1aa863c ("libcamera: camera: Zero streams before validate()")
> > 
> > Missing SoB line.
> > 
> > > ---
> > >  src/libcamera/camera.cpp | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > > index 25135d46..82a5186a 100644
> > > --- a/src/libcamera/camera.cpp
> > > +++ b/src/libcamera/camera.cpp
> > > @@ -1178,8 +1178,8 @@ int Camera::configure(CameraConfiguration *config)
> > >  	if (ret < 0)
> > >  		return ret;
> > >
> > > -	for (auto it : *config)
> > > -		it.setStream(nullptr);
> > > +	for (auto &cfg : *config)
> > > +		cfg.setStream(nullptr);
> > 
> > That's a worthy fix indeed, but I'm annoyed the compiler didn't tell us.
> > Or worse, that it won't tell us in the future if this occurs again.
> > 
> > It turns out we can do better by marking the copy constructor explicit:
> 
> I am not a fan of that for two reasons:
>  - I like using `=` :-)

I've actually grown fond of the

	Class foo{ other };

syntax, which I didn't like in the first place, as it make it clear that
we're invoking the copy constructor. Writing

	Class foo = other;

looks like a default construction followed by a copy assignment. The
compiler optimizes that, but it's still not explicit.

>  - it is limited to just this type

Yes, I thought about that too, and was thinking about going through
other classes to see if we should mark their copy constructor explicit.
That however won't cover all classes that can be iterated over, as shown
in the clang-tidy report below. Using clang-tidy is possibly a better
option.

> `clang-tidy` has a check for a very close scenario:
> https://clang.llvm.org/extra/clang-tidy/checks/performance/for-range-copy.html
> 
> I believe integrating that into the CI or something would be a more general solution.
> 
> Executing
> 
>   clang-tidy \
>     -p ./build/compile_commands.json \
>     --config="{ Checks: [ '-*', 'performance-for-range-copy' ], CheckOptions: [{key: 'performance-for-range-copy.WarnOnAllAutoCopies', value: true}] }" \
>     $(find -type f -name '*.cpp')
> 
> reveals:
> 
> ./src/libcamera/converter.cpp:328:13: warning: the loop variable's type is not a reference type; this creates a copy in each iteration; consider making this a reference [performance-for-range-copy]
>   328 |                 for (auto alias : factory->compatibles())
>       |                           ^
>       |                      const  &
> ./src/libcamera/pipeline/rpi/common/pipeline_base.cpp:108:12: warning: the loop variable's type is not a reference type; this creates a copy in each iteration; consider making this a reference [performance-for-range-copy]
>   108 |         for (auto cfg : config_) {
>       |                   ^
>       |              const  &
> ./src/libcamera/pipeline/rpi/common/pipeline_base.cpp:133:12: warning: the loop variable's type is not a reference type; this creates a copy in each iteration; consider making this a reference [performance-for-range-copy]
>   133 |         for (auto cfg : config_) {
>       |                   ^
>       |              const  &
> ./src/libcamera/pipeline/virtual/image_frame_generator.cpp:42:29: warning: loop variable is copied but only used as const reference; consider making it a const reference [performance-for-range-copy]
>    42 |         for (std::filesystem::path path : imageFrames.files) {
>       |                                    ^
>       |              const                &
> ./test/gstreamer/gstreamer_device_provider_test.cpp:60:14: warning: the loop variable's type is not a reference type; this creates a copy in each iteration; consider making this a reference [performance-for-range-copy]
>    60 |                         for (auto name : cameraNames) {
>       |                                   ^
>       |                              const  &

Would you send a patch series to fix all of those ?

> Apart from the two you mention below, they all seem to be performance
> issues that do not affect correctness.
> 
> I have just discovered that there is already a .clang-tidy file, and that meson
> automatically generates a clang-tidy target if it is installed and a .clang-tidy
> file is found, so I am suggesting the following:
> 
> diff --git a/.clang-tidy b/.clang-tidy
> index 8056d7a8..6ef418d9 100644
> --- a/.clang-tidy
> +++ b/.clang-tidy
> @@ -1,4 +1,10 @@
>  # SPDX-License-Identifier: CC0-1.0
>  
> -Checks:                -clang-diagnostic-c99-designator
> +Checks:
> +  - '-clang-diagnostic-c99-designator'
> +  - 'performance-*'
> +CheckOptions:
> +  - key: 'performance-for-range-copy.WarnOnAllAutoCopies'
> +    value: true
> +WarningsAsErrors: 'performance-for-range-copy'
>  FormatStyle:   file
> 
> and adding `ninja -C build clang-tidy` to an appropriate CI job.
> 
> Admittedly, there are cases that this won't report, but I think
> this is worthwhile nonetheless.

Ideally that should be integrated in checkstyle.py.

How do we handle false positives in CI if we use clang-tidy ? We strive
for a warning-less build, and CI checks that output known warnings will
just be ignored. Keeping a database of known warnings may be one option,
but I'd like to avoid that if possible (partly because I'm not sure how
we could implement it).

> > 
> > diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h
> > index 071b71698acb..96b1ef36702e 100644
> > --- a/include/libcamera/stream.h
> > +++ b/include/libcamera/stream.h
> > @@ -41,6 +41,12 @@ struct StreamConfiguration {
> >  	StreamConfiguration();
> >  	StreamConfiguration(const StreamFormats &formats);
> > 
> > +	explicit StreamConfiguration(const StreamConfiguration &cfg) = default;
> > +	StreamConfiguration(StreamConfiguration &&cfg) = default;
> > +
> > +	StreamConfiguration &operator=(const StreamConfiguration &cfg) = default;
> > +	StreamConfiguration &operator=(StreamConfiguration &&cfg) = default;
> > +
> >  	PixelFormat pixelFormat;
> >  	Size size;
> >  	unsigned int stride;
> > [...]
> > No functional change so far. Then the compiler will complain about the
> > code fixed by your patch:
> > 
> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > index 7507e9ddae77..4c865a46af53 100644
> > --- a/src/libcamera/camera.cpp
> > +++ b/src/libcamera/camera.cpp
> > @@ -1178,8 +1178,8 @@ int Camera::configure(CameraConfiguration *config)
> >  	if (ret < 0)
> >  		return ret;
> > 
> > -	for (auto it : *config)
> > -		it.setStream(nullptr);
> > +	for (auto &cfg : *config)
> > +		cfg.setStream(nullptr);
> > 
> >  	if (config->validate() != CameraConfiguration::Valid) {
> >  		LOG(Camera, Error)
> > 
> > But... surprise surprise, it complains somewhere else !
> 
> Ahh... I should've looked further.
> 
> 
> > 
> > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > index 9e2d9d23c527..6f278b29331a 100644
> > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > @@ -105,7 +105,7 @@ CameraConfiguration::Status RPiCameraConfiguration::validateColorSpaces([[maybe_
> >  	Status status = Valid;
> >  	yuvColorSpace_.reset();
> > 
> > -	for (auto cfg : config_) {
> > +	for (auto &cfg : config_) {
> >  		/* First fix up raw streams to have the "raw" colour space. */
> >  		if (PipelineHandlerBase::isRaw(cfg.pixelFormat)) {
> >  			/* If there was no value here, that doesn't count as "adjusted". */
> > @@ -130,7 +130,7 @@ CameraConfiguration::Status RPiCameraConfiguration::validateColorSpaces([[maybe_
> >  	rgbColorSpace_->range = ColorSpace::Range::Full;
> > 
> >  	/* Go through the streams again and force everyone to the same colour space. */
> > -	for (auto cfg : config_) {
> > +	for (auto &cfg : config_) {
> >  		if (cfg.colorSpace == ColorSpace::Raw)
> >  			continue;
> > 
> > Would you be able to respin this in a small series? I'd start invoking
> > copy constructor explicitly through the code, then fixing the bugs, and
> > finally marking the copy constructor as explicit in a third patch.
> > 
> > >
> > >  	if (config->validate() != CameraConfiguration::Valid) {
> > >  		LOG(Camera, Error)
Robert Mader Nov. 26, 2024, 12:19 p.m. UTC | #5
On 26.11.24 13:08, Laurent Pinchart wrote:
> Ideally that should be integrated in checkstyle.py.
>
> How do we handle false positives in CI if we use clang-tidy ? We strive
> for a warning-less build, and CI checks that output known warnings will
> just be ignored. Keeping a database of known warnings may be one option,
> but I'd like to avoid that if possible (partly because I'm not sure how
> we could implement it).

Apparently there are multiple NOLINT inline comments that one can use, 
see 
https://clang.llvm.org/extra/clang-tidy/#suppressing-undesired-diagnostics
Laurent Pinchart Nov. 26, 2024, 3:55 p.m. UTC | #6
On Tue, Nov 26, 2024 at 02:08:46PM +0200, Laurent Pinchart wrote:
> On Tue, Nov 26, 2024 at 02:03:09AM +0000, Barnabás Pőcze wrote:
> > 2024. november 25., hétfő 23:48 keltezéssel, Laurent Pinchart írta:
> > > On Mon, Nov 25, 2024 at 12:08:37AM +0000, Barnabás Pőcze wrote:
> > > > A copy is made in the range-based for loop, and thus `setStream()`
> > > > operates on this copy, leading to the `stream_` member of the given
> > > > `StreamConfiguration` object in `*config` never being set to `nullptr`.
> > > >
> > > > Fix that by taking a reference in the range-based for loop.
> > > >
> > > > Also rename the variable from `it` since it is not an iterator.
> > > >
> > > > Fixes: 4217c9f1aa863c ("libcamera: camera: Zero streams before validate()")
> > > 
> > > Missing SoB line.
> > > 
> > > > ---
> > > >  src/libcamera/camera.cpp | 4 ++--
> > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > > > index 25135d46..82a5186a 100644
> > > > --- a/src/libcamera/camera.cpp
> > > > +++ b/src/libcamera/camera.cpp
> > > > @@ -1178,8 +1178,8 @@ int Camera::configure(CameraConfiguration *config)
> > > >  	if (ret < 0)
> > > >  		return ret;
> > > >
> > > > -	for (auto it : *config)
> > > > -		it.setStream(nullptr);
> > > > +	for (auto &cfg : *config)
> > > > +		cfg.setStream(nullptr);
> > > 
> > > That's a worthy fix indeed, but I'm annoyed the compiler didn't tell us.
> > > Or worse, that it won't tell us in the future if this occurs again.
> > > 
> > > It turns out we can do better by marking the copy constructor explicit:
> > 
> > I am not a fan of that for two reasons:
> >  - I like using `=` :-)
> 
> I've actually grown fond of the
> 
> 	Class foo{ other };
> 
> syntax, which I didn't like in the first place, as it make it clear that
> we're invoking the copy constructor. Writing
> 
> 	Class foo = other;
> 
> looks like a default construction followed by a copy assignment. The
> compiler optimizes that, but it's still not explicit.
> 
> >  - it is limited to just this type
> 
> Yes, I thought about that too, and was thinking about going through
> other classes to see if we should mark their copy constructor explicit.
> That however won't cover all classes that can be iterated over, as shown
> in the clang-tidy report below. Using clang-tidy is possibly a better
> option.
> 
> > `clang-tidy` has a check for a very close scenario:
> > https://clang.llvm.org/extra/clang-tidy/checks/performance/for-range-copy.html
> > 
> > I believe integrating that into the CI or something would be a more general solution.
> > 
> > Executing
> > 
> >   clang-tidy \
> >     -p ./build/compile_commands.json \
> >     --config="{ Checks: [ '-*', 'performance-for-range-copy' ], CheckOptions: [{key: 'performance-for-range-copy.WarnOnAllAutoCopies', value: true}] }" \
> >     $(find -type f -name '*.cpp')
> > 
> > reveals:
> > 
> > ./src/libcamera/converter.cpp:328:13: warning: the loop variable's type is not a reference type; this creates a copy in each iteration; consider making this a reference [performance-for-range-copy]
> >   328 |                 for (auto alias : factory->compatibles())
> >       |                           ^
> >       |                      const  &
> > ./src/libcamera/pipeline/rpi/common/pipeline_base.cpp:108:12: warning: the loop variable's type is not a reference type; this creates a copy in each iteration; consider making this a reference [performance-for-range-copy]
> >   108 |         for (auto cfg : config_) {
> >       |                   ^
> >       |              const  &
> > ./src/libcamera/pipeline/rpi/common/pipeline_base.cpp:133:12: warning: the loop variable's type is not a reference type; this creates a copy in each iteration; consider making this a reference [performance-for-range-copy]
> >   133 |         for (auto cfg : config_) {
> >       |                   ^
> >       |              const  &
> > ./src/libcamera/pipeline/virtual/image_frame_generator.cpp:42:29: warning: loop variable is copied but only used as const reference; consider making it a const reference [performance-for-range-copy]
> >    42 |         for (std::filesystem::path path : imageFrames.files) {
> >       |                                    ^
> >       |              const                &
> > ./test/gstreamer/gstreamer_device_provider_test.cpp:60:14: warning: the loop variable's type is not a reference type; this creates a copy in each iteration; consider making this a reference [performance-for-range-copy]
> >    60 |                         for (auto name : cameraNames) {
> >       |                                   ^
> >       |                              const  &
> 
> Would you send a patch series to fix all of those ?
> 
> > Apart from the two you mention below, they all seem to be performance
> > issues that do not affect correctness.
> > 
> > I have just discovered that there is already a .clang-tidy file, and that meson
> > automatically generates a clang-tidy target if it is installed and a .clang-tidy
> > file is found, so I am suggesting the following:
> > 
> > diff --git a/.clang-tidy b/.clang-tidy
> > index 8056d7a8..6ef418d9 100644
> > --- a/.clang-tidy
> > +++ b/.clang-tidy
> > @@ -1,4 +1,10 @@
> >  # SPDX-License-Identifier: CC0-1.0
> >  
> > -Checks:                -clang-diagnostic-c99-designator
> > +Checks:
> > +  - '-clang-diagnostic-c99-designator'
> > +  - 'performance-*'
> > +CheckOptions:
> > +  - key: 'performance-for-range-copy.WarnOnAllAutoCopies'
> > +    value: true
> > +WarningsAsErrors: 'performance-for-range-copy'
> >  FormatStyle:   file
> > 
> > and adding `ninja -C build clang-tidy` to an appropriate CI job.
> > 
> > Admittedly, there are cases that this won't report, but I think
> > this is worthwhile nonetheless.

I had a look at this, and it's *very* noisy. We could suppress some
warnings to make it better, and fix other issues, but we also import
files such as kernel headers or Android sources verbatim, and clang-tidy
reports lots of issues there. I haven't yet found a way to exclude
files.

> Ideally that should be integrated in checkstyle.py.
> 
> How do we handle false positives in CI if we use clang-tidy ? We strive
> for a warning-less build, and CI checks that output known warnings will
> just be ignored. Keeping a database of known warnings may be one option,
> but I'd like to avoid that if possible (partly because I'm not sure how
> we could implement it).
> 
> > > 
> > > diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h
> > > index 071b71698acb..96b1ef36702e 100644
> > > --- a/include/libcamera/stream.h
> > > +++ b/include/libcamera/stream.h
> > > @@ -41,6 +41,12 @@ struct StreamConfiguration {
> > >  	StreamConfiguration();
> > >  	StreamConfiguration(const StreamFormats &formats);
> > > 
> > > +	explicit StreamConfiguration(const StreamConfiguration &cfg) = default;
> > > +	StreamConfiguration(StreamConfiguration &&cfg) = default;
> > > +
> > > +	StreamConfiguration &operator=(const StreamConfiguration &cfg) = default;
> > > +	StreamConfiguration &operator=(StreamConfiguration &&cfg) = default;
> > > +
> > >  	PixelFormat pixelFormat;
> > >  	Size size;
> > >  	unsigned int stride;
> > > [...]
> > > No functional change so far. Then the compiler will complain about the
> > > code fixed by your patch:
> > > 
> > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > > index 7507e9ddae77..4c865a46af53 100644
> > > --- a/src/libcamera/camera.cpp
> > > +++ b/src/libcamera/camera.cpp
> > > @@ -1178,8 +1178,8 @@ int Camera::configure(CameraConfiguration *config)
> > >  	if (ret < 0)
> > >  		return ret;
> > > 
> > > -	for (auto it : *config)
> > > -		it.setStream(nullptr);
> > > +	for (auto &cfg : *config)
> > > +		cfg.setStream(nullptr);
> > > 
> > >  	if (config->validate() != CameraConfiguration::Valid) {
> > >  		LOG(Camera, Error)
> > > 
> > > But... surprise surprise, it complains somewhere else !
> > 
> > Ahh... I should've looked further.
> > 
> > > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > > index 9e2d9d23c527..6f278b29331a 100644
> > > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > > @@ -105,7 +105,7 @@ CameraConfiguration::Status RPiCameraConfiguration::validateColorSpaces([[maybe_
> > >  	Status status = Valid;
> > >  	yuvColorSpace_.reset();
> > > 
> > > -	for (auto cfg : config_) {
> > > +	for (auto &cfg : config_) {
> > >  		/* First fix up raw streams to have the "raw" colour space. */
> > >  		if (PipelineHandlerBase::isRaw(cfg.pixelFormat)) {
> > >  			/* If there was no value here, that doesn't count as "adjusted". */
> > > @@ -130,7 +130,7 @@ CameraConfiguration::Status RPiCameraConfiguration::validateColorSpaces([[maybe_
> > >  	rgbColorSpace_->range = ColorSpace::Range::Full;
> > > 
> > >  	/* Go through the streams again and force everyone to the same colour space. */
> > > -	for (auto cfg : config_) {
> > > +	for (auto &cfg : config_) {
> > >  		if (cfg.colorSpace == ColorSpace::Raw)
> > >  			continue;
> > > 
> > > Would you be able to respin this in a small series? I'd start invoking
> > > copy constructor explicitly through the code, then fixing the bugs, and
> > > finally marking the copy constructor as explicit in a third patch.
> > > 
> > > >
> > > >  	if (config->validate() != CameraConfiguration::Valid) {
> > > >  		LOG(Camera, Error)
Barnabás Pőcze Nov. 26, 2024, 4:14 p.m. UTC | #7
2024. november 26., kedd 16:55 keltezéssel, Laurent Pinchart <laurent.pinchart@ideasonboard.com> írta:

> On Tue, Nov 26, 2024 at 02:08:46PM +0200, Laurent Pinchart wrote:
> > On Tue, Nov 26, 2024 at 02:03:09AM +0000, Barnabás Pőcze wrote:
> > > 2024. november 25., hétfő 23:48 keltezéssel, Laurent Pinchart írta:
> > > > On Mon, Nov 25, 2024 at 12:08:37AM +0000, Barnabás Pőcze wrote:
> > > > > A copy is made in the range-based for loop, and thus `setStream()`
> > > > > operates on this copy, leading to the `stream_` member of the given
> > > > > `StreamConfiguration` object in `*config` never being set to `nullptr`.
> > > > >
> > > > > Fix that by taking a reference in the range-based for loop.
> > > > >
> > > > > Also rename the variable from `it` since it is not an iterator.
> > > > >
> > > > > Fixes: 4217c9f1aa863c ("libcamera: camera: Zero streams before validate()")
> > > >
> > > > Missing SoB line.
> > > >
> > > > > ---
> > > > >  src/libcamera/camera.cpp | 4 ++--
> > > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > > > > index 25135d46..82a5186a 100644
> > > > > --- a/src/libcamera/camera.cpp
> > > > > +++ b/src/libcamera/camera.cpp
> > > > > @@ -1178,8 +1178,8 @@ int Camera::configure(CameraConfiguration *config)
> > > > >  	if (ret < 0)
> > > > >  		return ret;
> > > > >
> > > > > -	for (auto it : *config)
> > > > > -		it.setStream(nullptr);
> > > > > +	for (auto &cfg : *config)
> > > > > +		cfg.setStream(nullptr);
> > > >
> > > > That's a worthy fix indeed, but I'm annoyed the compiler didn't tell us.
> > > > Or worse, that it won't tell us in the future if this occurs again.
> > > >
> > > > It turns out we can do better by marking the copy constructor explicit:
> > >
> > > I am not a fan of that for two reasons:
> > >  - I like using `=` :-)
> >
> > I've actually grown fond of the
> >
> > 	Class foo{ other };
> >
> > syntax, which I didn't like in the first place, as it make it clear that
> > we're invoking the copy constructor. Writing
> >
> > 	Class foo = other;
> >
> > looks like a default construction followed by a copy assignment. The
> > compiler optimizes that, but it's still not explicit.
> >
> > >  - it is limited to just this type
> >
> > Yes, I thought about that too, and was thinking about going through
> > other classes to see if we should mark their copy constructor explicit.
> > That however won't cover all classes that can be iterated over, as shown
> > in the clang-tidy report below. Using clang-tidy is possibly a better
> > option.
> >
> > > `clang-tidy` has a check for a very close scenario:
> > > https://clang.llvm.org/extra/clang-tidy/checks/performance/for-range-copy.html
> > >
> > > I believe integrating that into the CI or something would be a more general solution.
> > >
> > > Executing
> > >
> > >   clang-tidy \
> > >     -p ./build/compile_commands.json \
> > >     --config="{ Checks: [ '-*', 'performance-for-range-copy' ], CheckOptions: [{key: 'performance-for-range-copy.WarnOnAllAutoCopies', value: true}] }" \
> > >     $(find -type f -name '*.cpp')
> > >
> > > reveals:
> > >
> > > ./src/libcamera/converter.cpp:328:13: warning: the loop variable's type is not a reference type; this creates a copy in each iteration; consider making this a reference [performance-for-range-copy]
> > >   328 |                 for (auto alias : factory->compatibles())
> > >       |                           ^
> > >       |                      const  &
> > > ./src/libcamera/pipeline/rpi/common/pipeline_base.cpp:108:12: warning: the loop variable's type is not a reference type; this creates a copy in each iteration; consider making this a reference [performance-for-range-copy]
> > >   108 |         for (auto cfg : config_) {
> > >       |                   ^
> > >       |              const  &
> > > ./src/libcamera/pipeline/rpi/common/pipeline_base.cpp:133:12: warning: the loop variable's type is not a reference type; this creates a copy in each iteration; consider making this a reference [performance-for-range-copy]
> > >   133 |         for (auto cfg : config_) {
> > >       |                   ^
> > >       |              const  &
> > > ./src/libcamera/pipeline/virtual/image_frame_generator.cpp:42:29: warning: loop variable is copied but only used as const reference; consider making it a const reference [performance-for-range-copy]
> > >    42 |         for (std::filesystem::path path : imageFrames.files) {
> > >       |                                    ^
> > >       |              const                &
> > > ./test/gstreamer/gstreamer_device_provider_test.cpp:60:14: warning: the loop variable's type is not a reference type; this creates a copy in each iteration; consider making this a reference [performance-for-range-copy]
> > >    60 |                         for (auto name : cameraNames) {
> > >       |                                   ^
> > >       |                              const  &
> >
> > Would you send a patch series to fix all of those ?
> >
> > > Apart from the two you mention below, they all seem to be performance
> > > issues that do not affect correctness.
> > >
> > > I have just discovered that there is already a .clang-tidy file, and that meson
> > > automatically generates a clang-tidy target if it is installed and a .clang-tidy
> > > file is found, so I am suggesting the following:
> > >
> > > diff --git a/.clang-tidy b/.clang-tidy
> > > index 8056d7a8..6ef418d9 100644
> > > --- a/.clang-tidy
> > > +++ b/.clang-tidy
> > > @@ -1,4 +1,10 @@
> > >  # SPDX-License-Identifier: CC0-1.0
> > >
> > > -Checks:                -clang-diagnostic-c99-designator
> > > +Checks:
> > > +  - '-clang-diagnostic-c99-designator'
> > > +  - 'performance-*'
> > > +CheckOptions:
> > > +  - key: 'performance-for-range-copy.WarnOnAllAutoCopies'
> > > +    value: true
> > > +WarningsAsErrors: 'performance-for-range-copy'
> > >  FormatStyle:   file
> > >
> > > and adding `ninja -C build clang-tidy` to an appropriate CI job.
> > >
> > > Admittedly, there are cases that this won't report, but I think
> > > this is worthwhile nonetheless.
> 
> I had a look at this, and it's *very* noisy. We could suppress some
> warnings to make it better, and fix other issues, but we also import
> files such as kernel headers or Android sources verbatim, and clang-tidy
> reports lots of issues there. I haven't yet found a way to exclude
> files.

There is `ExcludeHeaderFilterRegex` and `HeaderFilterRegex`: https://clang.llvm.org/extra/clang-tidy/index.html
I believe that should be sufficient to ignore the linux and android headers.


Regards,
Barnabás Pőcze

> 
> > Ideally that should be integrated in checkstyle.py.
> >
> > How do we handle false positives in CI if we use clang-tidy ? We strive
> > for a warning-less build, and CI checks that output known warnings will
> > just be ignored. Keeping a database of known warnings may be one option,
> > but I'd like to avoid that if possible (partly because I'm not sure how
> > we could implement it).
> >
> > > >
> > > > diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h
> > > > index 071b71698acb..96b1ef36702e 100644
> > > > --- a/include/libcamera/stream.h
> > > > +++ b/include/libcamera/stream.h
> > > > @@ -41,6 +41,12 @@ struct StreamConfiguration {
> > > >  	StreamConfiguration();
> > > >  	StreamConfiguration(const StreamFormats &formats);
> > > >
> > > > +	explicit StreamConfiguration(const StreamConfiguration &cfg) = default;
> > > > +	StreamConfiguration(StreamConfiguration &&cfg) = default;
> > > > +
> > > > +	StreamConfiguration &operator=(const StreamConfiguration &cfg) = default;
> > > > +	StreamConfiguration &operator=(StreamConfiguration &&cfg) = default;
> > > > +
> > > >  	PixelFormat pixelFormat;
> > > >  	Size size;
> > > >  	unsigned int stride;
> > > > [...]
> > > > No functional change so far. Then the compiler will complain about the
> > > > code fixed by your patch:
> > > >
> > > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > > > index 7507e9ddae77..4c865a46af53 100644
> > > > --- a/src/libcamera/camera.cpp
> > > > +++ b/src/libcamera/camera.cpp
> > > > @@ -1178,8 +1178,8 @@ int Camera::configure(CameraConfiguration *config)
> > > >  	if (ret < 0)
> > > >  		return ret;
> > > >
> > > > -	for (auto it : *config)
> > > > -		it.setStream(nullptr);
> > > > +	for (auto &cfg : *config)
> > > > +		cfg.setStream(nullptr);
> > > >
> > > >  	if (config->validate() != CameraConfiguration::Valid) {
> > > >  		LOG(Camera, Error)
> > > >
> > > > But... surprise surprise, it complains somewhere else !
> > >
> > > Ahh... I should've looked further.
> > >
> > > > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > > > index 9e2d9d23c527..6f278b29331a 100644
> > > > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > > > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > > > @@ -105,7 +105,7 @@ CameraConfiguration::Status RPiCameraConfiguration::validateColorSpaces([[maybe_
> > > >  	Status status = Valid;
> > > >  	yuvColorSpace_.reset();
> > > >
> > > > -	for (auto cfg : config_) {
> > > > +	for (auto &cfg : config_) {
> > > >  		/* First fix up raw streams to have the "raw" colour space. */
> > > >  		if (PipelineHandlerBase::isRaw(cfg.pixelFormat)) {
> > > >  			/* If there was no value here, that doesn't count as "adjusted". */
> > > > @@ -130,7 +130,7 @@ CameraConfiguration::Status RPiCameraConfiguration::validateColorSpaces([[maybe_
> > > >  	rgbColorSpace_->range = ColorSpace::Range::Full;
> > > >
> > > >  	/* Go through the streams again and force everyone to the same colour space. */
> > > > -	for (auto cfg : config_) {
> > > > +	for (auto &cfg : config_) {
> > > >  		if (cfg.colorSpace == ColorSpace::Raw)
> > > >  			continue;
> > > >
> > > > Would you be able to respin this in a small series? I'd start invoking
> > > > copy constructor explicitly through the code, then fixing the bugs, and
> > > > finally marking the copy constructor as explicit in a third patch.
> > > >
> > > > >
> > > > >  	if (config->validate() != CameraConfiguration::Valid) {
> > > > >  		LOG(Camera, Error)
> 
> --
> Regards,
> 
> Laurent Pinchart
>
Laurent Pinchart Nov. 26, 2024, 4:58 p.m. UTC | #8
On Tue, Nov 26, 2024 at 04:14:21PM +0000, Barnabás Pőcze wrote:
> 2024. november 26., kedd 16:55 keltezéssel, Laurent Pinchart írta:
> > On Tue, Nov 26, 2024 at 02:08:46PM +0200, Laurent Pinchart wrote:
> > > On Tue, Nov 26, 2024 at 02:03:09AM +0000, Barnabás Pőcze wrote:
> > > > 2024. november 25., hétfő 23:48 keltezéssel, Laurent Pinchart írta:
> > > > > On Mon, Nov 25, 2024 at 12:08:37AM +0000, Barnabás Pőcze wrote:
> > > > > > A copy is made in the range-based for loop, and thus `setStream()`
> > > > > > operates on this copy, leading to the `stream_` member of the given
> > > > > > `StreamConfiguration` object in `*config` never being set to `nullptr`.
> > > > > >
> > > > > > Fix that by taking a reference in the range-based for loop.
> > > > > >
> > > > > > Also rename the variable from `it` since it is not an iterator.
> > > > > >
> > > > > > Fixes: 4217c9f1aa863c ("libcamera: camera: Zero streams before validate()")
> > > > >
> > > > > Missing SoB line.
> > > > >
> > > > > > ---
> > > > > >  src/libcamera/camera.cpp | 4 ++--
> > > > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > > >
> > > > > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > > > > > index 25135d46..82a5186a 100644
> > > > > > --- a/src/libcamera/camera.cpp
> > > > > > +++ b/src/libcamera/camera.cpp
> > > > > > @@ -1178,8 +1178,8 @@ int Camera::configure(CameraConfiguration *config)
> > > > > >  	if (ret < 0)
> > > > > >  		return ret;
> > > > > >
> > > > > > -	for (auto it : *config)
> > > > > > -		it.setStream(nullptr);
> > > > > > +	for (auto &cfg : *config)
> > > > > > +		cfg.setStream(nullptr);
> > > > >
> > > > > That's a worthy fix indeed, but I'm annoyed the compiler didn't tell us.
> > > > > Or worse, that it won't tell us in the future if this occurs again.
> > > > >
> > > > > It turns out we can do better by marking the copy constructor explicit:
> > > >
> > > > I am not a fan of that for two reasons:
> > > >  - I like using `=` :-)
> > >
> > > I've actually grown fond of the
> > >
> > > 	Class foo{ other };
> > >
> > > syntax, which I didn't like in the first place, as it make it clear that
> > > we're invoking the copy constructor. Writing
> > >
> > > 	Class foo = other;
> > >
> > > looks like a default construction followed by a copy assignment. The
> > > compiler optimizes that, but it's still not explicit.
> > >
> > > >  - it is limited to just this type
> > >
> > > Yes, I thought about that too, and was thinking about going through
> > > other classes to see if we should mark their copy constructor explicit.
> > > That however won't cover all classes that can be iterated over, as shown
> > > in the clang-tidy report below. Using clang-tidy is possibly a better
> > > option.
> > >
> > > > `clang-tidy` has a check for a very close scenario:
> > > > https://clang.llvm.org/extra/clang-tidy/checks/performance/for-range-copy.html
> > > >
> > > > I believe integrating that into the CI or something would be a more general solution.
> > > >
> > > > Executing
> > > >
> > > >   clang-tidy \
> > > >     -p ./build/compile_commands.json \
> > > >     --config="{ Checks: [ '-*', 'performance-for-range-copy' ], CheckOptions: [{key: 'performance-for-range-copy.WarnOnAllAutoCopies', value: true}] }" \
> > > >     $(find -type f -name '*.cpp')
> > > >
> > > > reveals:
> > > >
> > > > ./src/libcamera/converter.cpp:328:13: warning: the loop variable's type is not a reference type; this creates a copy in each iteration; consider making this a reference [performance-for-range-copy]
> > > >   328 |                 for (auto alias : factory->compatibles())
> > > >       |                           ^
> > > >       |                      const  &
> > > > ./src/libcamera/pipeline/rpi/common/pipeline_base.cpp:108:12: warning: the loop variable's type is not a reference type; this creates a copy in each iteration; consider making this a reference [performance-for-range-copy]
> > > >   108 |         for (auto cfg : config_) {
> > > >       |                   ^
> > > >       |              const  &
> > > > ./src/libcamera/pipeline/rpi/common/pipeline_base.cpp:133:12: warning: the loop variable's type is not a reference type; this creates a copy in each iteration; consider making this a reference [performance-for-range-copy]
> > > >   133 |         for (auto cfg : config_) {
> > > >       |                   ^
> > > >       |              const  &
> > > > ./src/libcamera/pipeline/virtual/image_frame_generator.cpp:42:29: warning: loop variable is copied but only used as const reference; consider making it a const reference [performance-for-range-copy]
> > > >    42 |         for (std::filesystem::path path : imageFrames.files) {
> > > >       |                                    ^
> > > >       |              const                &
> > > > ./test/gstreamer/gstreamer_device_provider_test.cpp:60:14: warning: the loop variable's type is not a reference type; this creates a copy in each iteration; consider making this a reference [performance-for-range-copy]
> > > >    60 |                         for (auto name : cameraNames) {
> > > >       |                                   ^
> > > >       |                              const  &
> > >
> > > Would you send a patch series to fix all of those ?
> > >
> > > > Apart from the two you mention below, they all seem to be performance
> > > > issues that do not affect correctness.
> > > >
> > > > I have just discovered that there is already a .clang-tidy file, and that meson
> > > > automatically generates a clang-tidy target if it is installed and a .clang-tidy
> > > > file is found, so I am suggesting the following:
> > > >
> > > > diff --git a/.clang-tidy b/.clang-tidy
> > > > index 8056d7a8..6ef418d9 100644
> > > > --- a/.clang-tidy
> > > > +++ b/.clang-tidy
> > > > @@ -1,4 +1,10 @@
> > > >  # SPDX-License-Identifier: CC0-1.0
> > > >
> > > > -Checks:                -clang-diagnostic-c99-designator
> > > > +Checks:
> > > > +  - '-clang-diagnostic-c99-designator'
> > > > +  - 'performance-*'
> > > > +CheckOptions:
> > > > +  - key: 'performance-for-range-copy.WarnOnAllAutoCopies'
> > > > +    value: true
> > > > +WarningsAsErrors: 'performance-for-range-copy'
> > > >  FormatStyle:   file
> > > >
> > > > and adding `ninja -C build clang-tidy` to an appropriate CI job.
> > > >
> > > > Admittedly, there are cases that this won't report, but I think
> > > > this is worthwhile nonetheless.
> > 
> > I had a look at this, and it's *very* noisy. We could suppress some
> > warnings to make it better, and fix other issues, but we also import
> > files such as kernel headers or Android sources verbatim, and clang-tidy
> > reports lots of issues there. I haven't yet found a way to exclude
> > files.
> 
> There is `ExcludeHeaderFilterRegex` and `HeaderFilterRegex`: https://clang.llvm.org/extra/clang-tidy/index.html
> I believe that should be sufficient to ignore the linux and android headers.

That option was introduced in clang-tidy 19. Debian stable ships clang
14, and even on Gentoo the latest stable version is 18 :-S

This doesn't prevent us from fixing issues reporting by clang-tidy, but
CI integration will be difficult short term. This raises the question of
whether we want to be defensive against these code patterns through
other means in the meantime, such as marking copy constructors as
explicit, or if that would be overkill.

> > > Ideally that should be integrated in checkstyle.py.
> > >
> > > How do we handle false positives in CI if we use clang-tidy ? We strive
> > > for a warning-less build, and CI checks that output known warnings will
> > > just be ignored. Keeping a database of known warnings may be one option,
> > > but I'd like to avoid that if possible (partly because I'm not sure how
> > > we could implement it).
> > >
> > > > >
> > > > > diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h
> > > > > index 071b71698acb..96b1ef36702e 100644
> > > > > --- a/include/libcamera/stream.h
> > > > > +++ b/include/libcamera/stream.h
> > > > > @@ -41,6 +41,12 @@ struct StreamConfiguration {
> > > > >  	StreamConfiguration();
> > > > >  	StreamConfiguration(const StreamFormats &formats);
> > > > >
> > > > > +	explicit StreamConfiguration(const StreamConfiguration &cfg) = default;
> > > > > +	StreamConfiguration(StreamConfiguration &&cfg) = default;
> > > > > +
> > > > > +	StreamConfiguration &operator=(const StreamConfiguration &cfg) = default;
> > > > > +	StreamConfiguration &operator=(StreamConfiguration &&cfg) = default;
> > > > > +
> > > > >  	PixelFormat pixelFormat;
> > > > >  	Size size;
> > > > >  	unsigned int stride;
> > > > > [...]
> > > > > No functional change so far. Then the compiler will complain about the
> > > > > code fixed by your patch:
> > > > >
> > > > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > > > > index 7507e9ddae77..4c865a46af53 100644
> > > > > --- a/src/libcamera/camera.cpp
> > > > > +++ b/src/libcamera/camera.cpp
> > > > > @@ -1178,8 +1178,8 @@ int Camera::configure(CameraConfiguration *config)
> > > > >  	if (ret < 0)
> > > > >  		return ret;
> > > > >
> > > > > -	for (auto it : *config)
> > > > > -		it.setStream(nullptr);
> > > > > +	for (auto &cfg : *config)
> > > > > +		cfg.setStream(nullptr);
> > > > >
> > > > >  	if (config->validate() != CameraConfiguration::Valid) {
> > > > >  		LOG(Camera, Error)
> > > > >
> > > > > But... surprise surprise, it complains somewhere else !
> > > >
> > > > Ahh... I should've looked further.
> > > >
> > > > > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > > > > index 9e2d9d23c527..6f278b29331a 100644
> > > > > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > > > > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > > > > @@ -105,7 +105,7 @@ CameraConfiguration::Status RPiCameraConfiguration::validateColorSpaces([[maybe_
> > > > >  	Status status = Valid;
> > > > >  	yuvColorSpace_.reset();
> > > > >
> > > > > -	for (auto cfg : config_) {
> > > > > +	for (auto &cfg : config_) {
> > > > >  		/* First fix up raw streams to have the "raw" colour space. */
> > > > >  		if (PipelineHandlerBase::isRaw(cfg.pixelFormat)) {
> > > > >  			/* If there was no value here, that doesn't count as "adjusted". */
> > > > > @@ -130,7 +130,7 @@ CameraConfiguration::Status RPiCameraConfiguration::validateColorSpaces([[maybe_
> > > > >  	rgbColorSpace_->range = ColorSpace::Range::Full;
> > > > >
> > > > >  	/* Go through the streams again and force everyone to the same colour space. */
> > > > > -	for (auto cfg : config_) {
> > > > > +	for (auto &cfg : config_) {
> > > > >  		if (cfg.colorSpace == ColorSpace::Raw)
> > > > >  			continue;
> > > > >
> > > > > Would you be able to respin this in a small series? I'd start invoking
> > > > > copy constructor explicitly through the code, then fixing the bugs, and
> > > > > finally marking the copy constructor as explicit in a third patch.
> > > > >
> > > > > >
> > > > > >  	if (config->validate() != CameraConfiguration::Valid) {
> > > > > >  		LOG(Camera, Error)

Patch
diff mbox series

diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
index 25135d46..82a5186a 100644
--- a/src/libcamera/camera.cpp
+++ b/src/libcamera/camera.cpp
@@ -1178,8 +1178,8 @@  int Camera::configure(CameraConfiguration *config)
 	if (ret < 0)
 		return ret;
 
-	for (auto it : *config)
-		it.setStream(nullptr);
+	for (auto &cfg : *config)
+		cfg.setStream(nullptr);
 
 	if (config->validate() != CameraConfiguration::Valid) {
 		LOG(Camera, Error)