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

[Naushir Patuck]

Hi Jacopo,

On Tue, 12 Jul 2022 at 11:20, Jacopo Mondi <jacopo at jmondi.org> 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 <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!  Happy to change the commit message when applying :)

Regards,
Naush


>
> Thanks
>    j
>
> >
> > 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/4f324a6a/attachment.htm>