[libcamera-devel] [PATCH v1 8/9] ipa: raspberrypi: Allow full line length control
Naushir Patuck
naush at raspberrypi.com
Wed Oct 5 15:38:49 CEST 2022
Hi Laurent,
On Tue, 4 Oct 2022 at 18:46, Laurent Pinchart <
laurent.pinchart at ideasonboard.com> wrote:
> Hi Naush,
>
> Thank you for the patch.
>
> On Mon, Oct 03, 2022 at 09:39:34AM +0100, Naushir Patuck via
> libcamera-devel wrote:
> > Rename CamHelper::getVBlanking to CamHelper::getBlanking, and update the
> > calculations in that function to return both horizontal and vertical
> blanking
> > values for a given exposure time and frame duration limits. The
> calculations
> > are setup such that vertical blanking is extended to the maximum
> allowable
> > value, and any remainder gets put into horizontal blanking.
>
> Hmmm... It would be nice if the heuristics was implemented in the IPA
> module itself and not in the helpers, but given that the helpers are
> specific to the Raspberry Pi IPA module, I think that's fine for now.
>
> > The calculated horizontal blanking value is now return to the pipeline
> handler
>
> s/return/returned/
>
> > to pass into DelayedControls to program into the sensor.
> >
> > Update the IPA to now specify the maximum frame duration from the maximum
> > horizontal + vertical blanking values provided by the sensor mode.
> Additionally,
> > the IPA now uses the frame specific horizontal blanking value (as
> returned by
> > DelayedControls) in all instances.
> >
> > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > ---
> > src/ipa/raspberrypi/cam_helper.cpp | 47 +++++++++++++++++------
> > src/ipa/raspberrypi/cam_helper.h | 7 ++--
> > src/ipa/raspberrypi/cam_helper_imx477.cpp | 24 +++++++-----
> > src/ipa/raspberrypi/cam_helper_imx519.cpp | 24 +++++++-----
> > src/ipa/raspberrypi/raspberrypi.cpp | 45 +++++++++++++---------
> > 5 files changed, 95 insertions(+), 52 deletions(-)
> >
> > diff --git a/src/ipa/raspberrypi/cam_helper.cpp
> b/src/ipa/raspberrypi/cam_helper.cpp
> > index c255ab0cb53f..f5f034ece711 100644
> > --- a/src/ipa/raspberrypi/cam_helper.cpp
> > +++ b/src/ipa/raspberrypi/cam_helper.cpp
> > @@ -8,6 +8,7 @@
> > #include <linux/videodev2.h>
> >
> > #include <assert.h>
> > +#include <limits>
> > #include <map>
> > #include <string.h>
> >
> > @@ -74,33 +75,57 @@ Duration CamHelper::exposure(uint32_t exposureLines,
> const Duration lineLength)
> > return exposureLines * lineLength;
> > }
> >
> > -uint32_t CamHelper::getVBlanking(Duration &exposure,
> > - Duration minFrameDuration,
> > - Duration maxFrameDuration) const
> > +std::pair<uint32_t, uint32_t> CamHelper::getBlanking(Duration &exposure,
> > + Duration
> minFrameDuration,
> > + Duration
> maxFrameDuration) const
> > {
> > - uint32_t frameLengthMin, frameLengthMax, vblank;
> > - uint32_t exposureLines = CamHelper::exposureLines(exposure,
> mode_.minLineLength);
> > + uint32_t frameLengthMin, frameLengthMax, vblank, hblank;
> > + Duration lineLength = mode_.minLineLength;
> >
> > assert(initialized_);
> >
> > /*
> > * minFrameDuration and maxFrameDuration are clamped by the caller
> > * based on the limits for the active sensor mode.
> > + *
> > + * frameLengthMax gets calculated on the smallest line length as
> we do
> > + * not want to extend that unless absolutely necessary.
> > */
> > frameLengthMin = minFrameDuration / mode_.minLineLength;
> > frameLengthMax = maxFrameDuration / mode_.minLineLength;
> >
> > + /*
> > + * Watch out for exposureLines overflowing a uint32_t when the
> exposure
> > + * time is extremely (extremely!) long - as happens when the IPA
> calculates
> > + * the maximum possible exposure time.
> > + */
> > + uint32_t exposureLines =
> std::min(CamHelper::exposureLines(exposure, lineLength),
> > +
> std::numeric_limits<uint32_t>::max() - frameIntegrationDiff_);
> > + uint32_t frameLengthLines = std::clamp(exposureLines +
> frameIntegrationDiff_,
> > + frameLengthMin,
> frameLengthMax);
>
> This doesn't seem to match the comment. You're protecting against
> frameLenghtLines overflowing, but not exposureLines().
>
I'll clarify the comment above. What I meant to convey here was the
exposureLines + frameIntegrationDiff_ bit of the clamp() can overflow the
uint32_t storage so frameLengthLines could clamp to the wrong value. To
avoid this we do the std::min() statement just above.
Regards,
Naush
>
> > +
> > + /*
> > + * If our frame length lines is above the maximum allowed, see if
> we can
> > + * extend the line length to accommodate the requested frame
> length.
> > + */
> > + if (frameLengthLines > mode_.maxFrameLength) {
> > + Duration lineLengthAdjusted = lineLength *
> frameLengthLines / mode_.maxFrameLength;
> > + lineLength = std::min(mode_.maxLineLength,
> lineLengthAdjusted);
> > + frameLengthLines = mode_.maxFrameLength;
> > + }
> > +
> > + hblank = lineLengthToHblank(lineLength);
> > + vblank = frameLengthLines - mode_.height;
> > +
> > /*
> > * Limit the exposure to the maximum frame duration requested, and
> > * re-calculate if it has been clipped.
> > */
> > - exposureLines = std::min(frameLengthMax - frameIntegrationDiff_,
> exposureLines);
> > - exposure = CamHelper::exposure(exposureLines, mode_.minLineLength);
> > + exposureLines = std::min(frameLengthLines - frameIntegrationDiff_,
> > + CamHelper::exposureLines(exposure,
> lineLength));
> > + exposure = CamHelper::exposure(exposureLines, lineLength);
> >
> > - /* Limit the vblank to the range allowed by the frame length
> limits. */
> > - vblank = std::clamp(exposureLines + frameIntegrationDiff_,
> > - frameLengthMin, frameLengthMax) - mode_.height;
> > - return vblank;
> > + return { vblank, hblank };
>
> I can imagine this being error-prone, with a caller interpreting the
> value as { hblank, vblank } (which is what I would have written
> intuitively). A blanking structure with named fields would solve that.
> The Size class could also be used, but blanking.width and
> blanking.height may be confusing to read.
>
> It doesn't have to be done in this series, up to you.
>
> > }
> >
> > Duration CamHelper::hblankToLineLength(uint32_t hblank) const
> > diff --git a/src/ipa/raspberrypi/cam_helper.h
> b/src/ipa/raspberrypi/cam_helper.h
> > index b5c0726ff00e..21ac101a0a0b 100644
> > --- a/src/ipa/raspberrypi/cam_helper.h
> > +++ b/src/ipa/raspberrypi/cam_helper.h
> > @@ -8,6 +8,7 @@
> >
> > #include <memory>
> > #include <string>
> > +#include <utility>
> >
> > #include <libcamera/base/span.h>
> > #include <libcamera/base/utils.h>
> > @@ -82,9 +83,9 @@ public:
> > const libcamera::utils::Duration
> lineLength) const;
> > virtual libcamera::utils::Duration exposure(uint32_t exposureLines,
> > const
> libcamera::utils::Duration lineLength) const;
> > - virtual uint32_t getVBlanking(libcamera::utils::Duration &exposure,
> > - libcamera::utils::Duration
> minFrameDuration,
> > - libcamera::utils::Duration
> maxFrameDuration) const;
> > + virtual std::pair<uint32_t, uint32_t>
> getBlanking(libcamera::utils::Duration &exposure,
> > +
> libcamera::utils::Duration minFrameDuration,
> > +
> libcamera::utils::Duration maxFrameDuration) const;
> > libcamera::utils::Duration hblankToLineLength(uint32_t hblank)
> const;
> > uint32_t lineLengthToHblank(const libcamera::utils::Duration
> &duration) const;
> > libcamera::utils::Duration lineLengthPckToDuration(uint32_t
> lineLengthPck) const;
> > diff --git a/src/ipa/raspberrypi/cam_helper_imx477.cpp
> b/src/ipa/raspberrypi/cam_helper_imx477.cpp
> > index 76a82cc51378..19a5e471c27e 100644
> > --- a/src/ipa/raspberrypi/cam_helper_imx477.cpp
> > +++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp
> > @@ -46,8 +46,8 @@ public:
> > uint32_t gainCode(double gain) const override;
> > double gain(uint32_t gainCode) const override;
> > void prepare(libcamera::Span<const uint8_t> buffer, Metadata
> &metadata) override;
> > - uint32_t getVBlanking(Duration &exposure, Duration
> minFrameDuration,
> > - Duration maxFrameDuration) const override;
> > + std::pair<uint32_t, uint32_t> getBlanking(Duration &exposure,
> Duration minFrameDuration,
> > + Duration
> maxFrameDuration) const override;
> > void getDelays(int &exposureDelay, int &gainDelay,
> > int &vblankDelay, int &hblankDelay) const override;
> > bool sensorEmbeddedDataPresent() const override;
> > @@ -118,15 +118,19 @@ void
> CamHelperImx477::prepare(libcamera::Span<const uint8_t> buffer, Metadata &m
> > }
> > }
> >
> > -uint32_t CamHelperImx477::getVBlanking(Duration &exposure,
> > - Duration minFrameDuration,
> > - Duration maxFrameDuration) const
> > +std::pair<uint32_t, uint32_t> CamHelperImx477::getBlanking(Duration
> &exposure,
> > + Duration
> minFrameDuration,
> > + Duration
> maxFrameDuration) const
> > {
> > uint32_t frameLength, exposureLines;
> > unsigned int shift = 0;
> >
> > - frameLength = mode_.height + CamHelper::getVBlanking(exposure,
> minFrameDuration,
> > -
> maxFrameDuration);
> > + auto [vblank, hblank] = CamHelper::getBlanking(exposure,
> minFrameDuration,
> > + maxFrameDuration);
> > +
> > + frameLength = mode_.height + vblank;
> > + Duration lineLength = hblankToLineLength(hblank);
> > +
> > /*
> > * Check if the frame length calculated needs to be setup for long
> > * exposure mode. This will require us to use a long exposure scale
> > @@ -144,12 +148,12 @@ uint32_t CamHelperImx477::getVBlanking(Duration
> &exposure,
> > if (shift) {
> > /* Account for any rounding in the scaled frame length
> value. */
> > frameLength <<= shift;
> > - exposureLines = CamHelperImx477::exposureLines(exposure,
> mode_.minLineLength);
> > + exposureLines = CamHelperImx477::exposureLines(exposure,
> lineLength);
> > exposureLines = std::min(exposureLines, frameLength -
> frameIntegrationDiff);
> > - exposure = CamHelperImx477::exposure(exposureLines,
> mode_.minLineLength);
> > + exposure = CamHelperImx477::exposure(exposureLines,
> lineLength);
> > }
> >
> > - return frameLength - mode_.height;
> > + return { frameLength - mode_.height, hblank };
> > }
> >
> > void CamHelperImx477::getDelays(int &exposureDelay, int &gainDelay,
> > diff --git a/src/ipa/raspberrypi/cam_helper_imx519.cpp
> b/src/ipa/raspberrypi/cam_helper_imx519.cpp
> > index 9dff1eeb899f..d2eb171912da 100644
> > --- a/src/ipa/raspberrypi/cam_helper_imx519.cpp
> > +++ b/src/ipa/raspberrypi/cam_helper_imx519.cpp
> > @@ -46,8 +46,8 @@ public:
> > uint32_t gainCode(double gain) const override;
> > double gain(uint32_t gainCode) const override;
> > void prepare(libcamera::Span<const uint8_t> buffer, Metadata
> &metadata) override;
> > - uint32_t getVBlanking(Duration &exposure, Duration
> minFrameDuration,
> > - Duration maxFrameDuration) const override;
> > + std::pair<uint32_t, uint32_t> getBlanking(Duration &exposure,
> Duration minFrameDuration,
> > + Duration
> maxFrameDuration) const override;
> > void getDelays(int &exposureDelay, int &gainDelay,
> > int &vblankDelay, int &hblankDelay) const override;
> > bool sensorEmbeddedDataPresent() const override;
> > @@ -118,15 +118,19 @@ void
> CamHelperImx519::prepare(libcamera::Span<const uint8_t> buffer, Metadata &m
> > }
> > }
> >
> > -uint32_t CamHelperImx519::getVBlanking(Duration &exposure,
> > - Duration minFrameDuration,
> > - Duration maxFrameDuration) const
> > +std::pair<uint32_t, uint32_t> CamHelperImx519::getBlanking(Duration
> &exposure,
> > + Duration
> minFrameDuration,
> > + Duration
> maxFrameDuration) const
> > {
> > uint32_t frameLength, exposureLines;
> > unsigned int shift = 0;
> >
> > - frameLength = mode_.height + CamHelper::getVBlanking(exposure,
> minFrameDuration,
> > -
> maxFrameDuration);
> > + auto [vblank, hblank] = CamHelper::getBlanking(exposure,
> minFrameDuration,
> > + maxFrameDuration);
> > +
> > + frameLength = mode_.height + vblank;
> > + Duration lineLength = hblankToLineLength(hblank);
> > +
> > /*
> > * Check if the frame length calculated needs to be setup for long
> > * exposure mode. This will require us to use a long exposure scale
> > @@ -144,12 +148,12 @@ uint32_t CamHelperImx519::getVBlanking(Duration
> &exposure,
> > if (shift) {
> > /* Account for any rounding in the scaled frame length
> value. */
> > frameLength <<= shift;
> > - exposureLines = CamHelperImx519::exposureLines(exposure,
> mode_.minLineLength);
> > + exposureLines = CamHelperImx519::exposureLines(exposure,
> lineLength);
> > exposureLines = std::min(exposureLines, frameLength -
> frameIntegrationDiff);
> > - exposure = CamHelperImx519::exposure(exposureLines,
> mode_.minLineLength);
> > + exposure = CamHelperImx519::exposure(exposureLines,
> lineLength);
> > }
> >
> > - return frameLength - mode_.height;
> > + return { frameLength - mode_.height, hblank };
> > }
> >
> > void CamHelperImx519::getDelays(int &exposureDelay, int &gainDelay,
> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp
> b/src/ipa/raspberrypi/raspberrypi.cpp
> > index 5d6b22ef6813..497a83939ae6 100644
> > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > @@ -315,7 +315,7 @@ void IPARPi::start(const ControlList &controls,
> StartConfig *startConfig)
> > }
> >
> > startConfig->dropFrameCount = dropFrameCount_;
> > - const Duration maxSensorFrameDuration = mode_.maxFrameLength *
> mode_.minLineLength;
> > + const Duration maxSensorFrameDuration = mode_.maxFrameLength *
> mode_.maxLineLength;
> > startConfig->maxSensorFrameLengthMs =
> maxSensorFrameDuration.get<std::milli>();
> >
> > firstStart_ = false;
> > @@ -462,7 +462,7 @@ int IPARPi::configure(const IPACameraSensorInfo
> &sensorInfo,
> > */
> > ControlInfoMap::Map ctrlMap = ipaControls;
> > const Duration minSensorFrameDuration = mode_.minFrameLength *
> mode_.minLineLength;
> > - const Duration maxSensorFrameDuration = mode_.maxFrameLength *
> mode_.minLineLength;
> > + const Duration maxSensorFrameDuration = mode_.maxFrameLength *
> mode_.maxLineLength;
> > ctrlMap[&controls::FrameDurationLimits] =
> >
> ControlInfo(static_cast<int64_t>(minSensorFrameDuration.get<std::micro>()),
> >
> static_cast<int64_t>(maxSensorFrameDuration.get<std::micro>()));
> > @@ -475,7 +475,7 @@ int IPARPi::configure(const IPACameraSensorInfo
> &sensorInfo,
> > * will limit the maximum control value based on the current
> VBLANK value.
> > */
> > Duration maxShutter = Duration::max();
> > - helper_->getVBlanking(maxShutter, minSensorFrameDuration,
> maxSensorFrameDuration);
> > + helper_->getBlanking(maxShutter, minSensorFrameDuration,
> maxSensorFrameDuration);
> > const uint32_t exposureMin =
> sensorCtrls_.at(V4L2_CID_EXPOSURE).min().get<int32_t>();
> >
> > ctrlMap[&controls::ExposureTime] =
> > @@ -552,7 +552,7 @@ void IPARPi::reportMetadata()
> >
> deviceStatus->shutterSpeed.get<std::micro>());
> > libcameraMetadata_.set(controls::AnalogueGain,
> deviceStatus->analogueGain);
> > libcameraMetadata_.set(controls::FrameDuration,
> > -
> helper_->exposure(deviceStatus->frameLength,
> mode_.minLineLength).get<std::micro>());
> > +
> helper_->exposure(deviceStatus->frameLength,
> deviceStatus->lineLength).get<std::micro>());
> > if (deviceStatus->sensorTemperature)
> >
> libcameraMetadata_.set(controls::SensorTemperature,
> *deviceStatus->sensorTemperature);
> > }
> > @@ -1110,8 +1110,8 @@ void IPARPi::fillDeviceStatus(const ControlList
> &sensorControls)
> > int32_t vblank =
> sensorControls.get(V4L2_CID_VBLANK).get<int32_t>();
> > int32_t hblank =
> sensorControls.get(V4L2_CID_HBLANK).get<int32_t>();
> >
> > - deviceStatus.lineLength = (mode_.width + hblank) * (1.0s /
> mode_.pixelRate);
> > - deviceStatus.shutterSpeed = helper_->exposure(exposureLines,
> mode_.minLineLength);
> > + deviceStatus.lineLength = helper_->hblankToLineLength(hblank);
> > + deviceStatus.shutterSpeed = helper_->exposure(exposureLines,
> deviceStatus.lineLength);
> > deviceStatus.analogueGain = helper_->gain(gainCode);
> > deviceStatus.frameLength = mode_.height + vblank;
> >
> > @@ -1157,7 +1157,7 @@ void IPARPi::applyAWB(const struct AwbStatus
> *awbStatus, ControlList &ctrls)
> > void IPARPi::applyFrameDurations(Duration minFrameDuration, Duration
> maxFrameDuration)
> > {
> > const Duration minSensorFrameDuration = mode_.minFrameLength *
> mode_.minLineLength;
> > - const Duration maxSensorFrameDuration = mode_.maxFrameLength *
> mode_.minLineLength;
> > + const Duration maxSensorFrameDuration = mode_.maxFrameLength *
> mode_.maxLineLength;
> >
> > /*
> > * This will only be applied once AGC recalculations occur.
> > @@ -1178,11 +1178,11 @@ void IPARPi::applyFrameDurations(Duration
> minFrameDuration, Duration maxFrameDur
> >
> > /*
> > * Calculate the maximum exposure time possible for the AGC to use.
> > - * getVBlanking() will update maxShutter with the largest exposure
> > + * getBlanking() will update maxShutter with the largest exposure
> > * value possible.
> > */
> > Duration maxShutter = Duration::max();
> > - helper_->getVBlanking(maxShutter, minFrameDuration_,
> maxFrameDuration_);
> > + helper_->getBlanking(maxShutter, minFrameDuration_,
> maxFrameDuration_);
> >
> > RPiController::AgcAlgorithm *agc =
> dynamic_cast<RPiController::AgcAlgorithm *>(
> > controller_.getAlgorithm("agc"));
> > @@ -1200,10 +1200,11 @@ void IPARPi::applyAGC(const struct AgcStatus
> *agcStatus, ControlList &ctrls)
> > */
> > gainCode = std::min<int32_t>(gainCode, maxSensorGainCode_);
> >
> > - /* getVBlanking might clip exposure time to the fps limits. */
> > + /* getBlanking might clip exposure time to the fps limits. */
> > Duration exposure = agcStatus->shutterTime;
> > - int32_t vblanking = helper_->getVBlanking(exposure,
> minFrameDuration_, maxFrameDuration_);
> > - int32_t exposureLines = helper_->exposureLines(exposure,
> mode_.minLineLength);
> > + auto [vblank, hblank] = helper_->getBlanking(exposure,
> minFrameDuration_, maxFrameDuration_);
> > + int32_t exposureLines = helper_->exposureLines(exposure,
> > +
> helper_->hblankToLineLength(hblank));
> >
> > LOG(IPARPI, Debug) << "Applying AGC Exposure: " << exposure
> > << " (Shutter lines: " << exposureLines << ",
> AGC requested "
> > @@ -1211,14 +1212,22 @@ void IPARPi::applyAGC(const struct AgcStatus
> *agcStatus, ControlList &ctrls)
> > << agcStatus->analogueGain << " (Gain Code: "
> > << gainCode << ")";
> >
> > - /*
> > - * Due to the behavior of V4L2, the current value of VBLANK could
> clip the
> > - * exposure time without us knowing. The next time though this
> function should
> > - * clip exposure correctly.
> > - */
> > - ctrls.set(V4L2_CID_VBLANK, vblanking);
> > + ctrls.set(V4L2_CID_VBLANK, static_cast<int32_t>(vblank));
> > ctrls.set(V4L2_CID_EXPOSURE, exposureLines);
> > ctrls.set(V4L2_CID_ANALOGUE_GAIN, gainCode);
> > +
> > + /*
> > + * At present, there is no way of knowing if a control is
> read-only.
> > + * As a workaround, assume that if the minimum and maximum values
> of
> > + * the V4L2_CID_HBLANK control are the same, it implies the control
> > + * is read-only. This seems to be the case for all the cameras our
> IPA
> > + * works with.
> > + *
> > + * \todo The control API ought to have a flag to specify if a
> control
> > + * is read-only which could be used below.
>
> Indeed, that could be useful.
>
> > + */
> > + if (mode_.minLineLength != mode_.maxLineLength)
> > + ctrls.set(V4L2_CID_HBLANK, static_cast<int32_t>(hblank));
> > }
> >
> > void IPARPi::applyDG(const struct AgcStatus *dgStatus, ControlList
> &ctrls)
>
> --
> Regards,
>
> Laurent Pinchart
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20221005/bb2ff4a1/attachment.htm>
More information about the libcamera-devel
mailing list