[PATCH v1] libcamera: camera: Fix clearing of stream associations before `validate()`
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Nov 26 13:08:46 CET 2024
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)
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list