[libcamera-devel] [PATCH v3 5/6] libcamera: pipeline: raspberrypi: Implementation of digital zoom

[Kieran Bingham]

Hi David,

On 29/09/2020 17:39, David Plowman wrote:
> During configure() we update the SensorOutputSize 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 IspCrop 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 at raspberrypi.com>
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  include/libcamera/ipa/raspberrypi.h           |  1 +
>  src/ipa/raspberrypi/raspberrypi.cpp           |  7 +++
>  .../pipeline/raspberrypi/raspberrypi.cpp      | 47 +++++++++++++++++++
>  3 files changed, 55 insertions(+)
> 
> diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h
> index b3041591..9ef5f6fc 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::IspCrop, 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 b0c7d1c1..dc73c20b 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::ISP_CROP: {
> +			/* Just copy the information back. */
> +			Rectangle crop = ctrl.second.get<Rectangle>();
> +			libcameraMetadata_.set(controls::IspCrop, 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 d2bee150..4a6d0bf4 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. */
> +	Size ispMinSize_;
> +	Size sensorOutputSize_;
> +	Rectangle lastIspCrop_;
> +
>  	unsigned int dropFrameCount_;
>  
>  private:
> @@ -587,6 +592,14 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
>  	 */
>  	ret = data->isp_[Isp::Input].dev()->setFormat(&sensorFormat);
>  
> +	/*
> +	 * Update the SensorOutputSize to the correct value for this camera mode.
> +	 *
> +	 * \todo Move this property to CameraConfiguration when that
> +	 * feature will be made available and set it at validate() time
> +	 */
> +	data->properties_.set(properties::SensorOutputSize, sensorFormat.size);
> +
>  	/*
>  	 * See which streams are requested, and route the user
>  	 * StreamConfiguration appropriately.
> @@ -677,6 +690,16 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
>  		return ret;
>  	}
>  
> +	/*
> +	 * Figure out the smallest selection the ISP will allow. We also store
> +	 * the output image size, in pixels, from the sensor. These will be
> +	 * used for digital zoom.
> +	 */
> +	Rectangle testCrop(0, 0, 1, 1);
> +	data->isp_[Isp::Input].dev()->setSelection(V4L2_SEL_TGT_CROP, &testCrop);
> +	data->ispMinSize_ = testCrop.size();

I might have called testCrop minCrop, but I don't think that matters
much. testCrop is ok too.

Part of me is itching to say this should happen at camera creation time
or such, as I don't think it would change on any configure parameters?

But arguing against myself - that would then involve changing settings
on the device which we should avoid, and as this is used on the
per-frame controls, I think this is actually probably OK here. It's not
a big overhead for configure()


> +	data->sensorOutputSize_ = sensorFormat.size;
> +
>  	/* Adjust aspect ratio by providing crops on the input image. */
>  	Rectangle crop{ 0, 0, sensorFormat.size };
>  
> @@ -693,6 +716,8 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
>  	crop.y = (sensorFormat.size.height - crop.height) >> 1;
>  	data->isp_[Isp::Input].dev()->setSelection(V4L2_SEL_TGT_CROP, &crop);
>  
> +	data->lastIspCrop_ = crop;
> +
>  	ret = data->configureIPA(config);
>  	if (ret)
>  		LOG(RPI, Error) << "Failed to configure the IPA: " << ret;
> @@ -1588,6 +1613,28 @@ void RPiCameraData::tryRunPipeline()
>  	/* Take the first request from the queue and action the IPA. */
>  	Request *request = requestQueue_.front();
>  
> +	if (request->controls().contains(controls::IspCrop)) {
> +		Rectangle crop = request->controls().get<Rectangle>(controls::IspCrop);
> +		if (crop.width && crop.height) {
> +			/*
> +			 * 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_.alignedUpToAspectRatio(crop.size());

I like how the helpers make this expressive, over the underlying
calculations.

> +			Size size = crop.size().expandedTo(minSize);
> +			crop = size.centredTo(crop).boundedTo(Rectangle(sensorOutputSize_));
> +
> +			if (crop != lastIspCrop_)
> +				isp_[Isp::Input].dev()->setSelection(V4L2_SEL_TGT_CROP, &crop);
> +			lastIspCrop_ = crop;
> +		}
> +
> +		request->controls().set(controls::IspCrop, lastIspCrop_);
> +	}
> +

Reviewed-by: Kieran Bingham <kieran.bingham at 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
> 

-- 
Regards
--
Kieran