[libcamera-devel,v2,3/6] libcamera: Refactor the camera configuration storage and API

1234 diff mbox series

Refactor the CameraConfiguration structure to not rely on Stream
instances. This is a step towards making the camera configuration object
more powerful with configuration validation using "try" semantics.

The CameraConfiguration now exposes a simple vector-like API to access
the contained stream configurations. Both operator[]() and at() are
provided to access elements. The isEmpty() method is renamed to empty()
and the methods reordered to match the std::vector class.

As applications need access to the Stream instances associated with the
configuration entries in order to associate buffers with streams when
creating requests, expose the stream selected by the pipeline handler
through a new StreamConfiguration::stream().

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
---
 include/libcamera/camera.h               |  36 +--
 include/libcamera/stream.h               |   7 +
 src/cam/main.cpp                         |  35 +--
 src/libcamera/camera.cpp                 | 268 +++++++++++------------
 src/libcamera/include/pipeline_handler.h |   2 +-
 src/libcamera/pipeline/ipu3/ipu3.cpp     |  32 +--
 src/libcamera/pipeline/rkisp1/rkisp1.cpp |  12 +-
 src/libcamera/pipeline/uvcvideo.cpp      |  23 +-
 src/libcamera/pipeline/vimc.cpp          |  23 +-
 src/libcamera/pipeline_handler.cpp       |   4 +
 src/libcamera/stream.cpp                 |  22 ++
 src/qcam/main_window.cpp                 |   4 +-
 test/camera/capture.cpp                  |   4 +-
 test/camera/configuration_set.cpp        |   2 +-
 14 files changed, 251 insertions(+), 223 deletions(-)

Hi Laurent,

On Sun, May 19, 2019 at 06:00:44PM +0300, Laurent Pinchart wrote:
> Refactor the CameraConfiguration structure to not rely on Stream
> instances. This is a step towards making the camera configuration object
> more powerful with configuration validation using "try" semantics.
>
> The CameraConfiguration now exposes a simple vector-like API to access
> the contained stream configurations. Both operator[]() and at() are
> provided to access elements. The isEmpty() method is renamed to empty()
> and the methods reordered to match the std::vector class.
>
> As applications need access to the Stream instances associated with the
> configuration entries in order to associate buffers with streams when
> creating requests, expose the stream selected by the pipeline handler
> through a new StreamConfiguration::stream().
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> ---
>  include/libcamera/camera.h               |  36 +--
>  include/libcamera/stream.h               |   7 +
>  src/cam/main.cpp                         |  35 +--
>  src/libcamera/camera.cpp                 | 268 +++++++++++------------
>  src/libcamera/include/pipeline_handler.h |   2 +-
>  src/libcamera/pipeline/ipu3/ipu3.cpp     |  32 +--
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp |  12 +-
>  src/libcamera/pipeline/uvcvideo.cpp      |  23 +-
>  src/libcamera/pipeline/vimc.cpp          |  23 +-
>  src/libcamera/pipeline_handler.cpp       |   4 +
>  src/libcamera/stream.cpp                 |  22 ++
>  src/qcam/main_window.cpp                 |   4 +-
>  test/camera/capture.cpp                  |   4 +-
>  test/camera/configuration_set.cpp        |   2 +-
>  14 files changed, 251 insertions(+), 223 deletions(-)
>
> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> index 42ba5201eabc..284e5276a055 100644
> --- a/include/libcamera/camera.h
> +++ b/include/libcamera/camera.h
> @@ -25,30 +25,36 @@ class Request;
>  class CameraConfiguration
>  {
>  public:
> -	using iterator = std::vector<Stream *>::iterator;
> -	using const_iterator = std::vector<Stream *>::const_iterator;
> +	using iterator = std::vector<StreamConfiguration>::iterator;
> +	using const_iterator = std::vector<StreamConfiguration>::const_iterator;
>
>  	CameraConfiguration();
>
> +	void addConfiguration(const StreamConfiguration &cfg);

I would use push_back() to match the std::vector API

> +
> +	bool isValid() const;

As you s/isEmpty()/empty()/ should we s/isValid()/valid()/ ?

Ah, don't worry, this will become validate(), just noticed

> +
> +	StreamConfiguration &at(unsigned int index);
> +	const StreamConfiguration &at(unsigned int index) const;
> +	StreamConfiguration &operator[](unsigned int index)
> +	{
> +		return at(index);
> +	}
> +	const StreamConfiguration &operator[](unsigned int index) const
> +	{
> +		return at(index);
> +	}

Ok, so we have [] and at() like std::vector, but it seems to me their
behaviour is inverted.

std::vector::at() performs bound checking, and
CameraConfiguration::at() is implemented with std::vector::operator[],
which does not perform bounds checking

std::vector::operator[] does not perform bounds checking, but
CameraConfiguration::operator[] is implemented with std::vector::at()
which performs bound checking.

Is this intentional ?
I would also question the need to have both operator[] and at()
accessors, I know std::vector does, but do we need that or is it just
an API expansion we could save?

> +
>  	iterator begin();
> -	iterator end();
>  	const_iterator begin() const;
> +	iterator end();
>  	const_iterator end() const;
>
> -	bool isValid() const;
> -	bool isEmpty() const;
> +	bool empty() const;
>  	std::size_t size() const;
>
> -	Stream *front();
> -	const Stream *front() const;
> -
> -	Stream *operator[](unsigned int index) const;
> -	StreamConfiguration &operator[](Stream *stream);
> -	const StreamConfiguration &operator[](Stream *stream) const;
> -
>  private:
> -	std::vector<Stream *> order_;
> -	std::map<Stream *, StreamConfiguration> config_;
> +	std::vector<StreamConfiguration> config_;
>  };
>
>  class Camera final
> @@ -72,7 +78,7 @@ public:
>
>  	const std::set<Stream *> &streams() const;
>  	CameraConfiguration generateConfiguration(const StreamRoles &roles);
> -	int configure(const CameraConfiguration &config);
> +	int configure(CameraConfiguration &config);
>
>  	int allocateBuffers();
>  	int freeBuffers();
> diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h
> index 59bdf217eb31..47c007ed52e2 100644
> --- a/include/libcamera/stream.h
> +++ b/include/libcamera/stream.h
> @@ -16,6 +16,7 @@
>  namespace libcamera {
>
>  class Camera;
> +class Stream;
>
>  struct StreamConfiguration {
>  	unsigned int pixelFormat;
> @@ -23,7 +24,13 @@ struct StreamConfiguration {
>
>  	unsigned int bufferCount;
>
> +	Stream *stream() const { return stream_; }
> +	void setStream(Stream *stream) { stream_ = stream; }
> +
>  	std::string toString() const;
> +
> +private:
> +	Stream *stream_;

Should we protect access to unitialized streams_ ?
If StreamConfiguration is not inialized to {} what's the value of
*stream_ ?

>  };
>
>  enum StreamRole {
> diff --git a/src/cam/main.cpp b/src/cam/main.cpp
> index d603228c0116..cd165bea34cd 100644
> --- a/src/cam/main.cpp
> +++ b/src/cam/main.cpp
> @@ -89,12 +89,9 @@ static int prepareCameraConfig(CameraConfiguration *config)
>  {
>  	StreamRoles roles;
>
> -	streamInfo.clear();
> -
>  	/* If no configuration is provided assume a single video stream. */
>  	if (!options.isSet(OptStream)) {
>  		*config = camera->generateConfiguration({ StreamRole::VideoRecording });
> -		streamInfo[config->front()] = "stream0";
>  		return 0;
>  	}
>
> @@ -129,27 +126,20 @@ static int prepareCameraConfig(CameraConfiguration *config)
>  	}
>
>  	/* Apply configuration explicitly requested. */
> -	CameraConfiguration::iterator it = config->begin();
> +	unsigned int i = 0;
>  	for (auto const &value : streamOptions) {
>  		KeyValueParser::Options conf = value.toKeyValues();

nit and not related to this patch: s/conf/opt ? otherwise we have
*config and cfg in this loop, which is quite confusing.

> -		Stream *stream = *it;
> -		it++;
> +		StreamConfiguration &cfg = (*config)[i++];
>
>  		if (conf.isSet("width"))
> -			(*config)[stream].size.width = conf["width"];
> +			cfg.size.width = conf["width"];
>
>  		if (conf.isSet("height"))
> -			(*config)[stream].size.height = conf["height"];
> +			cfg.size.height = conf["height"];
>
>  		/* TODO: Translate 4CC string to ID. */
>  		if (conf.isSet("pixelformat"))
> -			(*config)[stream].pixelFormat = conf["pixelformat"];
> -	}
> -
> -	unsigned int index = 0;
> -	for (Stream *stream : *config) {
> -		streamInfo[stream] = "stream" + std::to_string(index);
> -		index++;
> +			cfg.pixelFormat = conf["pixelformat"];
>  	}
>
>  	return 0;
> @@ -216,6 +206,13 @@ static int capture()
>  		return ret;
>  	}
>
> +	streamInfo.clear();
> +
> +	for (unsigned int index = 0; index < config.size(); ++index) {
> +		StreamConfiguration &cfg = config[index];
> +		streamInfo[cfg.stream()] = "stream" + std::to_string(index);
> +	}
> +
>  	ret = camera->allocateBuffers();
>  	if (ret) {
>  		std::cerr << "Failed to allocate buffers"
> @@ -227,8 +224,10 @@ static int capture()
>
>  	/* Identify the stream with the least number of buffers. */
>  	unsigned int nbuffers = UINT_MAX;
> -	for (Stream *stream : config)
> +	for (StreamConfiguration &cfg : config) {
> +		Stream *stream = cfg.stream();
>  		nbuffers = std::min(nbuffers, stream->bufferPool().count());
> +	}
>
>  	/*
>  	 * TODO: make cam tool smarter to support still capture by for
> @@ -245,8 +244,10 @@ static int capture()
>  		}
>
>  		std::map<Stream *, Buffer *> map;
> -		for (Stream *stream : config)
> +		for (StreamConfiguration &cfg : config) {
> +			Stream *stream = cfg.stream();
>  			map[stream] = &stream->bufferPool().buffers()[i];
> +		}
>
>  		ret = request->setBuffers(map);
>  		if (ret < 0) {
> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> index a3921f91f1c9..5848330f639a 100644
> --- a/src/libcamera/camera.cpp
> +++ b/src/libcamera/camera.cpp
> @@ -46,72 +46,40 @@ LOG_DECLARE_CATEGORY(Camera)
>   * \class CameraConfiguration
>   * \brief Hold configuration for streams of the camera
>
> - * The CameraConfiguration holds an ordered list of streams and their associated
> - * StreamConfiguration. From a data storage point of view, the class operates as
> - * a map of Stream pointers to StreamConfiguration, with entries accessed with
> - * operator[](Stream *). Accessing an entry for a Stream pointer not yet stored
> - * in the configuration inserts a new empty entry.
> - *
> - * The class also suppors iterators, and from that point of view operates as a
> - * vector of Stream pointers. The streams are iterated in insertion order, and
> - * the operator[](int) returns the Stream pointer based on its insertion index.
> - * Accessing a stream with an invalid index returns a null pointer.
> + * The CameraConfiguration holds an ordered list of stream configurations. It
> + * supports iterators and operates as a vector of StreamConfiguration instances.
> + * The stream configurations are inserted by addConfiguration(), and the
> + * operator[](int) returns a reference to the StreamConfiguration based on its
> + * insertion index. Accessing a stream configuration with an invalid index
> + * results in undefined behaviour.

As operator[] is implemented with std::vector::at() accessing with an
invalid index, an exception is thrown (even if we don't use them in
the library).

I would document CameraConfiguration::at() as well, or just provide one of
the two only.

>   */
>
>  /**
>   * \typedef CameraConfiguration::iterator
> - * \brief Iterator for the streams in the configuration
> + * \brief Iterator for the stream configurations in the camera configuration
>   */
>
>  /**
>   * \typedef CameraConfiguration::const_iterator
> - * \brief Const iterator for the streams in the configuration
> + * \brief Const iterator for the stream configuration in the camera
> + * configuration
>   */
>
>  /**
>   * \brief Create an empty camera configuration
>   */
>  CameraConfiguration::CameraConfiguration()
> -	: order_({}), config_({})
> +	: config_({})
>  {
>  }
>
>  /**
> - * \brief Retrieve an iterator to the first stream in the sequence
> - * \return An iterator to the first stream
> + * \brief Add a stream configuration to the camera configuration
> + * \param[in] cfg The stream configuration
>   */
> -std::vector<Stream *>::iterator CameraConfiguration::begin()
> +void CameraConfiguration::addConfiguration(const StreamConfiguration &cfg)
>  {
> -	return order_.begin();
> -}
> -
> -/**
> - * \brief Retrieve an iterator pointing to the past-the-end stream in the
> - * sequence
> - * \return An iterator to the element following the last stream
> - */
> -std::vector<Stream *>::iterator CameraConfiguration::end()
> -{
> -	return order_.end();
> -}
> -
> -/**
> - * \brief Retrieve a const iterator to the first element of the streams
> - * \return A const iterator to the first stream
> - */
> -std::vector<Stream *>::const_iterator CameraConfiguration::begin() const
> -{
> -	return order_.begin();
> -}
> -
> -/**
> - * \brief Retrieve a const iterator pointing to the past-the-end stream in the
> - * sequence
> - * \return A const iterator to the element following the last stream
> - */
> -std::vector<Stream *>::const_iterator CameraConfiguration::end() const
> -{
> -	return order_.end();
> +	config_.push_back(cfg);
>  }
>
>  /**
> @@ -125,12 +93,10 @@ std::vector<Stream *>::const_iterator CameraConfiguration::end() const
>   */
>  bool CameraConfiguration::isValid() const
>  {
> -	if (isEmpty())
> +	if (empty())
>  		return false;
>
> -	for (auto const &it : config_) {
> -		const StreamConfiguration &cfg = it.second;
> -
> +	for (const StreamConfiguration &cfg : config_) {
>  		if (cfg.size.width == 0 || cfg.size.height == 0 ||
>  		    cfg.pixelFormat == 0 || cfg.bufferCount == 0)
>  			return false;
> @@ -139,13 +105,108 @@ bool CameraConfiguration::isValid() const
>  	return true;
>  }
>
> +/**
> + * \brief Retrieve a reference to a stream configuration
> + * \param[in] index Numerical index

STL uses pos in place of index. Not sure we care, though.

> + *
> + * The \a index represents the zero based insertion order of stream
> + * configuration into the camera configuration with addConfiguration(). Calling
> + * this method with an invalid index results in undefined behaviour.
> + *
> + * \return The stream configuration

> + */
> +StreamConfiguration &CameraConfiguration::at(unsigned int index)
> +{
> +	return config_[index];
> +}
> +
> +/**
> + * \brief Retrieve a const reference to a stream configuration
> + * \param[in] index Numerical index
> + *
> + * The \a index represents the zero based insertion order of stream
> + * configuration into the camera configuration with addConfiguration(). Calling
> + * this method with an invalid index results in undefined behaviour.
> + *
> + * \return The stream configuration
> + */
> +const StreamConfiguration &CameraConfiguration::at(unsigned int index) const
> +{
> +	return config_[index];
> +}
> +
> +/**
> + * \fn StreamConfiguration &CameraConfiguration::operator[](unsigned int)
> + * \brief Retrieve a reference to a stream configuration
> + * \param[in] index Numerical index
> + *
> + * The \a index represents the zero based insertion order of stream
> + * configuration into the camera configuration with addConfiguration(). Calling
> + * this method with an invalid index results in undefined behaviour.
> + *
> + * \return The stream configuration
> + */
> +
> +/**
> + * \fn const StreamConfiguration &CameraConfiguration::operator[](unsigned int) const
> + * \brief Retrieve a const reference to a stream configuration
> + * \param[in] index Numerical index
> + *
> + * The \a index represents the zero based insertion order of stream
> + * configuration into the camera configuration with addConfiguration(). Calling
> + * this method with an invalid index results in undefined behaviour.
> + *
> + * \return The stream configuration
> + */
> +
> +/**
> + * \brief Retrieve an iterator to the first stream configuration in the
> + * sequence
> + * \return An iterator to the first stream configuration
> + */
> +CameraConfiguration::iterator CameraConfiguration::begin()
> +{
> +	return config_.begin();
> +}
> +
> +/**
> + * \brief Retrieve a const iterator to the first element of the stream
> + * configurations
> + * \return A const iterator to the first stream configuration
> + */
> +CameraConfiguration::const_iterator CameraConfiguration::begin() const
> +{
> +	return config_.begin();
> +}
> +
> +/**
> + * \brief Retrieve an iterator pointing to the past-the-end stream
> + * configuration in the sequence
> + * \return An iterator to the element following the last stream configuration
> + */
> +CameraConfiguration::iterator CameraConfiguration::end()

Aren't these StreamConfiguration::iterators ?

> +{
> +	return config_.end();
> +}
> +
> +/**
> + * \brief Retrieve a const iterator pointing to the past-the-end stream
> + * configuration in the sequence
> + * \return A const iterator to the element following the last stream
> + * configuration
> + */
> +CameraConfiguration::const_iterator CameraConfiguration::end() const
> +{
> +	return config_.end();
> +}
> +
>  /**
>   * \brief Check if the camera configuration is empty
>   * \return True if the configuration is empty
>   */
> -bool CameraConfiguration::isEmpty() const
> +bool CameraConfiguration::empty() const
>  {
> -	return order_.empty();
> +	return config_.empty();
>  }
>
>  /**
> @@ -154,75 +215,7 @@ bool CameraConfiguration::isEmpty() const
>   */
>  std::size_t CameraConfiguration::size() const
>  {
> -	return order_.size();
> -}
> -
> -/**
> - * \brief Access the first stream in the configuration
> - * \return The first stream in the configuration
> - */
> -Stream *CameraConfiguration::front()
> -{
> -	return order_.front();
> -}
> -
> -/**
> - * \brief Access the first stream in the configuration
> - * \return The first const stream pointer in the configuration
> - */
> -const Stream *CameraConfiguration::front() const
> -{
> -	return order_.front();
> -}
> -
> -/**
> - * \brief Retrieve a stream pointer from index
> - * \param[in] index Numerical index
> - *
> - * The \a index represents the zero based insertion order of stream and stream
> - * configuration into the camera configuration.
> - *
> - * \return The stream pointer at index, or a nullptr if the index is out of
> - * bounds
> - */
> -Stream *CameraConfiguration::operator[](unsigned int index) const
> -{
> -	if (index >= order_.size())
> -		return nullptr;
> -
> -	return order_.at(index);
> -}
> -
> -/**
> - * \brief Retrieve a reference to a stream configuration
> - * \param[in] stream Stream to retrieve configuration for
> - *
> - * If the camera configuration does not yet contain a configuration for
> - * the requested stream, create and return an empty stream configuration.
> - *
> - * \return The configuration for the stream
> - */
> -StreamConfiguration &CameraConfiguration::operator[](Stream *stream)
> -{
> -	if (config_.find(stream) == config_.end())
> -		order_.push_back(stream);
> -
> -	return config_[stream];
> -}
> -
> -/**
> - * \brief Retrieve a const reference to a stream configuration
> - * \param[in] stream Stream to retrieve configuration for
> - *
> - * No new stream configuration is created if called with \a stream that is not
> - * already part of the camera configuration, doing so is an invalid operation
> - * and results in undefined behaviour.
> - *
> - * \return The configuration for the stream
> - */
> -const StreamConfiguration &CameraConfiguration::operator[](Stream *stream) const
> -{
> -	return config_.at(stream);
> +	return config_.size();
>  }
>
>  /**
> @@ -561,13 +554,9 @@ Camera::generateConfiguration(const StreamRoles &roles)
>  	CameraConfiguration config = pipe_->generateConfiguration(this, roles);
>
>  	std::ostringstream msg("streams configuration:", std::ios_base::ate);
> -	unsigned int index = 0;
>
> -	for (Stream *stream : config) {
> -		const StreamConfiguration &cfg = config[stream];
> -		msg << " (" << index << ") " << cfg.toString();
> -		index++;
> -	}
> +	for (unsigned int index = 0; index < config.size(); ++index)
> +		msg << " (" << index << ") " << config[index].toString();
>
>  	LOG(Camera, Debug) << msg.str();
>
> @@ -593,12 +582,15 @@ Camera::generateConfiguration(const StreamRoles &roles)
>   *
>   * This function affects the state of the camera, see \ref camera_operation.
>   *
> + * Upon return the StreamConfiguration entries in \a config are associated with
> + * Stream instances which can be retrieved with StreamConfiguration::stream().
> + *
>   * \return 0 on success or a negative error code otherwise
>   * \retval -ENODEV The camera has been disconnected from the system
>   * \retval -EACCES The camera is not in a state where it can be configured
>   * \retval -EINVAL The configuration is not valid
>   */
> -int Camera::configure(const CameraConfiguration &config)
> +int Camera::configure(CameraConfiguration &config)
>  {
>  	int ret;
>
> @@ -615,16 +607,11 @@ int Camera::configure(const CameraConfiguration &config)
>  	}
>
>  	std::ostringstream msg("configuring streams:", std::ios_base::ate);
> -	unsigned int index = 0;
>
> -	for (Stream *stream : config) {
> -		if (streams_.find(stream) == streams_.end())
> -			return -EINVAL;
> -
> -		const StreamConfiguration &cfg = config[stream];
> -		msg << std::dec << " (" << index << ") " << cfg.toString();
> -
> -		index++;
> +	for (unsigned int index = 0; index < config.size(); ++index) {
> +		StreamConfiguration &cfg = config[index];
> +		cfg.setStream(nullptr);
> +		msg << " (" << index << ") " << cfg.toString();

Isn't this better printed after pipe->configure(), once we know all
streams configuration have a stream assigned ?

Overall this is very good. The fact StreamConfiguration instances are
associated to Stream just after configure() might require clearly
preventing applications to try access it, but I think the
documentation is quite clear on that. The biggest part is discussing
the CameraConfiguration API I guess, which we might want to make more
similar to std::vector.

Thanks
   j

>  	}
>
>  	LOG(Camera, Info) << msg.str();
> @@ -634,8 +621,11 @@ int Camera::configure(const CameraConfiguration &config)
>  		return ret;
>
>  	activeStreams_.clear();
> -	for (Stream *stream : config) {
> -		const StreamConfiguration &cfg = config[stream];
> +	for (const StreamConfiguration &cfg : config) {
> +		Stream *stream = cfg.stream();
> +		if (!stream)
> +			LOG(Camera, Fatal)
> +				<< "Pipeline handler failed to update stream configuration";
>
>  		stream->configuration_ = cfg;
>  		activeStreams_.insert(stream);
> diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h
> index 3352cb0e5bc9..a025905ab68f 100644
> --- a/src/libcamera/include/pipeline_handler.h
> +++ b/src/libcamera/include/pipeline_handler.h
> @@ -62,7 +62,7 @@ public:
>
>  	virtual CameraConfiguration
>  	generateConfiguration(Camera *camera, const StreamRoles &roles) = 0;
> -	virtual int configure(Camera *camera, const CameraConfiguration &config) = 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 d234a8ac5289..ed0ef69de1d1 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -152,8 +152,7 @@ public:
>
>  	CameraConfiguration
>  	generateConfiguration(Camera *camera, const StreamRoles &roles) override;
> -	int configure(Camera *camera,
> -		      const CameraConfiguration &config) override;
> +	int configure(Camera *camera, CameraConfiguration &config) override;
>
>  	int allocateBuffers(Camera *camera,
>  			    const std::set<Stream *> &streams) override;
> @@ -299,14 +298,13 @@ PipelineHandlerIPU3::generateConfiguration(Camera *camera,
>  		cfg.pixelFormat = V4L2_PIX_FMT_NV12;
>  		cfg.bufferCount = IPU3_BUFFER_COUNT;
>
> -		config[stream] = cfg;
> +		config.addConfiguration(cfg);
>  	}
>
>  	return config;
>  }
>
> -int PipelineHandlerIPU3::configure(Camera *camera,
> -				   const CameraConfiguration &config)
> +int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration &config)
>  {
>  	IPU3CameraData *data = cameraData(camera);
>  	IPU3Stream *outStream = &data->outStream_;
> @@ -318,9 +316,13 @@ int PipelineHandlerIPU3::configure(Camera *camera,
>
>  	outStream->active_ = false;
>  	vfStream->active_ = false;
> -	for (Stream *s : config) {
> -		IPU3Stream *stream = static_cast<IPU3Stream *>(s);
> -		const StreamConfiguration &cfg = config[stream];
> +	for (StreamConfiguration &cfg : config) {
> +		/*
> +		 * Pick a stream for the configuration entry.
> +		 * \todo: This is a naive temporary implementation that will be
> +		 * reworked when implementing camera configuration validation.
> +		 */
> +		IPU3Stream *stream = vfStream->active_ ? outStream : vfStream;
>
>  		/*
>  		 * Verify that the requested size respects the IPU3 alignment
> @@ -355,6 +357,7 @@ int PipelineHandlerIPU3::configure(Camera *camera,
>  			sensorSize.height = cfg.size.height;
>
>  		stream->active_ = true;
> +		cfg.setStream(stream);
>  	}
>
>  	/*
> @@ -379,10 +382,9 @@ int PipelineHandlerIPU3::configure(Camera *camera,
>  		return ret;
>
>  	/* Apply the format to the configured streams output devices. */
> -	for (Stream *s : config) {
> -		IPU3Stream *stream = static_cast<IPU3Stream *>(s);
> -
> -		ret = imgu->configureOutput(stream->device_, config[stream]);
> +	for (StreamConfiguration &cfg : config) {
> +		IPU3Stream *stream = static_cast<IPU3Stream *>(cfg.stream());
> +		ret = imgu->configureOutput(stream->device_, cfg);
>  		if (ret)
>  			return ret;
>  	}
> @@ -393,15 +395,13 @@ int PipelineHandlerIPU3::configure(Camera *camera,
>  	 * be at least one active stream in the configuration request).
>  	 */
>  	if (!outStream->active_) {
> -		ret = imgu->configureOutput(outStream->device_,
> -					    config[vfStream]);
> +		ret = imgu->configureOutput(outStream->device_, config[0]);
>  		if (ret)
>  			return ret;
>  	}
>
>  	if (!vfStream->active_) {
> -		ret = imgu->configureOutput(vfStream->device_,
> -					    config[outStream]);
> +		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 4bd8c5101a96..ec6980b0943a 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -36,8 +36,7 @@ public:
>
>  	CameraConfiguration generateConfiguration(Camera *camera,
>  		const StreamRoles &roles) override;
> -	int configure(Camera *camera,
> -		const CameraConfiguration &config) override;
> +	int configure(Camera *camera, CameraConfiguration &config) override;
>
>  	int allocateBuffers(Camera *camera,
>  		const std::set<Stream *> &streams) override;
> @@ -117,16 +116,15 @@ CameraConfiguration PipelineHandlerRkISP1::generateConfiguration(Camera *camera,
>  	cfg.size = data->sensor_->resolution();
>  	cfg.bufferCount = RKISP1_BUFFER_COUNT;
>
> -	config[&data->stream_] = cfg;
> +	config.addConfiguration(cfg);
>
>  	return config;
>  }
>
> -int PipelineHandlerRkISP1::configure(Camera *camera,
> -				     const CameraConfiguration &config)
> +int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration &config)
>  {
>  	RkISP1CameraData *data = cameraData(camera);
> -	const StreamConfiguration &cfg = config[&data->stream_];
> +	StreamConfiguration &cfg = config[0];
>  	CameraSensor *sensor = data->sensor_;
>  	int ret;
>
> @@ -217,6 +215,8 @@ int PipelineHandlerRkISP1::configure(Camera *camera,
>  		return -EINVAL;
>  	}
>
> +	cfg.setStream(&data->stream_);
> +
>  	return 0;
>  }
>
> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp
> index d2e1f7d4e5b2..5dcc868b2fc9 100644
> --- a/src/libcamera/pipeline/uvcvideo.cpp
> +++ b/src/libcamera/pipeline/uvcvideo.cpp
> @@ -27,8 +27,7 @@ public:
>
>  	CameraConfiguration
>  	generateConfiguration(Camera *camera, const StreamRoles &roles) override;
> -	int configure(Camera *camera,
> -		      const CameraConfiguration &config) override;
> +	int configure(Camera *camera, CameraConfiguration &config) override;
>
>  	int allocateBuffers(Camera *camera,
>  			    const std::set<Stream *> &streams) override;
> @@ -78,38 +77,38 @@ CameraConfiguration
>  PipelineHandlerUVC::generateConfiguration(Camera *camera,
>  					  const StreamRoles &roles)
>  {
> -	UVCCameraData *data = cameraData(camera);
>  	CameraConfiguration config;
> -	StreamConfiguration cfg{};
> +	StreamConfiguration cfg;
>
>  	cfg.pixelFormat = V4L2_PIX_FMT_YUYV;
>  	cfg.size = { 640, 480 };
>  	cfg.bufferCount = 4;
>
> -	config[&data->stream_] = cfg;
> +	config.addConfiguration(cfg);
>
>  	return config;
>  }
>
> -int PipelineHandlerUVC::configure(Camera *camera,
> -				  const CameraConfiguration &config)
> +int PipelineHandlerUVC::configure(Camera *camera, CameraConfiguration &config)
>  {
>  	UVCCameraData *data = cameraData(camera);
> -	const StreamConfiguration *cfg = &config[&data->stream_];
> +	StreamConfiguration &cfg = config[0];
>  	int ret;
>
>  	V4L2DeviceFormat format = {};
> -	format.fourcc = cfg->pixelFormat;
> -	format.size = cfg->size;
> +	format.fourcc = cfg.pixelFormat;
> +	format.size = cfg.size;
>
>  	ret = data->video_->setFormat(&format);
>  	if (ret)
>  		return ret;
>
> -	if (format.size != cfg->size ||
> -	    format.fourcc != cfg->pixelFormat)
> +	if (format.size != cfg.size ||
> +	    format.fourcc != cfg.pixelFormat)
>  		return -EINVAL;
>
> +	cfg.setStream(&data->stream_);
> +
>  	return 0;
>  }
>
> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp
> index 17e2491e5c27..af6b6f21e3c5 100644
> --- a/src/libcamera/pipeline/vimc.cpp
> +++ b/src/libcamera/pipeline/vimc.cpp
> @@ -27,8 +27,7 @@ public:
>
>  	CameraConfiguration
>  	generateConfiguration(Camera *camera, const StreamRoles &roles) override;
> -	int configure(Camera *camera,
> -		      const CameraConfiguration &config) override;
> +	int configure(Camera *camera, CameraConfiguration &config) override;
>
>  	int allocateBuffers(Camera *camera,
>  			    const std::set<Stream *> &streams) override;
> @@ -78,38 +77,38 @@ CameraConfiguration
>  PipelineHandlerVimc::generateConfiguration(Camera *camera,
>  					   const StreamRoles &roles)
>  {
> -	VimcCameraData *data = cameraData(camera);
>  	CameraConfiguration config;
> -	StreamConfiguration cfg{};
> +	StreamConfiguration cfg;
>
>  	cfg.pixelFormat = V4L2_PIX_FMT_RGB24;
>  	cfg.size = { 640, 480 };
>  	cfg.bufferCount = 4;
>
> -	config[&data->stream_] = cfg;
> +	config.addConfiguration(cfg);
>
>  	return config;
>  }
>
> -int PipelineHandlerVimc::configure(Camera *camera,
> -				   const CameraConfiguration &config)
> +int PipelineHandlerVimc::configure(Camera *camera, CameraConfiguration &config)
>  {
>  	VimcCameraData *data = cameraData(camera);
> -	const StreamConfiguration *cfg = &config[&data->stream_];
> +	StreamConfiguration &cfg = config[0];
>  	int ret;
>
>  	V4L2DeviceFormat format = {};
> -	format.fourcc = cfg->pixelFormat;
> -	format.size = cfg->size;
> +	format.fourcc = cfg.pixelFormat;
> +	format.size = cfg.size;
>
>  	ret = data->video_->setFormat(&format);
>  	if (ret)
>  		return ret;
>
> -	if (format.size != cfg->size ||
> -	    format.fourcc != cfg->pixelFormat)
> +	if (format.size != cfg.size ||
> +	    format.fourcc != cfg.pixelFormat)
>  		return -EINVAL;
>
> +	cfg.setStream(&data->stream_);
> +
>  	return 0;
>  }
>
> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> index 81c11149c9fe..4185e3557dcb 100644
> --- a/src/libcamera/pipeline_handler.cpp
> +++ b/src/libcamera/pipeline_handler.cpp
> @@ -255,6 +255,10 @@ void PipelineHandler::unlock()
>   * configuration of a subset of the streams can't be satisfied, the
>   * whole configuration is considered invalid.
>   *
> + * Once the configuration is validated and the camera configured, the pipeline
> + * handler shall associate a Stream instance to each StreamConfiguration entry
> + * in the CameraConfiguration with the StreamConfiguration::setStream() method.
> + *
>   * \return 0 on success or a negative error code otherwise
>   */
>
> diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp
> index fe4c4ecf4150..0c59a31a3a05 100644
> --- a/src/libcamera/stream.cpp
> +++ b/src/libcamera/stream.cpp
> @@ -58,6 +58,28 @@ namespace libcamera {
>   * \brief Requested number of buffers to allocate for the stream
>   */
>
> +/**
> + * \fn StreamConfiguration::stream()
> + * \brief Retrieve the stream associated with the configuration
> + *
> + * When a camera is configured with Camera::configure() Stream instances are
> + * associated with each stream configuration entry. This method retrieves the
> + * associated Stream, which remains valid until the next call to
> + * Camera::configure() or Camera::release().
> + *
> + * \return The stream associated with the configuration
> + */
> +
> +/**
> + * \fn StreamConfiguration::setStream()
> + * \brief Associate a stream with a configuration
> + *
> + * This method is meant for the PipelineHandler::configure() method and shall
> + * not be called by applications.
> + *
> + * \param[in] stream The stream
> + */
> +
>  /**
>   * \brief Assemble and return a string describing the configuration
>   *
> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
> index a984aaca764f..06ae2985f80d 100644
> --- a/src/qcam/main_window.cpp
> +++ b/src/qcam/main_window.cpp
> @@ -98,14 +98,14 @@ int MainWindow::startCapture()
>  	int ret;
>
>  	config_ = camera_->generateConfiguration({ StreamRole::VideoRecording });
> -	Stream *stream = config_.front();
>  	ret = camera_->configure(config_);
>  	if (ret < 0) {
>  		std::cout << "Failed to configure camera" << std::endl;
>  		return ret;
>  	}
>
> -	const StreamConfiguration &cfg = config_[stream];
> +	const StreamConfiguration &cfg = config_[0];
> +	Stream *stream = cfg.stream();
>  	ret = viewfinder_->setFormat(cfg.pixelFormat, cfg.size.width,
>  				     cfg.size.height);
>  	if (ret < 0) {
> diff --git a/test/camera/capture.cpp b/test/camera/capture.cpp
> index e7e6438203b9..bfd11eefedcf 100644
> --- a/test/camera/capture.cpp
> +++ b/test/camera/capture.cpp
> @@ -44,8 +44,7 @@ protected:
>  	{
>  		CameraConfiguration config =
>  			camera_->generateConfiguration({ StreamRole::VideoRecording });
> -		Stream *stream = config.front();
> -		StreamConfiguration *cfg = &config[stream];
> +		StreamConfiguration *cfg = &config[0];
>
>  		if (!config.isValid()) {
>  			cout << "Failed to read default configuration" << endl;
> @@ -67,6 +66,7 @@ protected:
>  			return TestFail;
>  		}
>
> +		Stream *stream = cfg->stream();
>  		BufferPool &pool = stream->bufferPool();
>  		std::vector<Request *> requests;
>  		for (Buffer &buffer : pool.buffers()) {
> diff --git a/test/camera/configuration_set.cpp b/test/camera/configuration_set.cpp
> index 76d8bc3e40a4..25b5db67103a 100644
> --- a/test/camera/configuration_set.cpp
> +++ b/test/camera/configuration_set.cpp
> @@ -20,7 +20,7 @@ protected:
>  	{
>  		CameraConfiguration config =
>  			camera_->generateConfiguration({ StreamRole::VideoRecording });
> -		StreamConfiguration *cfg = &config[config.front()];
> +		StreamConfiguration *cfg = &config[0];
>
>  		if (!config.isValid()) {
>  			cout << "Failed to read default configuration" << endl;
> --
> 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:19:41AM +0200, Jacopo Mondi wrote:
> On Sun, May 19, 2019 at 06:00:44PM +0300, Laurent Pinchart wrote:
> > Refactor the CameraConfiguration structure to not rely on Stream
> > instances. This is a step towards making the camera configuration object
> > more powerful with configuration validation using "try" semantics.
> >
> > The CameraConfiguration now exposes a simple vector-like API to access
> > the contained stream configurations. Both operator[]() and at() are
> > provided to access elements. The isEmpty() method is renamed to empty()
> > and the methods reordered to match the std::vector class.
> >
> > As applications need access to the Stream instances associated with the
> > configuration entries in order to associate buffers with streams when
> > creating requests, expose the stream selected by the pipeline handler
> > through a new StreamConfiguration::stream().
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > ---
> >  include/libcamera/camera.h               |  36 +--
> >  include/libcamera/stream.h               |   7 +
> >  src/cam/main.cpp                         |  35 +--
> >  src/libcamera/camera.cpp                 | 268 +++++++++++------------
> >  src/libcamera/include/pipeline_handler.h |   2 +-
> >  src/libcamera/pipeline/ipu3/ipu3.cpp     |  32 +--
> >  src/libcamera/pipeline/rkisp1/rkisp1.cpp |  12 +-
> >  src/libcamera/pipeline/uvcvideo.cpp      |  23 +-
> >  src/libcamera/pipeline/vimc.cpp          |  23 +-
> >  src/libcamera/pipeline_handler.cpp       |   4 +
> >  src/libcamera/stream.cpp                 |  22 ++
> >  src/qcam/main_window.cpp                 |   4 +-
> >  test/camera/capture.cpp                  |   4 +-
> >  test/camera/configuration_set.cpp        |   2 +-
> >  14 files changed, 251 insertions(+), 223 deletions(-)
> >
> > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> > index 42ba5201eabc..284e5276a055 100644
> > --- a/include/libcamera/camera.h
> > +++ b/include/libcamera/camera.h
> > @@ -25,30 +25,36 @@ class Request;
> >  class CameraConfiguration
> >  {
> >  public:
> > -	using iterator = std::vector<Stream *>::iterator;
> > -	using const_iterator = std::vector<Stream *>::const_iterator;
> > +	using iterator = std::vector<StreamConfiguration>::iterator;
> > +	using const_iterator = std::vector<StreamConfiguration>::const_iterator;
> >
> >  	CameraConfiguration();
> >
> > +	void addConfiguration(const StreamConfiguration &cfg);
> 
> I would use push_back() to match the std::vector API

I've thought about it, but the goal here isn't to mimick the vector API
completely. For an interation point of view I think it makes sense to
operate as a vector, but when it comes to building the configuration I
think we need explicit calls (same for validation). addConfiguration()
may not be the best name either, maybe addStreamConfiguration() ?
There's also a high chance we'll refactor this some more.

> > +
> > +	bool isValid() const;
> 
> As you s/isEmpty()/empty()/ should we s/isValid()/valid()/ ?

Don't make me go back to isEmpty() ;-)

> Ah, don't worry, this will become validate(), just noticed
> 
> > +
> > +	StreamConfiguration &at(unsigned int index);
> > +	const StreamConfiguration &at(unsigned int index) const;
> > +	StreamConfiguration &operator[](unsigned int index)
> > +	{
> > +		return at(index);
> > +	}
> > +	const StreamConfiguration &operator[](unsigned int index) const
> > +	{
> > +		return at(index);
> > +	}
> 
> Ok, so we have [] and at() like std::vector, but it seems to me their
> behaviour is inverted.
> 
> std::vector::at() performs bound checking, and
> CameraConfiguration::at() is implemented with std::vector::operator[],
> which does not perform bounds checking
> 
> std::vector::operator[] does not perform bounds checking, but
> CameraConfiguration::operator[] is implemented with std::vector::at()
> which performs bound checking.

No, CameraConfiguration::operator[] is implemented with
CameraConfiguration::at(), which, as you noted above, doesn't perform
bound checking as it's implemented with std::vector::operator[].

> 
> Is this intentional ?

As we don't support exceptions, it makes no real difference, and as a
result I went for the operator that won't throw an exception.

> I would also question the need to have both operator[] and at()
> accessors, I know std::vector does, but do we need that or is it just
> an API expansion we could save?

It was requested by Niklas to replace (*config)[] with config->at() in
the callers. I have no personal problem with (*config)[], and if that's
desired, I'm fine dropping at().

> > +
> >  	iterator begin();
> > -	iterator end();
> >  	const_iterator begin() const;
> > +	iterator end();
> >  	const_iterator end() const;
> >
> > -	bool isValid() const;
> > -	bool isEmpty() const;
> > +	bool empty() const;
> >  	std::size_t size() const;
> >
> > -	Stream *front();
> > -	const Stream *front() const;
> > -
> > -	Stream *operator[](unsigned int index) const;
> > -	StreamConfiguration &operator[](Stream *stream);
> > -	const StreamConfiguration &operator[](Stream *stream) const;
> > -
> >  private:
> > -	std::vector<Stream *> order_;
> > -	std::map<Stream *, StreamConfiguration> config_;
> > +	std::vector<StreamConfiguration> config_;
> >  };
> >
> >  class Camera final
> > @@ -72,7 +78,7 @@ public:
> >
> >  	const std::set<Stream *> &streams() const;
> >  	CameraConfiguration generateConfiguration(const StreamRoles &roles);
> > -	int configure(const CameraConfiguration &config);
> > +	int configure(CameraConfiguration &config);
> >
> >  	int allocateBuffers();
> >  	int freeBuffers();
> > diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h
> > index 59bdf217eb31..47c007ed52e2 100644
> > --- a/include/libcamera/stream.h
> > +++ b/include/libcamera/stream.h
> > @@ -16,6 +16,7 @@
> >  namespace libcamera {
> >
> >  class Camera;
> > +class Stream;
> >
> >  struct StreamConfiguration {
> >  	unsigned int pixelFormat;
> > @@ -23,7 +24,13 @@ struct StreamConfiguration {
> >
> >  	unsigned int bufferCount;
> >
> > +	Stream *stream() const { return stream_; }
> > +	void setStream(Stream *stream) { stream_ = stream; }
> > +
> >  	std::string toString() const;
> > +
> > +private:
> > +	Stream *stream_;
> 
> Should we protect access to unitialized streams_ ?
> If StreamConfiguration is not inialized to {} what's the value of
> *stream_ ?

It's indeed not initialised, but applications are not allowed to use it
before the stream has been set by Camera::configure(). I can add a
default constructor, but I really hope to remove Stream from the public
API. I suppose it then won't hurt if I add the constructor just to be
safe in the meantime.

> >  };
> >
> >  enum StreamRole {
> > diff --git a/src/cam/main.cpp b/src/cam/main.cpp
> > index d603228c0116..cd165bea34cd 100644
> > --- a/src/cam/main.cpp
> > +++ b/src/cam/main.cpp
> > @@ -89,12 +89,9 @@ static int prepareCameraConfig(CameraConfiguration *config)
> >  {
> >  	StreamRoles roles;
> >
> > -	streamInfo.clear();
> > -
> >  	/* If no configuration is provided assume a single video stream. */
> >  	if (!options.isSet(OptStream)) {
> >  		*config = camera->generateConfiguration({ StreamRole::VideoRecording });
> > -		streamInfo[config->front()] = "stream0";
> >  		return 0;
> >  	}
> >
> > @@ -129,27 +126,20 @@ static int prepareCameraConfig(CameraConfiguration *config)
> >  	}
> >
> >  	/* Apply configuration explicitly requested. */
> > -	CameraConfiguration::iterator it = config->begin();
> > +	unsigned int i = 0;
> >  	for (auto const &value : streamOptions) {
> >  		KeyValueParser::Options conf = value.toKeyValues();
> 
> nit and not related to this patch: s/conf/opt ? otherwise we have
> *config and cfg in this loop, which is quite confusing.

I agree. I'll add a patch.

> > -		Stream *stream = *it;
> > -		it++;
> > +		StreamConfiguration &cfg = (*config)[i++];
> >
> >  		if (conf.isSet("width"))
> > -			(*config)[stream].size.width = conf["width"];
> > +			cfg.size.width = conf["width"];
> >
> >  		if (conf.isSet("height"))
> > -			(*config)[stream].size.height = conf["height"];
> > +			cfg.size.height = conf["height"];
> >
> >  		/* TODO: Translate 4CC string to ID. */
> >  		if (conf.isSet("pixelformat"))
> > -			(*config)[stream].pixelFormat = conf["pixelformat"];
> > -	}
> > -
> > -	unsigned int index = 0;
> > -	for (Stream *stream : *config) {
> > -		streamInfo[stream] = "stream" + std::to_string(index);
> > -		index++;
> > +			cfg.pixelFormat = conf["pixelformat"];
> >  	}
> >
> >  	return 0;
> > @@ -216,6 +206,13 @@ static int capture()
> >  		return ret;
> >  	}
> >
> > +	streamInfo.clear();
> > +
> > +	for (unsigned int index = 0; index < config.size(); ++index) {
> > +		StreamConfiguration &cfg = config[index];
> > +		streamInfo[cfg.stream()] = "stream" + std::to_string(index);
> > +	}
> > +
> >  	ret = camera->allocateBuffers();
> >  	if (ret) {
> >  		std::cerr << "Failed to allocate buffers"
> > @@ -227,8 +224,10 @@ static int capture()
> >
> >  	/* Identify the stream with the least number of buffers. */
> >  	unsigned int nbuffers = UINT_MAX;
> > -	for (Stream *stream : config)
> > +	for (StreamConfiguration &cfg : config) {
> > +		Stream *stream = cfg.stream();
> >  		nbuffers = std::min(nbuffers, stream->bufferPool().count());
> > +	}
> >
> >  	/*
> >  	 * TODO: make cam tool smarter to support still capture by for
> > @@ -245,8 +244,10 @@ static int capture()
> >  		}
> >
> >  		std::map<Stream *, Buffer *> map;
> > -		for (Stream *stream : config)
> > +		for (StreamConfiguration &cfg : config) {
> > +			Stream *stream = cfg.stream();
> >  			map[stream] = &stream->bufferPool().buffers()[i];
> > +		}
> >
> >  		ret = request->setBuffers(map);
> >  		if (ret < 0) {
> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > index a3921f91f1c9..5848330f639a 100644
> > --- a/src/libcamera/camera.cpp
> > +++ b/src/libcamera/camera.cpp
> > @@ -46,72 +46,40 @@ LOG_DECLARE_CATEGORY(Camera)
> >   * \class CameraConfiguration
> >   * \brief Hold configuration for streams of the camera
> >
> > - * The CameraConfiguration holds an ordered list of streams and their associated
> > - * StreamConfiguration. From a data storage point of view, the class operates as
> > - * a map of Stream pointers to StreamConfiguration, with entries accessed with
> > - * operator[](Stream *). Accessing an entry for a Stream pointer not yet stored
> > - * in the configuration inserts a new empty entry.
> > - *
> > - * The class also suppors iterators, and from that point of view operates as a
> > - * vector of Stream pointers. The streams are iterated in insertion order, and
> > - * the operator[](int) returns the Stream pointer based on its insertion index.
> > - * Accessing a stream with an invalid index returns a null pointer.
> > + * The CameraConfiguration holds an ordered list of stream configurations. It
> > + * supports iterators and operates as a vector of StreamConfiguration instances.
> > + * The stream configurations are inserted by addConfiguration(), and the
> > + * operator[](int) returns a reference to the StreamConfiguration based on its
> > + * insertion index. Accessing a stream configuration with an invalid index
> > + * results in undefined behaviour.
> 
> As operator[] is implemented with std::vector::at() accessing with an
> invalid index, an exception is thrown (even if we don't use them in
> the library).

It's actually implemented with std::evctor::operator[] as explained
above :-)

> I would document CameraConfiguration::at() as well, or just provide one of
> the two only.

The at() functions are documented ;-)

> >   */
> >
> >  /**
> >   * \typedef CameraConfiguration::iterator
> > - * \brief Iterator for the streams in the configuration
> > + * \brief Iterator for the stream configurations in the camera configuration
> >   */
> >
> >  /**
> >   * \typedef CameraConfiguration::const_iterator
> > - * \brief Const iterator for the streams in the configuration
> > + * \brief Const iterator for the stream configuration in the camera
> > + * configuration
> >   */
> >
> >  /**
> >   * \brief Create an empty camera configuration
> >   */
> >  CameraConfiguration::CameraConfiguration()
> > -	: order_({}), config_({})
> > +	: config_({})
> >  {
> >  }
> >
> >  /**
> > - * \brief Retrieve an iterator to the first stream in the sequence
> > - * \return An iterator to the first stream
> > + * \brief Add a stream configuration to the camera configuration
> > + * \param[in] cfg The stream configuration
> >   */
> > -std::vector<Stream *>::iterator CameraConfiguration::begin()
> > +void CameraConfiguration::addConfiguration(const StreamConfiguration &cfg)
> >  {
> > -	return order_.begin();
> > -}
> > -
> > -/**
> > - * \brief Retrieve an iterator pointing to the past-the-end stream in the
> > - * sequence
> > - * \return An iterator to the element following the last stream
> > - */
> > -std::vector<Stream *>::iterator CameraConfiguration::end()
> > -{
> > -	return order_.end();
> > -}
> > -
> > -/**
> > - * \brief Retrieve a const iterator to the first element of the streams
> > - * \return A const iterator to the first stream
> > - */
> > -std::vector<Stream *>::const_iterator CameraConfiguration::begin() const
> > -{
> > -	return order_.begin();
> > -}
> > -
> > -/**
> > - * \brief Retrieve a const iterator pointing to the past-the-end stream in the
> > - * sequence
> > - * \return A const iterator to the element following the last stream
> > - */
> > -std::vector<Stream *>::const_iterator CameraConfiguration::end() const
> > -{
> > -	return order_.end();
> > +	config_.push_back(cfg);
> >  }
> >
> >  /**
> > @@ -125,12 +93,10 @@ std::vector<Stream *>::const_iterator CameraConfiguration::end() const
> >   */
> >  bool CameraConfiguration::isValid() const
> >  {
> > -	if (isEmpty())
> > +	if (empty())
> >  		return false;
> >
> > -	for (auto const &it : config_) {
> > -		const StreamConfiguration &cfg = it.second;
> > -
> > +	for (const StreamConfiguration &cfg : config_) {
> >  		if (cfg.size.width == 0 || cfg.size.height == 0 ||
> >  		    cfg.pixelFormat == 0 || cfg.bufferCount == 0)
> >  			return false;
> > @@ -139,13 +105,108 @@ bool CameraConfiguration::isValid() const
> >  	return true;
> >  }
> >
> > +/**
> > + * \brief Retrieve a reference to a stream configuration
> > + * \param[in] index Numerical index
> 
> STL uses pos in place of index. Not sure we care, though.

I had considered this too :-) In the end I went for index to be
consistent with what we usually use (and I think it's also more explicit
than pos in this particular case).

> > + *
> > + * The \a index represents the zero based insertion order of stream
> > + * configuration into the camera configuration with addConfiguration(). Calling
> > + * this method with an invalid index results in undefined behaviour.
> > + *
> > + * \return The stream configuration
> 
> > + */
> > +StreamConfiguration &CameraConfiguration::at(unsigned int index)
> > +{
> > +	return config_[index];
> > +}
> > +
> > +/**
> > + * \brief Retrieve a const reference to a stream configuration
> > + * \param[in] index Numerical index
> > + *
> > + * The \a index represents the zero based insertion order of stream
> > + * configuration into the camera configuration with addConfiguration(). Calling
> > + * this method with an invalid index results in undefined behaviour.
> > + *
> > + * \return The stream configuration
> > + */
> > +const StreamConfiguration &CameraConfiguration::at(unsigned int index) const
> > +{
> > +	return config_[index];
> > +}
> > +
> > +/**
> > + * \fn StreamConfiguration &CameraConfiguration::operator[](unsigned int)
> > + * \brief Retrieve a reference to a stream configuration
> > + * \param[in] index Numerical index
> > + *
> > + * The \a index represents the zero based insertion order of stream
> > + * configuration into the camera configuration with addConfiguration(). Calling
> > + * this method with an invalid index results in undefined behaviour.
> > + *
> > + * \return The stream configuration
> > + */
> > +
> > +/**
> > + * \fn const StreamConfiguration &CameraConfiguration::operator[](unsigned int) const
> > + * \brief Retrieve a const reference to a stream configuration
> > + * \param[in] index Numerical index
> > + *
> > + * The \a index represents the zero based insertion order of stream
> > + * configuration into the camera configuration with addConfiguration(). Calling
> > + * this method with an invalid index results in undefined behaviour.
> > + *
> > + * \return The stream configuration
> > + */
> > +
> > +/**
> > + * \brief Retrieve an iterator to the first stream configuration in the
> > + * sequence
> > + * \return An iterator to the first stream configuration
> > + */
> > +CameraConfiguration::iterator CameraConfiguration::begin()
> > +{
> > +	return config_.begin();
> > +}
> > +
> > +/**
> > + * \brief Retrieve a const iterator to the first element of the stream
> > + * configurations
> > + * \return A const iterator to the first stream configuration
> > + */
> > +CameraConfiguration::const_iterator CameraConfiguration::begin() const
> > +{
> > +	return config_.begin();
> > +}
> > +
> > +/**
> > + * \brief Retrieve an iterator pointing to the past-the-end stream
> > + * configuration in the sequence
> > + * \return An iterator to the element following the last stream configuration
> > + */
> > +CameraConfiguration::iterator CameraConfiguration::end()
> 
> Aren't these StreamConfiguration::iterators ?

There's no StreamConfiguration::iterator. CameraConfiguration::iterator
is the iterator that iterates over the contents of CameraConfiguration.
This is similar to std::vector::iterator iterating over the contents of
the vector, we don't use int::iterator to iterate over a
std::vector<int>. The iterator is implemented by the container, not the
contained data.

> > +{
> > +	return config_.end();
> > +}
> > +
> > +/**
> > + * \brief Retrieve a const iterator pointing to the past-the-end stream
> > + * configuration in the sequence
> > + * \return A const iterator to the element following the last stream
> > + * configuration
> > + */
> > +CameraConfiguration::const_iterator CameraConfiguration::end() const
> > +{
> > +	return config_.end();
> > +}
> > +
> >  /**
> >   * \brief Check if the camera configuration is empty
> >   * \return True if the configuration is empty
> >   */
> > -bool CameraConfiguration::isEmpty() const
> > +bool CameraConfiguration::empty() const
> >  {
> > -	return order_.empty();
> > +	return config_.empty();
> >  }
> >
> >  /**
> > @@ -154,75 +215,7 @@ bool CameraConfiguration::isEmpty() const
> >   */
> >  std::size_t CameraConfiguration::size() const
> >  {
> > -	return order_.size();
> > -}
> > -
> > -/**
> > - * \brief Access the first stream in the configuration
> > - * \return The first stream in the configuration
> > - */
> > -Stream *CameraConfiguration::front()
> > -{
> > -	return order_.front();
> > -}
> > -
> > -/**
> > - * \brief Access the first stream in the configuration
> > - * \return The first const stream pointer in the configuration
> > - */
> > -const Stream *CameraConfiguration::front() const
> > -{
> > -	return order_.front();
> > -}
> > -
> > -/**
> > - * \brief Retrieve a stream pointer from index
> > - * \param[in] index Numerical index
> > - *
> > - * The \a index represents the zero based insertion order of stream and stream
> > - * configuration into the camera configuration.
> > - *
> > - * \return The stream pointer at index, or a nullptr if the index is out of
> > - * bounds
> > - */
> > -Stream *CameraConfiguration::operator[](unsigned int index) const
> > -{
> > -	if (index >= order_.size())
> > -		return nullptr;
> > -
> > -	return order_.at(index);
> > -}
> > -
> > -/**
> > - * \brief Retrieve a reference to a stream configuration
> > - * \param[in] stream Stream to retrieve configuration for
> > - *
> > - * If the camera configuration does not yet contain a configuration for
> > - * the requested stream, create and return an empty stream configuration.
> > - *
> > - * \return The configuration for the stream
> > - */
> > -StreamConfiguration &CameraConfiguration::operator[](Stream *stream)
> > -{
> > -	if (config_.find(stream) == config_.end())
> > -		order_.push_back(stream);
> > -
> > -	return config_[stream];
> > -}
> > -
> > -/**
> > - * \brief Retrieve a const reference to a stream configuration
> > - * \param[in] stream Stream to retrieve configuration for
> > - *
> > - * No new stream configuration is created if called with \a stream that is not
> > - * already part of the camera configuration, doing so is an invalid operation
> > - * and results in undefined behaviour.
> > - *
> > - * \return The configuration for the stream
> > - */
> > -const StreamConfiguration &CameraConfiguration::operator[](Stream *stream) const
> > -{
> > -	return config_.at(stream);
> > +	return config_.size();
> >  }
> >
> >  /**
> > @@ -561,13 +554,9 @@ Camera::generateConfiguration(const StreamRoles &roles)
> >  	CameraConfiguration config = pipe_->generateConfiguration(this, roles);
> >
> >  	std::ostringstream msg("streams configuration:", std::ios_base::ate);
> > -	unsigned int index = 0;
> >
> > -	for (Stream *stream : config) {
> > -		const StreamConfiguration &cfg = config[stream];
> > -		msg << " (" << index << ") " << cfg.toString();
> > -		index++;
> > -	}
> > +	for (unsigned int index = 0; index < config.size(); ++index)
> > +		msg << " (" << index << ") " << config[index].toString();
> >
> >  	LOG(Camera, Debug) << msg.str();
> >
> > @@ -593,12 +582,15 @@ Camera::generateConfiguration(const StreamRoles &roles)
> >   *
> >   * This function affects the state of the camera, see \ref camera_operation.
> >   *
> > + * Upon return the StreamConfiguration entries in \a config are associated with
> > + * Stream instances which can be retrieved with StreamConfiguration::stream().
> > + *
> >   * \return 0 on success or a negative error code otherwise
> >   * \retval -ENODEV The camera has been disconnected from the system
> >   * \retval -EACCES The camera is not in a state where it can be configured
> >   * \retval -EINVAL The configuration is not valid
> >   */
> > -int Camera::configure(const CameraConfiguration &config)
> > +int Camera::configure(CameraConfiguration &config)
> >  {
> >  	int ret;
> >
> > @@ -615,16 +607,11 @@ int Camera::configure(const CameraConfiguration &config)
> >  	}
> >
> >  	std::ostringstream msg("configuring streams:", std::ios_base::ate);
> > -	unsigned int index = 0;
> >
> > -	for (Stream *stream : config) {
> > -		if (streams_.find(stream) == streams_.end())
> > -			return -EINVAL;
> > -
> > -		const StreamConfiguration &cfg = config[stream];
> > -		msg << std::dec << " (" << index << ") " << cfg.toString();
> > -
> > -		index++;
> > +	for (unsigned int index = 0; index < config.size(); ++index) {
> > +		StreamConfiguration &cfg = config[index];
> > +		cfg.setStream(nullptr);
> > +		msg << " (" << index << ") " << cfg.toString();
> 
> Isn't this better printed after pipe->configure(), once we know all
> streams configuration have a stream assigned ?

It shouldn't make a difference as the stream is not printed, and I think
it's useful to print it before in case pipe->configure() fails, to help
debugging the issue.

> Overall this is very good. The fact StreamConfiguration instances are
> associated to Stream just after configure() might require clearly
> preventing applications to try access it, but I think the
> documentation is quite clear on that.

I don't like that part too much, but I didn't go through great lengths
to fix it as I plan to remove the Stream anyway.

> The biggest part is discussing the CameraConfiguration API I guess,
> which we might want to make more similar to std::vector.
> 
> >  	}
> >
> >  	LOG(Camera, Info) << msg.str();
> > @@ -634,8 +621,11 @@ int Camera::configure(const CameraConfiguration &config)
> >  		return ret;
> >
> >  	activeStreams_.clear();
> > -	for (Stream *stream : config) {
> > -		const StreamConfiguration &cfg = config[stream];
> > +	for (const StreamConfiguration &cfg : config) {
> > +		Stream *stream = cfg.stream();
> > +		if (!stream)
> > +			LOG(Camera, Fatal)
> > +				<< "Pipeline handler failed to update stream configuration";
> >
> >  		stream->configuration_ = cfg;
> >  		activeStreams_.insert(stream);
> > diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h
> > index 3352cb0e5bc9..a025905ab68f 100644
> > --- a/src/libcamera/include/pipeline_handler.h
> > +++ b/src/libcamera/include/pipeline_handler.h
> > @@ -62,7 +62,7 @@ public:
> >
> >  	virtual CameraConfiguration
> >  	generateConfiguration(Camera *camera, const StreamRoles &roles) = 0;
> > -	virtual int configure(Camera *camera, const CameraConfiguration &config) = 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 d234a8ac5289..ed0ef69de1d1 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -152,8 +152,7 @@ public:
> >
> >  	CameraConfiguration
> >  	generateConfiguration(Camera *camera, const StreamRoles &roles) override;
> > -	int configure(Camera *camera,
> > -		      const CameraConfiguration &config) override;
> > +	int configure(Camera *camera, CameraConfiguration &config) override;
> >
> >  	int allocateBuffers(Camera *camera,
> >  			    const std::set<Stream *> &streams) override;
> > @@ -299,14 +298,13 @@ PipelineHandlerIPU3::generateConfiguration(Camera *camera,
> >  		cfg.pixelFormat = V4L2_PIX_FMT_NV12;
> >  		cfg.bufferCount = IPU3_BUFFER_COUNT;
> >
> > -		config[stream] = cfg;
> > +		config.addConfiguration(cfg);
> >  	}
> >
> >  	return config;
> >  }
> >
> > -int PipelineHandlerIPU3::configure(Camera *camera,
> > -				   const CameraConfiguration &config)
> > +int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration &config)
> >  {
> >  	IPU3CameraData *data = cameraData(camera);
> >  	IPU3Stream *outStream = &data->outStream_;
> > @@ -318,9 +316,13 @@ int PipelineHandlerIPU3::configure(Camera *camera,
> >
> >  	outStream->active_ = false;
> >  	vfStream->active_ = false;
> > -	for (Stream *s : config) {
> > -		IPU3Stream *stream = static_cast<IPU3Stream *>(s);
> > -		const StreamConfiguration &cfg = config[stream];
> > +	for (StreamConfiguration &cfg : config) {
> > +		/*
> > +		 * Pick a stream for the configuration entry.
> > +		 * \todo: This is a naive temporary implementation that will be
> > +		 * reworked when implementing camera configuration validation.
> > +		 */
> > +		IPU3Stream *stream = vfStream->active_ ? outStream : vfStream;
> >
> >  		/*
> >  		 * Verify that the requested size respects the IPU3 alignment
> > @@ -355,6 +357,7 @@ int PipelineHandlerIPU3::configure(Camera *camera,
> >  			sensorSize.height = cfg.size.height;
> >
> >  		stream->active_ = true;
> > +		cfg.setStream(stream);
> >  	}
> >
> >  	/*
> > @@ -379,10 +382,9 @@ int PipelineHandlerIPU3::configure(Camera *camera,
> >  		return ret;
> >
> >  	/* Apply the format to the configured streams output devices. */
> > -	for (Stream *s : config) {
> > -		IPU3Stream *stream = static_cast<IPU3Stream *>(s);
> > -
> > -		ret = imgu->configureOutput(stream->device_, config[stream]);
> > +	for (StreamConfiguration &cfg : config) {
> > +		IPU3Stream *stream = static_cast<IPU3Stream *>(cfg.stream());
> > +		ret = imgu->configureOutput(stream->device_, cfg);
> >  		if (ret)
> >  			return ret;
> >  	}
> > @@ -393,15 +395,13 @@ int PipelineHandlerIPU3::configure(Camera *camera,
> >  	 * be at least one active stream in the configuration request).
> >  	 */
> >  	if (!outStream->active_) {
> > -		ret = imgu->configureOutput(outStream->device_,
> > -					    config[vfStream]);
> > +		ret = imgu->configureOutput(outStream->device_, config[0]);
> >  		if (ret)
> >  			return ret;
> >  	}
> >
> >  	if (!vfStream->active_) {
> > -		ret = imgu->configureOutput(vfStream->device_,
> > -					    config[outStream]);
> > +		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 4bd8c5101a96..ec6980b0943a 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > @@ -36,8 +36,7 @@ public:
> >
> >  	CameraConfiguration generateConfiguration(Camera *camera,
> >  		const StreamRoles &roles) override;
> > -	int configure(Camera *camera,
> > -		const CameraConfiguration &config) override;
> > +	int configure(Camera *camera, CameraConfiguration &config) override;
> >
> >  	int allocateBuffers(Camera *camera,
> >  		const std::set<Stream *> &streams) override;
> > @@ -117,16 +116,15 @@ CameraConfiguration PipelineHandlerRkISP1::generateConfiguration(Camera *camera,
> >  	cfg.size = data->sensor_->resolution();
> >  	cfg.bufferCount = RKISP1_BUFFER_COUNT;
> >
> > -	config[&data->stream_] = cfg;
> > +	config.addConfiguration(cfg);
> >
> >  	return config;
> >  }
> >
> > -int PipelineHandlerRkISP1::configure(Camera *camera,
> > -				     const CameraConfiguration &config)
> > +int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration &config)
> >  {
> >  	RkISP1CameraData *data = cameraData(camera);
> > -	const StreamConfiguration &cfg = config[&data->stream_];
> > +	StreamConfiguration &cfg = config[0];
> >  	CameraSensor *sensor = data->sensor_;
> >  	int ret;
> >
> > @@ -217,6 +215,8 @@ int PipelineHandlerRkISP1::configure(Camera *camera,
> >  		return -EINVAL;
> >  	}
> >
> > +	cfg.setStream(&data->stream_);
> > +
> >  	return 0;
> >  }
> >
> > diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp
> > index d2e1f7d4e5b2..5dcc868b2fc9 100644
> > --- a/src/libcamera/pipeline/uvcvideo.cpp
> > +++ b/src/libcamera/pipeline/uvcvideo.cpp
> > @@ -27,8 +27,7 @@ public:
> >
> >  	CameraConfiguration
> >  	generateConfiguration(Camera *camera, const StreamRoles &roles) override;
> > -	int configure(Camera *camera,
> > -		      const CameraConfiguration &config) override;
> > +	int configure(Camera *camera, CameraConfiguration &config) override;
> >
> >  	int allocateBuffers(Camera *camera,
> >  			    const std::set<Stream *> &streams) override;
> > @@ -78,38 +77,38 @@ CameraConfiguration
> >  PipelineHandlerUVC::generateConfiguration(Camera *camera,
> >  					  const StreamRoles &roles)
> >  {
> > -	UVCCameraData *data = cameraData(camera);
> >  	CameraConfiguration config;
> > -	StreamConfiguration cfg{};
> > +	StreamConfiguration cfg;
> >
> >  	cfg.pixelFormat = V4L2_PIX_FMT_YUYV;
> >  	cfg.size = { 640, 480 };
> >  	cfg.bufferCount = 4;
> >
> > -	config[&data->stream_] = cfg;
> > +	config.addConfiguration(cfg);
> >
> >  	return config;
> >  }
> >
> > -int PipelineHandlerUVC::configure(Camera *camera,
> > -				  const CameraConfiguration &config)
> > +int PipelineHandlerUVC::configure(Camera *camera, CameraConfiguration &config)
> >  {
> >  	UVCCameraData *data = cameraData(camera);
> > -	const StreamConfiguration *cfg = &config[&data->stream_];
> > +	StreamConfiguration &cfg = config[0];
> >  	int ret;
> >
> >  	V4L2DeviceFormat format = {};
> > -	format.fourcc = cfg->pixelFormat;
> > -	format.size = cfg->size;
> > +	format.fourcc = cfg.pixelFormat;
> > +	format.size = cfg.size;
> >
> >  	ret = data->video_->setFormat(&format);
> >  	if (ret)
> >  		return ret;
> >
> > -	if (format.size != cfg->size ||
> > -	    format.fourcc != cfg->pixelFormat)
> > +	if (format.size != cfg.size ||
> > +	    format.fourcc != cfg.pixelFormat)
> >  		return -EINVAL;
> >
> > +	cfg.setStream(&data->stream_);
> > +
> >  	return 0;
> >  }
> >
> > diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp
> > index 17e2491e5c27..af6b6f21e3c5 100644
> > --- a/src/libcamera/pipeline/vimc.cpp
> > +++ b/src/libcamera/pipeline/vimc.cpp
> > @@ -27,8 +27,7 @@ public:
> >
> >  	CameraConfiguration
> >  	generateConfiguration(Camera *camera, const StreamRoles &roles) override;
> > -	int configure(Camera *camera,
> > -		      const CameraConfiguration &config) override;
> > +	int configure(Camera *camera, CameraConfiguration &config) override;
> >
> >  	int allocateBuffers(Camera *camera,
> >  			    const std::set<Stream *> &streams) override;
> > @@ -78,38 +77,38 @@ CameraConfiguration
> >  PipelineHandlerVimc::generateConfiguration(Camera *camera,
> >  					   const StreamRoles &roles)
> >  {
> > -	VimcCameraData *data = cameraData(camera);
> >  	CameraConfiguration config;
> > -	StreamConfiguration cfg{};
> > +	StreamConfiguration cfg;
> >
> >  	cfg.pixelFormat = V4L2_PIX_FMT_RGB24;
> >  	cfg.size = { 640, 480 };
> >  	cfg.bufferCount = 4;
> >
> > -	config[&data->stream_] = cfg;
> > +	config.addConfiguration(cfg);
> >
> >  	return config;
> >  }
> >
> > -int PipelineHandlerVimc::configure(Camera *camera,
> > -				   const CameraConfiguration &config)
> > +int PipelineHandlerVimc::configure(Camera *camera, CameraConfiguration &config)
> >  {
> >  	VimcCameraData *data = cameraData(camera);
> > -	const StreamConfiguration *cfg = &config[&data->stream_];
> > +	StreamConfiguration &cfg = config[0];
> >  	int ret;
> >
> >  	V4L2DeviceFormat format = {};
> > -	format.fourcc = cfg->pixelFormat;
> > -	format.size = cfg->size;
> > +	format.fourcc = cfg.pixelFormat;
> > +	format.size = cfg.size;
> >
> >  	ret = data->video_->setFormat(&format);
> >  	if (ret)
> >  		return ret;
> >
> > -	if (format.size != cfg->size ||
> > -	    format.fourcc != cfg->pixelFormat)
> > +	if (format.size != cfg.size ||
> > +	    format.fourcc != cfg.pixelFormat)
> >  		return -EINVAL;
> >
> > +	cfg.setStream(&data->stream_);
> > +
> >  	return 0;
> >  }
> >
> > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> > index 81c11149c9fe..4185e3557dcb 100644
> > --- a/src/libcamera/pipeline_handler.cpp
> > +++ b/src/libcamera/pipeline_handler.cpp
> > @@ -255,6 +255,10 @@ void PipelineHandler::unlock()
> >   * configuration of a subset of the streams can't be satisfied, the
> >   * whole configuration is considered invalid.
> >   *
> > + * Once the configuration is validated and the camera configured, the pipeline
> > + * handler shall associate a Stream instance to each StreamConfiguration entry
> > + * in the CameraConfiguration with the StreamConfiguration::setStream() method.
> > + *
> >   * \return 0 on success or a negative error code otherwise
> >   */
> >
> > diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp
> > index fe4c4ecf4150..0c59a31a3a05 100644
> > --- a/src/libcamera/stream.cpp
> > +++ b/src/libcamera/stream.cpp
> > @@ -58,6 +58,28 @@ namespace libcamera {
> >   * \brief Requested number of buffers to allocate for the stream
> >   */
> >
> > +/**
> > + * \fn StreamConfiguration::stream()
> > + * \brief Retrieve the stream associated with the configuration
> > + *
> > + * When a camera is configured with Camera::configure() Stream instances are
> > + * associated with each stream configuration entry. This method retrieves the
> > + * associated Stream, which remains valid until the next call to
> > + * Camera::configure() or Camera::release().
> > + *
> > + * \return The stream associated with the configuration
> > + */
> > +
> > +/**
> > + * \fn StreamConfiguration::setStream()
> > + * \brief Associate a stream with a configuration
> > + *
> > + * This method is meant for the PipelineHandler::configure() method and shall
> > + * not be called by applications.
> > + *
> > + * \param[in] stream The stream
> > + */
> > +
> >  /**
> >   * \brief Assemble and return a string describing the configuration
> >   *
> > diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
> > index a984aaca764f..06ae2985f80d 100644
> > --- a/src/qcam/main_window.cpp
> > +++ b/src/qcam/main_window.cpp
> > @@ -98,14 +98,14 @@ int MainWindow::startCapture()
> >  	int ret;
> >
> >  	config_ = camera_->generateConfiguration({ StreamRole::VideoRecording });
> > -	Stream *stream = config_.front();
> >  	ret = camera_->configure(config_);
> >  	if (ret < 0) {
> >  		std::cout << "Failed to configure camera" << std::endl;
> >  		return ret;
> >  	}
> >
> > -	const StreamConfiguration &cfg = config_[stream];
> > +	const StreamConfiguration &cfg = config_[0];
> > +	Stream *stream = cfg.stream();
> >  	ret = viewfinder_->setFormat(cfg.pixelFormat, cfg.size.width,
> >  				     cfg.size.height);
> >  	if (ret < 0) {
> > diff --git a/test/camera/capture.cpp b/test/camera/capture.cpp
> > index e7e6438203b9..bfd11eefedcf 100644
> > --- a/test/camera/capture.cpp
> > +++ b/test/camera/capture.cpp
> > @@ -44,8 +44,7 @@ protected:
> >  	{
> >  		CameraConfiguration config =
> >  			camera_->generateConfiguration({ StreamRole::VideoRecording });
> > -		Stream *stream = config.front();
> > -		StreamConfiguration *cfg = &config[stream];
> > +		StreamConfiguration *cfg = &config[0];
> >
> >  		if (!config.isValid()) {
> >  			cout << "Failed to read default configuration" << endl;
> > @@ -67,6 +66,7 @@ protected:
> >  			return TestFail;
> >  		}
> >
> > +		Stream *stream = cfg->stream();
> >  		BufferPool &pool = stream->bufferPool();
> >  		std::vector<Request *> requests;
> >  		for (Buffer &buffer : pool.buffers()) {
> > diff --git a/test/camera/configuration_set.cpp b/test/camera/configuration_set.cpp
> > index 76d8bc3e40a4..25b5db67103a 100644
> > --- a/test/camera/configuration_set.cpp
> > +++ b/test/camera/configuration_set.cpp
> > @@ -20,7 +20,7 @@ protected:
> >  	{
> >  		CameraConfiguration config =
> >  			camera_->generateConfiguration({ StreamRole::VideoRecording });
> > -		StreamConfiguration *cfg = &config[config.front()];
> > +		StreamConfiguration *cfg = &config[0];
> >
> >  		if (!config.isValid()) {
> >  			cout << "Failed to read default configuration" << endl;