[libcamera-devel] [PATCH 01/11] Fixes Bug 156, which breaks libcamera on Android < 12.
Nicholas Roth
nicholas at rothemail.net
Thu Oct 27 05:11:25 CEST 2022
Naushir,
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:
In file included from ../src/ipa/rkisp1/algorithms/agc.cpp:8:
In file included from ../src/ipa/rkisp1/algorithms/agc.h:12:
In file included from ../include/libcamera/base/utils.h:11:
/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>'
typename common_type<typename _Duration::rep,
_Rep2>::type>::value>
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~
/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
: __duration_divide_imp<duration<_Rep1, _Period>, _Rep2>
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/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
typename __duration_divide_result<duration<_Rep1, _Period>, _Rep2>::type
^
../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]
long double exposure = 10ms /
context.configuration.sensor.lineDuration;
^
1 error generated.
On Wed, Oct 26, 2022 at 9:26 AM Naushir Patuck <naush at raspberrypi.com>
wrote:
> On Wed, 26 Oct 2022 at 14:28, Nicholas Roth <nicholas at rothemail.net>
> wrote:
>
>> Naushir,
>> > I hope there is a centralized solution with operator / overloading, but
>> > if there's no choice...
>>
>> The only centralized solution I've been able to find with operator/ that
>> *might* 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.
>>
>
> I've briefly tried the following snippet of code:
>
> ---
>
> diff --git a/include/libcamera/base/utils.h
> b/include/libcamera/base/utils.h
> index eb7bcdf4c173..ebcd56475b17 100644
> --- a/include/libcamera/base/utils.h
> +++ b/include/libcamera/base/utils.h
> @@ -358,6 +358,32 @@ public:
> }
> };
>
> +inline double operator/(const Duration &num, const Duration &den)
> +{
> + double n = num.get<std::nano>();
> + double d = den.get<std::nano>();
> +
> + return n / d;
> +}
> +
> +template<typename Rep, typename Period>
> +inline double operator/(const Duration &num, const
> std::chrono::duration<Rep, Period> &den)
> +{
> + double n = num.get<std::nano>();
> + double d =
> (std::chrono::duration_cast<std::chrono::duration<double,
> std::nano>>(den)).count();
> +
> + return n / d;
> +}
> +
> +template<typename Rep, typename Period>
> +inline double operator/(const std::chrono::duration<Rep, Period> &num,
> const Duration &den)
> +{
> + double n =
> (std::chrono::duration_cast<std::chrono::duration<double,
> std::nano>>(num)).count();
> + double d = den.get<std::nano>();
> +
> + return n / d;
> +}
> +
> template<typename T>
> decltype(auto) abs_diff(const T &a, const T &b)
>
> ---
> 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.
> Are you able to try this out in your environment to see if it resolves
> your compile issues?
>
> Regards,
> Naush
>
>
>>
>> Kieran,
>> > That sounds like we still have to make global updates. Are any of those
>> > locations where we should store a libcamera::utils::Duration instead of
>> > a std::chrono::duration to prevent having to cast?
>> 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.
>>
>> > If casting works around the issue, is it sufficient / possible to
>> > provide an explicit casting operator? (I've never done this to more than
>> > the POD types, but it 'looks' like you can specify specific target
>> > classes as an operator?)
>>
>> 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.
>>
>> I'm also appending the proposed v2 of this patch below to add clarity to
>> what I propose:
>> From 4efd469a63288b89c65be45ea1724ccde4ec486f Mon Sep 17 00:00:00 2001
>> From: Nicholas Roth <nicholas at rothemail.net>
>> Date: Mon, 24 Oct 2022 00:03:25 -0500
>> Subject: [PATCH 1/3] ipa: workaround libcxx duration limitation
>>
>> A bug in libcxx [0] used by clang version 11.0.2 is prevalent when
>> building libcamera for Android SDK30. This has been fixed and
>> integrated upstream with [1].
>>
>> As a workaround, directly cast libcamera::utils::Duration objects to
>> std::chrono::duration when dividing.
>>
>> Alternatives evaluated:
>> Considered: Enable public inheritance of std::chrono::duration and
>> override operator/ in the class.
>> Outcome: Does not fix the original compiler error.
>>
>> Considered: Enable public inheritance of std::chrono::duration and
>> override operator/ in the libcamera namespace.
>> Outcome: new compiler error:
>> ld.lld: error: duplicate symbol: libcamera::operator/
>> (libcamera::utils::Duration const&, libcamera::utils::Duration const&)
>>
>> Considered: Use private inheritance of std::chrono::duration and
>> re-implement a pass-through version of each std::chrono::duration
>> operator within libcamera::utils::Duration and use template
>> metaprogramming to fix the division operator.
>> Outcome: Testing shows that this would introduce substantial
>> limitations, i.e. requring the Duration object to be on the LHS of any
>> arithmetic operation with other numeric types. This also substantially
>> increases implementation complexity.
>>
>> Considered: Extract double values from libcamera::utils::Duration
>> objects and use those to divide.
>> Outcome: This creates substantial readability and unit-safety issues.
>>
>> [0] https://github.com/llvm/llvm-project/issues/40475
>> [1]
>> https://github.com/llvm/llvm-project/commit/efa6d803c624f9251d0ab7881122501bb9d27368
>> Bug: https://bugs.libcamera.org/show_bug.cgi?id=156
>>
>> Signed-off-by: Nicholas Roth <nicholas at rothemail.net>
>> ---
>> src/ipa/ipu3/algorithms/agc.cpp | 17 +++++++++++------
>> src/ipa/raspberrypi/cam_helper.cpp | 6 +++---
>> src/ipa/raspberrypi/cam_helper_imx296.cpp | 3 ++-
>> src/ipa/raspberrypi/controller/rpi/agc.cpp | 12 ++++++++----
>> src/ipa/raspberrypi/controller/rpi/lux.cpp | 3 ++-
>> src/ipa/rkisp1/algorithms/agc.cpp | 12 ++++++++----
>> 6 files changed, 34 insertions(+), 19 deletions(-)
>>
>> diff --git a/src/ipa/ipu3/algorithms/agc.cpp
>> b/src/ipa/ipu3/algorithms/agc.cpp
>> index a1a3c38f..1d8518da 100644
>> --- a/src/ipa/ipu3/algorithms/agc.cpp
>> +++ b/src/ipa/ipu3/algorithms/agc.cpp
>> @@ -100,7 +100,8 @@ int Agc::configure(IPAContext &context,
>>
>> /* Configure the default exposure and gain. */
>> activeState.agc.gain = std::max(minAnalogueGain_, kMinAnalogueGain);
>> - activeState.agc.exposure = 10ms / configuration.sensor.lineDuration;
>> + activeState.agc.exposure = 10ms /
>> + std::chrono::duration(configuration.sensor.lineDuration);
>>
>> frameCount_ = 0;
>> return 0;
>> @@ -240,17 +241,21 @@ void Agc::computeExposure(IPAContext &context,
>> IPAFrameContext &frameContext,
>> * increase the gain.
>> */
>> utils::Duration shutterTime =
>> - std::clamp<utils::Duration>(exposureValue / minAnalogueGain_,
>> - minShutterSpeed_, maxShutterSpeed_);
>> - double stepGain = std::clamp(exposureValue / shutterTime,
>> - minAnalogueGain_, maxAnalogueGain_);
>> + std::clamp<utils::Duration>(std::chrono::duration(exposureValue) /
>> + std::chrono::duration(minAnalogueGain_),
>> + minShutterSpeed_, maxShutterSpeed_);
>> + double stepGain = std::clamp(std::chrono::duration(exposureValue) /
>> + std::chrono::duration(shutterTime),
>> + minAnalogueGain_, maxAnalogueGain_);
>> LOG(IPU3Agc, Debug) << "Divided up shutter and gain are "
>> << shutterTime << " and "
>> << stepGain;
>>
>> IPAActiveState &activeState = context.activeState;
>> /* Update the estimated exposure and gain. */
>> - activeState.agc.exposure = shutterTime /
>> configuration.sensor.lineDuration;
>> + double lineDurationDouble = .get<std::nano>();
>> + activeState.agc.exposure = std::chrono::duration(shutterTimeDouble) /
>> + std::chrono::duration(configuration.sensor.lineDuration);
>> activeState.agc.gain = stepGain;
>> }
>>
>> diff --git a/src/ipa/raspberrypi/cam_helper.cpp
>> b/src/ipa/raspberrypi/cam_helper.cpp
>> index d90ac1de..48a8a068 100644
>> --- a/src/ipa/raspberrypi/cam_helper.cpp
>> +++ b/src/ipa/raspberrypi/cam_helper.cpp
>> @@ -63,7 +63,7 @@ void CamHelper::process([[maybe_unused]] StatisticsPtr
>> &stats,
>>
>> uint32_t CamHelper::exposureLines(const Duration exposure, const
>> Duration lineLength) const
>> {
>> - return exposure / lineLength;
>> + return std::chrono::duration(exposure) /
>> std::chrono::duration(lineLength);
>> }
>>
>> Duration CamHelper::exposure(uint32_t exposureLines, const Duration
>> lineLength) const
>> @@ -85,8 +85,8 @@ std::pair<uint32_t, uint32_t>
>> CamHelper::getBlanking(Duration &exposure,
>> * frameLengthMax gets calculated on the smallest line length as we do
>> * not want to extend that unless absolutely necessary.
>> */
>> - frameLengthMin = minFrameDuration / mode_.minLineLength;
>> - frameLengthMax = maxFrameDuration / mode_.minLineLength;
>> + frameLengthMin = std::chrono::duration(minFrameDuration) /
>> std::chrono::duration(mode_.minLineLength);
>> + frameLengthMax = std::chrono::duration(maxFrameDuration) /
>> std::chrono::duration(mode_.minLineLength);
>>
>> /*
>> * Watch out for (exposureLines + frameIntegrationDiff_) overflowing a
>> diff --git a/src/ipa/raspberrypi/cam_helper_imx296.cpp
>> b/src/ipa/raspberrypi/cam_helper_imx296.cpp
>> index ecb845e7..65f76c3c 100644
>> --- a/src/ipa/raspberrypi/cam_helper_imx296.cpp
>> +++ b/src/ipa/raspberrypi/cam_helper_imx296.cpp
>> @@ -57,7 +57,8 @@ double CamHelperImx296::gain(uint32_t gainCode) const
>> uint32_t CamHelperImx296::exposureLines(const Duration exposure,
>> [[maybe_unused]] const Duration lineLength) const
>> {
>> - return std::max<uint32_t>(minExposureLines, (exposure - 14.26us) /
>> timePerLine);
>> + return std::max<uint32_t>(minExposureLines,
>> std::chrono::duration(exposure - 14.26us) /
>> + std::chrono::duration(timePerLine));
>> }
>>
>> Duration CamHelperImx296::exposure(uint32_t exposureLines,
>> diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp
>> b/src/ipa/raspberrypi/controller/rpi/agc.cpp
>> index bd54a639..cfa5ed90 100644
>> --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp
>> +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp
>> @@ -418,7 +418,8 @@ void Agc::prepare(Metadata *imageMetadata)
>> Duration actualExposure = deviceStatus.shutterSpeed *
>> deviceStatus.analogueGain;
>> if (actualExposure) {
>> - status_.digitalGain = status_.totalExposureValue / actualExposure;
>> + status_.digitalGain = std::chrono::duration(status_.totalExposureValue)
>> /
>> + std::chrono::duration(actualExposure);
>> LOG(RPiAgc, Debug) << "Want total exposure " <<
>> status_.totalExposureValue;
>> /*
>> * Never ask for a gain < 1.0, and also impose
>> @@ -823,7 +824,8 @@ void Agc::divideUpExposure()
>> }
>> if (status_.fixedAnalogueGain == 0.0) {
>> if (exposureMode_->gain[stage] * shutterTime >= exposureValue) {
>> - analogueGain = exposureValue / shutterTime;
>> + analogueGain = std::chrono::duration(exposureValue) /
>> + std::chrono::duration(shutterTime);
>> break;
>> }
>> analogueGain = exposureMode_->gain[stage];
>> @@ -838,10 +840,12 @@ void Agc::divideUpExposure()
>> */
>> if (!status_.fixedShutter && !status_.fixedAnalogueGain &&
>> status_.flickerPeriod) {
>> - int flickerPeriods = shutterTime / status_.flickerPeriod;
>> + int flickerPeriods = std::chrono::duration(shutterTime) /
>> + std::chrono::duration(status_.flickerPeriod);
>> if (flickerPeriods) {
>> Duration newShutterTime = flickerPeriods * status_.flickerPeriod;
>> - analogueGain *= shutterTime / newShutterTime;
>> + analogueGain *= std::chrono::duration(shutterTime) /
>> + std::chrono::duration(newShutterTime);
>> /*
>> * We should still not allow the ag to go over the
>> * largest value in the exposure mode. Note that this
>> diff --git a/src/ipa/raspberrypi/controller/rpi/lux.cpp
>> b/src/ipa/raspberrypi/controller/rpi/lux.cpp
>> index 9759186a..430642f0 100644
>> --- a/src/ipa/raspberrypi/controller/rpi/lux.cpp
>> +++ b/src/ipa/raspberrypi/controller/rpi/lux.cpp
>> @@ -94,7 +94,8 @@ void Lux::process(StatisticsPtr &stats, Metadata
>> *imageMetadata)
>> double currentY = sum / (double)num + .5;
>> double gainRatio = referenceGain_ / currentGain;
>> double shutterSpeedRatio =
>> - referenceShutterSpeed_ / deviceStatus.shutterSpeed;
>> + std::chrono::duration(referenceShutterSpeed_) /
>> + std::chrono::duration(deviceStatus.shutterSpeed);
>> double apertureRatio = referenceAperture_ / currentAperture;
>> double yRatio = currentY * (65536 / numBins) / referenceY_;
>> double estimatedLux = shutterSpeedRatio * gainRatio *
>> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp
>> b/src/ipa/rkisp1/algorithms/agc.cpp
>> index 04062a36..e8814802 100644
>> --- a/src/ipa/rkisp1/algorithms/agc.cpp
>> +++ b/src/ipa/rkisp1/algorithms/agc.cpp
>> @@ -74,7 +74,8 @@ int Agc::configure(IPAContext &context, const
>> IPACameraSensorInfo &configInfo)
>> {
>> /* Configure the default exposure and gain. */
>> context.activeState.agc.gain =
>> std::max(context.configuration.agc.minAnalogueGain, kMinAnalogueGain);
>> - context.activeState.agc.exposure = 10ms /
>> context.configuration.sensor.lineDuration;
>> + context.activeState.agc.exposure = 10ms /
>> + std::chrono::duration(context.configuration.sensor.lineDuration);
>>
>> /*
>> * According to the RkISP1 documentation:
>> @@ -212,16 +213,19 @@ void Agc::computeExposure(IPAContext &context,
>> IPAFrameContext &frameContext,
>> * Push the shutter time up to the maximum first, and only then
>> * increase the gain.
>> */
>> - utils::Duration shutterTime = std::clamp<utils::Duration>(exposureValue
>> / minAnalogueGain,
>> + utils::Duration shutterTime =
>> std::clamp<utils::Duration>(std::chrono::duration(exposureValue) /
>> + minAnalogueGain,
>> minShutterSpeed, maxShutterSpeed);
>> - double stepGain = std::clamp(exposureValue / shutterTime,
>> + double stepGain = std::clamp(std::chrono::duration(exposureValue) /
>> + std::chrono::duration(shutterTime),
>> minAnalogueGain, maxAnalogueGain);
>> LOG(RkISP1Agc, Debug) << "Divided up shutter and gain are "
>> << shutterTime << " and "
>> << stepGain;
>>
>> /* Update the estimated exposure and gain. */
>> - activeState.agc.exposure = shutterTime /
>> configuration.sensor.lineDuration;
>> + activeState.agc.exposure = std::chrono::duration(shutterTime) /
>> + std::chrono::duration(configuration.sensor.lineDuration);
>> activeState.agc.gain = stepGain;
>> }
>>
>> --
>> 2.34.1
>>
>>
>> On Wed, Oct 26, 2022 at 3:51 AM Kieran Bingham <
>> kieran.bingham at ideasonboard.com> wrote:
>>
>>> Quoting Naushir Patuck (2022-10-26 09:46:51)
>>> > Hi,
>>> >
>>> > On Wed, 26 Oct 2022 at 09:24, Kieran Bingham via libcamera-devel <
>>> > libcamera-devel at lists.libcamera.org> wrote:
>>> >
>>> > > Quoting Nicholas Roth (2022-10-26 02:50:34)
>>> > > > Another possible fix is to cast libcamera::utils::Duration to
>>> > > > std::chrono::duration before performing division. I've tested that
>>> and it
>>> > > > works. Would folks be amenable to that instead?
>>> > >
>>> > > That sounds like we still have to make global updates. Are any of
>>> those
>>> > > locations where we should store a libcamera::utils::Duration instead
>>> of
>>> > > a std::chrono::duration to prevent having to cast?
>>> > >
>>> > > If casting works around the issue, is it sufficient / possible to
>>> > > provide an explicit casting operator? (I've never done this to more
>>> than
>>> > > the POD types, but it 'looks' like you can specify specific target
>>> > > classes as an operator?)
>>> > >
>>> > > https://en.cppreference.com/w/cpp/language/cast_operator
>>> >
>>> >
>>> > Casting like this is indeed possible, but even then, the changes
>>> required
>>> > are spread across the codebase. IMO this is undesirable for
>>> readability,
>>> > and error prone as any future developers must know to do this cast
>>> > if they were to do computations with utils::Duration.
>>> >
>>> > I hope there is a centralized solution with operator / overloading, but
>>> > if there's no choice...
>>>
>>> ahh, I thought that would allow the compiler to do the implicit casting,
>>> if the type was defined.
>>>
>>> But perhaps the clue is in the name "cast_operator"
>>>
>>> --
>>> Kieran
>>>
>>> >
>>> > Naush
>>> >
>>> >
>>> > >
>>> > > Something like (I have no idea if this is something that can work,
>>> or if
>>> > > it helps the compiler as it should already know the type?!)...:
>>> > >
>>> > > class Duration : public std::chrono::duration<double, std::nano>
>>> > > {
>>> > > ...
>>> > >
>>> > > explicit operator std::chrono::duration() {
>>> > > return
>>> std::chrono::duration_cast<BaseDuration>>(*this);
>>> > > }
>>> > >
>>> > > or perhaps 'worse' ? (but at least isolated to here in the class?)...
>>> > >
>>> > > constexpr explicit operator double() {
>>> > > return get<std::nano>();
>>> > > }
>>> > >
>>> > > ...
>>> > > }
>>> > >
>>> > > --
>>> > > Kieran
>>> > >
>>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20221026/8ace4b52/attachment.htm>
More information about the libcamera-devel
mailing list