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

[Jacopo Mondi]

Hi David,

On Fri, Nov 27, 2020 at 04:53:22PM +0000, David Plowman wrote:
> Hi Jacopo
>
> Thanks for tackling this one again, glad it's not me! Just one or two
> minor observations, and one more significant thing.
>

Thanks for your precious inputs

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

Mmmm, I made the resoning that the ISP input was an absolute value.
Ie if your ISP does not accept anything smaller than 100x100, you
can't specify a rectangle smaller than this on the sensor pixel array.

But I get what you mean here: with a 2x2 binning in use, selecting a
(theoretically valid) 100x100 rectangle on the pixel array will give
you an output size of 50x50 (I presume it's more complex than this,
but plese bear with me with this simplified example).

>
> Rectangle sensorMinCrop =
> Rectangle(data->ispMinCropSize_).scaledBy(data->sensorInfo_.analogCrop.size(),
> sensorFormat.size);

I think the idea is right.
I wonder if we should not use sensorInfo_.ouputSize and not
sensorFormat.size, as the latter is the format to be applied to unicam

> data->properties_.set(properties::ScalerCropLimits, {
> data->sensorInfo_.analogCrop, sensorMinCrop.size() });
>
> but I haven't tried it and it's probably full of typos.
>
> A little part of me also wonders what the offset, sorry, *top left* of
> the minimum Rectangle means. Presumably it doesn't mean anything?

Yes, the min limit would be better expressed as a Size, but I liked
the symmetry of having two rectangles. I should better specify the
top-left corner position is not relevant ?

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

Mmm, what I meant was:
        ScalerCropLimits[0] = larger valid ScalerCrop rectangle
                            = minimum possible zoom/crop
        ScalerCropLimits[1] = smaller valid ScalerCrop rectangle
                            = maximum possible zoom/crop

Would it be less confusing if in the text description I use
"The larger and smaller valid rectangles" in place of
"The maximum and minimum valid rectangles" ?

>
> > +        mandatory cropping applied in the camera sensor and the rest of the
> > +        pipeline. Both Rectangles are defined relatively to the sensor's active
> > +        pixel array (properties::PixelArrayActiveAreas).
>
> Do we need to mention that the top left of the minimum doesn't mean anything?
>

Yes we should make it clear.

Or use a Size, but I like these two being rectangles part of a single
property. What do you think ?

> > +
> > +        Implementation details for the maximum valid crop rectangle.
> > +        The maximum valid crop rectangle depends on the camera configuration as
> > +        pipelines are free to adjust the frame size requested from the sensor
> > +        and used to produced the final image streams. The largest possible crop
>
> s/produced/produce/
>
> > +        rectangle is then limited by the size of the sensor's active pixel area
> > +        portion used to produce the sensor output frame.
> > +
> > +        Implementation details for the minimum valid crop rectangle.
> > +        If a pipeline cannot up-scale the minimum valid crop rectangle is equal
>
> I think you need a comma after "up-scale" (my internal language parser
> crashed when I reached "is equal"!)
>

Ah yes indeed.

> > +        to the smallest available stream resolution (plus any border pixels
> > +        required by the ISP for image processing) part of the current camera
> > +        configuration. If a pipeline can up-scale, the minimum valid crop
>
> Ah, it's right here!  :)
>
> Thanks again, and best regards

Thank you.

>From the top of your mind, as you know your platform better than me,
can you suggest a configuration (for imx219) that would let me test
the correctness of the reported limits ? I'm thinking about a mode
that uses binning and where the lower limit has to be scaled back to
the sensor's native pixel array coordinates.

Thanks
   j

> David
>
> > +        rectangle is equal to the smallest frame size accepted by the ISP input.
> > +
> > +        The Rectangles are valid only after the camera has been successfully
> > +        configured and their value may change whenever a new configuration is
> >          applied.
> >
> > -        \todo Turn this property into a "maximum control value" for the
> > -        ScalerCrop control once "dynamic" controls have been implemented.
> > +        \todo Express this property as the limits for the ScalerCrop control
> > +        once "dynamic" controls have been implemented.
> >
> >  ...
> > --
> > 2.29.1
> >