[libcamera-devel] [PATCH 01/11] Fixes Bug 156, which breaks libcamera on Android < 12.
Naushir Patuck
naush at raspberrypi.com
Mon Oct 24 12:50:53 CEST 2022
Hi Nicholas,
Thank you for your patch.
As you've already noted, this removes much of the niceness of using
std::chrono
types, and impacts code readability.
I wonder if it would be possible to overload "operator /" for the
utils::Duration type
to work-around this bug instead? That way majority of the code need not
change
and the fix only lives in one place in the codebase making it easier to
revert when
the time comes.
Regards,
Naush
On Mon, 24 Oct 2022 at 06:55, Nicholas Roth via libcamera-devel <
libcamera-devel at lists.libcamera.org> wrote:
> From: Nicholas Roth <nicholas at rothemail.net>
>
> ---
> 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>();
>
> 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.
> */
> + 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_);
> 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. */
> + 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. */
> + 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>());
> activeState.agc.gain = stepGain;
> }
>
> --
> 2.34.1
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20221024/8d2ecdba/attachment.htm>
More information about the libcamera-devel
mailing list