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

[Laurent Pinchart]

Hi Milan,

Thank you for the patch.

On Tue, Apr 23, 2024 at 08:19:59PM +0200, Milan Zamazal wrote:
> 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.

I still think we need to split those operations. The black level
subtraction (coming before debayering and the colour gains) should then
use black level values from a tuning file. An additional contrast
enhancement after debayering can perform automatic histogram stretching
if desired.

As this is still being discussed, and experimentation is needed to
assess the impact, I think the TODO item should stay until we reach a
conclusion.

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

-- 
Regards,

Laurent Pinchart