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

[Kieran Bingham]

Quoting Robert Mader (2025-03-18 13:52:02)
> 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.

I would actually think 'this' one is the right approach. I think active
state and the configuration should be reset before we recall configure
on each component, to ensure that each time we configure a camera it's
from a clean state, and that nothing is left dangling (err, apparently
except these algo-private data structures...).

Essentially, that's what I was querying with 'should the private
variables be part of those structures so they are also cleared at the
same time' ... but I agree this is just an internal detail to the blc
'algo here'.

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