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

[Jacopo Mondi]

On Tue, Jul 12, 2022 at 10:03:58AM +0100, Naushir Patuck wrote:
> 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.

I was missing that ScalerCrop is specified relatively to the analogue
gain rectangle, so re-scaling it in the (analogue crop / sensor
output) ratio makes sure the sensor's mode binning/subsampling is
taken into account.

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

Weird, the driver says this mode is 4x4 binned in one place but it's
instead cropped down to 2664x1980 then 2x2 binned.

Anyway, these nicely match the driver's analogue crop rectangles.

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

Thanks for having the patience to re-explain this to me.

If I'm not mistaken then the commit message should say:

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.
+ on the sensor analogue crop Rectangle. Fix this.

The patch looks good and the commit message can be updated (if you
agree with the change) when applying.

Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>

Thanks
   j

>
> Regards,
> Naush
>
>
> > >
> > >       data->controlInfo_ = ControlInfoMap(std::move(ctrlMap),
> > result.controlInfo.idmap());
> > >
> > > --
> > > 2.25.1
> > >
> >