[libcamera-devel] [PATCH v5 5/5] libcamera: pipeline: raspberrypi: Implementation of digital zoom
Jacopo Mondi
jacopo at jmondi.org
Sat Oct 24 18:56:27 CEST 2020
Hi David, Laurent,
On Sat, Oct 24, 2020 at 02:31:09AM +0300, Laurent Pinchart wrote:
> Hi David,
>
> Thank you for the patch.
>
> On Fri, Oct 23, 2020 at 11:21:59AM +0100, David Plowman wrote:
> > During configure() we update the ScalerCropMaximum to the correct
> > value for this camera mode 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. When the IPA returns its metadata to
> > us we add the ScalerCrop information, rescaled to sensor native
> > pixels.
> >
> > Signed-off-by: David Plowman <david.plowman at raspberrypi.com>
For the patch
Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
but to add to the discussion
> > ---
> > include/libcamera/ipa/raspberrypi.h | 1 +
> > src/ipa/raspberrypi/raspberrypi.cpp | 5 ++
> > .../pipeline/raspberrypi/raspberrypi.cpp | 82 +++++++++++++++----
> > 3 files changed, 72 insertions(+), 16 deletions(-)
> >
> > diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h
> > index b23baf2f..ff2faf86 100644
> > --- a/include/libcamera/ipa/raspberrypi.h
> > +++ b/include/libcamera/ipa/raspberrypi.h
> > @@ -62,6 +62,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 1c255aa2..f338ff8b 100644
> > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > @@ -699,6 +699,11 @@ void IPARPi::queueRequest(const ControlList &controls)
> > break;
> > }
> >
> > + case controls::SCALER_CROP: {
> > + /* We do nothing with this, but should avoid the warning below. */
> > + 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 763451a8..83e91576 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_;
>
> Maybe ispMinCropSize_ ?
>
> > +
> > unsigned int dropFrameCount_;
> >
> > private:
> > @@ -677,26 +682,31 @@ 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();
> >
> > - crop.width &= ~1;
> > - crop.height &= ~1;
> > + /* Adjust aspect ratio by providing crops on the input image. */
> > + Size size = sensorFormat.size.boundedToAspectRatio(maxSize);
> > + Rectangle crop = size.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;
> > }
> >
> > @@ -1154,8 +1164,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;
> > @@ -1164,7 +1174,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;
> > @@ -1243,8 +1253,23 @@ void RPiCameraData::queueFrameAction([[maybe_unused]] unsigned int frame,
> > FrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(bufferId);
> >
> > handleStreamBuffer(buffer, &isp_[Isp::Stats]);
> > +
> > /* Fill the Request metadata buffer with what the IPA has provided */
> > - requestQueue_.front()->metadata() = std::move(action.controls[0]);
> > + Request *request = requestQueue_.front();
> > + request->metadata() = std::move(action.controls[0]);
> > +
> > + /*
> > + * Also update the ScalerCrop in the metadata with what we actually
> > + * used. But we must first rescale that from ISP (camera mode) pixels
> > + * back into sensor native pixels.
> > + *
> > + * Sending this information on every frame may be helpful.
> > + */
> > + Rectangle crop = lastIspCrop_.scaledBy(sensorInfo_.analogCrop.size(),
> > + sensorInfo_.outputSize);
> > + crop.translateBy(sensorInfo_.analogCrop.topLeft());
>
> Would it make sense to store this in lastCrop_, along with lastIspCrop_,
> to avoid recomputing it in every frame ?
>
> > + request->metadata().set(controls::ScalerCrop, crop);
> > +
> > state_ = State::IpaComplete;
> > break;
> > }
> > @@ -1595,6 +1620,31 @@ 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) {
>
> Something we can address on top of this series, but I'd like to
> explicitly document how this case is to be handled, from an API point of
> view. The question goes beyond digital zoom: how should libcamera handle
> invalid control values ?
>
> In this specific case, the value can't be considered invalid as
> include/libcamera/ipa/raspberrypi.h sets the minimum to Rectangle{}, so
> width == 0 || height == 0 is valid from that point of view. Practically
> speaking that's of course not valid, and I think it would make sense to
> set the minimum to the hardware limit if possible.
>
> The question still holds, how should we react to invalid control values
> ? Should Camera::queueRequest() return an error ? That may be possible
> in some cases, such as checking against the boundaries set by the
> pipeline handler in the ControlInfoMap (and we'll possibly have a tiny
> race condition to handle there if we allow pipeline handlers to update
> the ControlInfoMap after start()), but not in all cases as the pipeline
> handler isn't involved in the synchronous part of
> Camera::queueRequest(). We could of course extend the pipeline handler
> API with a function to validate controls synchronously.
>
> Another option is to fail the request asynchronously, reporting an
> error. A third option is to proceeds with the control being either
> ignored, or set to the nearest acceptable value. I'm sure there could be
> other options, such as picking a default value for instance.
>
How is digital zoom supposed to be reset ? Should the application
reset it to the full active pixel array size ? Should a (0, 0)
Rectangle reset the scaler crop otherwise ?
Thanks
j
> > + /* First scale the crop from sensor native to camera mode pixels. */
> > + crop.translateBy(-sensorInfo_.analogCrop.topLeft());
> > + crop.scaleBy(sensorInfo_.outputSize, sensorInfo_.analogCrop.size());
> > +
> > + /*
> > + * 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()).enclosedIn(Rectangle(sensorInfo_.outputSize));
> > +
> > + if (crop != lastIspCrop_)
> > + isp_[Isp::Input].dev()->setSelection(V4L2_SEL_TGT_CROP, &crop);
> > + lastIspCrop_ = crop;
> > + }
> > + }
> > +
> > /*
> > * 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
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
More information about the libcamera-devel
mailing list