[PATCH v1] libcamera: camera: Fix clearing of stream associations before `validate()`
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Nov 25 23:48:22 CET 2024
Hi Barnabás,
Thank you for the patch.
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:
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;
diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp
index 1f75dbbc5b64..e34a112e7b01 100644
--- a/src/libcamera/stream.cpp
+++ b/src/libcamera/stream.cpp
@@ -294,6 +294,34 @@ StreamConfiguration::StreamConfiguration(const StreamFormats &formats)
{
}
+/**
+ * \fn StreamConfiguration::StreamConfiguration(const StreamConfiguration &other)
+ * \brief Copy constructor, create a StreamConfiguration from a copy of \a other
+ * \param[in] other The other StreamConfiguration
+ */
+
+/**
+ * \fn StreamConfiguration::StreamConfiguration(StreamConfiguration &&other)
+ * \brief Move constructor, create a StreamConfiguration by taking over \a other
+ * \param[in] other The other StreamConfiguration
+ */
+
+/**
+ * \fn StreamConfiguration::operator=(const StreamConfiguration &other)
+ * \brief Copy assignment operator, replace the StreamConfiguration with a copy
+ * of \a other
+ * \param[in] other The other StreamConfiguration
+ * \return This StreamConfiguration
+ */
+
+/**
+ * \fn StreamConfiguration::operator=(StreamConfiguration &&other)
+ * \brief Move assignment operator, replace the StreamConfiguration by taking
+ * over \a other
+ * \param[in] other The other StreamConfiguration
+ * \return This StreamConfiguration
+ */
+
/**
* \var StreamConfiguration::size
* \brief Stream size in pixels
(We could possibly guard the functions with #ifndef __DOXYGEN__ in the
header file instead of documenting them.)
We will then need to make use of the explicit copy constructor or the
compiler won't be happy:
diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
index 1d68540d7e50..83b7249b88e5 100644
--- a/src/android/camera_stream.cpp
+++ b/src/android/camera_stream.cpp
@@ -87,7 +87,7 @@ int CameraStream::configure()
if (type_ == Type::Internal || type_ == Type::Mapped) {
const PixelFormat outFormat =
cameraDevice_->capabilities()->toPixelFormat(camera3Stream_->format);
- StreamConfiguration output = configuration();
+ StreamConfiguration output{ configuration() };
output.pixelFormat = outFormat;
output.size.width = camera3Stream_->width;
output.size.height = camera3Stream_->height;
diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
index 0069d5e2558f..7ef18b00aee8 100644
--- a/src/libcamera/pipeline/ipu3/ipu3.cpp
+++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
@@ -280,7 +280,7 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()
bool mainOutputAvailable = true;
for (unsigned int i = 0; i < config_.size(); ++i) {
const PixelFormatInfo &info = PixelFormatInfo::info(config_[i].pixelFormat);
- const StreamConfiguration originalCfg = config_[i];
+ const StreamConfiguration originalCfg{ config_[i] };
StreamConfiguration *cfg = &config_[i];
LOG(IPU3, Debug) << "Validating stream: " << config_[i].toString();
diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
index 6c6d711f91b3..6935018bec13 100644
--- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
+++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
@@ -562,7 +562,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
/* Try to match stream without adjusting configuration. */
if (mainPathAvailable) {
- StreamConfiguration tryCfg = cfg;
+ StreamConfiguration tryCfg{ cfg };
if (data_->mainPath_->validate(sensor, sensorConfig, &tryCfg) == Valid) {
mainPathAvailable = false;
cfg = tryCfg;
@@ -572,7 +572,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
}
if (selfPathAvailable) {
- StreamConfiguration tryCfg = cfg;
+ StreamConfiguration tryCfg{ cfg };
if (data_->selfPath_->validate(sensor, sensorConfig, &tryCfg) == Valid) {
selfPathAvailable = false;
cfg = tryCfg;
@@ -583,7 +583,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
/* Try to match stream allowing adjusting configuration. */
if (mainPathAvailable) {
- StreamConfiguration tryCfg = cfg;
+ StreamConfiguration tryCfg{ cfg };
if (data_->mainPath_->validate(sensor, sensorConfig, &tryCfg) == Adjusted) {
mainPathAvailable = false;
cfg = tryCfg;
@@ -594,7 +594,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
}
if (selfPathAvailable) {
- StreamConfiguration tryCfg = cfg;
+ StreamConfiguration tryCfg{ cfg };
if (data_->selfPath_->validate(sensor, sensorConfig, &tryCfg) == Adjusted) {
selfPathAvailable = false;
cfg = tryCfg;
diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
index 236d05af7a2f..120e3a0bc263 100644
--- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
+++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
@@ -259,7 +259,7 @@ RkISP1Path::validate(const CameraSensor *sensor,
const std::vector<unsigned int> &mbusCodes = sensor->mbusCodes();
Size resolution = filterSensorResolution(sensor);
- const StreamConfiguration reqCfg = *cfg;
+ const StreamConfiguration reqCfg{ *cfg };
CameraConfiguration::Status status = CameraConfiguration::Valid;
/*
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 !
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