[libcamera-devel] [PATCH v2] libcamera: pipeline: raspberrypi: Fix scaler crop when sensor is configured
David Plowman
david.plowman at raspberrypi.com
Thu Mar 3 15:32:01 CET 2022
Hi Kieran
Thanks for review number 2!
On Thu, 3 Mar 2022 at 14:20, Kieran Bingham
<kieran.bingham at ideasonboard.com> wrote:
>
> Hi David,
>
> Quoting David Plowman (2022-03-03 12:11:13)
> > We must calculate the initial scaler crop when the camera is
> > configured, otherwise the metadata will report this rectangle as being
> > all zeroes.
> >
> > Because the calculation is identical to that performed later in handling
> > the scaler crop control, we factor it into a small helper function,
> > RPiCameraData::scaleIspCrop.
> >
> > Signed-off-by: David Plowman <david.plowman at raspberrypi.com>
> > ---
> > .../pipeline/raspberrypi/raspberrypi.cpp | 20 ++++++++++++++++---
> > 1 file changed, 17 insertions(+), 3 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > index 29bff9d6..eede78e6 100644
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > @@ -212,6 +212,7 @@ public:
> > void handleStreamBuffer(FrameBuffer *buffer, RPi::Stream *stream);
> > void handleExternalBuffer(FrameBuffer *buffer, RPi::Stream *stream);
> > void handleState();
> > + Rectangle scaleIspCrop(const Rectangle &ispCrop) const;
> > void applyScalerCrop(const ControlList &controls);
> >
> > std::unique_ptr<ipa::RPi::IPAProxyRPi> ipa_;
> > @@ -887,6 +888,9 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
> > if (ret)
> > LOG(RPI, Error) << "Failed to configure the IPA: " << ret;
> >
> > + /* Set the scaler crop to the value we are using (scaled to native sensor coordinates). */
> > + data->scalerCrop_ = data->scaleIspCrop(data->ispCrop_);
> > +
> > /*
> > * Configure the Unicam embedded data output format only if the sensor
> > * supports it.
> > @@ -1974,6 +1978,18 @@ void RPiCameraData::checkRequestCompleted()
> > }
> > }
> >
> > +Rectangle RPiCameraData::scaleIspCrop(const Rectangle &ispCrop) const
> > +{
> > + /*
> > + * Scale a crop rectangle defined in the ISP's coordinates into native sensor
> > + * coordinates.
> > + */
> > + Rectangle nativeCrop = ispCrop.scaledBy(sensorInfo_.analogCrop.size(),
> > + sensorInfo_.outputSize);
> > + nativeCrop.translateBy(sensorInfo_.analogCrop.topLeft());
>
> I'm struggling to be sure, but this is what the code was before, so I'm
> sure it's fine... should the amount we translate by also be scaled?
I think it's right as it is. sensorInfo_.analogCrop is necessarily in
the "sensor's native coordinates", which nativeCrop also is by dint of
the line above, so it looks plausible to me...
David
>
> Or perhaps that's just to maintain the original cropping point.
>
> > + return nativeCrop;
>
> I know it was only two lines, but I certainly prefer this, as it's now
> it's more clear (and more documented, and self-documenting) for what is
> being returned.
>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>
>
> > +}
> > +
> > void RPiCameraData::applyScalerCrop(const ControlList &controls)
> > {
> > if (controls.contains(controls::ScalerCrop)) {
> > @@ -2006,9 +2022,7 @@ void RPiCameraData::applyScalerCrop(const ControlList &controls)
> > * used. But we must first rescale that from ISP (camera mode) pixels
> > * back into sensor native pixels.
> > */
> > - scalerCrop_ = ispCrop_.scaledBy(sensorInfo_.analogCrop.size(),
> > - sensorInfo_.outputSize);
> > - scalerCrop_.translateBy(sensorInfo_.analogCrop.topLeft());
> > + scalerCrop_ = scaleIspCrop(ispCrop_);
> > }
> > }
> > }
> > --
> > 2.30.2
> >
More information about the libcamera-devel
mailing list