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

[Kieran Bingham]

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


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