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

[Laurent Pinchart]

On Fri, Mar 28, 2025 at 10:06:54PM +0100, Milan Zamazal wrote:
> Laurent Pinchart <laurent.pinchart at ideasonboard.com> writes:
> > On Wed, Mar 19, 2025 at 10:55:33AM +0100, 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.
> >
> > I'm a bit puzzled there. If the exposure time and gain don't change,
> > then the black level will stay at 0. However, the bug report mentions
> > very dark images. What am I missing ?
> 
> That the black level, if unspecified, is initially set to 255.  (The
> point is to get the lowest black level experienced, hence we start with
> the highest value.)

Aahhh of course.

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> >> Let's store exposure and gain in IPAActiveState.  Although the values
> >> are not supposed to be used outside BlackLevel class, storing them in
> >> the context has the advantage of their automatic reset together with the
> >> other context contents and having them in `blc' struct indicates their
> >> relationship to the black value computation.
> >> 
> >> 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 | 10 +++++-----
> >>  src/ipa/simple/algorithms/blc.h   |  2 --
> >>  src/ipa/simple/ipa_context.h      |  2 ++
> >>  3 files changed, 7 insertions(+), 7 deletions(-)
> >> 
> >> diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/simple/algorithms/blc.cpp
> >> index 1d7d370b..089e492a 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
> >>   */
> >> @@ -54,8 +54,8 @@ void BlackLevel::process(IPAContext &context,
> >>  	if (context.configuration.black.level.has_value())
> >>  		return;
> >>  
> >> -	if (frameContext.sensor.exposure == exposure_ &&
> >> -	    frameContext.sensor.gain == gain_) {
> >> +	if (frameContext.sensor.exposure == context.activeState.blc.lastExposure &&
> >> +	    frameContext.sensor.gain == context.activeState.blc.lastGain) {
> >>  		return;
> >>  	}
> >>  
> >> @@ -79,8 +79,8 @@ void BlackLevel::process(IPAContext &context,
> >>  		seen += histogram[i];
> >>  		if (seen >= pixelThreshold) {
> >>  			context.activeState.blc.level = i * histogramRatio;
> >> -			exposure_ = frameContext.sensor.exposure;
> >> -			gain_ = frameContext.sensor.gain;
> >> +			context.activeState.blc.lastExposure = frameContext.sensor.exposure;
> >> +			context.activeState.blc.lastGain = frameContext.sensor.gain;
> >>  			LOG(IPASoftBL, Debug)
> >>  				<< "Auto-set black level: "
> >>  				<< i << "/" << SwIspStats::kYHistogramSize
> >> diff --git a/src/ipa/simple/algorithms/blc.h b/src/ipa/simple/algorithms/blc.h
> >> index 52d59cab..db9e6d63 100644
> >> --- a/src/ipa/simple/algorithms/blc.h
> >> +++ b/src/ipa/simple/algorithms/blc.h
> >> @@ -30,8 +30,6 @@ public:
> >>  		     ControlList &metadata) override;
> >>  
> >>  private:
> >> -	int32_t exposure_;
> >> -	double gain_;
> >>  	std::optional<uint8_t> definedLevel_;
> >>  };
> >>  
> >> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h
> >> index 4af51306..b1198af5 100644
> >> --- a/src/ipa/simple/ipa_context.h
> >> +++ b/src/ipa/simple/ipa_context.h
> >> @@ -33,6 +33,8 @@ struct IPASessionConfiguration {
> >>  struct IPAActiveState {
> >>  	struct {
> >>  		uint8_t level;
> >> +		int32_t lastExposure;
> >> +		double lastGain;
> >>  	} blc;
> >>  
> >>  	struct {

-- 
Regards,

Laurent Pinchart