<div dir="ltr"><div dir="ltr">Hi Laurent,<div><br></div><div>Thank you for your feedback.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, 4 Oct 2022 at 18:02, 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:29AM +0100, Naushir Patuck via libcamera-devel wrote:<br>
> Update CamHelper::exposureLines() and CamHelper::exposure() to take a<br>
> line length duration parameter for use in the exposure calculations.<br>
> <br>
> For now, only use the minimum line length for all the calculations to match<br>
> the existing IPA behavior.<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 | 12 ++++++------<br>
> src/ipa/raspberrypi/cam_helper.h | 6 ++++--<br>
> src/ipa/raspberrypi/cam_helper_imx219.cpp | 3 ++-<br>
> src/ipa/raspberrypi/cam_helper_imx296.cpp | 10 ++++++----<br>
> src/ipa/raspberrypi/cam_helper_imx477.cpp | 7 ++++---<br>
> src/ipa/raspberrypi/cam_helper_imx519.cpp | 7 ++++---<br>
> src/ipa/raspberrypi/controller/camera_mode.h | 2 +-<br>
> src/ipa/raspberrypi/raspberrypi.cpp | 8 ++++----<br>
> 8 files changed, 31 insertions(+), 24 deletions(-)<br>
> <br>
> diff --git a/src/ipa/raspberrypi/cam_helper.cpp b/src/ipa/raspberrypi/cam_helper.cpp<br>
> index 42251ba29682..fd3527b94501 100644<br>
> --- a/src/ipa/raspberrypi/cam_helper.cpp<br>
> +++ b/src/ipa/raspberrypi/cam_helper.cpp<br>
> @@ -61,16 +61,16 @@ void CamHelper::process([[maybe_unused]] StatisticsPtr &stats,<br>
> {<br>
> }<br>
> <br>
> -uint32_t CamHelper::exposureLines(const Duration exposure) const<br>
> +uint32_t CamHelper::exposureLines(const Duration exposure, const Duration lineLength) const<br>
> {<br>
> assert(initialized_);<br>
<br>
Is the assert still needed ? Same below.<br></blockquote><div><br></div><div>Not strictly needed any more. I can remove it in another patch.</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>
> - return exposure / mode_.minLineLength;<br>
> + return exposure / lineLength;<br>
> }<br>
> <br>
> -Duration CamHelper::exposure(uint32_t exposureLines) const<br>
> +Duration CamHelper::exposure(uint32_t exposureLines, const Duration lineLength) const<br>
> {<br>
> assert(initialized_);<br>
> - return exposureLines * mode_.minLineLength;<br>
> + return exposureLines * lineLength;<br>
> }<br>
> <br>
> uint32_t CamHelper::getVBlanking(Duration &exposure,<br>
> @@ -78,7 +78,7 @@ uint32_t CamHelper::getVBlanking(Duration &exposure,<br>
> Duration maxFrameDuration) const<br>
> {<br>
> uint32_t frameLengthMin, frameLengthMax, vblank;<br>
> - uint32_t exposureLines = CamHelper::exposureLines(exposure);<br>
> + uint32_t exposureLines = CamHelper::exposureLines(exposure, mode_.minLineLength);<br>
> <br>
> assert(initialized_);<br>
> <br>
> @@ -94,7 +94,7 @@ uint32_t CamHelper::getVBlanking(Duration &exposure,<br>
> * re-calculate if it has been clipped.<br>
> */<br>
> exposureLines = std::min(frameLengthMax - frameIntegrationDiff_, exposureLines);<br>
> - exposure = CamHelper::exposure(exposureLines);<br>
> + exposure = CamHelper::exposure(exposureLines, mode_.minLineLength);<br>
> <br>
> /* Limit the vblank to the range allowed by the frame length limits. */<br>
> vblank = std::clamp(exposureLines + frameIntegrationDiff_,<br>
> diff --git a/src/ipa/raspberrypi/cam_helper.h b/src/ipa/raspberrypi/cam_helper.h<br>
> index 70d62719da86..9b5e602689f3 100644<br>
> --- a/src/ipa/raspberrypi/cam_helper.h<br>
> +++ b/src/ipa/raspberrypi/cam_helper.h<br>
> @@ -78,8 +78,10 @@ public:<br>
> virtual void prepare(libcamera::Span<const uint8_t> buffer,<br>
> Metadata &metadata);<br>
> virtual void process(StatisticsPtr &stats, Metadata &metadata);<br>
> - virtual uint32_t exposureLines(libcamera::utils::Duration exposure) const;<br>
> - virtual libcamera::utils::Duration exposure(uint32_t exposureLines) const;<br>
> + virtual uint32_t exposureLines(const libcamera::utils::Duration exposure,<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>
> diff --git a/src/ipa/raspberrypi/cam_helper_imx219.cpp b/src/ipa/raspberrypi/cam_helper_imx219.cpp<br>
> index 7ded07a213cd..98a3b31956ec 100644<br>
> --- a/src/ipa/raspberrypi/cam_helper_imx219.cpp<br>
> +++ b/src/ipa/raspberrypi/cam_helper_imx219.cpp<br>
> @@ -94,7 +94,8 @@ void CamHelperImx219::populateMetadata(const MdParser::RegisterMap ®isters,<br>
> {<br>
> DeviceStatus deviceStatus;<br>
> <br>
> - deviceStatus.shutterSpeed = exposure(<a href="http://registers.at" rel="noreferrer" target="_blank">registers.at</a>(expHiReg) * 256 + <a href="http://registers.at" rel="noreferrer" target="_blank">registers.at</a>(expLoReg));<br>
> + deviceStatus.shutterSpeed = exposure(<a href="http://registers.at" rel="noreferrer" target="_blank">registers.at</a>(expHiReg) * 256 + <a href="http://registers.at" rel="noreferrer" target="_blank">registers.at</a>(expLoReg),<br>
> + mode_.minLineLength);<br>
> deviceStatus.analogueGain = gain(<a href="http://registers.at" rel="noreferrer" target="_blank">registers.at</a>(gainReg));<br>
> deviceStatus.frameLength = <a href="http://registers.at" rel="noreferrer" target="_blank">registers.at</a>(frameLengthHiReg) * 256 + <a href="http://registers.at" rel="noreferrer" target="_blank">registers.at</a>(frameLengthLoReg);<br>
> <br>
> diff --git a/src/ipa/raspberrypi/cam_helper_imx296.cpp b/src/ipa/raspberrypi/cam_helper_imx296.cpp<br>
> index ab1d157aaf45..74361ffe7cc2 100644<br>
> --- a/src/ipa/raspberrypi/cam_helper_imx296.cpp<br>
> +++ b/src/ipa/raspberrypi/cam_helper_imx296.cpp<br>
> @@ -21,8 +21,8 @@ public:<br>
> CamHelperImx296();<br>
> uint32_t gainCode(double gain) const override;<br>
> double gain(uint32_t gainCode) const override;<br>
> - uint32_t exposureLines(Duration exposure) const override;<br>
> - Duration exposure(uint32_t exposureLines) const override;<br>
> + uint32_t exposureLines(const Duration exposure, const Duration lineLength) const override;<br>
> + Duration exposure(uint32_t exposureLines, const Duration lineLength) const override;<br>
> <br>
> private:<br>
> static constexpr uint32_t maxGainCode = 239;<br>
> @@ -51,12 +51,14 @@ double CamHelperImx296::gain(uint32_t gainCode) const<br>
> return std::pow(10.0, gainCode / 200.0);<br>
> }<br>
> <br>
> -uint32_t CamHelperImx296::exposureLines(Duration exposure) const<br>
> +uint32_t CamHelperImx296::exposureLines(const Duration exposure,<br>
> + [[maybe_unused]] const Duration lineLength) const<br>
<br>
Does this mean that the IMX296 has no configurable horizontal blanking ?<br></blockquote><div><br></div><div>I've not looked into it in detail yet. Hence leaving this as-is. Once I verify this,</div><div>I will update this code.</div><div><br></div><div>Regards,</div><div>Naush</div><div><br></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>
> return (exposure - 14.26us) / timePerLine;<br>
> }<br>
> <br>
> -Duration CamHelperImx296::exposure(uint32_t exposureLines) const<br>
> +Duration CamHelperImx296::exposure(uint32_t exposureLines,<br>
> + [[maybe_unused]] const Duration lineLength) const<br>
> {<br>
> return exposureLines * timePerLine + 14.26us;<br>
> }<br>
> diff --git a/src/ipa/raspberrypi/cam_helper_imx477.cpp b/src/ipa/raspberrypi/cam_helper_imx477.cpp<br>
> index aa306d6661c6..71529bdd3034 100644<br>
> --- a/src/ipa/raspberrypi/cam_helper_imx477.cpp<br>
> +++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp<br>
> @@ -144,9 +144,9 @@ 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);<br>
> + exposureLines = CamHelperImx477::exposureLines(exposure, mode_.minLineLength);<br>
> exposureLines = std::min(exposureLines, frameLength - frameIntegrationDiff);<br>
> - exposure = CamHelperImx477::exposure(exposureLines);<br>
> + exposure = CamHelperImx477::exposure(exposureLines, mode_.minLineLength);<br>
> }<br>
> <br>
> return frameLength - mode_.height;<br>
> @@ -170,7 +170,8 @@ void CamHelperImx477::populateMetadata(const MdParser::RegisterMap ®isters,<br>
> {<br>
> DeviceStatus deviceStatus;<br>
> <br>
> - deviceStatus.shutterSpeed = exposure(<a href="http://registers.at" rel="noreferrer" target="_blank">registers.at</a>(expHiReg) * 256 + <a href="http://registers.at" rel="noreferrer" target="_blank">registers.at</a>(expLoReg));<br>
> + deviceStatus.shutterSpeed = exposure(<a href="http://registers.at" rel="noreferrer" target="_blank">registers.at</a>(expHiReg) * 256 + <a href="http://registers.at" rel="noreferrer" target="_blank">registers.at</a>(expLoReg),<br>
> + mode_.minLineLength);<br>
> deviceStatus.analogueGain = gain(<a href="http://registers.at" rel="noreferrer" target="_blank">registers.at</a>(gainHiReg) * 256 + <a href="http://registers.at" rel="noreferrer" target="_blank">registers.at</a>(gainLoReg));<br>
> deviceStatus.frameLength = <a href="http://registers.at" rel="noreferrer" target="_blank">registers.at</a>(frameLengthHiReg) * 256 + <a href="http://registers.at" rel="noreferrer" target="_blank">registers.at</a>(frameLengthLoReg);<br>
> deviceStatus.sensorTemperature = std::clamp<int8_t>(<a href="http://registers.at" rel="noreferrer" target="_blank">registers.at</a>(temperatureReg), -20, 80);<br>
> diff --git a/src/ipa/raspberrypi/cam_helper_imx519.cpp b/src/ipa/raspberrypi/cam_helper_imx519.cpp<br>
> index 54e104e7659a..2c120dad1680 100644<br>
> --- a/src/ipa/raspberrypi/cam_helper_imx519.cpp<br>
> +++ b/src/ipa/raspberrypi/cam_helper_imx519.cpp<br>
> @@ -144,9 +144,9 @@ 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);<br>
> + exposureLines = CamHelperImx519::exposureLines(exposure, mode_.minLineLength);<br>
> exposureLines = std::min(exposureLines, frameLength - frameIntegrationDiff);<br>
> - exposure = CamHelperImx519::exposure(exposureLines);<br>
> + exposure = CamHelperImx519::exposure(exposureLines, mode_.minLineLength);<br>
> }<br>
> <br>
> return frameLength - mode_.height;<br>
> @@ -170,7 +170,8 @@ void CamHelperImx519::populateMetadata(const MdParser::RegisterMap ®isters,<br>
> {<br>
> DeviceStatus deviceStatus;<br>
> <br>
> - deviceStatus.shutterSpeed = exposure(<a href="http://registers.at" rel="noreferrer" target="_blank">registers.at</a>(expHiReg) * 256 + <a href="http://registers.at" rel="noreferrer" target="_blank">registers.at</a>(expLoReg));<br>
> + deviceStatus.shutterSpeed = exposure(<a href="http://registers.at" rel="noreferrer" target="_blank">registers.at</a>(expHiReg) * 256 + <a href="http://registers.at" rel="noreferrer" target="_blank">registers.at</a>(expLoReg),<br>
> + mode_.minLineLength);<br>
> deviceStatus.analogueGain = gain(<a href="http://registers.at" rel="noreferrer" target="_blank">registers.at</a>(gainHiReg) * 256 + <a href="http://registers.at" rel="noreferrer" target="_blank">registers.at</a>(gainLoReg));<br>
> deviceStatus.frameLength = <a href="http://registers.at" rel="noreferrer" target="_blank">registers.at</a>(frameLengthHiReg) * 256 + <a href="http://registers.at" rel="noreferrer" target="_blank">registers.at</a>(frameLengthLoReg);<br>
> <br>
> diff --git a/src/ipa/raspberrypi/controller/camera_mode.h b/src/ipa/raspberrypi/controller/camera_mode.h<br>
> index b7e73aae4698..cda6ac4e200a 100644<br>
> --- a/src/ipa/raspberrypi/controller/camera_mode.h<br>
> +++ b/src/ipa/raspberrypi/controller/camera_mode.h<br>
> @@ -31,7 +31,7 @@ struct CameraMode {<br>
> double scaleX, scaleY;<br>
> /* scaling of the noise compared to the native sensor mode */<br>
> double noiseFactor;<br>
> - /* line time */<br>
> + /* minimum and maximum line times */<br>
<br>
This belongs to the previous patch.<br>
<br>
> libcamera::utils::Duration minLineLength, maxLineLength;<br>
> /* any camera transform *not* reflected already in the camera tuning */<br>
> libcamera::Transform transform;<br>
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp<br>
> index 67326bcf4a14..13807f4d47f7 100644<br>
> --- a/src/ipa/raspberrypi/raspberrypi.cpp<br>
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp<br>
> @@ -477,7 +477,7 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo,<br>
> const uint32_t exposureMin = sensorCtrls_.at(V4L2_CID_EXPOSURE).min().get<int32_t>();<br>
> <br>
> ctrlMap[&controls::ExposureTime] =<br>
> - ControlInfo(static_cast<int32_t>(helper_->exposure(exposureMin).get<std::micro>()),<br>
> + ControlInfo(static_cast<int32_t>(helper_->exposure(exposureMin, mode_.minLineLength).get<std::micro>()),<br>
> static_cast<int32_t>(maxShutter.get<std::micro>()));<br>
> <br>
> result->controlInfo = ControlInfoMap(std::move(ctrlMap), controls::controls);<br>
> @@ -550,7 +550,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).get<std::micro>());<br>
> + helper_->exposure(deviceStatus->frameLength, mode_.minLineLength).get<std::micro>());<br>
> if (deviceStatus->sensorTemperature)<br>
> libcameraMetadata_.set(controls::SensorTemperature, *deviceStatus->sensorTemperature);<br>
> }<br>
> @@ -1106,7 +1106,7 @@ void IPARPi::fillDeviceStatus(const ControlList &sensorControls)<br>
> int32_t gainCode = sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>();<br>
> int32_t vblank = sensorControls.get(V4L2_CID_VBLANK).get<int32_t>();<br>
> <br>
> - deviceStatus.shutterSpeed = helper_->exposure(exposureLines);<br>
> + deviceStatus.shutterSpeed = helper_->exposure(exposureLines, mode_.minLineLength);<br>
> deviceStatus.analogueGain = helper_->gain(gainCode);<br>
> deviceStatus.frameLength = mode_.height + vblank;<br>
> <br>
> @@ -1198,7 +1198,7 @@ void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls)<br>
> /* getVBlanking 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);<br>
> + int32_t exposureLines = helper_->exposureLines(exposure, mode_.minLineLength);<br>
> <br>
> LOG(IPARPI, Debug) << "Applying AGC Exposure: " << exposure<br>
> << " (Shutter lines: " << exposureLines << ", AGC requested "<br>
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
</blockquote></div></div>