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

[Jacopo Mondi]

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

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