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

10526 diff mbox series

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@raspberrypi.com>
Signed-off-by: Jacopo Mondi <jacopo@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(-)

--
2.29.1

Hi Jacopo

Thanks for version 3 - just confirming that it all looks good to me now!

Best regards
David

On Tue, 1 Dec 2020 at 10:06, Jacopo Mondi <jacopo@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.
>
> Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
> Signed-off-by: Jacopo Mondi <jacopo@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.
> +
> +        This two rectangles respectively reflect the minimum and maximum
> +        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).
> +
> +        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 produce the final image streams. The largest possible crop
> +        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 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. 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.
> +
> +        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.
>
>  ...
> --
> 2.29.1
>

Hi Jacopo,

Thanks for your patch.

On 2020-12-01 11:06:54 +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@raspberrypi.com>
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>

I think this is a step in the right direction.

Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>

> ---
> 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.
> +
> +        This two rectangles respectively reflect the minimum and maximum
> +        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).
> +
> +        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 produce the final image streams. The largest possible crop
> +        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 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. 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.
> +
> +        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.
> 
>  ...
> --
> 2.29.1
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

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@raspberrypi.com>
> Signed-off-by: Jacopo Mondi <jacopo@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.
> 
>  ...

Hi Laurent,

On Tue, Dec 01, 2020 at 03:50:39PM +0200, Laurent Pinchart wrote:
> 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@raspberrypi.com>
> > Signed-off-by: Jacopo Mondi <jacopo@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 ?
>

No specific reasons, I can swap them

> > +
> > +        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 ?

Yes, "max mandatory cropping doesn't sound right".

>
>         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)/
>

Ack

> > +        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 ...".
>

Ack

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

Ack

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

Ack

> > +        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 ?

I meant the "smallest resolution in the current configuration"
and I think "the smallest" is right, but I get what you mean.

It's not easy to express this with words but let me try.

  What I meant is that the min crop rectangle should be equal to the
  analog crop rectangle of the smaller sensor mode the pipeline can
  select, when considered in isolation.

  I thought at this a static property mostly, that will give you the
  possibility to calculate the largest possible zoom factor, to be
  populated at camera initialization time (to make it possible to
  report ANDROID_SCALER_MAX_DIGITAL_ZOOM). When a mode is applied, the
  pipeline will of course select a sensor mode large enough to
  accommodate the largest stream configuration (assuming no
  up-scaling) and update this. To me it was implied, but I agree it's
  not clear.

So, I guess the thing boils down to the fact I want to use this also
to report the max digital zoom factor to android, and this is an
absolute value, so I envisioned pipelines to initialize this at the
smallest selectable sensor mode.

How should we reword this to make it fit for both usages ?

>
> > 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).
>

ok

> > +
> > +        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

Hi Laurent,
  gentle ping

On Tue, Dec 01, 2020 at 06:01:57PM +0100, Jacopo Mondi wrote:
> Hi Laurent,
>
> On Tue, Dec 01, 2020 at 03:50:39PM +0200, Laurent Pinchart wrote:
> > Hi Jacopo,
> >
> > Thank you for the patch.
> >

[snip]

> > > +        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 ?
>
> I meant the "smallest resolution in the current configuration"
> and I think "the smallest" is right, but I get what you mean.
>
> It's not easy to express this with words but let me try.
>
>   What I meant is that the min crop rectangle should be equal to the
>   analog crop rectangle of the smaller sensor mode the pipeline can
>   select, when considered in isolation.
>
>   I thought at this a static property mostly, that will give you the
>   possibility to calculate the largest possible zoom factor, to be
>   populated at camera initialization time (to make it possible to
>   report ANDROID_SCALER_MAX_DIGITAL_ZOOM). When a mode is applied, the
>   pipeline will of course select a sensor mode large enough to
>   accommodate the largest stream configuration (assuming no
>   up-scaling) and update this. To me it was implied, but I agree it's
>   not clear.
>
> So, I guess the thing boils down to the fact I want to use this also
> to report the max digital zoom factor to android, and this is an
> absolute value, so I envisioned pipelines to initialize this at the
> smallest selectable sensor mode.
>
> How should we reword this to make it fit for both usages ?
>

Hi Jacopo,

On Tue, Dec 01, 2020 at 06:01:57PM +0100, Jacopo Mondi wrote:
> On Tue, Dec 01, 2020 at 03:50:39PM +0200, Laurent Pinchart wrote:
> > 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@raspberrypi.com>
> > > Signed-off-by: Jacopo Mondi <jacopo@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 ?
> 
> No specific reasons, I can swap them
> 
> > > +
> > > +        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 ?
> 
> Yes, "max mandatory cropping doesn't sound right".
> 
> >         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)/
> >
> 
> Ack
> 
> > > +        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 ...".
> >
> 
> Ack
> 
> > > +        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.
> >
> 
> Ack
> 
> > > +        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.
> >
> 
> Ack
> 
> > > +        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 ?
> 
> I meant the "smallest resolution in the current configuration"
> and I think "the smallest" is right, but I get what you mean.
> 
> It's not easy to express this with words but let me try.
> 
>   What I meant is that the min crop rectangle should be equal to the
>   analog crop rectangle of the smaller sensor mode the pipeline can
>   select, when considered in isolation.

I don't think that's correct. The minimum crop rectangle is indeed
defined in pixel array coordinates, but it corresponds to the smallest
input size for the scaler, when considering the scaler output size and
its maximum upscaling factor, not the smallest analog crop rectangle the
sensor can produce (otherwise, for non mode-based sensors, that would
usually be a very very small value).

This can be a bit tricky, and easily misleading. Let's assume a sensor
that can bin, and a scaler at the very last stage of the ISP that can at
most downscale by 1/4 and upscale by x4. Let's further assume that
nothing in the pipeline applies additional cropping. This is likely
incorrect, but simplifies the example and doesn't affect the conclusion.
Finally, let's consider we capture a 1920x1080 stream.

The minimum crop rectangle size at the scaler input is 480x270 (1/4 of
1920x1080). Translated to sensor coordinates, this becomes 960x540 if
the sensor is configured with binning, or stays 480x270 otherwise. The
maximum digital zoom that the system can apply is thus the sensor
resolution (note how we haven't defined it yet) divided by 960x540 or by
480x270, depending on the sensor configuration selected by the pipeline
handler. We thus have a different maximum zoom depending on the camera
configuration.

Note that the maximium zoom factor is usually *not* x4 in this case,
even if that's the maximum upscaling factor of the sensor. Let's assume
a sensor resolution larger than 1920x1080, for instance 4056x3042. What
will a pipeline handler typically do ? There are two options.

- It can configure the sensor to output the full resolution. The aspect
  ratios don't match, so the image will be cropped to 4056x2282, and
  then *downscaled* to 1920x1080. When zooming in with the scaler, we
  can go down to a crop rectangle of 480x270. The maximum zoom factor is
  4056/480 = 2282/270 = 8.45. Note that if the application had decided
  to capture 1280x720, the minimum scaler crop would have been 320x180,
  and the maximum zoom factor 4056/320 = 2282/180 = 12.675.

- It can also configure the sensor to bin the image, for instance
  because the pipeline's bandwidth at full resolution would restrict the
  frame rate. The input to the scaler thus become 2028x1521, which would
  be cropped to 2028x1140 to product the correct aspect ratio. The image
  will again be downscaled to 1920x1080. When zomming in with the
  scaler, we can go down to a crop rectangle of 480x270. The maximum
  zoom factor is hus 2028/480 = 1140/270 = 4.225. If we translate the
  crop rectangle to sensor coordinates (as done by the scaler crop limit
  property), we get a rectangle of 960x540 pixels, but the maximum zoom
  factor is then calculated as 4056/960 = 2282/540 = 4.225, yielding the
  same value.

We assume there that the binning configuration of the sensor can't be
changed while streaming. Otherwise we would have two scalers that can be
combined, and the results would be different.

We also assume a sensor resolution larger than the stream size. This is
required by Android (upscaling isn't allowed when no digital zoom is
applied), but we don't have to set such a limitation in libcamera
itself. If the sensor resolution was smaller than the stream size, we
would already scale up when no digital zoom is applied, and the maximum
digital zoom factor would then be smaller than x4.

Finally, note that deciding whether to bin or not isn't always
completely up to the pipeline handler. When the scaler has minimum
downscaling ratio, binning may need to be applied to achieve small
output resolutions. With the above example, if the application wants to
capture 640x480, binning will be required as it would otherwise exceed
the limits of the scaler (it would require downscaling by 6.3375). This
is needed to fulfil Android's requirements (no digital zoom by default),
but is also not a limit that we need to enforce in libcamera (an
application may be interested in applying digital zoom between 1.584 and
25.35, instead of between 1.0 and 12.675).

>   I thought at this a static property mostly, that will give you the
>   possibility to calculate the largest possible zoom factor, to be
>   populated at camera initialization time (to make it possible to
>   report ANDROID_SCALER_MAX_DIGITAL_ZOOM). When a mode is applied, the
>   pipeline will of course select a sensor mode large enough to
>   accommodate the largest stream configuration (assuming no
>   up-scaling) and update this. To me it was implied, but I agree it's
>   not clear.
> 
> So, I guess the thing boils down to the fact I want to use this also
> to report the max digital zoom factor to android, and this is an
> absolute value, so I envisioned pipelines to initialize this at the
> smallest selectable sensor mode.

There are a few challenges in doing so, given the above, as the maximum
zoom factor depends on the camera configuration (it also gets more
complicated when we consider multiple streams). This requires a bit more
thinking. I'd be happy to brainstorm this further with you if that can
help.

> How should we reword this to make it fit for both usages ?

Let's agree on the above first :-)

> > > 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).
> >
> 
> ok
> 
> > > +
> > > +        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.
> > >
> > >  ...

Hi Laurent,

On Tue, Dec 15, 2020 at 06:38:33AM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>

[snip]

> > > > +
> > > > +        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 ?
> >
> > I meant the "smallest resolution in the current configuration"
> > and I think "the smallest" is right, but I get what you mean.
> >
> > It's not easy to express this with words but let me try.
> >
> >   What I meant is that the min crop rectangle should be equal to the
> >   analog crop rectangle of the smaller sensor mode the pipeline can
> >   select, when considered in isolation.
>
> I don't think that's correct. The minimum crop rectangle is indeed
> defined in pixel array coordinates, but it corresponds to the smallest
> input size for the scaler, when considering the scaler output size and
> its maximum upscaling factor, not the smallest analog crop rectangle the
> sensor can produce (otherwise, for non mode-based sensors, that would
> usually be a very very small value).

Maybe I don't fully get this but the text does not mention "the
smallest analog crop the sensor can produce" but rather
"the size of sensor pixel array portion used to produce the smallest
available stream resolution". All of this prefixed by "if a pipeline
cannot up-scale"

Probably it's also questionable if the "pipeline cannot up-scale" case
is really relevant when talking about digital zoom with one stream.

>
> This can be a bit tricky, and easily misleading. Let's assume a sensor
> that can bin, and a scaler at the very last stage of the ISP that can at
> most downscale by 1/4 and upscale by x4. Let's further assume that
> nothing in the pipeline applies additional cropping. This is likely
> incorrect, but simplifies the example and doesn't affect the conclusion.
> Finally, let's consider we capture a 1920x1080 stream.
>
> The minimum crop rectangle size at the scaler input is 480x270 (1/4 of
> 1920x1080). Translated to sensor coordinates, this becomes 960x540 if
> the sensor is configured with binning, or stays 480x270 otherwise. The
> maximum digital zoom that the system can apply is thus the sensor
> resolution (note how we haven't defined it yet) divided by 960x540 or by
> 480x270, depending on the sensor configuration selected by the pipeline
> handler. We thus have a different maximum zoom depending on the camera
> configuration.

The fact that it depends on the camera configuration to me was
implied, the current property definition is already dependent on the
configuration and reports it with:

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

Which I've replaced with

        \todo Express this property as the limits for the ScalerCrop control
        once "dynamic" controls have been implemented.

do you think the new definition makes the situation worse ?
I don't think it does for libcamera, but is problematic for android.

>
> Note that the maximium zoom factor is usually *not* x4 in this case,
> even if that's the maximum upscaling factor of the sensor. Let's assume

s/of the sensor/of the scaler/ right ? Or did I missed something ?

> a sensor resolution larger than 1920x1080, for instance 4056x3042. What
> will a pipeline handler typically do ? There are two options.
>
> - It can configure the sensor to output the full resolution. The aspect
>   ratios don't match, so the image will be cropped to 4056x2282, and
>   then *downscaled* to 1920x1080. When zooming in with the scaler, we
>   can go down to a crop rectangle of 480x270. The maximum zoom factor is
>   4056/480 = 2282/270 = 8.45. Note that if the application had decided
>   to capture 1280x720, the minimum scaler crop would have been 320x180,
>   and the maximum zoom factor 4056/320 = 2282/180 = 12.675.
>

Indeed.
Doesn't this example match the "size of the pixel array portion used
to produce the smallest available stream resolution" definition I gave
for the minimum crop rectangle ?

I think replacing 'smallest available stream resolution' with 'the smallest
of the stream resolutions part of a configuration' would make it more
clear ?

If I resume your examples here:
1) output=1920x1080, minScalerInput=480x270
        if 480x720 is 2x2 binned -> analogCrop = 960x540
                zoomFactor = (active pixel array / analog crop) = 4.225
2) output=1920x1080, minScalerInput=480x270
        if 480x720 is not binned -> analogCrop = 480x720
                zoomFactor = (active pixel array / analog crop) = 8.45
3) output=1280x720, minScalerInput=320x180
        if 320x180 is not binned -> analogCrop = 320x180
                zoomFactor = (active pixel array / analog crop) = 12.675

> - It can also configure the sensor to bin the image, for instance
>   because the pipeline's bandwidth at full resolution would restrict the
>   frame rate. The input to the scaler thus become 2028x1521, which would
>   be cropped to 2028x1140 to product the correct aspect ratio. The image
>   will again be downscaled to 1920x1080. When zomming in with the
>   scaler, we can go down to a crop rectangle of 480x270. The maximum
>   zoom factor is hus 2028/480 = 1140/270 = 4.225. If we translate the
>   crop rectangle to sensor coordinates (as done by the scaler crop limit
>   property), we get a rectangle of 960x540 pixels, but the maximum zoom
>   factor is then calculated as 4056/960 = 2282/540 = 4.225, yielding the
>   same value.

Isn't this actually correct ? If we can only obtain 480x270 by binning
and we translate it back to the sensor coordinates to 960x540 and we
do the same for the scaler input size (which is not relevant as we reason
in terms of pixel array size 2028x1521 2x2 binned = 4056x2282)

Again, this changes with the camera configuration, and depends on a
combination of the sensor capabilities (both the device capabilities
and the driver limitations) and the pipeline.

It doesn't seem contraditory to me, if not that establishing an
absolute zoom factor as requested by android is not possible or
easy. This can be solved in the HAL by testing the list of supported
configurations and inspecting the property to collect the maximum and
report it.

>
> We assume there that the binning configuration of the sensor can't be
> changed while streaming. Otherwise we would have two scalers that can be
> combined, and the results would be different.
>
> We also assume a sensor resolution larger than the stream size. This is
> required by Android (upscaling isn't allowed when no digital zoom is
> applied), but we don't have to set such a limitation in libcamera
> itself. If the sensor resolution was smaller than the stream size, we
> would already scale up when no digital zoom is applied, and the maximum
> digital zoom factor would then be smaller than x4.

Can we mark the "sensor resolution smaller than the largest stream"
use case as a corner case for devices that support android ?
It seems very unlikely to me that an Android device relies on
up-scaling to produce its largest stream. Am I overlooking something ?

>
> Finally, note that deciding whether to bin or not isn't always
> completely up to the pipeline handler. When the scaler has minimum
> downscaling ratio, binning may need to be applied to achieve small
> output resolutions. With the above example, if the application wants to
> capture 640x480, binning will be required as it would otherwise exceed
> the limits of the scaler (it would require downscaling by 6.3375). This
> is needed to fulfil Android's requirements (no digital zoom by default),
> but is also not a limit that we need to enforce in libcamera (an
> application may be interested in applying digital zoom between 1.584 and
> 25.35, instead of between 1.0 and 12.675).
>
> >   I thought at this a static property mostly, that will give you the
> >   possibility to calculate the largest possible zoom factor, to be
> >   populated at camera initialization time (to make it possible to
> >   report ANDROID_SCALER_MAX_DIGITAL_ZOOM). When a mode is applied, the
> >   pipeline will of course select a sensor mode large enough to
> >   accommodate the largest stream configuration (assuming no
> >   up-scaling) and update this. To me it was implied, but I agree it's
> >   not clear.
> >
> > So, I guess the thing boils down to the fact I want to use this also
> > to report the max digital zoom factor to android, and this is an
> > absolute value, so I envisioned pipelines to initialize this at the
> > smallest selectable sensor mode.
>
> There are a few challenges in doing so, given the above, as the maximum
> zoom factor depends on the camera configuration (it also gets more
> complicated when we consider multiple streams). This requires a bit more
> thinking. I'd be happy to brainstorm this further with you if that can
> help.
>

I think the definition for libcamera only is not worse than what we
had. It depends on the current configurtion indeed, but I don't see it
too much controversial.

For the HAL side, it can't probably be used as it is. It can be worked
around in the HAL (not easily, as static metadata should be reported
even before the camera is acquired) or exposed through a draft
property. Or we can decide to make regular property to allow pipeline
handlers to report their maximum absolute zoom factor if they have any
known and easily computable limits (unfortunately I doubt so).

This apart, I extending this property to make it useful to calculate
the maximum possible zoom factor -per camera configuration- still has
merits.

> > How should we reword this to make it fit for both usages ?
>
> Let's agree on the above first :-)
>
> > > > 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).
> > >
> >
> > ok
> >
> > > > +
> > > > +        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