[libcamera-devel] pipeline: raspberrypi: Improve the values reported in the ScalerCrop control

18101 diff mbox series

Previously the x,y offsets in the min/max ScalerCrop control values
were zero. Here we make them the same as the sensor's analogue crop
offset which is I think less misleading.

With this change, it also seems reasonable to advertise the default
scaler crop value to be the true default that you will get. This makes
it possible for applications to see what that value will be without
having to start the camera and wait for frames.

Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
---
 src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

Hi David,

Thank you for this fix!

On Thu, 12 Jan 2023 at 11:06, David Plowman via libcamera-devel <
libcamera-devel@lists.libcamera.org> wrote:

> Previously the x,y offsets in the min/max ScalerCrop control values
> were zero. Here we make them the same as the sensor's analogue crop
> offset which is I think less misleading.
>
> With this change, it also seems reasonable to advertise the default
> scaler crop value to be the true default that you will get. This makes
> it possible for applications to see what that value will be without
> having to start the camera and wait for frames.
>
> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
>

Reviewed-by: Naushir Patuck <naush@raspberrypi.com>


> ---
>  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 8569df17..809af4d2 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -905,6 +905,7 @@ int PipelineHandlerRPi::configure(Camera *camera,
> CameraConfiguration *config)
>         /* Adjust aspect ratio by providing crops on the input image. */
>         Size size = unicamFormat.size.boundedToAspectRatio(maxSize);
>         Rectangle crop =
> size.centeredTo(Rectangle(unicamFormat.size).center());
> +       Rectangle defaultCrop = crop;
>         data->ispCrop_ = crop;
>
>         data->isp_[Isp::Input].dev()->setSelection(V4L2_SEL_TGT_CROP,
> &crop);
> @@ -958,9 +959,9 @@ int PipelineHandlerRPi::configure(Camera *camera,
> CameraConfiguration *config)
>                 ctrlMap.emplace(c.first, c.second);
>
>         /* Add the ScalerCrop control limits based on the current mode. */
> -       Rectangle ispMinCrop(data->ispMinCropSize_);
> -       ispMinCrop.scaleBy(data->sensorInfo_.analogCrop.size(),
> data->sensorInfo_.outputSize);
> -       ctrlMap[&controls::ScalerCrop] = ControlInfo(ispMinCrop,
> Rectangle(data->sensorInfo_.analogCrop.size()));
> +       Rectangle ispMinCrop =
> data->scaleIspCrop(Rectangle(data->ispMinCropSize_));
> +       defaultCrop = data->scaleIspCrop(defaultCrop);
> +       ctrlMap[&controls::ScalerCrop] = ControlInfo(ispMinCrop,
> data->sensorInfo_.analogCrop, defaultCrop);
>
>         data->controlInfo_ = ControlInfoMap(std::move(ctrlMap),
> result.controlInfo.idmap());
>
> --
> 2.30.2
>
>

Hi David

On Thu, Jan 12, 2023 at 11:06:33AM +0000, David Plowman via libcamera-devel wrote:
> Previously the x,y offsets in the min/max ScalerCrop control values
> were zero. Here we make them the same as the sensor's analogue crop
> offset which is I think less misleading.

This matches the definition of CameraSensorInfo::analogueCrop and
controls::ScalerCrop, as they're both defined relatively to the active
area size

  - ScalerCrop:
      type: Rectangle
      description: |
        Sets the image portion that will be scaled to form the whole of
        the final output image. The (x,y) location of this rectangle is
        relative to the PixelArrayActiveAreas that is being used. The units
/**
 * \var IPACameraSensorInfo::analogCrop
 * \brief The portion of the pixel array active area which is read-out and
 * processed
 *
 * The analog crop rectangle top-left corner is defined as the displacement
 * from the top-left corner of the pixel array active area. The rectangle


>
> With this change, it also seems reasonable to advertise the default
> scaler crop value to be the true default that you will get. This makes
> it possible for applications to see what that value will be without
> having to start the camera and wait for frames.
>
> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>

Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

Thanks
  j

> ---
>  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 8569df17..809af4d2 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -905,6 +905,7 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
>  	/* Adjust aspect ratio by providing crops on the input image. */
>  	Size size = unicamFormat.size.boundedToAspectRatio(maxSize);
>  	Rectangle crop = size.centeredTo(Rectangle(unicamFormat.size).center());
> +	Rectangle defaultCrop = crop;
>  	data->ispCrop_ = crop;
>
>  	data->isp_[Isp::Input].dev()->setSelection(V4L2_SEL_TGT_CROP, &crop);
> @@ -958,9 +959,9 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
>  		ctrlMap.emplace(c.first, c.second);
>
>  	/* Add the ScalerCrop control limits based on the current mode. */
> -	Rectangle ispMinCrop(data->ispMinCropSize_);
> -	ispMinCrop.scaleBy(data->sensorInfo_.analogCrop.size(), data->sensorInfo_.outputSize);
> -	ctrlMap[&controls::ScalerCrop] = ControlInfo(ispMinCrop, Rectangle(data->sensorInfo_.analogCrop.size()));
> +	Rectangle ispMinCrop = data->scaleIspCrop(Rectangle(data->ispMinCropSize_));
> +	defaultCrop = data->scaleIspCrop(defaultCrop);
> +	ctrlMap[&controls::ScalerCrop] = ControlInfo(ispMinCrop, data->sensorInfo_.analogCrop, defaultCrop);
>
>  	data->controlInfo_ = ControlInfoMap(std::move(ctrlMap), result.controlInfo.idmap());
>
> --
> 2.30.2
>