libcamera: software_isp: Apply black level compensation

Black may not be represented as 0 pixel value for given hardware, it may
be higher.  If this is not compensated then various problems may occur
such as low contrast or suboptimal exposure.

The black pixel value can be either retrieved from a tuning file for the
given hardware, or automatically on the fly.  The former is the right
and correct method, while the latter can be used when a tuning file is
not available for the given hardware.  Since there is currently no
support for tuning files in software ISP, the automatic, hardware
independent way, is always used.  Support for tuning files should be
added in future but it will require more work than this patch.

The patch looks at the image histogram and assumes that black starts
when pixel values start occurring on the left.  A certain amount of the
darkest pixels is ignored; it doesn't matter whether they represent
various kinds of noise or are real, they are better to omit in any case
to make the image looking better.  It also doesn't matter whether the
darkest pixels occur around the supposed black level or are spread
between 0 and the black level, the difference is not important.

An arbitrary threshold of 2% darkest pixels is applied; there is no
magic about that value.

The patch assumes that the black values for different colors are the
same and doesn't attempt any other non-primitive enhancements.  It
cannot completely replace tuning files and simplicity, while providing
visible benefit, is its goal.  Anything more sophisticated is left for
future patches.

A possible cheap enhancement, if needed, could be setting exposure +
gain to minimum values temporarily, before setting the black level.  In
theory, the black level should be fixed but it may not be reached in all
images.  For this reason, the patch updates black level only if the
observed value is lower than the current one; it should be never
increased.

The purpose of the patch is to compensate for hardware properties.
General image contrast enhancements are out of scope of this patch.

Stats are still gathered as an uncorrected histogram, to avoid any
confusion and to represent the raw image data.  Exposure must be
determined after the black level correction -- it has no influence on
the sub-black area and must be correct after applying the black level
correction.  The granularity of the histogram is increased from 16 to 64
to provide a better precision (there is no theory behind either of those
numbers).

Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

10 files changed, 173 insertions, 18 deletions

diff --git a/include/libcamera/internal/software_isp/debayer_params.h b/include/libcamera/internal/software_isp/debayer_params.h
index c818ca3a..32cd448a 100644
--- a/include/libcamera/internal/software_isp/debayer_params.h
+++ b/include/libcamera/internal/software_isp/debayer_params.h
@@ -20,6 +20,10 @@ struct DebayerParams {
 	unsigned int gainB;
 
 	float gamma;
+	/**
+	 * \brief Level of the black point, 0..255, 0 is no correction.
+	 */
+	unsigned int blackLevel;
 };
 
 } /* namespace libcamera */
diff --git a/include/libcamera/internal/software_isp/swisp_stats.h b/include/libcamera/internal/software_isp/swisp_stats.h
index dd5e207d..4ca8d647 100644
--- a/include/libcamera/internal/software_isp/swisp_stats.h
+++ b/include/libcamera/internal/software_isp/swisp_stats.h
@@ -35,11 +35,15 @@ struct SwIspStats {
 	/**
 	 * \brief Number of bins in the yHistogram
 	 */
-	static constexpr unsigned int kYHistogramSize = 16;
+	static constexpr unsigned int kYHistogramSize = 64;
+	/**
+	 * \brief Type of the histogram.
+	 */
+	using Histogram = std::array<uint32_t, kYHistogramSize>;
 	/**
 	 * \brief A histogram of luminance values
 	 */
-	std::array<uint32_t, kYHistogramSize> yHistogram;
+	Histogram yHistogram;
 };
 
 } /* namespace libcamera */
diff --git a/src/ipa/simple/black_level.cpp b/src/ipa/simple/black_level.cpp
new file mode 100644
index 00000000..c7e8d8b7
--- /dev/null
+++ b/src/ipa/simple/black_level.cpp
@@ -0,0 +1,88 @@
+/* SPDX-License-Identifier: LGPL-2.1-or-later */
+/*
+ * Copyright (C) 2024, Red Hat Inc.
+ *
+ * black_level.cpp - black level handling
+ */
+
+#include "black_level.h"
+
+#include <numeric>
+
+#include <libcamera/base/log.h>
+
+namespace libcamera {
+
+LOG_DEFINE_CATEGORY(IPASoftBL)
+
+/**
+ * \class BlackLevel
+ * \brief Object providing black point level for software ISP
+ *
+ * Black level can be provided in hardware tuning files or, if no tuning file is
+ * available for the given hardware, guessed automatically, with less accuracy.
+ * As tuning files are not yet implemented for software ISP, BlackLevel
+ * currently provides only guessed black levels.
+ *
+ * This class serves for tracking black level as a property of the underlying
+ * hardware, not as means of enhancing a particular scene or image.
+ *
+ * The class is supposed to be instantiated for the given camera stream.
+ * The black level can be retrieved using BlackLevel::get() method. It is
+ * initially 0 and may change when updated using BlackLevel::update() method.
+ */
+
+BlackLevel::BlackLevel()
+	: blackLevel_(255), blackLevelSet_(false)
+{
+}
+
+/**
+ * \brief Return the current black level
+ *
+ * \return The black level, in the range from 0 (minimum) to 255 (maximum).
+ * If the black level couldn't be determined yet, return 0.
+ */
+unsigned int BlackLevel::get() const
+{
+	return blackLevelSet_ ? blackLevel_ : 0;
+}
+
+/**
+ * \brief Update black level from the provided histogram
+ * \param[in] yHistogram The histogram to be used for updating black level
+ *
+ * The black level is property of the given hardware, not image. It is updated
+ * only if it has not been yet set or if it is lower than the lowest value seen
+ * so far.
+ */
+void BlackLevel::update(SwIspStats::Histogram &yHistogram)
+{
+	/*
+	 * The constant is selected to be "good enough", not overly conservative or
+	 * aggressive. There is no magic about the given value.
+	 */
+	constexpr float ignoredPercentage_ = 0.02;
+	const unsigned int total =
+		std::accumulate(begin(yHistogram), end(yHistogram), 0);
+	const unsigned int pixelThreshold = ignoredPercentage_ * total;
+	const unsigned int histogramRatio = 256 / SwIspStats::kYHistogramSize;
+	const unsigned int currentBlackIdx = blackLevel_ / histogramRatio;
+
+	for (unsigned int i = 0, seen = 0;
+	     i < currentBlackIdx && i < SwIspStats::kYHistogramSize;
+	     i++) {
+		seen += yHistogram[i];
+		if (seen >= pixelThreshold) {
+			blackLevel_ = i * histogramRatio;
+			blackLevelSet_ = true;
+			LOG(IPASoftBL, Debug)
+				<< "Auto-set black level: "
+				<< i << "/" << SwIspStats::kYHistogramSize
+				<< " (" << 100 * (seen - yHistogram[i]) / total << "% below, "
+				<< 100 * seen / total << "% at or below)";
+			break;
+		}
+	};
+}
+} /* namespace libcamera */
diff --git a/src/ipa/simple/black_level.h b/src/ipa/simple/black_level.h
new file mode 100644
index 00000000..7e37757e
--- /dev/null
+++ b/src/ipa/simple/black_level.h
@@ -0,0 +1,28 @@
+/* SPDX-License-Identifier: LGPL-2.1-or-later */
+/*
+ * Copyright (C) 2024, Red Hat Inc.
+ *
+ * black_level.h - black level handling
+ */
+
+#pragma once
+
+#include <array>
+
+#include "libcamera/internal/software_isp/swisp_stats.h"
+
+namespace libcamera {
+
+class BlackLevel
+{
+public:
+	BlackLevel();
+	unsigned int get() const;
+	void update(SwIspStats::Histogram &yHistogram);
+
+private:
+	unsigned int blackLevel_;
+	bool blackLevelSet_;
+};
+
+} /* namespace libcamera */
diff --git a/src/ipa/simple/meson.build b/src/ipa/simple/meson.build
index 3e863db7..44b5f1d7 100644
--- a/src/ipa/simple/meson.build
+++ b/src/ipa/simple/meson.build
@@ -2,8 +2,13 @@
 
 ipa_name = 'ipa_soft_simple'
 
+soft_simple_sources = files([
+    'soft_simple.cpp',
+    'black_level.cpp',
+])
+
 mod = shared_module(ipa_name,
-                    ['soft_simple.cpp', libcamera_generated_ipa_headers],
+                    [soft_simple_sources, libcamera_generated_ipa_headers],
                     name_prefix : '',
                     include_directories : [ipa_includes, libipa_includes],
                     dependencies : libcamera_private,
diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp
index ff1d8e0c..b9fb58b5 100644
--- a/src/ipa/simple/soft_simple.cpp
+++ b/src/ipa/simple/soft_simple.cpp
@@ -26,8 +26,9 @@
 
 #include "libipa/camera_sensor_helper.h"
 
-namespace libcamera {
+#include "black_level.h"
 
+namespace libcamera {
 LOG_DEFINE_CATEGORY(IPASoft)
 
 namespace ipa::soft {
@@ -54,7 +55,8 @@ class IPASoftSimple : public ipa::soft::IPASoftInterface
 {
 public:
 	IPASoftSimple()
-		: params_(nullptr), stats_(nullptr), ignoreUpdates_(0)
+		: params_(nullptr), stats_(nullptr), blackLevel_(BlackLevel()),
+		  ignoreUpdates_(0)
 	{
 	}
 
@@ -78,6 +80,7 @@ private:
 	SwIspStats *stats_;
 	std::unique_ptr<CameraSensorHelper> camHelper_;
 	ControlInfoMap sensorInfoMap_;
+	BlackLevel blackLevel_;
 
 	int32_t exposureMin_, exposureMax_;
 	int32_t exposure_;
@@ -255,6 +258,10 @@ void IPASoftSimple::processStats(const ControlList &sensorControls)
 	params_->gainG = 256;
 	params_->gamma = 0.5;
 
+	if (ignoreUpdates_ > 0)
+		blackLevel_.update(stats_->yHistogram);
+	params_->blackLevel = blackLevel_.get();
+
 	setIspParams.emit();
 
 	/* \todo Switch to the libipa/algorithm.h API someday. */
@@ -273,18 +280,20 @@ void IPASoftSimple::processStats(const ControlList &sensorControls)
 	 * Calculate Mean Sample Value (MSV) according to formula from:
 	 * https://www.araa.asn.au/acra/acra2007/papers/paper84final.pdf
 	 */
-	constexpr unsigned int yHistValsPerBin =
-		SwIspStats::kYHistogramSize / kExposureBinsCount;
-	constexpr unsigned int yHistValsPerBinMod =
-		SwIspStats::kYHistogramSize /
-		(SwIspStats::kYHistogramSize % kExposureBinsCount + 1);
+	const unsigned int blackLevelHistIdx =
+		params_->blackLevel / (256 / SwIspStats::kYHistogramSize);
+	const unsigned int histogramSize =
+		SwIspStats::kYHistogramSize - blackLevelHistIdx;
+	const unsigned int yHistValsPerBin = histogramSize / kExposureBinsCount;
+	const unsigned int yHistValsPerBinMod =
+		histogramSize / (histogramSize % kExposureBinsCount + 1);
 	int exposureBins[kExposureBinsCount] = {};
 	unsigned int denom = 0;
 	unsigned int num = 0;
 
-	for (unsigned int i = 0; i < SwIspStats::kYHistogramSize; i++) {
+	for (unsigned int i = 0; i < histogramSize; i++) {
 		unsigned int idx = (i - (i / yHistValsPerBinMod)) / yHistValsPerBin;
-		exposureBins[idx] += stats_->yHistogram[i];
+		exposureBins[idx] += stats_->yHistogram[blackLevelHistIdx + i];
 	}
 
 	for (unsigned int i = 0; i < kExposureBinsCount; i++) {
@@ -320,7 +329,8 @@ void IPASoftSimple::processStats(const ControlList &sensorControls)
 
 	LOG(IPASoft, Debug) << "exposureMSV " << exposureMSV
 			    << " exp " << exposure_ << " again " << again_
-			    << " gain R/B " << params_->gainR << "/" << params_->gainB;
+			    << " gain R/B " << params_->gainR << "/" << params_->gainB
+			    << " black level " << params_->blackLevel;
 }
 
 void IPASoftSimple::updateExposure(double exposureMSV)
diff --git a/src/libcamera/software_isp/TODO b/src/libcamera/software_isp/TODO
index fcb02588..4fcee39b 100644
--- a/src/libcamera/software_isp/TODO
+++ b/src/libcamera/software_isp/TODO
@@ -267,3 +267,13 @@ This could be handled better with DelayedControls.
 > 						    V4L2_CID_EXPOSURE }));
 
 You should use the DelayedControls class.
+
+---
+
+13. Improve black level and colour gains application
+
+I think the black level should eventually be moved before debayering, and
+ideally the colour gains as well. I understand the need for optimizations to
+lower the CPU consumption, but at the same time I don't feel comfortable
+building up on top of an implementation that may work a bit more by chance than
+by correctness, as that's not very maintainable.
diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp
index 5b553162..88d6578b 100644
--- a/src/libcamera/software_isp/debayer_cpu.cpp
+++ b/src/libcamera/software_isp/debayer_cpu.cpp
@@ -35,7 +35,7 @@ namespace libcamera {
  * \param[in] stats Pointer to the stats object to use
  */
 DebayerCpu::DebayerCpu(std::unique_ptr<SwStatsCpu> stats)
-	: stats_(std::move(stats)), gammaCorrection_(1.0)
+	: stats_(std::move(stats)), gammaCorrection_(1.0), blackLevel_(0)
 {
 	/*
 	 * Reading from uncached buffers may be very slow.
@@ -699,11 +699,16 @@ void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams
 	}
 
 	/* Apply DebayerParams */
-	if (params.gamma != gammaCorrection_) {
-		for (unsigned int i = 0; i < kGammaLookupSize; i++)
-			gamma_[i] = UINT8_MAX * powf(i / (kGammaLookupSize - 1.0), params.gamma);
+	if (params.gamma != gammaCorrection_ || params.blackLevel != blackLevel_) {
+		const unsigned int blackIndex =
+			params.blackLevel * kGammaLookupSize / 256;
+		std::fill(gamma_.begin(), gamma_.begin() + blackIndex, 0);
+		const float divisor = kGammaLookupSize - blackIndex - 1.0;
+		for (unsigned int i = blackIndex; i < kGammaLookupSize; i++)
+			gamma_[i] = UINT8_MAX * powf((i - blackIndex) / divisor, params.gamma);
 
 		gammaCorrection_ = params.gamma;
+		blackLevel_ = params.blackLevel;
 	}
 
 	if (swapRedBlueGains_)
diff --git a/src/libcamera/software_isp/debayer_cpu.h b/src/libcamera/software_isp/debayer_cpu.h
index e7a8ba74..689c1075 100644
--- a/src/libcamera/software_isp/debayer_cpu.h
+++ b/src/libcamera/software_isp/debayer_cpu.h
@@ -147,6 +147,7 @@ private:
 	bool enableInputMemcpy_;
 	bool swapRedBlueGains_;
 	float gammaCorrection_;
+	unsigned int blackLevel_;
 	unsigned int measuredFrames_;
 	int64_t frameProcessTime_;
 	/* Skip 30 frames for things to stabilize then measure 30 frames */
diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp
index d746d893..e4e56086 100644
--- a/src/libcamera/software_isp/software_isp.cpp
+++ b/src/libcamera/software_isp/software_isp.cpp
@@ -64,7 +64,7 @@ LOG_DEFINE_CATEGORY(SoftwareIsp)
  */
 SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor)
 	: debayerParams_{ DebayerParams::kGain10, DebayerParams::kGain10,
-			  DebayerParams::kGain10, 0.5f },
+			  DebayerParams::kGain10, 0.5f, 0 },
 	  dmaHeap_(DmaHeap::DmaHeapFlag::Cma | DmaHeap::DmaHeapFlag::System)
 {
 	if (!dmaHeap_.isValid()) {