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

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Tue, Dec 01, 2020 at 11:06:54AM +0100, Jacopo Mondi 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.
> 
> Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
> v2 -> v3:
> - Fix grammar issues pointed out by David
> - Add David's tag
> 
> v1 -> v2:
> - Re-based on master
> - Specify that the top-left corner of the minimum rectangle is not relevant
> - RPi: scale ispMinCropSize_ in the sensor's pixel array matrix coordinates
>   Tested with 640x480 2x2 binned mode -> min = 128x128
>   Tested with 3280x2464 non binned mode -> min = 64x64
> ---
>  src/libcamera/control_ids.yaml                |  2 +-
>  .../pipeline/raspberrypi/raspberrypi.cpp      | 17 +++++---
>  src/libcamera/property_ids.yaml               | 42 ++++++++++++++-----
>  3 files changed, 45 insertions(+), 16 deletions(-)
> 
> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml
> index a883e27e22e9..44c4bb8e86b6 100644
> --- a/src/libcamera/control_ids.yaml
> +++ b/src/libcamera/control_ids.yaml
> @@ -283,7 +283,7 @@ 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 6fcdf557afcf..22d37e7f1ea1 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -708,13 +708,19 @@ 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 pixel array
> +	 * portion used to produce the ispMinCropSize_;
>  	 *
>  	 * \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);
> +	Rectangle sensorMinCrop =
> +		Rectangle(data->ispMinCropSize_).scaledBy(
> +				data->sensorInfo_.analogCrop.size(),
> +				data->sensorInfo_.outputSize);
> +	data->properties_.set(properties::ScalerCropLimits,
> +			      { data->sensorInfo_.analogCrop, sensorMinCrop });
> 
>  	return ret;
>  }
> @@ -946,11 +952,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..76474bfc5c31 100644
> --- a/src/libcamera/property_ids.yaml
> +++ b/src/libcamera/property_ids.yaml
> @@ -663,19 +663,41 @@ 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.

It's quite customary to report limits in the minimum, maximum order. Is
there a reason to do it the other way around here ?

> +
> +        This two rectangles respectively reflect the minimum and maximum

s/This/These/

> +        mandatory cropping applied in the camera sensor and the rest of the

That's not quite right I think. It's true for the maximum value
(reporting the minimum mandatory cropping), but for the minimum value,
it's more about reporting the scaler limits (maximum scaling factor or
minimum input size) than the "maximum mandatory cropping". How about the
following ?

        The minimum value reflects the upscaling limit, resulting from the
        maximum scaling factor or the minimum scaler input size. The maximum
        value reflects the minimum 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)/

> +        pipeline. Both Rectangles are defined relatively to the sensor's active
> +        pixel array (properties::PixelArrayActiveAreas).
> +
> +        Implementation details for the maximum valid crop rectangle.

How about

        \par Implementation details

with a white line just after to create a header ? Otherwise it looks a
bit weird to have one single paragraph whose first sentence is
"Implementation details for ...".

> +        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 produce the final image streams. The largest possible crop

"frame size" makes me think about binning/skipping. Maybe "... pipelines
may select different portions of the sensor's pixel array depending on
the requested streams" ? Up to you.

> +        rectangle is then limited by the size of the sensor's active pixel area

s/then/thus/ ?

I would drop "the size of", as the position of the maximum rectangle
depends on the sensor crop configuration, it's not just the size.

> +        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 to
> +        the size of sensor pixel array portion used to produce the smallest
> +        available stream resolution, accounting for any applied pixel
> +        sub-sampling technique in use and including any border pixels required
> +        by the ISP for image processing.

I'm having trouble parsing this. By "smallest available stream
resolution", are you talking about the smallest resolution a camera can
produce, or the smallest stream in the current configuration ? In the
latter case, shouldn't it be the largest stream, as upscaling is not
possible ?

> If a pipeline can up-scale, the minimum
> +        valid crop rectangle is equal to the pixel array portion that produces
> +        the smallest frame size accepted by the ISP input. The rectangle
> +        position, defined by its top-left corner, is not relevant and should be
> +        set to (0, 0) by pipelines.

Is the last sentence valid for both the non-upscaling and upscaling
cases ? It sounds like the latter, but I suppose it should be the
former. I don't think it's an implementation detail, I'd move it to the
first paragraph that could then be written

        The maximum value reflects the minimum mandatory cropping applied in the
        camera sensor and the rest of the pipeline. It specifies the boundary
        rectangle within which the ScalerCrop rectangle can be positioned. The
        value is defined relatively to the sensor's active pixel array
        (properties::PixelArrayActiveAreas)/

        The minimum value reflects the upscaling limit. It results from the
        maximum scaling factor or the minimum scaler input size, and specifies
        the minimum size for the ScalerCrop rectangle. The position of the
        minmum rectangle isn't relevant and shall be set to (0, 0).

> +
> +        The two 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.
> 
>  ...

-- 
Regards,

Laurent Pinchart