| Message ID | 20220705135956.30444-1-naush@raspberrypi.com |
|---|---|
| State | Accepted |
| Commit | 090ac6941679a49ea3c96de492a9f0ec00b2dd0c |
| Headers | show |
| Series |
|
| Related | show |
Hi Naush Thanks for the new version! On Tue, 5 Jul 2022 at 14:59, Naushir Patuck <naush@raspberrypi.com> 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. > > 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@raspberrypi.com> > Signed-off-by: Naushir Patuck <naush@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); > + ctrlMap[&controls::ScalerCrop] = ControlInfo(ispMinCrop, Rectangle(data->sensorInfo_.analogCrop.size())); Yep, I think I'm good with this now! Reviewed-by: David Plowman <david.plowman@raspberrypi.com> Tested-by: David Plowman <david.plowman@raspberrypi.com> Thanks David > > data->controlInfo_ = ControlInfoMap(std::move(ctrlMap), result.controlInfo.idmap()); > > -- > 2.25.1 >
Hi, Ping for a review please. Naush On Tue, 5 Jul 2022 at 14:59, Naushir Patuck <naush@raspberrypi.com> 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. > > 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@raspberrypi.com> > Signed-off-by: Naushir Patuck <naush@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); > + ctrlMap[&controls::ScalerCrop] = ControlInfo(ispMinCrop, > Rectangle(data->sensorInfo_.analogCrop.size())); > > data->controlInfo_ = ControlInfoMap(std::move(ctrlMap), > result.controlInfo.idmap()); > > -- > 2.25.1 > >
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 ? > > 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@raspberrypi.com> > Signed-off-by: Naushir Patuck <naush@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 ? > + 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). I'm slightly confused :) > > data->controlInfo_ = ControlInfoMap(std::move(ctrlMap), result.controlInfo.idmap()); > > -- > 2.25.1 >
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@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@raspberrypi.com> > > Signed-off-by: Naushir Patuck <naush@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. > > > + 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] 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. Regards, Naush > > > > data->controlInfo_ = ControlInfoMap(std::move(ctrlMap), > result.controlInfo.idmap()); > > > > -- > > 2.25.1 > > >
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@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@raspberrypi.com> > > > Signed-off-by: Naushir Patuck <naush@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@jmondi.org> Thanks j > > Regards, > Naush > > > > > > > > data->controlInfo_ = ControlInfoMap(std::move(ctrlMap), > > result.controlInfo.idmap()); > > > > > > -- > > > 2.25.1 > > > > >
Hi Jacopo, On Tue, 12 Jul 2022 at 11:20, Jacopo Mondi <jacopo@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@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@raspberrypi.com> > > > > Signed-off-by: Naushir Patuck <naush@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@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 > > > > > > > >
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@raspberrypi.com> > > > > > Signed-off-by: Naushir Patuck <naush@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@jmondi.org> > > Thanks! Happy to change the commit message when applying :) I'll handle it. > > > > > data->controlInfo_ = ControlInfoMap(std::move(ctrlMap), result.controlInfo.idmap()); > > > > >
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); + ctrlMap[&controls::ScalerCrop] = ControlInfo(ispMinCrop, Rectangle(data->sensorInfo_.analogCrop.size())); data->controlInfo_ = ControlInfoMap(std::move(ctrlMap), result.controlInfo.idmap());
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. 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@raspberrypi.com> Signed-off-by: Naushir Patuck <naush@raspberrypi.com> --- src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)