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

[Robert Mader]

Oh, I wasn't aware that it automatically picks those tags up - thanks!

On 26.03.25 16:18, Kieran Bingham wrote:
> 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
>>
-- 
Robert Mader
Consultant Software Developer

Collabora Ltd.
Platinum Building, St John's Innovation Park, Cambridge CB4 0DS, UK
Registered in England & Wales, no. 5513718