[libcamera-devel,PATCH/RFC,05/12] libcamera: camera: Return a pointer from generateConfiguration()

1222 diff mbox series

To prepare for specialising the CameraConfiguration class in pipeline
handlers, return a pointer to a camera configuration instead of a
reference from Camera::generateConfiguration(). The camera configuration
always needs to be allocated from the pipeline handler, and its
ownership is passed to the application.

For symmetry, change Camera::configure() to take a CameraConfiguration
pointer instead of a reference. This aligns with our coding practice of
passing parameters that are modified by the callee by pointer.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 include/libcamera/camera.h               |  4 +--
 src/cam/main.cpp                         | 38 ++++++++++----------
 src/libcamera/camera.cpp                 | 31 +++++++++--------
 src/libcamera/include/pipeline_handler.h |  6 ++--
 src/libcamera/pipeline/ipu3/ipu3.cpp     | 31 +++++++++--------
 src/libcamera/pipeline/rkisp1/rkisp1.cpp | 25 ++++++++------
 src/libcamera/pipeline/uvcvideo.cpp      | 30 ++++++++--------
 src/libcamera/pipeline/vimc.cpp          | 30 ++++++++--------
 src/libcamera/pipeline_handler.cpp       |  3 +-
 src/qcam/main_window.cpp                 |  5 ++-
 src/qcam/main_window.h                   |  2 +-
 test/camera/capture.cpp                  | 38 +++++++++++++++-----
 test/camera/configuration_default.cpp    | 14 +++++---
 test/camera/configuration_set.cpp        | 44 +++++++++++++++++-------
 test/camera/statemachine.cpp             | 22 ++++++++++--
 15 files changed, 201 insertions(+), 122 deletions(-)

Hi Laurent,

Thanks for your work.

On 2019-05-18 02:06:14 +0300, Laurent Pinchart wrote:
> To prepare for specialising the CameraConfiguration class in pipeline
> handlers, return a pointer to a camera configuration instead of a
> reference from Camera::generateConfiguration(). The camera configuration
> always needs to be allocated from the pipeline handler, and its
> ownership is passed to the application.

I understand the need for pipeline handlers to be able to specialise the 
CameraConfiguration, at the same time I'm not super happy with how this 
change effects the API towards applications. Specially as we model the 
CameraConfiguration around a vector.

    CameraConfiguration *config = ... prepareCameraConfig();
    StreamConfiguration &cfg = (*config)[index];

Is not the most intuitive interface ;-)

I wonder if we could improve here using a d-pointer design and a private 
pointer with a reference count, or maybe even use a shared_ptr<> 
internally?

> 
> For symmetry, change Camera::configure() to take a CameraConfiguration
> pointer instead of a reference. This aligns with our coding practice of
> passing parameters that are modified by the callee by pointer.

This agree with and should be changed disregarding of the above, and 
maybe even moved to a patch (or it's own patch) earlier in this series
where 'const' is dropped from the argument.

> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  include/libcamera/camera.h               |  4 +--
>  src/cam/main.cpp                         | 38 ++++++++++----------
>  src/libcamera/camera.cpp                 | 31 +++++++++--------
>  src/libcamera/include/pipeline_handler.h |  6 ++--
>  src/libcamera/pipeline/ipu3/ipu3.cpp     | 31 +++++++++--------
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 25 ++++++++------
>  src/libcamera/pipeline/uvcvideo.cpp      | 30 ++++++++--------
>  src/libcamera/pipeline/vimc.cpp          | 30 ++++++++--------
>  src/libcamera/pipeline_handler.cpp       |  3 +-
>  src/qcam/main_window.cpp                 |  5 ++-
>  src/qcam/main_window.h                   |  2 +-
>  test/camera/capture.cpp                  | 38 +++++++++++++++-----
>  test/camera/configuration_default.cpp    | 14 +++++---
>  test/camera/configuration_set.cpp        | 44 +++++++++++++++++-------
>  test/camera/statemachine.cpp             | 22 ++++++++++--
>  15 files changed, 201 insertions(+), 122 deletions(-)
> 
> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> index e2759405f497..841e8fc505b9 100644
> --- a/include/libcamera/camera.h
> +++ b/include/libcamera/camera.h
> @@ -68,8 +68,8 @@ public:
>  	int release();
>  
>  	const std::set<Stream *> &streams() const;
> -	CameraConfiguration generateConfiguration(const StreamRoles &roles);
> -	int configure(CameraConfiguration &config);
> +	CameraConfiguration *generateConfiguration(const StreamRoles &roles);
> +	int configure(CameraConfiguration *config);
>  
>  	int allocateBuffers();
>  	int freeBuffers();
> diff --git a/src/cam/main.cpp b/src/cam/main.cpp
> index cd165bea34cd..7a1b332f68c7 100644
> --- a/src/cam/main.cpp
> +++ b/src/cam/main.cpp
> @@ -85,15 +85,13 @@ static int parseOptions(int argc, char *argv[])
>  	return 0;
>  }
>  
> -static int prepareCameraConfig(CameraConfiguration *config)
> +static CameraConfiguration *prepareCameraConfig()
>  {
>  	StreamRoles roles;
>  
>  	/* If no configuration is provided assume a single video stream. */
> -	if (!options.isSet(OptStream)) {
> -		*config = camera->generateConfiguration({ StreamRole::VideoRecording });
> -		return 0;
> -	}
> +	if (!options.isSet(OptStream))
> +		return camera->generateConfiguration({ StreamRole::VideoRecording });
>  
>  	const std::vector<OptionValue> &streamOptions =
>  		options[OptStream].toArray();
> @@ -113,16 +111,16 @@ static int prepareCameraConfig(CameraConfiguration *config)
>  		} else {
>  			std::cerr << "Unknown stream role "
>  				  << conf["role"].toString() << std::endl;
> -			return -EINVAL;
> +			return nullptr;
>  		}
>  	}
>  
> -	*config = camera->generateConfiguration(roles);
> -
> -	if (!config->isValid()) {
> +	CameraConfiguration *config = camera->generateConfiguration(roles);
> +	if (!config || !config->isValid()) {
>  		std::cerr << "Failed to get default stream configuration"
>  			  << std::endl;
> -		return -EINVAL;
> +		delete config;
> +		return nullptr;
>  	}
>  
>  	/* Apply configuration explicitly requested. */
> @@ -142,7 +140,7 @@ static int prepareCameraConfig(CameraConfiguration *config)
>  			cfg.pixelFormat = conf["pixelformat"];
>  	}
>  
> -	return 0;
> +	return config;
>  }
>  
>  static void requestComplete(Request *request, const std::map<Stream *, Buffer *> &buffers)
> @@ -191,25 +189,25 @@ static void requestComplete(Request *request, const std::map<Stream *, Buffer *>
>  
>  static int capture()
>  {
> -	CameraConfiguration config;
>  	int ret;
>  
> -	ret = prepareCameraConfig(&config);
> -	if (ret) {
> +	CameraConfiguration *config = prepareCameraConfig();
> +	if (!config) {
>  		std::cout << "Failed to prepare camera configuration" << std::endl;
> -		return ret;
> +		return -EINVAL;
>  	}
>  
>  	ret = camera->configure(config);
>  	if (ret < 0) {
>  		std::cout << "Failed to configure camera" << std::endl;
> +		delete config;
>  		return ret;
>  	}
>  
>  	streamInfo.clear();
>  
> -	for (unsigned int index = 0; index < config.size(); ++index) {
> -		StreamConfiguration &cfg = config[index];
> +	for (unsigned int index = 0; index < config->size(); ++index) {
> +		StreamConfiguration &cfg = (*config)[index];
>  		streamInfo[cfg.stream()] = "stream" + std::to_string(index);
>  	}
>  
> @@ -217,6 +215,7 @@ static int capture()
>  	if (ret) {
>  		std::cerr << "Failed to allocate buffers"
>  			  << std::endl;
> +		delete config;
>  		return ret;
>  	}
>  
> @@ -224,7 +223,7 @@ static int capture()
>  
>  	/* Identify the stream with the least number of buffers. */
>  	unsigned int nbuffers = UINT_MAX;
> -	for (StreamConfiguration &cfg : config) {
> +	for (StreamConfiguration &cfg : *config) {
>  		Stream *stream = cfg.stream();
>  		nbuffers = std::min(nbuffers, stream->bufferPool().count());
>  	}
> @@ -244,7 +243,7 @@ static int capture()
>  		}
>  
>  		std::map<Stream *, Buffer *> map;
> -		for (StreamConfiguration &cfg : config) {
> +		for (StreamConfiguration &cfg : *config) {
>  			Stream *stream = cfg.stream();
>  			map[stream] = &stream->bufferPool().buffers()[i];
>  		}
> @@ -280,6 +279,7 @@ static int capture()
>  		std::cout << "Failed to stop capture" << std::endl;
>  out:
>  	camera->freeBuffers();
> +	delete config;
>  
>  	return ret;
>  }
> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> index f8a4946b4a6a..115cdb1c024b 100644
> --- a/src/libcamera/camera.cpp
> +++ b/src/libcamera/camera.cpp
> @@ -518,21 +518,24 @@ const std::set<Stream *> &Camera::streams() const
>   * specifies a list of stream roles and the camera returns a configuration
>   * containing suitable streams and their suggested default configurations.
>   *
> - * \return A valid CameraConfiguration if the requested roles can be satisfied,
> - * or a invalid one otherwise
> + * \return A CameraConfiguration if the requested roles can be satisfied, or a
> + * null pointer otherwise. The ownership of the returned configuration is
> + * passed to the caller.
>   */
> -CameraConfiguration
> -Camera::generateConfiguration(const StreamRoles &roles)
> +CameraConfiguration *Camera::generateConfiguration(const StreamRoles &roles)
>  {
> -	if (disconnected_ || !roles.size() || roles.size() > streams_.size())
> -		return CameraConfiguration();
> +	if (disconnected_ || roles.size() > streams_.size())
> +		return nullptr;
>  
> -	CameraConfiguration config = pipe_->generateConfiguration(this, roles);
> +	CameraConfiguration *config = pipe_->generateConfiguration(this, roles);
>  
>  	std::ostringstream msg("streams configuration:", std::ios_base::ate);
>  
> -	for (unsigned int index = 0; index < config.size(); ++index)
> -		msg << " (" << index << ") " << config[index].toString();
> +	if (config->isEmpty())
> +		msg << " empty";
> +
> +	for (unsigned int index = 0; index < config->size(); ++index)
> +		msg << " (" << index << ") " << (*config)[index].toString();
>  
>  	LOG(Camera, Debug) << msg.str();
>  
> @@ -566,7 +569,7 @@ Camera::generateConfiguration(const StreamRoles &roles)
>   * \retval -EACCES The camera is not in a state where it can be configured
>   * \retval -EINVAL The configuration is not valid
>   */
> -int Camera::configure(CameraConfiguration &config)
> +int Camera::configure(CameraConfiguration *config)
>  {
>  	int ret;
>  
> @@ -576,7 +579,7 @@ int Camera::configure(CameraConfiguration &config)
>  	if (!stateBetween(CameraAcquired, CameraConfigured))
>  		return -EACCES;
>  
> -	if (!config.isValid()) {
> +	if (!config->isValid()) {
>  		LOG(Camera, Error)
>  			<< "Can't configure camera with invalid configuration";
>  		return -EINVAL;
> @@ -584,8 +587,8 @@ int Camera::configure(CameraConfiguration &config)
>  
>  	std::ostringstream msg("configuring streams:", std::ios_base::ate);
>  
> -	for (unsigned int index = 0; index < config.size(); ++index) {
> -		StreamConfiguration &cfg = config[index];
> +	for (unsigned int index = 0; index < config->size(); ++index) {
> +		StreamConfiguration &cfg = (*config)[index];
>  		cfg.setStream(nullptr);
>  		msg << " (" << index << ") " << cfg.toString();
>  	}
> @@ -597,7 +600,7 @@ int Camera::configure(CameraConfiguration &config)
>  		return ret;
>  
>  	activeStreams_.clear();
> -	for (const StreamConfiguration &cfg : config) {
> +	for (const StreamConfiguration &cfg : *config) {
>  		Stream *stream = cfg.stream();
>  		if (!stream)
>  			LOG(Camera, Fatal)
> diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h
> index a025905ab68f..7da6df1ab2a0 100644
> --- a/src/libcamera/include/pipeline_handler.h
> +++ b/src/libcamera/include/pipeline_handler.h
> @@ -60,9 +60,9 @@ public:
>  	bool lock();
>  	void unlock();
>  
> -	virtual CameraConfiguration
> -	generateConfiguration(Camera *camera, const StreamRoles &roles) = 0;
> -	virtual int configure(Camera *camera, CameraConfiguration &config) = 0;
> +	virtual CameraConfiguration *generateConfiguration(Camera *camera,
> +		const StreamRoles &roles) = 0;
> +	virtual int configure(Camera *camera, CameraConfiguration *config) = 0;
>  
>  	virtual int allocateBuffers(Camera *camera,
>  				    const std::set<Stream *> &streams) = 0;
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index ed0ef69de1d1..7c6f2d4a23be 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -150,9 +150,9 @@ class PipelineHandlerIPU3 : public PipelineHandler
>  public:
>  	PipelineHandlerIPU3(CameraManager *manager);
>  
> -	CameraConfiguration
> -	generateConfiguration(Camera *camera, const StreamRoles &roles) override;
> -	int configure(Camera *camera, CameraConfiguration &config) override;
> +	CameraConfiguration *generateConfiguration(Camera *camera,
> +		const StreamRoles &roles) override;
> +	int configure(Camera *camera, CameraConfiguration *config) override;
>  
>  	int allocateBuffers(Camera *camera,
>  			    const std::set<Stream *> &streams) override;
> @@ -207,12 +207,11 @@ PipelineHandlerIPU3::PipelineHandlerIPU3(CameraManager *manager)
>  {
>  }
>  
> -CameraConfiguration
> -PipelineHandlerIPU3::generateConfiguration(Camera *camera,
> -					   const StreamRoles &roles)
> +CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,
> +	const StreamRoles &roles)
>  {
>  	IPU3CameraData *data = cameraData(camera);
> -	CameraConfiguration config = {};
> +	CameraConfiguration *config = new CameraConfiguration();
>  	std::set<IPU3Stream *> streams = {
>  		&data->outStream_,
>  		&data->vfStream_,
> @@ -290,21 +289,23 @@ PipelineHandlerIPU3::generateConfiguration(Camera *camera,
>  			break;
>  		}
>  
> -		if (!stream)
> -			return CameraConfiguration{};
> +		if (!stream) {
> +			delete config;
> +			return nullptr;
> +		}
>  
>  		streams.erase(stream);
>  
>  		cfg.pixelFormat = V4L2_PIX_FMT_NV12;
>  		cfg.bufferCount = IPU3_BUFFER_COUNT;
>  
> -		config.addConfiguration(cfg);
> +		config->addConfiguration(cfg);
>  	}
>  
>  	return config;
>  }
>  
> -int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration &config)
> +int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *config)
>  {
>  	IPU3CameraData *data = cameraData(camera);
>  	IPU3Stream *outStream = &data->outStream_;
> @@ -316,7 +317,7 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration &config)
>  
>  	outStream->active_ = false;
>  	vfStream->active_ = false;
> -	for (StreamConfiguration &cfg : config) {
> +	for (StreamConfiguration &cfg : *config) {
>  		/*
>  		 * Pick a stream for the configuration entry.
>  		 * \todo: This is a naive temporary implementation that will be
> @@ -382,7 +383,7 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration &config)
>  		return ret;
>  
>  	/* Apply the format to the configured streams output devices. */
> -	for (StreamConfiguration &cfg : config) {
> +	for (StreamConfiguration &cfg : *config) {
>  		IPU3Stream *stream = static_cast<IPU3Stream *>(cfg.stream());
>  		ret = imgu->configureOutput(stream->device_, cfg);
>  		if (ret)
> @@ -395,13 +396,13 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration &config)
>  	 * be at least one active stream in the configuration request).
>  	 */
>  	if (!outStream->active_) {
> -		ret = imgu->configureOutput(outStream->device_, config[0]);
> +		ret = imgu->configureOutput(outStream->device_, (*config)[0]);
>  		if (ret)
>  			return ret;
>  	}
>  
>  	if (!vfStream->active_) {
> -		ret = imgu->configureOutput(vfStream->device_, config[0]);
> +		ret = imgu->configureOutput(vfStream->device_, (*config)[0]);
>  		if (ret)
>  			return ret;
>  	}
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 6b35aa3fe7c3..c3b3912c96f3 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -34,9 +34,9 @@ public:
>  	PipelineHandlerRkISP1(CameraManager *manager);
>  	~PipelineHandlerRkISP1();
>  
> -	CameraConfiguration generateConfiguration(Camera *camera,
> +	CameraConfiguration *generateConfiguration(Camera *camera,
>  		const StreamRoles &roles) override;
> -	int configure(Camera *camera, CameraConfiguration &config) override;
> +	int configure(Camera *camera, CameraConfiguration *config) override;
>  
>  	int allocateBuffers(Camera *camera,
>  		const std::set<Stream *> &streams) override;
> @@ -105,26 +105,29 @@ PipelineHandlerRkISP1::~PipelineHandlerRkISP1()
>   * Pipeline Operations
>   */
>  
> -CameraConfiguration PipelineHandlerRkISP1::generateConfiguration(Camera *camera,
> +CameraConfiguration *PipelineHandlerRkISP1::generateConfiguration(Camera *camera,
>  	const StreamRoles &roles)
>  {
>  	RkISP1CameraData *data = cameraData(camera);
> -	CameraConfiguration config;
> -	StreamConfiguration cfg{};
> +	CameraConfiguration *config = new CameraConfiguration();
>  
> -	cfg.pixelFormat = V4L2_PIX_FMT_NV12;
> -	cfg.size = data->sensor_->resolution();
> -	cfg.bufferCount = RKISP1_BUFFER_COUNT;
> +	if (!roles.empty()) {
> +		StreamConfiguration cfg{};
>  
> -	config.addConfiguration(cfg);
> +		cfg.pixelFormat = V4L2_PIX_FMT_NV12;
> +		cfg.size = data->sensor_->resolution();
> +		cfg.bufferCount = RKISP1_BUFFER_COUNT;
> +
> +		config->addConfiguration(cfg);
> +	}
>  
>  	return config;
>  }
>  
> -int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration &config)
> +int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *config)
>  {
>  	RkISP1CameraData *data = cameraData(camera);
> -	StreamConfiguration &cfg = config[0];
> +	StreamConfiguration &cfg = (*config)[0];
>  	CameraSensor *sensor = data->sensor_;
>  	int ret;
>  
> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp
> index 5dcc868b2fc9..5c061ca61016 100644
> --- a/src/libcamera/pipeline/uvcvideo.cpp
> +++ b/src/libcamera/pipeline/uvcvideo.cpp
> @@ -25,9 +25,9 @@ class PipelineHandlerUVC : public PipelineHandler
>  public:
>  	PipelineHandlerUVC(CameraManager *manager);
>  
> -	CameraConfiguration
> -	generateConfiguration(Camera *camera, const StreamRoles &roles) override;
> -	int configure(Camera *camera, CameraConfiguration &config) override;
> +	CameraConfiguration *generateConfiguration(Camera *camera,
> +		const StreamRoles &roles) override;
> +	int configure(Camera *camera, CameraConfiguration *config) override;
>  
>  	int allocateBuffers(Camera *camera,
>  			    const std::set<Stream *> &streams) override;
> @@ -73,26 +73,28 @@ PipelineHandlerUVC::PipelineHandlerUVC(CameraManager *manager)
>  {
>  }
>  
> -CameraConfiguration
> -PipelineHandlerUVC::generateConfiguration(Camera *camera,
> -					  const StreamRoles &roles)
> +CameraConfiguration *PipelineHandlerUVC::generateConfiguration(Camera *camera,
> +	const StreamRoles &roles)
>  {
> -	CameraConfiguration config;
> -	StreamConfiguration cfg;
> +	CameraConfiguration *config = new CameraConfiguration();
>  
> -	cfg.pixelFormat = V4L2_PIX_FMT_YUYV;
> -	cfg.size = { 640, 480 };
> -	cfg.bufferCount = 4;
> +	if (!roles.empty()) {
> +		StreamConfiguration cfg;
>  
> -	config.addConfiguration(cfg);
> +		cfg.pixelFormat = V4L2_PIX_FMT_YUYV;
> +		cfg.size = { 640, 480 };
> +		cfg.bufferCount = 4;
> +
> +		config->addConfiguration(cfg);
> +	}
>  
>  	return config;
>  }
>  
> -int PipelineHandlerUVC::configure(Camera *camera, CameraConfiguration &config)
> +int PipelineHandlerUVC::configure(Camera *camera, CameraConfiguration *config)
>  {
>  	UVCCameraData *data = cameraData(camera);
> -	StreamConfiguration &cfg = config[0];
> +	StreamConfiguration &cfg = (*config)[0];
>  	int ret;
>  
>  	V4L2DeviceFormat format = {};
> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp
> index af6b6f21e3c5..0ece97f09e7e 100644
> --- a/src/libcamera/pipeline/vimc.cpp
> +++ b/src/libcamera/pipeline/vimc.cpp
> @@ -25,9 +25,9 @@ class PipelineHandlerVimc : public PipelineHandler
>  public:
>  	PipelineHandlerVimc(CameraManager *manager);
>  
> -	CameraConfiguration
> -	generateConfiguration(Camera *camera, const StreamRoles &roles) override;
> -	int configure(Camera *camera, CameraConfiguration &config) override;
> +	CameraConfiguration *generateConfiguration(Camera *camera,
> +		const StreamRoles &roles) override;
> +	int configure(Camera *camera, CameraConfiguration *config) override;
>  
>  	int allocateBuffers(Camera *camera,
>  			    const std::set<Stream *> &streams) override;
> @@ -73,26 +73,28 @@ PipelineHandlerVimc::PipelineHandlerVimc(CameraManager *manager)
>  {
>  }
>  
> -CameraConfiguration
> -PipelineHandlerVimc::generateConfiguration(Camera *camera,
> -					   const StreamRoles &roles)
> +CameraConfiguration *PipelineHandlerVimc::generateConfiguration(Camera *camera,
> +	const StreamRoles &roles)
>  {
> -	CameraConfiguration config;
> -	StreamConfiguration cfg;
> +	CameraConfiguration *config = new CameraConfiguration();
>  
> -	cfg.pixelFormat = V4L2_PIX_FMT_RGB24;
> -	cfg.size = { 640, 480 };
> -	cfg.bufferCount = 4;
> +	if (!roles.empty()) {
> +		StreamConfiguration cfg;
>  
> -	config.addConfiguration(cfg);
> +		cfg.pixelFormat = V4L2_PIX_FMT_RGB24;
> +		cfg.size = { 640, 480 };
> +		cfg.bufferCount = 4;
> +
> +		config->addConfiguration(cfg);
> +	}
>  
>  	return config;
>  }
>  
> -int PipelineHandlerVimc::configure(Camera *camera, CameraConfiguration &config)
> +int PipelineHandlerVimc::configure(Camera *camera, CameraConfiguration *config)
>  {
>  	VimcCameraData *data = cameraData(camera);
> -	StreamConfiguration &cfg = config[0];
> +	StreamConfiguration &cfg = (*config)[0];
>  	int ret;
>  
>  	V4L2DeviceFormat format = {};
> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> index 4185e3557dcb..de46e98880a2 100644
> --- a/src/libcamera/pipeline_handler.cpp
> +++ b/src/libcamera/pipeline_handler.cpp
> @@ -233,7 +233,8 @@ void PipelineHandler::unlock()
>   * the group of streams parameters.
>   *
>   * \return A valid CameraConfiguration if the requested roles can be satisfied,
> - * or a invalid configuration otherwise
> + * or a null pointer otherwise. The ownership of the returned configuration is
> + * passed to the caller.
>   */
>  
>  /**
> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
> index 06ae2985f80d..9f0454b7a5f3 100644
> --- a/src/qcam/main_window.cpp
> +++ b/src/qcam/main_window.cpp
> @@ -104,7 +104,7 @@ int MainWindow::startCapture()
>  		return ret;
>  	}
>  
> -	const StreamConfiguration &cfg = config_[0];
> +	const StreamConfiguration &cfg = (*config_)[0];
>  	Stream *stream = cfg.stream();
>  	ret = viewfinder_->setFormat(cfg.pixelFormat, cfg.size.width,
>  				     cfg.size.height);
> @@ -180,6 +180,9 @@ void MainWindow::stopCapture()
>  
>  	camera_->freeBuffers();
>  	isCapturing_ = false;
> +
> +	delete config_;
> +	config_ = nullptr;
>  }
>  
>  void MainWindow::requestComplete(Request *request,
> diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h
> index 143b5b08a5a0..866866e93d23 100644
> --- a/src/qcam/main_window.h
> +++ b/src/qcam/main_window.h
> @@ -45,7 +45,7 @@ private:
>  
>  	std::shared_ptr<Camera> camera_;
>  	bool isCapturing_;
> -	CameraConfiguration config_;
> +	CameraConfiguration *config_;
>  
>  	ViewFinder *viewfinder_;
>  };
> diff --git a/test/camera/capture.cpp b/test/camera/capture.cpp
> index bfd11eefedcf..f640e80fbaf3 100644
> --- a/test/camera/capture.cpp
> +++ b/test/camera/capture.cpp
> @@ -40,13 +40,25 @@ protected:
>  		camera_->queueRequest(request);
>  	}
>  
> -	int run()
> +	int init() override
>  	{
> -		CameraConfiguration config =
> -			camera_->generateConfiguration({ StreamRole::VideoRecording });
> -		StreamConfiguration *cfg = &config[0];
> +		CameraTest::init();
>  
> -		if (!config.isValid()) {
> +		config_ = camera_->generateConfiguration({ StreamRole::VideoRecording });
> +		if (!config_) {
> +			cout << "Failed to generate default configuration" << endl;
> +			CameraTest::cleanup();
> +			return TestFail;
> +		}
> +
> +		return TestPass;
> +	}
> +
> +	int run() override
> +	{
> +		StreamConfiguration &cfg = (*config_)[0];
> +
> +		if (!config_->isValid()) {
>  			cout << "Failed to read default configuration" << endl;
>  			return TestFail;
>  		}
> @@ -56,7 +68,7 @@ protected:
>  			return TestFail;
>  		}
>  
> -		if (camera_->configure(config)) {
> +		if (camera_->configure(config_)) {
>  			cout << "Failed to set default configuration" << endl;
>  			return TestFail;
>  		}
> @@ -66,7 +78,7 @@ protected:
>  			return TestFail;
>  		}
>  
> -		Stream *stream = cfg->stream();
> +		Stream *stream = cfg.stream();
>  		BufferPool &pool = stream->bufferPool();
>  		std::vector<Request *> requests;
>  		for (Buffer &buffer : pool.buffers()) {
> @@ -110,10 +122,10 @@ protected:
>  		while (timer.isRunning())
>  			dispatcher->processEvents();
>  
> -		if (completeRequestsCount_ <= cfg->bufferCount * 2) {
> +		if (completeRequestsCount_ <= cfg.bufferCount * 2) {
>  			cout << "Failed to capture enough frames (got "
>  			     << completeRequestsCount_ << " expected at least "
> -			     << cfg->bufferCount * 2 << ")" << endl;
> +			     << cfg.bufferCount * 2 << ")" << endl;
>  			return TestFail;
>  		}
>  
> @@ -134,6 +146,14 @@ protected:
>  
>  		return TestPass;
>  	}
> +
> +	void cleanup() override
> +	{
> +		delete config_;
> +		CameraTest::cleanup();
> +	}
> +
> +	CameraConfiguration *config_;
>  };
>  
>  } /* namespace */
> diff --git a/test/camera/configuration_default.cpp b/test/camera/configuration_default.cpp
> index 8c4a03db498a..d5cefc1127c9 100644
> --- a/test/camera/configuration_default.cpp
> +++ b/test/camera/configuration_default.cpp
> @@ -18,26 +18,32 @@ class ConfigurationDefault : public CameraTest
>  protected:
>  	int run()
>  	{
> -		CameraConfiguration config;
> +		CameraConfiguration *config;
>  
>  		/* Test asking for configuration for a video stream. */
>  		config = camera_->generateConfiguration({ StreamRole::VideoRecording });
> -		if (!config.isValid()) {
> +		if (!config || !config->isValid()) {
>  			cout << "Default configuration invalid" << endl;
> +			delete config;
>  			return TestFail;
>  		}
>  
> +		delete config;
> +
>  		/*
>  		 * Test that asking for configuration for an empty array of
> -		 * stream roles returns an empty list of configurations.
> +		 * stream roles returns an empty camera configuration.
>  		 */
>  		config = camera_->generateConfiguration({});
> -		if (config.isValid()) {
> +		if (!config || config->isValid()) {
>  			cout << "Failed to retrieve configuration for empty roles list"
>  			     << endl;
> +			delete config;
>  			return TestFail;
>  		}
>  
> +		delete config;
> +
>  		return TestPass;
>  	}
>  };
> diff --git a/test/camera/configuration_set.cpp b/test/camera/configuration_set.cpp
> index 25b5db67103a..ca911f459c32 100644
> --- a/test/camera/configuration_set.cpp
> +++ b/test/camera/configuration_set.cpp
> @@ -16,13 +16,25 @@ namespace {
>  class ConfigurationSet : public CameraTest
>  {
>  protected:
> -	int run()
> +	int init() override
>  	{
> -		CameraConfiguration config =
> -			camera_->generateConfiguration({ StreamRole::VideoRecording });
> -		StreamConfiguration *cfg = &config[0];
> +		CameraTest::init();
>  
> -		if (!config.isValid()) {
> +		config_ = camera_->generateConfiguration({ StreamRole::VideoRecording });
> +		if (!config_) {
> +			cout << "Failed to generate default configuration" << endl;
> +			CameraTest::cleanup();
> +			return TestFail;
> +		}
> +
> +		return TestPass;
> +	}
> +
> +	int run() override
> +	{
> +		StreamConfiguration &cfg = (*config_)[0];
> +
> +		if (!config_->isValid()) {
>  			cout << "Failed to read default configuration" << endl;
>  			return TestFail;
>  		}
> @@ -33,7 +45,7 @@ protected:
>  		}
>  
>  		/* Test that setting the default configuration works. */
> -		if (camera_->configure(config)) {
> +		if (camera_->configure(config_)) {
>  			cout << "Failed to set default configuration" << endl;
>  			return TestFail;
>  		}
> @@ -48,7 +60,7 @@ protected:
>  			return TestFail;
>  		}
>  
> -		if (!camera_->configure(config)) {
> +		if (!camera_->configure(config_)) {
>  			cout << "Setting configuration on a camera not acquired succeeded when it should have failed"
>  			     << endl;
>  			return TestFail;
> @@ -64,9 +76,9 @@ protected:
>  		 * the default configuration of the VIMC camera is known to
>  		 * work.
>  		 */
> -		cfg->size.width *= 2;
> -		cfg->size.height *= 2;
> -		if (camera_->configure(config)) {
> +		cfg.size.width *= 2;
> +		cfg.size.height *= 2;
> +		if (camera_->configure(config_)) {
>  			cout << "Failed to set modified configuration" << endl;
>  			return TestFail;
>  		}
> @@ -74,14 +86,22 @@ protected:
>  		/*
>  		 * Test that setting an invalid configuration fails.
>  		 */
> -		cfg->size = { 0, 0 };
> -		if (!camera_->configure(config)) {
> +		cfg.size = { 0, 0 };
> +		if (!camera_->configure(config_)) {
>  			cout << "Invalid configuration incorrectly accepted" << endl;
>  			return TestFail;
>  		}
>  
>  		return TestPass;
>  	}
> +
> +	void cleanup() override
> +	{
> +		delete config_;
> +		CameraTest::cleanup();
> +	}
> +
> +	CameraConfiguration *config_;
>  };
>  
>  } /* namespace */
> diff --git a/test/camera/statemachine.cpp b/test/camera/statemachine.cpp
> index 7a74cd85a37a..a662e869fadf 100644
> --- a/test/camera/statemachine.cpp
> +++ b/test/camera/statemachine.cpp
> @@ -233,10 +233,22 @@ protected:
>  		return TestPass;
>  	}
>  
> -	int run()
> +	int init() override
>  	{
> +		CameraTest::init();
> +
>  		defconf_ = camera_->generateConfiguration({ StreamRole::VideoRecording });
> +		if (!defconf_) {
> +			cout << "Failed to generate default configuration" << endl;
> +			CameraTest::cleanup();
> +			return TestFail;
> +		}
>  
> +		return TestPass;
> +	}
> +
> +	int run() override
> +	{
>  		if (testAvailable() != TestPass) {
>  			cout << "State machine in Available state failed" << endl;
>  			return TestFail;
> @@ -265,7 +277,13 @@ protected:
>  		return TestPass;
>  	}
>  
> -	CameraConfiguration defconf_;
> +	void cleanup() override
> +	{
> +		delete defconf_;
> +		CameraTest::cleanup();
> +	}
> +
> +	CameraConfiguration *defconf_;
>  };
>  
>  } /* namespace */
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Hi Niklas,

On Sat, May 18, 2019 at 06:41:27PM +0200, Niklas Söderlund wrote:
> On 2019-05-18 02:06:14 +0300, Laurent Pinchart wrote:
> > To prepare for specialising the CameraConfiguration class in pipeline
> > handlers, return a pointer to a camera configuration instead of a
> > reference from Camera::generateConfiguration(). The camera configuration
> > always needs to be allocated from the pipeline handler, and its
> > ownership is passed to the application.
> 
> I understand the need for pipeline handlers to be able to specialise the 
> CameraConfiguration, at the same time I'm not super happy with how this 
> change effects the API towards applications. Specially as we model the 
> CameraConfiguration around a vector.
> 
>     CameraConfiguration *config = ... prepareCameraConfig();
>     StreamConfiguration &cfg = (*config)[index];
> 
> Is not the most intuitive interface ;-)

I've seen worse :-) A vector pointer isn't that bad.

> I wonder if we could improve here using a d-pointer design and a private 
> pointer with a reference count, or maybe even use a shared_ptr<> 
> internally?

If we do this I fear it will make the interface towards pipeline
handlers much more complex. We will need to allow copies of
CameraConfiguration, which internally will need to duplicate the
pipeline-handler specific data stored in the CameraConfiguration. I
don't really see how we could keep it clean and simple. Of course, feel
free to prove me wrong :-)

> > For symmetry, change Camera::configure() to take a CameraConfiguration
> > pointer instead of a reference. This aligns with our coding practice of
> > passing parameters that are modified by the callee by pointer.
> 
> This agree with and should be changed disregarding of the above, and 
> maybe even moved to a patch (or it's own patch) earlier in this series
> where 'const' is dropped from the argument.

Let's first see if we find a good way to return configurations by value
instead of pointer. If we don't I think we can bundle the two changes in
this patch.

> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  include/libcamera/camera.h               |  4 +--
> >  src/cam/main.cpp                         | 38 ++++++++++----------
> >  src/libcamera/camera.cpp                 | 31 +++++++++--------
> >  src/libcamera/include/pipeline_handler.h |  6 ++--
> >  src/libcamera/pipeline/ipu3/ipu3.cpp     | 31 +++++++++--------
> >  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 25 ++++++++------
> >  src/libcamera/pipeline/uvcvideo.cpp      | 30 ++++++++--------
> >  src/libcamera/pipeline/vimc.cpp          | 30 ++++++++--------
> >  src/libcamera/pipeline_handler.cpp       |  3 +-
> >  src/qcam/main_window.cpp                 |  5 ++-
> >  src/qcam/main_window.h                   |  2 +-
> >  test/camera/capture.cpp                  | 38 +++++++++++++++-----
> >  test/camera/configuration_default.cpp    | 14 +++++---
> >  test/camera/configuration_set.cpp        | 44 +++++++++++++++++-------
> >  test/camera/statemachine.cpp             | 22 ++++++++++--
> >  15 files changed, 201 insertions(+), 122 deletions(-)
> > 
> > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> > index e2759405f497..841e8fc505b9 100644
> > --- a/include/libcamera/camera.h
> > +++ b/include/libcamera/camera.h
> > @@ -68,8 +68,8 @@ public:
> >  	int release();
> >  
> >  	const std::set<Stream *> &streams() const;
> > -	CameraConfiguration generateConfiguration(const StreamRoles &roles);
> > -	int configure(CameraConfiguration &config);
> > +	CameraConfiguration *generateConfiguration(const StreamRoles &roles);
> > +	int configure(CameraConfiguration *config);
> >  
> >  	int allocateBuffers();
> >  	int freeBuffers();
> > diff --git a/src/cam/main.cpp b/src/cam/main.cpp
> > index cd165bea34cd..7a1b332f68c7 100644
> > --- a/src/cam/main.cpp
> > +++ b/src/cam/main.cpp
> > @@ -85,15 +85,13 @@ static int parseOptions(int argc, char *argv[])
> >  	return 0;
> >  }
> >  
> > -static int prepareCameraConfig(CameraConfiguration *config)
> > +static CameraConfiguration *prepareCameraConfig()
> >  {
> >  	StreamRoles roles;
> >  
> >  	/* If no configuration is provided assume a single video stream. */
> > -	if (!options.isSet(OptStream)) {
> > -		*config = camera->generateConfiguration({ StreamRole::VideoRecording });
> > -		return 0;
> > -	}
> > +	if (!options.isSet(OptStream))
> > +		return camera->generateConfiguration({ StreamRole::VideoRecording });
> >  
> >  	const std::vector<OptionValue> &streamOptions =
> >  		options[OptStream].toArray();
> > @@ -113,16 +111,16 @@ static int prepareCameraConfig(CameraConfiguration *config)
> >  		} else {
> >  			std::cerr << "Unknown stream role "
> >  				  << conf["role"].toString() << std::endl;
> > -			return -EINVAL;
> > +			return nullptr;
> >  		}
> >  	}
> >  
> > -	*config = camera->generateConfiguration(roles);
> > -
> > -	if (!config->isValid()) {
> > +	CameraConfiguration *config = camera->generateConfiguration(roles);
> > +	if (!config || !config->isValid()) {
> >  		std::cerr << "Failed to get default stream configuration"
> >  			  << std::endl;
> > -		return -EINVAL;
> > +		delete config;
> > +		return nullptr;
> >  	}
> >  
> >  	/* Apply configuration explicitly requested. */
> > @@ -142,7 +140,7 @@ static int prepareCameraConfig(CameraConfiguration *config)
> >  			cfg.pixelFormat = conf["pixelformat"];
> >  	}
> >  
> > -	return 0;
> > +	return config;
> >  }
> >  
> >  static void requestComplete(Request *request, const std::map<Stream *, Buffer *> &buffers)
> > @@ -191,25 +189,25 @@ static void requestComplete(Request *request, const std::map<Stream *, Buffer *>
> >  
> >  static int capture()
> >  {
> > -	CameraConfiguration config;
> >  	int ret;
> >  
> > -	ret = prepareCameraConfig(&config);
> > -	if (ret) {
> > +	CameraConfiguration *config = prepareCameraConfig();
> > +	if (!config) {
> >  		std::cout << "Failed to prepare camera configuration" << std::endl;
> > -		return ret;
> > +		return -EINVAL;
> >  	}
> >  
> >  	ret = camera->configure(config);
> >  	if (ret < 0) {
> >  		std::cout << "Failed to configure camera" << std::endl;
> > +		delete config;
> >  		return ret;
> >  	}
> >  
> >  	streamInfo.clear();
> >  
> > -	for (unsigned int index = 0; index < config.size(); ++index) {
> > -		StreamConfiguration &cfg = config[index];
> > +	for (unsigned int index = 0; index < config->size(); ++index) {
> > +		StreamConfiguration &cfg = (*config)[index];
> >  		streamInfo[cfg.stream()] = "stream" + std::to_string(index);
> >  	}
> >  
> > @@ -217,6 +215,7 @@ static int capture()
> >  	if (ret) {
> >  		std::cerr << "Failed to allocate buffers"
> >  			  << std::endl;
> > +		delete config;
> >  		return ret;
> >  	}
> >  
> > @@ -224,7 +223,7 @@ static int capture()
> >  
> >  	/* Identify the stream with the least number of buffers. */
> >  	unsigned int nbuffers = UINT_MAX;
> > -	for (StreamConfiguration &cfg : config) {
> > +	for (StreamConfiguration &cfg : *config) {
> >  		Stream *stream = cfg.stream();
> >  		nbuffers = std::min(nbuffers, stream->bufferPool().count());
> >  	}
> > @@ -244,7 +243,7 @@ static int capture()
> >  		}
> >  
> >  		std::map<Stream *, Buffer *> map;
> > -		for (StreamConfiguration &cfg : config) {
> > +		for (StreamConfiguration &cfg : *config) {
> >  			Stream *stream = cfg.stream();
> >  			map[stream] = &stream->bufferPool().buffers()[i];
> >  		}
> > @@ -280,6 +279,7 @@ static int capture()
> >  		std::cout << "Failed to stop capture" << std::endl;
> >  out:
> >  	camera->freeBuffers();
> > +	delete config;
> >  
> >  	return ret;
> >  }
> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > index f8a4946b4a6a..115cdb1c024b 100644
> > --- a/src/libcamera/camera.cpp
> > +++ b/src/libcamera/camera.cpp
> > @@ -518,21 +518,24 @@ const std::set<Stream *> &Camera::streams() const
> >   * specifies a list of stream roles and the camera returns a configuration
> >   * containing suitable streams and their suggested default configurations.
> >   *
> > - * \return A valid CameraConfiguration if the requested roles can be satisfied,
> > - * or a invalid one otherwise
> > + * \return A CameraConfiguration if the requested roles can be satisfied, or a
> > + * null pointer otherwise. The ownership of the returned configuration is
> > + * passed to the caller.
> >   */
> > -CameraConfiguration
> > -Camera::generateConfiguration(const StreamRoles &roles)
> > +CameraConfiguration *Camera::generateConfiguration(const StreamRoles &roles)
> >  {
> > -	if (disconnected_ || !roles.size() || roles.size() > streams_.size())
> > -		return CameraConfiguration();
> > +	if (disconnected_ || roles.size() > streams_.size())
> > +		return nullptr;
> >  
> > -	CameraConfiguration config = pipe_->generateConfiguration(this, roles);
> > +	CameraConfiguration *config = pipe_->generateConfiguration(this, roles);
> >  
> >  	std::ostringstream msg("streams configuration:", std::ios_base::ate);
> >  
> > -	for (unsigned int index = 0; index < config.size(); ++index)
> > -		msg << " (" << index << ") " << config[index].toString();
> > +	if (config->isEmpty())
> > +		msg << " empty";
> > +
> > +	for (unsigned int index = 0; index < config->size(); ++index)
> > +		msg << " (" << index << ") " << (*config)[index].toString();
> >  
> >  	LOG(Camera, Debug) << msg.str();
> >  
> > @@ -566,7 +569,7 @@ Camera::generateConfiguration(const StreamRoles &roles)
> >   * \retval -EACCES The camera is not in a state where it can be configured
> >   * \retval -EINVAL The configuration is not valid
> >   */
> > -int Camera::configure(CameraConfiguration &config)
> > +int Camera::configure(CameraConfiguration *config)
> >  {
> >  	int ret;
> >  
> > @@ -576,7 +579,7 @@ int Camera::configure(CameraConfiguration &config)
> >  	if (!stateBetween(CameraAcquired, CameraConfigured))
> >  		return -EACCES;
> >  
> > -	if (!config.isValid()) {
> > +	if (!config->isValid()) {
> >  		LOG(Camera, Error)
> >  			<< "Can't configure camera with invalid configuration";
> >  		return -EINVAL;
> > @@ -584,8 +587,8 @@ int Camera::configure(CameraConfiguration &config)
> >  
> >  	std::ostringstream msg("configuring streams:", std::ios_base::ate);
> >  
> > -	for (unsigned int index = 0; index < config.size(); ++index) {
> > -		StreamConfiguration &cfg = config[index];
> > +	for (unsigned int index = 0; index < config->size(); ++index) {
> > +		StreamConfiguration &cfg = (*config)[index];
> >  		cfg.setStream(nullptr);
> >  		msg << " (" << index << ") " << cfg.toString();
> >  	}
> > @@ -597,7 +600,7 @@ int Camera::configure(CameraConfiguration &config)
> >  		return ret;
> >  
> >  	activeStreams_.clear();
> > -	for (const StreamConfiguration &cfg : config) {
> > +	for (const StreamConfiguration &cfg : *config) {
> >  		Stream *stream = cfg.stream();
> >  		if (!stream)
> >  			LOG(Camera, Fatal)
> > diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h
> > index a025905ab68f..7da6df1ab2a0 100644
> > --- a/src/libcamera/include/pipeline_handler.h
> > +++ b/src/libcamera/include/pipeline_handler.h
> > @@ -60,9 +60,9 @@ public:
> >  	bool lock();
> >  	void unlock();
> >  
> > -	virtual CameraConfiguration
> > -	generateConfiguration(Camera *camera, const StreamRoles &roles) = 0;
> > -	virtual int configure(Camera *camera, CameraConfiguration &config) = 0;
> > +	virtual CameraConfiguration *generateConfiguration(Camera *camera,
> > +		const StreamRoles &roles) = 0;
> > +	virtual int configure(Camera *camera, CameraConfiguration *config) = 0;
> >  
> >  	virtual int allocateBuffers(Camera *camera,
> >  				    const std::set<Stream *> &streams) = 0;
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index ed0ef69de1d1..7c6f2d4a23be 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -150,9 +150,9 @@ class PipelineHandlerIPU3 : public PipelineHandler
> >  public:
> >  	PipelineHandlerIPU3(CameraManager *manager);
> >  
> > -	CameraConfiguration
> > -	generateConfiguration(Camera *camera, const StreamRoles &roles) override;
> > -	int configure(Camera *camera, CameraConfiguration &config) override;
> > +	CameraConfiguration *generateConfiguration(Camera *camera,
> > +		const StreamRoles &roles) override;
> > +	int configure(Camera *camera, CameraConfiguration *config) override;
> >  
> >  	int allocateBuffers(Camera *camera,
> >  			    const std::set<Stream *> &streams) override;
> > @@ -207,12 +207,11 @@ PipelineHandlerIPU3::PipelineHandlerIPU3(CameraManager *manager)
> >  {
> >  }
> >  
> > -CameraConfiguration
> > -PipelineHandlerIPU3::generateConfiguration(Camera *camera,
> > -					   const StreamRoles &roles)
> > +CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,
> > +	const StreamRoles &roles)
> >  {
> >  	IPU3CameraData *data = cameraData(camera);
> > -	CameraConfiguration config = {};
> > +	CameraConfiguration *config = new CameraConfiguration();
> >  	std::set<IPU3Stream *> streams = {
> >  		&data->outStream_,
> >  		&data->vfStream_,
> > @@ -290,21 +289,23 @@ PipelineHandlerIPU3::generateConfiguration(Camera *camera,
> >  			break;
> >  		}
> >  
> > -		if (!stream)
> > -			return CameraConfiguration{};
> > +		if (!stream) {
> > +			delete config;
> > +			return nullptr;
> > +		}
> >  
> >  		streams.erase(stream);
> >  
> >  		cfg.pixelFormat = V4L2_PIX_FMT_NV12;
> >  		cfg.bufferCount = IPU3_BUFFER_COUNT;
> >  
> > -		config.addConfiguration(cfg);
> > +		config->addConfiguration(cfg);
> >  	}
> >  
> >  	return config;
> >  }
> >  
> > -int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration &config)
> > +int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *config)
> >  {
> >  	IPU3CameraData *data = cameraData(camera);
> >  	IPU3Stream *outStream = &data->outStream_;
> > @@ -316,7 +317,7 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration &config)
> >  
> >  	outStream->active_ = false;
> >  	vfStream->active_ = false;
> > -	for (StreamConfiguration &cfg : config) {
> > +	for (StreamConfiguration &cfg : *config) {
> >  		/*
> >  		 * Pick a stream for the configuration entry.
> >  		 * \todo: This is a naive temporary implementation that will be
> > @@ -382,7 +383,7 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration &config)
> >  		return ret;
> >  
> >  	/* Apply the format to the configured streams output devices. */
> > -	for (StreamConfiguration &cfg : config) {
> > +	for (StreamConfiguration &cfg : *config) {
> >  		IPU3Stream *stream = static_cast<IPU3Stream *>(cfg.stream());
> >  		ret = imgu->configureOutput(stream->device_, cfg);
> >  		if (ret)
> > @@ -395,13 +396,13 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration &config)
> >  	 * be at least one active stream in the configuration request).
> >  	 */
> >  	if (!outStream->active_) {
> > -		ret = imgu->configureOutput(outStream->device_, config[0]);
> > +		ret = imgu->configureOutput(outStream->device_, (*config)[0]);
> >  		if (ret)
> >  			return ret;
> >  	}
> >  
> >  	if (!vfStream->active_) {
> > -		ret = imgu->configureOutput(vfStream->device_, config[0]);
> > +		ret = imgu->configureOutput(vfStream->device_, (*config)[0]);
> >  		if (ret)
> >  			return ret;
> >  	}
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > index 6b35aa3fe7c3..c3b3912c96f3 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > @@ -34,9 +34,9 @@ public:
> >  	PipelineHandlerRkISP1(CameraManager *manager);
> >  	~PipelineHandlerRkISP1();
> >  
> > -	CameraConfiguration generateConfiguration(Camera *camera,
> > +	CameraConfiguration *generateConfiguration(Camera *camera,
> >  		const StreamRoles &roles) override;
> > -	int configure(Camera *camera, CameraConfiguration &config) override;
> > +	int configure(Camera *camera, CameraConfiguration *config) override;
> >  
> >  	int allocateBuffers(Camera *camera,
> >  		const std::set<Stream *> &streams) override;
> > @@ -105,26 +105,29 @@ PipelineHandlerRkISP1::~PipelineHandlerRkISP1()
> >   * Pipeline Operations
> >   */
> >  
> > -CameraConfiguration PipelineHandlerRkISP1::generateConfiguration(Camera *camera,
> > +CameraConfiguration *PipelineHandlerRkISP1::generateConfiguration(Camera *camera,
> >  	const StreamRoles &roles)
> >  {
> >  	RkISP1CameraData *data = cameraData(camera);
> > -	CameraConfiguration config;
> > -	StreamConfiguration cfg{};
> > +	CameraConfiguration *config = new CameraConfiguration();
> >  
> > -	cfg.pixelFormat = V4L2_PIX_FMT_NV12;
> > -	cfg.size = data->sensor_->resolution();
> > -	cfg.bufferCount = RKISP1_BUFFER_COUNT;
> > +	if (!roles.empty()) {
> > +		StreamConfiguration cfg{};
> >  
> > -	config.addConfiguration(cfg);
> > +		cfg.pixelFormat = V4L2_PIX_FMT_NV12;
> > +		cfg.size = data->sensor_->resolution();
> > +		cfg.bufferCount = RKISP1_BUFFER_COUNT;
> > +
> > +		config->addConfiguration(cfg);
> > +	}
> >  
> >  	return config;
> >  }
> >  
> > -int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration &config)
> > +int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *config)
> >  {
> >  	RkISP1CameraData *data = cameraData(camera);
> > -	StreamConfiguration &cfg = config[0];
> > +	StreamConfiguration &cfg = (*config)[0];
> >  	CameraSensor *sensor = data->sensor_;
> >  	int ret;
> >  
> > diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp
> > index 5dcc868b2fc9..5c061ca61016 100644
> > --- a/src/libcamera/pipeline/uvcvideo.cpp
> > +++ b/src/libcamera/pipeline/uvcvideo.cpp
> > @@ -25,9 +25,9 @@ class PipelineHandlerUVC : public PipelineHandler
> >  public:
> >  	PipelineHandlerUVC(CameraManager *manager);
> >  
> > -	CameraConfiguration
> > -	generateConfiguration(Camera *camera, const StreamRoles &roles) override;
> > -	int configure(Camera *camera, CameraConfiguration &config) override;
> > +	CameraConfiguration *generateConfiguration(Camera *camera,
> > +		const StreamRoles &roles) override;
> > +	int configure(Camera *camera, CameraConfiguration *config) override;
> >  
> >  	int allocateBuffers(Camera *camera,
> >  			    const std::set<Stream *> &streams) override;
> > @@ -73,26 +73,28 @@ PipelineHandlerUVC::PipelineHandlerUVC(CameraManager *manager)
> >  {
> >  }
> >  
> > -CameraConfiguration
> > -PipelineHandlerUVC::generateConfiguration(Camera *camera,
> > -					  const StreamRoles &roles)
> > +CameraConfiguration *PipelineHandlerUVC::generateConfiguration(Camera *camera,
> > +	const StreamRoles &roles)
> >  {
> > -	CameraConfiguration config;
> > -	StreamConfiguration cfg;
> > +	CameraConfiguration *config = new CameraConfiguration();
> >  
> > -	cfg.pixelFormat = V4L2_PIX_FMT_YUYV;
> > -	cfg.size = { 640, 480 };
> > -	cfg.bufferCount = 4;
> > +	if (!roles.empty()) {
> > +		StreamConfiguration cfg;
> >  
> > -	config.addConfiguration(cfg);
> > +		cfg.pixelFormat = V4L2_PIX_FMT_YUYV;
> > +		cfg.size = { 640, 480 };
> > +		cfg.bufferCount = 4;
> > +
> > +		config->addConfiguration(cfg);
> > +	}
> >  
> >  	return config;
> >  }
> >  
> > -int PipelineHandlerUVC::configure(Camera *camera, CameraConfiguration &config)
> > +int PipelineHandlerUVC::configure(Camera *camera, CameraConfiguration *config)
> >  {
> >  	UVCCameraData *data = cameraData(camera);
> > -	StreamConfiguration &cfg = config[0];
> > +	StreamConfiguration &cfg = (*config)[0];
> >  	int ret;
> >  
> >  	V4L2DeviceFormat format = {};
> > diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp
> > index af6b6f21e3c5..0ece97f09e7e 100644
> > --- a/src/libcamera/pipeline/vimc.cpp
> > +++ b/src/libcamera/pipeline/vimc.cpp
> > @@ -25,9 +25,9 @@ class PipelineHandlerVimc : public PipelineHandler
> >  public:
> >  	PipelineHandlerVimc(CameraManager *manager);
> >  
> > -	CameraConfiguration
> > -	generateConfiguration(Camera *camera, const StreamRoles &roles) override;
> > -	int configure(Camera *camera, CameraConfiguration &config) override;
> > +	CameraConfiguration *generateConfiguration(Camera *camera,
> > +		const StreamRoles &roles) override;
> > +	int configure(Camera *camera, CameraConfiguration *config) override;
> >  
> >  	int allocateBuffers(Camera *camera,
> >  			    const std::set<Stream *> &streams) override;
> > @@ -73,26 +73,28 @@ PipelineHandlerVimc::PipelineHandlerVimc(CameraManager *manager)
> >  {
> >  }
> >  
> > -CameraConfiguration
> > -PipelineHandlerVimc::generateConfiguration(Camera *camera,
> > -					   const StreamRoles &roles)
> > +CameraConfiguration *PipelineHandlerVimc::generateConfiguration(Camera *camera,
> > +	const StreamRoles &roles)
> >  {
> > -	CameraConfiguration config;
> > -	StreamConfiguration cfg;
> > +	CameraConfiguration *config = new CameraConfiguration();
> >  
> > -	cfg.pixelFormat = V4L2_PIX_FMT_RGB24;
> > -	cfg.size = { 640, 480 };
> > -	cfg.bufferCount = 4;
> > +	if (!roles.empty()) {
> > +		StreamConfiguration cfg;
> >  
> > -	config.addConfiguration(cfg);
> > +		cfg.pixelFormat = V4L2_PIX_FMT_RGB24;
> > +		cfg.size = { 640, 480 };
> > +		cfg.bufferCount = 4;
> > +
> > +		config->addConfiguration(cfg);
> > +	}
> >  
> >  	return config;
> >  }
> >  
> > -int PipelineHandlerVimc::configure(Camera *camera, CameraConfiguration &config)
> > +int PipelineHandlerVimc::configure(Camera *camera, CameraConfiguration *config)
> >  {
> >  	VimcCameraData *data = cameraData(camera);
> > -	StreamConfiguration &cfg = config[0];
> > +	StreamConfiguration &cfg = (*config)[0];
> >  	int ret;
> >  
> >  	V4L2DeviceFormat format = {};
> > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> > index 4185e3557dcb..de46e98880a2 100644
> > --- a/src/libcamera/pipeline_handler.cpp
> > +++ b/src/libcamera/pipeline_handler.cpp
> > @@ -233,7 +233,8 @@ void PipelineHandler::unlock()
> >   * the group of streams parameters.
> >   *
> >   * \return A valid CameraConfiguration if the requested roles can be satisfied,
> > - * or a invalid configuration otherwise
> > + * or a null pointer otherwise. The ownership of the returned configuration is
> > + * passed to the caller.
> >   */
> >  
> >  /**
> > diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
> > index 06ae2985f80d..9f0454b7a5f3 100644
> > --- a/src/qcam/main_window.cpp
> > +++ b/src/qcam/main_window.cpp
> > @@ -104,7 +104,7 @@ int MainWindow::startCapture()
> >  		return ret;
> >  	}
> >  
> > -	const StreamConfiguration &cfg = config_[0];
> > +	const StreamConfiguration &cfg = (*config_)[0];
> >  	Stream *stream = cfg.stream();
> >  	ret = viewfinder_->setFormat(cfg.pixelFormat, cfg.size.width,
> >  				     cfg.size.height);
> > @@ -180,6 +180,9 @@ void MainWindow::stopCapture()
> >  
> >  	camera_->freeBuffers();
> >  	isCapturing_ = false;
> > +
> > +	delete config_;
> > +	config_ = nullptr;
> >  }
> >  
> >  void MainWindow::requestComplete(Request *request,
> > diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h
> > index 143b5b08a5a0..866866e93d23 100644
> > --- a/src/qcam/main_window.h
> > +++ b/src/qcam/main_window.h
> > @@ -45,7 +45,7 @@ private:
> >  
> >  	std::shared_ptr<Camera> camera_;
> >  	bool isCapturing_;
> > -	CameraConfiguration config_;
> > +	CameraConfiguration *config_;
> >  
> >  	ViewFinder *viewfinder_;
> >  };
> > diff --git a/test/camera/capture.cpp b/test/camera/capture.cpp
> > index bfd11eefedcf..f640e80fbaf3 100644
> > --- a/test/camera/capture.cpp
> > +++ b/test/camera/capture.cpp
> > @@ -40,13 +40,25 @@ protected:
> >  		camera_->queueRequest(request);
> >  	}
> >  
> > -	int run()
> > +	int init() override
> >  	{
> > -		CameraConfiguration config =
> > -			camera_->generateConfiguration({ StreamRole::VideoRecording });
> > -		StreamConfiguration *cfg = &config[0];
> > +		CameraTest::init();
> >  
> > -		if (!config.isValid()) {
> > +		config_ = camera_->generateConfiguration({ StreamRole::VideoRecording });
> > +		if (!config_) {
> > +			cout << "Failed to generate default configuration" << endl;
> > +			CameraTest::cleanup();
> > +			return TestFail;
> > +		}
> > +
> > +		return TestPass;
> > +	}
> > +
> > +	int run() override
> > +	{
> > +		StreamConfiguration &cfg = (*config_)[0];
> > +
> > +		if (!config_->isValid()) {
> >  			cout << "Failed to read default configuration" << endl;
> >  			return TestFail;
> >  		}
> > @@ -56,7 +68,7 @@ protected:
> >  			return TestFail;
> >  		}
> >  
> > -		if (camera_->configure(config)) {
> > +		if (camera_->configure(config_)) {
> >  			cout << "Failed to set default configuration" << endl;
> >  			return TestFail;
> >  		}
> > @@ -66,7 +78,7 @@ protected:
> >  			return TestFail;
> >  		}
> >  
> > -		Stream *stream = cfg->stream();
> > +		Stream *stream = cfg.stream();
> >  		BufferPool &pool = stream->bufferPool();
> >  		std::vector<Request *> requests;
> >  		for (Buffer &buffer : pool.buffers()) {
> > @@ -110,10 +122,10 @@ protected:
> >  		while (timer.isRunning())
> >  			dispatcher->processEvents();
> >  
> > -		if (completeRequestsCount_ <= cfg->bufferCount * 2) {
> > +		if (completeRequestsCount_ <= cfg.bufferCount * 2) {
> >  			cout << "Failed to capture enough frames (got "
> >  			     << completeRequestsCount_ << " expected at least "
> > -			     << cfg->bufferCount * 2 << ")" << endl;
> > +			     << cfg.bufferCount * 2 << ")" << endl;
> >  			return TestFail;
> >  		}
> >  
> > @@ -134,6 +146,14 @@ protected:
> >  
> >  		return TestPass;
> >  	}
> > +
> > +	void cleanup() override
> > +	{
> > +		delete config_;
> > +		CameraTest::cleanup();
> > +	}
> > +
> > +	CameraConfiguration *config_;
> >  };
> >  
> >  } /* namespace */
> > diff --git a/test/camera/configuration_default.cpp b/test/camera/configuration_default.cpp
> > index 8c4a03db498a..d5cefc1127c9 100644
> > --- a/test/camera/configuration_default.cpp
> > +++ b/test/camera/configuration_default.cpp
> > @@ -18,26 +18,32 @@ class ConfigurationDefault : public CameraTest
> >  protected:
> >  	int run()
> >  	{
> > -		CameraConfiguration config;
> > +		CameraConfiguration *config;
> >  
> >  		/* Test asking for configuration for a video stream. */
> >  		config = camera_->generateConfiguration({ StreamRole::VideoRecording });
> > -		if (!config.isValid()) {
> > +		if (!config || !config->isValid()) {
> >  			cout << "Default configuration invalid" << endl;
> > +			delete config;
> >  			return TestFail;
> >  		}
> >  
> > +		delete config;
> > +
> >  		/*
> >  		 * Test that asking for configuration for an empty array of
> > -		 * stream roles returns an empty list of configurations.
> > +		 * stream roles returns an empty camera configuration.
> >  		 */
> >  		config = camera_->generateConfiguration({});
> > -		if (config.isValid()) {
> > +		if (!config || config->isValid()) {
> >  			cout << "Failed to retrieve configuration for empty roles list"
> >  			     << endl;
> > +			delete config;
> >  			return TestFail;
> >  		}
> >  
> > +		delete config;
> > +
> >  		return TestPass;
> >  	}
> >  };
> > diff --git a/test/camera/configuration_set.cpp b/test/camera/configuration_set.cpp
> > index 25b5db67103a..ca911f459c32 100644
> > --- a/test/camera/configuration_set.cpp
> > +++ b/test/camera/configuration_set.cpp
> > @@ -16,13 +16,25 @@ namespace {
> >  class ConfigurationSet : public CameraTest
> >  {
> >  protected:
> > -	int run()
> > +	int init() override
> >  	{
> > -		CameraConfiguration config =
> > -			camera_->generateConfiguration({ StreamRole::VideoRecording });
> > -		StreamConfiguration *cfg = &config[0];
> > +		CameraTest::init();
> >  
> > -		if (!config.isValid()) {
> > +		config_ = camera_->generateConfiguration({ StreamRole::VideoRecording });
> > +		if (!config_) {
> > +			cout << "Failed to generate default configuration" << endl;
> > +			CameraTest::cleanup();
> > +			return TestFail;
> > +		}
> > +
> > +		return TestPass;
> > +	}
> > +
> > +	int run() override
> > +	{
> > +		StreamConfiguration &cfg = (*config_)[0];
> > +
> > +		if (!config_->isValid()) {
> >  			cout << "Failed to read default configuration" << endl;
> >  			return TestFail;
> >  		}
> > @@ -33,7 +45,7 @@ protected:
> >  		}
> >  
> >  		/* Test that setting the default configuration works. */
> > -		if (camera_->configure(config)) {
> > +		if (camera_->configure(config_)) {
> >  			cout << "Failed to set default configuration" << endl;
> >  			return TestFail;
> >  		}
> > @@ -48,7 +60,7 @@ protected:
> >  			return TestFail;
> >  		}
> >  
> > -		if (!camera_->configure(config)) {
> > +		if (!camera_->configure(config_)) {
> >  			cout << "Setting configuration on a camera not acquired succeeded when it should have failed"
> >  			     << endl;
> >  			return TestFail;
> > @@ -64,9 +76,9 @@ protected:
> >  		 * the default configuration of the VIMC camera is known to
> >  		 * work.
> >  		 */
> > -		cfg->size.width *= 2;
> > -		cfg->size.height *= 2;
> > -		if (camera_->configure(config)) {
> > +		cfg.size.width *= 2;
> > +		cfg.size.height *= 2;
> > +		if (camera_->configure(config_)) {
> >  			cout << "Failed to set modified configuration" << endl;
> >  			return TestFail;
> >  		}
> > @@ -74,14 +86,22 @@ protected:
> >  		/*
> >  		 * Test that setting an invalid configuration fails.
> >  		 */
> > -		cfg->size = { 0, 0 };
> > -		if (!camera_->configure(config)) {
> > +		cfg.size = { 0, 0 };
> > +		if (!camera_->configure(config_)) {
> >  			cout << "Invalid configuration incorrectly accepted" << endl;
> >  			return TestFail;
> >  		}
> >  
> >  		return TestPass;
> >  	}
> > +
> > +	void cleanup() override
> > +	{
> > +		delete config_;
> > +		CameraTest::cleanup();
> > +	}
> > +
> > +	CameraConfiguration *config_;
> >  };
> >  
> >  } /* namespace */
> > diff --git a/test/camera/statemachine.cpp b/test/camera/statemachine.cpp
> > index 7a74cd85a37a..a662e869fadf 100644
> > --- a/test/camera/statemachine.cpp
> > +++ b/test/camera/statemachine.cpp
> > @@ -233,10 +233,22 @@ protected:
> >  		return TestPass;
> >  	}
> >  
> > -	int run()
> > +	int init() override
> >  	{
> > +		CameraTest::init();
> > +
> >  		defconf_ = camera_->generateConfiguration({ StreamRole::VideoRecording });
> > +		if (!defconf_) {
> > +			cout << "Failed to generate default configuration" << endl;
> > +			CameraTest::cleanup();
> > +			return TestFail;
> > +		}
> >  
> > +		return TestPass;
> > +	}
> > +
> > +	int run() override
> > +	{
> >  		if (testAvailable() != TestPass) {
> >  			cout << "State machine in Available state failed" << endl;
> >  			return TestFail;
> > @@ -265,7 +277,13 @@ protected:
> >  		return TestPass;
> >  	}
> >  
> > -	CameraConfiguration defconf_;
> > +	void cleanup() override
> > +	{
> > +		delete defconf_;
> > +		CameraTest::cleanup();
> > +	}
> > +
> > +	CameraConfiguration *defconf_;
> >  };
> >  
> >  } /* namespace */