[libcamera-devel] [PATCH v3] libcamera: properties: Re-define ScalerCropMaximum
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Dec 15 05:38:33 CET 2020
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 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 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.
> > >
> > > ...
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list