[libcamera-devel] [RFC PATCH 2/4] libcamera: Add SensorCrop control

[Jacopo Mondi]

Hi David,
  super nit: in Subject: "libcamera: controls: Add SensorCrop"

On Mon, Sep 07, 2020 at 05:44:48PM +0100, David Plowman wrote:
> The SensorCrop 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.
> ---
>  src/libcamera/control_ids.yaml | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml
> index 3560d4a..cebaa25 100644
> --- a/src/libcamera/control_ids.yaml
> +++ b/src/libcamera/control_ids.yaml
> @@ -273,4 +273,13 @@ controls:
>          order in an array of 9 floating point values.
>
>        size: [3x3]
> +
> +  - SensorCrop:

I wonder, is this a Sensor related property ? Roughly speaking that's
a selection on the input frame that is provided to the ISP for
processing, if I think about the possibly forthcoming namespacing of
the controls IDs I don't see this being well placed as Sensor::Crop
(but rather as ISP::Crop possibly ?)

I recall you had 'PipelineCrop' in previous version, am I wrong ?

> +      type: Rectangle
> +      description: |
> +        Sets the portion of the full sensor image, in pixels, that will be
> +        scaled up to form the whole of the final output image. This control

As a suggestion, take whatever you like in:

           Selection rectangle (in pixel units) that defines the
           portion of the image that will be scaled up to form the
           final output image. This control can be used to implement
           digital zoom.

> +        can be used to implement digital zoom. The size of the full sensor
> +        image within which an application can crop is available from the
> +        SensorOutputSize property.

           The rectangle is defined in respect to the size of the
           image which is processed by the ISP to produce the output
           streams, whose size is reported by the SensorOutputSize
           property.

           \sa properties::SensorOutputSize

How bad is it in your opinion that this will apply to all streams ? I
don't see many ways around and to me it's fine, but maybe we have
different expectations ?

How does it work if the Rectangle sizes are larger than the input
frame sizes ? Should we enforce a behaviour like always clamping the
SensorCrop size to the SensorOutputSize ones ?

Thanks
  j


>  ...
> --
> 2.20.1
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel