| Message ID | 20240808102346.13065-3-naush@raspberrypi.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series |
|
| Related | show |
Hi Naush, Thank you for the patch. On Thu, Aug 08, 2024 at 11:23:41AM +0100, Naushir Patuck wrote: > Do not cache the scalerCrop_ parameter. The cached value is used to > update the request metadata, but since this is not an expensive > operation (and can only occur once per frame), caching it is of limited > value. > > This will simplify logic in a future commit where we can specify a > crop per-output stream. > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > .../pipeline/rpi/common/pipeline_base.cpp | 17 +++-------------- > .../pipeline/rpi/common/pipeline_base.h | 1 - > 2 files changed, 3 insertions(+), 15 deletions(-) > > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > index 3041fd1ed9fd..5322fd798a36 100644 > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > @@ -544,12 +544,6 @@ int PipelineHandlerBase::configure(Camera *camera, CameraConfiguration *config) > return ret; > } > > - /* > - * Set the scaler crop to the value we are using (scaled to native sensor > - * coordinates). > - */ > - data->scalerCrop_ = data->scaleIspCrop(data->ispCrop_); > - > /* > * Update the ScalerCropMaximum to the correct value for this camera mode. > * For us, it's the same as the "analogue crop". > @@ -569,7 +563,8 @@ int PipelineHandlerBase::configure(Camera *camera, CameraConfiguration *config) > > /* Add the ScalerCrop control limits based on the current mode. */ > Rectangle ispMinCrop = data->scaleIspCrop(Rectangle(data->ispMinCropSize_)); > - ctrlMap[&controls::ScalerCrop] = ControlInfo(ispMinCrop, data->sensorInfo_.analogCrop, data->scalerCrop_); > + ctrlMap[&controls::ScalerCrop] = ControlInfo(ispMinCrop, data->sensorInfo_.analogCrop, > + data->scaleIspCrop(data->ispCrop_)); > > data->controlInfo_ = ControlInfoMap(std::move(ctrlMap), result.controlInfo.idmap()); > > @@ -1321,12 +1316,6 @@ void CameraData::applyScalerCrop(const ControlList &controls) > ispCrop_ = ispCrop; > platformSetIspCrop(); > > - /* > - * Also update the ScalerCrop in the metadata with what we actually > - * used. But we must first rescale that from ISP (camera mode) pixels > - * back into sensor native pixels. > - */ > - scalerCrop_ = scaleIspCrop(ispCrop_); > } > } > } > @@ -1483,7 +1472,7 @@ void CameraData::fillRequestMetadata(const ControlList &bufferControls, Request > request->metadata().set(controls::SensorTimestamp, > bufferControls.get(controls::SensorTimestamp).value_or(0)); > > - request->metadata().set(controls::ScalerCrop, scalerCrop_); > + request->metadata().set(controls::ScalerCrop, scaleIspCrop(ispCrop_)); > } > > } /* namespace libcamera */ > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.h b/src/libcamera/pipeline/rpi/common/pipeline_base.h > index f9cecf70f179..5161c16e518f 100644 > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.h > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.h > @@ -134,7 +134,6 @@ public: > /* For handling digital zoom. */ > IPACameraSensorInfo sensorInfo_; > Rectangle ispCrop_; /* crop in ISP (camera mode) pixels */ > - Rectangle scalerCrop_; /* crop in sensor native pixels */ > Size ispMinCropSize_; > > unsigned int dropFrameCount_;
diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp index 3041fd1ed9fd..5322fd798a36 100644 --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp @@ -544,12 +544,6 @@ int PipelineHandlerBase::configure(Camera *camera, CameraConfiguration *config) return ret; } - /* - * Set the scaler crop to the value we are using (scaled to native sensor - * coordinates). - */ - data->scalerCrop_ = data->scaleIspCrop(data->ispCrop_); - /* * Update the ScalerCropMaximum to the correct value for this camera mode. * For us, it's the same as the "analogue crop". @@ -569,7 +563,8 @@ int PipelineHandlerBase::configure(Camera *camera, CameraConfiguration *config) /* Add the ScalerCrop control limits based on the current mode. */ Rectangle ispMinCrop = data->scaleIspCrop(Rectangle(data->ispMinCropSize_)); - ctrlMap[&controls::ScalerCrop] = ControlInfo(ispMinCrop, data->sensorInfo_.analogCrop, data->scalerCrop_); + ctrlMap[&controls::ScalerCrop] = ControlInfo(ispMinCrop, data->sensorInfo_.analogCrop, + data->scaleIspCrop(data->ispCrop_)); data->controlInfo_ = ControlInfoMap(std::move(ctrlMap), result.controlInfo.idmap()); @@ -1321,12 +1316,6 @@ void CameraData::applyScalerCrop(const ControlList &controls) ispCrop_ = ispCrop; platformSetIspCrop(); - /* - * Also update the ScalerCrop in the metadata with what we actually - * used. But we must first rescale that from ISP (camera mode) pixels - * back into sensor native pixels. - */ - scalerCrop_ = scaleIspCrop(ispCrop_); } } } @@ -1483,7 +1472,7 @@ void CameraData::fillRequestMetadata(const ControlList &bufferControls, Request request->metadata().set(controls::SensorTimestamp, bufferControls.get(controls::SensorTimestamp).value_or(0)); - request->metadata().set(controls::ScalerCrop, scalerCrop_); + request->metadata().set(controls::ScalerCrop, scaleIspCrop(ispCrop_)); } } /* namespace libcamera */ diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.h b/src/libcamera/pipeline/rpi/common/pipeline_base.h index f9cecf70f179..5161c16e518f 100644 --- a/src/libcamera/pipeline/rpi/common/pipeline_base.h +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.h @@ -134,7 +134,6 @@ public: /* For handling digital zoom. */ IPACameraSensorInfo sensorInfo_; Rectangle ispCrop_; /* crop in ISP (camera mode) pixels */ - Rectangle scalerCrop_; /* crop in sensor native pixels */ Size ispMinCropSize_; unsigned int dropFrameCount_;
Do not cache the scalerCrop_ parameter. The cached value is used to update the request metadata, but since this is not an expensive operation (and can only occur once per frame), caching it is of limited value. This will simplify logic in a future commit where we can specify a crop per-output stream. Signed-off-by: Naushir Patuck <naush@raspberrypi.com> --- .../pipeline/rpi/common/pipeline_base.cpp | 17 +++-------------- .../pipeline/rpi/common/pipeline_base.h | 1 - 2 files changed, 3 insertions(+), 15 deletions(-)