[PATCH v3 8/9] libcamera: software_isp: Track whether CCM is enabled
Milan Zamazal
mzamazal at redhat.com
Fri Jan 10 21:03:57 CET 2025
Hi Kieran,
thank you for review.
Kieran Bingham <kieran.bingham at ideasonboard.com> writes:
> 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...
Indeed, it looks confused, I'll revert this change.
>> return 0;
>> }
>>
>> +int Ccm::configure(IPAContext &context,
>> + [[maybe_unused]] const IPAConfigInfo &configInfo)
>> +{
>> + context.activeState.ccm.enabled = true;
This should be changed to:
context.activeState.ccm.enabled = ccmEnabled_;
>> + 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_
Yes, the removed lines should be replaced by
if (!context.activeState.ccm.enabled)
return;
>> 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.
AFAIK yes, `false' is already the default value.
>> +
>> 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.
Yes.
>> +
>> 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)
^^^^^^^^^^
`bool' missing here.
>> * \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