[PATCH v4 5/5] libcamera: software_isp: Pass color lookup tables as floats

[Milan Zamazal]

Hi Laurent,

thank you for review.

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

> Hi Milan,
>
> Thank you for the patch.
>
> On Tue, May 28, 2024 at 06:11:26PM +0200, Milan Zamazal wrote:
>> The color lookup tables are passed from stats processing to debayering
>> for direct use as 8-bit color output values.  Let's pass the values as
>> 0.0..1.0 floats to make the gains color bit width independent.
>> 
>> Completes software ISP TODO #4.
>
> The implementation looks good, and this indeed covers TODO #4. The
> conversion of the tables from float to integer takes a big more CPU
> time, which is reasonable, but it got me thinking.
>
> TODO #4 was recorded at a time where the IPA module computed gain
> values, and the ISP computed the look up tables. The gains were
> higher-level parameters. I thought it would be good to not hardcode in
> their representation what was an internal implementation feature of the
> ISP, as that would allow changing the ISP implementation later without
> impacting the IPA module.
>
> Now that the look up tables are computed in the IPA module, the IPA and
> ISP are more tightly coupled. Do you think it's still useful to abstract
> the internal representation of the table entries from the IPA module, or
> would that just consume more CPU cycles without offering any benefit for
> later evolutions of the ISP ? I'm thinking in particular about future
> work on a GPU-based implementation of the soft ISP. Do you foresee that
> we would use the same IPA module, or a different one ?
>
> If you think it's useful we can merge this patch, otherwise we could
> drop the patch and drop TODO #4.

Well, after thinking about it too, I decided for dropping the patch (and
dropping TODO #4 as a replacement patch).  My reasoning:

- The conversion cost is negligible to the whole frame processing on CPU
  but it's still some cost and it's better to avoid adding small costs
  without a good reason (and let's not make Hans crying ;-) ).

- We don't have a good reason for the conversion right now.

- Hardware pipelines seem to use integers in similar situations.

- I'm not sure about a GPU-based implementation but I think using the
  same IPA module would be preferable.

> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
>> Signed-off-by: Milan Zamazal <mzamazal at redhat.com>
>> Reviewed-by: Andrei Konovalov <andrey.konovalov.ynk at gmail.com>
>> ---
>>  .../internal/software_isp/debayer_params.h     |  2 +-
>>  src/ipa/simple/soft_simple.cpp                 |  5 ++---
>>  src/libcamera/software_isp/TODO                | 13 -------------
>>  src/libcamera/software_isp/debayer.cpp         |  6 +++---
>>  src/libcamera/software_isp/debayer_cpu.cpp     | 18 +++++++++++++++---
>>  src/libcamera/software_isp/debayer_cpu.h       | 10 +++++++---
>>  6 files changed, 28 insertions(+), 26 deletions(-)
>> 
>> diff --git a/include/libcamera/internal/software_isp/debayer_params.h b/include/libcamera/internal/software_isp/debayer_params.h
>> index 2f75b4b5..647905c5 100644
>> --- a/include/libcamera/internal/software_isp/debayer_params.h
>> +++ b/include/libcamera/internal/software_isp/debayer_params.h
>> @@ -19,7 +19,7 @@ struct DebayerParams {
>>  	static constexpr unsigned int kRGBLookupSize = 256;
>>  	static constexpr float kGamma = 0.5;
>>  
>> -	using ColorLookupTable = std::array<uint8_t, kRGBLookupSize>;
>> +	using ColorLookupTable = std::array<float, kRGBLookupSize>;
>>  
>>  	ColorLookupTable red;
>>  	ColorLookupTable green;
>> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp
>> index 26597c99..b006ec4a 100644
>> --- a/src/ipa/simple/soft_simple.cpp
>> +++ b/src/ipa/simple/soft_simple.cpp
>> @@ -86,7 +86,7 @@ private:
>>  	BlackLevel blackLevel_;
>>  
>>  	static constexpr unsigned int kGammaLookupSize = 1024;
>> -	std::array<uint8_t, kGammaLookupSize> gammaTable_;
>> +	std::array<float, kGammaLookupSize> gammaTable_;
>>  	int lastBlackLevel_ = -1;
>>  
>>  	int32_t exposureMin_, exposureMax_;
>> @@ -281,8 +281,7 @@ void IPASoftSimple::processStats(const ControlList &sensorControls)
>>  		std::fill(gammaTable_.begin(), gammaTable_.begin() + blackIndex, 0);
>>  		const float divisor = kGammaLookupSize - blackIndex - 1.0;
>>  		for (unsigned int i = blackIndex; i < kGammaLookupSize; i++)
>> -			gammaTable_[i] = UINT8_MAX *
>> -					 powf((i - blackIndex) / divisor, DebayerParams::kGamma);
>> +			gammaTable_[i] = powf((i - blackIndex) / divisor, DebayerParams::kGamma);
>>  
>>  		lastBlackLevel_ = blackLevel;
>>  	}
>> diff --git a/src/libcamera/software_isp/TODO b/src/libcamera/software_isp/TODO
>> index 4fcee39b..6bdc5905 100644
>> --- a/src/libcamera/software_isp/TODO
>> +++ b/src/libcamera/software_isp/TODO
>> @@ -72,19 +72,6 @@ stats in hardware, such as the i.MX7), but please keep it on your radar.
>>  
>>  ---
>>  
>> -4. Hide internal representation of gains from callers
>> -
>> -> struct DebayerParams {
>> -> 	static constexpr unsigned int kGain10 = 256;
>> -
>> -Forcing the caller to deal with the internal representation of gains
>> -isn't nice, especially given that it precludes implementing gains of
>> -different precisions in different backend. Wouldn't it be better to pass
>> -the values as floating point numbers, and convert them to the internal
>> -representation in the implementation of process() before using them ?
>> -
>> ----
>> -
>>  5. Store ISP parameters in per-frame buffers
>>  
>>  > /**
>> diff --git a/src/libcamera/software_isp/debayer.cpp b/src/libcamera/software_isp/debayer.cpp
>> index 80bf5dbe..03001ae1 100644
>> --- a/src/libcamera/software_isp/debayer.cpp
>> +++ b/src/libcamera/software_isp/debayer.cpp
>> @@ -35,17 +35,17 @@ namespace libcamera {
>>  
>>  /**
>>   * \var DebayerParams::red
>> - * \brief Lookup table for red color, mapping input values to output values
>> + * \brief Lookup table for red color, mapping input values to 0.0..1.0
>>   */
>>  
>>  /**
>>   * \var DebayerParams::green
>> - * \brief Lookup table for green color, mapping input values to output values
>> + * \brief Lookup table for green color, mapping input values to 0.0..1.0
>>   */
>>  
>>  /**
>>   * \var DebayerParams::blue
>> - * \brief Lookup table for blue color, mapping input values to output values
>> + * \brief Lookup table for blue color, mapping input values to 0.0..1.0
>>   */
>>  
>>  /**
>> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp
>> index c038eed4..9dcb5f91 100644
>> --- a/src/libcamera/software_isp/debayer_cpu.cpp
>> +++ b/src/libcamera/software_isp/debayer_cpu.cpp
>> @@ -11,6 +11,8 @@
>>  
>>  #include "debayer_cpu.h"
>>  
>> +#include <stddef.h>
>> +#include <stdint.h>
>>  #include <stdlib.h>
>>  #include <time.h>
>>  
>> @@ -688,6 +690,14 @@ static inline int64_t timeDiff(timespec &after, timespec &before)
>>  	       (int64_t)after.tv_nsec - (int64_t)before.tv_nsec;
>>  }
>>  
>> +void DebayerCpu::updateColorLookupTable(
>> +	const DebayerParams::ColorLookupTable &src,
>> +	ColorLookupTable &dst)
>> +{
>> +	for (std::size_t i = 0; i < src.size(); i++)
>> +		dst[i] = src[i] * UINT8_MAX;
>> +}
>> +
>>  void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams params)
>>  {
>>  	timespec frameStartTime;
>> @@ -697,9 +707,11 @@ void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams
>>  		clock_gettime(CLOCK_MONOTONIC_RAW, &frameStartTime);
>>  	}
>>  
>> -	green_ = params.green;
>> -	red_ = swapRedBlueGains_ ? params.blue : params.red;
>> -	blue_ = swapRedBlueGains_ ? params.red : params.blue;
>> +	updateColorLookupTable(params.green, green_);
>> +	updateColorLookupTable(swapRedBlueGains_ ? params.blue : params.red,
>> +			       red_);
>> +	updateColorLookupTable(swapRedBlueGains_ ? params.red : params.blue,
>> +			       blue_);
>>  
>>  	/* Copy metadata from the input buffer */
>>  	FrameMetadata &metadata = output->_d()->metadata();
>> diff --git a/src/libcamera/software_isp/debayer_cpu.h b/src/libcamera/software_isp/debayer_cpu.h
>> index be7dcdca..dcf4e37d 100644
>> --- a/src/libcamera/software_isp/debayer_cpu.h
>> +++ b/src/libcamera/software_isp/debayer_cpu.h
>> @@ -18,6 +18,7 @@
>>  #include <libcamera/base/object.h>
>>  
>>  #include "libcamera/internal/bayer_format.h"
>> +#include "libcamera/internal/software_isp/debayer_params.h"
>>  
>>  #include "debayer.h"
>>  #include "swstats_cpu.h"
>> @@ -125,9 +126,12 @@ private:
>>  	/* Max. supported Bayer pattern height is 4, debayering this requires 5 lines */
>>  	static constexpr unsigned int kMaxLineBuffers = 5;
>>  
>> -	DebayerParams::ColorLookupTable red_;
>> -	DebayerParams::ColorLookupTable green_;
>> -	DebayerParams::ColorLookupTable blue_;
>> +	using ColorLookupTable = std::array<uint8_t, DebayerParams::kRGBLookupSize>;
>> +	void updateColorLookupTable(const DebayerParams::ColorLookupTable &src,
>> +				    ColorLookupTable &dst);
>> +	ColorLookupTable red_;
>> +	ColorLookupTable green_;
>> +	ColorLookupTable blue_;
>>  	debayerFn debayer0_;
>>  	debayerFn debayer1_;
>>  	debayerFn debayer2_;