<div dir="ltr"><div dir="ltr">Hi Jacopo,<div><br></div><div>This is all very confusing for me as well, so I might be wrong in my</div><div>explanation :-)</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, 12 Jul 2022 at 08:23, 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">Hi Naush,<br>
<br>
On Tue, Jul 05, 2022 at 02:59:56PM +0100, Naushir Patuck via libcamera-devel wrote:<br>
> The controls::ScalerCrop in the ControlInfoMap was advertised based on the ISP<br>
> output Rectangle. This is incorrect, it needs to be set based on the 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_), 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></blockquote><div><br></div><div>ispMinCropSize_ is based on the ISP's minimum output size (64x64). This does not have</div><div>any relation to the sensor size, so....</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>
><br>
> Additionally, do not use emplace to be consistent with the other controls set<br>
> in the ControlInfoMap.<br>
><br>
> Fixes: 9dacde0d651d (pipeline: raspberrypi: Advertise ScalerCrop from 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 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, 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_), Rectangle(data->sensorInfo_.outputSize)));<br>
> + Rectangle ispMinCrop(data->ispMinCropSize_);<br>
> + ispMinCrop.scaleBy(data->sensorInfo_.analogCrop.size(), 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></blockquote><div><br></div><div>.... this bit of code scales the ispMinCropSize_ (64x64) relative to the sensor input.</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>
> + ctrlMap[&controls::ScalerCrop] = ControlInfo(ispMinCrop, 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></blockquote><div><br></div><div>If I understand correctly, the ScalerCrop control is based on the "unbinned" resolution,</div><div>i.e. full analogue crop readout of the sensor. The translation to sensor output scale</div><div>happens internally in the pipeline handler here:</div><div><a href="https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp#n2052">https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp#n2052</a><br></div><div><br></div><div>For imx477, this is the result:</div><div> </div><div>Sensor mode 4056x3040:</div><div>ScalerCrop : [(0, 0)/64x64..(0, 0)/4056x3040]<br></div><div><br></div><div>Sensor mode 2028x1520:</div><div>ScalerCrop : [(0, 0)/128x128..(0, 0)/4056x3040]<br></div><div><br></div><div>Sensor mode 2028x1080:</div><div>ScalerCrop : [(0, 0)/128x128..(0, 0)/4056x2160]<br></div><div><br></div><div>Sensor mode 1332x990:</div><div>ScalerCrop : [(0, 0)/128x128..(0, 0)/2664x1980]<br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
I'm slightly confused :)<br></blockquote><div><br></div><div>I was as well :-) David had to patiently work though all this with me to get an understanding on what's going on.</div><div><br></div><div>Regards,</div><div>Naush</div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
><br>
> data->controlInfo_ = ControlInfoMap(std::move(ctrlMap), result.controlInfo.idmap());<br>
><br>
> --<br>
> 2.25.1<br>
><br>
</blockquote></div></div>