<div dir="ltr"><div dir="ltr">Hi Jacopo,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, 12 Jul 2022 at 11:20, Jacopo Mondi <<a href="mailto:jacopo@jmondi.org">jacopo@jmondi.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Tue, Jul 12, 2022 at 10:03:58AM +0100, Naushir Patuck wrote:<br>
> Hi Jacopo,<br>
><br>
> This is all very confusing for me as well, so I might be wrong in my<br>
> explanation :-)<br>
><br>
> On Tue, 12 Jul 2022 at 08:23, Jacopo Mondi <<a href="mailto:jacopo@jmondi.org" target="_blank">jacopo@jmondi.org</a>> wrote:<br>
><br>
> > Hi Naush,<br>
> ><br>
> > On Tue, Jul 05, 2022 at 02:59:56PM +0100, Naushir Patuck via<br>
> > libcamera-devel wrote:<br>
> > > The controls::ScalerCrop in the ControlInfoMap was advertised based on<br>
> > the ISP<br>
> > > output Rectangle. This is incorrect, it needs to be set based on the<br>
> > sensor<br>
> > > output Rectangle. Fix this.<br>
> ><br>
> > I might have not completely woken up yet but the existing<br>
> ><br>
> >         ctrlMap.emplace(&controls::ScalerCrop,<br>
> >                         ControlInfo(Rectangle(data->ispMinCropSize_),<br>
> > Rectangle(data->sensorInfo_.outputSize)));<br>
> ><br>
> > Seems to be based on the sensor output size, not the ISP output size<br>
> > as you mention here.<br>
> ><br>
> > What am I missing ?<br>
> ><br>
><br>
> ispMinCropSize_ is based on the ISP's minimum output size (64x64).  This<br>
> does not have<br>
> any relation to the sensor size, so....<br>
><br>
><br>
> ><br>
> > ><br>
> > > Additionally, do not use emplace to be consistent with the other<br>
> > controls set<br>
> > > in the ControlInfoMap.<br>
> > ><br>
> > > Fixes: 9dacde0d651d (pipeline: raspberrypi: Advertise ScalerCrop from<br>
> > the pipeline handler)<br>
> > > Reported-by: David Plowman <<a href="mailto:david.plowman@raspberrypi.com" target="_blank">david.plowman@raspberrypi.com</a>><br>
> > > Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
> > > ---<br>
> > >  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 5 +++--<br>
> > >  1 file changed, 3 insertions(+), 2 deletions(-)<br>
> > ><br>
> > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> > > index 66a84b1dfb97..fdc24cd530c2 100644<br>
> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> > > @@ -946,8 +946,9 @@ int PipelineHandlerRPi::configure(Camera *camera,<br>
> > CameraConfiguration *config)<br>
> > >               ctrlMap.emplace(c.first, c.second);<br>
> > ><br>
> > >       /* Add the ScalerCrop control limits based on the current mode. */<br>
> > > -     ctrlMap.emplace(&controls::ScalerCrop,<br>
> > > -                     ControlInfo(Rectangle(data->ispMinCropSize_),<br>
> > Rectangle(data->sensorInfo_.outputSize)));<br>
> > > +     Rectangle ispMinCrop(data->ispMinCropSize_);<br>
> > > +     ispMinCrop.scaleBy(data->sensorInfo_.analogCrop.size(),<br>
> > data->sensorInfo_.outputSize);<br>
> ><br>
> > What is the purpose of scaling the minimum accepted ISP input in the<br>
> > sensor's analogue crop / sensor's output size ratio ?<br>
> ><br>
><br>
> .... this bit of code scales the ispMinCropSize_  (64x64) relative to the<br>
> sensor input.<br>
<br>
I was missing that ScalerCrop is specified relatively to the analogue<br>
gain rectangle, so re-scaling it in the (analogue crop / sensor<br>
output) ratio makes sure the sensor's mode binning/subsampling is<br>
taken into account.<br>
<br>
><br>
><br>
> ><br>
> > > +     ctrlMap[&controls::ScalerCrop] = ControlInfo(ispMinCrop,<br>
> > Rectangle(data->sensorInfo_.analogCrop.size()));<br>
> ><br>
> > And I don't get this one either, how can the maximum scaler rectangle<br>
> > be larger than the sensor's output size ? (assumiming<br>
> > sensor->analogCrop > sensor->outputSize).<br>
> ><br>
><br>
> If I understand correctly, the ScalerCrop control is based on the<br>
> "unbinned" resolution,<br>
> i.e. full analogue crop readout of the sensor.  The translation to sensor<br>
> output scale<br>
> happens internally in the pipeline handler here:<br>
> <a href="https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp#n2052" rel="noreferrer" target="_blank">https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp#n2052</a><br>
><br>
> For imx477, this is the result:<br>
><br>
> Sensor mode 4056x3040:<br>
> ScalerCrop : [(0, 0)/64x64..(0, 0)/4056x3040]<br>
><br>
> Sensor mode 2028x1520:<br>
> ScalerCrop : [(0, 0)/128x128..(0, 0)/4056x3040]<br>
><br>
> Sensor mode 2028x1080:<br>
> ScalerCrop : [(0, 0)/128x128..(0, 0)/4056x2160]<br>
><br>
> Sensor mode 1332x990:<br>
> ScalerCrop : [(0, 0)/128x128..(0, 0)/2664x1980]<br>
<br>
Weird, the driver says this mode is 4x4 binned in one place but it's<br>
instead cropped down to 2664x1980 then 2x2 binned.<br>
<br>
Anyway, these nicely match the driver's analogue crop rectangles.<br>
<br>
><br>
> I'm slightly confused :)<br>
> ><br>
><br>
> I was as well :-) David had to patiently work though all this with me to<br>
> get an understanding on what's going on.<br>
<br>
Thanks for having the patience to re-explain this to me.<br>
<br>
If I'm not mistaken then the commit message should say:<br>
<br>
The controls::ScalerCrop in the ControlInfoMap was advertised based on<br>
the ISP output Rectangle. This is incorrect, it needs to be set based<br>
- on the sensor output Rectangle. Fix this.<br>
+ on the sensor analogue crop Rectangle. Fix this.<br>
<br>
The patch looks good and the commit message can be updated (if you<br>
agree with the change) when applying.<br>
<br>
Reviewed-by: Jacopo Mondi <<a href="mailto:jacopo@jmondi.org" target="_blank">jacopo@jmondi.org</a>><br></blockquote><div><br></div><div>Thanks!  Happy to change the commit message when applying :)</div><div><br></div><div>Regards,</div><div>Naush</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Thanks<br>
   j<br>
<br>
><br>
> Regards,<br>
> Naush<br>
><br>
><br>
> > ><br>
> > >       data->controlInfo_ = ControlInfoMap(std::move(ctrlMap),<br>
> > result.controlInfo.idmap());<br>
> > ><br>
> > > --<br>
> > > 2.25.1<br>
> > ><br>
> ><br>
</blockquote></div></div>