| Message ID | 20250123140727.458567-3-dan.scally@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
On Thu, Jan 23, 2025 at 02:07:26PM +0000, Daniel Scally wrote: > Update the ExposureModeHelper class to compensate for flickering > light sources in the ExposureModeHelper::splitExposure() function. > The adjustment simply caps exposure time at a multiple of the given > flicker period and compensates for any loss in the effective exposure > value by increasing analogue and then digital gain. > > Initially in the one call-site for this function, a std::nullopt is > passed to make this a no-op. > > Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com> > --- > Changes in v2: > > - Added a function to perform all the exposure time functionality in a > single place so we're not repeating ourselves > - Called that function in all the return sites rather than just one so > the flicker mitigation takes effect using exposure from the stages > list > - Switched the flickerPeriod input to a std::optional > - Clamped the calculated exposure time to guarantee it can't go beneath > the configured minima > > src/ipa/libipa/agc_mean_luminance.cpp | 3 +- > src/ipa/libipa/exposure_mode_helper.cpp | 37 ++++++++++++++++++++++--- > src/ipa/libipa/exposure_mode_helper.h | 6 +++- > 3 files changed, 40 insertions(+), 6 deletions(-) > > diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp > index 02555a44..273ec4e5 100644 > --- a/src/ipa/libipa/agc_mean_luminance.cpp > +++ b/src/ipa/libipa/agc_mean_luminance.cpp > @@ -8,6 +8,7 @@ > #include "agc_mean_luminance.h" > > #include <cmath> > +#include <optional> > > #include <libcamera/base/log.h> > #include <libcamera/control_ids.h> > @@ -560,7 +561,7 @@ AgcMeanLuminance::calculateNewEv(uint32_t constraintModeIndex, > newExposureValue = filterExposure(newExposureValue); > > frameCount_++; > - return exposureModeHelper->splitExposure(newExposureValue); > + return exposureModeHelper->splitExposure(newExposureValue, std::nullopt); > } > > /** > diff --git a/src/ipa/libipa/exposure_mode_helper.cpp b/src/ipa/libipa/exposure_mode_helper.cpp > index f235316d..4e1ba943 100644 > --- a/src/ipa/libipa/exposure_mode_helper.cpp > +++ b/src/ipa/libipa/exposure_mode_helper.cpp > @@ -7,6 +7,7 @@ > #include "exposure_mode_helper.h" > > #include <algorithm> > +#include <optional> > > #include <libcamera/base/log.h> > > @@ -118,9 +119,31 @@ double ExposureModeHelper::clampGain(double gain) const > return std::clamp(gain, minGain_, maxGain_); > } > > +utils::Duration > +ExposureModeHelper::calculateExposureTime(utils::Duration exposure, double stageGain, > + std::optional<utils::Duration> flickerPeriod) const > +{ > + utils::Duration exposureTime; > + > + exposureTime = clampExposureTime(exposure / stageGain); > + > + /* > + * If we haven't been given a flicker period to adjust for or if it's > + * longer than the exposure time that we need to set then there's not > + * much we can do to compensate. > + */ > + if (!flickerPeriod.has_value() || flickerPeriod.value() >= exposureTime) > + return exposureTime; > + > + unsigned int flickerPeriods = exposureTime / flickerPeriod.value(); > + > + return clampExposureTime(flickerPeriods * flickerPeriod.value()); Something doesn't seem right to me...? Premise: - exposure * stageGain gives us sufficient total effective exposure - flickerPeriod.has_value() and flickerPeriod < exposureTime Scenario: - flickerPeriods * flickerPeriod < minExposureTime_ - Which is possible afaiu if flickerPeriod < exposureTime = minExposureTime_ - Thus we would end up with exposureTime = minExposureTime_ at a value that is not a multiple of flickerPeriod For example: - Let: - exposureTime = minExposureTime_ - flickerPeriod = minExposureTime_ * 0.8 - Then: - double dFlickerPeriods = exposureTime / flickerPeriod = minExposureTime_ / (minExposureTime_ * 0.8) = 1.25 - unsigned int flickerPeriods = (unsigned int)dFlickerPeriods = 1 - Thus: - ret = flickerPeriods * flickerPeriod = 1 * minExposureTime_ * 0.8 - Since ret < minExposureTime_, it will get clamped to minExposureTime_, and is not a multiple of flickerPeriod Result: - The exposure time doesn't adjust for flicker, since it's not a multiple of flickerPeriod Expected result: - We go up one more stage in the splitExposure() loop and run calculateExposureTime() on that next stage - exposureTime would be high enough that we have leeway to lower exposureTime to flicker-adjust Or did I miss something...? Paul > +} > + > /** > * \brief Split exposure into exposure time and gain > * \param[in] exposure Exposure value > + * \param[in] flickerPeriod The period of a flickering light source > * > * This function divides a given exposure into exposure time, analogue and > * digital gain by iterating through stages of exposure time and gain limits. > @@ -147,10 +170,15 @@ double ExposureModeHelper::clampGain(double gain) const > * required exposure, the helper falls-back to simply maximising the exposure > * time first, followed by analogue gain, followed by digital gain. > * > + * Once the exposure time has been determined from the modes, an adjustment is > + * made to compensate for a flickering light source by fixing the exposure time > + * to an exact multiple of the flicker period. Any effective exposure value that > + * is lost is added back via analogue and digital gain. > + * > * \return Tuple of exposure time, analogue gain, and digital gain > */ > std::tuple<utils::Duration, double, double> > -ExposureModeHelper::splitExposure(utils::Duration exposure) const > +ExposureModeHelper::splitExposure(utils::Duration exposure, std::optional<utils::Duration> flickerPeriod) const > { > ASSERT(maxExposureTime_); > ASSERT(maxGain_); > @@ -183,14 +211,14 @@ ExposureModeHelper::splitExposure(utils::Duration exposure) const > */ > > if (stageExposureTime * lastStageGain >= exposure) { > - exposureTime = clampExposureTime(exposure / clampGain(lastStageGain)); > + exposureTime = calculateExposureTime(exposure, clampGain(lastStageGain), flickerPeriod); > gain = clampGain(exposure / exposureTime); > > return { exposureTime, gain, exposure / (exposureTime * gain) }; > } > > if (stageExposureTime * stageGain >= exposure) { > - exposureTime = clampExposureTime(exposure / clampGain(stageGain)); > + exposureTime = calculateExposureTime(exposure, clampGain(stageGain), flickerPeriod); > gain = clampGain(exposure / exposureTime); > > return { exposureTime, gain, exposure / (exposureTime * gain) }; > @@ -204,7 +232,8 @@ ExposureModeHelper::splitExposure(utils::Duration exposure) const > * stages to use then the default stageGain of 1.0 is used so that > * exposure time is maxed before gain is touched at all. > */ > - exposureTime = clampExposureTime(exposure / clampGain(stageGain)); > + exposureTime = calculateExposureTime(exposure, clampGain(stageGain), flickerPeriod); > + > gain = clampGain(exposure / exposureTime); > > return { exposureTime, gain, exposure / (exposureTime * gain) }; > diff --git a/src/ipa/libipa/exposure_mode_helper.h b/src/ipa/libipa/exposure_mode_helper.h > index c5be1b67..a1d8c6bf 100644 > --- a/src/ipa/libipa/exposure_mode_helper.h > +++ b/src/ipa/libipa/exposure_mode_helper.h > @@ -7,6 +7,7 @@ > > #pragma once > > +#include <optional> > #include <tuple> > #include <utility> > #include <vector> > @@ -28,7 +29,7 @@ public: > double minGain, double maxGain); > > std::tuple<utils::Duration, double, double> > - splitExposure(utils::Duration exposure) const; > + splitExposure(utils::Duration exposure, std::optional<utils::Duration> flickerPeriod) const; > > utils::Duration minExposureTime() const { return minExposureTime_; } > utils::Duration maxExposureTime() const { return maxExposureTime_; } > @@ -38,6 +39,9 @@ public: > private: > utils::Duration clampExposureTime(utils::Duration exposureTime) const; > double clampGain(double gain) const; > + utils::Duration > + calculateExposureTime(utils::Duration exposureTime, double stageGain, > + std::optional<utils::Duration> flickerPeriod) const; > > std::vector<utils::Duration> exposureTimes_; > std::vector<double> gains_; > -- > 2.30.2 >
Hi Dan, Thank you for the patch. On Thu, Jan 23, 2025 at 02:07:26PM +0000, Daniel Scally wrote: > Update the ExposureModeHelper class to compensate for flickering > light sources in the ExposureModeHelper::splitExposure() function. > The adjustment simply caps exposure time at a multiple of the given > flicker period and compensates for any loss in the effective exposure > value by increasing analogue and then digital gain. > > Initially in the one call-site for this function, a std::nullopt is > passed to make this a no-op. > > Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com> > --- > Changes in v2: > > - Added a function to perform all the exposure time functionality in a > single place so we're not repeating ourselves > - Called that function in all the return sites rather than just one so > the flicker mitigation takes effect using exposure from the stages > list > - Switched the flickerPeriod input to a std::optional > - Clamped the calculated exposure time to guarantee it can't go beneath > the configured minima > > src/ipa/libipa/agc_mean_luminance.cpp | 3 +- > src/ipa/libipa/exposure_mode_helper.cpp | 37 ++++++++++++++++++++++--- > src/ipa/libipa/exposure_mode_helper.h | 6 +++- > 3 files changed, 40 insertions(+), 6 deletions(-) > > diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp > index 02555a44..273ec4e5 100644 > --- a/src/ipa/libipa/agc_mean_luminance.cpp > +++ b/src/ipa/libipa/agc_mean_luminance.cpp > @@ -8,6 +8,7 @@ > #include "agc_mean_luminance.h" > > #include <cmath> > +#include <optional> > > #include <libcamera/base/log.h> > #include <libcamera/control_ids.h> > @@ -560,7 +561,7 @@ AgcMeanLuminance::calculateNewEv(uint32_t constraintModeIndex, > newExposureValue = filterExposure(newExposureValue); > > frameCount_++; > - return exposureModeHelper->splitExposure(newExposureValue); > + return exposureModeHelper->splitExposure(newExposureValue, std::nullopt); > } > > /** > diff --git a/src/ipa/libipa/exposure_mode_helper.cpp b/src/ipa/libipa/exposure_mode_helper.cpp > index f235316d..4e1ba943 100644 > --- a/src/ipa/libipa/exposure_mode_helper.cpp > +++ b/src/ipa/libipa/exposure_mode_helper.cpp > @@ -7,6 +7,7 @@ > #include "exposure_mode_helper.h" > > #include <algorithm> > +#include <optional> > > #include <libcamera/base/log.h> > > @@ -118,9 +119,31 @@ double ExposureModeHelper::clampGain(double gain) const > return std::clamp(gain, minGain_, maxGain_); > } > > +utils::Duration > +ExposureModeHelper::calculateExposureTime(utils::Duration exposure, double stageGain, > + std::optional<utils::Duration> flickerPeriod) const > +{ > + utils::Duration exposureTime; > + Let's assume that the [minExposureTime_, maxExposureTime_] interval contains exposure/stageGain and exposure/stageGain rounded up to the next multiple of the flicker period, but not rounded down. > + exposureTime = clampExposureTime(exposure / stageGain); exposureTime will be exactly exposure/stageGain. > + > + /* > + * If we haven't been given a flicker period to adjust for or if it's > + * longer than the exposure time that we need to set then there's not > + * much we can do to compensate. > + */ > + if (!flickerPeriod.has_value() || flickerPeriod.value() >= exposureTime) > + return exposureTime; > + > + unsigned int flickerPeriods = exposureTime / flickerPeriod.value(); > + > + return clampExposureTime(flickerPeriods * flickerPeriod.value()); This will round exposureTime down to a multiple of flickerPeriod and clamp it. The returned value will be minExposureTime_, which is not a multiple of flickerPeriod, while the [min, max] interval contains a valid multiple of flickerPeriod. The preconditions and postconditions of this function should be documented. > +} > + > /** > * \brief Split exposure into exposure time and gain > * \param[in] exposure Exposure value > + * \param[in] flickerPeriod The period of a flickering light source > * > * This function divides a given exposure into exposure time, analogue and > * digital gain by iterating through stages of exposure time and gain limits. > @@ -147,10 +170,15 @@ double ExposureModeHelper::clampGain(double gain) const > * required exposure, the helper falls-back to simply maximising the exposure > * time first, followed by analogue gain, followed by digital gain. > * > + * Once the exposure time has been determined from the modes, an adjustment is > + * made to compensate for a flickering light source by fixing the exposure time > + * to an exact multiple of the flicker period. Any effective exposure value that > + * is lost is added back via analogue and digital gain. > + * > * \return Tuple of exposure time, analogue gain, and digital gain > */ > std::tuple<utils::Duration, double, double> > -ExposureModeHelper::splitExposure(utils::Duration exposure) const > +ExposureModeHelper::splitExposure(utils::Duration exposure, std::optional<utils::Duration> flickerPeriod) const Let's keep lines below 100 characters at least. Same below. > { > ASSERT(maxExposureTime_); > ASSERT(maxGain_); > @@ -183,14 +211,14 @@ ExposureModeHelper::splitExposure(utils::Duration exposure) const > */ > > if (stageExposureTime * lastStageGain >= exposure) { > - exposureTime = clampExposureTime(exposure / clampGain(lastStageGain)); > + exposureTime = calculateExposureTime(exposure, clampGain(lastStageGain), flickerPeriod); You always pass clampGain(...) to the calculateExposureTime() function. Should the clampGain() call be moved there ? I suppose it depends on how calculateExposureTime() is defined. Even if it's a private function, I think documenting it would help understand the code flow. > gain = clampGain(exposure / exposureTime); > > return { exposureTime, gain, exposure / (exposureTime * gain) }; > } > > if (stageExposureTime * stageGain >= exposure) { > - exposureTime = clampExposureTime(exposure / clampGain(stageGain)); > + exposureTime = calculateExposureTime(exposure, clampGain(stageGain), flickerPeriod); > gain = clampGain(exposure / exposureTime); > > return { exposureTime, gain, exposure / (exposureTime * gain) }; I have a bit of trouble following the impact of rounding the exposure at every stage. It may be fine, but I think an explanation in the commit message of why it's fine will help. > @@ -204,7 +232,8 @@ ExposureModeHelper::splitExposure(utils::Duration exposure) const > * stages to use then the default stageGain of 1.0 is used so that > * exposure time is maxed before gain is touched at all. > */ > - exposureTime = clampExposureTime(exposure / clampGain(stageGain)); > + exposureTime = calculateExposureTime(exposure, clampGain(stageGain), flickerPeriod); > + The code here tries to maximize the exposure time. Can it also suffer from calculateExposureTime() rounding down ? > gain = clampGain(exposure / exposureTime); > > return { exposureTime, gain, exposure / (exposureTime * gain) }; > diff --git a/src/ipa/libipa/exposure_mode_helper.h b/src/ipa/libipa/exposure_mode_helper.h > index c5be1b67..a1d8c6bf 100644 > --- a/src/ipa/libipa/exposure_mode_helper.h > +++ b/src/ipa/libipa/exposure_mode_helper.h > @@ -7,6 +7,7 @@ > > #pragma once > > +#include <optional> > #include <tuple> > #include <utility> > #include <vector> > @@ -28,7 +29,7 @@ public: > double minGain, double maxGain); > > std::tuple<utils::Duration, double, double> > - splitExposure(utils::Duration exposure) const; > + splitExposure(utils::Duration exposure, std::optional<utils::Duration> flickerPeriod) const; > > utils::Duration minExposureTime() const { return minExposureTime_; } > utils::Duration maxExposureTime() const { return maxExposureTime_; } > @@ -38,6 +39,9 @@ public: > private: > utils::Duration clampExposureTime(utils::Duration exposureTime) const; > double clampGain(double gain) const; > + utils::Duration > + calculateExposureTime(utils::Duration exposureTime, double stageGain, > + std::optional<utils::Duration> flickerPeriod) const; > > std::vector<utils::Duration> exposureTimes_; > std::vector<double> gains_;
Hi Paul, thanks for the very in-depth review On 24/01/2025 22:51, Paul Elder wrote: > On Thu, Jan 23, 2025 at 02:07:26PM +0000, Daniel Scally wrote: >> Update the ExposureModeHelper class to compensate for flickering >> light sources in the ExposureModeHelper::splitExposure() function. >> The adjustment simply caps exposure time at a multiple of the given >> flicker period and compensates for any loss in the effective exposure >> value by increasing analogue and then digital gain. >> >> Initially in the one call-site for this function, a std::nullopt is >> passed to make this a no-op. >> >> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com> >> --- >> Changes in v2: >> >> - Added a function to perform all the exposure time functionality in a >> single place so we're not repeating ourselves >> - Called that function in all the return sites rather than just one so >> the flicker mitigation takes effect using exposure from the stages >> list >> - Switched the flickerPeriod input to a std::optional >> - Clamped the calculated exposure time to guarantee it can't go beneath >> the configured minima >> >> src/ipa/libipa/agc_mean_luminance.cpp | 3 +- >> src/ipa/libipa/exposure_mode_helper.cpp | 37 ++++++++++++++++++++++--- >> src/ipa/libipa/exposure_mode_helper.h | 6 +++- >> 3 files changed, 40 insertions(+), 6 deletions(-) >> >> diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp >> index 02555a44..273ec4e5 100644 >> --- a/src/ipa/libipa/agc_mean_luminance.cpp >> +++ b/src/ipa/libipa/agc_mean_luminance.cpp >> @@ -8,6 +8,7 @@ >> #include "agc_mean_luminance.h" >> >> #include <cmath> >> +#include <optional> >> >> #include <libcamera/base/log.h> >> #include <libcamera/control_ids.h> >> @@ -560,7 +561,7 @@ AgcMeanLuminance::calculateNewEv(uint32_t constraintModeIndex, >> newExposureValue = filterExposure(newExposureValue); >> >> frameCount_++; >> - return exposureModeHelper->splitExposure(newExposureValue); >> + return exposureModeHelper->splitExposure(newExposureValue, std::nullopt); >> } >> >> /** >> diff --git a/src/ipa/libipa/exposure_mode_helper.cpp b/src/ipa/libipa/exposure_mode_helper.cpp >> index f235316d..4e1ba943 100644 >> --- a/src/ipa/libipa/exposure_mode_helper.cpp >> +++ b/src/ipa/libipa/exposure_mode_helper.cpp >> @@ -7,6 +7,7 @@ >> #include "exposure_mode_helper.h" >> >> #include <algorithm> >> +#include <optional> >> >> #include <libcamera/base/log.h> >> >> @@ -118,9 +119,31 @@ double ExposureModeHelper::clampGain(double gain) const >> return std::clamp(gain, minGain_, maxGain_); >> } >> >> +utils::Duration >> +ExposureModeHelper::calculateExposureTime(utils::Duration exposure, double stageGain, >> + std::optional<utils::Duration> flickerPeriod) const >> +{ >> + utils::Duration exposureTime; >> + >> + exposureTime = clampExposureTime(exposure / stageGain); >> + >> + /* >> + * If we haven't been given a flicker period to adjust for or if it's >> + * longer than the exposure time that we need to set then there's not >> + * much we can do to compensate. >> + */ >> + if (!flickerPeriod.has_value() || flickerPeriod.value() >= exposureTime) >> + return exposureTime; >> + >> + unsigned int flickerPeriods = exposureTime / flickerPeriod.value(); >> + >> + return clampExposureTime(flickerPeriods * flickerPeriod.value()); > Something doesn't seem right to me...? > > Premise: > - exposure * stageGain gives us sufficient total effective exposure > - flickerPeriod.has_value() and flickerPeriod < exposureTime > > Scenario: > - flickerPeriods * flickerPeriod < minExposureTime_ > - Which is possible afaiu if flickerPeriod < exposureTime = minExposureTime_ > - Thus we would end up with exposureTime = minExposureTime_ at a value > that is not a multiple of flickerPeriod > > For example: > - Let: > - exposureTime = minExposureTime_ > - flickerPeriod = minExposureTime_ * 0.8 > - Then: > - double dFlickerPeriods = exposureTime / flickerPeriod > = minExposureTime_ / (minExposureTime_ * 0.8) > = 1.25 > - unsigned int flickerPeriods = (unsigned int)dFlickerPeriods > = 1 > - Thus: > - ret = flickerPeriods * flickerPeriod > = 1 * minExposureTime_ * 0.8 > - Since ret < minExposureTime_, it will get clamped to > minExposureTime_, and is not a multiple of flickerPeriod > > Result: > - The exposure time doesn't adjust for flicker, since it's not a > multiple of flickerPeriod > > Expected result: > - We go up one more stage in the splitExposure() loop and run > calculateExposureTime() on that next stage > - exposureTime would be high enough that we have leeway to lower > exposureTime to flicker-adjust > > > Or did I miss something...? This made my brain hurt, but no I don't think so. I think I've accounted for this by moving the clamping to a multiple of the flicker period inside clampExposureTime() itself, which means that when the loop evaluates whether stageExposureTime multiplied by either lastStageGain or stageGain is sufficient to meet the requirements, it's doing so based on an exposure time that's already fixed to a multiple of flickerPeriod (if possible). > > > Paul > >> +} >> + >> /** >> * \brief Split exposure into exposure time and gain >> * \param[in] exposure Exposure value >> + * \param[in] flickerPeriod The period of a flickering light source >> * >> * This function divides a given exposure into exposure time, analogue and >> * digital gain by iterating through stages of exposure time and gain limits. >> @@ -147,10 +170,15 @@ double ExposureModeHelper::clampGain(double gain) const >> * required exposure, the helper falls-back to simply maximising the exposure >> * time first, followed by analogue gain, followed by digital gain. >> * >> + * Once the exposure time has been determined from the modes, an adjustment is >> + * made to compensate for a flickering light source by fixing the exposure time >> + * to an exact multiple of the flicker period. Any effective exposure value that >> + * is lost is added back via analogue and digital gain. >> + * >> * \return Tuple of exposure time, analogue gain, and digital gain >> */ >> std::tuple<utils::Duration, double, double> >> -ExposureModeHelper::splitExposure(utils::Duration exposure) const >> +ExposureModeHelper::splitExposure(utils::Duration exposure, std::optional<utils::Duration> flickerPeriod) const >> { >> ASSERT(maxExposureTime_); >> ASSERT(maxGain_); >> @@ -183,14 +211,14 @@ ExposureModeHelper::splitExposure(utils::Duration exposure) const >> */ >> >> if (stageExposureTime * lastStageGain >= exposure) { >> - exposureTime = clampExposureTime(exposure / clampGain(lastStageGain)); >> + exposureTime = calculateExposureTime(exposure, clampGain(lastStageGain), flickerPeriod); >> gain = clampGain(exposure / exposureTime); >> >> return { exposureTime, gain, exposure / (exposureTime * gain) }; >> } >> >> if (stageExposureTime * stageGain >= exposure) { >> - exposureTime = clampExposureTime(exposure / clampGain(stageGain)); >> + exposureTime = calculateExposureTime(exposure, clampGain(stageGain), flickerPeriod); >> gain = clampGain(exposure / exposureTime); >> >> return { exposureTime, gain, exposure / (exposureTime * gain) }; >> @@ -204,7 +232,8 @@ ExposureModeHelper::splitExposure(utils::Duration exposure) const >> * stages to use then the default stageGain of 1.0 is used so that >> * exposure time is maxed before gain is touched at all. >> */ >> - exposureTime = clampExposureTime(exposure / clampGain(stageGain)); >> + exposureTime = calculateExposureTime(exposure, clampGain(stageGain), flickerPeriod); >> + >> gain = clampGain(exposure / exposureTime); >> >> return { exposureTime, gain, exposure / (exposureTime * gain) }; >> diff --git a/src/ipa/libipa/exposure_mode_helper.h b/src/ipa/libipa/exposure_mode_helper.h >> index c5be1b67..a1d8c6bf 100644 >> --- a/src/ipa/libipa/exposure_mode_helper.h >> +++ b/src/ipa/libipa/exposure_mode_helper.h >> @@ -7,6 +7,7 @@ >> >> #pragma once >> >> +#include <optional> >> #include <tuple> >> #include <utility> >> #include <vector> >> @@ -28,7 +29,7 @@ public: >> double minGain, double maxGain); >> >> std::tuple<utils::Duration, double, double> >> - splitExposure(utils::Duration exposure) const; >> + splitExposure(utils::Duration exposure, std::optional<utils::Duration> flickerPeriod) const; >> >> utils::Duration minExposureTime() const { return minExposureTime_; } >> utils::Duration maxExposureTime() const { return maxExposureTime_; } >> @@ -38,6 +39,9 @@ public: >> private: >> utils::Duration clampExposureTime(utils::Duration exposureTime) const; >> double clampGain(double gain) const; >> + utils::Duration >> + calculateExposureTime(utils::Duration exposureTime, double stageGain, >> + std::optional<utils::Duration> flickerPeriod) const; >> >> std::vector<utils::Duration> exposureTimes_; >> std::vector<double> gains_; >> -- >> 2.30.2 >>
diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp index 02555a44..273ec4e5 100644 --- a/src/ipa/libipa/agc_mean_luminance.cpp +++ b/src/ipa/libipa/agc_mean_luminance.cpp @@ -8,6 +8,7 @@ #include "agc_mean_luminance.h" #include <cmath> +#include <optional> #include <libcamera/base/log.h> #include <libcamera/control_ids.h> @@ -560,7 +561,7 @@ AgcMeanLuminance::calculateNewEv(uint32_t constraintModeIndex, newExposureValue = filterExposure(newExposureValue); frameCount_++; - return exposureModeHelper->splitExposure(newExposureValue); + return exposureModeHelper->splitExposure(newExposureValue, std::nullopt); } /** diff --git a/src/ipa/libipa/exposure_mode_helper.cpp b/src/ipa/libipa/exposure_mode_helper.cpp index f235316d..4e1ba943 100644 --- a/src/ipa/libipa/exposure_mode_helper.cpp +++ b/src/ipa/libipa/exposure_mode_helper.cpp @@ -7,6 +7,7 @@ #include "exposure_mode_helper.h" #include <algorithm> +#include <optional> #include <libcamera/base/log.h> @@ -118,9 +119,31 @@ double ExposureModeHelper::clampGain(double gain) const return std::clamp(gain, minGain_, maxGain_); } +utils::Duration +ExposureModeHelper::calculateExposureTime(utils::Duration exposure, double stageGain, + std::optional<utils::Duration> flickerPeriod) const +{ + utils::Duration exposureTime; + + exposureTime = clampExposureTime(exposure / stageGain); + + /* + * If we haven't been given a flicker period to adjust for or if it's + * longer than the exposure time that we need to set then there's not + * much we can do to compensate. + */ + if (!flickerPeriod.has_value() || flickerPeriod.value() >= exposureTime) + return exposureTime; + + unsigned int flickerPeriods = exposureTime / flickerPeriod.value(); + + return clampExposureTime(flickerPeriods * flickerPeriod.value()); +} + /** * \brief Split exposure into exposure time and gain * \param[in] exposure Exposure value + * \param[in] flickerPeriod The period of a flickering light source * * This function divides a given exposure into exposure time, analogue and * digital gain by iterating through stages of exposure time and gain limits. @@ -147,10 +170,15 @@ double ExposureModeHelper::clampGain(double gain) const * required exposure, the helper falls-back to simply maximising the exposure * time first, followed by analogue gain, followed by digital gain. * + * Once the exposure time has been determined from the modes, an adjustment is + * made to compensate for a flickering light source by fixing the exposure time + * to an exact multiple of the flicker period. Any effective exposure value that + * is lost is added back via analogue and digital gain. + * * \return Tuple of exposure time, analogue gain, and digital gain */ std::tuple<utils::Duration, double, double> -ExposureModeHelper::splitExposure(utils::Duration exposure) const +ExposureModeHelper::splitExposure(utils::Duration exposure, std::optional<utils::Duration> flickerPeriod) const { ASSERT(maxExposureTime_); ASSERT(maxGain_); @@ -183,14 +211,14 @@ ExposureModeHelper::splitExposure(utils::Duration exposure) const */ if (stageExposureTime * lastStageGain >= exposure) { - exposureTime = clampExposureTime(exposure / clampGain(lastStageGain)); + exposureTime = calculateExposureTime(exposure, clampGain(lastStageGain), flickerPeriod); gain = clampGain(exposure / exposureTime); return { exposureTime, gain, exposure / (exposureTime * gain) }; } if (stageExposureTime * stageGain >= exposure) { - exposureTime = clampExposureTime(exposure / clampGain(stageGain)); + exposureTime = calculateExposureTime(exposure, clampGain(stageGain), flickerPeriod); gain = clampGain(exposure / exposureTime); return { exposureTime, gain, exposure / (exposureTime * gain) }; @@ -204,7 +232,8 @@ ExposureModeHelper::splitExposure(utils::Duration exposure) const * stages to use then the default stageGain of 1.0 is used so that * exposure time is maxed before gain is touched at all. */ - exposureTime = clampExposureTime(exposure / clampGain(stageGain)); + exposureTime = calculateExposureTime(exposure, clampGain(stageGain), flickerPeriod); + gain = clampGain(exposure / exposureTime); return { exposureTime, gain, exposure / (exposureTime * gain) }; diff --git a/src/ipa/libipa/exposure_mode_helper.h b/src/ipa/libipa/exposure_mode_helper.h index c5be1b67..a1d8c6bf 100644 --- a/src/ipa/libipa/exposure_mode_helper.h +++ b/src/ipa/libipa/exposure_mode_helper.h @@ -7,6 +7,7 @@ #pragma once +#include <optional> #include <tuple> #include <utility> #include <vector> @@ -28,7 +29,7 @@ public: double minGain, double maxGain); std::tuple<utils::Duration, double, double> - splitExposure(utils::Duration exposure) const; + splitExposure(utils::Duration exposure, std::optional<utils::Duration> flickerPeriod) const; utils::Duration minExposureTime() const { return minExposureTime_; } utils::Duration maxExposureTime() const { return maxExposureTime_; } @@ -38,6 +39,9 @@ public: private: utils::Duration clampExposureTime(utils::Duration exposureTime) const; double clampGain(double gain) const; + utils::Duration + calculateExposureTime(utils::Duration exposureTime, double stageGain, + std::optional<utils::Duration> flickerPeriod) const; std::vector<utils::Duration> exposureTimes_; std::vector<double> gains_;
Update the ExposureModeHelper class to compensate for flickering light sources in the ExposureModeHelper::splitExposure() function. The adjustment simply caps exposure time at a multiple of the given flicker period and compensates for any loss in the effective exposure value by increasing analogue and then digital gain. Initially in the one call-site for this function, a std::nullopt is passed to make this a no-op. Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com> --- Changes in v2: - Added a function to perform all the exposure time functionality in a single place so we're not repeating ourselves - Called that function in all the return sites rather than just one so the flicker mitigation takes effect using exposure from the stages list - Switched the flickerPeriod input to a std::optional - Clamped the calculated exposure time to guarantee it can't go beneath the configured minima src/ipa/libipa/agc_mean_luminance.cpp | 3 +- src/ipa/libipa/exposure_mode_helper.cpp | 37 ++++++++++++++++++++++--- src/ipa/libipa/exposure_mode_helper.h | 6 +++- 3 files changed, 40 insertions(+), 6 deletions(-)