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

[Milan Zamazal]

Kieran Bingham <kieran.bingham at ideasonboard.com> writes:

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

Thank you, Robert, for testing.

> Patch work only picks up on full tags.
>
> Tested-by: Robert Mader <robert.mader at collabora.com>

I believe the patch is all right now and could get in for 0.5?

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