[PATCH 2/3] ipa: libipa: Adjust for flicker in ExposureModeHelper
Paul Elder
paul.elder at ideasonboard.com
Thu Jan 23 05:31:29 CET 2025
On Wed, Jan 22, 2025 at 03:22:11PM +0200, Laurent Pinchart wrote:
> On Wed, Jan 22, 2025 at 01:15:43PM +0000, Daniel Scally wrote:
> > On 22/01/2025 13:09, Laurent Pinchart wrote:
> > > On Tue, Jan 21, 2025 at 09:29:01PM +0000, Daniel Scally wrote:
> > >> On 21/01/2025 17:17, Laurent Pinchart wrote:
> > >>> On Tue, Jan 21, 2025 at 04:30:19PM +0000, Daniel Scally wrote:
> > >>>> On 21/01/2025 16:27, Laurent Pinchart wrote:
> > >>>>> On Fri, Jan 17, 2025 at 02:34:09PM +0000, Daniel Scally wrote:
> > >>>>>> Update the ExposureModeHelper class to compensate for flickering
> > >>>>>> light sources in the ::splitExposure() function. The adjustment
> > >>>>>
> > >>>>> ExposureModeHelper::splitExposure() or just splitExposure()
> > >>>>
> > >>>> The former :)
> > >>>>
> > >>>>>> 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 flicker period
> > >>>>>> of 0us is passed, making this a no-op.
> > >>>>>>
> > >>>>>> Signed-off-by: Daniel Scally <dan.scally at ideasonboard.com>
> > >>>>>> ---
> > >>>>>> src/ipa/libipa/agc_mean_luminance.cpp | 2 +-
> > >>>>>> src/ipa/libipa/exposure_mode_helper.cpp | 20 +++++++++++++++++++-
> > >>>>>> src/ipa/libipa/exposure_mode_helper.h | 2 +-
> > >>>>>> 3 files changed, 21 insertions(+), 3 deletions(-)
> > >>>>>>
> > >>>>>> diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp
> > >>>>>> index 02555a44..b5e6afe3 100644
> > >>>>>> --- a/src/ipa/libipa/agc_mean_luminance.cpp
> > >>>>>> +++ b/src/ipa/libipa/agc_mean_luminance.cpp
> > >>>>>> @@ -560,7 +560,7 @@ AgcMeanLuminance::calculateNewEv(uint32_t constraintModeIndex,
> > >>>>>> newExposureValue = filterExposure(newExposureValue);
> > >>>>>>
> > >>>>>> frameCount_++;
> > >>>>>> - return exposureModeHelper->splitExposure(newExposureValue);
> > >>>>>> + return exposureModeHelper->splitExposure(newExposureValue, 0us);
> > >>>>>> }
> > >>>>>>
> > >>>>>> /**
> > >>>>>> diff --git a/src/ipa/libipa/exposure_mode_helper.cpp b/src/ipa/libipa/exposure_mode_helper.cpp
> > >>>>>> index f235316d..038aa33c 100644
> > >>>>>> --- a/src/ipa/libipa/exposure_mode_helper.cpp
> > >>>>>> +++ b/src/ipa/libipa/exposure_mode_helper.cpp
> > >>>>>> @@ -121,6 +121,7 @@ double ExposureModeHelper::clampGain(double gain) const
> > >>>>>> /**
> > >>>>>> * \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 +148,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, utils::Duration flickerPeriod) const
> > >>>>>
> > >>>>> We could avoid magic values by passing a std::optional<utils::Duration>.
> > >>>>> What do you think ?
> > >>>>
> > >>>> Good idea
> > >>>>
> > >>>>>> {
> > >>>>>> ASSERT(maxExposureTime_);
> > >>>>>> ASSERT(maxGain_);
> > >>>>>> @@ -205,6 +211,18 @@ ExposureModeHelper::splitExposure(utils::Duration exposure) const
> > >>>>>> * exposure time is maxed before gain is touched at all.
> > >>>>>> */
> > >>>>>> exposureTime = clampExposureTime(exposure / clampGain(stageGain));
> > >>>>>> +
> > >>>>>> + /*
> > >>>>>> + * Finally, if we have been given a flicker period we need to reduce the
> > >>>>>> + * exposure time to be a multiple of that period (if possible). The
> > >>>>>> + * effect of this should be to hide the flicker.
> > >>>>>> + */
> > >>>>>> + if (flickerPeriod > 0us && flickerPeriod < exposureTime) {
> > >>>>>> + unsigned int flickerPeriods = exposureTime / flickerPeriod;
> > >>>>>> + if (flickerPeriods)
> > >>>>>> + exposureTime = flickerPeriods * flickerPeriod;
> > >>>>>> + }
> > >>>>>
> > >>>>> This means that the exposure time could become lower than
> > >>>>> minExposureTime_. Is that an issue ?
> > >>>>
> > >>>> I doubt it given likely exposure times and flicker periods, but I can
> > >>>> extend it to guarantee that doesn't happen.
I would also be more comfortable with a clamp.
Also, do we not have to worry about cameras that have really high
framerate?
> > >>>>
> > >>>>> Should we define what controls take
> > >>>>> precedence in that case ? For instance, when manual exposure time is
> > >>>>> enabled, should the flicker period be ignored ?
> > >>>>
> > >>>> Yes, and is. This only takes effect in automatic exposure mode.
> > >>>
> > >>> Is it ? If exposure time is manual and gain is automatic, the above code
> > >>> still gets run as far as I can tell.
> > >>
> > >> This calculation still runs as is, but the rkisp1 and mali-c55 IPAs both take exposure time from a
> > >> different field in the IPA context if they are running in manual mode than the field this value is
> > >> set to, and the IPU3 IPA has no handling of controls for AGC at all at the moment.
> > >
> > > That doesn't seem right. It means the helpers will split exposure in
> > > exposure time and gain without knowing that one of the two will be
> > > overridden. It's not an issue with this patch, but I think it needs to
> > > be fixed.
> >
> > Sorry - I should have been clearer - analogue and digital gain are treated the same way. Either all
> > three sensor controls / ISP parameters are set from the manual values passed in through libcamera
> > controls, or all three values are set from the automatic values calculated here. I don't think there
> > would be a situation in which only one of the values calculated here was overridden.
>
> With the AEGC series that we merged recently, the exposure time mode can
> be set to manual and the gain mode to automatic.
>
> > > Paul, what do you think ?
Ah, I see the problem, I forgot to plumb those sub-controls into the
exposure mode helper.
I don't think this has to be addressed in this series though, and we can
do it on top (otherwise that fix and this series will start
conflicting).
Paul
> > >
> > >>> The interactions between the flicker and AE controls also need to be
> > >>> documented in the definition of the appropriate controls.
> > >>>
> > >>>>>> +
> > >>>>>> 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..a5fcc366 100644
> > >>>>>> --- a/src/ipa/libipa/exposure_mode_helper.h
> > >>>>>> +++ b/src/ipa/libipa/exposure_mode_helper.h
> > >>>>>> @@ -28,7 +28,7 @@ public:
> > >>>>>> double minGain, double maxGain);
> > >>>>>>
> > >>>>>> std::tuple<utils::Duration, double, double>
> > >>>>>> - splitExposure(utils::Duration exposure) const;
> > >>>>>> + splitExposure(utils::Duration exposure, utils::Duration flickerPeriod) const;
> > >>>>>>
> > >>>>>> utils::Duration minExposureTime() const { return minExposureTime_; }
> > >>>>>> utils::Duration maxExposureTime() const { return maxExposureTime_; }
>
> --
> Regards,
>
> Laurent Pinchart
More information about the libcamera-devel
mailing list