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

[Barnabás Pőcze]

Hi


2024. november 25., hétfő 23:48 keltezéssel, Laurent Pinchart <laurent.pinchart at 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