[PATCH v3 8/9] libcamera: software_isp: Track whether CCM is enabled
Kieran Bingham
kieran.bingham at ideasonboard.com
Thu Jan 9 18:32:35 CET 2025
Quoting Milan Zamazal (2024-12-10 15:34:38)
> Applying color correction matrix (CCM) in software ISP is optional due
> to performance reasons. CCM is applied if and only if `Ccm' algorithm
> is present in the tuning file and it defines some CCM.
> Software ISP debayering is a performance critical piece of code and we
> do not want to use dynamic conditionals there. Therefore we pass
> information about CCM application to debayering configuration and let it
> select the right versions of debayering functions using templates. This
> is a similar trick as the previously used one for adding or not adding
> an alpha channel to the output.
>
> Debayering gets this information but it ignores it in this patch.
> Actual processing with CCM is added in the followup patch.
>
> Signed-off-by: Milan Zamazal <mzamazal at redhat.com>
> ---
> include/libcamera/ipa/soft.mojom | 2 +-
> src/ipa/simple/algorithms/ccm.cpp | 18 +++++------
> src/ipa/simple/algorithms/ccm.h | 1 +
> src/ipa/simple/soft_simple.cpp | 8 +++--
> src/libcamera/software_isp/debayer.cpp | 3 +-
> src/libcamera/software_isp/debayer.h | 3 +-
> src/libcamera/software_isp/debayer_cpu.cpp | 35 ++++++++++++---------
> src/libcamera/software_isp/debayer_cpu.h | 24 +++++++-------
> src/libcamera/software_isp/software_isp.cpp | 5 +--
> 9 files changed, 56 insertions(+), 43 deletions(-)
>
> diff --git a/include/libcamera/ipa/soft.mojom b/include/libcamera/ipa/soft.mojom
> index d52e6f1a..b5ed3905 100644
> --- a/include/libcamera/ipa/soft.mojom
> +++ b/include/libcamera/ipa/soft.mojom
> @@ -21,7 +21,7 @@ interface IPASoftInterface {
> start() => (int32 ret);
> stop();
> configure(IPAConfigInfo configInfo)
> - => (int32 ret);
> + => (int32 ret, bool ccmEnabled);
>
> [async] queueRequest(uint32 frame, libcamera.ControlList sensorControls);
> [async] computeParams(uint32 frame);
> diff --git a/src/ipa/simple/algorithms/ccm.cpp b/src/ipa/simple/algorithms/ccm.cpp
> index 40fe12c8..26bf8ff1 100644
> --- a/src/ipa/simple/algorithms/ccm.cpp
> +++ b/src/ipa/simple/algorithms/ccm.cpp
> @@ -25,26 +25,24 @@ unsigned int Ccm::kTemperatureThreshold = 100;
> int Ccm::init([[maybe_unused]] IPAContext &context, const YamlObject &tuningData)
> {
> int ret = ccm_.readYaml(tuningData["ccms"], "ct", "ccm");
> - if (ret < 0) {
> + if (ret < 0)
> LOG(IPASoftCcm, Warning)
> << "Failed to parse 'ccm' "
> << "parameter from tuning file; falling back to unit matrix";
> - ccmEnabled_ = false;
> - } else {
> - ccmEnabled_ = true;
> - }
I'm not seeing why we're not tracking this...
>
> return 0;
> }
>
> +int Ccm::configure(IPAContext &context,
> + [[maybe_unused]] const IPAConfigInfo &configInfo)
> +{
> + context.activeState.ccm.enabled = true;
> + return 0;
> +}
> +
> void Ccm::prepare(IPAContext &context, const uint32_t frame,
> IPAFrameContext &frameContext, [[maybe_unused]] DebayerParams *params)
> {
> - context.activeState.ccm.enabled = ccmEnabled_;
> -
> - if (!ccmEnabled_)
> - return;
> -
I'm not sure I understand why we're not returning and doing nothing if !ccmEnabled_
> unsigned int ct = context.activeState.awb.temperatureK;
>
> /* Change CCM only on bigger temperature changes. */
> diff --git a/src/ipa/simple/algorithms/ccm.h b/src/ipa/simple/algorithms/ccm.h
> index 23481a08..fe81ebd5 100644
> --- a/src/ipa/simple/algorithms/ccm.h
> +++ b/src/ipa/simple/algorithms/ccm.h
> @@ -24,6 +24,7 @@ public:
> ~Ccm() = default;
>
> int init(IPAContext &context, const YamlObject &tuningData) override;
> + int configure(IPAContext &context, const IPAConfigInfo &configInfo) override;
> void prepare(IPAContext &context,
> const uint32_t frame,
> IPAFrameContext &frameContext,
> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp
> index b26e4e15..545773fa 100644
> --- a/src/ipa/simple/soft_simple.cpp
> +++ b/src/ipa/simple/soft_simple.cpp
> @@ -52,7 +52,7 @@ public:
> const SharedFD &fdParams,
> const ControlInfoMap &sensorInfoMap,
> ControlInfoMap *ipaControls) override;
> - int configure(const IPAConfigInfo &configInfo) override;
> + int configure(const IPAConfigInfo &configInfo, bool *ccmEnabled) override;
>
> int start() override;
> void stop() override;
> @@ -182,7 +182,7 @@ int IPASoftSimple::init(const IPASettings &settings,
> return 0;
> }
>
> -int IPASoftSimple::configure(const IPAConfigInfo &configInfo)
> +int IPASoftSimple::configure(const IPAConfigInfo &configInfo, bool *ccmEnabled)
> {
> sensorInfoMap_ = configInfo.sensorControls;
>
> @@ -242,12 +242,16 @@ int IPASoftSimple::configure(const IPAConfigInfo &configInfo)
> context_.configuration.agc.againMinStep = 1.0;
> }
>
> + context_.activeState.ccm.enabled = false;
I would hope/expect this to be initialised to false already.
> +
> for (auto const &algo : algorithms()) {
> int ret = algo->configure(context_, configInfo);
> if (ret)
> return ret;
> }
>
> + *ccmEnabled = context_.activeState.ccm.enabled;
And if there's no ccm enabled in the tuning file - it should never
update it so it would still be possible to use this line, without
changing the current state tracking in the ccm module itself.
> +
> LOG(IPASoft, Info)
> << "Exposure " << context_.configuration.agc.exposureMin << "-"
> << context_.configuration.agc.exposureMax
> diff --git a/src/libcamera/software_isp/debayer.cpp b/src/libcamera/software_isp/debayer.cpp
> index f0b83261..595f73f6 100644
> --- a/src/libcamera/software_isp/debayer.cpp
> +++ b/src/libcamera/software_isp/debayer.cpp
> @@ -57,10 +57,11 @@ Debayer::~Debayer()
> }
>
> /**
> - * \fn int Debayer::configure(const StreamConfiguration &inputCfg, const std::vector<std::reference_wrapper<StreamConfiguration>> &outputCfgs)
> + * \fn int Debayer::configure(const StreamConfiguration &inputCfg, const std::vector<std::reference_wrapper<StreamConfiguration>> &outputCfgs, ccmEnabled)
> * \brief Configure the debayer object according to the passed in parameters
> * \param[in] inputCfg The input configuration
> * \param[in] outputCfgs The output configurations
> + * \param[in] ccmEnabled Whether a color correction matrix is applied
> *
> * \return 0 on success, a negative errno on failure
> */
> diff --git a/src/libcamera/software_isp/debayer.h b/src/libcamera/software_isp/debayer.h
> index d7ca060d..ba033d44 100644
> --- a/src/libcamera/software_isp/debayer.h
> +++ b/src/libcamera/software_isp/debayer.h
> @@ -33,7 +33,8 @@ public:
> virtual ~Debayer() = 0;
>
> virtual int configure(const StreamConfiguration &inputCfg,
> - const std::vector<std::reference_wrapper<StreamConfiguration>> &outputCfgs) = 0;
> + const std::vector<std::reference_wrapper<StreamConfiguration>> &outputCfgs,
> + bool ccmEnabled) = 0;
>
> virtual std::vector<PixelFormat> formats(PixelFormat inputFormat) = 0;
>
> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp
> index 01cfb36b..3c6597f9 100644
> --- a/src/libcamera/software_isp/debayer_cpu.cpp
> +++ b/src/libcamera/software_isp/debayer_cpu.cpp
> @@ -114,7 +114,7 @@ DebayerCpu::~DebayerCpu() = default;
> (prev[x] + curr[x - p] + curr[x + n] + next[x]) / (4 * (div)), \
> curr[x] / (div))
>
> -template<bool addAlphaByte>
> +template<bool addAlphaByte, bool ccmEnabled>
> void DebayerCpu::debayer8_BGBG_BGR888(uint8_t *dst, const uint8_t *src[])
> {
> DECLARE_SRC_POINTERS(uint8_t)
> @@ -125,7 +125,7 @@ void DebayerCpu::debayer8_BGBG_BGR888(uint8_t *dst, const uint8_t *src[])
> }
> }
>
> -template<bool addAlphaByte>
> +template<bool addAlphaByte, bool ccmEnabled>
> void DebayerCpu::debayer8_GRGR_BGR888(uint8_t *dst, const uint8_t *src[])
> {
> DECLARE_SRC_POINTERS(uint8_t)
> @@ -136,7 +136,7 @@ void DebayerCpu::debayer8_GRGR_BGR888(uint8_t *dst, const uint8_t *src[])
> }
> }
>
> -template<bool addAlphaByte>
> +template<bool addAlphaByte, bool ccmEnabled>
> void DebayerCpu::debayer10_BGBG_BGR888(uint8_t *dst, const uint8_t *src[])
> {
> DECLARE_SRC_POINTERS(uint16_t)
> @@ -148,7 +148,7 @@ void DebayerCpu::debayer10_BGBG_BGR888(uint8_t *dst, const uint8_t *src[])
> }
> }
>
> -template<bool addAlphaByte>
> +template<bool addAlphaByte, bool ccmEnabled>
> void DebayerCpu::debayer10_GRGR_BGR888(uint8_t *dst, const uint8_t *src[])
> {
> DECLARE_SRC_POINTERS(uint16_t)
> @@ -160,7 +160,7 @@ void DebayerCpu::debayer10_GRGR_BGR888(uint8_t *dst, const uint8_t *src[])
> }
> }
>
> -template<bool addAlphaByte>
> +template<bool addAlphaByte, bool ccmEnabled>
> void DebayerCpu::debayer12_BGBG_BGR888(uint8_t *dst, const uint8_t *src[])
> {
> DECLARE_SRC_POINTERS(uint16_t)
> @@ -172,7 +172,7 @@ void DebayerCpu::debayer12_BGBG_BGR888(uint8_t *dst, const uint8_t *src[])
> }
> }
>
> -template<bool addAlphaByte>
> +template<bool addAlphaByte, bool ccmEnabled>
> void DebayerCpu::debayer12_GRGR_BGR888(uint8_t *dst, const uint8_t *src[])
> {
> DECLARE_SRC_POINTERS(uint16_t)
> @@ -184,7 +184,7 @@ void DebayerCpu::debayer12_GRGR_BGR888(uint8_t *dst, const uint8_t *src[])
> }
> }
>
> -template<bool addAlphaByte>
> +template<bool addAlphaByte, bool ccmEnabled>
> void DebayerCpu::debayer10P_BGBG_BGR888(uint8_t *dst, const uint8_t *src[])
> {
> const int widthInBytes = window_.width * 5 / 4;
> @@ -210,7 +210,7 @@ void DebayerCpu::debayer10P_BGBG_BGR888(uint8_t *dst, const uint8_t *src[])
> }
> }
>
> -template<bool addAlphaByte>
> +template<bool addAlphaByte, bool ccmEnabled>
> void DebayerCpu::debayer10P_GRGR_BGR888(uint8_t *dst, const uint8_t *src[])
> {
> const int widthInBytes = window_.width * 5 / 4;
> @@ -231,7 +231,7 @@ void DebayerCpu::debayer10P_GRGR_BGR888(uint8_t *dst, const uint8_t *src[])
> }
> }
>
> -template<bool addAlphaByte>
> +template<bool addAlphaByte, bool ccmEnabled>
> void DebayerCpu::debayer10P_GBGB_BGR888(uint8_t *dst, const uint8_t *src[])
> {
> const int widthInBytes = window_.width * 5 / 4;
> @@ -252,7 +252,7 @@ void DebayerCpu::debayer10P_GBGB_BGR888(uint8_t *dst, const uint8_t *src[])
> }
> }
>
> -template<bool addAlphaByte>
> +template<bool addAlphaByte, bool ccmEnabled>
> void DebayerCpu::debayer10P_RGRG_BGR888(uint8_t *dst, const uint8_t *src[])
> {
> const int widthInBytes = window_.width * 5 / 4;
> @@ -368,10 +368,11 @@ int DebayerCpu::setupStandardBayerOrder(BayerFormat::Order order)
> return 0;
> }
>
> -#define SET_DEBAYER_METHODS(method0, method1) \
> - debayer0_ = addAlphaByte ? &DebayerCpu::method0<true> : &DebayerCpu::method0<false>; \
> - debayer1_ = addAlphaByte ? &DebayerCpu::method1<true> : &DebayerCpu::method1<false>;
> +#define SET_DEBAYER_METHODS(method0, method1) \
> + debayer0_ = addAlphaByte ? &DebayerCpu::method0<true, ccmEnabled> : &DebayerCpu::method0<false, ccmEnabled>; \
> + debayer1_ = addAlphaByte ? &DebayerCpu::method1<true, ccmEnabled> : &DebayerCpu::method1<false, ccmEnabled>;
>
> +template<bool ccmEnabled>
> int DebayerCpu::setDebayerFunctions(PixelFormat inputFormat, PixelFormat outputFormat)
> {
> BayerFormat bayerFormat =
> @@ -464,7 +465,8 @@ int DebayerCpu::setDebayerFunctions(PixelFormat inputFormat, PixelFormat outputF
> }
>
> int DebayerCpu::configure(const StreamConfiguration &inputCfg,
> - const std::vector<std::reference_wrapper<StreamConfiguration>> &outputCfgs)
> + const std::vector<std::reference_wrapper<StreamConfiguration>> &outputCfgs,
> + bool ccmEnabled)
> {
> if (getInputConfig(inputCfg.pixelFormat, inputConfig_) != 0)
> return -EINVAL;
> @@ -503,7 +505,10 @@ int DebayerCpu::configure(const StreamConfiguration &inputCfg,
> return -EINVAL;
> }
>
> - if (setDebayerFunctions(inputCfg.pixelFormat, outputCfg.pixelFormat) != 0)
> + int ret = ccmEnabled
> + ? setDebayerFunctions<true>(inputCfg.pixelFormat, outputCfg.pixelFormat)
> + : setDebayerFunctions<false>(inputCfg.pixelFormat, outputCfg.pixelFormat);
> + if (ret != 0)
> return -EINVAL;
>
> window_.x = ((inputCfg.size.width - outputCfg.size.width) / 2) &
> diff --git a/src/libcamera/software_isp/debayer_cpu.h b/src/libcamera/software_isp/debayer_cpu.h
> index 2c47e7c6..b2ec8f1b 100644
> --- a/src/libcamera/software_isp/debayer_cpu.h
> +++ b/src/libcamera/software_isp/debayer_cpu.h
> @@ -31,7 +31,8 @@ public:
> ~DebayerCpu();
>
> int configure(const StreamConfiguration &inputCfg,
> - const std::vector<std::reference_wrapper<StreamConfiguration>> &outputCfgs);
> + const std::vector<std::reference_wrapper<StreamConfiguration>> &outputCfgs,
> + bool ccmEnabled);
> Size patternSize(PixelFormat inputFormat);
> std::vector<PixelFormat> formats(PixelFormat input);
> std::tuple<unsigned int, unsigned int>
> @@ -85,28 +86,28 @@ private:
> using debayerFn = void (DebayerCpu::*)(uint8_t *dst, const uint8_t *src[]);
>
> /* 8-bit raw bayer format */
> - template<bool addAlphaByte>
> + template<bool addAlphaByte, bool ccmEnabled>
> void debayer8_BGBG_BGR888(uint8_t *dst, const uint8_t *src[]);
> - template<bool addAlphaByte>
> + template<bool addAlphaByte, bool ccmEnabled>
> void debayer8_GRGR_BGR888(uint8_t *dst, const uint8_t *src[]);
> /* unpacked 10-bit raw bayer format */
> - template<bool addAlphaByte>
> + template<bool addAlphaByte, bool ccmEnabled>
> void debayer10_BGBG_BGR888(uint8_t *dst, const uint8_t *src[]);
> - template<bool addAlphaByte>
> + template<bool addAlphaByte, bool ccmEnabled>
> void debayer10_GRGR_BGR888(uint8_t *dst, const uint8_t *src[]);
> /* unpacked 12-bit raw bayer format */
> - template<bool addAlphaByte>
> + template<bool addAlphaByte, bool ccmEnabled>
> void debayer12_BGBG_BGR888(uint8_t *dst, const uint8_t *src[]);
> - template<bool addAlphaByte>
> + template<bool addAlphaByte, bool ccmEnabled>
> void debayer12_GRGR_BGR888(uint8_t *dst, const uint8_t *src[]);
> /* CSI-2 packed 10-bit raw bayer format (all the 4 orders) */
> - template<bool addAlphaByte>
> + template<bool addAlphaByte, bool ccmEnabled>
> void debayer10P_BGBG_BGR888(uint8_t *dst, const uint8_t *src[]);
> - template<bool addAlphaByte>
> + template<bool addAlphaByte, bool ccmEnabled>
> void debayer10P_GRGR_BGR888(uint8_t *dst, const uint8_t *src[]);
> - template<bool addAlphaByte>
> + template<bool addAlphaByte, bool ccmEnabled>
> void debayer10P_GBGB_BGR888(uint8_t *dst, const uint8_t *src[]);
> - template<bool addAlphaByte>
> + template<bool addAlphaByte, bool ccmEnabled>
> void debayer10P_RGRG_BGR888(uint8_t *dst, const uint8_t *src[]);
>
> struct DebayerInputConfig {
> @@ -125,6 +126,7 @@ private:
> int getInputConfig(PixelFormat inputFormat, DebayerInputConfig &config);
> int getOutputConfig(PixelFormat outputFormat, DebayerOutputConfig &config);
> int setupStandardBayerOrder(BayerFormat::Order order);
> + template<bool ccmEnabled>
> int setDebayerFunctions(PixelFormat inputFormat, PixelFormat outputFormat);
> void setupInputMemcpy(const uint8_t *linePointers[]);
> void shiftLinePointers(const uint8_t *linePointers[], const uint8_t *src);
> diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp
> index 2bea64d9..6a07de85 100644
> --- a/src/libcamera/software_isp/software_isp.cpp
> +++ b/src/libcamera/software_isp/software_isp.cpp
> @@ -236,11 +236,12 @@ int SoftwareIsp::configure(const StreamConfiguration &inputCfg,
> {
> ASSERT(ipa_ && debayer_);
>
> - int ret = ipa_->configure(configInfo);
> + bool ccmEnabled;
> + int ret = ipa_->configure(configInfo, &ccmEnabled);
> if (ret < 0)
> return ret;
>
> - return debayer_->configure(inputCfg, outputCfgs);
> + return debayer_->configure(inputCfg, outputCfgs, ccmEnabled);
> }
>
> /**
> --
> 2.44.2
>
More information about the libcamera-devel
mailing list