[libcamera-devel] [PATCH v2] pipeline: raspberrypi: Fix incorrect advertising of ScalerCrop

[Naushir Patuck]

Hi Jacopo,

This is all very confusing for me as well, so I might be wrong in my
explanation :-)

On Tue, 12 Jul 2022 at 08:23, Jacopo Mondi <jacopo at jmondi.org> wrote:

> Hi Naush,
>
> On Tue, Jul 05, 2022 at 02:59:56PM +0100, Naushir Patuck via
> libcamera-devel wrote:
> > The controls::ScalerCrop in the ControlInfoMap was advertised based on
> the ISP
> > output Rectangle. This is incorrect, it needs to be set based on the
> sensor
> > output Rectangle. Fix this.
>
> I might have not completely woken up yet but the existing
>
>         ctrlMap.emplace(&controls::ScalerCrop,
>                         ControlInfo(Rectangle(data->ispMinCropSize_),
> Rectangle(data->sensorInfo_.outputSize)));
>
> Seems to be based on the sensor output size, not the ISP output size
> as you mention here.
>
> What am I missing ?
>

ispMinCropSize_ is based on the ISP's minimum output size (64x64).  This
does not have
any relation to the sensor size, so....


>
> >
> > Additionally, do not use emplace to be consistent with the other
> controls set
> > in the ControlInfoMap.
> >
> > Fixes: 9dacde0d651d (pipeline: raspberrypi: Advertise ScalerCrop from
> the pipeline handler)
> > Reported-by: David Plowman <david.plowman at raspberrypi.com>
> > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > ---
> >  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > index 66a84b1dfb97..fdc24cd530c2 100644
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > @@ -946,8 +946,9 @@ int PipelineHandlerRPi::configure(Camera *camera,
> CameraConfiguration *config)
> >               ctrlMap.emplace(c.first, c.second);
> >
> >       /* Add the ScalerCrop control limits based on the current mode. */
> > -     ctrlMap.emplace(&controls::ScalerCrop,
> > -                     ControlInfo(Rectangle(data->ispMinCropSize_),
> Rectangle(data->sensorInfo_.outputSize)));
> > +     Rectangle ispMinCrop(data->ispMinCropSize_);
> > +     ispMinCrop.scaleBy(data->sensorInfo_.analogCrop.size(),
> data->sensorInfo_.outputSize);
>
> What is the purpose of scaling the minimum accepted ISP input in the
> sensor's analogue crop / sensor's output size ratio ?
>

.... this bit of code scales the ispMinCropSize_  (64x64) relative to the
sensor input.


>
> > +     ctrlMap[&controls::ScalerCrop] = ControlInfo(ispMinCrop,
> Rectangle(data->sensorInfo_.analogCrop.size()));
>
> And I don't get this one either, how can the maximum scaler rectangle
> be larger than the sensor's output size ? (assumiming
> sensor->analogCrop > sensor->outputSize).
>

If I understand correctly, the ScalerCrop control is based on the
"unbinned" resolution,
i.e. full analogue crop readout of the sensor.  The translation to sensor
output scale
happens internally in the pipeline handler here:
https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp#n2052

For imx477, this is the result:

Sensor mode 4056x3040:
ScalerCrop : [(0, 0)/64x64..(0, 0)/4056x3040]

Sensor mode 2028x1520:
ScalerCrop : [(0, 0)/128x128..(0, 0)/4056x3040]

Sensor mode 2028x1080:
ScalerCrop : [(0, 0)/128x128..(0, 0)/4056x2160]

Sensor mode 1332x990:
ScalerCrop : [(0, 0)/128x128..(0, 0)/2664x1980]

I'm slightly confused :)
>

I was as well :-) David had to patiently work though all this with me to
get an understanding on what's going on.

Regards,
Naush


> >
> >       data->controlInfo_ = ControlInfoMap(std::move(ctrlMap),
> result.controlInfo.idmap());
> >
> > --
> > 2.25.1
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20220712/82dff518/attachment.htm>