| Message ID | 20220209120804.21352-1-david.plowman@raspberrypi.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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@ideasonboard.com> > Signed-off-by: David Plowman <david.plowman@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()); > + > /* > * Configure the Unicam embedded data output format only if the sensor > * supports it. > -- > 2.30.2 >
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@ideasonboard.com> > > > Signed-off-by: David Plowman <david.plowman@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 ? > > /* > > * Configure the Unicam embedded data output format only if the sensor > > * supports it.
Hi Laurent Thanks for looking at this! On Tue, 1 Mar 2022 at 14:35, Laurent Pinchart <laurent.pinchart@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@ideasonboard.com> > > > > > Signed-off-by: David Plowman <david.plowman@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
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()); + /* * Configure the Unicam embedded data output format only if the sensor * supports it.
We must calculate the initial scaler crop when the camera is configured, otherwise the metadata will report this rectangle as being all zeroes. Signed-off-by: David Plowman <david.plowman@raspberrypi.com> --- src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 5 +++++ 1 file changed, 5 insertions(+)