[libcamera-devel] [PATCH 01/11] Fixes Bug 156, which breaks libcamera on Android < 12.
Kieran Bingham
kieran.bingham at ideasonboard.com
Mon Oct 24 12:41:12 CEST 2022
Hi Nicholas,
Thanks for breaking these up - much easier to review, and work out how
to integrate.
Could I ask you to read https://cbea.ms/git-commit/ please?
That's not written by us, but it's a close summary of how we like to see
commit messages formed for the project.
Quoting Nicholas Roth via libcamera-devel (2022-10-24 06:55:33)
> From: Nicholas Roth <nicholas at rothemail.net>
>
So for this patch, I would propose a subject line / commit message of:
ipa: workaround libcxx duration limitation
A bug in libcxx used by clang version 11.0.2 is prevalent when building
libcamera for Android SDK30. This has been fixed and integrated upstream
with [0]. As a workaround, directly cast the affected chrono types
[0] https://github.com/llvm/llvm-project/commit/efa6d803c624f9251d0ab7881122501bb9d27368
> ---
> src/ipa/ipu3/algorithms/agc.cpp | 18 ++++++++++++++----
> src/ipa/raspberrypi/cam_helper.cpp | 9 ++++++---
> src/ipa/raspberrypi/cam_helper_imx296.cpp | 5 ++++-
> src/ipa/raspberrypi/controller/rpi/agc.cpp | 18 ++++++++++++++----
> src/ipa/raspberrypi/controller/rpi/lux.cpp | 5 ++++-
> src/ipa/rkisp1/algorithms/agc.cpp | 22 ++++++++++++++++++----
> 6 files changed, 60 insertions(+), 17 deletions(-)
>
> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
> index a1a3c38f..80c551bb 100644
> --- a/src/ipa/ipu3/algorithms/agc.cpp
> +++ b/src/ipa/ipu3/algorithms/agc.cpp
> @@ -100,7 +100,10 @@ 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;
> + /* TODO(Bug 156): Workaround for LLVM bug. */
> + double ten_millis = utils::Duration(10ms).get<std::nano>();
> + activeState.agc.exposure = ten_millis /
> + configuration.sensor.lineDuration.get<std::nano>();
It's not yet clear to me if it's Duration / Duration that breaks, or
(explicitly) Chrono / Duration ?
Edit: Reading further I think the below won't work...
Do we need the extra casts? Is this possible? :
/*
* Default to 10 milliseconds. Avoid use of chrono suffixes due
* to Bug: 156.
*
* \todo: Bug 156: Use a chrono suffixed duration (10ms) when possible
*/
utils::Duration exposure = utils::Duration(10000) / configuration.sensor.lineDuration;
activeState.agc.exposure = exposure.get<std::nano>();
Or perhaps even does that then allow this directly?:
activeState.agc.exposure = utils::Duration(10000) / configuration.sensor.lineDuration;
I'm not sure which part of the bug causes the breakage, so it might be
that those are affected too. If so, please try to include a description
in the commit of precisely what the limitations are of the bug and what
we're prevented from doing.
> frameCount_ = 0;
> return 0;
> @@ -238,11 +241,16 @@ void Agc::computeExposure(IPAContext &context, IPAFrameContext &frameContext,
> *
> * Push the shutter time up to the maximum first, and only then
> * increase the gain.
> + *
> + * TODO(Bug 156): Workaround for LLVM bug.
We use "\todo" for todo's, to match doxygen. Though this component isn't
parsed by doxygen, but it's useful to be constent.
* \todo Bug 156: Workaround for LLVM
> */
> + double exposureValueDouble = exposureValue.get<std::nano>();
> + utils::Duration shutterTimeRaw(exposureValueDouble / minAnalogueGain_);
> utils::Duration shutterTime =
> - std::clamp<utils::Duration>(exposureValue / minAnalogueGain_,
> + std::clamp<utils::Duration>(shutterTimeRaw,
> minShutterSpeed_, maxShutterSpeed_);
> - double stepGain = std::clamp(exposureValue / shutterTime,
> + double shutterTimeDouble = shutterTime.get<std::nano>();
> + double stepGain = std::clamp(exposureValueDouble / shutterTimeDouble,
> minAnalogueGain_, maxAnalogueGain_);
Aha, ok so the limiation is that we can't do a Duration / Duration ?
(So my above suggestions are useless, but I'll leave them there as that was
my thought process above).
> LOG(IPU3Agc, Debug) << "Divided up shutter and gain are "
> << shutterTime << " and "
> @@ -250,7 +258,9 @@ void Agc::computeExposure(IPAContext &context, IPAFrameContext &frameContext,
>
> IPAActiveState &activeState = context.activeState;
> /* Update the estimated exposure and gain. */
> - activeState.agc.exposure = shutterTime / configuration.sensor.lineDuration;
> + /* TODO(Bug 156): Workaround for LLVM bug. */
> + double lineDurationDouble = configuration.sensor.lineDuration.get<std::nano>();
> + activeState.agc.exposure = shutterTimeDouble / lineDurationDouble;
> activeState.agc.gain = stepGain;
> }
>
> diff --git a/src/ipa/raspberrypi/cam_helper.cpp b/src/ipa/raspberrypi/cam_helper.cpp
> index d90ac1de..31a9a1ef 100644
> --- a/src/ipa/raspberrypi/cam_helper.cpp
> +++ b/src/ipa/raspberrypi/cam_helper.cpp
> @@ -63,7 +63,8 @@ void CamHelper::process([[maybe_unused]] StatisticsPtr &stats,
>
> uint32_t CamHelper::exposureLines(const Duration exposure, const Duration lineLength) const
> {
> - return exposure / lineLength;
> + /* TODO(Bug 156): Workaround for LLVM bug. */
Same minor about the todo styles.
> + return exposure.get<std::nano>() / lineLength.get<std::nano>();
> }
>
> Duration CamHelper::exposure(uint32_t exposureLines, const Duration lineLength) const
> @@ -84,9 +85,11 @@ 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.
> + *
> + * TODO(Bug 156): Workaround for LLVM bug.
> */
> - frameLengthMin = minFrameDuration / mode_.minLineLength;
> - frameLengthMax = maxFrameDuration / mode_.minLineLength;
> + frameLengthMin = minFrameDuration.get<std::nano>() / mode_.minLineLength.get<std::nano>();
> + frameLengthMax = maxFrameDuration.get<std::nano>() / mode_.minLineLength.get<std::nano>();
>
> /*
> * 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..e48f5cf2 100644
> --- a/src/ipa/raspberrypi/cam_helper_imx296.cpp
> +++ b/src/ipa/raspberrypi/cam_helper_imx296.cpp
> @@ -57,7 +57,10 @@ 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);
> + /* TODO(Bug 156): Workaround for LLVM bug. */
> + double exposureTime = Duration(exposure - 14.26us).get<std::nano>();
> + double timePerLineNano = timePerLine.get<std::nano>();
> + return std::max<uint32_t>(minExposureLines, exposureTime / timePerLineNano);
> }
>
> 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..720ba788 100644
> --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp
> +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp
> @@ -418,7 +418,10 @@ void Agc::prepare(Metadata *imageMetadata)
> Duration actualExposure = deviceStatus.shutterSpeed *
> deviceStatus.analogueGain;
> if (actualExposure) {
> - status_.digitalGain = status_.totalExposureValue / actualExposure;
> + /* TODO(Bug 156): Workaround for LLVM bug. */
> + double totalExposureDouble = status_.totalExposureValue.get<std::nano>();
> + double actualExposureDouble = actualExposure.get<std::nano>();
> + status_.digitalGain = totalExposureDouble / actualExposureDouble;
> LOG(RPiAgc, Debug) << "Want total exposure " << status_.totalExposureValue;
> /*
> * Never ask for a gain < 1.0, and also impose
> @@ -823,7 +826,10 @@ void Agc::divideUpExposure()
> }
> if (status_.fixedAnalogueGain == 0.0) {
> if (exposureMode_->gain[stage] * shutterTime >= exposureValue) {
> - analogueGain = exposureValue / shutterTime;
> + /* TODO(Bug 156): Workaround for LLVM bug. */
> + double exposureDouble = exposureValue.get<std::nano>();
> + double shutterTimeDouble = shutterTime.get<std::nano>();
> + analogueGain = exposureDouble / shutterTimeDouble;
> break;
> }
> analogueGain = exposureMode_->gain[stage];
> @@ -838,10 +844,14 @@ void Agc::divideUpExposure()
> */
> if (!status_.fixedShutter && !status_.fixedAnalogueGain &&
> status_.flickerPeriod) {
> - int flickerPeriods = shutterTime / status_.flickerPeriod;
> + /* TODO(Bug 156): Workaround for LLVM bug. */
> + double shutterTimeDouble = shutterTime.get<std::nano>();
> + double flickerPeriod = status_.flickerPeriod.get<std::nano>();
> + int flickerPeriods = shutterTimeDouble / flickerPeriod;
> if (flickerPeriods) {
> Duration newShutterTime = flickerPeriods * status_.flickerPeriod;
> - analogueGain *= shutterTime / newShutterTime;
> + double newShutterTimeDouble = newShutterTime.get<std::nano>();
> + analogueGain *= shutterTimeDouble / newShutterTimeDouble;
> /*
> * 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..49303409 100644
> --- a/src/ipa/raspberrypi/controller/rpi/lux.cpp
> +++ b/src/ipa/raspberrypi/controller/rpi/lux.cpp
> @@ -93,8 +93,11 @@ void Lux::process(StatisticsPtr &stats, Metadata *imageMetadata)
> /* add .5 to reflect the mid-points of bins */
> double currentY = sum / (double)num + .5;
> double gainRatio = referenceGain_ / currentGain;
> + /* TODO(Bug 156): Workaround for LLVM bug. */
> + double referenceShutterSpeedDouble = referenceShutterSpeed_.get<std::nano>();
> + double deviceShutterSpeed = deviceStatus.shutterSpeed.get<std::nano>();
> double shutterSpeedRatio =
> - referenceShutterSpeed_ / deviceStatus.shutterSpeed;
> + referenceShutterSpeedDouble / deviceShutterSpeed;
> 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..3ea0b732 100644
> --- a/src/ipa/rkisp1/algorithms/agc.cpp
> +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> @@ -74,7 +74,13 @@ 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;
> + /* TODO(Bug 156): Explicit division of ticks (e.g., `x.get<std::nano>() /
> + * y.get<std::nano>()` as opposed to `x / y`) is a workaround for
> + * LLVM bug 41130 and should be reverted once we no longer target
> + * Android 11 / sdk30 since it compromises unit safety and readability. */
/*
* For multiline comments, please keep the opening '/*' and
* closing '*/' on their own line.
*/
> + constexpr libcamera::utils::Duration ten_millis(10ms);
> + long double exposure = ten_millis.get<std::nano>() / context.configuration.sensor.lineDuration.get<std::nano>();
> + context.activeState.agc.exposure = uint32_t(exposure);
>
> /*
> * According to the RkISP1 documentation:
> @@ -211,17 +217,25 @@ void Agc::computeExposure(IPAContext &context, IPAFrameContext &frameContext,
> /*
> * Push the shutter time up to the maximum first, and only then
> * increase the gain.
> + *
> + * TODO(Bug 156): Explicit division of ticks (e.g., `x.get<std::nano>() /
> + * y.get<std::nano>()` as opposed to `x / y`) is a workaround for
> + * LLVM bug 41130 and should be reverted once we no longer target
> + * Android 11 / sdk30 since it compromises unit safety and readability.
> */
> - utils::Duration shutterTime = std::clamp<utils::Duration>(exposureValue / minAnalogueGain,
> + utils::Duration shutterTimeUnclamped(exposureValue.get<std::nano>() / minAnalogueGain);
> + utils::Duration shutterTime = std::clamp<utils::Duration>(shutterTimeUnclamped,
> minShutterSpeed, maxShutterSpeed);
> - double stepGain = std::clamp(exposureValue / shutterTime,
> + double stepGainUnclamped = exposureValue.get<std::nano>() / shutterTime.get<std::nano>();
> + double stepGain = std::clamp(stepGainUnclamped,
> 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 = uint32_t(shutterTime.get<std::nano>() /
> + configuration.sensor.lineDuration.get<std::nano>());
(Not for your patch, but) I wonder if activeState.agc.exposure would be
better stored as a Duration. I guess it depends on where it gets used
most. (as a duration, or as a double)
> activeState.agc.gain = stepGain;
> }
>
> --
> 2.34.1
>
More information about the libcamera-devel
mailing list