[PATCH v1] libcamera: camera: Fix clearing of stream associations before `validate()`
Barnabás Pőcze
pobrn at protonmail.com
Tue Nov 26 17:14:21 CET 2024
2024. november 26., kedd 16:55 keltezéssel, Laurent Pinchart <laurent.pinchart at 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
>
More information about the libcamera-devel
mailing list