[PATCH v4 5/5] libcamera: software_isp: Pass color lookup tables as floats
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed May 29 02:34:00 CEST 2024
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.
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_;
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list