[libcamera-devel] [PATCH 01/11] Fixes Bug 156, which breaks libcamera on Android < 12.
Naushir Patuck
naush at raspberrypi.com
Wed Oct 26 10:01:05 CEST 2022
Hi Nicholas,
On Wed, 26 Oct 2022 at 02:50, Nicholas Roth via libcamera-devel <
libcamera-devel at lists.libcamera.org> wrote:
> Another possible fix is to cast libcamera::utils::Duration to
> std::chrono::duration before performing division. I've tested that and it
> works. Would folks be amenable to that instead?
>
> > Ayee, this sounds like more work ... which is painful.
> Yep
>
> > What's the compiler error here?
>
Is this the compile error you get from overloading "operator /()" in
utils::Duration?
If so, could you possibly share the code snippet of code for this
implementation?
Thanks,
Naush
>
> Enable public inheritance of std::chrono::duration and override operator/
> in the class:
> /media/nroth/BIGLINUX/lineageos/external/libcxx/include/chrono:1273:81:
> error: no type named 'type' in 'std::__1::common_type<double,
> libcamera::utils::Duration>'
> typename common_type<typename _Duration::rep,
> _Rep2>::type>::value>
>
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~
> /media/nroth/BIGLINUX/lineageos/external/libcxx/include/chrono:1286:7:
> note: in instantiation of default argument for
> '__duration_divide_imp<std::__1::chrono::duration<double,
> std::__1::ratio<1, 1000000000> >, libcamera::utils::Duration>' required here
> : __duration_divide_imp<duration<_Rep1, _Period>, _Rep2>
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> /media/nroth/BIGLINUX/lineageos/external/libcxx/include/chrono:1293:10:
> note: in instantiation of template class
> 'std::__1::chrono::__duration_divide_result<std::__1::chrono::duration<double,
> std::__1::ratio<1, 1000000000> >, libcamera::utils::Duration, false>'
> requested here
> typename __duration_divide_result<duration<_Rep1, _Period>, _Rep2>::type
> ^
> ../src/ipa/raspberrypi/cam_helper.cpp:136:39: note: while substituting
> deduced template arguments into function template 'operator/' [with _Rep1 =
> double, _Period = std::__1::ratio<1, 1000000000>, _Rep2 =
> libcamera::utils::Duration]
> return (lineLength * mode_.pixelRate / Duration(1.0s)) -
> mode_.width;
> ^
> 1 error generated.
>
> Enable public inheritance of std::chrono::duration and override operator/
> at the namespace level:
> ld.lld: error: duplicate symbol:
> libcamera::operator/(libcamera::utils::Duration const&,
> libcamera::utils::Duration const&)
> >>> defined at chrono:1068
> (/media/nroth/BIGLINUX/lineageos/external/libcxx/include/chrono:1068)
> >>>
> src/libcamera/base/libcamera-base.so.0.0.1.p/backtrace.cpp.o:(libcamera::operator/(libcamera::utils::Duration
> const&, libcamera::utils::Duration const&))
> >>> defined at chrono:1068
> (/media/nroth/BIGLINUX/lineageos/external/libcxx/include/chrono:1068)
> >>>
> src/libcamera/base/libcamera-base.so.0.0.1.p/unique_fd.cpp.o:(.text._ZN9libcameradvERKNS_5utils8DurationES3_+0x0)
>
> ld.lld: error: duplicate symbol:
> libcamera::operator/(libcamera::utils::Duration const&,
> libcamera::utils::Duration const&)
> >>> defined at chrono:1068
> (/media/nroth/BIGLINUX/lineageos/external/libcxx/include/chrono:1068)
> >>>
> src/libcamera/base/libcamera-base.so.0.0.1.p/backtrace.cpp.o:(libcamera::operator/(libcamera::utils::Duration
> const&, libcamera::utils::Duration const&))
> >>> defined at chrono:1068
> (/media/nroth/BIGLINUX/lineageos/external/libcxx/include/chrono:1068)
> >>>
> src/libcamera/base/libcamera-base.so.0.0.1.p/utils.cpp.o:(.text._ZN9libcameradvERKNS_5utils8DurationES3_+0x0)
> ...
>
> 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/40fefa6d/attachment.htm>
More information about the libcamera-devel
mailing list