[libcamera-devel] [PATCH 01/11] Fixes Bug 156, which breaks libcamera on Android < 12.
Nicholas Roth
nicholas at rothemail.net
Thu Oct 27 06:24:28 CEST 2022
> cp utils/hooks/post-commit .git/hooks/post-commit
The post-commits seem to want me to replace my tabs with spaces. I use tabs
to be consistent with the surrounding codebase. Is it OK to ignore these
errors, or should I do something else?
Example:
--- src/android/camera_hal_config.cpp
+++ src/android/camera_hal_config.cpp
@@ -164,7 +164,7 @@
File file(filePath);
if (!file.exist()) {
LOG(HALConfig, Debug)
- << "Configuration file: \"" <<
filePath << "\" not found";
+ << "Configuration file: \"" << filePath << "\" not
found";
return -ENOENT;
}
---
1 potential issue detected, please review
On Tue, Oct 25, 2022 at 6:14 AM Kieran Bingham <
kieran.bingham at ideasonboard.com> wrote:
> 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.*
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20221026/ce3f141b/attachment.htm>
More information about the libcamera-devel
mailing list