[libcamera-devel] [PATCH 06/15] libcamera: ipu3: Rework stream validation

[Niklas Söderlund]

Hi Jacopo,

Thanks for your work.

On 2020-07-01 14:30:27 +0200, Jacopo Mondi wrote:
> With the goal of moving stream assignement to configuration from
> validate() to configure(), rework the validate() function to not
> inspect the stream assigned to a configuration to identify it.
> 
> Re-work the validation logic to follow this simpler flow:
> - if a stream is a raw stream use the sensor configuration
> - if a stream is a processed one, make sure it respects the ImgU
>   output size and alignment constraints.
> 
> Remove the 'adjustStream()' function as it depends on stream assignment
> and was built on the assumption the main output should always work at
> the maximum available resolution, and it addressed the case where no
> width or height where supplied for a viewfinder stream, which should
> only be validated against the ImgU output constraints, not defaulted to a
> sane value.
> 
> Retain the call to assignStreams() in validate() for the moment in order
> not to break capture operations, but while at it clean up the code a bit
> by removing a rogue empty line and make a conditional check fit on a
> single line.
> 
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>

Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>

> ---
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 85 ++++++++++------------------
>  1 file changed, 31 insertions(+), 54 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index ed2360347fb4..daa6d71dae72 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -68,7 +68,6 @@ public:
>  
>  private:
>  	void assignStreams();
> -	void adjustStream(StreamConfiguration &cfg, bool scale);
>  
>  	/*
>  	 * The IPU3CameraData instance is guaranteed to be valid as long as the
> @@ -178,52 +177,6 @@ void IPU3CameraConfiguration::assignStreams()
>  	}
>  }
>  
> -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.width || !cfg.size.height) {
> -			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;
> @@ -244,7 +197,6 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()
>  	 * resolution is large enough, pick the largest one.
>  	 */
>  	Size size = {};
> -
>  	for (const StreamConfiguration &cfg : config_) {
>  		if (cfg.size.width > size.width)
>  			size.width = cfg.size.width;
> @@ -264,20 +216,45 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()
>  	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_) {
> +		if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW) {
>  			cfg.size = cio2Configuration_.size;
>  			cfg.pixelFormat = cio2Configuration_.pixelFormat;
>  			cfg.bufferCount = cio2Configuration_.bufferCount;
>  		} else {
> -			bool scale = stream == &data_->vfStream_;
> -			adjustStream(config_[i], scale);
> +			/*
> +			 * Clamp the size to match the ImgU alignment
> +			 * constraints.
> +			 */
> +			size.width = std::min(cfg.size.width,
> +					      IPU3_OUTPUT_MAX_WIDTH);
> +			size.height = std::min(cfg.size.height,
> +					       IPU3_OUTPUT_MAX_HEIGHT);
> +			size.width = std::max(cfg.size.width,
> +					      minIPU3OutputSize.width);
> +			size.height = std::max(cfg.size.height,
> +					       minIPU3OutputSize.height);
> +			if (cfg.size.width % 8 || cfg.size.height % 4) {
> +				cfg.size.width &= ~7;
> +				cfg.size.height &= ~3;
> +			}
> +			cfg.pixelFormat = formats::NV12;
>  			cfg.bufferCount = IPU3_BUFFER_COUNT;
> +
> +			/*
> +			 * \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.
> +			 */
>  		}
>  
> -		if (cfg.pixelFormat != oldCfg.pixelFormat ||
> -		    cfg.size != oldCfg.size) {
> +		if (cfg.pixelFormat != oldCfg.pixelFormat || cfg.size != oldCfg.size) {
>  			LOG(IPU3, Debug)
>  				<< "Stream " << i << " configuration adjusted to "
>  				<< cfg.toString();
> -- 
> 2.27.0
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

-- 
Regards,
Niklas Söderlund