[libcamera-devel,01/11] Fixes Bug 156, which breaks libcamera on Android < 12.

17677 diff mbox series

From: Nicholas Roth <nicholas@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(-)

Hi Nicholas,

Thanks for breaking these up - much easier to review, and work out how
to integrate.

Could I ask you to read https://cbea.ms/git-commit/ please?

That's not written by us, but it's a close summary of how we like to see
commit messages formed for the project.



Quoting Nicholas Roth via libcamera-devel (2022-10-24 06:55:33)
> From: Nicholas Roth <nicholas@rothemail.net>
> 

So for this patch, I would propose a subject line / commit message of:

ipa: workaround libcxx duration limitation

A bug in libcxx used by clang version 11.0.2 is prevalent when building
libcamera for Android SDK30. This has been fixed and integrated upstream
with [0]. As a workaround, directly cast the affected chrono types 


[0] https://github.com/llvm/llvm-project/commit/efa6d803c624f9251d0ab7881122501bb9d27368

> ---
>  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>();

It's not yet clear to me if it's Duration / Duration that breaks, or
(explicitly) Chrono / Duration ?

Edit: Reading further I think the below won't work...


Do we need the extra casts? Is this possible? :

	/*
	 * Default to 10 milliseconds. Avoid use of chrono suffixes due
	 * to Bug: 156.
	 *
	 * \todo: Bug 156: Use a chrono suffixed duration (10ms) when possible
	 */
 	utils::Duration exposure = utils::Duration(10000) / configuration.sensor.lineDuration;
	activeState.agc.exposure = exposure.get<std::nano>();

Or perhaps even does that then allow this directly?:

	activeState.agc.exposure = utils::Duration(10000) / configuration.sensor.lineDuration;


I'm not sure which part of the bug causes the breakage, so it might be
that those are affected too. If so, please try to include a description
in the commit of precisely what the limitations are of the bug and what
we're prevented from doing.



>         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.

We use "\todo" for todo's, to match doxygen. Though this component isn't
parsed by doxygen, but it's useful to be constent.

	  * \todo Bug 156: Workaround for LLVM 

>          */
> +       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_);

Aha, ok so the limiation is that we can't do a Duration / Duration ?

(So my above suggestions are useless, but I'll leave them there as that was
my thought process above).

>         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. */

Same minor about the todo styles.

> +       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. */

	/*
	 * For multiline comments, please keep the opening '/*' and
	 * closing '*/' on their own line.
	 */

> +       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>());

(Not for your patch, but) I wonder if activeState.agc.exposure would be
better stored as a Duration. I guess it depends on where it gets used
most. (as a duration, or as a double)


>         activeState.agc.gain = stepGain;
>  }
>  
> -- 
> 2.34.1
>

Quoting Kieran Bingham (2022-10-24 11:41:12)
> Hi Nicholas,
> 
> Thanks for breaking these up - much easier to review, and work out how
> to integrate.
> 
> Could I ask you to read https://cbea.ms/git-commit/ please?
> 
> That's not written by us, but it's a close summary of how we like to see
> commit messages formed for the project.
> 
> 
> 
> Quoting Nicholas Roth via libcamera-devel (2022-10-24 06:55:33)
> > From: Nicholas Roth <nicholas@rothemail.net>
> > 
> 

Ooops, I didn't come back up to finish this:
> So for this patch, I would propose a subject line / commit message of:

ipa: workaround libcxx duration limitation
 
A bug in libcxx used by clang version 11.0.2 is prevalent when building
libcamera for Android SDK30. This has been fixed and integrated upstream
with [0].

As a workaround, directly cast the affected chrono types to prevent
operations of Chrono or Duration being divided.

[0] https://github.com/llvm/llvm-project/commit/efa6d803c624f9251d0ab7881122501bb9d27368
Bug: https://bugs.libcamera.org/show_bug.cgi?id=156

Signed-off-by: Nicholas Roth <nicholas@rothemail.net>


(Adapt, expand, correct the above of course)
--
Kieran

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.

Regards,
Naush

On Mon, 24 Oct 2022 at 06:55, Nicholas Roth via libcamera-devel <
libcamera-devel@lists.libcamera.org> wrote:

> From: Nicholas Roth <nicholas@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
>
>

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@lists.libcamera.org> wrote:
> 
> > From: Nicholas Roth <nicholas@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
> >
> >

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@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;
> >  }
> >

> 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@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@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
>

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@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@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@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.*

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?

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@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@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@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@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.*
>

Hi Nicholas,

On Wed, 26 Oct 2022 at 02:50, Nicholas Roth via libcamera-devel <
libcamera-devel@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@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@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@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@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.*
>>
>

Quoting Nicholas Roth (2022-10-26 02:50:34)
> 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?

That sounds like we still have to make global updates. Are any of those
locations where we should store a libcamera::utils::Duration instead of
a std::chrono::duration to prevent having to cast?

If casting works around the issue, is it sufficient / possible to
provide an explicit casting operator? (I've never done this to more than
the POD types, but it 'looks' like you can specify specific target
classes as an operator?)

  https://en.cppreference.com/w/cpp/language/cast_operator

Something like (I have no idea if this is something that can work, or if
it helps the compiler as it should already know the type?!)...:

class Duration : public std::chrono::duration<double, std::nano>
{
...

	explicit operator std::chrono::duration() {
		return std::chrono::duration_cast<BaseDuration>>(*this);
	}

or perhaps 'worse' ? (but at least isolated to here in the class?)...

	constexpr explicit operator double() {
		return get<std::nano>();
	}

...
}

--
Kieran

Hi,

On Wed, 26 Oct 2022 at 09:24, Kieran Bingham via libcamera-devel <
libcamera-devel@lists.libcamera.org> wrote:

> Quoting Nicholas Roth (2022-10-26 02:50:34)
> > 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?
>
> That sounds like we still have to make global updates. Are any of those
> locations where we should store a libcamera::utils::Duration instead of
> a std::chrono::duration to prevent having to cast?
>
> If casting works around the issue, is it sufficient / possible to
> provide an explicit casting operator? (I've never done this to more than
> the POD types, but it 'looks' like you can specify specific target
> classes as an operator?)
>
>   https://en.cppreference.com/w/cpp/language/cast_operator


Casting like this is indeed possible, but even then, the changes required
are spread across the codebase.  IMO this is undesirable for readability,
and error prone as any future developers must know to do this cast
if they were to do computations with utils::Duration.

I hope there is a centralized solution with operator / overloading, but
if there's no choice...

Naush


>
> Something like (I have no idea if this is something that can work, or if
> it helps the compiler as it should already know the type?!)...:
>
> class Duration : public std::chrono::duration<double, std::nano>
> {
> ...
>
>         explicit operator std::chrono::duration() {
>                 return std::chrono::duration_cast<BaseDuration>>(*this);
>         }
>
> or perhaps 'worse' ? (but at least isolated to here in the class?)...
>
>         constexpr explicit operator double() {
>                 return get<std::nano>();
>         }
>
> ...
> }
>
> --
> Kieran
>

Quoting Naushir Patuck (2022-10-26 09:46:51)
> Hi,
> 
> On Wed, 26 Oct 2022 at 09:24, Kieran Bingham via libcamera-devel <
> libcamera-devel@lists.libcamera.org> wrote:
> 
> > Quoting Nicholas Roth (2022-10-26 02:50:34)
> > > 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?
> >
> > That sounds like we still have to make global updates. Are any of those
> > locations where we should store a libcamera::utils::Duration instead of
> > a std::chrono::duration to prevent having to cast?
> >
> > If casting works around the issue, is it sufficient / possible to
> > provide an explicit casting operator? (I've never done this to more than
> > the POD types, but it 'looks' like you can specify specific target
> > classes as an operator?)
> >
> >   https://en.cppreference.com/w/cpp/language/cast_operator
> 
> 
> Casting like this is indeed possible, but even then, the changes required
> are spread across the codebase.  IMO this is undesirable for readability,
> and error prone as any future developers must know to do this cast
> if they were to do computations with utils::Duration.
> 
> I hope there is a centralized solution with operator / overloading, but
> if there's no choice...

ahh, I thought that would allow the compiler to do the implicit casting,
if the type was defined.

But perhaps the clue is in the name "cast_operator"

--
Kieran

> 
> Naush
> 
> 
> >
> > Something like (I have no idea if this is something that can work, or if
> > it helps the compiler as it should already know the type?!)...:
> >
> > class Duration : public std::chrono::duration<double, std::nano>
> > {
> > ...
> >
> >         explicit operator std::chrono::duration() {
> >                 return std::chrono::duration_cast<BaseDuration>>(*this);
> >         }
> >
> > or perhaps 'worse' ? (but at least isolated to here in the class?)...
> >
> >         constexpr explicit operator double() {
> >                 return get<std::nano>();
> >         }
> >
> > ...
> > }
> >
> > --
> > Kieran
> >

Naushir,
> I hope there is a centralized solution with operator / overloading, but
> if there's no choice...

The only centralized solution I've been able to find with operator/ that
*might* work has some major limitations. Namely, it only really works when
the LHS is a libcamera::utils::Duration and you get weird errors when it's
on the RHS. I can't overload at the namespace level because then it causes
ambiguity with (sometimes even linker errors with) existing std::chrono
overloads.

Kieran,
> That sounds like we still have to make global updates. Are any of those
> locations where we should store a libcamera::utils::Duration instead of
> a std::chrono::duration to prevent having to cast?
Did you mean vice versa (store a std::chrono::duration instead of
libcamera::utils::Duration)? That would fix it, but from my understanding
that would miss the point of standardizing around a double-width floating
point representation at std::nano timebase.

> If casting works around the issue, is it sufficient / possible to
> provide an explicit casting operator? (I've never done this to more than
> the POD types, but it 'looks' like you can specify specific target
> classes as an operator?)

I'm not sure what this would accomplish. I've been able to upcast
libcamera::utils::Duration to std::chrono::duration with just
`std::chrono::duration(x)` without adding an operator. Note that this seems
to have nuked my <TAB>s, which I used instead of spaces for indentation to
be consistent with surrounding code.

I'm also appending the proposed v2 of this patch below to add clarity to
what I propose:
From 4efd469a63288b89c65be45ea1724ccde4ec486f Mon Sep 17 00:00:00 2001
From: Nicholas Roth <nicholas@rothemail.net>
Date: Mon, 24 Oct 2022 00:03:25 -0500
Subject: [PATCH 1/3] ipa: workaround libcxx duration limitation

A bug in libcxx [0] used by clang version 11.0.2 is prevalent when
building libcamera for Android SDK30. This has been fixed and
integrated upstream with [1].

As a workaround, directly cast libcamera::utils::Duration objects to
std::chrono::duration when dividing.

Alternatives evaluated:
Considered: Enable public inheritance of std::chrono::duration and
override operator/ in the class.
Outcome: Does not fix the original compiler error.

Considered: Enable public inheritance of std::chrono::duration and
override operator/ in the libcamera namespace.
Outcome: new compiler error:
ld.lld: error: duplicate symbol: libcamera::operator/
(libcamera::utils::Duration const&, libcamera::utils::Duration const&)

Considered: Use private inheritance of std::chrono::duration and
re-implement a pass-through version of each std::chrono::duration
operator within libcamera::utils::Duration and use template
metaprogramming to fix the division operator.
Outcome: Testing shows that this would introduce substantial
limitations, i.e. requring the Duration object to be on the LHS of any
arithmetic operation with other numeric types. This also substantially
increases implementation complexity.

Considered: Extract double values from libcamera::utils::Duration
objects and use those to divide.
Outcome: This creates substantial readability and unit-safety issues.

[0] https://github.com/llvm/llvm-project/issues/40475
[1]
https://github.com/llvm/llvm-project/commit/efa6d803c624f9251d0ab7881122501bb9d27368
Bug: https://bugs.libcamera.org/show_bug.cgi?id=156

Signed-off-by: Nicholas Roth <nicholas@rothemail.net>
---
 src/ipa/ipu3/algorithms/agc.cpp            | 17 +++++++++++------
 src/ipa/raspberrypi/cam_helper.cpp         |  6 +++---
 src/ipa/raspberrypi/cam_helper_imx296.cpp  |  3 ++-
 src/ipa/raspberrypi/controller/rpi/agc.cpp | 12 ++++++++----
 src/ipa/raspberrypi/controller/rpi/lux.cpp |  3 ++-
 src/ipa/rkisp1/algorithms/agc.cpp          | 12 ++++++++----
 6 files changed, 34 insertions(+), 19 deletions(-)

diff --git a/src/ipa/ipu3/algorithms/agc.cpp
b/src/ipa/ipu3/algorithms/agc.cpp
index a1a3c38f..1d8518da 100644
--- a/src/ipa/ipu3/algorithms/agc.cpp
+++ b/src/ipa/ipu3/algorithms/agc.cpp
@@ -100,7 +100,8 @@ 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;
+ activeState.agc.exposure = 10ms /
+ std::chrono::duration(configuration.sensor.lineDuration);

  frameCount_ = 0;
  return 0;
@@ -240,17 +241,21 @@ void Agc::computeExposure(IPAContext &context,
IPAFrameContext &frameContext,
  * increase the gain.
  */
  utils::Duration shutterTime =
- std::clamp<utils::Duration>(exposureValue / minAnalogueGain_,
-    minShutterSpeed_, maxShutterSpeed_);
- double stepGain = std::clamp(exposureValue / shutterTime,
-     minAnalogueGain_, maxAnalogueGain_);
+ std::clamp<utils::Duration>(std::chrono::duration(exposureValue) /
+ std::chrono::duration(minAnalogueGain_),
+    minShutterSpeed_, maxShutterSpeed_);
+ double stepGain = std::clamp(std::chrono::duration(exposureValue) /
+ std::chrono::duration(shutterTime),
+ minAnalogueGain_, maxAnalogueGain_);
  LOG(IPU3Agc, Debug) << "Divided up shutter and gain are "
     << shutterTime << " and "
     << stepGain;

  IPAActiveState &activeState = context.activeState;
  /* Update the estimated exposure and gain. */
- activeState.agc.exposure = shutterTime /
configuration.sensor.lineDuration;
+ double lineDurationDouble = .get<std::nano>();
+ activeState.agc.exposure = std::chrono::duration(shutterTimeDouble) /
+ std::chrono::duration(configuration.sensor.lineDuration);
  activeState.agc.gain = stepGain;
 }

diff --git a/src/ipa/raspberrypi/cam_helper.cpp
b/src/ipa/raspberrypi/cam_helper.cpp
index d90ac1de..48a8a068 100644
--- a/src/ipa/raspberrypi/cam_helper.cpp
+++ b/src/ipa/raspberrypi/cam_helper.cpp
@@ -63,7 +63,7 @@ void CamHelper::process([[maybe_unused]] StatisticsPtr
&stats,

 uint32_t CamHelper::exposureLines(const Duration exposure, const Duration
lineLength) const
 {
- return exposure / lineLength;
+ return std::chrono::duration(exposure) /
std::chrono::duration(lineLength);
 }

 Duration CamHelper::exposure(uint32_t exposureLines, const Duration
lineLength) const
@@ -85,8 +85,8 @@ 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.
  */
- frameLengthMin = minFrameDuration / mode_.minLineLength;
- frameLengthMax = maxFrameDuration / mode_.minLineLength;
+ frameLengthMin = std::chrono::duration(minFrameDuration) /
std::chrono::duration(mode_.minLineLength);
+ frameLengthMax = std::chrono::duration(maxFrameDuration) /
std::chrono::duration(mode_.minLineLength);

  /*
  * 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..65f76c3c 100644
--- a/src/ipa/raspberrypi/cam_helper_imx296.cpp
+++ b/src/ipa/raspberrypi/cam_helper_imx296.cpp
@@ -57,7 +57,8 @@ 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);
+ return std::max<uint32_t>(minExposureLines,
std::chrono::duration(exposure - 14.26us) /
+ std::chrono::duration(timePerLine));
 }

 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..cfa5ed90 100644
--- a/src/ipa/raspberrypi/controller/rpi/agc.cpp
+++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp
@@ -418,7 +418,8 @@ void Agc::prepare(Metadata *imageMetadata)
  Duration actualExposure = deviceStatus.shutterSpeed *
   deviceStatus.analogueGain;
  if (actualExposure) {
- status_.digitalGain = status_.totalExposureValue / actualExposure;
+ status_.digitalGain = std::chrono::duration(status_.totalExposureValue) /
+ std::chrono::duration(actualExposure);
  LOG(RPiAgc, Debug) << "Want total exposure " <<
status_.totalExposureValue;
  /*
  * Never ask for a gain < 1.0, and also impose
@@ -823,7 +824,8 @@ void Agc::divideUpExposure()
  }
  if (status_.fixedAnalogueGain == 0.0) {
  if (exposureMode_->gain[stage] * shutterTime >= exposureValue) {
- analogueGain = exposureValue / shutterTime;
+ analogueGain = std::chrono::duration(exposureValue) /
+ std::chrono::duration(shutterTime);
  break;
  }
  analogueGain = exposureMode_->gain[stage];
@@ -838,10 +840,12 @@ void Agc::divideUpExposure()
  */
  if (!status_.fixedShutter && !status_.fixedAnalogueGain &&
     status_.flickerPeriod) {
- int flickerPeriods = shutterTime / status_.flickerPeriod;
+ int flickerPeriods = std::chrono::duration(shutterTime) /
+ std::chrono::duration(status_.flickerPeriod);
  if (flickerPeriods) {
  Duration newShutterTime = flickerPeriods * status_.flickerPeriod;
- analogueGain *= shutterTime / newShutterTime;
+ analogueGain *= std::chrono::duration(shutterTime) /
+ std::chrono::duration(newShutterTime);
  /*
  * 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..430642f0 100644
--- a/src/ipa/raspberrypi/controller/rpi/lux.cpp
+++ b/src/ipa/raspberrypi/controller/rpi/lux.cpp
@@ -94,7 +94,8 @@ void Lux::process(StatisticsPtr &stats, Metadata
*imageMetadata)
  double currentY = sum / (double)num + .5;
  double gainRatio = referenceGain_ / currentGain;
  double shutterSpeedRatio =
- referenceShutterSpeed_ / deviceStatus.shutterSpeed;
+ std::chrono::duration(referenceShutterSpeed_) /
+ std::chrono::duration(deviceStatus.shutterSpeed);
  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..e8814802 100644
--- a/src/ipa/rkisp1/algorithms/agc.cpp
+++ b/src/ipa/rkisp1/algorithms/agc.cpp
@@ -74,7 +74,8 @@ 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;
+ context.activeState.agc.exposure = 10ms /
+ std::chrono::duration(context.configuration.sensor.lineDuration);

  /*
  * According to the RkISP1 documentation:
@@ -212,16 +213,19 @@ void Agc::computeExposure(IPAContext &context,
IPAFrameContext &frameContext,
  * Push the shutter time up to the maximum first, and only then
  * increase the gain.
  */
- utils::Duration shutterTime = std::clamp<utils::Duration>(exposureValue /
minAnalogueGain,
+ utils::Duration shutterTime =
std::clamp<utils::Duration>(std::chrono::duration(exposureValue) /
+ minAnalogueGain,
   minShutterSpeed, maxShutterSpeed);
- double stepGain = std::clamp(exposureValue / shutterTime,
+ double stepGain = std::clamp(std::chrono::duration(exposureValue) /
+ std::chrono::duration(shutterTime),
      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 = std::chrono::duration(shutterTime) /
+ std::chrono::duration(configuration.sensor.lineDuration);
  activeState.agc.gain = stepGain;
 }

On Wed, 26 Oct 2022 at 14:28, Nicholas Roth <nicholas@rothemail.net> wrote:

> Naushir,
> > I hope there is a centralized solution with operator / overloading, but
> > if there's no choice...
>
> The only centralized solution I've been able to find with operator/ that
> *might* work has some major limitations. Namely, it only really works
> when the LHS is a libcamera::utils::Duration and you get weird errors when
> it's on the RHS. I can't overload at the namespace level because then it
> causes ambiguity with (sometimes even linker errors with) existing
> std::chrono overloads.
>

I've briefly tried the following snippet of code:

---

diff --git a/include/libcamera/base/utils.h b/include/libcamera/base/utils.h
index eb7bcdf4c173..ebcd56475b17 100644
--- a/include/libcamera/base/utils.h
+++ b/include/libcamera/base/utils.h
@@ -358,6 +358,32 @@ public:
        }
 };

+inline double operator/(const Duration &num, const Duration &den)
+{
+       double n = num.get<std::nano>();
+       double d = den.get<std::nano>();
+
+       return n / d;
+}
+
+template<typename Rep, typename Period>
+inline double operator/(const Duration &num, const
std::chrono::duration<Rep, Period> &den)
+{
+       double n = num.get<std::nano>();
+       double d =
(std::chrono::duration_cast<std::chrono::duration<double,
std::nano>>(den)).count();
+
+       return n / d;
+}
+
+template<typename Rep, typename Period>
+inline double operator/(const std::chrono::duration<Rep, Period> &num,
const Duration &den)
+{
+       double n =
(std::chrono::duration_cast<std::chrono::duration<double,
std::nano>>(num)).count();
+       double d = den.get<std::nano>();
+
+       return n / d;
+}
+
 template<typename T>
 decltype(auto) abs_diff(const T &a, const T &b)

---
It compiles/links ok on clang 11.0.1 (the only version of clang-11 that is
available on my platform, but does not exactly match your version).
Functionally it seems to do the right thing as well.
Are you able to try this out in your environment to see if it resolves your
compile issues?

Regards,
Naush


>
> Kieran,
> > That sounds like we still have to make global updates. Are any of those
> > locations where we should store a libcamera::utils::Duration instead of
> > a std::chrono::duration to prevent having to cast?
> Did you mean vice versa (store a std::chrono::duration instead of
> libcamera::utils::Duration)? That would fix it, but from my understanding
> that would miss the point of standardizing around a double-width floating
> point representation at std::nano timebase.
>
> > If casting works around the issue, is it sufficient / possible to
> > provide an explicit casting operator? (I've never done this to more than
> > the POD types, but it 'looks' like you can specify specific target
> > classes as an operator?)
>
> I'm not sure what this would accomplish. I've been able to upcast
> libcamera::utils::Duration to std::chrono::duration with just
> `std::chrono::duration(x)` without adding an operator. Note that this seems
> to have nuked my <TAB>s, which I used instead of spaces for indentation to
> be consistent with surrounding code.
>
> I'm also appending the proposed v2 of this patch below to add clarity to
> what I propose:
> From 4efd469a63288b89c65be45ea1724ccde4ec486f Mon Sep 17 00:00:00 2001
> From: Nicholas Roth <nicholas@rothemail.net>
> Date: Mon, 24 Oct 2022 00:03:25 -0500
> Subject: [PATCH 1/3] ipa: workaround libcxx duration limitation
>
> A bug in libcxx [0] used by clang version 11.0.2 is prevalent when
> building libcamera for Android SDK30. This has been fixed and
> integrated upstream with [1].
>
> As a workaround, directly cast libcamera::utils::Duration objects to
> std::chrono::duration when dividing.
>
> Alternatives evaluated:
> Considered: Enable public inheritance of std::chrono::duration and
> override operator/ in the class.
> Outcome: Does not fix the original compiler error.
>
> Considered: Enable public inheritance of std::chrono::duration and
> override operator/ in the libcamera namespace.
> Outcome: new compiler error:
> ld.lld: error: duplicate symbol: libcamera::operator/
> (libcamera::utils::Duration const&, libcamera::utils::Duration const&)
>
> Considered: Use private inheritance of std::chrono::duration and
> re-implement a pass-through version of each std::chrono::duration
> operator within libcamera::utils::Duration and use template
> metaprogramming to fix the division operator.
> Outcome: Testing shows that this would introduce substantial
> limitations, i.e. requring the Duration object to be on the LHS of any
> arithmetic operation with other numeric types. This also substantially
> increases implementation complexity.
>
> Considered: Extract double values from libcamera::utils::Duration
> objects and use those to divide.
> Outcome: This creates substantial readability and unit-safety issues.
>
> [0] https://github.com/llvm/llvm-project/issues/40475
> [1]
> https://github.com/llvm/llvm-project/commit/efa6d803c624f9251d0ab7881122501bb9d27368
> Bug: https://bugs.libcamera.org/show_bug.cgi?id=156
>
> Signed-off-by: Nicholas Roth <nicholas@rothemail.net>
> ---
>  src/ipa/ipu3/algorithms/agc.cpp            | 17 +++++++++++------
>  src/ipa/raspberrypi/cam_helper.cpp         |  6 +++---
>  src/ipa/raspberrypi/cam_helper_imx296.cpp  |  3 ++-
>  src/ipa/raspberrypi/controller/rpi/agc.cpp | 12 ++++++++----
>  src/ipa/raspberrypi/controller/rpi/lux.cpp |  3 ++-
>  src/ipa/rkisp1/algorithms/agc.cpp          | 12 ++++++++----
>  6 files changed, 34 insertions(+), 19 deletions(-)
>
> diff --git a/src/ipa/ipu3/algorithms/agc.cpp
> b/src/ipa/ipu3/algorithms/agc.cpp
> index a1a3c38f..1d8518da 100644
> --- a/src/ipa/ipu3/algorithms/agc.cpp
> +++ b/src/ipa/ipu3/algorithms/agc.cpp
> @@ -100,7 +100,8 @@ 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;
> + activeState.agc.exposure = 10ms /
> + std::chrono::duration(configuration.sensor.lineDuration);
>
>   frameCount_ = 0;
>   return 0;
> @@ -240,17 +241,21 @@ void Agc::computeExposure(IPAContext &context,
> IPAFrameContext &frameContext,
>   * increase the gain.
>   */
>   utils::Duration shutterTime =
> - std::clamp<utils::Duration>(exposureValue / minAnalogueGain_,
> -    minShutterSpeed_, maxShutterSpeed_);
> - double stepGain = std::clamp(exposureValue / shutterTime,
> -     minAnalogueGain_, maxAnalogueGain_);
> + std::clamp<utils::Duration>(std::chrono::duration(exposureValue) /
> + std::chrono::duration(minAnalogueGain_),
> +    minShutterSpeed_, maxShutterSpeed_);
> + double stepGain = std::clamp(std::chrono::duration(exposureValue) /
> + std::chrono::duration(shutterTime),
> + minAnalogueGain_, maxAnalogueGain_);
>   LOG(IPU3Agc, Debug) << "Divided up shutter and gain are "
>      << shutterTime << " and "
>      << stepGain;
>
>   IPAActiveState &activeState = context.activeState;
>   /* Update the estimated exposure and gain. */
> - activeState.agc.exposure = shutterTime /
> configuration.sensor.lineDuration;
> + double lineDurationDouble = .get<std::nano>();
> + activeState.agc.exposure = std::chrono::duration(shutterTimeDouble) /
> + std::chrono::duration(configuration.sensor.lineDuration);
>   activeState.agc.gain = stepGain;
>  }
>
> diff --git a/src/ipa/raspberrypi/cam_helper.cpp
> b/src/ipa/raspberrypi/cam_helper.cpp
> index d90ac1de..48a8a068 100644
> --- a/src/ipa/raspberrypi/cam_helper.cpp
> +++ b/src/ipa/raspberrypi/cam_helper.cpp
> @@ -63,7 +63,7 @@ void CamHelper::process([[maybe_unused]] StatisticsPtr
> &stats,
>
>  uint32_t CamHelper::exposureLines(const Duration exposure, const Duration
> lineLength) const
>  {
> - return exposure / lineLength;
> + return std::chrono::duration(exposure) /
> std::chrono::duration(lineLength);
>  }
>
>  Duration CamHelper::exposure(uint32_t exposureLines, const Duration
> lineLength) const
> @@ -85,8 +85,8 @@ 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.
>   */
> - frameLengthMin = minFrameDuration / mode_.minLineLength;
> - frameLengthMax = maxFrameDuration / mode_.minLineLength;
> + frameLengthMin = std::chrono::duration(minFrameDuration) /
> std::chrono::duration(mode_.minLineLength);
> + frameLengthMax = std::chrono::duration(maxFrameDuration) /
> std::chrono::duration(mode_.minLineLength);
>
>   /*
>   * 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..65f76c3c 100644
> --- a/src/ipa/raspberrypi/cam_helper_imx296.cpp
> +++ b/src/ipa/raspberrypi/cam_helper_imx296.cpp
> @@ -57,7 +57,8 @@ 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);
> + return std::max<uint32_t>(minExposureLines,
> std::chrono::duration(exposure - 14.26us) /
> + std::chrono::duration(timePerLine));
>  }
>
>  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..cfa5ed90 100644
> --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp
> +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp
> @@ -418,7 +418,8 @@ void Agc::prepare(Metadata *imageMetadata)
>   Duration actualExposure = deviceStatus.shutterSpeed *
>    deviceStatus.analogueGain;
>   if (actualExposure) {
> - status_.digitalGain = status_.totalExposureValue / actualExposure;
> + status_.digitalGain = std::chrono::duration(status_.totalExposureValue) /
> + std::chrono::duration(actualExposure);
>   LOG(RPiAgc, Debug) << "Want total exposure " <<
> status_.totalExposureValue;
>   /*
>   * Never ask for a gain < 1.0, and also impose
> @@ -823,7 +824,8 @@ void Agc::divideUpExposure()
>   }
>   if (status_.fixedAnalogueGain == 0.0) {
>   if (exposureMode_->gain[stage] * shutterTime >= exposureValue) {
> - analogueGain = exposureValue / shutterTime;
> + analogueGain = std::chrono::duration(exposureValue) /
> + std::chrono::duration(shutterTime);
>   break;
>   }
>   analogueGain = exposureMode_->gain[stage];
> @@ -838,10 +840,12 @@ void Agc::divideUpExposure()
>   */
>   if (!status_.fixedShutter && !status_.fixedAnalogueGain &&
>      status_.flickerPeriod) {
> - int flickerPeriods = shutterTime / status_.flickerPeriod;
> + int flickerPeriods = std::chrono::duration(shutterTime) /
> + std::chrono::duration(status_.flickerPeriod);
>   if (flickerPeriods) {
>   Duration newShutterTime = flickerPeriods * status_.flickerPeriod;
> - analogueGain *= shutterTime / newShutterTime;
> + analogueGain *= std::chrono::duration(shutterTime) /
> + std::chrono::duration(newShutterTime);
>   /*
>   * 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..430642f0 100644
> --- a/src/ipa/raspberrypi/controller/rpi/lux.cpp
> +++ b/src/ipa/raspberrypi/controller/rpi/lux.cpp
> @@ -94,7 +94,8 @@ void Lux::process(StatisticsPtr &stats, Metadata
> *imageMetadata)
>   double currentY = sum / (double)num + .5;
>   double gainRatio = referenceGain_ / currentGain;
>   double shutterSpeedRatio =
> - referenceShutterSpeed_ / deviceStatus.shutterSpeed;
> + std::chrono::duration(referenceShutterSpeed_) /
> + std::chrono::duration(deviceStatus.shutterSpeed);
>   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..e8814802 100644
> --- a/src/ipa/rkisp1/algorithms/agc.cpp
> +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> @@ -74,7 +74,8 @@ 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;
> + context.activeState.agc.exposure = 10ms /
> + std::chrono::duration(context.configuration.sensor.lineDuration);
>
>   /*
>   * According to the RkISP1 documentation:
> @@ -212,16 +213,19 @@ void Agc::computeExposure(IPAContext &context,
> IPAFrameContext &frameContext,
>   * Push the shutter time up to the maximum first, and only then
>   * increase the gain.
>   */
> - utils::Duration shutterTime = std::clamp<utils::Duration>(exposureValue
> / minAnalogueGain,
> + utils::Duration shutterTime =
> std::clamp<utils::Duration>(std::chrono::duration(exposureValue) /
> + minAnalogueGain,
>    minShutterSpeed, maxShutterSpeed);
> - double stepGain = std::clamp(exposureValue / shutterTime,
> + double stepGain = std::clamp(std::chrono::duration(exposureValue) /
> + std::chrono::duration(shutterTime),
>       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 = std::chrono::duration(shutterTime) /
> + std::chrono::duration(configuration.sensor.lineDuration);
>   activeState.agc.gain = stepGain;
>  }
>
> --
> 2.34.1
>
>
> On Wed, Oct 26, 2022 at 3:51 AM Kieran Bingham <
> kieran.bingham@ideasonboard.com> wrote:
>
>> Quoting Naushir Patuck (2022-10-26 09:46:51)
>> > Hi,
>> >
>> > On Wed, 26 Oct 2022 at 09:24, Kieran Bingham via libcamera-devel <
>> > libcamera-devel@lists.libcamera.org> wrote:
>> >
>> > > Quoting Nicholas Roth (2022-10-26 02:50:34)
>> > > > 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?
>> > >
>> > > That sounds like we still have to make global updates. Are any of
>> those
>> > > locations where we should store a libcamera::utils::Duration instead
>> of
>> > > a std::chrono::duration to prevent having to cast?
>> > >
>> > > If casting works around the issue, is it sufficient / possible to
>> > > provide an explicit casting operator? (I've never done this to more
>> than
>> > > the POD types, but it 'looks' like you can specify specific target
>> > > classes as an operator?)
>> > >
>> > >   https://en.cppreference.com/w/cpp/language/cast_operator
>> >
>> >
>> > Casting like this is indeed possible, but even then, the changes
>> required
>> > are spread across the codebase.  IMO this is undesirable for
>> readability,
>> > and error prone as any future developers must know to do this cast
>> > if they were to do computations with utils::Duration.
>> >
>> > I hope there is a centralized solution with operator / overloading, but
>> > if there's no choice...
>>
>> ahh, I thought that would allow the compiler to do the implicit casting,
>> if the type was defined.
>>
>> But perhaps the clue is in the name "cast_operator"
>>
>> --
>> Kieran
>>
>> >
>> > Naush
>> >
>> >
>> > >
>> > > Something like (I have no idea if this is something that can work, or
>> if
>> > > it helps the compiler as it should already know the type?!)...:
>> > >
>> > > class Duration : public std::chrono::duration<double, std::nano>
>> > > {
>> > > ...
>> > >
>> > >         explicit operator std::chrono::duration() {
>> > >                 return
>> std::chrono::duration_cast<BaseDuration>>(*this);
>> > >         }
>> > >
>> > > or perhaps 'worse' ? (but at least isolated to here in the class?)...
>> > >
>> > >         constexpr explicit operator double() {
>> > >                 return get<std::nano>();
>> > >         }
>> > >
>> > > ...
>> > > }
>> > >
>> > > --
>> > > Kieran
>> > >
>>
>

Naushir,

I really wanted to be wrong and was hoping that you cracked the code! But.
While the example above compiles in isolation, I still get the following
error in SDK30 when I undo my fixes:

In file included from ../src/ipa/rkisp1/algorithms/agc.cpp:8:
In file included from ../src/ipa/rkisp1/algorithms/agc.h:12:
In file included from ../include/libcamera/base/utils.h:11:
/media/nroth/BIGLINUX/lineageos/external/libcxx/include/chrono:1273:81:
error: no type named 'type' in 'std::__1::common_type<long long,
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<long long,
std::__1::ratio<1, 1000> >, 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<long
long, std::__1::ratio<1, 1000> >, libcamera::utils::Duration, false>'
requested here
typename __duration_divide_result<duration<_Rep1, _Period>, _Rep2>::type
         ^
../src/ipa/rkisp1/algorithms/agc.cpp:81:30: note: while substituting
deduced template arguments into function template 'operator/' [with _Rep1 =
long long, _Period = std::__1::ratio<1, 1000>, _Rep2 =
libcamera::utils::Duration]
        long double exposure = 10ms /
context.configuration.sensor.lineDuration;
                                    ^
1 error generated.


On Wed, Oct 26, 2022 at 9:26 AM Naushir Patuck <naush@raspberrypi.com>
wrote:

> On Wed, 26 Oct 2022 at 14:28, Nicholas Roth <nicholas@rothemail.net>
> wrote:
>
>> Naushir,
>> > I hope there is a centralized solution with operator / overloading, but
>> > if there's no choice...
>>
>> The only centralized solution I've been able to find with operator/ that
>> *might* work has some major limitations. Namely, it only really works
>> when the LHS is a libcamera::utils::Duration and you get weird errors when
>> it's on the RHS. I can't overload at the namespace level because then it
>> causes ambiguity with (sometimes even linker errors with) existing
>> std::chrono overloads.
>>
>
> I've briefly tried the following snippet of code:
>
> ---
>
> diff --git a/include/libcamera/base/utils.h
> b/include/libcamera/base/utils.h
> index eb7bcdf4c173..ebcd56475b17 100644
> --- a/include/libcamera/base/utils.h
> +++ b/include/libcamera/base/utils.h
> @@ -358,6 +358,32 @@ public:
>         }
>  };
>
> +inline double operator/(const Duration &num, const Duration &den)
> +{
> +       double n = num.get<std::nano>();
> +       double d = den.get<std::nano>();
> +
> +       return n / d;
> +}
> +
> +template<typename Rep, typename Period>
> +inline double operator/(const Duration &num, const
> std::chrono::duration<Rep, Period> &den)
> +{
> +       double n = num.get<std::nano>();
> +       double d =
> (std::chrono::duration_cast<std::chrono::duration<double,
> std::nano>>(den)).count();
> +
> +       return n / d;
> +}
> +
> +template<typename Rep, typename Period>
> +inline double operator/(const std::chrono::duration<Rep, Period> &num,
> const Duration &den)
> +{
> +       double n =
> (std::chrono::duration_cast<std::chrono::duration<double,
> std::nano>>(num)).count();
> +       double d = den.get<std::nano>();
> +
> +       return n / d;
> +}
> +
>  template<typename T>
>  decltype(auto) abs_diff(const T &a, const T &b)
>
> ---
> It compiles/links ok on clang 11.0.1 (the only version of clang-11 that is
> available on my platform, but does not exactly match your version).
> Functionally it seems to do the right thing as well.
> Are you able to try this out in your environment to see if it resolves
> your compile issues?
>
> Regards,
> Naush
>
>
>>
>> Kieran,
>> > That sounds like we still have to make global updates. Are any of those
>> > locations where we should store a libcamera::utils::Duration instead of
>> > a std::chrono::duration to prevent having to cast?
>> Did you mean vice versa (store a std::chrono::duration instead of
>> libcamera::utils::Duration)? That would fix it, but from my understanding
>> that would miss the point of standardizing around a double-width floating
>> point representation at std::nano timebase.
>>
>> > If casting works around the issue, is it sufficient / possible to
>> > provide an explicit casting operator? (I've never done this to more than
>> > the POD types, but it 'looks' like you can specify specific target
>> > classes as an operator?)
>>
>> I'm not sure what this would accomplish. I've been able to upcast
>> libcamera::utils::Duration to std::chrono::duration with just
>> `std::chrono::duration(x)` without adding an operator. Note that this seems
>> to have nuked my <TAB>s, which I used instead of spaces for indentation to
>> be consistent with surrounding code.
>>
>> I'm also appending the proposed v2 of this patch below to add clarity to
>> what I propose:
>> From 4efd469a63288b89c65be45ea1724ccde4ec486f Mon Sep 17 00:00:00 2001
>> From: Nicholas Roth <nicholas@rothemail.net>
>> Date: Mon, 24 Oct 2022 00:03:25 -0500
>> Subject: [PATCH 1/3] ipa: workaround libcxx duration limitation
>>
>> A bug in libcxx [0] used by clang version 11.0.2 is prevalent when
>> building libcamera for Android SDK30. This has been fixed and
>> integrated upstream with [1].
>>
>> As a workaround, directly cast libcamera::utils::Duration objects to
>> std::chrono::duration when dividing.
>>
>> Alternatives evaluated:
>> Considered: Enable public inheritance of std::chrono::duration and
>> override operator/ in the class.
>> Outcome: Does not fix the original compiler error.
>>
>> Considered: Enable public inheritance of std::chrono::duration and
>> override operator/ in the libcamera namespace.
>> Outcome: new compiler error:
>> ld.lld: error: duplicate symbol: libcamera::operator/
>> (libcamera::utils::Duration const&, libcamera::utils::Duration const&)
>>
>> Considered: Use private inheritance of std::chrono::duration and
>> re-implement a pass-through version of each std::chrono::duration
>> operator within libcamera::utils::Duration and use template
>> metaprogramming to fix the division operator.
>> Outcome: Testing shows that this would introduce substantial
>> limitations, i.e. requring the Duration object to be on the LHS of any
>> arithmetic operation with other numeric types. This also substantially
>> increases implementation complexity.
>>
>> Considered: Extract double values from libcamera::utils::Duration
>> objects and use those to divide.
>> Outcome: This creates substantial readability and unit-safety issues.
>>
>> [0] https://github.com/llvm/llvm-project/issues/40475
>> [1]
>> https://github.com/llvm/llvm-project/commit/efa6d803c624f9251d0ab7881122501bb9d27368
>> Bug: https://bugs.libcamera.org/show_bug.cgi?id=156
>>
>> Signed-off-by: Nicholas Roth <nicholas@rothemail.net>
>> ---
>>  src/ipa/ipu3/algorithms/agc.cpp            | 17 +++++++++++------
>>  src/ipa/raspberrypi/cam_helper.cpp         |  6 +++---
>>  src/ipa/raspberrypi/cam_helper_imx296.cpp  |  3 ++-
>>  src/ipa/raspberrypi/controller/rpi/agc.cpp | 12 ++++++++----
>>  src/ipa/raspberrypi/controller/rpi/lux.cpp |  3 ++-
>>  src/ipa/rkisp1/algorithms/agc.cpp          | 12 ++++++++----
>>  6 files changed, 34 insertions(+), 19 deletions(-)
>>
>> diff --git a/src/ipa/ipu3/algorithms/agc.cpp
>> b/src/ipa/ipu3/algorithms/agc.cpp
>> index a1a3c38f..1d8518da 100644
>> --- a/src/ipa/ipu3/algorithms/agc.cpp
>> +++ b/src/ipa/ipu3/algorithms/agc.cpp
>> @@ -100,7 +100,8 @@ 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;
>> + activeState.agc.exposure = 10ms /
>> + std::chrono::duration(configuration.sensor.lineDuration);
>>
>>   frameCount_ = 0;
>>   return 0;
>> @@ -240,17 +241,21 @@ void Agc::computeExposure(IPAContext &context,
>> IPAFrameContext &frameContext,
>>   * increase the gain.
>>   */
>>   utils::Duration shutterTime =
>> - std::clamp<utils::Duration>(exposureValue / minAnalogueGain_,
>> -    minShutterSpeed_, maxShutterSpeed_);
>> - double stepGain = std::clamp(exposureValue / shutterTime,
>> -     minAnalogueGain_, maxAnalogueGain_);
>> + std::clamp<utils::Duration>(std::chrono::duration(exposureValue) /
>> + std::chrono::duration(minAnalogueGain_),
>> +    minShutterSpeed_, maxShutterSpeed_);
>> + double stepGain = std::clamp(std::chrono::duration(exposureValue) /
>> + std::chrono::duration(shutterTime),
>> + minAnalogueGain_, maxAnalogueGain_);
>>   LOG(IPU3Agc, Debug) << "Divided up shutter and gain are "
>>      << shutterTime << " and "
>>      << stepGain;
>>
>>   IPAActiveState &activeState = context.activeState;
>>   /* Update the estimated exposure and gain. */
>> - activeState.agc.exposure = shutterTime /
>> configuration.sensor.lineDuration;
>> + double lineDurationDouble = .get<std::nano>();
>> + activeState.agc.exposure = std::chrono::duration(shutterTimeDouble) /
>> + std::chrono::duration(configuration.sensor.lineDuration);
>>   activeState.agc.gain = stepGain;
>>  }
>>
>> diff --git a/src/ipa/raspberrypi/cam_helper.cpp
>> b/src/ipa/raspberrypi/cam_helper.cpp
>> index d90ac1de..48a8a068 100644
>> --- a/src/ipa/raspberrypi/cam_helper.cpp
>> +++ b/src/ipa/raspberrypi/cam_helper.cpp
>> @@ -63,7 +63,7 @@ void CamHelper::process([[maybe_unused]] StatisticsPtr
>> &stats,
>>
>>  uint32_t CamHelper::exposureLines(const Duration exposure, const
>> Duration lineLength) const
>>  {
>> - return exposure / lineLength;
>> + return std::chrono::duration(exposure) /
>> std::chrono::duration(lineLength);
>>  }
>>
>>  Duration CamHelper::exposure(uint32_t exposureLines, const Duration
>> lineLength) const
>> @@ -85,8 +85,8 @@ 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.
>>   */
>> - frameLengthMin = minFrameDuration / mode_.minLineLength;
>> - frameLengthMax = maxFrameDuration / mode_.minLineLength;
>> + frameLengthMin = std::chrono::duration(minFrameDuration) /
>> std::chrono::duration(mode_.minLineLength);
>> + frameLengthMax = std::chrono::duration(maxFrameDuration) /
>> std::chrono::duration(mode_.minLineLength);
>>
>>   /*
>>   * 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..65f76c3c 100644
>> --- a/src/ipa/raspberrypi/cam_helper_imx296.cpp
>> +++ b/src/ipa/raspberrypi/cam_helper_imx296.cpp
>> @@ -57,7 +57,8 @@ 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);
>> + return std::max<uint32_t>(minExposureLines,
>> std::chrono::duration(exposure - 14.26us) /
>> + std::chrono::duration(timePerLine));
>>  }
>>
>>  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..cfa5ed90 100644
>> --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp
>> +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp
>> @@ -418,7 +418,8 @@ void Agc::prepare(Metadata *imageMetadata)
>>   Duration actualExposure = deviceStatus.shutterSpeed *
>>    deviceStatus.analogueGain;
>>   if (actualExposure) {
>> - status_.digitalGain = status_.totalExposureValue / actualExposure;
>> + status_.digitalGain = std::chrono::duration(status_.totalExposureValue)
>> /
>> + std::chrono::duration(actualExposure);
>>   LOG(RPiAgc, Debug) << "Want total exposure " <<
>> status_.totalExposureValue;
>>   /*
>>   * Never ask for a gain < 1.0, and also impose
>> @@ -823,7 +824,8 @@ void Agc::divideUpExposure()
>>   }
>>   if (status_.fixedAnalogueGain == 0.0) {
>>   if (exposureMode_->gain[stage] * shutterTime >= exposureValue) {
>> - analogueGain = exposureValue / shutterTime;
>> + analogueGain = std::chrono::duration(exposureValue) /
>> + std::chrono::duration(shutterTime);
>>   break;
>>   }
>>   analogueGain = exposureMode_->gain[stage];
>> @@ -838,10 +840,12 @@ void Agc::divideUpExposure()
>>   */
>>   if (!status_.fixedShutter && !status_.fixedAnalogueGain &&
>>      status_.flickerPeriod) {
>> - int flickerPeriods = shutterTime / status_.flickerPeriod;
>> + int flickerPeriods = std::chrono::duration(shutterTime) /
>> + std::chrono::duration(status_.flickerPeriod);
>>   if (flickerPeriods) {
>>   Duration newShutterTime = flickerPeriods * status_.flickerPeriod;
>> - analogueGain *= shutterTime / newShutterTime;
>> + analogueGain *= std::chrono::duration(shutterTime) /
>> + std::chrono::duration(newShutterTime);
>>   /*
>>   * 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..430642f0 100644
>> --- a/src/ipa/raspberrypi/controller/rpi/lux.cpp
>> +++ b/src/ipa/raspberrypi/controller/rpi/lux.cpp
>> @@ -94,7 +94,8 @@ void Lux::process(StatisticsPtr &stats, Metadata
>> *imageMetadata)
>>   double currentY = sum / (double)num + .5;
>>   double gainRatio = referenceGain_ / currentGain;
>>   double shutterSpeedRatio =
>> - referenceShutterSpeed_ / deviceStatus.shutterSpeed;
>> + std::chrono::duration(referenceShutterSpeed_) /
>> + std::chrono::duration(deviceStatus.shutterSpeed);
>>   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..e8814802 100644
>> --- a/src/ipa/rkisp1/algorithms/agc.cpp
>> +++ b/src/ipa/rkisp1/algorithms/agc.cpp
>> @@ -74,7 +74,8 @@ 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;
>> + context.activeState.agc.exposure = 10ms /
>> + std::chrono::duration(context.configuration.sensor.lineDuration);
>>
>>   /*
>>   * According to the RkISP1 documentation:
>> @@ -212,16 +213,19 @@ void Agc::computeExposure(IPAContext &context,
>> IPAFrameContext &frameContext,
>>   * Push the shutter time up to the maximum first, and only then
>>   * increase the gain.
>>   */
>> - utils::Duration shutterTime = std::clamp<utils::Duration>(exposureValue
>> / minAnalogueGain,
>> + utils::Duration shutterTime =
>> std::clamp<utils::Duration>(std::chrono::duration(exposureValue) /
>> + minAnalogueGain,
>>    minShutterSpeed, maxShutterSpeed);
>> - double stepGain = std::clamp(exposureValue / shutterTime,
>> + double stepGain = std::clamp(std::chrono::duration(exposureValue) /
>> + std::chrono::duration(shutterTime),
>>       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 = std::chrono::duration(shutterTime) /
>> + std::chrono::duration(configuration.sensor.lineDuration);
>>   activeState.agc.gain = stepGain;
>>  }
>>
>> --
>> 2.34.1
>>
>>
>> On Wed, Oct 26, 2022 at 3:51 AM Kieran Bingham <
>> kieran.bingham@ideasonboard.com> wrote:
>>
>>> Quoting Naushir Patuck (2022-10-26 09:46:51)
>>> > Hi,
>>> >
>>> > On Wed, 26 Oct 2022 at 09:24, Kieran Bingham via libcamera-devel <
>>> > libcamera-devel@lists.libcamera.org> wrote:
>>> >
>>> > > Quoting Nicholas Roth (2022-10-26 02:50:34)
>>> > > > 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?
>>> > >
>>> > > That sounds like we still have to make global updates. Are any of
>>> those
>>> > > locations where we should store a libcamera::utils::Duration instead
>>> of
>>> > > a std::chrono::duration to prevent having to cast?
>>> > >
>>> > > If casting works around the issue, is it sufficient / possible to
>>> > > provide an explicit casting operator? (I've never done this to more
>>> than
>>> > > the POD types, but it 'looks' like you can specify specific target
>>> > > classes as an operator?)
>>> > >
>>> > >   https://en.cppreference.com/w/cpp/language/cast_operator
>>> >
>>> >
>>> > Casting like this is indeed possible, but even then, the changes
>>> required
>>> > are spread across the codebase.  IMO this is undesirable for
>>> readability,
>>> > and error prone as any future developers must know to do this cast
>>> > if they were to do computations with utils::Duration.
>>> >
>>> > I hope there is a centralized solution with operator / overloading, but
>>> > if there's no choice...
>>>
>>> ahh, I thought that would allow the compiler to do the implicit casting,
>>> if the type was defined.
>>>
>>> But perhaps the clue is in the name "cast_operator"
>>>
>>> --
>>> Kieran
>>>
>>> >
>>> > Naush
>>> >
>>> >
>>> > >
>>> > > Something like (I have no idea if this is something that can work,
>>> or if
>>> > > it helps the compiler as it should already know the type?!)...:
>>> > >
>>> > > class Duration : public std::chrono::duration<double, std::nano>
>>> > > {
>>> > > ...
>>> > >
>>> > >         explicit operator std::chrono::duration() {
>>> > >                 return
>>> std::chrono::duration_cast<BaseDuration>>(*this);
>>> > >         }
>>> > >
>>> > > or perhaps 'worse' ? (but at least isolated to here in the class?)...
>>> > >
>>> > >         constexpr explicit operator double() {
>>> > >                 return get<std::nano>();
>>> > >         }
>>> > >
>>> > > ...
>>> > > }
>>> > >
>>> > > --
>>> > > Kieran
>>> > >
>>>
>>

>  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@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@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@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@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.*
>

Quoting Nicholas Roth (2022-10-27 05:24:28)
> >  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?

That's odd - it shouldn't be changing tabs for spaces?

What's your tabsize set to ? We use 8-character tabs.

 
> 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;

In my terminal, this change looks correct:

		LOG(HALConfig, Debug)
			<< "Configuration file: \"" << filePath << "\" not found";

We often take the debug print lines to the following line to allow a
longer length without breaking the text, or hitting our line length
limit. We only use a single indentation from the start of the previous
line

--
Kieran


>         }
> 
> ---
> 1 potential issue detected, please review

Hey, so now I'm *really* confused by the post-commit. It almost seems like
it wants me to use a mix of spaces and tabs, which I've never seen before.
Let me know what you make of this:

I start with this suggestion:
--- src/ipa/ipu3/algorithms/agc.cpp
+++ src/ipa/ipu3/algorithms/agc.cpp
@@ -101,7 +101,7 @@
        /* Configure the default exposure and gain. */
        activeState.agc.gain = std::max(minAnalogueGain_, kMinAnalogueGain);
        activeState.agc.exposure = 10ms /
-
std::chrono::duration(configuration.sensor.lineDuration);
+
 std::chrono::duration(configuration.sensor.lineDuration);

        frameCount_ = 0;
        return 0;

Then when I delete a tab as suggested, I end up with this:
--- src/ipa/ipu3/algorithms/agc.cpp
+++ src/ipa/ipu3/algorithms/agc.cpp
@@ -101,7 +101,7 @@
        /* Configure the default exposure and gain. */
        activeState.agc.gain = std::max(minAnalogueGain_, kMinAnalogueGain);
        activeState.agc.exposure = 10ms /
-
std::chrono::duration(configuration.sensor.lineDuration);
+
 std::chrono::duration(configuration.sensor.lineDuration);

        frameCount_ = 0;
        return 0;

If a mix of tabs and spaces is what's standard for us, I'll do it. I just
thought to ask first since it would be the first time I've seen a project
with this convention.

Thanks,
-Nicholas

On Thu, Oct 27, 2022 at 6:18 AM Kieran Bingham <
kieran.bingham@ideasonboard.com> wrote:

> Quoting Nicholas Roth (2022-10-27 05:24:28)
> > >  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?
>
> That's odd - it shouldn't be changing tabs for spaces?
>
> What's your tabsize set to ? We use 8-character tabs.
>
>
> > 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;
>
> In my terminal, this change looks correct:
>
>                 LOG(HALConfig, Debug)
>                         << "Configuration file: \"" << filePath << "\" not
> found";
>
> We often take the debug print lines to the following line to allow a
> longer length without breaking the text, or hitting our line length
> limit. We only use a single indentation from the start of the previous
> line
>
> --
> Kieran
>
>
> >         }
> >
> > ---
> > 1 potential issue detected, please review
>

Quoting Nicholas Roth (2022-10-27 22:52:46)
> Hey, so now I'm *really* confused by the post-commit. It almost seems like
> it wants me to use a mix of spaces and tabs, which I've never seen before.
> Let me know what you make of this:
> 
> I start with this suggestion:
> --- src/ipa/ipu3/algorithms/agc.cpp
> +++ src/ipa/ipu3/algorithms/agc.cpp
> @@ -101,7 +101,7 @@
>         /* Configure the default exposure and gain. */
>         activeState.agc.gain = std::max(minAnalogueGain_, kMinAnalogueGain);
>         activeState.agc.exposure = 10ms /
> -
> std::chrono::duration(configuration.sensor.lineDuration);
> +
>  std::chrono::duration(configuration.sensor.lineDuration);
> 
>         frameCount_ = 0;
>         return 0;
> 
> Then when I delete a tab as suggested, I end up with this:
> --- src/ipa/ipu3/algorithms/agc.cpp
> +++ src/ipa/ipu3/algorithms/agc.cpp
> @@ -101,7 +101,7 @@
>         /* Configure the default exposure and gain. */
>         activeState.agc.gain = std::max(minAnalogueGain_, kMinAnalogueGain);
>         activeState.agc.exposure = 10ms /
> -
> std::chrono::duration(configuration.sensor.lineDuration);
> +
>  std::chrono::duration(configuration.sensor.lineDuration);
> 
>         frameCount_ = 0;
>         return 0;
> 
> If a mix of tabs and spaces is what's standard for us, I'll do it. I just
> thought to ask first since it would be the first time I've seen a project
> with this convention.

There 'are' a mix of spaces and tabs - but it's about alignment.

It's really hard to determine from your mail above which is
space/tabs/indent due to the mail wraps. but hopefully this can lay it
out. clang-format gets it right most of the time (which is what the post
commit hook is using) but it always needs a quick review because
sometimes clang-format does things that are just plain stupid or less
readable. So it's there for guidance rather than specification.


~######~ == Tab (8 per tab)
.....    == Spaces ( 1 per space )


So these single indents for example are all tabs:

~######~if (cfg.size != size) {
~######~~######~LOG(UVC, Debug)
~######~~######~~######~<< "Adjusting size from " << size << " to " << cfg.size;
~######~~######~status = Adjusted;
~######~}

But if there was something which needs to be directly aligned below a
character on the previous line, then it's tabs to fill the gap, with
spaces for the alignment:

For example on this function prototype, the paramaters are aligned:

~######~int exportFrameBuffers(Camera *camera, Stream *stream,
~######~~######~~######~.......std::vector<std::unique_ptr<FrameBuffer>> *buffers) override;

As I understand it, that's pretty close to the style in the kernel:

  https://www.kernel.org/doc/html/v4.10/process/coding-style.html


And this is the extract from our https://libcamera.org/coding-style.html#coding-style-guidelines

  Coding Style

Even if the programming language in use is different, the project
embraces the Linux Kernel Coding Style with a few exception and some C++
specificities.

In particular, from the kernel style document, the following section are
adopted:

    1 “Indentation”
    2 “Breaking Long Lines” striving to fit code within 80 columns and accepting up to 120 columns when necessary
    3 “Placing Braces and Spaces”
    3.1 “Spaces”
    8 “Commenting” with the exception that in-function comments are not always un-welcome.


> 
> Thanks,
> -Nicholas
> 
> On Thu, Oct 27, 2022 at 6:18 AM Kieran Bingham <
> kieran.bingham@ideasonboard.com> wrote:
> 
> > Quoting Nicholas Roth (2022-10-27 05:24:28)
> > > >  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?
> >
> > That's odd - it shouldn't be changing tabs for spaces?
> >
> > What's your tabsize set to ? We use 8-character tabs.
> >
> >
> > > 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;
> >
> > In my terminal, this change looks correct:
> >
> >                 LOG(HALConfig, Debug)
> >                         << "Configuration file: \"" << filePath << "\" not
> > found";
> >
> > We often take the debug print lines to the following line to allow a
> > longer length without breaking the text, or hitting our line length
> > limit. We only use a single indentation from the start of the previous
> > line
> >
> > --
> > Kieran
> >
> >
> > >         }
> > >
> > > ---
> > > 1 potential issue detected, please review
> >