[libcamera-devel] [PATCH v4 1/5] libcamera: Add SensorCropMaximum property

[Laurent Pinchart]

Hi David,

Thank you for the patch.

On Mon, Oct 19, 2020 at 01:51:52PM +0100, David Plowman wrote:
> The SensorCropMaximum camera property reports the location of that
> part of the image sensor array that is scaled to produce the output
> images, given in native sensor pixels. It will normally change when a
> new camera mode is selected, and can be used to implement digital
> zoom.
>
> Signed-off-by: David Plowman <david.plowman at raspberrypi.com>
> ---
>  src/libcamera/property_ids.yaml | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml
> index 7261263a..022cf65d 100644
> --- a/src/libcamera/property_ids.yaml
> +++ b/src/libcamera/property_ids.yaml
> @@ -663,4 +663,24 @@ controls:
>          \todo Rename this property to ActiveAreas once we will have property
>                categories (i.e. Properties::PixelArray::ActiveAreas)
>  
> +  - ScalerCropMaximum:
> +      type: Rectangle
> +      description: |
> +        The size and location, in native sensor pixels, of the part of the
> +        sensor that is rescaled to produce the output images. Note that the
> +        units remain native sensor pixels, even if the sensor is being used in
> +        a binning skipping or scaling mode.

Now that I've reviewed 5/5 I think we agree on the meaning of this
property, but the text here doesn't seem to correctly match what I
understand is our agreement :-) The above paragraph is actually what I
was expecting for the ScalerCrop control, not the ScalerCropMaximum
property.

How about simply defining this property as the maximum value for
ScalerCrop ? It could be written as just

        The maximum valid rectangle for the controls::ScalerCrop control.

with the last two paragraphs kept. The rest would then be moved to the
documentation of ScalerCrop (if they're not there already, I'll review
that patch next).

If you agree with this, and with the commit message updated too,

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> +
> +        The (x,y) location of this rectangle is relative to the
> +        PixelArrayActiveArea that is being used. The property also takes into
> +        account any further cropping being done by the CSI-2 receiver or
> +        elsewhere.
> +
> +        This property is valid only after the Camera has been successfully
> +        configured and its value changes whenever a new configuration is
> +        applied.
> +
> +        \todo Turn this property into a "maximum control value" for the
> +        ScalerCrop control once "dynamic" controls have been implemented.
> +
>  ...

-- 
Regards,

Laurent Pinchart