| Message ID | 20241125000834.396815-1-pobrn@protonmail.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
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 > >
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)
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
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)
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
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)
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 >
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)
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)
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(-)