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

[Milan Zamazal]

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

> Quoting Milan Zamazal (2025-03-18 13:41:31)
>> 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.
>
> As this is your IPA - that's mostly for you to decide - but my query is
> that I consider that the top level 
>
> IPASoftSimple::configure(const IPAConfigInfo &configInfo)
> {
> ...
>  	/* Clear the IPA context before the streaming session. */
> 	context_.configuration = {};
> 	context_.activeState = {};
>  	context_.frameContexts.clear();
> ...
> }
>
> being called before each algo->configure() is the 'right thing to do'
> ... but that misses clearing any private state in an algorithm.
>
> Resetting the whole structure is convenient and easy that's all...

I think clarity outweighs encapsulation here so I changed it in v2.
Posted, let's see whether it feels better.

> Knowing that there is no equivalent bug between start()->stop()->start()
> calls, I'm fine with this patch.
>
>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>
>
>> 
>> > 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.
>
> It's not clear 'here in configure()' that exposure_ and gain_ are tied to
> the black level. That's the only reason I think a comment would be
> beneficial ...
>
> Ultimately they are used to identify that the auto-black-level should
> recalibrate black level if the 'scene' is changed.
>
> Knowing that black level is in fact an intrinsically 'static' property
> of cameras, I do wonder if this should be run on every change in
> exposure - but I think the point is it's a fall back when we don't have
> a definition of what the real black level is - so it 'shouldn't' be used
> much in real terms.
>
> --
>
>
>
>> 
>> >>         if (definedLevel_.has_value())
>> >>                 context.configuration.black.level = definedLevel_;
>> >>         context.activeState.blc.level =
>> >> -- 
>> >> 2.48.1
>> >>
>>