<div dir="ltr"><div dir="ltr">Hi Nicholas,<div><br></div><div>Thank you for your patch.</div><div><br></div><div>As you've already noted, this removes much of the niceness of using std::chrono</div><div>types, and impacts code readability.</div><div><br></div><div>I wonder if it would be possible to overload "operator /" for the utils::Duration type</div><div>to work-around this bug instead? That way majority of the code need not change</div><div>and the fix only lives in one place in the codebase making it easier to revert when</div><div>the time comes.</div><div><br></div><div>Regards,</div><div>Naush </div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, 24 Oct 2022 at 06:55, Nicholas Roth via libcamera-devel <<a href="mailto:libcamera-devel@lists.libcamera.org">libcamera-devel@lists.libcamera.org</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">From: Nicholas Roth <<a href="mailto:nicholas@rothemail.net" target="_blank">nicholas@rothemail.net</a>><br>
<br>
---<br>
src/ipa/ipu3/algorithms/agc.cpp | 18 ++++++++++++++----<br>
src/ipa/raspberrypi/cam_helper.cpp | 9 ++++++---<br>
src/ipa/raspberrypi/cam_helper_imx296.cpp | 5 ++++-<br>
src/ipa/raspberrypi/controller/rpi/agc.cpp | 18 ++++++++++++++----<br>
src/ipa/raspberrypi/controller/rpi/lux.cpp | 5 ++++-<br>
src/ipa/rkisp1/algorithms/agc.cpp | 22 ++++++++++++++++++----<br>
6 files changed, 60 insertions(+), 17 deletions(-)<br>
<br>
diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp<br>
index a1a3c38f..80c551bb 100644<br>
--- a/src/ipa/ipu3/algorithms/agc.cpp<br>
+++ b/src/ipa/ipu3/algorithms/agc.cpp<br>
@@ -100,7 +100,10 @@ int Agc::configure(IPAContext &context,<br>
<br>
/* Configure the default exposure and gain. */<br>
activeState.agc.gain = std::max(minAnalogueGain_, kMinAnalogueGain);<br>
- activeState.agc.exposure = 10ms / configuration.sensor.lineDuration;<br>
+ /* TODO(Bug 156): Workaround for LLVM bug. */<br>
+ double ten_millis = utils::Duration(10ms).get<std::nano>();<br>
+ activeState.agc.exposure = ten_millis /<br>
+ configuration.sensor.lineDuration.get<std::nano>();<br>
<br>
frameCount_ = 0;<br>
return 0;<br>
@@ -238,11 +241,16 @@ void Agc::computeExposure(IPAContext &context, IPAFrameContext &frameContext,<br>
*<br>
* Push the shutter time up to the maximum first, and only then<br>
* increase the gain.<br>
+ *<br>
+ * TODO(Bug 156): Workaround for LLVM bug.<br>
*/<br>
+ double exposureValueDouble = exposureValue.get<std::nano>();<br>
+ utils::Duration shutterTimeRaw(exposureValueDouble / minAnalogueGain_);<br>
utils::Duration shutterTime =<br>
- std::clamp<utils::Duration>(exposureValue / minAnalogueGain_,<br>
+ std::clamp<utils::Duration>(shutterTimeRaw,<br>
minShutterSpeed_, maxShutterSpeed_);<br>
- double stepGain = std::clamp(exposureValue / shutterTime,<br>
+ double shutterTimeDouble = shutterTime.get<std::nano>();<br>
+ double stepGain = std::clamp(exposureValueDouble / shutterTimeDouble,<br>
minAnalogueGain_, maxAnalogueGain_);<br>
LOG(IPU3Agc, Debug) << "Divided up shutter and gain are "<br>
<< shutterTime << " and "<br>
@@ -250,7 +258,9 @@ void Agc::computeExposure(IPAContext &context, IPAFrameContext &frameContext,<br>
<br>
IPAActiveState &activeState = context.activeState;<br>
/* Update the estimated exposure and gain. */<br>
- activeState.agc.exposure = shutterTime / configuration.sensor.lineDuration;<br>
+ /* TODO(Bug 156): Workaround for LLVM bug. */<br>
+ double lineDurationDouble = configuration.sensor.lineDuration.get<std::nano>();<br>
+ activeState.agc.exposure = shutterTimeDouble / lineDurationDouble;<br>
activeState.agc.gain = stepGain;<br>
}<br>
<br>
diff --git a/src/ipa/raspberrypi/cam_helper.cpp b/src/ipa/raspberrypi/cam_helper.cpp<br>
index d90ac1de..31a9a1ef 100644<br>
--- a/src/ipa/raspberrypi/cam_helper.cpp<br>
+++ b/src/ipa/raspberrypi/cam_helper.cpp<br>
@@ -63,7 +63,8 @@ void CamHelper::process([[maybe_unused]] StatisticsPtr &stats,<br>
<br>
uint32_t CamHelper::exposureLines(const Duration exposure, const Duration lineLength) const<br>
{<br>
- return exposure / lineLength;<br>
+ /* TODO(Bug 156): Workaround for LLVM bug. */<br>
+ return exposure.get<std::nano>() / lineLength.get<std::nano>();<br>
}<br>
<br>
Duration CamHelper::exposure(uint32_t exposureLines, const Duration lineLength) const<br>
@@ -84,9 +85,11 @@ std::pair<uint32_t, uint32_t> CamHelper::getBlanking(Duration &exposure,<br>
*<br>
* frameLengthMax gets calculated on the smallest line length as we do<br>
* not want to extend that unless absolutely necessary.<br>
+ *<br>
+ * TODO(Bug 156): Workaround for LLVM bug.<br>
*/<br>
- frameLengthMin = minFrameDuration / mode_.minLineLength;<br>
- frameLengthMax = maxFrameDuration / mode_.minLineLength;<br>
+ frameLengthMin = minFrameDuration.get<std::nano>() / mode_.minLineLength.get<std::nano>();<br>
+ frameLengthMax = maxFrameDuration.get<std::nano>() / mode_.minLineLength.get<std::nano>();<br>
<br>
/*<br>
* Watch out for (exposureLines + frameIntegrationDiff_) overflowing a<br>
diff --git a/src/ipa/raspberrypi/cam_helper_imx296.cpp b/src/ipa/raspberrypi/cam_helper_imx296.cpp<br>
index ecb845e7..e48f5cf2 100644<br>
--- a/src/ipa/raspberrypi/cam_helper_imx296.cpp<br>
+++ b/src/ipa/raspberrypi/cam_helper_imx296.cpp<br>
@@ -57,7 +57,10 @@ double CamHelperImx296::gain(uint32_t gainCode) const<br>
uint32_t CamHelperImx296::exposureLines(const Duration exposure,<br>
[[maybe_unused]] const Duration lineLength) const<br>
{<br>
- return std::max<uint32_t>(minExposureLines, (exposure - 14.26us) / timePerLine);<br>
+ /* TODO(Bug 156): Workaround for LLVM bug. */<br>
+ double exposureTime = Duration(exposure - 14.26us).get<std::nano>();<br>
+ double timePerLineNano = timePerLine.get<std::nano>();<br>
+ return std::max<uint32_t>(minExposureLines, exposureTime / timePerLineNano);<br>
}<br>
<br>
Duration CamHelperImx296::exposure(uint32_t exposureLines,<br>
diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/raspberrypi/controller/rpi/agc.cpp<br>
index bd54a639..720ba788 100644<br>
--- a/src/ipa/raspberrypi/controller/rpi/agc.cpp<br>
+++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp<br>
@@ -418,7 +418,10 @@ void Agc::prepare(Metadata *imageMetadata)<br>
Duration actualExposure = deviceStatus.shutterSpeed *<br>
deviceStatus.analogueGain;<br>
if (actualExposure) {<br>
- status_.digitalGain = status_.totalExposureValue / actualExposure;<br>
+ /* TODO(Bug 156): Workaround for LLVM bug. */<br>
+ double totalExposureDouble = status_.totalExposureValue.get<std::nano>();<br>
+ double actualExposureDouble = actualExposure.get<std::nano>();<br>
+ status_.digitalGain = totalExposureDouble / actualExposureDouble;<br>
LOG(RPiAgc, Debug) << "Want total exposure " << status_.totalExposureValue;<br>
/*<br>
* Never ask for a gain < 1.0, and also impose<br>
@@ -823,7 +826,10 @@ void Agc::divideUpExposure()<br>
}<br>
if (status_.fixedAnalogueGain == 0.0) {<br>
if (exposureMode_->gain[stage] * shutterTime >= exposureValue) {<br>
- analogueGain = exposureValue / shutterTime;<br>
+ /* TODO(Bug 156): Workaround for LLVM bug. */<br>
+ double exposureDouble = exposureValue.get<std::nano>();<br>
+ double shutterTimeDouble = shutterTime.get<std::nano>();<br>
+ analogueGain = exposureDouble / shutterTimeDouble;<br>
break;<br>
}<br>
analogueGain = exposureMode_->gain[stage];<br>
@@ -838,10 +844,14 @@ void Agc::divideUpExposure()<br>
*/<br>
if (!status_.fixedShutter && !status_.fixedAnalogueGain &&<br>
status_.flickerPeriod) {<br>
- int flickerPeriods = shutterTime / status_.flickerPeriod;<br>
+ /* TODO(Bug 156): Workaround for LLVM bug. */<br>
+ double shutterTimeDouble = shutterTime.get<std::nano>();<br>
+ double flickerPeriod = status_.flickerPeriod.get<std::nano>();<br>
+ int flickerPeriods = shutterTimeDouble / flickerPeriod;<br>
if (flickerPeriods) {<br>
Duration newShutterTime = flickerPeriods * status_.flickerPeriod;<br>
- analogueGain *= shutterTime / newShutterTime;<br>
+ double newShutterTimeDouble = newShutterTime.get<std::nano>();<br>
+ analogueGain *= shutterTimeDouble / newShutterTimeDouble;<br>
/*<br>
* We should still not allow the ag to go over the<br>
* largest value in the exposure mode. Note that this<br>
diff --git a/src/ipa/raspberrypi/controller/rpi/lux.cpp b/src/ipa/raspberrypi/controller/rpi/lux.cpp<br>
index 9759186a..49303409 100644<br>
--- a/src/ipa/raspberrypi/controller/rpi/lux.cpp<br>
+++ b/src/ipa/raspberrypi/controller/rpi/lux.cpp<br>
@@ -93,8 +93,11 @@ void Lux::process(StatisticsPtr &stats, Metadata *imageMetadata)<br>
/* add .5 to reflect the mid-points of bins */<br>
double currentY = sum / (double)num + .5;<br>
double gainRatio = referenceGain_ / currentGain;<br>
+ /* TODO(Bug 156): Workaround for LLVM bug. */<br>
+ double referenceShutterSpeedDouble = referenceShutterSpeed_.get<std::nano>();<br>
+ double deviceShutterSpeed = deviceStatus.shutterSpeed.get<std::nano>();<br>
double shutterSpeedRatio =<br>
- referenceShutterSpeed_ / deviceStatus.shutterSpeed;<br>
+ referenceShutterSpeedDouble / deviceShutterSpeed;<br>
double apertureRatio = referenceAperture_ / currentAperture;<br>
double yRatio = currentY * (65536 / numBins) / referenceY_;<br>
double estimatedLux = shutterSpeedRatio * gainRatio *<br>
diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp<br>
index 04062a36..3ea0b732 100644<br>
--- a/src/ipa/rkisp1/algorithms/agc.cpp<br>
+++ b/src/ipa/rkisp1/algorithms/agc.cpp<br>
@@ -74,7 +74,13 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)<br>
{<br>
/* Configure the default exposure and gain. */<br>
context.activeState.agc.gain = std::max(context.configuration.agc.minAnalogueGain, kMinAnalogueGain);<br>
- context.activeState.agc.exposure = 10ms / context.configuration.sensor.lineDuration;<br>
+ /* TODO(Bug 156): Explicit division of ticks (e.g., `x.get<std::nano>() /<br>
+ * y.get<std::nano>()` as opposed to `x / y`) is a workaround for<br>
+ * LLVM bug 41130 and should be reverted once we no longer target<br>
+ * Android 11 / sdk30 since it compromises unit safety and readability. */<br>
+ constexpr libcamera::utils::Duration ten_millis(10ms);<br>
+ long double exposure = ten_millis.get<std::nano>() / context.configuration.sensor.lineDuration.get<std::nano>();<br>
+ context.activeState.agc.exposure = uint32_t(exposure);<br>
<br>
/*<br>
* According to the RkISP1 documentation:<br>
@@ -211,17 +217,25 @@ void Agc::computeExposure(IPAContext &context, IPAFrameContext &frameContext,<br>
/*<br>
* Push the shutter time up to the maximum first, and only then<br>
* increase the gain.<br>
+ *<br>
+ * TODO(Bug 156): Explicit division of ticks (e.g., `x.get<std::nano>() /<br>
+ * y.get<std::nano>()` as opposed to `x / y`) is a workaround for<br>
+ * LLVM bug 41130 and should be reverted once we no longer target<br>
+ * Android 11 / sdk30 since it compromises unit safety and readability.<br>
*/<br>
- utils::Duration shutterTime = std::clamp<utils::Duration>(exposureValue / minAnalogueGain,<br>
+ utils::Duration shutterTimeUnclamped(exposureValue.get<std::nano>() / minAnalogueGain);<br>
+ utils::Duration shutterTime = std::clamp<utils::Duration>(shutterTimeUnclamped,<br>
minShutterSpeed, maxShutterSpeed);<br>
- double stepGain = std::clamp(exposureValue / shutterTime,<br>
+ double stepGainUnclamped = exposureValue.get<std::nano>() / shutterTime.get<std::nano>();<br>
+ double stepGain = std::clamp(stepGainUnclamped,<br>
minAnalogueGain, maxAnalogueGain);<br>
LOG(RkISP1Agc, Debug) << "Divided up shutter and gain are "<br>
<< shutterTime << " and "<br>
<< stepGain;<br>
<br>
/* Update the estimated exposure and gain. */<br>
- activeState.agc.exposure = shutterTime / configuration.sensor.lineDuration;<br>
+ activeState.agc.exposure = uint32_t(shutterTime.get<std::nano>() /<br>
+ configuration.sensor.lineDuration.get<std::nano>());<br>
activeState.agc.gain = stepGain;<br>
}<br>
<br>
-- <br>
2.34.1<br>
<br>
</blockquote></div></div>