Hi Naush,
Thank you for the patch.
On Thu, Aug 08, 2024 at 11:23:43AM +0100, Naushir Patuck wrote:
> In preparation for assigning separate crop windows for each stream, add
> a new CropParams structure that stores the existing ispCrop_ and
> ispMinCropSize_ as fields. Use a new std::map to store a CropParams
> structure where the map key is the index of the stream configuration in
> the CameraConfiguration vector.
>
> At preset, only a single CropParams structure will be set at key == 0 to
> preserve the existing crop handling logic.
>
> Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> ---
> .../pipeline/rpi/common/pipeline_base.cpp | 23 +++++++++++--------
> .../pipeline/rpi/common/pipeline_base.h | 21 +++++++++++++++--
> src/libcamera/pipeline/rpi/vc4/vc4.cpp | 13 ++++++++---
> 3 files changed, 43 insertions(+), 14 deletions(-)
>
> diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> index 2de6111bacfd..412e71648231 100644
> --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> @@ -561,10 +561,12 @@ int PipelineHandlerBase::configure(Camera *camera, CameraConfiguration *config)
> for (auto const &c : result.controlInfo)
> ctrlMap.emplace(c.first, c.second);
>
> - /* 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->scaleIspCrop(data->ispCrop_));
> + if (data->cropParams_.count(0)) {
You can avoid the double lookup with
const auto cropParams = data->cropParams_.find(0);
if (cropParams != data->cropParams_.end()) {
and use cropParams below.
> + /* Add the ScalerCrop control limits based on the current mode. */
> + Rectangle ispMinCrop = data->scaleIspCrop(Rectangle(data->cropParams_[0].ispMinCropSize));
> + ctrlMap[&controls::ScalerCrop] = ControlInfo(ispMinCrop, data->sensorInfo_.analogCrop,
> + data->scaleIspCrop(data->cropParams_[0].ispCrop));
> + }
>
> data->controlInfo_ = ControlInfoMap(std::move(ctrlMap), result.controlInfo.idmap());
>
> @@ -1291,7 +1293,8 @@ Rectangle CameraData::scaleIspCrop(const Rectangle &ispCrop) const
> void CameraData::applyScalerCrop(const ControlList &controls)
> {
> const auto &scalerCrop = controls.get<Rectangle>(controls::ScalerCrop);
> - if (scalerCrop) {
> + if (scalerCrop && cropParams_.count(0)) {
> + CropParams &cropParams = cropParams_[0];
Same here.
> Rectangle nativeCrop = *scalerCrop;
>
> if (!nativeCrop.width || !nativeCrop.height)
> @@ -1308,12 +1311,12 @@ void CameraData::applyScalerCrop(const ControlList &controls)
> * 2. With the same mid-point, if possible.
> * 3. But it can't go outside the sensor area.
> */
> - Size minSize = ispMinCropSize_.expandedToAspectRatio(nativeCrop.size());
> + Size minSize = cropParams.ispMinCropSize.expandedToAspectRatio(nativeCrop.size());
> Size size = ispCrop.size().expandedTo(minSize);
> ispCrop = size.centeredTo(ispCrop.center()).enclosedIn(Rectangle(sensorInfo_.outputSize));
>
> - if (ispCrop != ispCrop_) {
> - ispCrop_ = ispCrop;
> + if (ispCrop != cropParams.ispCrop) {
> + cropParams.ispCrop = ispCrop;
> platformSetIspCrop(ispCrop);
> }
> }
> @@ -1471,7 +1474,9 @@ void CameraData::fillRequestMetadata(const ControlList &bufferControls, Request
> request->metadata().set(controls::SensorTimestamp,
> bufferControls.get(controls::SensorTimestamp).value_or(0));
>
> - request->metadata().set(controls::ScalerCrop, scaleIspCrop(ispCrop_));
> + if (cropParams_.count(0))
> + request->metadata().set(controls::ScalerCrop,
> + scaleIspCrop(cropParams_[0].ispCrop));
> }
>
> } /* namespace libcamera */
> diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.h b/src/libcamera/pipeline/rpi/common/pipeline_base.h
> index d65b695c30b5..2bed905178bc 100644
> --- a/src/libcamera/pipeline/rpi/common/pipeline_base.h
> +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.h
> @@ -133,8 +133,25 @@ public:
>
> /* For handling digital zoom. */
> IPACameraSensorInfo sensorInfo_;
> - Rectangle ispCrop_; /* crop in ISP (camera mode) pixels */
> - Size ispMinCropSize_;
> +
> + struct CropParams {
> + CropParams(Rectangle ispCrop_, Size ispMinCropSize_)
> + : ispCrop(ispCrop_), ispMinCropSize(ispMinCropSize_)
> + {
> + }
> +
> + CropParams()
> + {
> + }
> +
> + /* Crop in ISP (camera mode) pixels */
> + Rectangle ispCrop;
> + /* Minimum crop size in ISP output pixels */
> + Size ispMinCropSize;
> + };
> +
> + /* Mapping of CropParams keyed by the stream index in CameraConfiguration */
> + std::map<unsigned int, CropParams> cropParams_;
>
> unsigned int dropFrameCount_;
>
> diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp b/src/libcamera/pipeline/rpi/vc4/vc4.cpp
> index 0ea032293bc9..d118252f02dd 100644
> --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp
> +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp
> @@ -702,13 +702,20 @@ int Vc4CameraData::platformConfigure(const RPi::RPiCameraConfiguration *rpiConfi
> /* Figure out the smallest selection the ISP will allow. */
> Rectangle testCrop(0, 0, 1, 1);
> isp_[Isp::Input].dev()->setSelection(V4L2_SEL_TGT_CROP, &testCrop);
> - ispMinCropSize_ = testCrop.size();
>
> /* Adjust aspect ratio by providing crops on the input image. */
> Size size = unicamFormat.size.boundedToAspectRatio(maxSize);
> - ispCrop_ = size.centeredTo(Rectangle(unicamFormat.size).center());
> + Rectangle ispCrop = size.centeredTo(Rectangle(unicamFormat.size).center());
>
> - platformSetIspCrop(ispCrop_);
> + platformSetIspCrop(ispCrop);
> + /*
> + * Set the scaler crop to the value we are using (scaled to native sensor
> + * coordinates).
> + */
> + cropParams_.clear();
This line could possibly go to the common implementation of
PipelineHandlerBase::configure(). Up to you.
Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> + cropParams_.emplace(std::piecewise_construct,
> + std::forward_as_tuple(0),
> + std::forward_as_tuple(scaleIspCrop(ispCrop), testCrop.size()));
>
> return 0;
> }
--
Regards,
Laurent Pinchart