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

[Jacopo Mondi]

Hi Laurent,

On Tue, Dec 15, 2020 at 06:38:33AM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>

[snip]

> > > > +
> > > > +        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).

Maybe I don't fully get this but the text does not mention "the
smallest analog crop the sensor can produce" but rather
"the size of sensor pixel array portion used to produce the smallest
available stream resolution". All of this prefixed by "if a pipeline
cannot up-scale"

Probably it's also questionable if the "pipeline cannot up-scale" case
is really relevant when talking about digital zoom with one stream.

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

The fact that it depends on the camera configuration to me was
implied, the current property definition is already dependent on the
configuration and reports it with:

        \todo Turn this property into a "maximum control value" for the
        ScalerCrop control once "dynamic" controls have been implemented.

Which I've replaced with

        \todo Express this property as the limits for the ScalerCrop control
        once "dynamic" controls have been implemented.

do you think the new definition makes the situation worse ?
I don't think it does for libcamera, but is problematic for android.

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

s/of the sensor/of the scaler/ right ? Or did I missed something ?

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

Indeed.
Doesn't this example match the "size of the pixel array portion used
to produce the smallest available stream resolution" definition I gave
for the minimum crop rectangle ?

I think replacing 'smallest available stream resolution' with 'the smallest
of the stream resolutions part of a configuration' would make it more
clear ?

If I resume your examples here:
1) output=1920x1080, minScalerInput=480x270
        if 480x720 is 2x2 binned -> analogCrop = 960x540
                zoomFactor = (active pixel array / analog crop) = 4.225
2) output=1920x1080, minScalerInput=480x270
        if 480x720 is not binned -> analogCrop = 480x720
                zoomFactor = (active pixel array / analog crop) = 8.45
3) output=1280x720, minScalerInput=320x180
        if 320x180 is not binned -> analogCrop = 320x180
                zoomFactor = (active pixel array / analog crop) = 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.

Isn't this actually correct ? If we can only obtain 480x270 by binning
and we translate it back to the sensor coordinates to 960x540 and we
do the same for the scaler input size (which is not relevant as we reason
in terms of pixel array size 2028x1521 2x2 binned = 4056x2282)

Again, this changes with the camera configuration, and depends on a
combination of the sensor capabilities (both the device capabilities
and the driver limitations) and the pipeline.

It doesn't seem contraditory to me, if not that establishing an
absolute zoom factor as requested by android is not possible or
easy. This can be solved in the HAL by testing the list of supported
configurations and inspecting the property to collect the maximum and
report it.

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

Can we mark the "sensor resolution smaller than the largest stream"
use case as a corner case for devices that support android ?
It seems very unlikely to me that an Android device relies on
up-scaling to produce its largest stream. Am I overlooking something ?

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

I think the definition for libcamera only is not worse than what we
had. It depends on the current configurtion indeed, but I don't see it
too much controversial.

For the HAL side, it can't probably be used as it is. It can be worked
around in the HAL (not easily, as static metadata should be reported
even before the camera is acquired) or exposed through a draft
property. Or we can decide to make regular property to allow pipeline
handlers to report their maximum absolute zoom factor if they have any
known and easily computable limits (unfortunately I doubt so).

This apart, I extending this property to make it useful to calculate
the maximum possible zoom factor -per camera configuration- still has
merits.

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