[libcamera-devel] [PATCH v2] pipeline: raspberrypi: Fix incorrect advertising of ScalerCrop

[Jacopo Mondi]

Hi Naush,

On Tue, Jul 05, 2022 at 02:59:56PM +0100, Naushir Patuck via libcamera-devel wrote:
> The controls::ScalerCrop in the ControlInfoMap was advertised based on the ISP
> output Rectangle. This is incorrect, it needs to be set based on the sensor
> output Rectangle. Fix this.

I might have not completely woken up yet but the existing

	ctrlMap.emplace(&controls::ScalerCrop,
			ControlInfo(Rectangle(data->ispMinCropSize_), Rectangle(data->sensorInfo_.outputSize)));

Seems to be based on the sensor output size, not the ISP output size
as you mention here.

What am I missing ?

>
> Additionally, do not use emplace to be consistent with the other controls set
> in the ControlInfoMap.
>
> Fixes: 9dacde0d651d (pipeline: raspberrypi: Advertise ScalerCrop from the pipeline handler)
> Reported-by: David Plowman <david.plowman at raspberrypi.com>
> Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> ---
>  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 66a84b1dfb97..fdc24cd530c2 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -946,8 +946,9 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
>  		ctrlMap.emplace(c.first, c.second);
>
>  	/* Add the ScalerCrop control limits based on the current mode. */
> -	ctrlMap.emplace(&controls::ScalerCrop,
> -			ControlInfo(Rectangle(data->ispMinCropSize_), Rectangle(data->sensorInfo_.outputSize)));
> +	Rectangle ispMinCrop(data->ispMinCropSize_);
> +	ispMinCrop.scaleBy(data->sensorInfo_.analogCrop.size(), data->sensorInfo_.outputSize);

What is the purpose of scaling the minimum accepted ISP input in the
sensor's analogue crop / sensor's output size ratio ?

> +	ctrlMap[&controls::ScalerCrop] = ControlInfo(ispMinCrop, Rectangle(data->sensorInfo_.analogCrop.size()));

And I don't get this one either, how can the maximum scaler rectangle
be larger than the sensor's output size ? (assumiming
sensor->analogCrop > sensor->outputSize).

I'm slightly confused :)
>
>  	data->controlInfo_ = ControlInfoMap(std::move(ctrlMap), result.controlInfo.idmap());
>
> --
> 2.25.1
>