[PATCH 4/5] libcamera: software_isp: Remove TODO #13

[Milan Zamazal]

The current software ISP debayering implementation uses color lookup
tables that apply black level subtraction, color gain and gamma
correction in a single step, while performing debayering.  In theory,
black level subtraction and color gains should be applied before
debayering and gamma correction after debayering.  But because black
level subtraction and color gain corrections are currently linear and we
average only same-color pixels in debayering, we can apply all the
operations after debayering.  The only difference is with clipping where
out-of-range pixels contribute more than they deserve but this is not
generally significant.

Combining all the operations at once is conceptually not ideal but it is
not incorrect in this case and saves CPU cycles, which is critical for
software ISP CPU implementation (it may be less important with future
GPU implementation).  This means we need the current implementation.  It
may get replaced or become optional (configurable) in future, for
example if the separation is needed due to introducing other image
processing operations.

Under these circumstances and considering that the lookup tables
construction has been moved out of the debayering code in the preceding
patch, it makes no sense to keep software ISP TODO #13.  Let's remove it
in favor of a clarifying code comment.

Signed-off-by: Milan Zamazal <mzamazal at redhat.com>
---
 src/ipa/simple/soft_simple.cpp  | 10 ++++++++++
 src/libcamera/software_isp/TODO | 10 ----------
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp
index c5085f71..b2f4d308 100644
--- a/src/ipa/simple/soft_simple.cpp
+++ b/src/ipa/simple/soft_simple.cpp
@@ -247,6 +247,16 @@ void IPASoftSimple::stop()
 
 void IPASoftSimple::processStats(const ControlList &sensorControls)
 {
+	/*
+	 * Here black level subtraction, color gains and gamma correction are
+	 * combined into a single operation (table lookup) to save CPU cycles.
+	 * This works because black level subtraction and color gains are currently
+	 * linear operations and we average only same-color pixels in debayering.
+	 * If we change anything on this or introduce other operations in between,
+	 * it may be needed to split the operations and perform black and gain
+	 * corrections before debayering and gamma correction after debayering.
+	 */
+
 	SwIspStats::Histogram histogram = stats_->yHistogram;
 	if (ignoreUpdates_ > 0)
 		blackLevel_.update(histogram);
diff --git a/src/libcamera/software_isp/TODO b/src/libcamera/software_isp/TODO
index 4fcee39b..fcb02588 100644
--- a/src/libcamera/software_isp/TODO
+++ b/src/libcamera/software_isp/TODO
@@ -267,13 +267,3 @@ 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.
-- 
2.42.0