[libcamera-devel] [PATCH v2 2/2] libcamera: raspberrypi: Implement digital zoom
David Plowman
david.plowman at raspberrypi.com
Tue Jul 21 12:35:28 CEST 2020
Hi Kieran
Thanks for the Rectangle constructors that I've seen appear in the
code, that's obviously helpful. Also the suggestion on adding a clip
function to the Rectangle class is clearly sensible - so I'll include
that in the next version of patches!
Best regards
David
On Fri, 10 Jul 2020 at 14:40, Kieran Bingham
<kieran.bingham at ideasonboard.com> wrote:
>
> 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
More information about the libcamera-devel
mailing list