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