[4/5] libcamera: software_isp: Remove TODO #13
diff mbox series

Message ID 20240423182000.1527425-5-mzamazal@redhat.com
State Superseded
Headers show
Series
  • Software ISP levels cleanup
Related show

Commit Message

Milan Zamazal April 23, 2024, 6:19 p.m. UTC
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@redhat.com>
---
 src/ipa/simple/soft_simple.cpp  | 10 ++++++++++
 src/libcamera/software_isp/TODO | 10 ----------
 2 files changed, 10 insertions(+), 10 deletions(-)

Comments

Laurent Pinchart April 28, 2024, 11:11 p.m. UTC | #1
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@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.
Milan Zamazal April 29, 2024, 9:14 a.m. UTC | #2
Laurent Pinchart <laurent.pinchart@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@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.

Patch
diff mbox series

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.