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

[David Plowman]

Hi Laurent

Thanks for looking at this!

On Tue, 1 Mar 2022 at 14:35, Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hello,
>
> On Fri, Feb 11, 2022 at 10:58:56AM +0000, Kieran Bingham wrote:
> > Quoting David Plowman (2022-02-09 12:08:04)
> > > We must calculate the initial scaler crop when the camera is
> > > configured, otherwise the metadata will report this rectangle as being
> > > all zeroes.
> >
> > I tried to check to see if this is something that should be initialised
> > earlier. I'd almost be tempted to make an init() for the RPi Camera Data
> > class, to make sure there's a common location to initialise all of it's
> > data ..., but I don't think that actually helps for this patch (unless
> > we re-initialise things before reconfiguring?).
> >
> > I think in configure is ok here too, as it's not something that should
> > be changed in validate(), and I think it's something that may need to
> > reset/configured correctly after any previous configuration that may
> > have had a different scalerCrop_ set.
> >
> > So ...
> >
> > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> >
> > > Signed-off-by: David Plowman <david.plowman at raspberrypi.com>
> > > ---
> > >  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 5 +++++
> > >  1 file changed, 5 insertions(+)
> > >
> > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > index 583ee798..d6c57e7a 100644
> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > @@ -887,6 +887,11 @@ 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 that we are using. */
> > > +       data->scalerCrop_ = data->ispCrop_.scaledBy(data->sensorInfo_.analogCrop.size(),
> > > +                                                   data->sensorInfo_.outputSize);
> > > +       data->scalerCrop_.translateBy(data->sensorInfo_.analogCrop.topLeft());
> > > +
>
> This is the same calculation as in RPiCameraData::applyScalerCrop().
> Should it be factored out to a separate function ?

Actually this did cross my mind, but for the sake of two statements I
ended up being a bit lazy! But let me submit a v2 that does this, and
perhaps we will prefer it.

Thanks
David

>
> > >         /*
> > >          * Configure the Unicam embedded data output format only if the sensor
> > >          * supports it.
>
> --
> Regards,
>
> Laurent Pinchart