[PATCH v5 09/10] libcamera: software_isp: Track whether CCM is enabled
Milan Zamazal
mzamazal at redhat.com
Tue Feb 4 16:18:48 CET 2025
Hi Laurent,
thank you for review.
Laurent Pinchart <laurent.pinchart at ideasonboard.com> writes:
> Hi Milan,
>
> Thank you for the patch.
>
> On Thu, Jan 30, 2025 at 07:14:46PM +0100, Milan Zamazal wrote:
>> 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.
>>
>> 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 trick similar to 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>
>> ---
>> .../internal/software_isp/software_isp.h | 1 +
>> include/libcamera/ipa/soft.mojom | 2 +-
>> src/ipa/simple/algorithms/ccm.cpp | 2 +
>> src/ipa/simple/ipa_context.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 | 44 ++++++++++++-------
>> src/libcamera/software_isp/debayer_cpu.h | 29 ++++++------
>> src/libcamera/software_isp/software_isp.cpp | 5 ++-
>> 10 files changed, 61 insertions(+), 37 deletions(-)
>>
>> diff --git a/include/libcamera/internal/software_isp/software_isp.h b/include/libcamera/internal/software_isp/software_isp.h
>> index d51b03fd..156fde96 100644
>> --- a/include/libcamera/internal/software_isp/software_isp.h
>> +++ b/include/libcamera/internal/software_isp/software_isp.h
>> @@ -97,6 +97,7 @@ private:
>> SharedMemObject<DebayerParams> sharedParams_;
>> DebayerParams debayerParams_;
>> DmaBufAllocator dmaHeap_;
>> + bool ccmEnabled_;
>>
>> std::unique_ptr<ipa::soft::IPAProxySoft> ipa_;
>> };
>> diff --git a/include/libcamera/ipa/soft.mojom b/include/libcamera/ipa/soft.mojom
>> index d52e6f1a..ede74413 100644
>> --- a/include/libcamera/ipa/soft.mojom
>> +++ b/include/libcamera/ipa/soft.mojom
>> @@ -17,7 +17,7 @@ interface IPASoftInterface {
>> libcamera.SharedFD fdStats,
>> libcamera.SharedFD fdParams,
>> libcamera.ControlInfoMap sensorCtrlInfoMap)
>> - => (int32 ret, libcamera.ControlInfoMap ipaControls);
>> + => (int32 ret, libcamera.ControlInfoMap ipaControls, bool ccmEnabled);
>> start() => (int32 ret);
>> stop();
>> configure(IPAConfigInfo configInfo)
>> diff --git a/src/ipa/simple/algorithms/ccm.cpp b/src/ipa/simple/algorithms/ccm.cpp
>> index 069a12f8..c50eb17d 100644
>> --- a/src/ipa/simple/algorithms/ccm.cpp
>> +++ b/src/ipa/simple/algorithms/ccm.cpp
>> @@ -34,6 +34,8 @@ int Ccm::init([[maybe_unused]] IPAContext &context, const YamlObject &tuningData
>> return ret;
>> }
>>
>> + context.ccmEnabled = true;
>> +
>> return 0;
>> }
>>
>> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h
>> index bd6c66d8..40c13731 100644
>> --- a/src/ipa/simple/ipa_context.h
>> +++ b/src/ipa/simple/ipa_context.h
>> @@ -82,6 +82,7 @@ struct IPAContext {
>> IPAActiveState activeState;
>> FCQueue<IPAFrameContext> frameContexts;
>> ControlInfoMap::Map ctrlMap;
>> + bool ccmEnabled;
>> };
>>
>> } /* namespace ipa::soft */
>> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp
>> index b26e4e15..a87c6cdd 100644
>> --- a/src/ipa/simple/soft_simple.cpp
>> +++ b/src/ipa/simple/soft_simple.cpp
>> @@ -51,7 +51,8 @@ public:
>> const SharedFD &fdStats,
>> const SharedFD &fdParams,
>> const ControlInfoMap &sensorInfoMap,
>> - ControlInfoMap *ipaControls) override;
>> + ControlInfoMap *ipaControls,
>> + bool *ccmEnabled) override;
>> int configure(const IPAConfigInfo &configInfo) override;
>>
>> int start() override;
>> @@ -89,7 +90,8 @@ int IPASoftSimple::init(const IPASettings &settings,
>> const SharedFD &fdStats,
>> const SharedFD &fdParams,
>> const ControlInfoMap &sensorInfoMap,
>> - ControlInfoMap *ipaControls)
>> + ControlInfoMap *ipaControls,
>> + bool *ccmEnabled)
>> {
>> camHelper_ = CameraSensorHelperFactoryBase::create(settings.sensorModel);
>> if (!camHelper_) {
>> @@ -125,6 +127,8 @@ int IPASoftSimple::init(const IPASettings &settings,
>> if (ret)
>> return ret;
>>
>> + *ccmEnabled = context_.ccmEnabled;
>> +
>> params_ = nullptr;
>> stats_ = nullptr;
>>
>> diff --git a/src/libcamera/software_isp/debayer.cpp b/src/libcamera/software_isp/debayer.cpp
>> index f0b83261..45fe6960 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, bool ccmEnabled)
>
> If there are no overloads of the function, you can write
>
> * \fn int Debayer::configure()
>
> and doxygen will figure it out.
I see OK.
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
>> * \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..0cd03a8f 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,11 +368,17 @@ 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>;
>> -
>> -int DebayerCpu::setDebayerFunctions(PixelFormat inputFormat, PixelFormat outputFormat)
>> +#define SET_DEBAYER_METHODS(method0, method1) \
>> + debayer0_ = addAlphaByte \
>> + ? (ccmEnabled ? &DebayerCpu::method0<true, true> : &DebayerCpu::method0<true, false>) \
>> + : (ccmEnabled ? &DebayerCpu::method0<false, true> : &DebayerCpu::method0<false, false>); \
>> + debayer1_ = addAlphaByte \
>> + ? (ccmEnabled ? &DebayerCpu::method1<true, true> : &DebayerCpu::method1<true, false>) \
>> + : (ccmEnabled ? &DebayerCpu::method1<false, true> : &DebayerCpu::method1<false, false>);
>> +
>> +int DebayerCpu::setDebayerFunctions(PixelFormat inputFormat,
>> + PixelFormat outputFormat,
>> + bool ccmEnabled)
>> {
>> BayerFormat bayerFormat =
>> BayerFormat::fromPixelFormat(inputFormat);
>> @@ -464,7 +470,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 +510,10 @@ int DebayerCpu::configure(const StreamConfiguration &inputCfg,
>> return -EINVAL;
>> }
>>
>> - if (setDebayerFunctions(inputCfg.pixelFormat, outputCfg.pixelFormat) != 0)
>> + int ret = setDebayerFunctions(inputCfg.pixelFormat,
>> + outputCfg.pixelFormat,
>> + ccmEnabled);
>> + 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..21c08a2d 100644
>> --- a/src/libcamera/software_isp/debayer_cpu.h
>> +++ b/src/libcamera/software_isp/debayer_cpu.h
>> @@ -1,7 +1,7 @@
>> /* SPDX-License-Identifier: LGPL-2.1-or-later */
>> /*
>> * Copyright (C) 2023, Linaro Ltd
>> - * Copyright (C) 2023, Red Hat Inc.
>> + * Copyright (C) 2023-2025 Red Hat Inc.
>> *
>> * Authors:
>> * Hans de Goede <hdegoede at redhat.com>
>> @@ -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,7 +126,9 @@ private:
>> int getInputConfig(PixelFormat inputFormat, DebayerInputConfig &config);
>> int getOutputConfig(PixelFormat outputFormat, DebayerOutputConfig &config);
>> int setupStandardBayerOrder(BayerFormat::Order order);
>> - int setDebayerFunctions(PixelFormat inputFormat, PixelFormat outputFormat);
>> + int setDebayerFunctions(PixelFormat inputFormat,
>> + PixelFormat outputFormat,
>> + bool ccmEnabled);
>> void setupInputMemcpy(const uint8_t *linePointers[]);
>> void shiftLinePointers(const uint8_t *linePointers[], const uint8_t *src);
>> void memcpyNextLine(const uint8_t *linePointers[]);
>> diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp
>> index 44baf200..b03c4baa 100644
>> --- a/src/libcamera/software_isp/software_isp.cpp
>> +++ b/src/libcamera/software_isp/software_isp.cpp
>> @@ -128,7 +128,8 @@ SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor,
>> debayer_->getStatsFD(),
>> sharedParams_.fd(),
>> sensor->controls(),
>> - ipaControls);
>> + ipaControls,
>> + &ccmEnabled_);
>> if (ret) {
>> LOG(SoftwareIsp, Error) << "IPA init failed";
>> debayer_.reset();
>> @@ -240,7 +241,7 @@ int SoftwareIsp::configure(const StreamConfiguration &inputCfg,
>> if (ret < 0)
>> return ret;
>>
>> - return debayer_->configure(inputCfg, outputCfgs);
>> + return debayer_->configure(inputCfg, outputCfgs, ccmEnabled_);
>> }
>>
>> /**
More information about the libcamera-devel
mailing list