[libcamera-devel] [PATCH 01/11] Fixes Bug 156, which breaks libcamera on Android < 12.
Kieran Bingham
kieran.bingham at ideasonboard.com
Mon Oct 24 13:39:54 CEST 2022
Quoting Naushir Patuck via libcamera-devel (2022-10-24 11:50:53)
> 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.
Seconded here too (and in
https://bugs.libcamera.org/show_bug.cgi?id=156#c2), but I haven't yet
looked into what the underlying bug is.
If we can't tackle this with utils::Duration, I'm weary that we'll end
up rapidly introducing more breakage :-S
--
Kieran
>
> 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
> >
> >
More information about the libcamera-devel
mailing list