[libcamera-devel] [PATCH v2 10/20] libcamera: ipu3: Adjust and assign streams in validate()

[Niklas Söderlund]

Hi Jacopo,

Thanks for your work, I like this change.

On 2020-07-09 10:41:18 +0200, Jacopo Mondi wrote:
> Remove the adjustStream() and assignStream() methods, and perform stream
> adjustment and assignment while iterating the StreamConfiguration
> items.
> 
> The adjustStream() implementation had some arbitrary assumption, like
> the main output having to be as large as the sensor resolution, and did
> not take into account the different alignment requirements between the
> main output and the viewfinder output.
> 
> The assignStream() implementation also assume only full-size streams
> can be produced by the main output, and having it as a separate function
> prevents adjusting streams according to which output they are assigned.
> 
> Blend the two implementation in a single loop and perform the required
> stream adjustment and assignment in one go.
> 
> As streams are now assigned at validate() time, remove the same
> operation from the configure() function.
> 
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 204 ++++++++++++---------------
>  1 file changed, 91 insertions(+), 113 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 9128e42d42ed..18f4a02cc270 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -69,9 +69,6 @@ public:
>  	const std::vector<const Stream *> &streams() { return streams_; }
>  
>  private:
> -	void assignStreams();
> -	void adjustStream(StreamConfiguration &cfg, bool scale);
> -
>  	/*
>  	 * The IPU3CameraData instance is guaranteed to be valid as long as the
>  	 * corresponding Camera instance is valid. In order to borrow a
> @@ -136,96 +133,6 @@ 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 Stream *> 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 Stream *stream;
> -
> -		if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW)
> -			stream = &data_->rawStream_;
> -		else if (cfg.size == cio2Configuration_.size)
> -			stream = &data_->outStream_;
> -		else
> -			stream = &data_->vfStream_;
> -
> -		if (availableStreams.find(stream) == availableStreams.end())
> -			stream = *availableStreams.begin();
> -
> -		streams_.push_back(stream);
> -		availableStreams.erase(stream);
> -	}
> -}
> -
> -void IPU3CameraConfiguration::adjustStream(StreamConfiguration &cfg, bool scale)
> -{
> -	/* The only pixel format the driver supports is NV12. */
> -	cfg.pixelFormat = formats::NV12;
> -
> -	if (scale) {
> -		/*
> -		 * Provide a suitable default that matches the sensor aspect
> -		 * ratio.
> -		 */
> -		if (cfg.size.isNull()) {
> -			cfg.size.width = 1280;
> -			cfg.size.height = 1280 * cio2Configuration_.size.height
> -					/ cio2Configuration_.size.width;
> -		}
> -
> -		/*
> -		 * \todo: Clamp the size to the hardware bounds when we will
> -		 * figure them out.
> -		 *
> -		 * \todo: Handle the scaler (BDS) restrictions. The BDS can
> -		 * only scale with the same factor in both directions, and the
> -		 * scaling factor is limited to a multiple of 1/32. At the
> -		 * moment the ImgU driver hides these constraints by applying
> -		 * additional cropping, this should be fixed on the driver
> -		 * side, and cropping should be exposed to us.
> -		 */
> -	} else {
> -		/*
> -		 * \todo: Properly support cropping when the ImgU driver
> -		 * interface will be cleaned up.
> -		 */
> -		cfg.size = cio2Configuration_.size;
> -	}
> -
> -	/*
> -	 * Clamp the size to match the ImgU alignment constraints. The width
> -	 * shall be a multiple of 8 pixels and the height a multiple of 4
> -	 * pixels.
> -	 */
> -	if (cfg.size.width % 8 || cfg.size.height % 4) {
> -		cfg.size.width &= ~7;
> -		cfg.size.height &= ~3;
> -	}
> -}
> -
>  CameraConfiguration::Status IPU3CameraConfiguration::validate()
>  {
>  	Status status = Valid;
> @@ -248,17 +155,26 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()
>  	 */
>  	unsigned int rawCount = 0;
>  	unsigned int outCount = 0;
> +	Size yuvSize;
>  	Size size;
>  
>  	for (const StreamConfiguration &cfg : config_) {
>  		const PixelFormatInfo &info =
>  			PixelFormatInfo::info(cfg.pixelFormat);
>  
> -		if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW)
> +		if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW) {
>  			rawCount++;
> -		else
> +		} else {
>  			outCount++;
>  
> +			/*
> +			 * Collect the maximum processed size to later assign
> +			 * streams to configurations.
> +			 */
> +			if (cfg.size > yuvSize)
> +				yuvSize = cfg.size;
> +		}
> +
>  		if (cfg.size.width > size.width)
>  			size.width = cfg.size.width;
>  		if (cfg.size.height > size.height)
> @@ -277,24 +193,94 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()
>  	if (!cio2Configuration_.pixelFormat.isValid())
>  		return Invalid;
>  
> -	/* Assign streams to each configuration entry. */
> -	assignStreams();
> -
> -	/* Verify and adjust configuration if needed. */
> +	/*
> +	 * Adjust the configurations if needed and assign streams while
> +	 * iterating them.
> +	 */
> +	bool mainOutputAvailable = true;
>  	for (unsigned int i = 0; i < config_.size(); ++i) {
>  		StreamConfiguration &cfg = config_[i];
>  		const StreamConfiguration oldCfg = cfg;
> -		const Stream *stream = streams_[i];
> +		const PixelFormatInfo &info =
> +			PixelFormatInfo::info(cfg.pixelFormat);
>  
> -		if (stream == &data_->rawStream_) {
> +		LOG(IPU3, Debug) << "Validating configuration: " << cfg.toString();
> +
> +		/* Initialize the RAW stream with the CIO2 configuration. */
> +		if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW) {
>  			cfg.size = cio2Configuration_.size;
>  			cfg.pixelFormat = cio2Configuration_.pixelFormat;
>  			cfg.bufferCount = cio2Configuration_.bufferCount;
> +			cfg.setStream(const_cast<Stream *>(&data_->rawStream_));
> +
> +			LOG(IPU3, Debug) << "Assigned " << cfg.toString()
> +					 << " to the raw stream";
> +			continue;
> +		}
> +
> +		/*
> +		 * Assign and configure the main and viewfinder outputs.
> +		 */
> +
> +		/* Clamp the size to match the ImgU size limits. */
> +		cfg.size.width = utils::clamp(cfg.size.width,
> +					      minIPU3OutputSize.width,
> +					      IPU3_OUTPUT_MAX_WIDTH);
> +		cfg.size.height = utils::clamp(cfg.size.height,
> +					       minIPU3OutputSize.height,
> +					       IPU3_OUTPUT_MAX_HEIGHT);
> +
> +		/*
> +		 * All the ImgU output stream should be smaller than the ImgU
> +		 * input frame, to give space to the IF and BDS to crop the
> +		 * image.
> +		 *
> +		 * \todo Give the BDS rectangle at least 32 pixels from the
> +		 * input frame size. This is an assumption deduced
> +		 * from inspecting the pipe configuration tool output
> +		 * and the platform configuration files shipped for the
> +		 * Soraka device by ChromeOS.
> +		 */
> +		if (cfg.size.width >= (cio2Configuration_.size.width - 31))
> +			cfg.size.width = cio2Configuration_.size.width - 32;
> +
> +		if (cfg.size.height >= (cio2Configuration_.size.height - 31))
> +			cfg.size.height = cio2Configuration_.size.height - 32;

Could you not clamp cfg.size.width within minIPU3OutputSize.width and 
cio2Configuration_.size.width - 32 instead of minIPU3OutputSize.width 
and IPU3_OUTPUT_MAX_WIDTH?

Same for hight of course.

> +
> +		/*
> +		 * Adjust to match the main output or the viewfinder
> +		 * output constraints and assign streams.
> +		 *
> +		 * Use the main output stream in case only one stream is
> +		 * requested or if the current configuration is the one with
> +		 * the maximum RGB/YUV output size.
> +		 */

This comment is 1 (see below).

> +		cfg.size.width &= ~IPU3_OUTPUT_WIDTH_ALIGN;
> +		cfg.size.height &= ~IPU3_OUTPUT_HEIGHT_ALIGN;
> +		cfg.bufferCount = IPU3_BUFFER_COUNT;
> +		cfg.pixelFormat = formats::NV12;
> +
> +		/*
> +		 * Use a const_cast<> here instead of storing a mutable stream
> +		 * pointer in the configuration to let the compiler catch
> +		 * unwanted modifications of camera data in the configuration
> +		 * validate() implementation.
> +		 */

I would drop this comment and move comment [1] here. There is no need to 
document const_cast<>() for the setStream() call for the RAW stream what 
is different here?

> +		Stream *stream;
> +		if (mainOutputAvailable &&
> +		    (oldCfg.size == yuvSize || outCount == 1)) {
> +			stream = const_cast<Stream *>(&data_->outStream_);
> +			mainOutputAvailable = false;
> +
> +			LOG(IPU3, Debug) << "Assigned " << cfg.toString()
> +					 << " to the main output";
>  		} else {
> -			bool scale = stream == &data_->vfStream_;
> -			adjustStream(config_[i], scale);
> -			cfg.bufferCount = IPU3_BUFFER_COUNT;
> +			stream = const_cast<Stream *>(&data_->vfStream_);
> +
> +			LOG(IPU3, Debug) << "Assigned " << cfg.toString()
> +					 << " to the viewfinder output";
>  		}
> +		cfg.setStream(stream);
>  
>  		if (cfg.pixelFormat != oldCfg.pixelFormat ||
>  		    cfg.size != oldCfg.size) {
> @@ -472,16 +458,8 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)
>  	bool vfActive = false;
>  
>  	for (unsigned int i = 0; i < config->size(); ++i) {
> -		/*
> -		 * Use a const_cast<> here instead of storing a mutable stream
> -		 * pointer in the configuration to let the compiler catch
> -		 * unwanted modifications of camera data in the configuration
> -		 * validate() implementation.
> -		 */
> -		Stream *stream = const_cast<Stream *>(config->streams()[i]);
>  		StreamConfiguration &cfg = (*config)[i];
> -
> -		cfg.setStream(stream);
> +		Stream *stream = cfg.stream();
>  
>  		if (stream == outStream) {
>  			ret = imgu->configureOutput(cfg, &outputFormat);
> -- 
> 2.27.0
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

-- 
Regards,
Niklas Söderlund