[PATCH v3 7/7] pipeline: rpi: Handler controls::rpi::ScalerCrops

[Laurent Pinchart]

Hi Naush,

Thank you for the patch.

On Thu, Oct 03, 2024 at 08:47:20AM +0100, Naushir Patuck wrote:
> Handle multiple scaler crops being set through the rpi::ScalerCrops
> control. We now populate the cropParams_ map in the loop where we handle
> the output stream configuration items. The key of this map is the index
> of the stream configuration structure set by the application. This will
> also be the same index used to specify the crop rectangles through the
> ScalerCrops control.
> 
> CameraData::applyScalerCrop() has been adapted to look at either
> controls::ScalerCrop or controls::rpi::ScalerCrops. The former takes
> priority over the latter, and if present, will apply the same scaler
> crop to all output streams.

That contradicts patch 1/7 that states

        If both rpi::ScalerCrops and ScalerCrop controls are present in a
        ControlList, the latter is discarded, and crops are obtained from this
        control.

and also contradicts the code below.

> 
> Finally return all crops through the same ScalerCrops control via
> request metadata. The first configure stream's crop rectangle is also
> returned via the ScalerCrop control in the request metadata.
> 
> Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> ---
>  .../pipeline/rpi/common/pipeline_base.cpp     | 76 ++++++++++++++-----
>  1 file changed, 59 insertions(+), 17 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> index 267e6bd9cd70..eba4dad6d212 100644
> --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> @@ -181,12 +181,14 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()
>  
>  	rawStreams_.clear();
>  	outStreams_.clear();
> +	unsigned int rawStreamIndex = 0;
> +	unsigned int outStreamIndex = 0;
>  
> -	for (const auto &[index, cfg] : utils::enumerate(config_)) {
> +	for (auto &cfg : config_) {
>  		if (PipelineHandlerBase::isRaw(cfg.pixelFormat))
> -			rawStreams_.emplace_back(index, &cfg);
> +			rawStreams_.emplace_back(rawStreamIndex++, &cfg);
>  		else
> -			outStreams_.emplace_back(index, &cfg);
> +			outStreams_.emplace_back(outStreamIndex++, &cfg);

This changes the indices, but as far as I can tell, they're currently
unused, so that should be fine. Do I assume correctly the will be used
in the Pi 5 platformConfigure() implementation to populate cropParams_
in the right order ?

>  	}
>  
>  	/* Sort the streams so the highest resolution is first. */
> @@ -565,10 +567,24 @@ int PipelineHandlerBase::configure(Camera *camera, CameraConfiguration *config)
>  	const auto cropParamsIt = data->cropParams_.find(0);
>  	if (cropParamsIt != data->cropParams_.end()) {
>  		const CameraData::CropParams &cropParams = cropParamsIt->second;
> -		/* Add the ScalerCrop control limits based on the current mode. */
> +		/*
> +		 * Add the ScalerCrop control limits based on the current mode and
> +		 * the first configured stream.
> +		 */
>  		Rectangle ispMinCrop = data->scaleIspCrop(Rectangle(cropParams.ispMinCropSize));
>  		ctrlMap[&controls::ScalerCrop] = ControlInfo(ispMinCrop, data->sensorInfo_.analogCrop,
>  							     data->scaleIspCrop(cropParams.ispCrop));
> +		if (data->cropParams_.size() == 2) {
> +			/*
> +			 * The control map for rpi::ScalerCrops has the min value
> +			 * as the default crop for stream 0, max value as the default
> +			 * value for stream 1.

That sounds like a real hack. I'm fine with it for now, but should it be
documented in 1/7 ? Or would you rather not have anyone depending on
this ?

> +			 */
> +			ctrlMap[&controls::rpi::ScalerCrops] =
> +				ControlInfo(data->scaleIspCrop(data->cropParams_.at(0).ispCrop),
> +					    data->scaleIspCrop(data->cropParams_.at(1).ispCrop),
> +					    ctrlMap[&controls::ScalerCrop].def());
> +		}
>  	}
>  
>  	data->controlInfo_ = ControlInfoMap(std::move(ctrlMap), result.controlInfo.idmap());
> @@ -1295,11 +1311,29 @@ Rectangle CameraData::scaleIspCrop(const Rectangle &ispCrop) const
>  
>  void CameraData::applyScalerCrop(const ControlList &controls)
>  {
> -	const auto &scalerCrop = controls.get<Rectangle>(controls::ScalerCrop);
> -	const auto cropParamsIt = cropParams_.find(0);
> -	if (scalerCrop && cropParamsIt != cropParams_.end()) {
> -		Rectangle nativeCrop = *scalerCrop;
> -		CropParams &cropParams = cropParamsIt->second;
> +	const auto &scalerCropRPi = controls.get<Span<const Rectangle>>(controls::rpi::ScalerCrops);
> +	const auto &scalerCropCore = controls.get<Rectangle>(controls::ScalerCrop);
> +	std::vector<Rectangle> scalerCrops;
> +
> +	/*
> +	 * First thing to do is create a vector of crops to apply to each ISP output
> +	 * based on either controls::ScalerCrop or controls::rpi::ScalerCrops if
> +	 * present.
> +	 *
> +	 * If controls::ScalerCrop is present, apply the same crop to all ISP output
> +	 * streams. Otherwise if controls::rpi::ScalerCrops, apply the given crops
> +	 * to the ISP output streams, indexed by the same order in which they had
> +	 * been configured. This is not the same as the ISP output index.

This also contradicts the code below. You can pick the begaviour you
want, but let's make documentation, comments, commit messages and code
consistent.

> +	 */
> +	for (unsigned int i = 0; i < cropParams_.size(); i++) {
> +		if (scalerCropRPi && i < scalerCropRPi->size())
> +			scalerCrops.push_back(scalerCropRPi->data()[i]);
> +		else if (scalerCropCore)
> +			scalerCrops.push_back(*scalerCropCore);
> +	}
> +
> +	for (auto const &[i, scalerCrop] : utils::enumerate(scalerCrops)) {
> +		Rectangle nativeCrop = scalerCrop;
>  
>  		if (!nativeCrop.width || !nativeCrop.height)
>  			nativeCrop = { 0, 0, 1, 1 };
> @@ -1315,13 +1349,13 @@ 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 = cropParams.ispMinCropSize.expandedToAspectRatio(nativeCrop.size());
> +		Size minSize = cropParams_.at(i).ispMinCropSize.expandedToAspectRatio(nativeCrop.size());
>  		Size size = ispCrop.size().expandedTo(minSize);
>  		ispCrop = size.centeredTo(ispCrop.center()).enclosedIn(Rectangle(sensorInfo_.outputSize));
>  
> -		if (ispCrop != cropParams.ispCrop) {
> -			cropParams.ispCrop = ispCrop;
> -			platformSetIspCrop(cropParams.ispIndex, ispCrop);
> +		if (ispCrop != cropParams_.at(i).ispCrop) {
> +			cropParams_.at(i).ispCrop = ispCrop;
> +			platformSetIspCrop(cropParams_.at(i).ispIndex, ispCrop);
>  		}
>  	}
>  }
> @@ -1478,10 +1512,18 @@ void CameraData::fillRequestMetadata(const ControlList &bufferControls, Request
>  	request->metadata().set(controls::SensorTimestamp,
>  				bufferControls.get(controls::SensorTimestamp).value_or(0));
>  
> -	const auto cropParamsIt = cropParams_.find(0);
> -	if (cropParamsIt != cropParams_.end())
> -		request->metadata().set(controls::ScalerCrop,
> -					scaleIspCrop(cropParamsIt->second.ispCrop));
> +	if (cropParams_.size()) {
> +		std::vector<Rectangle> crops;
> +
> +		for (auto const &[k, v] : cropParams_)
> +			crops.push_back(scaleIspCrop(v.ispCrop));
> +
> +		request->metadata().set(controls::ScalerCrop, crops[0]);
> +		if (crops.size() > 1) {
> +			request->metadata().set(controls::rpi::ScalerCrops,
> +						Span<const Rectangle>(crops.data(), crops.size()));
> +		}
> +	}
>  }
>  
>  } /* namespace libcamera */

-- 
Regards,

Laurent Pinchart