[libcamera-devel] [PATCH] libcamera: properties: Re-define ScalerCropMaximum
Jacopo Mondi
jacopo at jmondi.org
Mon Nov 30 12:51:18 CET 2020
Hi David,
On Mon, Nov 30, 2020 at 09:42:54AM +0000, David Plowman wrote:
> Hi again Jacopo
>
> Thanks for the update!
>
> On Sat, 28 Nov 2020 at 10:26, Jacopo Mondi <jacopo at jmondi.org> wrote:
> >
> > 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
>
> The important thing is to recover the scaling factors in the sensor,
> that is, how many input pixels does one output pixel correspond to? So
> I think what you've said sounds correct, as we don't want unicam to
> have any effect.
>
> >
> > > 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 ?
>
> Well, maybe just a mention somewhere, just to calm down people like me!
>
> >
> > >
> > > >
> > > > 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 ?
>
> I agree. Having both as rectangles in the same property feels nice.
> That meaningless top left coordinate is just a bit weird, but I guess
> we don't worry about it!
>
> >
> > > > +
> > > > + 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.
>
> So I believe all our sensors (ov5647, imx219, imx477) will run in 2x2
> binning mode when started just by typing "qcam". So, as the minimum
> crop size is 64x64 into the ISP, we should get 128x128.
Thanks, I've just tested the above and I get:
-srole=viewfinder (2x2 binning)
Selected mode: 1640x1232-pRAA
ScalerCropLimits: max = (0x0)/3280x2464 min = (0x0)/128x128
-srole=viewfinder, -srole=raw (no binning)
Selected mode: 3280x2464-pRAA
ScalerCropLimits: max = (0x0)/3280x2464 min = (0x0)/64x64
I'll report this in the v2 commit message maybe
Thanks
j
>
> If you run "qcam -s role=viewfinder -s role=raw" we should get the
> full resolution, so the rectangle should come out as 64x64.
>
> (I do wonder whether we should be placing a restriction of, say, 16x,
> for upscaling in our pipeline for other reasons - but I think that's
> our business and doesn't affect the discussion here.)
>
> Thanks and best regards
> David
>
> >
> > 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
> > > >
More information about the libcamera-devel
mailing list