[libcamera-devel,v4,5/5] libcamera: pipeline: raspberrypi: Implementation of digital zoom

10099 diff mbox series

During configure() we update the ScalerCropMaximum to the correct
value for this camera mode (which we also note internally) and work
out the minimum crop size allowed by the ISP.

Whenever a new ScalerCrop request is received we check it's valid and
apply it to the ISP V4L2 device. We also forward it to the IPA so that
the IPA can return the values used in the image metadata.

Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
---
 include/libcamera/ipa/raspberrypi.h           |  1 +
 src/ipa/raspberrypi/raspberrypi.cpp           |  7 ++
 .../pipeline/raspberrypi/raspberrypi.cpp      | 70 +++++++++++++++----
 3 files changed, 63 insertions(+), 15 deletions(-)

Hi David,

Thank you for the patch.

On Mon, Oct 19, 2020 at 01:51:56PM +0100, David Plowman wrote:
> During configure() we update the ScalerCropMaximum to the correct
> value for this camera mode (which we also note internally) and work
> out the minimum crop size allowed by the ISP.
> 
> Whenever a new ScalerCrop request is received we check it's valid and
> apply it to the ISP V4L2 device. We also forward it to the IPA so that
> the IPA can return the values used in the image metadata.
> 
> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> ---
>  include/libcamera/ipa/raspberrypi.h           |  1 +
>  src/ipa/raspberrypi/raspberrypi.cpp           |  7 ++
>  .../pipeline/raspberrypi/raspberrypi.cpp      | 70 +++++++++++++++----
>  3 files changed, 63 insertions(+), 15 deletions(-)
> 
> diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h
> index b3041591..bb4e1a8c 100644
> --- a/include/libcamera/ipa/raspberrypi.h
> +++ b/include/libcamera/ipa/raspberrypi.h
> @@ -60,6 +60,7 @@ static const ControlInfoMap Controls = {
>  	{ &controls::Saturation, ControlInfo(0.0f, 32.0f) },
>  	{ &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },
>  	{ &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) },
> +	{ &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },
>  };
>  
>  } /* namespace RPi */
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index 48a72dd2..e777f0b4 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -699,6 +699,13 @@ void IPARPi::queueRequest(const ControlList &controls)
>  			break;
>  		}
>  
> +		case controls::SCALER_CROP: {
> +			/* Just copy the information back. */
> +			Rectangle crop = ctrl.second.get<Rectangle>();
> +			libcameraMetadata_.set(controls::ScalerCrop, crop);
> +			break;
> +		}
> +
>  		default:
>  			LOG(IPARPI, Warning)
>  				<< "Ctrl " << controls::controls.at(ctrl.first)->name()
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 26dbd257..43b860ae 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -193,6 +193,11 @@ public:
>  	bool flipsAlterBayerOrder_;
>  	BayerFormat::Order nativeBayerOrder_;
>  
> +	/* For handling digital zoom. */
> +	CameraSensorInfo sensorInfo_;
> +	Rectangle lastIspCrop_;
> +	Size ispMinSize_;
> +
>  	unsigned int dropFrameCount_;
>  
>  private:
> @@ -677,26 +682,30 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
>  		return ret;
>  	}
>  
> -	/* Adjust aspect ratio by providing crops on the input image. */
> -	Rectangle crop{ 0, 0, sensorFormat.size };
> -
> -	int ar = maxSize.height * sensorFormat.size.width - maxSize.width * sensorFormat.size.height;
> -	if (ar > 0)
> -		crop.width = maxSize.width * sensorFormat.size.height / maxSize.height;
> -	else if (ar < 0)
> -		crop.height = maxSize.height * sensorFormat.size.width / maxSize.width;
> +	/* Figure out the smallest selection the ISP will allow. */
> +	Rectangle testCrop(0, 0, 1, 1);
> +	data->isp_[Isp::Input].dev()->setSelection(V4L2_SEL_TGT_CROP, &testCrop);
> +	data->ispMinSize_ = testCrop.size();

Can this vary based on the configuration, or could it be done at match()
time ?

>  
> -	crop.width &= ~1;
> -	crop.height &= ~1;
> +	/* Adjust aspect ratio by providing crops on the input image. */
> +	Rectangle crop = sensorFormat.size.boundedToAspectRatio(maxSize).centeredTo(sensorFormat.size.center());

The line is a bit long, should it be wrapped ?

	Rectangle crop = sensorFormat.size.boundedToAspectRatio(maxSize)
					  .centeredTo(sensorFormat.size.center());

> +	data->lastIspCrop_ = crop;
>  
> -	crop.x = (sensorFormat.size.width - crop.width) >> 1;
> -	crop.y = (sensorFormat.size.height - crop.height) >> 1;
>  	data->isp_[Isp::Input].dev()->setSelection(V4L2_SEL_TGT_CROP, &crop);
>  
>  	ret = data->configureIPA(config);
>  	if (ret)
>  		LOG(RPI, Error) << "Failed to configure the IPA: " << ret;
>  
> +	/*
> +	 * Update the ScalerCropMaximum to the correct value for this camera mode.
> +	 * For us, it's the same as the "analogue crop".
> +	 *
> +	 * \todo Make this property the ScalerCrop maximum value when dynamic
> +	 * controls are available and set it at validate() time
> +	 */
> +	data->properties_.set(properties::ScalerCropMaximum, data->sensorInfo_.analogCrop);
> +
>  	return ret;
>  }
>  
> @@ -1147,8 +1156,8 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)
>  		ipaConfig.data.push_back(static_cast<unsigned int>(lsTable_.fd()));
>  	}
>  
> -	CameraSensorInfo sensorInfo = {};
> -	int ret = sensor_->sensorInfo(&sensorInfo);
> +	/* We store the CameraSensorInfo for digital zoom calculations. */
> +	int ret = sensor_->sensorInfo(&sensorInfo_);
>  	if (ret) {
>  		LOG(RPI, Error) << "Failed to retrieve camera sensor info";
>  		return ret;
> @@ -1157,7 +1166,7 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)
>  	/* Ready the IPA - it must know about the sensor resolution. */
>  	IPAOperationData result;
>  
> -	ipa_->configure(sensorInfo, streamConfig, entityControls, ipaConfig,
> +	ipa_->configure(sensorInfo_, streamConfig, entityControls, ipaConfig,
>  			&result);
>  
>  	unsigned int resultIdx = 0;
> @@ -1588,6 +1597,37 @@ void RPiCameraData::tryRunPipeline()
>  	/* Take the first request from the queue and action the IPA. */
>  	Request *request = requestQueue_.front();
>  
> +	if (request->controls().contains(controls::ScalerCrop)) {
> +		Rectangle crop = request->controls().get<Rectangle>(controls::ScalerCrop);
> +
> +		if (crop.width && crop.height) {
> +			/* First scale the crop from sensor native to camera mode pixels. */
> +			crop.translateBy(-sensorInfo_.analogCrop.x, -sensorInfo_.analogCrop.y);
> +			crop.rescaleTo(sensorInfo_.analogCrop.size(), sensorInfo_.outputSize);
> +
> +			/*
> +			 * The crop that we set must be:
> +			 * 1. At least as big as ispMinSize_, once that's been
> +			 *    enlarged to the same aspect ratio.
> +			 * 2. With the same mid-point, if possible.
> +			 * 3. But it can't go outside the sensor area.
> +			 */
> +			Size minSize = ispMinSize_.expandedToAspectRatio(crop.size());
> +			Size size = crop.size().expandedTo(minSize);
> +			crop = size.centeredTo(crop.center()).clampedTo(Rectangle(sensorInfo_.outputSize));
> +
> +			if (crop != lastIspCrop_)
> +				isp_[Isp::Input].dev()->setSelection(V4L2_SEL_TGT_CROP, &crop);
> +			lastIspCrop_ = crop;
> +		}
> +
> +		/* We must scale the actual crop used back into native pixels. */
> +		crop = lastIspCrop_.rescaledTo(sensorInfo_.outputSize, sensorInfo_.analogCrop.size());
> +		crop.translateBy(sensorInfo_.analogCrop.x, sensorInfo_.analogCrop.y);

It takes a bit of time to process this, but I think it's quite fine,
there's a good amount of descriptive comments, and the function names
help. Great job :-) I agree with the cover letter that it may be a bit
nicer to write

	crop.translateBy(-sensorInfo_.analogCrop.topLeft());

and

	crop.translateBy(sensorInfo_.analogCrop.topLeft());

> +
> +		request->controls().set(controls::ScalerCrop, crop);

This bothers me a little bit, as we have not considered the pipeline
handler to be allowed to modify the controls stored in the request. If I
understand the code correctly, this is only used to pass the crop
rectangle to the IPA. I see two possible options:

- Pass the computed crop rectangle explicitly through IPAOperationData.
  It's not amazing as it requires serializing it explicitly in the data
  vector, but that would be temporary only, given Paul's work on the IPA
  interface that will allow custom parameters to be passed to IPA
  operations in a much better way.

- Don't pass the crop rectangle to the IPA, and set the metadata here
  directly. The IPA currently doesn't use the crop rectangle, but this
  may change later. Still, for the time being, this is an option.

I don't have a preference.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +	}
> +
>  	/*
>  	 * Process all the user controls by the IPA. Once this is complete, we
>  	 * queue the ISP output buffer listed in the request to start the HW