<div dir="ltr">Naushir,<div><br></div><div>I really wanted to be wrong and was hoping that you cracked the code! But. While the example above compiles in isolation, I still get the following error in SDK30 when I undo my fixes:</div><div><br></div><div>In file included from ../src/ipa/rkisp1/algorithms/agc.cpp:8:<br>In file included from ../src/ipa/rkisp1/algorithms/agc.h:12:<br>In file included from ../include/libcamera/base/utils.h:11:<br>/media/nroth/BIGLINUX/lineageos/external/libcxx/include/chrono:1273:81: error: no type named 'type' in 'std::__1::common_type<long long, libcamera::utils::Duration>'<br> typename common_type<typename _Duration::rep, _Rep2>::type>::value><br> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~<br>/media/nroth/BIGLINUX/lineageos/external/libcxx/include/chrono:1286:7: note: in instantiation of default argument for '__duration_divide_imp<std::__1::chrono::duration<long long, std::__1::ratio<1, 1000> >, libcamera::utils::Duration>' required here<br> : __duration_divide_imp<duration<_Rep1, _Period>, _Rep2><br> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~<br>/media/nroth/BIGLINUX/lineageos/external/libcxx/include/chrono:1293:10: note: in instantiation of template class 'std::__1::chrono::__duration_divide_result<std::__1::chrono::duration<long long, std::__1::ratio<1, 1000> >, libcamera::utils::Duration, false>' requested here<br>typename __duration_divide_result<duration<_Rep1, _Period>, _Rep2>::type<br> ^<br>../src/ipa/rkisp1/algorithms/agc.cpp:81:30: note: while substituting deduced template arguments into function template 'operator/' [with _Rep1 = long long, _Period = std::__1::ratio<1, 1000>, _Rep2 = libcamera::utils::Duration]<br> long double exposure = 10ms / context.configuration.sensor.lineDuration;<br> ^<br>1 error generated.<br><div><div dir="ltr" class="gmail_signature" data-smartmail="gmail_signature"><div dir="ltr"><div dir="ltr"><div style=""><div style="font-size:12.8px;color:rgb(136,136,136)"><br></div></div></div></div></div></div></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Oct 26, 2022 at 9:26 AM Naushir Patuck <<a href="mailto:naush@raspberrypi.com">naush@raspberrypi.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"><div dir="ltr"><div dir="ltr">On Wed, 26 Oct 2022 at 14:28, Nicholas Roth <<a href="mailto:nicholas@rothemail.net" target="_blank">nicholas@rothemail.net</a>> wrote:<br></div><div dir="ltr"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div><div dir="ltr"><div dir="ltr"><div dir="ltr"><div style="font-size:12.8px"><div style="font-size:12.8px"><div style="color:rgb(136,136,136);font-size:small"><font face="arial, helvetica, sans-serif" color="#000000">Naushir,</font></div><div style="font-size:small"><font color="#000000" face="arial, helvetica, sans-serif">> </font><span style="font-size:12.8px">I hope there is a centralized solution with operator / overloading, but</span></div>> if there's no choice...</div><div style="font-size:12.8px"><br></div><div style="font-size:12.8px">The only centralized solution I've been able to find with operator/ that <i>might</i> work has some major limitations. Namely, it only really works when the LHS is a libcamera::utils::Duration and you get weird errors when it's on the RHS. I can't overload at the namespace level because then it causes ambiguity with (sometimes even linker errors with) existing std::chrono overloads.<br></div></div></div></div></div></div></div></blockquote><div><br></div><div>I've briefly tried the following snippet of code:</div><div><br></div><div>---</div><div><br></div><div>diff --git a/include/libcamera/base/utils.h b/include/libcamera/base/utils.h<br>index eb7bcdf4c173..ebcd56475b17 100644<br>--- a/include/libcamera/base/utils.h<br>+++ b/include/libcamera/base/utils.h<br>@@ -358,6 +358,32 @@ public:<br> }<br> };<br><br>+inline double operator/(const Duration &num, const Duration &den)<br>+{<br>+ double n = num.get<std::nano>();<br>+ double d = den.get<std::nano>();<br>+<br>+ return n / d;<br>+}<br>+<br>+template<typename Rep, typename Period><br>+inline double operator/(const Duration &num, const std::chrono::duration<Rep, Period> &den)<br>+{<br>+ double n = num.get<std::nano>();<br>+ double d = (std::chrono::duration_cast<std::chrono::duration<double, std::nano>>(den)).count();<br>+<br>+ return n / d;<br>+}<br>+<br>+template<typename Rep, typename Period><br>+inline double operator/(const std::chrono::duration<Rep, Period> &num, const Duration &den)<br>+{<br>+ double n = (std::chrono::duration_cast<std::chrono::duration<double, std::nano>>(num)).count();<br>+ double d = den.get<std::nano>();<br>+<br>+ return n / d;<br>+}<br>+<br> template<typename T><br> decltype(auto) abs_diff(const T &a, const T &b)<br></div><div><br></div><div>---</div><div>It compiles/links ok on clang 11.0.1 (the only version of clang-11 that is available on my platform, but does not exactly match your version). Functionally it seems to do the right thing as well.</div><div>Are you able to try this out in your environment to see if it resolves your compile issues?<br></div><div><br></div><div>Regards,</div><div>Naush</div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div><div dir="ltr"><div dir="ltr"><div dir="ltr"><div style="font-size:12.8px"><div style="font-size:12.8px"><div style="color:rgb(136,136,136);font-size:small"><font face="arial, helvetica, sans-serif" color="#000000"><br></font></div><div style="color:rgb(136,136,136);font-size:small"><font face="arial, helvetica, sans-serif" color="#000000">Kieran,</font></div><div style="color:rgb(136,136,136);font-size:small"><span style="color:rgb(34,34,34)">> That sounds like we still have to make global updates. Are any of those</span><br style="color:rgb(34,34,34)"><span style="color:rgb(34,34,34)">> locations where we should store a libcamera::utils::Duration instead of</span><br style="color:rgb(34,34,34)"><span style="color:rgb(34,34,34)">> a std::chrono::duration to prevent having to cast?</span><br style="color:rgb(34,34,34)"></div><div style="color:rgb(136,136,136);font-size:small"><span style="color:rgb(34,34,34)">Did you mean vice versa (store a std::chrono::duration instead of libcamera::utils::Duration)? That would fix it, but from my understanding that would miss the point of standardizing around a double-width floating point representation at std::nano timebase.</span></div><div style="color:rgb(136,136,136);font-size:small"><font face="arial, helvetica, sans-serif" color="#000000"><br></font></div><div style="font-size:small"><font color="#000000" face="arial, helvetica, sans-serif">> </font><span style="font-size:12.8px">If casting works around the issue, is it sufficient / possible to</span></div>> provide an explicit casting operator? (I've never done this to more than<br>> the POD types, but it 'looks' like you can specify specific target<br>> classes as an operator?)</div><div style="font-size:12.8px"><br></div><div style="font-size:12.8px">I'm not sure what this would accomplish. I've been able to upcast libcamera::utils::Duration to std::chrono::duration with just `std::chrono::duration(x)` without adding an operator. Note that this seems to have nuked my <TAB>s, which I used instead of spaces for indentation to be consistent with surrounding code.</div><div style="font-size:12.8px"><div style="color:rgb(136,136,136);font-size:small"><font face="arial, helvetica, sans-serif" color="#000000"><br></font></div><div style="color:rgb(136,136,136);font-size:small"><font face="arial, helvetica, sans-serif" color="#000000">I'm also appending the proposed v2 of this patch below to add clarity to what I propose:</font></div><div style="font-size:small"><font color="#000000">>From 4efd469a63288b89c65be45ea1724ccde4ec486f Mon Sep 17 00:00:00 2001<br>From: Nicholas Roth <<a href="mailto:nicholas@rothemail.net" target="_blank">nicholas@rothemail.net</a>><br>Date: Mon, 24 Oct 2022 00:03:25 -0500<br>Subject: [PATCH 1/3] ipa: workaround libcxx duration limitation<br><br>A bug in libcxx [0] used by clang version 11.0.2 is prevalent when<br>building libcamera for Android SDK30. This has been fixed and<br>integrated upstream with [1].<br><br>As a workaround, directly cast libcamera::utils::Duration objects to<br>std::chrono::duration when dividing.<br><br>Alternatives evaluated:<br>Considered: Enable public inheritance of std::chrono::duration and<br>override operator/ in the class.<br>Outcome: Does not fix the original compiler error.<br><br>Considered: Enable public inheritance of std::chrono::duration and<br>override operator/ in the libcamera namespace.<br>Outcome: new compiler error:<br>ld.lld: error: duplicate symbol: libcamera::operator/<br>(libcamera::utils::Duration const&, libcamera::utils::Duration const&)<br><br>Considered: Use private inheritance of std::chrono::duration and<br>re-implement a pass-through version of each std::chrono::duration<br>operator within libcamera::utils::Duration and use template<br>metaprogramming to fix the division operator.<br>Outcome: Testing shows that this would introduce substantial<br>limitations, i.e. requring the Duration object to be on the LHS of any<br>arithmetic operation with other numeric types. This also substantially<br>increases implementation complexity.<br><br>Considered: Extract double values from libcamera::utils::Duration<br>objects and use those to divide.<br>Outcome: This creates substantial readability and unit-safety issues.<br><br>[0] <a href="https://github.com/llvm/llvm-project/issues/40475" target="_blank">https://github.com/llvm/llvm-project/issues/40475</a><br>[1] <a href="https://github.com/llvm/llvm-project/commit/efa6d803c624f9251d0ab7881122501bb9d27368" target="_blank">https://github.com/llvm/llvm-project/commit/efa6d803c624f9251d0ab7881122501bb9d27368</a><br>Bug: <a href="https://bugs.libcamera.org/show_bug.cgi?id=156" target="_blank">https://bugs.libcamera.org/show_bug.cgi?id=156</a><br><br>Signed-off-by: Nicholas Roth <<a href="mailto:nicholas@rothemail.net" target="_blank">nicholas@rothemail.net</a>><br>---<br> src/ipa/ipu3/algorithms/agc.cpp | 17 +++++++++++------<br> src/ipa/raspberrypi/cam_helper.cpp | 6 +++---<br> src/ipa/raspberrypi/cam_helper_imx296.cpp | 3 ++-<br> src/ipa/raspberrypi/controller/rpi/agc.cpp | 12 ++++++++----<br> src/ipa/raspberrypi/controller/rpi/lux.cpp | 3 ++-<br> src/ipa/rkisp1/algorithms/agc.cpp | 12 ++++++++----<br> 6 files changed, 34 insertions(+), 19 deletions(-)<br><br>diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp<br>index a1a3c38f..1d8518da 100644<br>--- a/src/ipa/ipu3/algorithms/agc.cpp<br>+++ b/src/ipa/ipu3/algorithms/agc.cpp<br>@@ -100,7 +100,8 @@ 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>+ activeState.agc.exposure = 10ms /<br>+ std::chrono::duration(configuration.sensor.lineDuration);<br> <br> frameCount_ = 0;<br> return 0;<br>@@ -240,17 +241,21 @@ void Agc::computeExposure(IPAContext &context, IPAFrameContext &frameContext,<br> * increase the gain.<br> */<br> utils::Duration shutterTime =<br>- std::clamp<utils::Duration>(exposureValue / minAnalogueGain_,<br>- minShutterSpeed_, maxShutterSpeed_);<br>- double stepGain = std::clamp(exposureValue / shutterTime,<br>- minAnalogueGain_, maxAnalogueGain_);<br>+ std::clamp<utils::Duration>(std::chrono::duration(exposureValue) /<br>+ std::chrono::duration(minAnalogueGain_),<br>+ minShutterSpeed_, maxShutterSpeed_);<br>+ double stepGain = std::clamp(std::chrono::duration(exposureValue) /<br>+ std::chrono::duration(shutterTime),<br>+ minAnalogueGain_, maxAnalogueGain_);<br> LOG(IPU3Agc, Debug) << "Divided up shutter and gain are "<br> << shutterTime << " and "<br> << stepGain;<br> <br> IPAActiveState &activeState = context.activeState;<br> /* Update the estimated exposure and gain. */<br>- activeState.agc.exposure = shutterTime / configuration.sensor.lineDuration;<br>+ double lineDurationDouble = .get<std::nano>();<br>+ activeState.agc.exposure = std::chrono::duration(shutterTimeDouble) /<br>+ std::chrono::duration(configuration.sensor.lineDuration);<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..48a8a068 100644<br>--- a/src/ipa/raspberrypi/cam_helper.cpp<br>+++ b/src/ipa/raspberrypi/cam_helper.cpp<br>@@ -63,7 +63,7 @@ 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>+ return std::chrono::duration(exposure) / std::chrono::duration(lineLength);<br> }<br> <br> Duration CamHelper::exposure(uint32_t exposureLines, const Duration lineLength) const<br>@@ -85,8 +85,8 @@ std::pair<uint32_t, uint32_t> CamHelper::getBlanking(Duration &exposure,<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>+ frameLengthMin = std::chrono::duration(minFrameDuration) / std::chrono::duration(mode_.minLineLength);<br>+ frameLengthMax = std::chrono::duration(maxFrameDuration) / std::chrono::duration(mode_.minLineLength);<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..65f76c3c 100644<br>--- a/src/ipa/raspberrypi/cam_helper_imx296.cpp<br>+++ b/src/ipa/raspberrypi/cam_helper_imx296.cpp<br>@@ -57,7 +57,8 @@ 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>+ return std::max<uint32_t>(minExposureLines, std::chrono::duration(exposure - 14.26us) /<br>+ std::chrono::duration(timePerLine));<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..cfa5ed90 100644<br>--- a/src/ipa/raspberrypi/controller/rpi/agc.cpp<br>+++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp<br>@@ -418,7 +418,8 @@ void Agc::prepare(Metadata *imageMetadata)<br> Duration actualExposure = deviceStatus.shutterSpeed *<br> deviceStatus.analogueGain;<br> if (actualExposure) {<br>- status_.digitalGain = status_.totalExposureValue / actualExposure;<br>+ status_.digitalGain = std::chrono::duration(status_.totalExposureValue) /<br>+ std::chrono::duration(actualExposure);<br> LOG(RPiAgc, Debug) << "Want total exposure " << status_.totalExposureValue;<br> /*<br> * Never ask for a gain < 1.0, and also impose<br>@@ -823,7 +824,8 @@ void Agc::divideUpExposure()<br> }<br> if (status_.fixedAnalogueGain == 0.0) {<br> if (exposureMode_->gain[stage] * shutterTime >= exposureValue) {<br>- analogueGain = exposureValue / shutterTime;<br>+ analogueGain = std::chrono::duration(exposureValue) /<br>+ std::chrono::duration(shutterTime);<br> break;<br> }<br> analogueGain = exposureMode_->gain[stage];<br>@@ -838,10 +840,12 @@ void Agc::divideUpExposure()<br> */<br> if (!status_.fixedShutter && !status_.fixedAnalogueGain &&<br> status_.flickerPeriod) {<br>- int flickerPeriods = shutterTime / status_.flickerPeriod;<br>+ int flickerPeriods = std::chrono::duration(shutterTime) /<br>+ std::chrono::duration(status_.flickerPeriod);<br> if (flickerPeriods) {<br> Duration newShutterTime = flickerPeriods * status_.flickerPeriod;<br>- analogueGain *= shutterTime / newShutterTime;<br>+ analogueGain *= std::chrono::duration(shutterTime) /<br>+ std::chrono::duration(newShutterTime);<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..430642f0 100644<br>--- a/src/ipa/raspberrypi/controller/rpi/lux.cpp<br>+++ b/src/ipa/raspberrypi/controller/rpi/lux.cpp<br>@@ -94,7 +94,8 @@ void Lux::process(StatisticsPtr &stats, Metadata *imageMetadata)<br> double currentY = sum / (double)num + .5;<br> double gainRatio = referenceGain_ / currentGain;<br> double shutterSpeedRatio =<br>- referenceShutterSpeed_ / deviceStatus.shutterSpeed;<br>+ std::chrono::duration(referenceShutterSpeed_) /<br>+ std::chrono::duration(deviceStatus.shutterSpeed);<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..e8814802 100644<br>--- a/src/ipa/rkisp1/algorithms/agc.cpp<br>+++ b/src/ipa/rkisp1/algorithms/agc.cpp<br>@@ -74,7 +74,8 @@ 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>+ context.activeState.agc.exposure = 10ms /<br>+ std::chrono::duration(context.configuration.sensor.lineDuration);<br> <br> /*<br> * According to the RkISP1 documentation:<br>@@ -212,16 +213,19 @@ void Agc::computeExposure(IPAContext &context, IPAFrameContext &frameContext,<br> * Push the shutter time up to the maximum first, and only then<br> * increase the gain.<br> */<br>- utils::Duration shutterTime = std::clamp<utils::Duration>(exposureValue / minAnalogueGain,<br>+ utils::Duration shutterTime = std::clamp<utils::Duration>(std::chrono::duration(exposureValue) /<br>+ minAnalogueGain,<br> minShutterSpeed, maxShutterSpeed);<br>- double stepGain = std::clamp(exposureValue / shutterTime,<br>+ double stepGain = std::clamp(std::chrono::duration(exposureValue) /<br>+ std::chrono::duration(shutterTime),<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 = std::chrono::duration(shutterTime) /<br>+ std::chrono::duration(configuration.sensor.lineDuration);<br> activeState.agc.gain = stepGain;<br> }<br> <br>-- <br>2.34.1</font><br></div></div></div></div></div></div></div><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Oct 26, 2022 at 3:51 AM Kieran Bingham <<a href="mailto:kieran.bingham@ideasonboard.com" target="_blank">kieran.bingham@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">Quoting Naushir Patuck (2022-10-26 09:46:51)<br>
> Hi,<br>
> <br>
> On Wed, 26 Oct 2022 at 09:24, Kieran Bingham via libcamera-devel <<br>
> <a href="mailto:libcamera-devel@lists.libcamera.org" target="_blank">libcamera-devel@lists.libcamera.org</a>> wrote:<br>
> <br>
> > Quoting Nicholas Roth (2022-10-26 02:50:34)<br>
> > > Another possible fix is to cast libcamera::utils::Duration to<br>
> > > std::chrono::duration before performing division. I've tested that and it<br>
> > > works. Would folks be amenable to that instead?<br>
> ><br>
> > That sounds like we still have to make global updates. Are any of those<br>
> > locations where we should store a libcamera::utils::Duration instead of<br>
> > a std::chrono::duration to prevent having to cast?<br>
> ><br>
> > If casting works around the issue, is it sufficient / possible to<br>
> > provide an explicit casting operator? (I've never done this to more than<br>
> > the POD types, but it 'looks' like you can specify specific target<br>
> > classes as an operator?)<br>
> ><br>
> > <a href="https://en.cppreference.com/w/cpp/language/cast_operator" rel="noreferrer" target="_blank">https://en.cppreference.com/w/cpp/language/cast_operator</a><br>
> <br>
> <br>
> Casting like this is indeed possible, but even then, the changes required<br>
> are spread across the codebase. IMO this is undesirable for readability,<br>
> and error prone as any future developers must know to do this cast<br>
> if they were to do computations with utils::Duration.<br>
> <br>
> I hope there is a centralized solution with operator / overloading, but<br>
> if there's no choice...<br>
<br>
ahh, I thought that would allow the compiler to do the implicit casting,<br>
if the type was defined.<br>
<br>
But perhaps the clue is in the name "cast_operator"<br>
<br>
--<br>
Kieran<br>
<br>
> <br>
> Naush<br>
> <br>
> <br>
> ><br>
> > Something like (I have no idea if this is something that can work, or if<br>
> > it helps the compiler as it should already know the type?!)...:<br>
> ><br>
> > class Duration : public std::chrono::duration<double, std::nano><br>
> > {<br>
> > ...<br>
> ><br>
> > explicit operator std::chrono::duration() {<br>
> > return std::chrono::duration_cast<BaseDuration>>(*this);<br>
> > }<br>
> ><br>
> > or perhaps 'worse' ? (but at least isolated to here in the class?)...<br>
> ><br>
> > constexpr explicit operator double() {<br>
> > return get<std::nano>();<br>
> > }<br>
> ><br>
> > ...<br>
> > }<br>
> ><br>
> > --<br>
> > Kieran<br>
> ><br>
</blockquote></div>
</blockquote></div></div>
</div>
</blockquote></div>