[libcamera-devel] [PATCH 07/12] libcamera: ipu3: Register ScalerCrop control

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Tue, Jan 05, 2021 at 08:05:17PM +0100, Jacopo Mondi wrote:
> Register the ScalerCrop control in the ImgU pipeline handler computed
> by using the Viewfinder [1280x720] pipeline output configuration and
> the sensor resolution as parameters.
> 
> The ScalerCrop control limits should be updated everytime a new
> configuration is applied to the sensor.
> 
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 53 ++++++++++++++++++++++++++--
>  1 file changed, 51 insertions(+), 2 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 418301b33a5e..f1329ffb0463 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -41,6 +41,7 @@ static constexpr unsigned int IMGU_OUTPUT_WIDTH_ALIGN = 64;
>  static constexpr unsigned int IMGU_OUTPUT_HEIGHT_ALIGN = 4;
>  static constexpr unsigned int IMGU_OUTPUT_WIDTH_MARGIN = 64;
>  static constexpr unsigned int IMGU_OUTPUT_HEIGHT_MARGIN = 32;
> +static constexpr Size IPU3ViewfinderSize(1280, 720);
>  
>  static const ControlInfoMap::Map IPU3Controls = {
>  	{ &controls::draft::PipelineDepth, ControlInfo(2, 3) },
> @@ -378,7 +379,7 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,
>  			 * capped to the maximum sensor resolution and aligned
>  			 * to the ImgU output constraints.
>  			 */
> -			size = sensorResolution.boundedTo({ 1280, 720 })
> +			size = sensorResolution.boundedTo(IPU3ViewfinderSize)

An unrelated change, but a good one. Could you briefly mention it in the
commit message ?

>  					       .alignedDownTo(IMGU_OUTPUT_WIDTH_ALIGN,
>  							      IMGU_OUTPUT_HEIGHT_ALIGN);
>  			pixelFormat = formats::NV12;
> @@ -785,7 +786,7 @@ int PipelineHandlerIPU3::initProperties(IPU3CameraData *data)
>   */
>  int PipelineHandlerIPU3::initControls(IPU3CameraData *data)
>  {
> -	const CameraSensor *sensor = data->cio2_.sensor();
> +	CameraSensor *sensor = data->cio2_.sensor();
>  	CameraSensorInfo sensorInfo{};
>  
>  	int ret = sensor->sensorInfo(&sensorInfo);
> @@ -822,6 +823,54 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data)
>  	controls[&controls::ExposureTime] = ControlInfo(minExposure, maxExposure,
>  							defExposure);
>  
> +	/*
> +	 * Compute the scaler crop limits.
> +	 *
> +	 * \todo The scaler crop limits depend on the sensor configuration.  It
> +	 * should be updated when a new configuration is applied.  To initialize
> +	 * the control use the 'Viewfinder' configuration (1280x720) as the
> +	 * pipeline output resolution and the full sensor size as input frame
> +	 * (see the todo note in the validation function).

s/the validation function/IPU3CameraConfiguration::validate()/

I'm not sure what todo note you're referring too though.

> +	 */
> +
> +	/*
> +	 * The maximum scaler crop rectangle is the analogue crop used to
> +	 * produce the maximum frame size.
> +	 */
> +	V4L2SubdeviceFormat sensorFormat;
> +	sensorFormat.size = sensor->resolution();
> +	ret = sensor->setFormat(&sensorFormat);
> +	if (ret)
> +		return ret;
> +
> +	/* Re-fetch the sensor info updated to use the largest resolution. */
> +	ret = sensor->sensorInfo(&sensorInfo);
> +	if (ret)
> +		return ret;
> +
> +	const Rectangle &analogueCrop = sensorInfo.analogCrop;
> +	Rectangle maxCrop = analogueCrop;
> +
> +	/*
> +	 * The minimum crop rectangle is the default viewfinder configuration
> +	 * (or the sensor resolution, if smaller) re-scaled in the sensor's pixel

Just rescaled, or does the ImgU apply cropping along the pipeline ? If
it does (or just if you're not sure), a \todo comment would be good.

> +	 * array coordinates. As the ImgU cannot up-scale, the minimum selection
> +	 * rectangle has to be as large as the desired pipeline output size.
> +	 *
> +	 * The top-left corner position is not relevant as the minimum crop
> +	 * rectangle can be placed anywhere inside the analogue crop region.

I would have sworn we had documented it as being (0,0) as a convention,
but I don't see that in the control documentation. As it's not relevant
we can set it to anything, but I believe we should standardize the
behaviour. What do you think would be best, (0,0), the same (x,y)
position as the maximium value, or centering the minimum rectangle in
the maximum rectangle ?

The code otherwise looks fine.

> +	 */
> +	const Size &sensorOutput = sensorInfo.outputSize;
> +	Size minCropSize = sensorOutput.boundedTo(IPU3ViewfinderSize)
> +				       .alignedDownTo(IMGU_OUTPUT_WIDTH_ALIGN,
> +						      IMGU_OUTPUT_HEIGHT_ALIGN);
> +	Rectangle minCrop(minCropSize);
> +	minCrop.scaledBy(analogueCrop.size(), sensorOutput);
> +	minCrop.x = analogueCrop.x;
> +	minCrop.y = analogueCrop.y;
> +
> +	controls[&controls::ScalerCrop] = ControlInfo(minCrop, maxCrop, maxCrop);
> +
>  	data->controlInfo_ = std::move(controls);
>  
>  	return 0;

-- 
Regards,

Laurent Pinchart