[libcamera-devel,v2,17/20] libcamera: ipu3: imgu: Calculate ImgU pipe configuration

8708 diff mbox series

Instrument the ImgU component to dynamically calculate the image
manipulation pipeline intermediate sizes.

To correctly configure the ImgU it is necessary to program the IF, BDS
and GDC sizes, which are currently fixed to the input frame size.

The procedure used to calculate the intermediate sizes has been ported
from the pipe_config.py python script, available at:
https://github.com/intel/intel-ipu3-pipecfg
at revision:
61e83f2f7606 ("Add more information into README")

Define two structures (ImgUDevice::Pipe and ImgUdevice::PipeConfig)
to allow the pipeline handler to supply and retrieve configuration
parameters from the ImgU unit.

Finally, add a new operation to the ImgUDdevice that calculates
the pipe configuration parameters based on the requested input and
output sizes.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 src/libcamera/pipeline/ipu3/imgu.cpp | 365 +++++++++++++++++++++++++++
 src/libcamera/pipeline/ipu3/imgu.h   |  20 ++
 2 files changed, 385 insertions(+)

Hi Jacopo,

Thanks for this Herculean effort.

On 2020-07-09 10:41:25 +0200, Jacopo Mondi wrote:
> Instrument the ImgU component to dynamically calculate the image
> manipulation pipeline intermediate sizes.
> 
> To correctly configure the ImgU it is necessary to program the IF, BDS
> and GDC sizes, which are currently fixed to the input frame size.
> 
> The procedure used to calculate the intermediate sizes has been ported
> from the pipe_config.py python script, available at:
> https://github.com/intel/intel-ipu3-pipecfg
> at revision:
> 61e83f2f7606 ("Add more information into README")
> 
> Define two structures (ImgUDevice::Pipe and ImgUdevice::PipeConfig)
> to allow the pipeline handler to supply and retrieve configuration
> parameters from the ImgU unit.
> 
> Finally, add a new operation to the ImgUDdevice that calculates
> the pipe configuration parameters based on the requested input and
> output sizes.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>

I will not review all the calculations from the python script to C++ and 
will therefore trust you on those :-) One small typo in a comment bellow 
other then that.

Acked-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>

> ---
>  src/libcamera/pipeline/ipu3/imgu.cpp | 365 +++++++++++++++++++++++++++
>  src/libcamera/pipeline/ipu3/imgu.h   |  20 ++
>  2 files changed, 385 insertions(+)
> 
> diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp
> index d7f4173d3607..d1addb7d84d1 100644
> --- a/src/libcamera/pipeline/ipu3/imgu.cpp
> +++ b/src/libcamera/pipeline/ipu3/imgu.cpp
> @@ -7,6 +7,9 @@
>  
>  #include "imgu.h"
>  
> +#include <limits>
> +#include <math.h>
> +
>  #include <linux/media-bus-format.h>
>  
>  #include <libcamera/formats.h>
> @@ -19,6 +22,311 @@ namespace libcamera {
>  
>  LOG_DECLARE_CATEGORY(IPU3)
>  
> +namespace {
> +
> +static constexpr unsigned int FILTER_H = 4;
> +
> +static constexpr unsigned int IF_ALIGN_W = 2;
> +static constexpr unsigned int IF_ALIGN_H = 4;
> +
> +static constexpr unsigned int BDS_ALIGN_W = 2;
> +static constexpr unsigned int BDS_ALIGN_H = 4;
> +
> +static constexpr unsigned int IF_CROP_MAX_W = 40;
> +static constexpr unsigned int IF_CROP_MAX_H = 540;
> +
> +static constexpr float BDS_SF_MAX = 2.5;
> +static constexpr float BDS_SF_MIN = 1.0;
> +static constexpr float BDS_SF_STEP = 0.03125;
> +
> +/* BSD scaling factors: min=1, max=2.5, step=1/32 */
> +const std::vector<float> bdsScalingFactors = {
> +	1, 1.03125, 1.0625, 1.09375, 1.125, 1.15625, 1.1875, 1.21875, 1.25,
> +	1.28125, 1.3125, 1.34375, 1.375, 1.40625, 1.4375, 1.46875, 1.5, 1.53125,
> +	1.5625, 1.59375, 1.625, 1.65625, 1.6875, 1.71875, 1.75, 1.78125, 1.8125,
> +	1.84375, 1.875, 1.90625, 1.9375, 1.96875, 2, 2.03125, 2.0625, 2.09375,
> +	2.125, 2.15625, 2.1875, 2.21875, 2.25, 2.28125, 2.3125, 2.34375, 2.375,
> +	2.40625, 2.4375, 2.46875, 2.5
> +};
> +
> +/* GDC scaling factors: min=1, max=16, step=1/4 */
> +const std::vector<float> gdcScalingFactors = {
> +	1, 1.25, 1.5, 1.75, 2, 2.25, 2.5, 2.75, 3, 3.25, 3.5, 3.75, 4, 4.25,
> +	4.5, 4.75, 5, 5.25, 5.5, 5.75, 6, 6.25, 6.5, 6.75, 7, 7.25, 7.5, 7.75,
> +	8, 8.25, 8.5, 8.75, 9, 9.25, 9.5, 9.75, 10, 10.25, 10.5, 10.75, 11,
> +	11.25, 11.5, 11.75, 12, 12.25, 12.5, 12.75, 13, 13.25, 13.5, 13.75, 14,
> +	14.25, 14.5, 14.75, 15, 15.25, 15.5, 15.75, 16,
> +};
> +
> +std::vector<ImgUDevice::PipeConfig> pipeConfigs;
> +
> +struct FOV {
> +	float w;
> +	float h;
> +
> +	bool isLarger(const FOV &other)
> +	{
> +		if (w > other.w)
> +			return true;
> +		if (w == other.w && h > other.h)
> +			return true;
> +		return false;
> +	}
> +};
> +
> +/* Approximate a scaling factor sf to the closest one available in a range. */
> +float findScaleFactor(float sf, const std::vector<float> &range,
> +		      bool roundDown = false)
> +{
> +	if (sf <= range[0])
> +		return range[0];
> +	if (sf >= range[range.size() - 1])
> +		return range[range.size() - 1];
> +
> +
> +	float bestDiff = std::numeric_limits<float>::max();
> +	unsigned int index = 0;
> +	for (unsigned int i = 0; i < range.size(); ++i) {
> +		float diff = std::abs(sf - range[i]);
> +		if (diff < bestDiff) {
> +			bestDiff = diff;
> +			index = i;
> +		}
> +	}
> +
> +	if (roundDown && index > 0 && sf < range[index])
> +		index--;
> +
> +	return range[index];
> +}
> +
> +bool isSameRatio(const Size &in, const Size &out)
> +{
> +	float inRatio = static_cast<float>(in.width) /
> +			static_cast<float>(in.height);
> +	float outRatio = static_cast<float>(out.width) /
> +			 static_cast<float>(out.height);
> +
> +	if (std::abs(inRatio - outRatio) > 0.1)
> +		return false;
> +
> +	return true;
> +}
> +
> +unsigned int alignIncrease(unsigned int len, unsigned int align)
> +{
> +	if (len % align)
> +		return (std::floor(static_cast<double>(len) /
> +				   static_cast<double>(align)) + 1) * align;
> +	return len;
> +}
> +
> +void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc,
> +			unsigned int bdsWidth, float bdsSF)
> +{
> +	unsigned int minIFHeight = iif.height - IF_CROP_MAX_H;
> +	unsigned int minBDSHeight = gdc.height + FILTER_H * 2;
> +	float bdsHeight;
> +
> +	if (!isSameRatio(pipe->input, gdc)) {
> +		bool found = false;
> +		float estIFHeight = static_cast<float>(iif.width) *
> +				    static_cast<float>(gdc.height) /
> +				    static_cast<float>(gdc.width);
> +		estIFHeight = utils::clamp<float>(estIFHeight,
> +						  static_cast<float>(minIFHeight),
> +						  static_cast<float>(iif.height));
> +		float ifHeight =
> +			static_cast<float>(alignIncrease(static_cast<unsigned int>(estIFHeight),
> +							 IF_ALIGN_H));
> +		while (ifHeight >= minIFHeight && (ifHeight / bdsSF >= minBDSHeight)) {
> +			bdsHeight = ifHeight / bdsSF;
> +			if (std::fmod(bdsHeight, 1.0) == 0) {
> +				unsigned int bdsIntHeight =
> +					static_cast<unsigned int>(bdsHeight);
> +				if (!(bdsIntHeight % BDS_ALIGN_H)) {
> +					found = true;
> +					break;
> +				}
> +			}
> +
> +			ifHeight -= IF_ALIGN_H;
> +		}
> +
> +		ifHeight = static_cast<float>(
> +			alignIncrease(static_cast<unsigned int>(estIFHeight),
> +				      IF_ALIGN_H));
> +
> +		while (ifHeight <= iif.height && ifHeight / bdsSF >= minBDSHeight) {
> +			bdsHeight = ifHeight / bdsSF;
> +			if (std::fmod(bdsHeight, 1.0) == 0) {
> +				unsigned int bdsIntHeight =
> +					static_cast<unsigned int>(bdsHeight);
> +				if (!(bdsIntHeight % BDS_ALIGN_H)) {
> +					found = true;
> +					break;
> +				}
> +			}
> +
> +			ifHeight += IF_ALIGN_H;
> +		}
> +
> +		if (found) {
> +			unsigned int ifIntHeight = static_cast<unsigned int>(ifHeight);
> +			unsigned int bdsIntHeight = static_cast<unsigned int>(bdsHeight);
> +			pipeConfigs.push_back({ bdsSF, { iif.width, ifIntHeight },
> +						{ bdsWidth, bdsIntHeight }, gdc });
> +			return;
> +		}
> +	} else {
> +		float ifHeight = static_cast<float>(alignIncrease(iif.height, IF_ALIGN_H));
> +		while (ifHeight > minIFHeight && (ifHeight / bdsSF > minBDSHeight)) {
> +			bdsHeight = ifHeight / bdsSF;
> +			if (std::fmod(ifHeight, 1.0) == 0 && std::fmod(bdsHeight, 1.0) == 0) {
> +				unsigned int ifIntHeight = static_cast<unsigned int>(ifHeight);
> +				unsigned int bdsIntHeight = static_cast<unsigned int>(bdsHeight);
> +
> +				if (!(ifIntHeight % IF_ALIGN_H) &&
> +				    !(bdsIntHeight % BDS_ALIGN_H)) {
> +					pipeConfigs.push_back({ bdsSF, { iif.width, ifIntHeight },
> +								{ bdsWidth, bdsIntHeight }, gdc });
> +				}
> +			}
> +
> +			ifHeight -= IF_ALIGN_H;
> +		}
> +	}
> +}
> +
> +void calculateBDS(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc,
> +		  float bdsSF)
> +{
> +	unsigned int minBDSWidth = gdc.width + FILTER_H * 2;
> +
> +	float sf = bdsSF;
> +	while (sf <= BDS_SF_MAX && sf >= BDS_SF_MIN) {
> +		float bdsWidth = static_cast<float>(iif.width) / sf;
> +
> +		if (std::fmod(bdsWidth, 1.0) == 0) {
> +			unsigned int bdsIntWidth = static_cast<unsigned int>(bdsWidth);
> +			if (!(bdsIntWidth % BDS_ALIGN_W) && bdsWidth >= minBDSWidth)
> +				calculateBDSHeight(pipe, iif, gdc, bdsIntWidth, sf);
> +		}
> +
> +		sf += BDS_SF_STEP;
> +	}
> +
> +	sf = bdsSF;
> +	while (sf <= BDS_SF_MAX && sf >= BDS_SF_MIN) {
> +		float bdsWidth = static_cast<float>(iif.width) / sf;
> +
> +		if (std::fmod(bdsWidth, 1.0) == 0) {
> +			unsigned int bdsIntWidth = static_cast<unsigned int>(bdsWidth);
> +			if (!(bdsIntWidth % BDS_ALIGN_W) && bdsWidth >= minBDSWidth)
> +				calculateBDSHeight(pipe, iif, gdc, bdsIntWidth, sf);
> +		}
> +
> +		sf -= BDS_SF_STEP;
> +	}
> +}
> +
> +Size calculateGDC(ImgUDevice::Pipe *pipe)
> +{
> +	const Size &in = pipe->input;
> +	const Size &main = pipe->output;
> +	const Size &vf = pipe->viewfinder;
> +	Size gdc;
> +
> +	if (!vf.isNull()) {
> +		gdc.width = main.width;
> +
> +		float ratio = static_cast<float>(main.width) *
> +			      static_cast<float>(vf.height) /
> +			      static_cast<float>(vf.width);
> +		gdc.height = std::max(static_cast<float>(main.height), ratio);
> +
> +		return gdc;
> +	}
> +
> +	if (!isSameRatio(in, main)) {
> +		gdc = main;
> +		return gdc;
> +	}
> +
> +	float totalSF = static_cast<float>(in.width) /
> +			static_cast<float>(main.width);
> +	float bdsSF = totalSF > 2 ? 2 : 1;
> +	float yuvSF = totalSF / bdsSF;
> +	float sf = findScaleFactor(yuvSF, gdcScalingFactors);
> +
> +	gdc.width = static_cast<float>(main.width) * sf;
> +	gdc.height = static_cast<float>(main.height) * sf;
> +
> +	return gdc;
> +}
> +
> +FOV calcFOV(const Size &in, const ImgUDevice::PipeConfig &pipe)
> +{
> +	FOV fov{};
> +
> +	float inW = static_cast<float>(in.width);
> +	float inH = static_cast<float>(in.height);
> +	float ifCropW = static_cast<float>(in.width) - static_cast<float>(pipe.iif.width);
> +	float ifCropH = static_cast<float>(in.height) - static_cast<float>(pipe.iif.height);
> +	float gdcCropW = static_cast<float>(pipe.bds.width - pipe.gdc.width) * pipe.bds_sf;
> +	float gdcCropH = static_cast<float>(pipe.bds.height - pipe.gdc.height) * pipe.bds_sf;
> +	fov.w = (inW - (ifCropW + gdcCropW)) / inW;
> +	fov.h = (inH - (ifCropH + gdcCropH)) / inH;
> +
> +	return fov;
> +}
> +
> +} /* namespace */
> +
> +/**
> + * \struct PipeConfig
> + * \brief The ImgU pipe configuration parameters
> + *
> + * The ImgU image pipeline is composed of several hardware blocks that crop
> + * and scale the input image to obtain the desired output sizes. The
> + * scaling/cropping operations of those components is configured though the
> + * V4L2 selection API and the V4L2 subdev API applied to the ImgU media entity.
> + *
> + * The configurable components in the pipeline are:
> + * - IF: image feeder
> + * - BDS: bayer downscaler
> + * = GDC: geometric distorsion correction

s/=/-/

> + *
> + * The IF crop rectangle is controlled by the V4L2_SEL_TGT_CROP selection target
> + * applied to the ImgU media entity sink pad number 0. The BDS scaler is
> + * controlled by the V4L2_SEL_TGT_COMPOSE target on the same pad, while the GDC
> + * output size is configured with the VIDIOC_SUBDEV_S_FMT IOCTL, again on pad
> + * number 0.
> + *
> + * The PipeConfig structure collects the sizes of each of those components
> + * plus the BDS scaling factor used to calculate the field of view
> + * of the final images.
> + */
> +
> +/**
> + * \struct Pipe
> + * \brief Describe the ImgU requested configuration
> + *
> + * The ImgU unit processes images through several components, which have
> + * to be properly configured inspecting the input image size and the desired
> + * output sizes. This structure collects the ImgU input configuration and the
> + * requested main output and viewfinder configurations.
> + *
> + * \var Pipe::input
> + * \brief The input image size
> + *
> + * \var Pipe::output
> + * \brief The requested main output size
> + *
> + * \var Pipe::viewfinder
> + * \brief The requested viewfinder size
> + */
> +
>  /**
>   * \brief Initialize components of the ImgU instance
>   * \param[in] mediaDevice The ImgU instance media device
> @@ -74,6 +382,63 @@ int ImgUDevice::init(MediaDevice *media, unsigned int index)
>  	return 0;
>  }
>  
> +/**
> + * \brief Calculate the ImgU pipe configuration parameters
> + * \param[in] pipe The requested ImgU configuration
> + * \return An ImgUDevice::PipeConfig instance on success, an empty configuration
> + * otherwise
> + */
> +ImgUDevice::PipeConfig ImgUDevice::calculatePipeConfig(Pipe *pipe)
> +{
> +	pipeConfigs.clear();
> +
> +	LOG(IPU3, Debug) << "Calculating pipe configuration for: ";
> +	LOG(IPU3, Debug) << "input: " << pipe->input.toString();
> +	LOG(IPU3, Debug) << "main: " << pipe->output.toString();
> +	LOG(IPU3, Debug) << "vf: " << pipe->viewfinder.toString();
> +
> +	const Size &in = pipe->input;
> +	Size gdc = calculateGDC(pipe);
> +
> +	unsigned int ifWidth = alignIncrease(in.width, IF_ALIGN_W);
> +	unsigned int ifHeight = in.height;
> +	unsigned int minIfWidth = in.width - IF_CROP_MAX_W;
> +	float bdsSF = static_cast<float>(in.width) /
> +		      static_cast<float>(gdc.width);
> +	float sf = findScaleFactor(bdsSF, bdsScalingFactors, true);
> +	while (ifWidth >= minIfWidth) {
> +		Size iif{ ifWidth, ifHeight };
> +		calculateBDS(pipe, iif, gdc, sf);
> +
> +		ifWidth -= IF_ALIGN_W;
> +	}
> +
> +	if (pipeConfigs.size() == 0) {
> +		LOG(IPU3, Error) << "Failed to calculate pipe configuration";
> +		return {};
> +	}
> +
> +	FOV bestFov = calcFOV(pipe->input, pipeConfigs[0]);
> +	unsigned int bestIndex = 0;
> +	unsigned int p = 0;
> +	for (auto pipeConfig : pipeConfigs) {
> +		FOV fov = calcFOV(pipe->input, pipeConfig);
> +		if (fov.isLarger(bestFov)) {
> +			bestFov = fov;
> +			bestIndex = p;
> +		}
> +
> +		++p;
> +	}
> +
> +	LOG(IPU3, Debug) << "Computed pipe configuration: ";
> +	LOG(IPU3, Debug) << "IF: " << pipeConfigs[bestIndex].iif.toString();
> +	LOG(IPU3, Debug) << "BDS: " << pipeConfigs[bestIndex].bds.toString();
> +	LOG(IPU3, Debug) << "GDC: " << pipeConfigs[bestIndex].gdc.toString();
> +
> +	return pipeConfigs[bestIndex];
> +}
> +
>  /**
>   * \brief Configure the ImgU unit input
>   * \param[in] size The ImgU input frame size
> diff --git a/src/libcamera/pipeline/ipu3/imgu.h b/src/libcamera/pipeline/ipu3/imgu.h
> index 5c124af2e9fe..15ee9a7f5698 100644
> --- a/src/libcamera/pipeline/ipu3/imgu.h
> +++ b/src/libcamera/pipeline/ipu3/imgu.h
> @@ -23,8 +23,28 @@ struct StreamConfiguration;
>  class ImgUDevice
>  {
>  public:
> +	struct PipeConfig {
> +		float bds_sf;
> +		Size iif;
> +		Size bds;
> +		Size gdc;
> +
> +		bool isNull() const
> +		{
> +			return iif.isNull() || bds.isNull() || gdc.isNull();
> +		}
> +	};
> +
> +	struct Pipe {
> +		Size input;
> +		Size output;
> +		Size viewfinder;
> +	};
> +
>  	int init(MediaDevice *media, unsigned int index);
>  
> +	PipeConfig calculatePipeConfig(Pipe *pipe);
> +
>  	int configureInput(const Size &size, V4L2DeviceFormat *inputFormat);
>  
>  	int configureOutput(const StreamConfiguration &cfg,
> -- 
> 2.27.0
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Hi Jacopo,

Thank you for the patch.

On Thu, Jul 09, 2020 at 10:41:25AM +0200, Jacopo Mondi wrote:
> Instrument the ImgU component to dynamically calculate the image
> manipulation pipeline intermediate sizes.
> 
> To correctly configure the ImgU it is necessary to program the IF, BDS
> and GDC sizes, which are currently fixed to the input frame size.
> 
> The procedure used to calculate the intermediate sizes has been ported
> from the pipe_config.py python script, available at:
> https://github.com/intel/intel-ipu3-pipecfg
> at revision:
> 61e83f2f7606 ("Add more information into README")
> 
> Define two structures (ImgUDevice::Pipe and ImgUdevice::PipeConfig)

s/ImgUdevice/ImgUDevice/

> to allow the pipeline handler to supply and retrieve configuration
> parameters from the ImgU unit.

s/ImgU unit/ImgU/

ImgU stands for imaging unit :-)

> Finally, add a new operation to the ImgUDdevice that calculates

s/ImgUDdevice/ImgUDevice/

(I wouldn't be opposed to renaming that simple ImgU)

> the pipe configuration parameters based on the requested input and
> output sizes.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  src/libcamera/pipeline/ipu3/imgu.cpp | 365 +++++++++++++++++++++++++++
>  src/libcamera/pipeline/ipu3/imgu.h   |  20 ++
>  2 files changed, 385 insertions(+)
> 
> diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp
> index d7f4173d3607..d1addb7d84d1 100644
> --- a/src/libcamera/pipeline/ipu3/imgu.cpp
> +++ b/src/libcamera/pipeline/ipu3/imgu.cpp
> @@ -7,6 +7,9 @@
>  
>  #include "imgu.h"
>  
> +#include <limits>
> +#include <math.h>
> +
>  #include <linux/media-bus-format.h>
>  
>  #include <libcamera/formats.h>
> @@ -19,6 +22,311 @@ namespace libcamera {
>  
>  LOG_DECLARE_CATEGORY(IPU3)
>  
> +namespace {
> +
> +static constexpr unsigned int FILTER_H = 4;
> +
> +static constexpr unsigned int IF_ALIGN_W = 2;
> +static constexpr unsigned int IF_ALIGN_H = 4;
> +
> +static constexpr unsigned int BDS_ALIGN_W = 2;
> +static constexpr unsigned int BDS_ALIGN_H = 4;
> +
> +static constexpr unsigned int IF_CROP_MAX_W = 40;
> +static constexpr unsigned int IF_CROP_MAX_H = 540;
> +
> +static constexpr float BDS_SF_MAX = 2.5;
> +static constexpr float BDS_SF_MIN = 1.0;
> +static constexpr float BDS_SF_STEP = 0.03125;
> +
> +/* BSD scaling factors: min=1, max=2.5, step=1/32 */
> +const std::vector<float> bdsScalingFactors = {
> +	1, 1.03125, 1.0625, 1.09375, 1.125, 1.15625, 1.1875, 1.21875, 1.25,
> +	1.28125, 1.3125, 1.34375, 1.375, 1.40625, 1.4375, 1.46875, 1.5, 1.53125,
> +	1.5625, 1.59375, 1.625, 1.65625, 1.6875, 1.71875, 1.75, 1.78125, 1.8125,
> +	1.84375, 1.875, 1.90625, 1.9375, 1.96875, 2, 2.03125, 2.0625, 2.09375,
> +	2.125, 2.15625, 2.1875, 2.21875, 2.25, 2.28125, 2.3125, 2.34375, 2.375,
> +	2.40625, 2.4375, 2.46875, 2.5
> +};
> +
> +/* GDC scaling factors: min=1, max=16, step=1/4 */
> +const std::vector<float> gdcScalingFactors = {
> +	1, 1.25, 1.5, 1.75, 2, 2.25, 2.5, 2.75, 3, 3.25, 3.5, 3.75, 4, 4.25,
> +	4.5, 4.75, 5, 5.25, 5.5, 5.75, 6, 6.25, 6.5, 6.75, 7, 7.25, 7.5, 7.75,
> +	8, 8.25, 8.5, 8.75, 9, 9.25, 9.5, 9.75, 10, 10.25, 10.5, 10.75, 11,
> +	11.25, 11.5, 11.75, 12, 12.25, 12.5, 12.75, 13, 13.25, 13.5, 13.75, 14,
> +	14.25, 14.5, 14.75, 15, 15.25, 15.5, 15.75, 16,
> +};
> +
> +std::vector<ImgUDevice::PipeConfig> pipeConfigs;
> +
> +struct FOV {
> +	float w;
> +	float h;
> +
> +	bool isLarger(const FOV &other)
> +	{
> +		if (w > other.w)
> +			return true;
> +		if (w == other.w && h > other.h)
> +			return true;
> +		return false;
> +	}
> +};

Could we use the Size class instead ?

> +
> +/* Approximate a scaling factor sf to the closest one available in a range. */
> +float findScaleFactor(float sf, const std::vector<float> &range,
> +		      bool roundDown = false)
> +{
> +	if (sf <= range[0])
> +		return range[0];
> +	if (sf >= range[range.size() - 1])
> +		return range[range.size() - 1];
> +
> +
> +	float bestDiff = std::numeric_limits<float>::max();
> +	unsigned int index = 0;
> +	for (unsigned int i = 0; i < range.size(); ++i) {
> +		float diff = std::abs(sf - range[i]);
> +		if (diff < bestDiff) {
> +			bestDiff = diff;
> +			index = i;
> +		}
> +	}
> +
> +	if (roundDown && index > 0 && sf < range[index])
> +		index--;
> +
> +	return range[index];
> +}

Given that the bdsScalingFactors factor is equal, in Python form, to

[1 + i / 32 for i in range(49)]

and gdcScalingFactors to

[ 1 + i / 4 for i in range(61)]

doesn't this function really round sf to a multiple of 1/32 or 1/4 with
bound cheking ? You could drop and it do the computation in the caller,
for instance

	float sf = findScaleFactor(yuvSF, gdcScalingFactors);

would become

	float sf = utils::clamp(roundf(yuvSF * 4) / 4, 1.0, 16.0);

and something similar with floorf for the BDS.

> +
> +bool isSameRatio(const Size &in, const Size &out)
> +{
> +	float inRatio = static_cast<float>(in.width) /
> +			static_cast<float>(in.height);
> +	float outRatio = static_cast<float>(out.width) /
> +			 static_cast<float>(out.height);
> +
> +	if (std::abs(inRatio - outRatio) > 0.1)
> +		return false;
> +
> +	return true;
> +}
> +
> +unsigned int alignIncrease(unsigned int len, unsigned int align)
> +{
> +	if (len % align)
> +		return (std::floor(static_cast<double>(len) /
> +				   static_cast<double>(align)) + 1) * align;

I find it quite peculiar to go through a double for this :-)

If a helper function is needed to align, I think it should go to
utils.h.

> +	return len;
> +}
> +
> +void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc,
> +			unsigned int bdsWidth, float bdsSF)
> +{
> +	unsigned int minIFHeight = iif.height - IF_CROP_MAX_H;
> +	unsigned int minBDSHeight = gdc.height + FILTER_H * 2;
> +	float bdsHeight;
> +
> +	if (!isSameRatio(pipe->input, gdc)) {
> +		bool found = false;
> +		float estIFHeight = static_cast<float>(iif.width) *
> +				    static_cast<float>(gdc.height) /
> +				    static_cast<float>(gdc.width);
> +		estIFHeight = utils::clamp<float>(estIFHeight,
> +						  static_cast<float>(minIFHeight),
> +						  static_cast<float>(iif.height));

As your force the utils::clamp type to float, is the static cast needed
for these parameters ?

> +		float ifHeight =

Unless I'm mistaken, ifHeight only takes integer values. Why is it a
float ?

Can I let you go through the rest of the code to simplify the
mathematical operations for the next version ?

> +			static_cast<float>(alignIncrease(static_cast<unsigned int>(estIFHeight),
> +							 IF_ALIGN_H));
> +		while (ifHeight >= minIFHeight && (ifHeight / bdsSF >= minBDSHeight)) {
> +			bdsHeight = ifHeight / bdsSF;
> +			if (std::fmod(bdsHeight, 1.0) == 0) {
> +				unsigned int bdsIntHeight =
> +					static_cast<unsigned int>(bdsHeight);
> +				if (!(bdsIntHeight % BDS_ALIGN_H)) {
> +					found = true;
> +					break;
> +				}
> +			}
> +
> +			ifHeight -= IF_ALIGN_H;
> +		}
> +
> +		ifHeight = static_cast<float>(
> +			alignIncrease(static_cast<unsigned int>(estIFHeight),
> +				      IF_ALIGN_H));
> +
> +		while (ifHeight <= iif.height && ifHeight / bdsSF >= minBDSHeight) {
> +			bdsHeight = ifHeight / bdsSF;
> +			if (std::fmod(bdsHeight, 1.0) == 0) {
> +				unsigned int bdsIntHeight =
> +					static_cast<unsigned int>(bdsHeight);
> +				if (!(bdsIntHeight % BDS_ALIGN_H)) {
> +					found = true;
> +					break;
> +				}
> +			}
> +
> +			ifHeight += IF_ALIGN_H;
> +		}
> +
> +		if (found) {
> +			unsigned int ifIntHeight = static_cast<unsigned int>(ifHeight);
> +			unsigned int bdsIntHeight = static_cast<unsigned int>(bdsHeight);
> +			pipeConfigs.push_back({ bdsSF, { iif.width, ifIntHeight },
> +						{ bdsWidth, bdsIntHeight }, gdc });
> +			return;
> +		}
> +	} else {
> +		float ifHeight = static_cast<float>(alignIncrease(iif.height, IF_ALIGN_H));
> +		while (ifHeight > minIFHeight && (ifHeight / bdsSF > minBDSHeight)) {
> +			bdsHeight = ifHeight / bdsSF;
> +			if (std::fmod(ifHeight, 1.0) == 0 && std::fmod(bdsHeight, 1.0) == 0) {
> +				unsigned int ifIntHeight = static_cast<unsigned int>(ifHeight);
> +				unsigned int bdsIntHeight = static_cast<unsigned int>(bdsHeight);
> +
> +				if (!(ifIntHeight % IF_ALIGN_H) &&
> +				    !(bdsIntHeight % BDS_ALIGN_H)) {
> +					pipeConfigs.push_back({ bdsSF, { iif.width, ifIntHeight },
> +								{ bdsWidth, bdsIntHeight }, gdc });
> +				}
> +			}
> +
> +			ifHeight -= IF_ALIGN_H;
> +		}
> +	}
> +}
> +
> +void calculateBDS(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc,
> +		  float bdsSF)
> +{
> +	unsigned int minBDSWidth = gdc.width + FILTER_H * 2;
> +
> +	float sf = bdsSF;
> +	while (sf <= BDS_SF_MAX && sf >= BDS_SF_MIN) {
> +		float bdsWidth = static_cast<float>(iif.width) / sf;
> +
> +		if (std::fmod(bdsWidth, 1.0) == 0) {
> +			unsigned int bdsIntWidth = static_cast<unsigned int>(bdsWidth);
> +			if (!(bdsIntWidth % BDS_ALIGN_W) && bdsWidth >= minBDSWidth)
> +				calculateBDSHeight(pipe, iif, gdc, bdsIntWidth, sf);
> +		}
> +
> +		sf += BDS_SF_STEP;
> +	}
> +
> +	sf = bdsSF;
> +	while (sf <= BDS_SF_MAX && sf >= BDS_SF_MIN) {
> +		float bdsWidth = static_cast<float>(iif.width) / sf;
> +
> +		if (std::fmod(bdsWidth, 1.0) == 0) {
> +			unsigned int bdsIntWidth = static_cast<unsigned int>(bdsWidth);
> +			if (!(bdsIntWidth % BDS_ALIGN_W) && bdsWidth >= minBDSWidth)
> +				calculateBDSHeight(pipe, iif, gdc, bdsIntWidth, sf);
> +		}
> +
> +		sf -= BDS_SF_STEP;
> +	}
> +}
> +
> +Size calculateGDC(ImgUDevice::Pipe *pipe)
> +{
> +	const Size &in = pipe->input;
> +	const Size &main = pipe->output;
> +	const Size &vf = pipe->viewfinder;
> +	Size gdc;
> +
> +	if (!vf.isNull()) {
> +		gdc.width = main.width;
> +
> +		float ratio = static_cast<float>(main.width) *
> +			      static_cast<float>(vf.height) /
> +			      static_cast<float>(vf.width);
> +		gdc.height = std::max(static_cast<float>(main.height), ratio);
> +
> +		return gdc;
> +	}
> +
> +	if (!isSameRatio(in, main)) {
> +		gdc = main;
> +		return gdc;
> +	}
> +
> +	float totalSF = static_cast<float>(in.width) /
> +			static_cast<float>(main.width);
> +	float bdsSF = totalSF > 2 ? 2 : 1;
> +	float yuvSF = totalSF / bdsSF;
> +	float sf = findScaleFactor(yuvSF, gdcScalingFactors);
> +
> +	gdc.width = static_cast<float>(main.width) * sf;
> +	gdc.height = static_cast<float>(main.height) * sf;
> +
> +	return gdc;
> +}
> +
> +FOV calcFOV(const Size &in, const ImgUDevice::PipeConfig &pipe)
> +{
> +	FOV fov{};
> +
> +	float inW = static_cast<float>(in.width);
> +	float inH = static_cast<float>(in.height);
> +	float ifCropW = static_cast<float>(in.width) - static_cast<float>(pipe.iif.width);
> +	float ifCropH = static_cast<float>(in.height) - static_cast<float>(pipe.iif.height);
> +	float gdcCropW = static_cast<float>(pipe.bds.width - pipe.gdc.width) * pipe.bds_sf;
> +	float gdcCropH = static_cast<float>(pipe.bds.height - pipe.gdc.height) * pipe.bds_sf;
> +	fov.w = (inW - (ifCropW + gdcCropW)) / inW;
> +	fov.h = (inH - (ifCropH + gdcCropH)) / inH;
> +
> +	return fov;
> +}
> +
> +} /* namespace */
> +
> +/**
> + * \struct PipeConfig
> + * \brief The ImgU pipe configuration parameters
> + *
> + * The ImgU image pipeline is composed of several hardware blocks that crop
> + * and scale the input image to obtain the desired output sizes. The
> + * scaling/cropping operations of those components is configured though the
> + * V4L2 selection API and the V4L2 subdev API applied to the ImgU media entity.
> + *
> + * The configurable components in the pipeline are:
> + * - IF: image feeder
> + * - BDS: bayer downscaler
> + * = GDC: geometric distorsion correction
> + *
> + * The IF crop rectangle is controlled by the V4L2_SEL_TGT_CROP selection target
> + * applied to the ImgU media entity sink pad number 0. The BDS scaler is
> + * controlled by the V4L2_SEL_TGT_COMPOSE target on the same pad, while the GDC
> + * output size is configured with the VIDIOC_SUBDEV_S_FMT IOCTL, again on pad
> + * number 0.
> + *
> + * The PipeConfig structure collects the sizes of each of those components
> + * plus the BDS scaling factor used to calculate the field of view
> + * of the final images.
> + */
> +
> +/**
> + * \struct Pipe
> + * \brief Describe the ImgU requested configuration
> + *
> + * The ImgU unit processes images through several components, which have
> + * to be properly configured inspecting the input image size and the desired
> + * output sizes. This structure collects the ImgU input configuration and the
> + * requested main output and viewfinder configurations.
> + *
> + * \var Pipe::input
> + * \brief The input image size
> + *
> + * \var Pipe::output
> + * \brief The requested main output size
> + *
> + * \var Pipe::viewfinder
> + * \brief The requested viewfinder size
> + */
> +
>  /**
>   * \brief Initialize components of the ImgU instance
>   * \param[in] mediaDevice The ImgU instance media device
> @@ -74,6 +382,63 @@ int ImgUDevice::init(MediaDevice *media, unsigned int index)
>  	return 0;
>  }
>  
> +/**
> + * \brief Calculate the ImgU pipe configuration parameters
> + * \param[in] pipe The requested ImgU configuration
> + * \return An ImgUDevice::PipeConfig instance on success, an empty configuration
> + * otherwise
> + */
> +ImgUDevice::PipeConfig ImgUDevice::calculatePipeConfig(Pipe *pipe)
> +{
> +	pipeConfigs.clear();
> +
> +	LOG(IPU3, Debug) << "Calculating pipe configuration for: ";
> +	LOG(IPU3, Debug) << "input: " << pipe->input.toString();
> +	LOG(IPU3, Debug) << "main: " << pipe->output.toString();
> +	LOG(IPU3, Debug) << "vf: " << pipe->viewfinder.toString();
> +
> +	const Size &in = pipe->input;
> +	Size gdc = calculateGDC(pipe);
> +
> +	unsigned int ifWidth = alignIncrease(in.width, IF_ALIGN_W);
> +	unsigned int ifHeight = in.height;
> +	unsigned int minIfWidth = in.width - IF_CROP_MAX_W;
> +	float bdsSF = static_cast<float>(in.width) /
> +		      static_cast<float>(gdc.width);
> +	float sf = findScaleFactor(bdsSF, bdsScalingFactors, true);
> +	while (ifWidth >= minIfWidth) {
> +		Size iif{ ifWidth, ifHeight };
> +		calculateBDS(pipe, iif, gdc, sf);
> +
> +		ifWidth -= IF_ALIGN_W;
> +	}
> +
> +	if (pipeConfigs.size() == 0) {
> +		LOG(IPU3, Error) << "Failed to calculate pipe configuration";
> +		return {};
> +	}
> +
> +	FOV bestFov = calcFOV(pipe->input, pipeConfigs[0]);
> +	unsigned int bestIndex = 0;
> +	unsigned int p = 0;
> +	for (auto pipeConfig : pipeConfigs) {
> +		FOV fov = calcFOV(pipe->input, pipeConfig);
> +		if (fov.isLarger(bestFov)) {
> +			bestFov = fov;
> +			bestIndex = p;
> +		}
> +
> +		++p;
> +	}
> +
> +	LOG(IPU3, Debug) << "Computed pipe configuration: ";
> +	LOG(IPU3, Debug) << "IF: " << pipeConfigs[bestIndex].iif.toString();
> +	LOG(IPU3, Debug) << "BDS: " << pipeConfigs[bestIndex].bds.toString();
> +	LOG(IPU3, Debug) << "GDC: " << pipeConfigs[bestIndex].gdc.toString();
> +
> +	return pipeConfigs[bestIndex];
> +}
> +
>  /**
>   * \brief Configure the ImgU unit input
>   * \param[in] size The ImgU input frame size
> diff --git a/src/libcamera/pipeline/ipu3/imgu.h b/src/libcamera/pipeline/ipu3/imgu.h
> index 5c124af2e9fe..15ee9a7f5698 100644
> --- a/src/libcamera/pipeline/ipu3/imgu.h
> +++ b/src/libcamera/pipeline/ipu3/imgu.h
> @@ -23,8 +23,28 @@ struct StreamConfiguration;
>  class ImgUDevice
>  {
>  public:
> +	struct PipeConfig {
> +		float bds_sf;
> +		Size iif;
> +		Size bds;
> +		Size gdc;
> +
> +		bool isNull() const
> +		{
> +			return iif.isNull() || bds.isNull() || gdc.isNull();
> +		}
> +	};
> +
> +	struct Pipe {
> +		Size input;
> +		Size output;
> +		Size viewfinder;
> +	};
> +
>  	int init(MediaDevice *media, unsigned int index);
>  
> +	PipeConfig calculatePipeConfig(Pipe *pipe);
> +
>  	int configureInput(const Size &size, V4L2DeviceFormat *inputFormat);
>  
>  	int configureOutput(const StreamConfiguration &cfg,

Hi Laurent,

On Fri, Jul 10, 2020 at 03:39:28PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Thu, Jul 09, 2020 at 10:41:25AM +0200, Jacopo Mondi wrote:
> > Instrument the ImgU component to dynamically calculate the image
> > manipulation pipeline intermediate sizes.
> >
> > To correctly configure the ImgU it is necessary to program the IF, BDS
> > and GDC sizes, which are currently fixed to the input frame size.
> >
> > The procedure used to calculate the intermediate sizes has been ported
> > from the pipe_config.py python script, available at:
> > https://github.com/intel/intel-ipu3-pipecfg
> > at revision:
> > 61e83f2f7606 ("Add more information into README")
> >
> > Define two structures (ImgUDevice::Pipe and ImgUdevice::PipeConfig)
>
> s/ImgUdevice/ImgUDevice/
>
> > to allow the pipeline handler to supply and retrieve configuration
> > parameters from the ImgU unit.
>
> s/ImgU unit/ImgU/
>
> ImgU stands for imaging unit :-)
>
> > Finally, add a new operation to the ImgUDdevice that calculates
>
> s/ImgUDdevice/ImgUDevice/
>
> (I wouldn't be opposed to renaming that simple ImgU)
>
> > the pipe configuration parameters based on the requested input and
> > output sizes.
> >
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  src/libcamera/pipeline/ipu3/imgu.cpp | 365 +++++++++++++++++++++++++++
> >  src/libcamera/pipeline/ipu3/imgu.h   |  20 ++
> >  2 files changed, 385 insertions(+)
> >
> > diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp
> > index d7f4173d3607..d1addb7d84d1 100644
> > --- a/src/libcamera/pipeline/ipu3/imgu.cpp
> > +++ b/src/libcamera/pipeline/ipu3/imgu.cpp
> > @@ -7,6 +7,9 @@
> >
> >  #include "imgu.h"
> >
> > +#include <limits>
> > +#include <math.h>
> > +
> >  #include <linux/media-bus-format.h>
> >
> >  #include <libcamera/formats.h>
> > @@ -19,6 +22,311 @@ namespace libcamera {
> >
> >  LOG_DECLARE_CATEGORY(IPU3)
> >
> > +namespace {
> > +
> > +static constexpr unsigned int FILTER_H = 4;
> > +
> > +static constexpr unsigned int IF_ALIGN_W = 2;
> > +static constexpr unsigned int IF_ALIGN_H = 4;
> > +
> > +static constexpr unsigned int BDS_ALIGN_W = 2;
> > +static constexpr unsigned int BDS_ALIGN_H = 4;
> > +
> > +static constexpr unsigned int IF_CROP_MAX_W = 40;
> > +static constexpr unsigned int IF_CROP_MAX_H = 540;
> > +
> > +static constexpr float BDS_SF_MAX = 2.5;
> > +static constexpr float BDS_SF_MIN = 1.0;
> > +static constexpr float BDS_SF_STEP = 0.03125;
> > +
> > +/* BSD scaling factors: min=1, max=2.5, step=1/32 */
> > +const std::vector<float> bdsScalingFactors = {
> > +	1, 1.03125, 1.0625, 1.09375, 1.125, 1.15625, 1.1875, 1.21875, 1.25,
> > +	1.28125, 1.3125, 1.34375, 1.375, 1.40625, 1.4375, 1.46875, 1.5, 1.53125,
> > +	1.5625, 1.59375, 1.625, 1.65625, 1.6875, 1.71875, 1.75, 1.78125, 1.8125,
> > +	1.84375, 1.875, 1.90625, 1.9375, 1.96875, 2, 2.03125, 2.0625, 2.09375,
> > +	2.125, 2.15625, 2.1875, 2.21875, 2.25, 2.28125, 2.3125, 2.34375, 2.375,
> > +	2.40625, 2.4375, 2.46875, 2.5
> > +};
> > +
> > +/* GDC scaling factors: min=1, max=16, step=1/4 */
> > +const std::vector<float> gdcScalingFactors = {
> > +	1, 1.25, 1.5, 1.75, 2, 2.25, 2.5, 2.75, 3, 3.25, 3.5, 3.75, 4, 4.25,
> > +	4.5, 4.75, 5, 5.25, 5.5, 5.75, 6, 6.25, 6.5, 6.75, 7, 7.25, 7.5, 7.75,
> > +	8, 8.25, 8.5, 8.75, 9, 9.25, 9.5, 9.75, 10, 10.25, 10.5, 10.75, 11,
> > +	11.25, 11.5, 11.75, 12, 12.25, 12.5, 12.75, 13, 13.25, 13.5, 13.75, 14,
> > +	14.25, 14.5, 14.75, 15, 15.25, 15.5, 15.75, 16,
> > +};
> > +
> > +std::vector<ImgUDevice::PipeConfig> pipeConfigs;
> > +
> > +struct FOV {
> > +	float w;
> > +	float h;
> > +
> > +	bool isLarger(const FOV &other)
> > +	{
> > +		if (w > other.w)
> > +			return true;
> > +		if (w == other.w && h > other.h)
> > +			return true;
> > +		return false;
> > +	}
> > +};
>
> Could we use the Size class instead ?
>

Not sure as the operator< of that class does not match the above
definition.

         * - A size with smaller width and smaller height is smaller.
         * - A size with smaller area is smaller.
         * - A size with smaller width is smaller.

I would keep this.

> > +
> > +/* Approximate a scaling factor sf to the closest one available in a range. */
> > +float findScaleFactor(float sf, const std::vector<float> &range,
> > +		      bool roundDown = false)
> > +{
> > +	if (sf <= range[0])
> > +		return range[0];
> > +	if (sf >= range[range.size() - 1])
> > +		return range[range.size() - 1];
> > +
> > +
> > +	float bestDiff = std::numeric_limits<float>::max();
> > +	unsigned int index = 0;
> > +	for (unsigned int i = 0; i < range.size(); ++i) {
> > +		float diff = std::abs(sf - range[i]);
> > +		if (diff < bestDiff) {
> > +			bestDiff = diff;
> > +			index = i;
> > +		}
> > +	}
> > +
> > +	if (roundDown && index > 0 && sf < range[index])
> > +		index--;
> > +
> > +	return range[index];
> > +}
>
> Given that the bdsScalingFactors factor is equal, in Python form, to
>
> [1 + i / 32 for i in range(49)]
>
> and gdcScalingFactors to
>
> [ 1 + i / 4 for i in range(61)]
>
> doesn't this function really round sf to a multiple of 1/32 or 1/4 with
> bound cheking ? You could drop and it do the computation in the caller,
> for instance
>
> 	float sf = findScaleFactor(yuvSF, gdcScalingFactors);
>
> would become
>
> 	float sf = utils::clamp(roundf(yuvSF * 4) / 4, 1.0, 16.0);
>
> and something similar with floorf for the BDS.
>

I'm afraid of deviating from the tool implementation as updates we
receive to the script would be harder to port here, and testing the
several corner cases is not trivial and incredibily time consuming for
a limited gain imho.

> > +
> > +bool isSameRatio(const Size &in, const Size &out)
> > +{
> > +	float inRatio = static_cast<float>(in.width) /
> > +			static_cast<float>(in.height);
> > +	float outRatio = static_cast<float>(out.width) /
> > +			 static_cast<float>(out.height);
> > +
> > +	if (std::abs(inRatio - outRatio) > 0.1)
> > +		return false;
> > +
> > +	return true;
> > +}
> > +
> > +unsigned int alignIncrease(unsigned int len, unsigned int align)
> > +{
> > +	if (len % align)
> > +		return (std::floor(static_cast<double>(len) /
> > +				   static_cast<double>(align)) + 1) * align;
>
> I find it quite peculiar to go through a double for this :-)

Yes, I don't know why I though I had to use:
        double  floor ( double arg );
>
> If a helper function is needed to align, I think it should go to
> utils.h.
>

This should be simple to add.

> > +	return len;
> > +}
> > +
> > +void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc,
> > +			unsigned int bdsWidth, float bdsSF)
> > +{
> > +	unsigned int minIFHeight = iif.height - IF_CROP_MAX_H;
> > +	unsigned int minBDSHeight = gdc.height + FILTER_H * 2;
> > +	float bdsHeight;
> > +
> > +	if (!isSameRatio(pipe->input, gdc)) {
> > +		bool found = false;
> > +		float estIFHeight = static_cast<float>(iif.width) *
> > +				    static_cast<float>(gdc.height) /
> > +				    static_cast<float>(gdc.width);
> > +		estIFHeight = utils::clamp<float>(estIFHeight,
> > +						  static_cast<float>(minIFHeight),
> > +						  static_cast<float>(iif.height));
>
> As your force the utils::clamp type to float, is the static cast needed
> for these parameters ?

Does the compiler implicitly cast thos integers to float ?
I see no error messages, so I assume so. I'll fix.

>
> > +		float ifHeight =
>
> Unless I'm mistaken, ifHeight only takes integer values. Why is it a
> float ?

Because of

        (ifHeight / bdsSF >= minBDSHeight)) {

And I've been multiple time bitten by roudings due to divisions by
integers having a different result than division by integers while
porting the computations from python to C++.


> Can I let you go through the rest of the code to simplify the
> mathematical operations for the next version ?
>

I'll see what I can do, but I won't push it too far, as subtle changes
could lead to different results, I would like to have this as close as
possible to the script to ease back-porting updates, and testing the
several corner cases and comparing it with the script result (as I
can't use the .xml file) is incredibly time consuming.

> > +			static_cast<float>(alignIncrease(static_cast<unsigned int>(estIFHeight),
> > +							 IF_ALIGN_H));
> > +		while (ifHeight >= minIFHeight && (ifHeight / bdsSF >= minBDSHeight)) {
> > +			bdsHeight = ifHeight / bdsSF;
> > +			if (std::fmod(bdsHeight, 1.0) == 0) {
> > +				unsigned int bdsIntHeight =
> > +					static_cast<unsigned int>(bdsHeight);
> > +				if (!(bdsIntHeight % BDS_ALIGN_H)) {
> > +					found = true;
> > +					break;
> > +				}
> > +			}
> > +
> > +			ifHeight -= IF_ALIGN_H;
> > +		}
> > +
> > +		ifHeight = static_cast<float>(
> > +			alignIncrease(static_cast<unsigned int>(estIFHeight),
> > +				      IF_ALIGN_H));
> > +
> > +		while (ifHeight <= iif.height && ifHeight / bdsSF >= minBDSHeight) {
> > +			bdsHeight = ifHeight / bdsSF;
> > +			if (std::fmod(bdsHeight, 1.0) == 0) {
> > +				unsigned int bdsIntHeight =
> > +					static_cast<unsigned int>(bdsHeight);
> > +				if (!(bdsIntHeight % BDS_ALIGN_H)) {
> > +					found = true;
> > +					break;
> > +				}
> > +			}
> > +
> > +			ifHeight += IF_ALIGN_H;
> > +		}
> > +
> > +		if (found) {
> > +			unsigned int ifIntHeight = static_cast<unsigned int>(ifHeight);
> > +			unsigned int bdsIntHeight = static_cast<unsigned int>(bdsHeight);
> > +			pipeConfigs.push_back({ bdsSF, { iif.width, ifIntHeight },
> > +						{ bdsWidth, bdsIntHeight }, gdc });
> > +			return;
> > +		}
> > +	} else {
> > +		float ifHeight = static_cast<float>(alignIncrease(iif.height, IF_ALIGN_H));
> > +		while (ifHeight > minIFHeight && (ifHeight / bdsSF > minBDSHeight)) {
> > +			bdsHeight = ifHeight / bdsSF;
> > +			if (std::fmod(ifHeight, 1.0) == 0 && std::fmod(bdsHeight, 1.0) == 0) {
> > +				unsigned int ifIntHeight = static_cast<unsigned int>(ifHeight);
> > +				unsigned int bdsIntHeight = static_cast<unsigned int>(bdsHeight);
> > +
> > +				if (!(ifIntHeight % IF_ALIGN_H) &&
> > +				    !(bdsIntHeight % BDS_ALIGN_H)) {
> > +					pipeConfigs.push_back({ bdsSF, { iif.width, ifIntHeight },
> > +								{ bdsWidth, bdsIntHeight }, gdc });
> > +				}
> > +			}
> > +
> > +			ifHeight -= IF_ALIGN_H;
> > +		}
> > +	}
> > +}
> > +
> > +void calculateBDS(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc,
> > +		  float bdsSF)
> > +{
> > +	unsigned int minBDSWidth = gdc.width + FILTER_H * 2;
> > +
> > +	float sf = bdsSF;
> > +	while (sf <= BDS_SF_MAX && sf >= BDS_SF_MIN) {
> > +		float bdsWidth = static_cast<float>(iif.width) / sf;
> > +
> > +		if (std::fmod(bdsWidth, 1.0) == 0) {
> > +			unsigned int bdsIntWidth = static_cast<unsigned int>(bdsWidth);
> > +			if (!(bdsIntWidth % BDS_ALIGN_W) && bdsWidth >= minBDSWidth)
> > +				calculateBDSHeight(pipe, iif, gdc, bdsIntWidth, sf);
> > +		}
> > +
> > +		sf += BDS_SF_STEP;
> > +	}
> > +
> > +	sf = bdsSF;
> > +	while (sf <= BDS_SF_MAX && sf >= BDS_SF_MIN) {
> > +		float bdsWidth = static_cast<float>(iif.width) / sf;
> > +
> > +		if (std::fmod(bdsWidth, 1.0) == 0) {
> > +			unsigned int bdsIntWidth = static_cast<unsigned int>(bdsWidth);
> > +			if (!(bdsIntWidth % BDS_ALIGN_W) && bdsWidth >= minBDSWidth)
> > +				calculateBDSHeight(pipe, iif, gdc, bdsIntWidth, sf);
> > +		}
> > +
> > +		sf -= BDS_SF_STEP;
> > +	}
> > +}
> > +
> > +Size calculateGDC(ImgUDevice::Pipe *pipe)
> > +{
> > +	const Size &in = pipe->input;
> > +	const Size &main = pipe->output;
> > +	const Size &vf = pipe->viewfinder;
> > +	Size gdc;
> > +
> > +	if (!vf.isNull()) {
> > +		gdc.width = main.width;
> > +
> > +		float ratio = static_cast<float>(main.width) *
> > +			      static_cast<float>(vf.height) /
> > +			      static_cast<float>(vf.width);
> > +		gdc.height = std::max(static_cast<float>(main.height), ratio);
> > +
> > +		return gdc;
> > +	}
> > +
> > +	if (!isSameRatio(in, main)) {
> > +		gdc = main;
> > +		return gdc;
> > +	}
> > +
> > +	float totalSF = static_cast<float>(in.width) /
> > +			static_cast<float>(main.width);
> > +	float bdsSF = totalSF > 2 ? 2 : 1;
> > +	float yuvSF = totalSF / bdsSF;
> > +	float sf = findScaleFactor(yuvSF, gdcScalingFactors);
> > +
> > +	gdc.width = static_cast<float>(main.width) * sf;
> > +	gdc.height = static_cast<float>(main.height) * sf;
> > +
> > +	return gdc;
> > +}
> > +
> > +FOV calcFOV(const Size &in, const ImgUDevice::PipeConfig &pipe)
> > +{
> > +	FOV fov{};
> > +
> > +	float inW = static_cast<float>(in.width);
> > +	float inH = static_cast<float>(in.height);
> > +	float ifCropW = static_cast<float>(in.width) - static_cast<float>(pipe.iif.width);
> > +	float ifCropH = static_cast<float>(in.height) - static_cast<float>(pipe.iif.height);
> > +	float gdcCropW = static_cast<float>(pipe.bds.width - pipe.gdc.width) * pipe.bds_sf;
> > +	float gdcCropH = static_cast<float>(pipe.bds.height - pipe.gdc.height) * pipe.bds_sf;
> > +	fov.w = (inW - (ifCropW + gdcCropW)) / inW;
> > +	fov.h = (inH - (ifCropH + gdcCropH)) / inH;
> > +
> > +	return fov;
> > +}
> > +
> > +} /* namespace */
> > +
> > +/**
> > + * \struct PipeConfig
> > + * \brief The ImgU pipe configuration parameters
> > + *
> > + * The ImgU image pipeline is composed of several hardware blocks that crop
> > + * and scale the input image to obtain the desired output sizes. The
> > + * scaling/cropping operations of those components is configured though the
> > + * V4L2 selection API and the V4L2 subdev API applied to the ImgU media entity.
> > + *
> > + * The configurable components in the pipeline are:
> > + * - IF: image feeder
> > + * - BDS: bayer downscaler
> > + * = GDC: geometric distorsion correction
> > + *
> > + * The IF crop rectangle is controlled by the V4L2_SEL_TGT_CROP selection target
> > + * applied to the ImgU media entity sink pad number 0. The BDS scaler is
> > + * controlled by the V4L2_SEL_TGT_COMPOSE target on the same pad, while the GDC
> > + * output size is configured with the VIDIOC_SUBDEV_S_FMT IOCTL, again on pad
> > + * number 0.
> > + *
> > + * The PipeConfig structure collects the sizes of each of those components
> > + * plus the BDS scaling factor used to calculate the field of view
> > + * of the final images.
> > + */
> > +
> > +/**
> > + * \struct Pipe
> > + * \brief Describe the ImgU requested configuration
> > + *
> > + * The ImgU unit processes images through several components, which have
> > + * to be properly configured inspecting the input image size and the desired
> > + * output sizes. This structure collects the ImgU input configuration and the
> > + * requested main output and viewfinder configurations.
> > + *
> > + * \var Pipe::input
> > + * \brief The input image size
> > + *
> > + * \var Pipe::output
> > + * \brief The requested main output size
> > + *
> > + * \var Pipe::viewfinder
> > + * \brief The requested viewfinder size
> > + */
> > +
> >  /**
> >   * \brief Initialize components of the ImgU instance
> >   * \param[in] mediaDevice The ImgU instance media device
> > @@ -74,6 +382,63 @@ int ImgUDevice::init(MediaDevice *media, unsigned int index)
> >  	return 0;
> >  }
> >
> > +/**
> > + * \brief Calculate the ImgU pipe configuration parameters
> > + * \param[in] pipe The requested ImgU configuration
> > + * \return An ImgUDevice::PipeConfig instance on success, an empty configuration
> > + * otherwise
> > + */
> > +ImgUDevice::PipeConfig ImgUDevice::calculatePipeConfig(Pipe *pipe)
> > +{
> > +	pipeConfigs.clear();
> > +
> > +	LOG(IPU3, Debug) << "Calculating pipe configuration for: ";
> > +	LOG(IPU3, Debug) << "input: " << pipe->input.toString();
> > +	LOG(IPU3, Debug) << "main: " << pipe->output.toString();
> > +	LOG(IPU3, Debug) << "vf: " << pipe->viewfinder.toString();
> > +
> > +	const Size &in = pipe->input;
> > +	Size gdc = calculateGDC(pipe);
> > +
> > +	unsigned int ifWidth = alignIncrease(in.width, IF_ALIGN_W);
> > +	unsigned int ifHeight = in.height;
> > +	unsigned int minIfWidth = in.width - IF_CROP_MAX_W;
> > +	float bdsSF = static_cast<float>(in.width) /
> > +		      static_cast<float>(gdc.width);
> > +	float sf = findScaleFactor(bdsSF, bdsScalingFactors, true);
> > +	while (ifWidth >= minIfWidth) {
> > +		Size iif{ ifWidth, ifHeight };
> > +		calculateBDS(pipe, iif, gdc, sf);
> > +
> > +		ifWidth -= IF_ALIGN_W;
> > +	}
> > +
> > +	if (pipeConfigs.size() == 0) {
> > +		LOG(IPU3, Error) << "Failed to calculate pipe configuration";
> > +		return {};
> > +	}
> > +
> > +	FOV bestFov = calcFOV(pipe->input, pipeConfigs[0]);
> > +	unsigned int bestIndex = 0;
> > +	unsigned int p = 0;
> > +	for (auto pipeConfig : pipeConfigs) {
> > +		FOV fov = calcFOV(pipe->input, pipeConfig);
> > +		if (fov.isLarger(bestFov)) {
> > +			bestFov = fov;
> > +			bestIndex = p;
> > +		}
> > +
> > +		++p;
> > +	}
> > +
> > +	LOG(IPU3, Debug) << "Computed pipe configuration: ";
> > +	LOG(IPU3, Debug) << "IF: " << pipeConfigs[bestIndex].iif.toString();
> > +	LOG(IPU3, Debug) << "BDS: " << pipeConfigs[bestIndex].bds.toString();
> > +	LOG(IPU3, Debug) << "GDC: " << pipeConfigs[bestIndex].gdc.toString();
> > +
> > +	return pipeConfigs[bestIndex];
> > +}
> > +
> >  /**
> >   * \brief Configure the ImgU unit input
> >   * \param[in] size The ImgU input frame size
> > diff --git a/src/libcamera/pipeline/ipu3/imgu.h b/src/libcamera/pipeline/ipu3/imgu.h
> > index 5c124af2e9fe..15ee9a7f5698 100644
> > --- a/src/libcamera/pipeline/ipu3/imgu.h
> > +++ b/src/libcamera/pipeline/ipu3/imgu.h
> > @@ -23,8 +23,28 @@ struct StreamConfiguration;
> >  class ImgUDevice
> >  {
> >  public:
> > +	struct PipeConfig {
> > +		float bds_sf;
> > +		Size iif;
> > +		Size bds;
> > +		Size gdc;
> > +
> > +		bool isNull() const
> > +		{
> > +			return iif.isNull() || bds.isNull() || gdc.isNull();
> > +		}
> > +	};
> > +
> > +	struct Pipe {
> > +		Size input;
> > +		Size output;
> > +		Size viewfinder;
> > +	};
> > +
> >  	int init(MediaDevice *media, unsigned int index);
> >
> > +	PipeConfig calculatePipeConfig(Pipe *pipe);
> > +
> >  	int configureInput(const Size &size, V4L2DeviceFormat *inputFormat);
> >
> >  	int configureOutput(const StreamConfiguration &cfg,
>
> --
> Regards,
>
> Laurent Pinchart

Hi Jacopo,

On Mon, Jul 13, 2020 at 11:25:16AM +0200, Jacopo Mondi wrote:
> On Fri, Jul 10, 2020 at 03:39:28PM +0300, Laurent Pinchart wrote:
> > On Thu, Jul 09, 2020 at 10:41:25AM +0200, Jacopo Mondi wrote:
> > > Instrument the ImgU component to dynamically calculate the image
> > > manipulation pipeline intermediate sizes.
> > >
> > > To correctly configure the ImgU it is necessary to program the IF, BDS
> > > and GDC sizes, which are currently fixed to the input frame size.
> > >
> > > The procedure used to calculate the intermediate sizes has been ported
> > > from the pipe_config.py python script, available at:
> > > https://github.com/intel/intel-ipu3-pipecfg
> > > at revision:
> > > 61e83f2f7606 ("Add more information into README")
> > >
> > > Define two structures (ImgUDevice::Pipe and ImgUdevice::PipeConfig)
> >
> > s/ImgUdevice/ImgUDevice/
> >
> > > to allow the pipeline handler to supply and retrieve configuration
> > > parameters from the ImgU unit.
> >
> > s/ImgU unit/ImgU/
> >
> > ImgU stands for imaging unit :-)
> >
> > > Finally, add a new operation to the ImgUDdevice that calculates
> >
> > s/ImgUDdevice/ImgUDevice/
> >
> > (I wouldn't be opposed to renaming that simple ImgU)
> >
> > > the pipe configuration parameters based on the requested input and
> > > output sizes.
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > ---
> > >  src/libcamera/pipeline/ipu3/imgu.cpp | 365 +++++++++++++++++++++++++++
> > >  src/libcamera/pipeline/ipu3/imgu.h   |  20 ++
> > >  2 files changed, 385 insertions(+)
> > >
> > > diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp
> > > index d7f4173d3607..d1addb7d84d1 100644
> > > --- a/src/libcamera/pipeline/ipu3/imgu.cpp
> > > +++ b/src/libcamera/pipeline/ipu3/imgu.cpp
> > > @@ -7,6 +7,9 @@
> > >
> > >  #include "imgu.h"
> > >
> > > +#include <limits>
> > > +#include <math.h>
> > > +
> > >  #include <linux/media-bus-format.h>
> > >
> > >  #include <libcamera/formats.h>
> > > @@ -19,6 +22,311 @@ namespace libcamera {
> > >
> > >  LOG_DECLARE_CATEGORY(IPU3)
> > >
> > > +namespace {
> > > +
> > > +static constexpr unsigned int FILTER_H = 4;
> > > +
> > > +static constexpr unsigned int IF_ALIGN_W = 2;
> > > +static constexpr unsigned int IF_ALIGN_H = 4;
> > > +
> > > +static constexpr unsigned int BDS_ALIGN_W = 2;
> > > +static constexpr unsigned int BDS_ALIGN_H = 4;
> > > +
> > > +static constexpr unsigned int IF_CROP_MAX_W = 40;
> > > +static constexpr unsigned int IF_CROP_MAX_H = 540;
> > > +
> > > +static constexpr float BDS_SF_MAX = 2.5;
> > > +static constexpr float BDS_SF_MIN = 1.0;
> > > +static constexpr float BDS_SF_STEP = 0.03125;
> > > +
> > > +/* BSD scaling factors: min=1, max=2.5, step=1/32 */
> > > +const std::vector<float> bdsScalingFactors = {
> > > +	1, 1.03125, 1.0625, 1.09375, 1.125, 1.15625, 1.1875, 1.21875, 1.25,
> > > +	1.28125, 1.3125, 1.34375, 1.375, 1.40625, 1.4375, 1.46875, 1.5, 1.53125,
> > > +	1.5625, 1.59375, 1.625, 1.65625, 1.6875, 1.71875, 1.75, 1.78125, 1.8125,
> > > +	1.84375, 1.875, 1.90625, 1.9375, 1.96875, 2, 2.03125, 2.0625, 2.09375,
> > > +	2.125, 2.15625, 2.1875, 2.21875, 2.25, 2.28125, 2.3125, 2.34375, 2.375,
> > > +	2.40625, 2.4375, 2.46875, 2.5
> > > +};
> > > +
> > > +/* GDC scaling factors: min=1, max=16, step=1/4 */
> > > +const std::vector<float> gdcScalingFactors = {
> > > +	1, 1.25, 1.5, 1.75, 2, 2.25, 2.5, 2.75, 3, 3.25, 3.5, 3.75, 4, 4.25,
> > > +	4.5, 4.75, 5, 5.25, 5.5, 5.75, 6, 6.25, 6.5, 6.75, 7, 7.25, 7.5, 7.75,
> > > +	8, 8.25, 8.5, 8.75, 9, 9.25, 9.5, 9.75, 10, 10.25, 10.5, 10.75, 11,
> > > +	11.25, 11.5, 11.75, 12, 12.25, 12.5, 12.75, 13, 13.25, 13.5, 13.75, 14,
> > > +	14.25, 14.5, 14.75, 15, 15.25, 15.5, 15.75, 16,
> > > +};
> > > +
> > > +std::vector<ImgUDevice::PipeConfig> pipeConfigs;
> > > +
> > > +struct FOV {
> > > +	float w;
> > > +	float h;
> > > +
> > > +	bool isLarger(const FOV &other)
> > > +	{
> > > +		if (w > other.w)
> > > +			return true;
> > > +		if (w == other.w && h > other.h)
> > > +			return true;
> > > +		return false;
> > > +	}
> > > +};
> >
> > Could we use the Size class instead ?
> 
> Not sure as the operator< of that class does not match the above
> definition.
> 
>          * - A size with smaller width and smaller height is smaller.
>          * - A size with smaller area is smaller.
>          * - A size with smaller width is smaller.
> 
> I would keep this.

We could use Size and have a custom comparison function though. If you
prefer keeping this structure, I would recommend renaming it to
FieldOfView, renaming w/h to width and height, and replacing isLarger()
with comparison operators. Could be also use integers instead of floats
?

> > > +
> > > +/* Approximate a scaling factor sf to the closest one available in a range. */
> > > +float findScaleFactor(float sf, const std::vector<float> &range,
> > > +		      bool roundDown = false)
> > > +{
> > > +	if (sf <= range[0])
> > > +		return range[0];
> > > +	if (sf >= range[range.size() - 1])
> > > +		return range[range.size() - 1];
> > > +
> > > +
> > > +	float bestDiff = std::numeric_limits<float>::max();
> > > +	unsigned int index = 0;
> > > +	for (unsigned int i = 0; i < range.size(); ++i) {
> > > +		float diff = std::abs(sf - range[i]);
> > > +		if (diff < bestDiff) {
> > > +			bestDiff = diff;
> > > +			index = i;
> > > +		}
> > > +	}
> > > +
> > > +	if (roundDown && index > 0 && sf < range[index])
> > > +		index--;
> > > +
> > > +	return range[index];
> > > +}
> >
> > Given that the bdsScalingFactors factor is equal, in Python form, to
> >
> > [1 + i / 32 for i in range(49)]
> >
> > and gdcScalingFactors to
> >
> > [ 1 + i / 4 for i in range(61)]
> >
> > doesn't this function really round sf to a multiple of 1/32 or 1/4 with
> > bound cheking ? You could drop and it do the computation in the caller,
> > for instance
> >
> > 	float sf = findScaleFactor(yuvSF, gdcScalingFactors);
> >
> > would become
> >
> > 	float sf = utils::clamp(roundf(yuvSF * 4) / 4, 1.0, 16.0);
> >
> > and something similar with floorf for the BDS.
> 
> I'm afraid of deviating from the tool implementation as updates we
> receive to the script would be harder to port here, and testing the
> several corner cases is not trivial and incredibily time consuming for
> a limited gain imho.

I wonder if we should expect updates :-) The python tool is really a
stop-gap measure to overcome the lack of documentation. I don't think
it's meant as a tool that will be maintained, and I believe the
authoritative open-source implementation of the ImgU pipeline
configuration will be ours, in the pipeline handler. I would thus rather
try to make it as clean as possible, to improve readability, and give us
a better understanding of the code.

> > > +
> > > +bool isSameRatio(const Size &in, const Size &out)
> > > +{
> > > +	float inRatio = static_cast<float>(in.width) /
> > > +			static_cast<float>(in.height);
> > > +	float outRatio = static_cast<float>(out.width) /
> > > +			 static_cast<float>(out.height);
> > > +
> > > +	if (std::abs(inRatio - outRatio) > 0.1)
> > > +		return false;
> > > +
> > > +	return true;
> > > +}
> > > +
> > > +unsigned int alignIncrease(unsigned int len, unsigned int align)
> > > +{
> > > +	if (len % align)
> > > +		return (std::floor(static_cast<double>(len) /
> > > +				   static_cast<double>(align)) + 1) * align;
> >
> > I find it quite peculiar to go through a double for this :-)
> 
> Yes, I don't know why I though I had to use:
>         double  floor ( double arg );
>
> > If a helper function is needed to align, I think it should go to
> > utils.h.
> 
> This should be simple to add.

Name bikeshedding aside of course :-) But yes, it shouldn't be hard.

> > > +	return len;
> > > +}
> > > +
> > > +void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc,
> > > +			unsigned int bdsWidth, float bdsSF)
> > > +{
> > > +	unsigned int minIFHeight = iif.height - IF_CROP_MAX_H;
> > > +	unsigned int minBDSHeight = gdc.height + FILTER_H * 2;
> > > +	float bdsHeight;
> > > +
> > > +	if (!isSameRatio(pipe->input, gdc)) {
> > > +		bool found = false;
> > > +		float estIFHeight = static_cast<float>(iif.width) *
> > > +				    static_cast<float>(gdc.height) /
> > > +				    static_cast<float>(gdc.width);
> > > +		estIFHeight = utils::clamp<float>(estIFHeight,
> > > +						  static_cast<float>(minIFHeight),
> > > +						  static_cast<float>(iif.height));
> >
> > As your force the utils::clamp type to float, is the static cast needed
> > for these parameters ?
> 
> Does the compiler implicitly cast thos integers to float ?
> I see no error messages, so I assume so. I'll fix.

When you force T = float with clamp<float>, all arguments to the
function are floats. The compiler thus implicitly converts the arguments
to floats, I don't think a cast is needed. If you didn't force the type,
and if the arguments were of different types, the compiler would likely
complain that it wouldn't know which type to pick.

> >
> > > +		float ifHeight =
> >
> > Unless I'm mistaken, ifHeight only takes integer values. Why is it a
> > float ?
> 
> Because of
> 
>         (ifHeight / bdsSF >= minBDSHeight)) {
> 
> And I've been multiple time bitten by roudings due to divisions by
> integers having a different result than division by integers while
> porting the computations from python to C++.

Isn't it equivalent in this case though ? The comparison uses >=, and
the round down caused by integer division would thus be covered by the =
case. Furthermore, bdsSF is a float, so the result of ifHeight / bdsSF
is also a float
(https://en.cppreference.com/w/cpp/language/operator_arithmetic#Conversions).
Up to you.

> > Can I let you go through the rest of the code to simplify the
> > mathematical operations for the next version ?
> 
> I'll see what I can do, but I won't push it too far, as subtle changes
> could lead to different results, I would like to have this as close as
> possible to the script to ease back-porting updates, and testing the
> several corner cases and comparing it with the script result (as I
> can't use the .xml file) is incredibly time consuming.

As stated above, I think that comparing the results of this initial
implementation with the output of the script is useful, but backporting
won't be the usual case. We'll have better result from code that we
understand.

> > > +			static_cast<float>(alignIncrease(static_cast<unsigned int>(estIFHeight),
> > > +							 IF_ALIGN_H));
> > > +		while (ifHeight >= minIFHeight && (ifHeight / bdsSF >= minBDSHeight)) {
> > > +			bdsHeight = ifHeight / bdsSF;
> > > +			if (std::fmod(bdsHeight, 1.0) == 0) {
> > > +				unsigned int bdsIntHeight =
> > > +					static_cast<unsigned int>(bdsHeight);
> > > +				if (!(bdsIntHeight % BDS_ALIGN_H)) {
> > > +					found = true;
> > > +					break;
> > > +				}
> > > +			}
> > > +
> > > +			ifHeight -= IF_ALIGN_H;
> > > +		}
> > > +
> > > +		ifHeight = static_cast<float>(
> > > +			alignIncrease(static_cast<unsigned int>(estIFHeight),
> > > +				      IF_ALIGN_H));
> > > +
> > > +		while (ifHeight <= iif.height && ifHeight / bdsSF >= minBDSHeight) {
> > > +			bdsHeight = ifHeight / bdsSF;
> > > +			if (std::fmod(bdsHeight, 1.0) == 0) {
> > > +				unsigned int bdsIntHeight =
> > > +					static_cast<unsigned int>(bdsHeight);
> > > +				if (!(bdsIntHeight % BDS_ALIGN_H)) {
> > > +					found = true;
> > > +					break;
> > > +				}
> > > +			}
> > > +
> > > +			ifHeight += IF_ALIGN_H;
> > > +		}
> > > +
> > > +		if (found) {
> > > +			unsigned int ifIntHeight = static_cast<unsigned int>(ifHeight);
> > > +			unsigned int bdsIntHeight = static_cast<unsigned int>(bdsHeight);
> > > +			pipeConfigs.push_back({ bdsSF, { iif.width, ifIntHeight },
> > > +						{ bdsWidth, bdsIntHeight }, gdc });
> > > +			return;
> > > +		}
> > > +	} else {
> > > +		float ifHeight = static_cast<float>(alignIncrease(iif.height, IF_ALIGN_H));
> > > +		while (ifHeight > minIFHeight && (ifHeight / bdsSF > minBDSHeight)) {
> > > +			bdsHeight = ifHeight / bdsSF;
> > > +			if (std::fmod(ifHeight, 1.0) == 0 && std::fmod(bdsHeight, 1.0) == 0) {
> > > +				unsigned int ifIntHeight = static_cast<unsigned int>(ifHeight);
> > > +				unsigned int bdsIntHeight = static_cast<unsigned int>(bdsHeight);
> > > +
> > > +				if (!(ifIntHeight % IF_ALIGN_H) &&
> > > +				    !(bdsIntHeight % BDS_ALIGN_H)) {
> > > +					pipeConfigs.push_back({ bdsSF, { iif.width, ifIntHeight },
> > > +								{ bdsWidth, bdsIntHeight }, gdc });
> > > +				}
> > > +			}
> > > +
> > > +			ifHeight -= IF_ALIGN_H;
> > > +		}
> > > +	}
> > > +}
> > > +
> > > +void calculateBDS(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc,
> > > +		  float bdsSF)
> > > +{
> > > +	unsigned int minBDSWidth = gdc.width + FILTER_H * 2;
> > > +
> > > +	float sf = bdsSF;
> > > +	while (sf <= BDS_SF_MAX && sf >= BDS_SF_MIN) {
> > > +		float bdsWidth = static_cast<float>(iif.width) / sf;
> > > +
> > > +		if (std::fmod(bdsWidth, 1.0) == 0) {
> > > +			unsigned int bdsIntWidth = static_cast<unsigned int>(bdsWidth);
> > > +			if (!(bdsIntWidth % BDS_ALIGN_W) && bdsWidth >= minBDSWidth)
> > > +				calculateBDSHeight(pipe, iif, gdc, bdsIntWidth, sf);
> > > +		}
> > > +
> > > +		sf += BDS_SF_STEP;
> > > +	}
> > > +
> > > +	sf = bdsSF;
> > > +	while (sf <= BDS_SF_MAX && sf >= BDS_SF_MIN) {
> > > +		float bdsWidth = static_cast<float>(iif.width) / sf;
> > > +
> > > +		if (std::fmod(bdsWidth, 1.0) == 0) {
> > > +			unsigned int bdsIntWidth = static_cast<unsigned int>(bdsWidth);
> > > +			if (!(bdsIntWidth % BDS_ALIGN_W) && bdsWidth >= minBDSWidth)
> > > +				calculateBDSHeight(pipe, iif, gdc, bdsIntWidth, sf);
> > > +		}
> > > +
> > > +		sf -= BDS_SF_STEP;
> > > +	}
> > > +}
> > > +
> > > +Size calculateGDC(ImgUDevice::Pipe *pipe)
> > > +{
> > > +	const Size &in = pipe->input;
> > > +	const Size &main = pipe->output;
> > > +	const Size &vf = pipe->viewfinder;
> > > +	Size gdc;
> > > +
> > > +	if (!vf.isNull()) {
> > > +		gdc.width = main.width;
> > > +
> > > +		float ratio = static_cast<float>(main.width) *
> > > +			      static_cast<float>(vf.height) /
> > > +			      static_cast<float>(vf.width);
> > > +		gdc.height = std::max(static_cast<float>(main.height), ratio);
> > > +
> > > +		return gdc;
> > > +	}
> > > +
> > > +	if (!isSameRatio(in, main)) {
> > > +		gdc = main;
> > > +		return gdc;
> > > +	}
> > > +
> > > +	float totalSF = static_cast<float>(in.width) /
> > > +			static_cast<float>(main.width);
> > > +	float bdsSF = totalSF > 2 ? 2 : 1;
> > > +	float yuvSF = totalSF / bdsSF;
> > > +	float sf = findScaleFactor(yuvSF, gdcScalingFactors);
> > > +
> > > +	gdc.width = static_cast<float>(main.width) * sf;
> > > +	gdc.height = static_cast<float>(main.height) * sf;
> > > +
> > > +	return gdc;
> > > +}
> > > +
> > > +FOV calcFOV(const Size &in, const ImgUDevice::PipeConfig &pipe)
> > > +{
> > > +	FOV fov{};
> > > +
> > > +	float inW = static_cast<float>(in.width);
> > > +	float inH = static_cast<float>(in.height);
> > > +	float ifCropW = static_cast<float>(in.width) - static_cast<float>(pipe.iif.width);
> > > +	float ifCropH = static_cast<float>(in.height) - static_cast<float>(pipe.iif.height);
> > > +	float gdcCropW = static_cast<float>(pipe.bds.width - pipe.gdc.width) * pipe.bds_sf;
> > > +	float gdcCropH = static_cast<float>(pipe.bds.height - pipe.gdc.height) * pipe.bds_sf;
> > > +	fov.w = (inW - (ifCropW + gdcCropW)) / inW;
> > > +	fov.h = (inH - (ifCropH + gdcCropH)) / inH;
> > > +
> > > +	return fov;
> > > +}
> > > +
> > > +} /* namespace */
> > > +
> > > +/**
> > > + * \struct PipeConfig
> > > + * \brief The ImgU pipe configuration parameters
> > > + *
> > > + * The ImgU image pipeline is composed of several hardware blocks that crop
> > > + * and scale the input image to obtain the desired output sizes. The
> > > + * scaling/cropping operations of those components is configured though the
> > > + * V4L2 selection API and the V4L2 subdev API applied to the ImgU media entity.
> > > + *
> > > + * The configurable components in the pipeline are:
> > > + * - IF: image feeder
> > > + * - BDS: bayer downscaler
> > > + * = GDC: geometric distorsion correction
> > > + *
> > > + * The IF crop rectangle is controlled by the V4L2_SEL_TGT_CROP selection target
> > > + * applied to the ImgU media entity sink pad number 0. The BDS scaler is
> > > + * controlled by the V4L2_SEL_TGT_COMPOSE target on the same pad, while the GDC
> > > + * output size is configured with the VIDIOC_SUBDEV_S_FMT IOCTL, again on pad
> > > + * number 0.
> > > + *
> > > + * The PipeConfig structure collects the sizes of each of those components
> > > + * plus the BDS scaling factor used to calculate the field of view
> > > + * of the final images.
> > > + */
> > > +
> > > +/**
> > > + * \struct Pipe
> > > + * \brief Describe the ImgU requested configuration
> > > + *
> > > + * The ImgU unit processes images through several components, which have
> > > + * to be properly configured inspecting the input image size and the desired
> > > + * output sizes. This structure collects the ImgU input configuration and the
> > > + * requested main output and viewfinder configurations.
> > > + *
> > > + * \var Pipe::input
> > > + * \brief The input image size
> > > + *
> > > + * \var Pipe::output
> > > + * \brief The requested main output size
> > > + *
> > > + * \var Pipe::viewfinder
> > > + * \brief The requested viewfinder size
> > > + */
> > > +
> > >  /**
> > >   * \brief Initialize components of the ImgU instance
> > >   * \param[in] mediaDevice The ImgU instance media device
> > > @@ -74,6 +382,63 @@ int ImgUDevice::init(MediaDevice *media, unsigned int index)
> > >  	return 0;
> > >  }
> > >
> > > +/**
> > > + * \brief Calculate the ImgU pipe configuration parameters
> > > + * \param[in] pipe The requested ImgU configuration
> > > + * \return An ImgUDevice::PipeConfig instance on success, an empty configuration
> > > + * otherwise
> > > + */
> > > +ImgUDevice::PipeConfig ImgUDevice::calculatePipeConfig(Pipe *pipe)
> > > +{
> > > +	pipeConfigs.clear();
> > > +
> > > +	LOG(IPU3, Debug) << "Calculating pipe configuration for: ";
> > > +	LOG(IPU3, Debug) << "input: " << pipe->input.toString();
> > > +	LOG(IPU3, Debug) << "main: " << pipe->output.toString();
> > > +	LOG(IPU3, Debug) << "vf: " << pipe->viewfinder.toString();
> > > +
> > > +	const Size &in = pipe->input;
> > > +	Size gdc = calculateGDC(pipe);
> > > +
> > > +	unsigned int ifWidth = alignIncrease(in.width, IF_ALIGN_W);
> > > +	unsigned int ifHeight = in.height;
> > > +	unsigned int minIfWidth = in.width - IF_CROP_MAX_W;
> > > +	float bdsSF = static_cast<float>(in.width) /
> > > +		      static_cast<float>(gdc.width);
> > > +	float sf = findScaleFactor(bdsSF, bdsScalingFactors, true);
> > > +	while (ifWidth >= minIfWidth) {
> > > +		Size iif{ ifWidth, ifHeight };
> > > +		calculateBDS(pipe, iif, gdc, sf);
> > > +
> > > +		ifWidth -= IF_ALIGN_W;
> > > +	}
> > > +
> > > +	if (pipeConfigs.size() == 0) {
> > > +		LOG(IPU3, Error) << "Failed to calculate pipe configuration";
> > > +		return {};
> > > +	}
> > > +
> > > +	FOV bestFov = calcFOV(pipe->input, pipeConfigs[0]);
> > > +	unsigned int bestIndex = 0;
> > > +	unsigned int p = 0;
> > > +	for (auto pipeConfig : pipeConfigs) {
> > > +		FOV fov = calcFOV(pipe->input, pipeConfig);
> > > +		if (fov.isLarger(bestFov)) {
> > > +			bestFov = fov;
> > > +			bestIndex = p;
> > > +		}
> > > +
> > > +		++p;
> > > +	}
> > > +
> > > +	LOG(IPU3, Debug) << "Computed pipe configuration: ";
> > > +	LOG(IPU3, Debug) << "IF: " << pipeConfigs[bestIndex].iif.toString();
> > > +	LOG(IPU3, Debug) << "BDS: " << pipeConfigs[bestIndex].bds.toString();
> > > +	LOG(IPU3, Debug) << "GDC: " << pipeConfigs[bestIndex].gdc.toString();
> > > +
> > > +	return pipeConfigs[bestIndex];
> > > +}
> > > +
> > >  /**
> > >   * \brief Configure the ImgU unit input
> > >   * \param[in] size The ImgU input frame size
> > > diff --git a/src/libcamera/pipeline/ipu3/imgu.h b/src/libcamera/pipeline/ipu3/imgu.h
> > > index 5c124af2e9fe..15ee9a7f5698 100644
> > > --- a/src/libcamera/pipeline/ipu3/imgu.h
> > > +++ b/src/libcamera/pipeline/ipu3/imgu.h
> > > @@ -23,8 +23,28 @@ struct StreamConfiguration;
> > >  class ImgUDevice
> > >  {
> > >  public:
> > > +	struct PipeConfig {
> > > +		float bds_sf;
> > > +		Size iif;
> > > +		Size bds;
> > > +		Size gdc;
> > > +
> > > +		bool isNull() const
> > > +		{
> > > +			return iif.isNull() || bds.isNull() || gdc.isNull();
> > > +		}
> > > +	};
> > > +
> > > +	struct Pipe {
> > > +		Size input;
> > > +		Size output;
> > > +		Size viewfinder;
> > > +	};
> > > +
> > >  	int init(MediaDevice *media, unsigned int index);
> > >
> > > +	PipeConfig calculatePipeConfig(Pipe *pipe);
> > > +
> > >  	int configureInput(const Size &size, V4L2DeviceFormat *inputFormat);
> > >
> > >  	int configureOutput(const StreamConfiguration &cfg,