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

[Robert Mader]

Sure, unfortunately not before Wednesday. Made me a reminder.

On 24.03.25 15:20, Milan Zamazal wrote:
> Hi Robert,
>
> would you have a chance to test that this changed version also works?
> (I expect it does, just testing before merging.)
>
> Milan Zamazal <mzamazal at redhat.com> writes:
>
>> 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 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 {