[libcamera-devel,v2,4/6] libcamera: camera: Return a pointer from generateConfiguration()

1235 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                         | 40 ++++++++++-----------
 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, 202 insertions(+), 123 deletions(-)

Hi Laurent,

Thanks for your work.

On 2019-05-19 18:00:45 +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.
> 
> 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                         | 40 ++++++++++-----------
>  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, 202 insertions(+), 123 deletions(-)
> 
> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> index 284e5276a055..144133c5de9f 100644
> --- a/include/libcamera/camera.h
> +++ b/include/libcamera/camera.h
> @@ -77,8 +77,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..7550ae4f3428 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,23 +111,23 @@ 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. */
>  	unsigned int i = 0;
>  	for (auto const &value : streamOptions) {
>  		KeyValueParser::Options conf = value.toKeyValues();
> -		StreamConfiguration &cfg = (*config)[i++];
> +		StreamConfiguration &cfg = config->at(i++);
>  
>  		if (conf.isSet("width"))
>  			cfg.size.width = conf["width"];
> @@ -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->at(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 5848330f639a..0e80691ed862 100644
> --- a/src/libcamera/camera.cpp
> +++ b/src/libcamera/camera.cpp
> @@ -542,21 +542,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->empty())
> +		msg << " empty";
> +
> +	for (unsigned int index = 0; index < config->size(); ++index)
> +		msg << " (" << index << ") " << config->at(index).toString();
>  
>  	LOG(Camera, Debug) << msg.str();
>  
> @@ -590,7 +593,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;
>  
> @@ -600,7 +603,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;
> @@ -608,8 +611,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->at(index);
>  		cfg.setStream(nullptr);
>  		msg << " (" << index << ") " << cfg.toString();
>  	}
> @@ -621,7 +624,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..3acf82ff4665 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->at(0));
>  		if (ret)
>  			return ret;
>  	}
>  
>  	if (!vfStream->active_) {
> -		ret = imgu->configureOutput(vfStream->device_, config[0]);
> +		ret = imgu->configureOutput(vfStream->device_, config->at(0));
>  		if (ret)
>  			return ret;
>  	}
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index ec6980b0943a..01ba50f09db1 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()) {

I would invert this check and reduce the indentation level,

    if (roles.empty())
        return config;

Same for other generateConfiguration() bellow.

With this fixed,

Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>

> +		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->at(0);
>  	CameraSensor *sensor = data->sensor_;
>  	int ret;
>  
> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp
> index 5dcc868b2fc9..91b4065c250b 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->at(0);
>  	int ret;
>  
>  	V4L2DeviceFormat format = {};
> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp
> index af6b6f21e3c5..510e6c082f13 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->at(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..3ae5aae9cc76 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_->at(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..137aa649a505 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_->at(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..23c611a93355 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_->at(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 Laurent,

On Sun, May 19, 2019 at 06:00:45PM +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.

Have you considered enforcing this returning CameraConfiguration as a
unique_ptr<> to applications ? Ownership is passed to them at
generateConfiguration() time, and then applications would pass a
reference accessing the unique_ptr<> at configure() time.

After all, there is no other way from an application to get back the
same CameraConfiguration from the Camera, so once it has been
returned, it's responsability of the application to keep a valid
reference to it. Also, to make it easier for app to deal with that,
we could abstract it with a d-pointer later (if appropriate)
>
> 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.
>

With unique_ptr this would be Camera::configure(config.get())

> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  include/libcamera/camera.h               |  4 +--
>  src/cam/main.cpp                         | 40 ++++++++++-----------
>  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, 202 insertions(+), 123 deletions(-)
>
> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> index 284e5276a055..144133c5de9f 100644
> --- a/include/libcamera/camera.h
> +++ b/include/libcamera/camera.h
> @@ -77,8 +77,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..7550ae4f3428 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,23 +111,23 @@ 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. */
>  	unsigned int i = 0;
>  	for (auto const &value : streamOptions) {
>  		KeyValueParser::Options conf = value.toKeyValues();
> -		StreamConfiguration &cfg = (*config)[i++];
> +		StreamConfiguration &cfg = config->at(i++);

I curious to know why at() here? Just for style reasons ?

>
>  		if (conf.isSet("width"))
>  			cfg.size.width = conf["width"];
> @@ -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->at(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;

using unique_ptr<> would avoid application having to do this. It comes
at a cost of a more verbose API indeed.

>  		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 5848330f639a..0e80691ed862 100644
> --- a/src/libcamera/camera.cpp
> +++ b/src/libcamera/camera.cpp
> @@ -542,21 +542,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())

I might have missed it, but I would document that we accept empty
StreamRoles to generate an empty CameraConfiguration to be filled by
applications.

I'm not sure but if that's the case, we could check here and return an
here instantiated empty CameraConfiguration without even going to the
pipeline handlers.

> +		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->empty())
> +		msg << " empty";
> +
> +	for (unsigned int index = 0; index < config->size(); ++index)
> +		msg << " (" << index << ") " << config->at(index).toString();
>
>  	LOG(Camera, Debug) << msg.str();
>
> @@ -590,7 +593,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;
>
> @@ -600,7 +603,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;
> @@ -608,8 +611,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->at(index);
>  		cfg.setStream(nullptr);
>  		msg << " (" << index << ") " << cfg.toString();
>  	}
> @@ -621,7 +624,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..3acf82ff4665 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)

nit: I would keep this aligned with the first parameter

>  {
>  	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->at(0));
>  		if (ret)
>  			return ret;
>  	}
>
>  	if (!vfStream->active_) {
> -		ret = imgu->configureOutput(vfStream->device_, config[0]);
> +		ret = imgu->configureOutput(vfStream->device_, config->at(0));
>  		if (ret)
>  			return ret;
>  	}
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index ec6980b0943a..01ba50f09db1 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{};

Same comment as Niklas here, with the additional point of moving this
check to Camera::generateConfiguration()

Thanks
   j
>
> -	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->at(0);
>  	CameraSensor *sensor = data->sensor_;
>  	int ret;
>
> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp
> index 5dcc868b2fc9..91b4065c250b 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->at(0);
>  	int ret;
>
>  	V4L2DeviceFormat format = {};
> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp
> index af6b6f21e3c5..510e6c082f13 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->at(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..3ae5aae9cc76 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_->at(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..137aa649a505 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_->at(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..23c611a93355 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_->at(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 Jacopo,

On Tue, May 21, 2019 at 11:52:44AM +0200, Jacopo Mondi wrote:
> On Sun, May 19, 2019 at 06:00:45PM +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.
> 
> Have you considered enforcing this returning CameraConfiguration as a
> unique_ptr<> to applications ? Ownership is passed to them at
> generateConfiguration() time, and then applications would pass a
> reference accessing the unique_ptr<> at configure() time.

It's a good idea, I hadn't thought about it. The code looks cleaner with
that change (9 files changed, 27 insertions(+), 55 deletions(-)).

> After all, there is no other way from an application to get back the
> same CameraConfiguration from the Camera, so once it has been
> returned, it's responsability of the application to keep a valid
> reference to it. Also, to make it easier for app to deal with that,
> we could abstract it with a d-pointer later (if appropriate)
>
> > 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.
> 
> With unique_ptr this would be Camera::configure(config.get())
> 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  include/libcamera/camera.h               |  4 +--
> >  src/cam/main.cpp                         | 40 ++++++++++-----------
> >  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, 202 insertions(+), 123 deletions(-)
> >
> > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> > index 284e5276a055..144133c5de9f 100644
> > --- a/include/libcamera/camera.h
> > +++ b/include/libcamera/camera.h
> > @@ -77,8 +77,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..7550ae4f3428 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,23 +111,23 @@ 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. */
> >  	unsigned int i = 0;
> >  	for (auto const &value : streamOptions) {
> >  		KeyValueParser::Options conf = value.toKeyValues();
> > -		StreamConfiguration &cfg = (*config)[i++];
> > +		StreamConfiguration &cfg = config->at(i++);
> 
> I curious to know why at() here? Just for style reasons ?

Yes, just for style reason. If you convince Niklas that (*config)[] is
fine, I'll switch back to it and drop the at() method.

> >
> >  		if (conf.isSet("width"))
> >  			cfg.size.width = conf["width"];
> > @@ -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->at(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;
> 
> using unique_ptr<> would avoid application having to do this. It comes
> at a cost of a more verbose API indeed.
> 
> >  		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 5848330f639a..0e80691ed862 100644
> > --- a/src/libcamera/camera.cpp
> > +++ b/src/libcamera/camera.cpp
> > @@ -542,21 +542,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())
> 
> I might have missed it, but I would document that we accept empty
> StreamRoles to generate an empty CameraConfiguration to be filled by
> applications.

I'll fix that.

> I'm not sure but if that's the case, we could check here and return an
> here instantiated empty CameraConfiguration without even going to the
> pipeline handlers.

We can't, as that empty configuration should get filled by applications,
and then validated. We need a specialised instance of
CameraConfiguration for that.

> > +		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->empty())
> > +		msg << " empty";
> > +
> > +	for (unsigned int index = 0; index < config->size(); ++index)
> > +		msg << " (" << index << ") " << config->at(index).toString();
> >
> >  	LOG(Camera, Debug) << msg.str();
> >
> > @@ -590,7 +593,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;
> >
> > @@ -600,7 +603,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;
> > @@ -608,8 +611,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->at(index);
> >  		cfg.setStream(nullptr);
> >  		msg << " (" << index << ") " << cfg.toString();
> >  	}
> > @@ -621,7 +624,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..3acf82ff4665 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)
> 
> nit: I would keep this aligned with the first parameter

I'm also in two minds about that, when the method name is long, all
parameters are pushed back far to the right. I don't think the style
used here is perfect, but it has the advantage of being consistently
usable regardless of how long the method name is.

> >  {
> >  	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->at(0));
> >  		if (ret)
> >  			return ret;
> >  	}
> >
> >  	if (!vfStream->active_) {
> > -		ret = imgu->configureOutput(vfStream->device_, config[0]);
> > +		ret = imgu->configureOutput(vfStream->device_, config->at(0));
> >  		if (ret)
> >  			return ret;
> >  	}
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > index ec6980b0943a..01ba50f09db1 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{};
> 
> Same comment as Niklas here, with the additional point of moving this
> check to Camera::generateConfiguration()
> 
> > -	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->at(0);
> >  	CameraSensor *sensor = data->sensor_;
> >  	int ret;
> >
> > diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp
> > index 5dcc868b2fc9..91b4065c250b 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->at(0);
> >  	int ret;
> >
> >  	V4L2DeviceFormat format = {};
> > diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp
> > index af6b6f21e3c5..510e6c082f13 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->at(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..3ae5aae9cc76 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_->at(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..137aa649a505 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_->at(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..23c611a93355 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_->at(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 */