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

[Milan Zamazal]

Hi,

Robert Mader <robert.mader at collabora.com> writes:

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

Thank you for testing.

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

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

If it is desirable to put these two variables to IPAContext although
they are basically private to BlackLevel class and are not supposed to
be used anywhere else, I can post v2 with such a change.

> 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() ?

AFAICS the active state gets reset only in configure, so it should be
fine as it is.

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

Yes.

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

OK.

>>         if (definedLevel_.has_value())
>>                 context.configuration.black.level = definedLevel_;
>>         context.activeState.blc.level =
>> -- 
>> 2.48.1
>>