libcamera: software_isp: Reset stored exposure in black level
diff mbox series

Message ID 20250317112658.15084-1-mzamazal@redhat.com
State Superseded
Headers show
Series
  • libcamera: software_isp: Reset stored exposure in black level
Related show

Commit Message

Milan Zamazal March 17, 2025, 11:26 a.m. UTC
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@redhat.com>
---
 src/ipa/simple/algorithms/blc.cpp | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Comments

Robert Mader March 17, 2025, 11:40 a.m. UTC | #1
Thanks!

Tested-by: Robert Mader <robert.mader@collabora.com>

On 17.03.25 12:26, Milan Zamazal wrote:
> 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@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;
> +
>   	if (definedLevel_.has_value())
>   		context.configuration.black.level = definedLevel_;
>   	context.activeState.blc.level =
Kieran Bingham March 18, 2025, 11:39 a.m. UTC | #2
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@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
>
Milan Zamazal March 18, 2025, 1:41 p.m. UTC | #3
Hi,

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

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

Thank you for testing.

Kieran Bingham <kieran.bingham@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@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
>>
Robert Mader March 18, 2025, 1:52 p.m. UTC | #4
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@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
>>
Kieran Bingham March 18, 2025, 2:07 p.m. UTC | #5
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@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
>
Kieran Bingham March 18, 2025, 2:13 p.m. UTC | #6
Quoting Milan Zamazal (2025-03-18 13:41:31)
> Hi,
> 
> Robert Mader <robert.mader@collabora.com> writes:
> 
> > Thanks!
> >
> > Tested-by: Robert Mader <robert.mader@collabora.com>
> 
> Thank you for testing.
> 
> Kieran Bingham <kieran.bingham@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@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@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
> >>
>
Milan Zamazal March 19, 2025, 9:57 a.m. UTC | #7
Kieran Bingham <kieran.bingham@ideasonboard.com> writes:

> Quoting Milan Zamazal (2025-03-18 13:41:31)
>> Hi,
>> 
>
>> Robert Mader <robert.mader@collabora.com> writes:
>> 
>> > Thanks!
>> >
>> > Tested-by: Robert Mader <robert.mader@collabora.com>
>> 
>> Thank you for testing.
>> 
>> Kieran Bingham <kieran.bingham@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@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@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
>> >>
>>

Patch
diff mbox series

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;
+
 	if (definedLevel_.has_value())
 		context.configuration.black.level = definedLevel_;
 	context.activeState.blc.level =