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

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

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
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 the updated patch!

On Mon, 30 Nov 2020 at 12:44, 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.
>
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
> 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

Perfect!

>
> ---
>  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, they're 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 });
>

Looks good to me!

>         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..97904a666c0d 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 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 to

Still seem to be missing my comma after up-scale!

> +        the size of sensor pixel array portion used to produce the smallest
> +        available stream resolution, accounting any applied pixel sub-sampling

s/accounting/accounting for/

> +        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 pixel array portion that produces the smallest

s/to/to the/

Apart from the little grammar things:

Reviewed-by: David Plowman <david.plowman@raspberrypi.com>

Thanks and best regards
David

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

On Mon, Nov 30, 2020 at 02:34:03PM +0000, David Plowman wrote:
> Hi Jacopo
>
> Thanks for the updated patch!
>
> On Mon, 30 Nov 2020 at 12:44, 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.
> >
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> > 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
>
> Perfect!
>
> >
> > ---
> >  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, they're 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 });
> >
>
> Looks good to me!
>
> >         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..97904a666c0d 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 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 to
>
> Still seem to be missing my comma after up-scale!

Ouch!

>
> > +        the size of sensor pixel array portion used to produce the smallest
> > +        available stream resolution, accounting any applied pixel sub-sampling
>
> s/accounting/accounting for/
>
> > +        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 pixel array portion that produces the smallest
>
> s/to/to the/
>
> Apart from the little grammar things:
>
> Reviewed-by: David Plowman <david.plowman@raspberrypi.com>

Thanks, will collect your suggestions on v3.

Thanks
  j

>
> Thanks and best regards
> David
>
> > +        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
> >