[libcamera-devel] [PATCH v2 2/2] libcamera: raspberrypi: Implement digital zoom

[Kieran Bingham]

Hi David,

I've just been chatting briefly with Laurent and he has mentioned that
this topic should be integrated with the work that Jacopo has done on
Pixel Array properties.

I haven't looked at that work yet, so I'm not (yet) in a position to
comment on that directly, but as I'd already written some generic
(libcamera) framework comments about the Rectangle class below, I
thought I should send this now...



On 09/07/2020 10:15, David Plowman wrote:
> These changes implement digital zoom for the Raspberry Pi. We detect
> the digital zoom control and update the V4L2 "selection" before
> starting the ISP. We also update the value in the control that we send
> to the IPA, so that it has the correct value.
> ---
>  include/libcamera/ipa/raspberrypi.h           |  1 +
>  src/ipa/raspberrypi/raspberrypi.cpp           | 10 ++++
>  .../pipeline/raspberrypi/raspberrypi.cpp      | 56 ++++++++++++++++++-
>  3 files changed, 66 insertions(+), 1 deletion(-)
> 
> diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h
> index a18ce9a..e66402e 100644
> --- a/include/libcamera/ipa/raspberrypi.h
> +++ b/include/libcamera/ipa/raspberrypi.h
> @@ -52,6 +52,7 @@ static const ControlInfoMap RPiControls = {
>  	{ &controls::Contrast, ControlInfo(0.0f, 32.0f) },
>  	{ &controls::Saturation, ControlInfo(0.0f, 32.0f) },
>  	{ &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },
> +	{ &controls::DigitalZoom, ControlInfo(Rectangle{ 0, 0, 0, 0 }, Rectangle{ 65535, 65535, 65535, 65535 }, Rectangle{ 0, 0, 0, 0 }) },

I wonder if we should have some constructor helpers to make that easier.
   (Rectangle(0), Rectangle(65535), Rectangle(0))


Oddly, (and this is not a fault of this patch), the idea of a Rectangle
which starts at x,y = {65535,65535}, and is of size {65535,65535} seems
wrong, but of course this is jsut storage of maximum values.

I can't currently think of a better way of storing that, and of course
the control has to store the max value like that anyway, so it's not an
issue I don't think  - just a quirk we might have to be aware of.



>  };
>  
>  } /* namespace libcamera */
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index b1f2786..3c68078 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -658,6 +658,16 @@ void IPARPi::queueRequest(const ControlList &controls)
>  			break;
>  		}
>  
> +		case controls::DIGITAL_ZOOM: {
> +			/*
> +			 * We send the zoom info back in the metadata where the pipeline
> +			 * handler will update it to the values actually used.
> +			 */
> +			Rectangle crop = ctrl.second.get<Rectangle>();
> +			libcameraMetadata_.set(controls::DigitalZoom, 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 f4966f8..55db11d 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -287,7 +287,8 @@ class RPiCameraData : public CameraData
>  public:
>  	RPiCameraData(PipelineHandler *pipe)
>  		: CameraData(pipe), sensor_(nullptr), lsTable_(nullptr),
> -		  state_(State::Stopped), dropFrame_(false), ispOutputCount_(0)
> +		  state_(State::Stopped), dropFrame_(false), ispOutputCount_(0),
> +		  ispMinSize_(0)
>  	{
>  	}
>  
> @@ -322,6 +323,14 @@ public:
>  	void handleStreamBuffer(FrameBuffer *buffer, const RPiStream *stream);
>  	void handleState();
>  
> +	void initSensorCrop(Rectangle const &crop, unsigned int ispMinSize)
> +	{
> +		/* The initial zoom region is the whole of the sensorCrop_. */
> +		sensorCrop_ = crop;
> +		zoomRect_ = Rectangle{ 0, 0, crop.width, crop.height };
> +		ispMinSize_ = ispMinSize;
> +	}
> +
>  	CameraSensor *sensor_;
>  	/* Array of Unicam and ISP device streams and associated buffers/streams. */
>  	RPiDevice<Unicam, 2> unicam_;
> @@ -358,6 +367,15 @@ private:
>  
>  	bool dropFrame_;
>  	int ispOutputCount_;
> +	/*
> +	 * sensorCrop_ is the largest region of the full sensor output that matches
> +	 * the desired aspect ratio, and therefore represents the area within
> +	 * which we can pan and zoom. zoomRect_ is the portion from within the
> +	 * sensorCrop_ that pan/zoom is currently using.
> +	 */
> +	Rectangle sensorCrop_;
> +	Rectangle zoomRect_;
> +	unsigned int ispMinSize_;
>  };
>  
>  class RPiCameraConfiguration : public CameraConfiguration
> @@ -744,6 +762,10 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
>  		return ret;
>  	}
>  
> +	Rectangle testCrop = { 0, 0, 1, 1 };
> +	data->isp_[Isp::Input].dev()->setSelection(V4L2_SEL_TGT_CROP, &testCrop);
> +	const unsigned int ispMinSize = testCrop.width;
> +
>  	/* Adjust aspect ratio by providing crops on the input image. */
>  	Rectangle crop = {
>  		.x = 0,
> @@ -763,8 +785,12 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
>  
>  	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);
>  
> +	sensorCrop_ = Size(crop.width, crop.height);
> +	data->initSensorCrop(crop, ispMinSize);
> +
>  	ret = configureIPA(camera);
>  	if (ret)
>  		LOG(RPI, Error) << "Failed to configure the IPA: " << ret;
> @@ -1553,6 +1579,34 @@ void RPiCameraData::tryRunPipeline()
>  	 */
>  	Request *request = requestQueue_.front();
>  
> +	if (request->controls().contains(controls::DigitalZoom)) {
> +		Rectangle rect = request->controls().get<Rectangle>(controls::DigitalZoom);
> +		/*
> +		 * If a new digital zoom value was given, check that it lies within the
> +		 * available sensorCrop_, coercing it if necessary.
> +		 */
> +		if (rect.width && rect.height) {

Laurent has very recently added a helper for the Size class as isNull.

I suspect we could add in a Rectangle.isNull() to make that statement
		if (!rect.isNull())

Of course, there could be intricacies of is a rectangle null only if the
width/height are 0, but position could be set or does the position have
to also be 0,0...



> +			zoomRect_ = rect;
> +			if (zoomRect_.width < ispMinSize_)
> +				zoomRect_.width = ispMinSize_;
> +			else if (zoomRect_.width > sensorCrop_.width)
> +				zoomRect_.width = sensorCrop_.width;
> +			if (zoomRect_.height < ispMinSize_)
> +				zoomRect_.height = ispMinSize_;
> +			else if (zoomRect_.height > sensorCrop_.height)
> +				zoomRect_.height = sensorCrop_.height;
> +			if (zoomRect_.x + zoomRect_.width > sensorCrop_.width)
> +				zoomRect_.x = sensorCrop_.width - zoomRect_.width;
> +			if (zoomRect_.y + zoomRect_.height > sensorCrop_.height)
> +				zoomRect_.y = sensorCrop_.height - zoomRect_.height;

Ayeee, and that is begging for some rectangle .clip(rect2) or
.clamp(rect2) helpers to be added to include/libcamera/geometry.h ...


> +		}
> +		Rectangle sensorRect = zoomRect_;
> +		sensorRect.x += sensorCrop_.x;
> +		sensorRect.y += sensorCrop_.y;
> +		isp_[Isp::Input].dev()->setSelection(V4L2_SEL_TGT_CROP, &sensorRect);
> +		request->controls().set(controls::DigitalZoom, zoomRect_);
> +	}
> +
>  	/*
>  	 * 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