[libcamera-devel] [PATCH 01/11] Fixes Bug 156, which breaks libcamera on Android < 12.
Kieran Bingham
kieran.bingham at ideasonboard.com
Tue Oct 25 13:14:01 CEST 2022
Quoting Nicholas Roth via libcamera-devel (2022-10-25 04:15:36)
> > 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
Style differences are fine. Content is King. We won't merge patches that
say "Latest code" or "fixes foo". I like to break things down to cover
three points, but it doesn't always apply. Capturing rationale is the
important part.
prefix: of: components: Clear subject
- What is wrong
- What this patch does
- Why it does it (the way it does)
Signed-off-by: <author>
Subject: ships: argo: Increase sailers on the Argo
The Argo is insuffienctly staffed and incapable of slaying any
dragons, (or managing any other adventures accordingly).
To avoid mortal peril beyond expectations, staff the Argo with at
least 50 of the finest Heroes available.
The target of 50 has been reached according to the size of the vessel,
and the number of beds available for resting during the journey, as
well as examining the food supplies that are expected to last over the
extended travel period.
The return Journey may have a reduced staffing level, but this is not
desired. Staffing levels should be kept as close to the maximum level
as possible at all times.
Signed-off-by: Jason of Aeson <jason at argonauts.org>
---
diff --git a/ships/argo b/ships/argo
index c9a40e8788e7..e257b2e257b4 100644
--- a/ships/argo
+++ b/ships/argo
@@ -1 +1 @@
-staff-level: 0
+staff-level: 50
Why do I have a sudden urge to write an anthology of mythological
stories in patch form ?
> 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.
Please feel free to look through the commit history of libcamera. We're
over 4000 commits now, and I would really hope that a vast majority
would be considered as good examples of commits. (we do allow some slack
at times... - justifying a typo is usually quite short :D )
- https://git.libcamera.org/libcamera/libcamera.git/log/
We mostly come with a background (and continuation) in Linux Kernel
development, where keeping this history is cruicial due to the scale of
the kernel. It could be argued that it's not crucial to the same level
at libcamera, but I believe it's good development practice to maintain
clear commits throughout.
The commit messages usually come into their own when bisecting failures,
or looking through git-blame to see /why/ a line of code is the way it
is.
Ah yes, also we expect bisectability - so each commit should compile and
be functional as an independent task, while aiming to do one thing per
patch.
> 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.
We have tooling in place to help already.
- https://git.libcamera.org/libcamera/libcamera.git/tree/utils/hooks/post-commit
To utilise this hook, install this file with:
cp utils/hooks/post-commit .git/hooks/post-commit
That will run our commit style checks on every commit.
I prefer doing it as a post-commit so you can still commit with
failures. pre-commit is usually too 'harsh'.
Beyond that, we have some documentation on the subject at:
- https://libcamera.org/contributing.html
and
- https://libcamera.org/coding-style.html
But I'm sure this is always something that needs more work on
documenting, or making it easier to read/find.
> > 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
What's the compiler error here ?
> 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.
Ayee, this sounds like more work ... which is painful. I'd be interested
to see what the compiler error is for the operator/() is to see if
that's a shorter solution.
--
Kieran
>
> -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.*
More information about the libcamera-devel
mailing list