[libcamera-devel] [PATCH v3 03/10] libcamera: ipu3: Breakout stream assignment to new function

[Laurent Pinchart]

Hi Jacopo,

On Wed, Jun 24, 2020 at 11:39:42AM +0200, Jacopo Mondi wrote:
> On Tue, Jun 23, 2020 at 04:09:23AM +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
> > function.
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> > * Changes since v2
> > - Document the fact that the number of requested streams in config_
> >   should be validated before calling assignStreams(). Add an ASSERT()
> >   and comment to catch the issue early if this should ever change.
> >
> > * 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 | 88 ++++++++++++++++------------
> >  1 file changed, 52 insertions(+), 36 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index c525e30a5ad35011..3b2ec684244881e5 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -191,6 +191,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);
> >
> >  	/*
> > @@ -257,6 +258,50 @@ 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_,
> > +	};
> > +
> > +	/*
> > +	 * The caller is responsible to limit the number of requested streams
> > +	 * to a number supported by the pipeline before calling this function.
> > +	 */
> > +	ASSERT(availableStreams.size() >= config_.size());
> > +
> > +	streams_.clear();
> > +	streams_.reserve(config_.size());
> > +
> > +	for (const StreamConfiguration &cfg : config_) {
> > +		const PixelFormatInfo &info =
> > +			PixelFormatInfo::info(cfg.pixelFormat);
> > +		const IPU3Stream *stream;
> > +
> > +		if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW)
> > +			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();
> 
> I understand this was here already, but it's worth pointing it out
> anyway: doesn't this allow an application to require 3 YUV streams ?
> Once we have assigned vfStream and outStream to the first two, the
> last one will be assigned to rawStream, which should not happen.

Agreed. It should be fixed on top though, as it's a change in behaviour.

> > +
> > +		streams_.push_back(stream);
> > +		availableStreams.erase(stream);
> > +	}
> > +}
> > +
> >  void IPU3CameraConfiguration::adjustStream(StreamConfiguration &cfg, bool scale)
> >  {
> >  	/* The only pixel format the driver supports is NV12. */
> > @@ -343,41 +388,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 PixelFormatInfo &info = PixelFormatInfo::info(pixelFormat);
> > -		const Size size = cfg.size;
> > -		const IPU3Stream *stream;
> > -
> > -		if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW)
> > -			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 =
> > @@ -394,15 +412,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);
> 
> The rest looks good.
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> 
> >  	}
> >
> >  	return status;

-- 
Regards,

Laurent Pinchart