[libcamera-devel] [PATCH 01/11] Fixes Bug 156, which breaks libcamera on Android < 12.
Nicholas Roth
nicholas at rothemail.net
Wed Oct 26 15:28:10 CEST 2022
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.
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/e3edb6af/attachment.htm>
More information about the libcamera-devel
mailing list