[libcamera-devel] [PATCH 01/11] Fixes Bug 156, which breaks libcamera on Android < 12.
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Oct 24 15:30:09 CEST 2022
On Mon, Oct 24, 2022 at 11:50:53AM +0100, Naushir Patuck via libcamera-devel wrote:
> 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.
Or, if that can't be done for the Duration type only, somehow override
the std::chrono::duration::operator/() globally, with a version that
fixes the bug. I think this change is fairly intrusive, I'd like a more
centralized solution that will not require patching every division.
> On Mon, 24 Oct 2022 at 06:55, Nicholas Roth via libcamera-devel 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;
> > }
> >
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list