[libcamera-devel,v4,14/21] libcamera: ipu3: Fill stride and frameSize at config validation

8683 diff mbox series

Fill the stride and frameSize fields of the StreamConfiguration at
configuration validation time instead of at camera configuration time.
This allows applications to get the stride when trying a configuration
without modifying the active configuration of the camera.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>

---
Changes in v4:
- fix stride and frameSize calculation
- mention motivation in commit message

New in v3
---
 src/libcamera/pipeline/ipu3/ipu3.cpp | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

Hi Paul,

Thank you for the patch.

On Wed, Jul 08, 2020 at 10:44:10PM +0900, Paul Elder wrote:
> Fill the stride and frameSize fields of the StreamConfiguration at
> configuration validation time instead of at camera configuration time.
> This allows applications to get the stride when trying a configuration
> without modifying the active configuration of the camera.
> 
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> 
> ---
> Changes in v4:
> - fix stride and frameSize calculation
> - mention motivation in commit message
> 
> New in v3
> ---
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 23 +++++++++++++++--------
>  1 file changed, 15 insertions(+), 8 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 00559ce..4a2d924 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -278,6 +278,21 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()
>  				<< cfg.toString();
>  			status = Adjusted;
>  		}
> +
> +		const PixelFormatInfo &info = PixelFormatInfo::info(cfg.pixelFormat);
> +		bool packedRaw = info.packed &&
> +				 info.colourEncoding == PixelFormatInfo::ColourEncodingRAW;
> +
> +		if (packedRaw) {

I think you can write

		if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW) {

as we can't support other raw formats than the IPU3 packed formats.

> +			cfg.stride = info.stride(cfg.size.width, 0, 64);
> +			cfg.frameSize = cfg.stride * cfg.size.height;
> +		} else {
> +			cfg.stride = info.stride(cfg.size.width, 0);
> +			std::array<unsigned int, 3> strides;
> +			for (unsigned int j = 0; j < 3; j++)
> +				strides[i] = info.stride(cfg.size.width, i, packedRaw ? 64 : 0);

packedRaw is always false here.

> +			cfg.frameSize = info.frameSize(cfg.size, strides);

I think you can thus just write

			cfg.stride = info.stride(cfg.size.width, 0);
			cfg.frameSize = info.frameSize(cfg.size);

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +		}
>  	}
>  
>  	return status;
> @@ -495,21 +510,13 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)
>  			if (ret)
>  				return ret;
>  
> -			cfg.stride = outputFormat.planes[0].bpl;
>  			outActive = true;
>  		} else if (stream == vfStream) {
>  			ret = imgu->configureViewfinder(cfg, &outputFormat);
>  			if (ret)
>  				return ret;
>  
> -			cfg.stride = outputFormat.planes[0].bpl;
>  			vfActive = true;
> -		} else {
> -			/*
> -			 * The RAW stream is configured as part of the CIO2 and
> -			 * no configuration is needed for the ImgU.
> -			 */
> -			cfg.stride = cio2Format.planes[0].bpl;
>  		}
>  	}
>

Hi Paul,

On Wed, Jul 08, 2020 at 10:44:10PM +0900, Paul Elder wrote:
> Fill the stride and frameSize fields of the StreamConfiguration at
> configuration validation time instead of at camera configuration time.
> This allows applications to get the stride when trying a configuration
> without modifying the active configuration of the camera.
>
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
>
> ---
> Changes in v4:
> - fix stride and frameSize calculation
> - mention motivation in commit message
>
> New in v3
> ---
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 23 +++++++++++++++--------
>  1 file changed, 15 insertions(+), 8 deletions(-)
>
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 00559ce..4a2d924 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -278,6 +278,21 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()
>  				<< cfg.toString();
>  			status = Adjusted;
>  		}
> +
> +		const PixelFormatInfo &info = PixelFormatInfo::info(cfg.pixelFormat);
> +		bool packedRaw = info.packed &&
> +				 info.colourEncoding == PixelFormatInfo::ColourEncodingRAW;
> +
> +		if (packedRaw) {
> +			cfg.stride = info.stride(cfg.size.width, 0, 64);
> +			cfg.frameSize = cfg.stride * cfg.size.height;
> +		} else {
> +			cfg.stride = info.stride(cfg.size.width, 0);
> +			std::array<unsigned int, 3> strides;
> +			for (unsigned int j = 0; j < 3; j++)
> +				strides[i] = info.stride(cfg.size.width, i, packedRaw ? 64 : 0);

Now that I see how PixelFormatInfo::frameSize() is used, I wonder if
align shouldn't be defaulted there and strides always calculated there, if
other pipeline handles too have to calculate it with the sole purpose
of providing it to that function.

> +			cfg.frameSize = info.frameSize(cfg.size, strides);
> +		}
>  	}
>
>  	return status;
> @@ -495,21 +510,13 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)
>  			if (ret)
>  				return ret;
>
> -			cfg.stride = outputFormat.planes[0].bpl;
>  			outActive = true;
>  		} else if (stream == vfStream) {
>  			ret = imgu->configureViewfinder(cfg, &outputFormat);
>  			if (ret)
>  				return ret;
>
> -			cfg.stride = outputFormat.planes[0].bpl;
>  			vfActive = true;
> -		} else {
> -			/*
> -			 * The RAW stream is configured as part of the CIO2 and
> -			 * no configuration is needed for the ImgU.
> -			 */
> -			cfg.stride = cio2Format.planes[0].bpl;
>  		}
>  	}
>
> --
> 2.27.0
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel