[libcamera-devel] [PATCH 01/11] Fixes Bug 156, which breaks libcamera on Android < 12.
Nicholas Roth
nicholas at rothemail.net
Tue Oct 25 05:15:36 CEST 2022
> Could I ask you to read https://cbea.ms/git-commit/ please?
Read. I thought that was quite informative. I noticed some differences
between the style suggested in the blog post and the commit messages you
suggest. Indeed, I have generally foregone good history in favor of
convenience in git projects, and I hope it will be interesting to
experience the advantages of clear history and educational to learn how
this looks in a Git project.
This might also be a good place to ask about a style guide for your code.
I've done my best to be consistent with surrounding code, but it would be
helpful to have something more concrete, maybe even someone's .vimrc if
that's available.
> I wonder if it would be possible to overload "operator /" for the
utils::Duration type
Gives an error: multiple definitions.
> Or, if that can't be done for the Duration type only, somehow override
the std::chrono::duration::operator/() globally
In global namespace: compiler error
In the class itself: read on
My power is out but I’ll follow up with the actual compiler errors sometime
tomorrow.
I’ve come up with a centralized solution that might work, but it’s not
ideal either. I’d like to get your thoughts here before going further:
* Make the chrono baseclass private to resolve ambiguity
* Implement all of the usual operators i.e. +, -, /, * inside the class
(even friend functions can’t see private base classes, oddly enough)
This might also involve some template metaprogramming but I’m trying hard
to do without it.
-Nicholas
On Mon, Oct 24, 2022 at 8:30 AM Laurent Pinchart <
laurent.pinchart at ideasonboard.com> wrote:
> 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
>
--
*Nicholas Roth*
*Software Engineer, Machine Learning*
*Google <https://www.google.com/> *
C: (512) 944-0747
*This footer provides context about my professional background. I am not
acting on behalf of Google. My words and opinions are my own, and not those
of Google.*
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20221024/29220778/attachment.htm>
More information about the libcamera-devel
mailing list