[PATCH v4 6/9] libcamera: software_isp: Add CCM algorithm

[Milan Zamazal]

Hi Laurent,

thank you for review.

Laurent Pinchart <laurent.pinchart at ideasonboard.com> writes:

> Hi Milan,
>
> On Mon, Jan 13, 2025 at 02:51:03PM +0100, Milan Zamazal wrote:
>> This patch adds color correction matrix (CCM) algorithm to software ISP.
>> It is based on the corresponding algorithm in rkisp1.
>> 
>> The primary difference against hardware pipelines is that applying the
>> CCM is optional.  Applying CCM causes a significant slowdown, time
>> needed to process a frame raises by 40-90% on tested platforms.  If CCM
>> is really needed, it can be applied, if not, it's better to stick
>> without it.  This can be configured by presence or omission of Ccm
>> algorithm in the tuning file.
>> 
>> CCM is changed only if the determined temperature changes by at least
>> 100 K (an arbitrarily selected value), to avoid recomputing the matrices
>> and lookup tables all the time.
>> 
>> The outputs of the algorithm are not used yet, they will be enabled in
>> followup patches.
>> 
>> Signed-off-by: Milan Zamazal <mzamazal at redhat.com>
>> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>> ---
>>  src/ipa/simple/algorithms/ccm.cpp     | 88 +++++++++++++++++++++++++++
>>  src/ipa/simple/algorithms/ccm.h       | 45 ++++++++++++++
>>  src/ipa/simple/algorithms/meson.build |  1 +
>>  src/ipa/simple/ipa_context.h          | 13 ++++
>>  4 files changed, 147 insertions(+)
>>  create mode 100644 src/ipa/simple/algorithms/ccm.cpp
>>  create mode 100644 src/ipa/simple/algorithms/ccm.h
>> 
>> diff --git a/src/ipa/simple/algorithms/ccm.cpp b/src/ipa/simple/algorithms/ccm.cpp
>> new file mode 100644
>> index 00000000..3c7fca2d
>> --- /dev/null
>> +++ b/src/ipa/simple/algorithms/ccm.cpp
>> @@ -0,0 +1,88 @@
>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>> +/*
>> + * Copyright (C) 2024, Ideas On Board
>> + * Copyright (C) 2024, Red Hat Inc.
>> + *
>> + * Color correction matrix
>> + */
>> +
>> +#include "ccm.h"
>> +
>> +#include <stdlib.h>
>
> I think this isn't needed anymore.

Right.

>> +
>> +#include <libcamera/base/log.h>
>> +#include <libcamera/base/utils.h>
>> +
>> +#include <libcamera/control_ids.h>
>> +
>> +namespace libcamera {
>> +
>> +namespace ipa::soft::algorithms {
>> +
>> +LOG_DEFINE_CATEGORY(IPASoftCcm)
>> +
>> +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) {
>> +		LOG(IPASoftCcm, Warning)
>> +			<< "Failed to parse 'ccm' "
>> +			<< "parameter from tuning file; falling back to unit matrix";
>
> s/unit/identity/
>
> But is this message correct ? I don't know you fall back to the identify
> matrix. Maybe "disabling CCM" would be better ?
>
>> +		ccmEnabled_ = false;
>
> Is there a reason not to return an error here ? What's the use case for
> specifying a CCM algorithm in the tuning file with valid data ? I
> understand you want to support disabling CCM, and that's easily done by
> not listing the algorithm in the tuning file at all. Unless I'm missing
> something, you can return an error here, and drop the ccmEnabled_
> variable.

I agree, I'll change it to error and drop ccmEnabled_ in v5.

>> +	} else {
>> +		ccmEnabled_ = 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;
>> +
>> +	unsigned int ct = context.activeState.awb.temperatureK;
>
> const

Ack.

>> +
>> +	/* Change CCM only on bigger temperature changes. */
>> +	if (frame > 0 &&
>> +	    utils::abs_diff(ct, ct_) < kTemperatureThreshold) {
>> +		frameContext.ccm.ccm = context.activeState.ccm.ccm;
>> +		context.activeState.ccm.changed = false;
>> +		return;
>> +	}
>> +
>> +	ct_ = ct;
>
> Renaming ct_ to lastCt_ would make the code clearer in my opinion.

Yes, will do.

>> +	Matrix<double, 3, 3> ccm = ccm_.getInterpolated(ct);
>> +
>> +	context.activeState.ccm.ccm = ccm;
>> +	frameContext.ccm.ccm = ccm;
>> +	context.activeState.ccm.changed = true;
>> +}
>> +
>> +void Ccm::process([[maybe_unused]] IPAContext &context,
>> +		  [[maybe_unused]] const uint32_t frame,
>> +		  IPAFrameContext &frameContext,
>> +		  [[maybe_unused]] const SwIspStats *stats,
>> +		  ControlList &metadata)
>> +{
>> +	if (!ccmEnabled_)
>> +		return;
>> +
>> +	float m[9];
>> +	for (unsigned int i = 0; i < 3; i++) {
>> +		for (unsigned int j = 0; j < 3; j++)
>> +			m[i * 3 + j] = frameContext.ccm.ccm[i][j];
>> +	}
>
> I wonder if the Matrix class should expose its data_ member with a
> read-only accessor, to avoid this kind of copies. Not a candidate for
> this patch.

OK.

>> +	metadata.set(controls::ColourCorrectionMatrix, m);
>> +}
>> +
>> +REGISTER_IPA_ALGORITHM(Ccm, "Ccm")
>> +
>> +} /* namespace ipa::soft::algorithms */
>> +
>> +} /* namespace libcamera */
>> diff --git a/src/ipa/simple/algorithms/ccm.h b/src/ipa/simple/algorithms/ccm.h
>> new file mode 100644
>> index 00000000..23481a08
>> --- /dev/null
>> +++ b/src/ipa/simple/algorithms/ccm.h
>> @@ -0,0 +1,45 @@
>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>> +/*
>> + * Copyright (C) 2024, Red Hat Inc.
>> + *
>> + * Color correction matrix
>> + */
>> +
>> +#pragma once
>> +
>> +#include "libcamera/internal/matrix.h"
>> +
>> +#include <libipa/interpolator.h>
>> +
>> +#include "algorithm.h"
>> +
>> +namespace libcamera {
>> +
>> +namespace ipa::soft::algorithms {
>> +
>> +class Ccm : public Algorithm
>> +{
>> +public:
>> +	Ccm() = default;
>> +	~Ccm() = default;
>> +
>> +	int init(IPAContext &context, const YamlObject &tuningData) override;
>> +	void prepare(IPAContext &context,
>> +		     const uint32_t frame,
>> +		     IPAFrameContext &frameContext,
>> +		     DebayerParams *params) override;
>> +	void process(IPAContext &context, const uint32_t frame,
>> +		     IPAFrameContext &frameContext,
>> +		     const SwIspStats *stats,
>> +		     ControlList &metadata) override;
>> +
>> +private:
>> +	static unsigned int kTemperatureThreshold;
>
> 	static constexpr unsigned int kTemperatureThreshold = 100;
>
> This could also be a global variable in ccm.cpp.

OK.

>> +	unsigned int ct_;
>> +	bool ccmEnabled_;
>> +	Interpolator<Matrix<double, 3, 3>> ccm_;
>> +};
>> +
>> +} /* namespace ipa::soft::algorithms */
>> +
>> +} /* namespace libcamera */
>> diff --git a/src/ipa/simple/algorithms/meson.build b/src/ipa/simple/algorithms/meson.build
>> index 37a2eb53..2d0adb05 100644
>> --- a/src/ipa/simple/algorithms/meson.build
>> +++ b/src/ipa/simple/algorithms/meson.build
>> @@ -4,5 +4,6 @@ soft_simple_ipa_algorithms = files([
>>      'awb.cpp',
>>      'agc.cpp',
>>      'blc.cpp',
>> +    'ccm.cpp',
>>      'lut.cpp',
>>  ])
>> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h
>> index 607af45a..0def3eef 100644
>> --- a/src/ipa/simple/ipa_context.h
>> +++ b/src/ipa/simple/ipa_context.h
>> @@ -13,6 +13,8 @@
>>  
>>  #include <libcamera/controls.h>
>>  
>> +#include "libcamera/internal/matrix.h"
>> +
>>  #include <libipa/fc_queue.h>
>>  
>>  namespace libcamera {
>> @@ -50,6 +52,13 @@ struct IPAActiveState {
>>  		uint8_t blackLevel;
>>  		double contrast;
>>  	} gamma;
>> +
>> +	struct {
>> +		Matrix<double, 3, 3> ccm;
>> +		bool enabled;
>> +		bool changed;
>> +	} ccm;
>> +
>>  	struct {
>>  		/* 0..2 range, 1.0 = normal */
>>  		std::optional<double> contrast;
>> @@ -57,6 +66,10 @@ struct IPAActiveState {
>>  };
>>  
>>  struct IPAFrameContext : public FrameContext {
>> +	struct {
>> +		Matrix<double, 3, 3> ccm;
>> +	} ccm;
>> +
>>  	struct {
>>  		int32_t exposure;
>>  		double gain;