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

3893 diff mbox series

Picking which stream that is 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 where added, break it out into a separate
function.

Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
---
 src/libcamera/pipeline/ipu3/ipu3.cpp | 87 +++++++++++++++++-----------
 1 file changed, 53 insertions(+), 34 deletions(-)

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@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@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

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