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

[Milan Zamazal]

Laurent Pinchart <laurent.pinchart at ideasonboard.com> writes:

> Hi Milan,
>
> Thank you for the patch.

Hi Laurent,

thank you for your comments.

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

OK but let's keep it actionable -- IMHO the TODO file should be
temporary and the items should be either resolved soon or transformed
into something else, such as in-code TODOs.

Regards,
Milan

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