[libcamera-devel,v2,04/10] libcamera: ipu3: Breakout stream assignment to new function

3966 diff mbox series

Selecting which stream is the most suitable for the requested
configuration is mixed with adjusting the requested format when
validating configurations. This is hard to read and got worse when
support for Bayer formats was added, break it out to a separate
function.

Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
---
* Changes since v1
- Update commit message.
- Rename updateStreams() to assignStreams().
- Revert and keep old comment on how streams are picked.
- Do not modify behavior on how streams are picked which means
  assignStreams() now can't fail so no need for it to return an int,
  switch to void.
---
 src/libcamera/pipeline/ipu3/ipu3.cpp | 79 ++++++++++++++++------------
 1 file changed, 44 insertions(+), 35 deletions(-)

Hi Niklas,

Thank you for the patch.

On Sat, Jun 06, 2020 at 05:04:30PM +0200, Niklas Söderlund wrote:
> Selecting which stream is the most suitable for the requested
> configuration is mixed with adjusting the requested format when
> validating configurations. This is hard to read and got worse when
> support for Bayer formats was added, break it out to a separate

s/, break/. Break/

> function.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> ---
> * Changes since v1
> - Update commit message.
> - Rename updateStreams() to assignStreams().
> - Revert and keep old comment on how streams are picked.
> - Do not modify behavior on how streams are picked which means
>   assignStreams() now can't fail so no need for it to return an int,
>   switch to void.
> ---
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 79 ++++++++++++++++------------
>  1 file changed, 44 insertions(+), 35 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index f7363244e1d2d0ff..6df93eedb365b904 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -190,6 +190,7 @@ private:
>  	static constexpr unsigned int IPU3_BUFFER_COUNT = 4;
>  	static constexpr unsigned int IPU3_MAX_STREAMS = 3;
>  
> +	void assignStreams();
>  	void adjustStream(StreamConfiguration &cfg, bool scale);
>  
>  	/*
> @@ -256,6 +257,42 @@ IPU3CameraConfiguration::IPU3CameraConfiguration(Camera *camera,
>  	data_ = data;
>  }
>  
> +void IPU3CameraConfiguration::assignStreams()
> +{
> +	/*
> +	 * Verify and update all configuration entries, and assign a stream to
> +	 * each of them. The viewfinder stream can scale, while the output
> +	 * stream can crop only, so select the output stream when the requested
> +	 * resolution is equal to the sensor resolution, and the viewfinder
> +	 * stream otherwise.
> +	 */
> +	std::set<const IPU3Stream *> availableStreams = {
> +		&data_->outStream_,
> +		&data_->vfStream_,
> +		&data_->rawStream_,
> +	};
> +
> +	streams_.clear();
> +	streams_.reserve(config_.size());
> +
> +	for (const StreamConfiguration &cfg : config_) {
> +		const IPU3Stream *stream;
> +
> +		if (cfg.pixelFormat.modifier() == IPU3_FORMAT_MOD_PACKED)
> +			stream = &data_->rawStream_;
> +		else if (cfg.size == sensorFormat_.size)
> +			stream = &data_->outStream_;
> +		else
> +			stream = &data_->vfStream_;
> +
> +		if (availableStreams.find(stream) == availableStreams.end())
> +			stream = *availableStreams.begin();
> +
> +		streams_.push_back(stream);
> +		availableStreams.erase(stream);

I was going to comment that you should check that availableStreams
doesn't become empty, but then realized the caller already clamps the
number of streams. Maybe a comment above this function should capture
the pre-conditions ?

I think we'll have to extend the logic here to cover the case where the
user requests three streams and none of them has a raw format. The above
code will select the raw stream as the third stream. Wouldn't it make
more sense in that case to only return two streams ? This can be
discussed and addressed separately from this patch, and in general I
think we need to document the heuristics that pipeline handlers shall
implement to validate the camera configuration.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +	}
> +}
> +
>  void IPU3CameraConfiguration::adjustStream(StreamConfiguration &cfg, bool scale)
>  {
>  	/* The only pixel format the driver supports is NV12. */
> @@ -342,40 +379,14 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()
>  	if (!sensorFormat_.size.width || !sensorFormat_.size.height)
>  		sensorFormat_.size = sensor->resolution();
>  
> -	/*
> -	 * Verify and update all configuration entries, and assign a stream to
> -	 * each of them. The viewfinder stream can scale, while the output
> -	 * stream can crop only, so select the output stream when the requested
> -	 * resolution is equal to the sensor resolution, and the viewfinder
> -	 * stream otherwise.
> -	 */
> -	std::set<const IPU3Stream *> availableStreams = {
> -		&data_->outStream_,
> -		&data_->vfStream_,
> -		&data_->rawStream_,
> -	};
> -
> -	streams_.clear();
> -	streams_.reserve(config_.size());
> +	/* Assign streams to each configuration entry. */
> +	assignStreams();
>  
> +	/* Verify and adjust configuration if needed. */
>  	for (unsigned int i = 0; i < config_.size(); ++i) {
>  		StreamConfiguration &cfg = config_[i];
> -		const PixelFormat pixelFormat = cfg.pixelFormat;
> -		const Size size = cfg.size;
> -		const IPU3Stream *stream;
> -
> -		if (cfg.pixelFormat.modifier() == IPU3_FORMAT_MOD_PACKED)
> -			stream = &data_->rawStream_;
> -		else if (cfg.size == sensorFormat_.size)
> -			stream = &data_->outStream_;
> -		else
> -			stream = &data_->vfStream_;
> -
> -		if (availableStreams.find(stream) == availableStreams.end())
> -			stream = *availableStreams.begin();
> -
> -		LOG(IPU3, Debug)
> -			<< "Assigned '" << stream->name_ << "' to stream " << i;
> +		const StreamConfiguration oldCfg = cfg;
> +		const IPU3Stream *stream = streams_[i];
>  
>  		if (stream->raw_) {
>  			const auto &itFormat =
> @@ -392,15 +403,13 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()
>  
>  		cfg.bufferCount = IPU3_BUFFER_COUNT;
>  
> -		if (cfg.pixelFormat != pixelFormat || cfg.size != size) {
> +		if (cfg.pixelFormat != oldCfg.pixelFormat ||
> +		    cfg.size != oldCfg.size) {
>  			LOG(IPU3, Debug)
>  				<< "Stream " << i << " configuration adjusted to "
>  				<< cfg.toString();
>  			status = Adjusted;
>  		}
> -
> -		streams_.push_back(stream);
> -		availableStreams.erase(stream);
>  	}
>  
>  	return status;