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

4130 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>
Reviewed-by: Laurent Pinchart <laurent.pinchart@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(-)

Hi Niklas,

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@ragnatech.se>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@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.

> +
> +		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@jmondi.org>

Thanks
  j
>  	}
>
>  	return status;
> --
> 2.27.0
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

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@ragnatech.se>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@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@jmondi.org>
> 
> >  	}
> >
> >  	return status;