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

[Milan Zamazal]

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 {