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

[Robert Mader]

FTR., I quickly tested whether 
https://patchwork.libcamera.org/patch/21703/ fixes the issue as well and 
apparently it does.

So maybe we should just land that instead.

On 18.03.25 12:39, Kieran Bingham wrote:
> Quoting Milan Zamazal (2025-03-17 11:26:58)
>> 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 reset the stored exposure and gain values in
>> BlackLevel::configure() to fix the problem.
>>
>> 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 | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/simple/algorithms/blc.cpp
>> index 1d7d370b..14cf31bf 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
>>    */
>> @@ -38,6 +38,9 @@ int BlackLevel::init([[maybe_unused]] IPAContext &context,
>>   int BlackLevel::configure(IPAContext &context,
>>                            [[maybe_unused]] const IPAConfigInfo &configInfo)
>>   {
>> +       exposure_ = 0;
>> +       gain_ = 0;
>> +
> I wonder if we should try to make sure algorithms do not store private
> state, and only use storage in the IPAContext so we can ensure we can
> zero/reset the context on configure for all algos?
>
> For now this seems reasonable to re-initialise.
>
> Just to check though - is this sufficient for stop/start cycles?
>
> If someone does:
>
>   camera->configure()
>   camera->start()
>    ....
>   camera->stop()
>   camera->start()
>     ... Will there be the same bug here? Or will it be ok ?
>
> Or should these be re-initialised at start() ?
>
> Perhaps it's ok - as the issue is that the black level was reset here in
> configure, so the flag that was also checking when to recalibrate also
> needs to be reset here.
>
> A comment to state that clearly might be helpful here somewhere that the
> exposure and gain are used to determine when to recalibrate the black
> levels ...
>
>
>>          if (definedLevel_.has_value())
>>                  context.configuration.black.level = definedLevel_;
>>          context.activeState.blc.level =
>> -- 
>> 2.48.1
>>
-- 
Robert Mader
Consultant Software Developer

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