[v2,5/5] libcamera: software_isp: Pass color lookup tables as floats

19973 diff mbox series

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.

Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
---
 .../internal/software_isp/debayer_params.h     |  2 +-
 src/ipa/simple/soft_simple.cpp                 |  4 ++--
 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(+), 25 deletions(-)

Hi Milan,

Thank you for the patch!

Reviewed-by: Andrei Konovalov <andrey.konovalov.ynk@gmail.com>

On 30.04.2024 20:34, 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.
> 
> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
> ---
>   .../internal/software_isp/debayer_params.h     |  2 +-
>   src/ipa/simple/soft_simple.cpp                 |  4 ++--
>   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(+), 25 deletions(-)
> 
> diff --git a/include/libcamera/internal/software_isp/debayer_params.h b/include/libcamera/internal/software_isp/debayer_params.h
> index 6aaa7d4c..a29eb37a 100644
> --- a/include/libcamera/internal/software_isp/debayer_params.h
> +++ b/include/libcamera/internal/software_isp/debayer_params.h
> @@ -18,7 +18,7 @@ namespace libcamera {
>   struct DebayerParams {
>   	static constexpr unsigned int kRGBLookupSize = 256;
>   
> -	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 0af23ee7..017fd15a 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_;
> @@ -283,7 +283,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, gamma);
> +			gammaTable_[i] = powf((i - blackIndex) / divisor, gamma);
>   
>   		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 548c2ccd..acc9cd21 100644
> --- a/src/libcamera/software_isp/debayer.cpp
> +++ b/src/libcamera/software_isp/debayer.cpp
> @@ -30,17 +30,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 8b2b2f40..df4c6e88 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 47373426..18808076 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_;