[libcamera-devel] [PATCH v6 5/6] libcamera: pipeline: raspberrypi: Implementation of digital zoom
David Plowman
david.plowman at raspberrypi.com
Tue Oct 27 11:35:52 CET 2020
Hi Laurent
Thanks for looking through this!
On Mon, 26 Oct 2020 at 23:07, Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi David,
>
> Thank you for the patch.
>
> On Mon, Oct 26, 2020 at 05:19:07PM +0000, 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>
> > ---
> > include/libcamera/ipa/raspberrypi.h | 1 +
> > src/ipa/raspberrypi/raspberrypi.cpp | 5 +
> > .../pipeline/raspberrypi/raspberrypi.cpp | 92 +++++++++++++++----
> > 3 files changed, 81 insertions(+), 17 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..b9d74a81 100644
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > @@ -135,7 +135,7 @@ public:
> > RPiCameraData(PipelineHandler *pipe)
> > : CameraData(pipe), sensor_(nullptr), state_(State::Stopped),
> > supportsFlips_(false), flipsAlterBayerOrder_(false),
> > - dropFrameCount_(0), ispOutputCount_(0)
> > + updateScalerCrop_(true), dropFrameCount_(0), ispOutputCount_(0)
> > {
> > }
> >
> > @@ -193,6 +193,13 @@ public:
> > bool flipsAlterBayerOrder_;
> > BayerFormat::Order nativeBayerOrder_;
> >
> > + /* For handling digital zoom. */
> > + CameraSensorInfo sensorInfo_;
> > + Rectangle ispCrop_; /* crop in ISP (camera mode) pixels */
> > + Rectangle scalerCrop_; /* crop in sensor native pixels */
> > + bool updateScalerCrop_;
> > + Size ispMinCropSize_;
> > +
> > unsigned int dropFrameCount_;
> >
> > private:
> > @@ -677,26 +684,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->ispMinCropSize_ = 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(Rectangle(sensorFormat.size).center());
> > + data->ispCrop_ = 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 +1166,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 +1176,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 +1255,26 @@ 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.
> > + */
> > + if (updateScalerCrop_) {
> > + updateScalerCrop_ = false;
> > + scalerCrop_ = ispCrop_.scaledBy(sensorInfo_.analogCrop.size(),
> > + sensorInfo_.outputSize);
> > + scalerCrop_.translateBy(sensorInfo_.analogCrop.topLeft());
>
> I initially thought this would be done in
> RPiCameraData::tryRunPipeline(), but here is fine too.
>
> > + }
> > + request->metadata().set(controls::ScalerCrop, scalerCrop_);
> > +
> > state_ = State::IpaComplete;
> > break;
> > }
> > @@ -1595,6 +1625,34 @@ 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) {
>
> This is the only part that I think needs a fix. If the crop rectangle is
> invalid, I think it should be set to the closed valid value. How about
>
> if (!crop.width || !crop.height)
> crop = { 0, 0, 1, 1 };
>
> and lowering the indentation of the next block of code ? Unless I'm
> mistaken this will avoid a crash and produce a valid crop rectangle. We
> can also try to make it a bit more fancy to use the same aspect ratio as
> the analog crop rectangle, but I think that's overkill, if the crop
> rectangle is invalid in the first place, it's the application's fault.
I wasn't quite clear why this would crash as it stands. As far as I
can see it would avoid the "if (crop.width && crop.height)" block,
leaving ispCrop_ unchanged (initially it has the default value). But
please let me know if I've missed something there.
However, this did alert me to a real problem, namely that crop.width
and crop.height might both be non-zero, but they could go to zero now
that we rescale from native to ISP pixels (we didn't do this before,
of course). This was revealed by that { 0, 0, 1, 1 } rectangle that
you used, and really does cause a crash! (It asserts in
expandedToAspectRatio - so good idea to put the assert in there!!)
Anyway, I'll fix that up and send a revised version of this single
patch (if I can figure out how to do that...)
Thanks
David
>
> I can fix this when applying if you're fine with the change (it would be
> nice if you could test it though).
>
> With this addressed,
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
> > + /* 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 ispMinCropSize_, 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 = ispMinCropSize_.expandedToAspectRatio(crop.size());
> > + Size size = crop.size().expandedTo(minSize);
> > + crop = size.centeredTo(crop.center()).enclosedIn(Rectangle(sensorInfo_.outputSize));
> > +
> > + if (crop != ispCrop_) {
> > + isp_[Isp::Input].dev()->setSelection(V4L2_SEL_TGT_CROP, &crop);
> > + ispCrop_ = crop;
> > + /* queueFrameAction will have to update its scalerCrop_ */
> > + updateScalerCrop_ = true;
> > + }
> > + }
> > + }
> > +
> > /*
> > * 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