Message ID | 20250317112658.15084-1-mzamazal@redhat.com |
---|---|
State | Superseded |
Headers | show |
Series |
|
Related | show |
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 =
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 >
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 >>
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 >>
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 >
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 > >> >
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 >> >> >>
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 =
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(-)