| Message ID | 20220303121113.16839-1-david.plowman@raspberrypi.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
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@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? 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@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 >
Hi Kieran Thanks for review number 2! On Thu, 3 Mar 2022 at 14:20, Kieran Bingham <kieran.bingham@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@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@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 > >
Hi David, Thank you for your patch. On Thu, 3 Mar 2022 at 12:11, David Plowman <david.plowman@raspberrypi.com> wrote: > 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@raspberrypi.com> > Reviewed-by: Naushir Patuck <naush@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()); > + return nativeCrop; > +} > + > 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 > >
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@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. > > > + data->scalerCrop_ = data->scaleIspCrop(data->ispCrop_); The function could also use ispCrop_ internally instead of receiving it as a parameter. Both of these can be done when applying (if desired). Reviewed-by: Laurent Pinchart <laurent.pinchart@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@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_); > > > } > > > } > > > }
Hi Laurent Thanks for the review. On Fri, 4 Mar 2022 at 16:14, Laurent Pinchart <laurent.pinchart@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@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. > > Both of these can be done when applying (if desired). So yes please, and thanks again! David > > Reviewed-by: Laurent Pinchart <laurent.pinchart@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@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
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@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@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@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@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
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()); + return nativeCrop; +} + 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_); } } }
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@raspberrypi.com> --- .../pipeline/raspberrypi/raspberrypi.cpp | 20 ++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-)