| Message ID | 20241126180302.685265-1-pobrn@protonmail.com |
|---|---|
| State | Accepted |
| Commit | 4e557e544bce6da8c00c73de2dfe76403bbf8c00 |
| Headers | show |
| Series |
|
| Related | show |
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@protonmail.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@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; >
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@protonmail.com> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@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; > >
+David Plowman <david.plowman@raspberrypi.com> On Tue, 26 Nov 2024, 6:09 pm Laurent Pinchart, < laurent.pinchart@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@protonmail.com> > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@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 >
Hi Barnabás, Thank you for this fix. On Tue, 26 Nov 2024 at 18:03, Barnabás Pőcze <pobrn@protonmail.com> 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@protonmail.com> I've run this change through our regression testing and it passes. I was expecting a failure with this change, but it turns out we have CS validation code in Picamera2 that avoids us hitting the below adjustment code. Reviewed-by: Naushir Patuck <naush@raspberrypi.com> > --- > 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; > > -- > 2.47.1 > >
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;
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@protonmail.com> --- src/libcamera/camera.cpp | 4 ++-- src/libcamera/pipeline/rpi/common/pipeline_base.cpp | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-)