[libcamera-devel] [PATCH] libcamera: properties: Re-define ScalerCropMaximum

[David Plowman]

Hi Jacopo

Thanks for tackling this one again, glad it's not me! Just one or two
minor observations, and one more significant thing.

On Fri, 27 Nov 2020 at 15:17, Jacopo Mondi <jacopo at jmondi.org> wrote:
>
> Currently the properties::ScalerCropMaximum property reports the maximum
> valid Rectangle for the controls::ScalerCrop control, which can be
> combined with the sensor pixel array size in order to obtain the minimum
> available zoom factor.
>
> It is also useful to be able to calculate the maximum available zoom
> factor, and in order to do so the minimum valid crop rectangle has to
> be reported.
>
> Transform the ScalerCropMaximum property in ScalerCropLimits, which
> reports both the maximum and minimum crop limits and port the only user
> of such a property (Raspberry Pi) to use it.
>
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  src/libcamera/control_ids.yaml                |  2 +-
>  .../pipeline/raspberrypi/raspberrypi.cpp      | 14 ++++---
>  src/libcamera/property_ids.yaml               | 38 ++++++++++++++-----
>  3 files changed, 38 insertions(+), 16 deletions(-)
>
> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml
> index c8874fa91965..11623fc56589 100644
> --- a/src/libcamera/control_ids.yaml
> +++ b/src/libcamera/control_ids.yaml
> @@ -528,6 +528,6 @@ controls:
>          a binning or skipping mode.
>
>          This control is only present when the pipeline supports scaling. Its
> -        maximum valid value is given by the properties::ScalerCropMaximum
> +        valid value limits are given by the properties::ScalerCropLimits
>          property, and the two can be used to implement digital zoom.
>  ...
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 00a40c558bfa..cac88ca65ba5 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -702,13 +702,16 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
>                 LOG(RPI, Error) << "Failed to configure the IPA: " << ret;
>
>         /*
> -        * Update the ScalerCropMaximum to the correct value for this camera mode.
> -        * For us, it's the same as the "analogue crop".
> +        * Update the ScalerCropLimits to the correct values for this camera
> +        * mode. For us, it's the same as the "analogue crop" and the ISP
> +        * minimum input size.
>          *
>          * \todo Make this property the ScalerCrop maximum value when dynamic
>          * controls are available and set it at validate() time
>          */
> -       data->properties_.set(properties::ScalerCropMaximum, data->sensorInfo_.analogCrop);
> +       data->properties_.set(properties::ScalerCropLimits,
> +                             { data->sensorInfo_.analogCrop,
> +                               Rectangle(data->ispMinCropSize_) });

So I think there might be a problem here, which is that
ispMinCropSize_ is in units of the sensor output pixels (which might
be binned), and the property wants them in sensor native pixels.
Therefore we'll need to transform them up to the analog_crop size,
maybe something like this:

Rectangle sensorMinCrop =
Rectangle(data->ispMinCropSize_).scaledBy(data->sensorInfo_.analogCrop.size(),
sensorFormat.size);
data->properties_.set(properties::ScalerCropLimits, {
data->sensorInfo_.analogCrop, sensorMinCrop.size() });

but I haven't tried it and it's probably full of typos.

A little part of me also wonders what the offset, sorry, *top left* of
the minimum Rectangle means. Presumably it doesn't mean anything?

>
>         return ret;
>  }
> @@ -937,11 +940,12 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)
>         data->properties_ = data->sensor_->properties();
>
>         /*
> -        * Set a default value for the ScalerCropMaximum property to show
> +        * Set default values for the ScalerCropLimits property to show
>          * that we support its use, however, initialise it to zero because
>          * it's not meaningful until a camera mode has been chosen.
>          */
> -       data->properties_.set(properties::ScalerCropMaximum, Rectangle{});
> +       data->properties_.set(properties::ScalerCropLimits,
> +                             { Rectangle{}, Rectangle{} });
>
>         /*
>          * We cache three things about the sensor in relation to transforms
> diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml
> index 64e88f0361d6..9a3ffcd05eeb 100644
> --- a/src/libcamera/property_ids.yaml
> +++ b/src/libcamera/property_ids.yaml
> @@ -663,19 +663,37 @@ controls:
>          \todo Rename this property to ActiveAreas once we will have property
>                categories (i.e. Properties::PixelArray::ActiveAreas)
>
> -  - ScalerCropMaximum:
> +  - ScalerCropLimits:
>        type: Rectangle
> +      size: [2]
>        description: |
> -        The maximum valid rectangle for the controls::ScalerCrop control. This
> -        reflects the minimum mandatory cropping applied in the camera sensor and
> -        the rest of the pipeline. Just as the ScalerCrop control, it defines a
> -        rectangle taken from the sensor's active pixel array.
> -
> -        This property is valid only after the camera has been successfully
> -        configured and its value may change whenever a new configuration is
> +        The maximum and minimum valid rectangles for the controls::ScalerCrop
> +        control, specified in this order.
> +
> +        This two rectangles respectively reflect the minimum and maximum

Should this be "maximum and minimum" rather than "minimum and
maximum", having just previously described the order like that?

> +        mandatory cropping applied in the camera sensor and the rest of the
> +        pipeline. Both Rectangles are defined relatively to the sensor's active
> +        pixel array (properties::PixelArrayActiveAreas).

Do we need to mention that the top left of the minimum doesn't mean anything?

> +
> +        Implementation details for the maximum valid crop rectangle.
> +        The maximum valid crop rectangle depends on the camera configuration as
> +        pipelines are free to adjust the frame size requested from the sensor
> +        and used to produced the final image streams. The largest possible crop

s/produced/produce/

> +        rectangle is then limited by the size of the sensor's active pixel area
> +        portion used to produce the sensor output frame.
> +
> +        Implementation details for the minimum valid crop rectangle.
> +        If a pipeline cannot up-scale the minimum valid crop rectangle is equal

I think you need a comma after "up-scale" (my internal language parser
crashed when I reached "is equal"!)

> +        to the smallest available stream resolution (plus any border pixels
> +        required by the ISP for image processing) part of the current camera
> +        configuration. If a pipeline can up-scale, the minimum valid crop

Ah, it's right here!  :)

Thanks again, and best regards
David

> +        rectangle is equal to the smallest frame size accepted by the ISP input.
> +
> +        The Rectangles are valid only after the camera has been successfully
> +        configured and their value may change 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.
> +        \todo Express this property as the limits for the ScalerCrop control
> +        once "dynamic" controls have been implemented.
>
>  ...
> --
> 2.29.1
>