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