[PATCH v4 8/9] libcamera: software_isp: Track whether CCM is enabled
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Jan 28 16:29:55 CET 2025
Hi Milan,
On Tue, Jan 28, 2025 at 04:19:39PM +0100, Milan Zamazal wrote:
> Laurent Pinchart writes:
> > On Mon, Jan 13, 2025 at 02:51:05PM +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 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 | 11 +++++--
> >> src/ipa/simple/algorithms/ccm.h | 1 +
> >> src/ipa/simple/soft_simple.cpp | 6 ++--
> >> 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, 54 insertions(+), 36 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);
> >
> > Do you foresee cases where the CCM algorithm could be enabled or
> > disabled depending on the camera configuration ? If not, I would return
> > this from the init() function instead of the configure() function.
>
> OK, it's a bit more complicated to pass to the right place but possible.
> I'll do it in v5.
>
> >> [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 3c7fca2d..96038966 100644
> >> --- a/src/ipa/simple/algorithms/ccm.cpp
> >> +++ b/src/ipa/simple/algorithms/ccm.cpp
> >> @@ -38,12 +38,17 @@ int Ccm::init([[maybe_unused]] IPAContext &context, const YamlObject &tuningData
> >> return 0;
> >> }
> >>
> >> -void Ccm::prepare(IPAContext &context, const uint32_t frame,
> >> - IPAFrameContext &frameContext, [[maybe_unused]] DebayerParams *params)
> >> +int Ccm::configure(IPAContext &context,
> >> + [[maybe_unused]] const IPAConfigInfo &configInfo)
> >
> > I think you can skip this and handled the ccmEnabled variable purely in
> > the IPA module, but checking if the CCM algorithm has been instantiated.
> > This can be done by inspecting the algorithms list.
>
> I wouldn't like inspecting the algorithms list, I think it's cleaner to
> provide the information from the algorithm itself. Moreover, other
> algorithms need information about CCM.
You need to support the case where the CCM algorithm is not listed in
the tuning file, so the information has to be handled in the IPA module
itself. If the need is to check if a CCM algorithm has been
instantiated, then looking at the algorithms list seems the best fit.
> And finally, it cannot be
> handled purely in the IPA module because it needs to be passed to
> debayering (in software_isp.cpp).
That's not what I meant, I was talking about how to check if CCM needs
to be applied. The information of course has to be passed to the
pipeline handler.
And now that I wrote this, I realize it would be good to also consider
how this will be hanlded when sharing "ISP" configuration buffers
between the IPA module and the pipeline handler. The CCM configuration
could then possibly vary per-frame.
> > Another option is to use IPAACtiveState::ccm::enabled, by adding a
> > default initializer to set it to false, and set it to true in
> > Ccm::init().
>
> IPAActiveState is reset in configure, there was a bug when it was
> previously done in init. But it's possible to store the info elsewhere
> in the context.
>
> >> {
> >> context.activeState.ccm.enabled = ccmEnabled_;
> >> + return 0;
> >> +}
> >>
> >> - if (!ccmEnabled_)
> >> +void Ccm::prepare(IPAContext &context, const uint32_t frame,
> >> + IPAFrameContext &frameContext, [[maybe_unused]] DebayerParams *params)
> >> +{
> >> + if (!context.activeState.ccm.enabled)
> >> return;
> >>
> >> unsigned int ct = context.activeState.awb.temperatureK;
> >> 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..9b32e02b 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;
> >>
> >> @@ -248,6 +248,8 @@ int IPASoftSimple::configure(const IPAConfigInfo &configInfo)
> >> return ret;
> >> }
> >>
> >> + *ccmEnabled = context_.activeState.ccm.enabled;
> >> +
> >> 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..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)
> >> * \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)
> >
> > This function is not performance-critical, so you can pass ccmEnabled as
> > a function parameter. The SET_DEBAYER_METHODS() macro will need to
> > become a bit more complex as a result, something along the lines of
> >
> > #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>);
>
> Right, will do in v5.
>
> >> {
> >> 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);
> >> }
> >>
> >> /**
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list