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

[Robert Mader]

Yep, works fine here -> T-b

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