[libcamera-devel] [PATCH 2/2] libcamera: raspberrypi: Implement digital zoom
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Jul 3 12:48:24 CEST 2020
Hi David and Naush,
Thank you for the patch.
On Fri, Jul 03, 2020 at 08:02:07AM +0100, Naushir Patuck wrote:
> On Thu, 2 Jul 2020 at 11:53, 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.
> >
> > Libcamera metadata is always filled in by the IPAs, so we intercept it
> > when it comes back to the pipeline handler and update the digital zoom
> > region with the rectangle that we actually used.
> > ---
> > include/libcamera/ipa/raspberrypi.h | 1 +
> > src/ipa/raspberrypi/raspberrypi.cpp | 10 ++++
> > .../pipeline/raspberrypi/raspberrypi.cpp | 59 +++++++++++++++++++
> > 3 files changed, 70 insertions(+)
> >
> > 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 }) },
> > };
> >
> > } /* namespace libcamera */
> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> > index bc89ab5..b4e42c1 100644
> > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > @@ -645,6 +645,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;
> > + }
> > +
Does this need to go through the IPA, if all the IPA does it to set it
in metadata without using or modifying the value ?
> > 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 9d887b7..25cbe13 100644
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > @@ -37,6 +37,8 @@ namespace libcamera {
> >
> > LOG_DEFINE_CATEGORY(RPI)
> >
> > +static constexpr int ISP_MIN_SIZE = 64;
>
> I'm wondering if we actually need this (see below comments).
>
> > +
> > using V4L2PixFmtMap = std::map<V4L2PixelFormat, std::vector<SizeRange>>;
> >
> > namespace {
> > @@ -322,6 +324,13 @@ public:
> > void handleStreamBuffer(FrameBuffer *buffer, const RPiStream *stream);
> > void handleState();
> >
> > + void initSensorCrop(Rectangle const &crop)
> > + {
> > + /* The initial zoom region is the whole of the sensorCrop_. */
> > + sensorCrop_ = crop;
> > + zoomRect_ = Rectangle{ 0, 0, crop.width, crop.height };
> > + }
> > +
> > CameraSensor *sensor_;
> > /* Array of Unicam and ISP device streams and associated buffers/streams. */
> > RPiDevice<Unicam, 2> unicam_;
> > @@ -358,6 +367,14 @@ 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_;
> > };
> >
> > class RPiCameraConfiguration : public CameraConfiguration
> > @@ -763,6 +780,14 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
> >
> > crop.x = (sensorFormat.size.width - crop.width) >> 1;
> > crop.y = (sensorFormat.size.height - crop.height) >> 1;
> > +
> > + if (crop.width < ISP_MIN_SIZE || crop.height < ISP_MIN_SIZE) {
> > + LOG(RPI, Error) << "Crop from sensor " << crop.width << "x" << crop.height << " is too small";
> > + return -1;
> > + }
>
> The isp driver does validate (and adjust if necessary) the crop window
> in the call to setSelection(). So you probably don't need this code
> here.
>
> > + sensorCrop_ = Size(crop.width, crop.height);
> > + data->initSensorCrop(crop);
> > +
>
> If this block is moved after the setSelection() call, then the crop
> structure should have been adjusted by the driver, and we can use it
> directly in initSensorCrop().
>
> > data->isp_[Isp::Input].dev()->setSelection(V4L2_SEL_TGT_CROP, &crop);
> >
> > ret = configureIPA(camera);
> > @@ -1226,6 +1251,13 @@ void RPiCameraData::queueFrameAction(unsigned int frame, const IPAOperationData
> > handleStreamBuffer(buffer, &isp_[Isp::Stats]);
> > /* Fill the Request metadata buffer with what the IPA has provided */
> > requestQueue_.front()->metadata() = std::move(action.controls[0]);
> > + ControlList &metadata = requestQueue_.front()->metadata();
> > + /*
> > + * The IPA sends the zoom metadata back to us, but we're the ones who
> > + * actually know what we used, so must fill it in correctly.
> > + */
> > + if (metadata.contains(controls::DigitalZoom))
> > + metadata.set(controls::DigitalZoom, zoomRect_);
>
> Perhaps it would be better if we were to give the IPA the crop
> rectangle corrected for aspect ratio as well as user digital zoom (in
> tryRunPipeline())? This way, if the IPA were to ever want the true
> crop used, it has it. In that case, there is no need for the above
> metadata munging either, the IPA will return back the true crop used.
>
> > state_ = State::IpaComplete;
> > break;
> > }
> > @@ -1548,6 +1580,33 @@ 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) {
> > + zoomRect_ = rect;
> > + if (zoomRect_.width < ISP_MIN_SIZE)
> > + zoomRect_.width = ISP_MIN_SIZE;
> > + else if (zoomRect_.width > sensorCrop_.width)
> > + zoomRect_.width = sensorCrop_.width;
> > + if (zoomRect_.height < ISP_MIN_SIZE)
> > + zoomRect_.height = ISP_MIN_SIZE;
> > + 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;
> > + }
>
> Again, this validation should already happen in the ISP driver.
>
> > + Rectangle sensorRect = zoomRect_;
> > + sensorRect.x += sensorCrop_.x;
> > + sensorRect.y += sensorCrop_.y;
> > + isp_[Isp::Input].dev()->setSelection(V4L2_SEL_TGT_CROP, &sensorRect);
> > + }
> > +
> > /*
> > * 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,
Laurent Pinchart
More information about the libcamera-devel
mailing list