[libcamera-devel] [PATCH v2] libcamera: pipeline: raspberrypi: Fix scaler crop when sensor is configured

[Kieran Bingham]

Hi David,

Quoting David Plowman (2022-03-04 16:29:16)
> Hi Laurent
> 
> Thanks for the review.
> 
> On Fri, 4 Mar 2022 at 16:14, Laurent Pinchart
> <laurent.pinchart at ideasonboard.com> wrote:
> >
> > Hi David,
> >
> > Thank you for the patch.
> >
> > On Thu, Mar 03, 2022 at 02:32:01PM +0000, David Plowman wrote:
> > > On Thu, 3 Mar 2022 at 14:20, Kieran Bingham wrote:
> > > > 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). */
> >
> > This line could be wrapped.
> 
> Yes, agree.
> 
> >
> > > > > +       data->scalerCrop_ = data->scaleIspCrop(data->ispCrop_);
> >
> > The function could also use ispCrop_ internally instead of receiving it
> > as a parameter.
> 
> I did consider this, actually. I vaguely wondered whether one might in
> future have some code where you might want to see what the native crop
> would be without actually setting it. But possibly I'm committing the
> common mistake of over-generalising something without actually having
> a reason, so I'm good to make that change too.

I've merged with the comment wrapped, but kept the variables here as
they are.

--
Kieran


> 
> >
> > Both of these can be done when applying (if desired).
> 
> So yes please, and thanks again!
> David
> 
> >
> > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> >
> > > > > +
> > > > >         /*
> > > > >          * 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...
> > >
> > > > 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_);
> > > > >                 }
> > > > >         }
> > > > >  }
> >
> > --
> > Regards,
> >
> > Laurent Pinchart