<div><div dir="ltr"><div><div dir="ltr" data-smartmail="gmail_signature"><div dir="ltr"><div dir="ltr"><div style="font-size:12.8px"><div style="color:rgb(136,136,136);font-size:12.8px"></div></div></div></div></div></div></div></div><div><div style="font-size:small"><font face="arial, helvetica, sans-serif" color="#000000">> </font><span style="color:rgb(34,34,34)">Could I ask you to read </span><a href="https://cbea.ms/git-commit/" rel="noreferrer" target="_blank">https://cbea.ms/git-commit/</a><span style="color:rgb(34,34,34)"> please?</span></div></div><div><div style="font-size:small" dir="auto"><span style="color:rgb(34,34,34)">Read. I thought that was quite informative. I noticed some differences between the style suggested in the blog post and the commit messages you suggest. Indeed, I have generally foregone good history in favor of convenience in git projects, and I hope it will be interesting to experience the advantages of clear history and educational to learn how this looks in a Git project.</span></div><div style="font-size:small"><span style="color:rgb(34,34,34)"><br></span></div><div style="font-size:small" dir="auto"><span style="color:rgb(34,34,34)">This might also be a good place to ask about a style guide for your code. I've done my best to be consistent with surrounding code, but it would be helpful to have something more concrete, maybe even someone's .vimrc if that's available.</span></div><div style="font-size:small"><br></div><div style="font-size:small" dir="auto"><span style="font-size:16px">> I wonder if it would be possible to overload "operator /" for the utils::Duration type</span><br></div></div><div style="font-size:small" dir="auto"><span style="font-size:16px">Gives an error: multiple definitions.</span></div><div style="font-size:small" dir="auto"><span style="font-size:16px">></span><span style="font-size:16px"> Or, if that can't be done for the Duration type only, somehow override</span></div>the std::chrono::duration::operator/() globally<div dir="auto">In global namespace: compiler error</div><div dir="auto">In the class itself: read on</div><div dir="auto"><br></div><div dir="auto">My power is out but I’ll follow up with the actual compiler errors sometime tomorrow.</div><div dir="auto"><br></div><div dir="auto">I’ve come up with a centralized solution that might work, but it’s not ideal either. I’d like to get your thoughts here before going further:</div><div dir="auto">* Make the chrono baseclass private to resolve ambiguity</div><div dir="auto">* Implement all of the usual operators i.e. +, -, /, * inside the class (even friend functions can’t see private base classes, oddly enough)</div><div dir="auto"><br></div><div dir="auto">This might also involve some template metaprogramming but I’m trying hard to do without it.</div><div dir="auto"><br></div><div dir="auto">-Nicholas</div><div dir="auto"><div style="font-size:small" dir="auto"><span style="font-size:16px"><br></span></div><div dir="auto"><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Oct 24, 2022 at 8:30 AM Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com" target="_blank">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">On Mon, Oct 24, 2022 at 11:50:53AM +0100, Naushir Patuck via libcamera-devel wrote:<br>
> Hi Nicholas,<br>
> <br>
> Thank you for your patch.<br>
> <br>
> As you've already noted, this removes much of the niceness of using<br>
> std::chrono<br>
> types, and impacts code readability.<br>
> <br>
> I wonder if it would be possible to overload "operator /" for the utils::Duration type<br>
> to work-around this bug instead? That way majority of the code need not change<br>
> and the fix only lives in one place in the codebase making it easier to revert when<br>
> the time comes.<br>
<br>
Or, if that can't be done for the Duration type only, somehow override<br>
the std::chrono::duration::operator/() globally, with a version that<br>
fixes the bug. I think this change is fairly intrusive, I'd like a more<br>
centralized solution that will not require patching every division.<br>
<br>
> On Mon, 24 Oct 2022 at 06:55, Nicholas Roth via libcamera-devel wrote:<br>
> <br>
> > 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<br>
> > &stats,<br>
> ><br>
> > uint32_t CamHelper::exposureLines(const Duration exposure, const Duration<br>
> > 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>
> > */<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>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
</blockquote></div>
</div></div>-- <br><div dir="ltr" class="gmail_signature" data-smartmail="gmail_signature"><div dir="ltr"><div dir="ltr"><div style="font-size:12.8px"><div style="color:rgb(136,136,136);font-size:12.8px"><font size="4" face="arial, helvetica, sans-serif" color="#000000"><b>Nicholas Roth</b></font><br></div><div style="color:rgb(136,136,136);font-size:12.8px"><font size="1" style="color:rgb(0,0,0);font-family:Arial,Helvetica,sans-serif,serif,EmojiFont"><span style="font-size:8pt"><b>Software Engineer, Machine Learning</b></span></font></div><div style="color:rgb(136,136,136);font-size:12.8px"><b style="font-size:8pt;color:rgb(0,0,0);font-family:Arial,Helvetica,sans-serif,serif,EmojiFont"><a href="https://www.google.com/" style="color:rgb(17,85,204)" target="_blank">Google</a> </b><font size="1" style="color:rgb(0,0,0);font-family:Arial,Helvetica,sans-serif,serif,EmojiFont"><span style="font-size:8pt"><b><br></b></span></font></div><div style="color:rgb(136,136,136);font-size:12.8px"><div style="font-size:small"><font face="arial, helvetica, sans-serif" color="#000000">C: (512) 944-0747</font></div><div style="font-size:small"><br></div><div style="font-size:small"><i style="color:rgb(34,34,34)">This footer provides context about my professional background. I am not acting on behalf of Google. My words and opinions are my own, and not those of Google.</i><font face="arial, helvetica, sans-serif" color="#000000"><br></font></div></div></div></div></div></div>