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

[Milan Zamazal]

Hi Laurent,

Laurent Pinchart <laurent.pinchart at ideasonboard.com> writes:

> Hi Milan,
>
> Thank you for the patch.
>
> 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.)

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