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

[Laurent Pinchart]

Hello,

On Tue, Jun 02, 2020 at 11:53:58AM +0200, Jacopo Mondi wrote:
> On Tue, Jun 02, 2020 at 03:39:03AM +0200, Niklas Söderlund wrote:
> > Picking which stream that is most suitable for the requested
> 
> s/Picking/Selecting
> s/that is/is the/
> 
> > configuration is mixed with adjusting the requested format when
> > validating configurations. This is hard to read and got worse when
> > support for Bayer formats where added, break it out into a separate
>
> s/where/were or better, was

And s/into/to/

> > function.
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > ---
> >  src/libcamera/pipeline/ipu3/ipu3.cpp | 87 +++++++++++++++++-----------
> >  1 file changed, 53 insertions(+), 34 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index f7363244e1d2d0ff..0e7555c716b36749 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;
> >
> > +	int updateStreams();
> >  	void adjustStream(StreamConfiguration &cfg, bool scale);
> >
> >  	/*
> > @@ -256,6 +257,51 @@ IPU3CameraConfiguration::IPU3CameraConfiguration(Camera *camera,
> >  	data_ = data;
> >  }
> >
> > +int IPU3CameraConfiguration::updateStreams()

Should this be called assignStreams() ?

> > +{
> > +	std::set<const IPU3Stream *> availableStreams = {
> > +		&data_->outStream_,
> > +		&data_->vfStream_,
> > +		&data_->rawStream_,
> > +	};
> > +
> > +	/* Pick the stream most suitable for the requested configuration. */
> > +	std::vector<const IPU3Stream *> streams;
> > +	for (unsigned int i = 0; i < config_.size(); ++i) {
> 
> Can't you iterate on config_ ?
> 
> > +		const StreamConfiguration &cfg = config_[i];
> > +		const IPU3Stream *stream;
> > +
> > +		/*
> > +		 * Only the raw stream can support Bayer formats.
> 
> Fits in one line
> 
> > +		 */
> > +		if (cfg.pixelFormat.modifier() == IPU3_FORMAT_MOD_PACKED)
> > +			stream = &data_->rawStream_;
> Emply line ?
> 
> > +		/*
> > +		 * Output stream can't scale so can only be used if the size
> > +		 * matches the size of the sensor.
> > +		 *
> > +		 * NOTE: It can crop but is not supported.
> 
> \todo ?
> 
> And I would keep the original comment. We can crop, the issue is that
> the FOV will be reduce when asking for a lower resolution than the
> one of the frame input the IMGU if I'm not mistaken.
> 
> > +		 */
> > +		else if (cfg.size == sensorFormat_.size)
> > +			stream = &data_->outStream_;
> > +		/*
> > +		 * Pick the view finder stream last as it may scale.
> 
> fits in one line ?
> 
> > +		 */
> > +		else
> > +			stream = &data_->vfStream_;
> > +
> > +		if (availableStreams.find(stream) == availableStreams.end())
> > +			return -EINVAL;
> 
> Is this right ? Shouldn't we fall back to use the first available
> stream if any ?

At least that's what the code used to do, and I think it's best to keep
that behaviour in this patch. If a change of behaviour is desired, it
should be implemented in a separate patch.

I would make availableStreams a vector to support this, as otherwise
availableStreams.begin() will return a random stream (not truly random,
but std::set is sorted using value comparison, and values are pointers
here).

> I would re-structure the code as
> 
> for (config : config_) {
> 
>         if (raw) {
>                 it = availableStreams.find(rawStream);
>                 if (it == end()) {
>                         // Fail as we can't provide two raw streams.
>                 }
> 
>                 availagleStreams.erase(rawStream);
>                 continue;
>         }

This is also a change of behaviour that should be discussed in a
separate patch.

> 
>         /* Handle non-raw streams. */
>         if (size == sensorSize)
>                 stream = output;
>         else
>                 stream = viewfinder;
> 
>         if (availableStream.find(stream) == end())
>                 stream = begin()
> 
>          availableStreams.erase(stream);
> }
> 
> 
> }
> > +
> > +		streams.push_back(stream);
> > +		availableStreams.erase(stream);
> > +	}
> > +
> > +	streams_ = streams;
> 
> Why don't you fill streams_ directly ?
> 
> > +
> > +	return 0;
> > +}
> > +
> >  void IPU3CameraConfiguration::adjustStream(StreamConfiguration &cfg, bool scale)
> >  {
> >  	/* The only pixel format the driver supports is NV12. */
> > @@ -342,40 +388,16 @@ 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_,
> > -	};
> >
> 
> This results in two empty lines. Doesn't checkstyle report that ?
> 
> > -	streams_.clear();
> > -	streams_.reserve(config_.size());
> > +	/* Assign streams to each configuration entry. */
> > +	if (updateStreams())
> > +		return Invalid;
> >
> > +	/* Verify and adjust configuration if needed. */
> >  	for (unsigned int i = 0; i < config_.size(); ++i) {
> 
> Can you iterate on config_ ?
> 
> >  		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 org = cfg;

org isn't a very explicit name. origCfg or oldCfg would be better.

> > +		const IPU3Stream *stream = streams_[i];
> 
> Ah no, you need i here.
> However, associating on index seems a bit weak, and as we can't assign
> streams to stream configurations here, have you considered having
> updateStreams() return a map of configs to streams ?
> 
> >
> >  		if (stream->raw_) {
> >  			const auto &itFormat =
> > @@ -392,15 +414,12 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()
> >
> >  		cfg.bufferCount = IPU3_BUFFER_COUNT;
> >
> > -		if (cfg.pixelFormat != pixelFormat || cfg.size != size) {
> > +		if (cfg.pixelFormat != org.pixelFormat || cfg.size != org.size) {
> >  			LOG(IPU3, Debug)
> >  				<< "Stream " << i << " configuration adjusted to "
> >  				<< cfg.toString();
> >  			status = Adjusted;
> >  		}
> > -
> > -		streams_.push_back(stream);
> > -		availableStreams.erase(stream);
> >  	}
> >
> >  	return status;

-- 
Regards,

Laurent Pinchart