[PATCH v1] libcamera: Don't copy `StreamConfiguration` when iterating

[Naushir Patuck]

+David Plowman <david.plowman at raspberrypi.com>

On Tue, 26 Nov 2024, 6:09 pm Laurent Pinchart, <
laurent.pinchart at ideasonboard.com> wrote:

> CC'ing Naush
>
> On Tue, Nov 26, 2024 at 08:08:59PM +0200, Laurent Pinchart wrote:
> > Hi Barnabás,
> >
> > Thank you for the patch.
> >
> > On Tue, Nov 26, 2024 at 06:03:05PM +0000, Barnabás Pőcze wrote:
> > > A copy is made in the range-based for loop, and thus all modifications
> > > done in the for loop body are lost, and not actually applied to
> > > the object in the container.
> > >
> > > Fix that by taking a reference in the range-based for loop.
> > >
> > > Fixes: 4217c9f1aa863c ("libcamera: camera: Zero streams before
> validate()")
> > > Fixes: 613d5402673eb9 ("pipeline: raspberrypi: Fix handling of colour
> spaces")
> > > Signed-off-by: Barnabás Pőcze <pobrn at protonmail.com>
> >
> > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> >
> > Naush, it looks like you may have been missing some tests related to
> > colour space handling. Could you double-check that this change doesn't
> > break anything ? It could be that fixing a bug would uncover another
> > one.
> >
> > > ---
> > >  src/libcamera/camera.cpp                            | 4 ++--
> > >  src/libcamera/pipeline/rpi/common/pipeline_base.cpp | 4 ++--
> > >  2 files changed, 4 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > > index 7507e9dd..4c865a46 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)
> > > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > > index 9e2d9d23..6f278b29 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;
> > >
>
> --
> Regards,
>
> Laurent Pinchart
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20241126/8b8ebd79/attachment.htm>