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

[Jacopo Mondi]

Hi Niklas,

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

> 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()
> +{
> +	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 ?

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;
        }

        /* 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;
> +		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 ?

Thanks
  j

>
>  		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;
> --
> 2.26.2
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel