[PATCH v2] libcamera: software_isp: Reset stored exposure in black level

[Kieran Bingham]

Quoting Robert Mader (2025-03-26 15:11:53)
> Yep, works fine here -> T-b

Patch work only picks up on full tags.

Tested-by: Robert Mader <robert.mader at collabora.com>

Thanks

> 
> On 24.03.25 15:22, Robert Mader wrote:
> > Sure, unfortunately not before Wednesday. Made me a reminder.
> >
> > On 24.03.25 15:20, Milan Zamazal wrote:
> >> Hi Robert,
> >>
> >> would you have a chance to test that this changed version also works?
> >> (I expect it does, just testing before merging.)
> >>
> >> Milan Zamazal <mzamazal at redhat.com> writes:
> >>
> >>> Automatic black level setting in software ISP updates the determined
> >>> black level value when exposure or gain change.  It stores the last
> >>> exposure and gain values to detect the change.
> >>>
> >>> BlackLevel::configure() resets the stored black level value but not the
> >>> exposure and gain values.  This can prevent updating the black value 
> >>> and
> >>> cause bad image output, e.g. after suspending and resuming a camera, if
> >>> exposure and gain remain unchanged.
> >>>
> >>> Let's store exposure and gain in IPAActiveState.  Although the values
> >>> are not supposed to be used outside BlackLevel class, storing them in
> >>> the context has the advantage of their automatic reset together with 
> >>> the
> >>> other context contents and having them in `blc' struct indicates their
> >>> relationship to the black value computation.
> >>>
> >>> Bug: https://bugs.libcamera.org/show_bug.cgi?id=259
> >>> Signed-off-by: Milan Zamazal <mzamazal at redhat.com>
> >>> ---
> >>>   src/ipa/simple/algorithms/blc.cpp | 10 +++++-----
> >>>   src/ipa/simple/algorithms/blc.h   |  2 --
> >>>   src/ipa/simple/ipa_context.h      |  2 ++
> >>>   3 files changed, 7 insertions(+), 7 deletions(-)
> >>>
> >>> diff --git a/src/ipa/simple/algorithms/blc.cpp 
> >>> b/src/ipa/simple/algorithms/blc.cpp
> >>> index 1d7d370b..089e492a 100644
> >>> --- a/src/ipa/simple/algorithms/blc.cpp
> >>> +++ b/src/ipa/simple/algorithms/blc.cpp
> >>> @@ -1,6 +1,6 @@
> >>>   /* SPDX-License-Identifier: LGPL-2.1-or-later */
> >>>   /*
> >>> - * Copyright (C) 2024, Red Hat Inc.
> >>> + * Copyright (C) 2024-2025, Red Hat Inc.
> >>>    *
> >>>    * Black level handling
> >>>    */
> >>> @@ -54,8 +54,8 @@ void BlackLevel::process(IPAContext &context,
> >>>       if (context.configuration.black.level.has_value())
> >>>           return;
> >>>   -    if (frameContext.sensor.exposure == exposure_ &&
> >>> -        frameContext.sensor.gain == gain_) {
> >>> +    if (frameContext.sensor.exposure == 
> >>> context.activeState.blc.lastExposure &&
> >>> +        frameContext.sensor.gain == 
> >>> context.activeState.blc.lastGain) {
> >>>           return;
> >>>       }
> >>>   @@ -79,8 +79,8 @@ void BlackLevel::process(IPAContext &context,
> >>>           seen += histogram[i];
> >>>           if (seen >= pixelThreshold) {
> >>>               context.activeState.blc.level = i * histogramRatio;
> >>> -            exposure_ = frameContext.sensor.exposure;
> >>> -            gain_ = frameContext.sensor.gain;
> >>> +            context.activeState.blc.lastExposure = 
> >>> frameContext.sensor.exposure;
> >>> +            context.activeState.blc.lastGain = 
> >>> frameContext.sensor.gain;
> >>>               LOG(IPASoftBL, Debug)
> >>>                   << "Auto-set black level: "
> >>>                   << i << "/" << SwIspStats::kYHistogramSize
> >>> diff --git a/src/ipa/simple/algorithms/blc.h 
> >>> b/src/ipa/simple/algorithms/blc.h
> >>> index 52d59cab..db9e6d63 100644
> >>> --- a/src/ipa/simple/algorithms/blc.h
> >>> +++ b/src/ipa/simple/algorithms/blc.h
> >>> @@ -30,8 +30,6 @@ public:
> >>>                ControlList &metadata) override;
> >>>     private:
> >>> -    int32_t exposure_;
> >>> -    double gain_;
> >>>       std::optional<uint8_t> definedLevel_;
> >>>   };
> >>>   diff --git a/src/ipa/simple/ipa_context.h 
> >>> b/src/ipa/simple/ipa_context.h
> >>> index 4af51306..b1198af5 100644
> >>> --- a/src/ipa/simple/ipa_context.h
> >>> +++ b/src/ipa/simple/ipa_context.h
> >>> @@ -33,6 +33,8 @@ struct IPASessionConfiguration {
> >>>   struct IPAActiveState {
> >>>       struct {
> >>>           uint8_t level;
> >>> +        int32_t lastExposure;
> >>> +        double lastGain;
> >>>       } blc;
> >>>         struct {
> 
> -- 
> Robert Mader
> Consultant Software Developer
> 
> Collabora Ltd.
> Platinum Building, St John's Innovation Park, Cambridge CB4 0DS, UK
> Registered in England & Wales, no. 5513718
>