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

[Laurent Pinchart]

Hello,

On Tue, Jul 12, 2022 at 11:26:30AM +0100, Naushir Patuck via libcamera-devel wrote:
> On Tue, 12 Jul 2022 at 11:20, Jacopo Mondi wrote:
> > 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 wrote:
> > > > 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!  Happy to change the commit message when applying :)

I'll handle it.

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

-- 
Regards,

Laurent Pinchart