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

[Laurent Pinchart]

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