[libcamera-devel] [PATCH v4 3/5] libcamera: Add ScalerCrop control

[Laurent Pinchart]

Hi David,

Thank you for the patch.

On Mon, Oct 19, 2020 at 01:51:54PM +0100, David Plowman wrote:
> The ScalerCrop control selects how much of the sensor's output image
> will be scaled to form the output image. It can be used to implement
> digital zoom.
> 
> Signed-off-by: David Plowman <david.plowman at raspberrypi.com>
> ---
>  src/libcamera/control_ids.yaml  | 12 ++++++++++++
>  src/libcamera/property_ids.yaml |  5 ++++-
>  2 files changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml
> index 4c415545..c6fbcd56 100644
> --- a/src/libcamera/control_ids.yaml
> +++ b/src/libcamera/control_ids.yaml
> @@ -284,4 +284,16 @@ controls:
>          order in an array of 9 floating point values.
>  
>        size: [3x3]
> +
> +  - ScalerCrop:
> +      type: Rectangle
> +      description: |
> +        Sets the image portion that will be scaled up to form the whole of

Scaled up or down, isn't it ?

> +        the final output image. The selection rectangle is expressed in the
> +        sensor's native pixels and defined relative to the size of the frame
> +        described by the ScalerCropMaximum property.

I find it a bit confusing to define this property relatively to its
maximum value (and that won't work anymore when ScalerCropMaximum gets
replaced by dynamic ControlInfo). How about already defining it
relatively to PixelArrayActiveArea ? Maybe something along the lines of

        The rectangle is expressed in the sensor's native pixels, relatively to
        the properties::PixelArrayActiveArea property.


I think we should move here the third paragraph from the documentation
of SensorCropMaximum.

> +
> +        This control can be used to implement digital zoom.
> +
> +        \sa properties::ScalerCropMaximum

Maybe

        The maximum valid value for this control is reported by the
	properties::ScalerCropMaximum property.

Doxygen should then automatically generate a link, you wouldn't need the
\sa.

>  ...
> diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml
> index 022cf65d..2d0fe9d3 100644
> --- a/src/libcamera/property_ids.yaml
> +++ b/src/libcamera/property_ids.yaml
> @@ -678,7 +678,10 @@ controls:
>  
>          This property is valid only after the Camera has been successfully
>          configured and its value changes whenever a new configuration is
> -        applied.
> +        applied. It can be used to implement digital zoom in conjunction with
> +        the ScalerCrop control.

We could keep the mention of digital zoom in the documentation of
ScalerCrop only. What I would however add here is that this property is
present if and only if the ScalerCrop control is supported.

> +
> +        \sa controls::ScalerCrop
>  
>          \todo Turn this property into a "maximum control value" for the
>          ScalerCrop control once "dynamic" controls have been implemented.

-- 
Regards,

Laurent Pinchart