[PATCH v6 3/3] ipa: rkisp1: algorithms: Add crosstalk algorithm
Paul Elder
paul.elder at ideasonboard.com
Tue Jun 11 10:53:27 CEST 2024
On Mon, Jun 10, 2024 at 01:57:04PM +0100, Kieran Bingham wrote:
> Quoting Paul Elder (2024-06-07 09:09:06)
> > Add an algorithm module to the rkisp1 IPA for crosstalk correction.
> >
> > Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> > Reviewed-by: Stefan Klug <stefan.klug at ideasonboard.com>
> >
> > ---
> > No change in v6
> >
> > Changes in v5:
> > - clean up documentation
> > - coalesce parseYaml into init
> >
> > Changes in v4:
> > - remove stray semicolons
> > - use the new matrix interpolator readYaml
> > - use the new matrix operator[] getter
> >
> > Changes in v3:
> > - read ccm offsets from tuning data, and write these offsets to the
> > parameters buffer
> > - make parseYaml return void, as it should fill in default data if
> > unable to read, thus never failing
> >
> > Changes in v2:
> > - rename ctk to ccm
> > - reset the matrix interpolator to identity matrix if failed to read
> > from tuning file
> > ---
> > src/ipa/rkisp1/algorithms/ccm.cpp | 109 ++++++++++++++++++++++++++
> > src/ipa/rkisp1/algorithms/ccm.h | 44 +++++++++++
> > src/ipa/rkisp1/algorithms/meson.build | 1 +
> > 3 files changed, 154 insertions(+)
> > create mode 100644 src/ipa/rkisp1/algorithms/ccm.cpp
> > create mode 100644 src/ipa/rkisp1/algorithms/ccm.h
> >
> > diff --git a/src/ipa/rkisp1/algorithms/ccm.cpp b/src/ipa/rkisp1/algorithms/ccm.cpp
> > new file mode 100644
> > index 000000000..7ae013601
> > --- /dev/null
> > +++ b/src/ipa/rkisp1/algorithms/ccm.cpp
> > @@ -0,0 +1,109 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2024, Ideas On Board
> > + *
> > + * RkISP1 Color Correction Matrix control algorithm
> > + */
> > +
> > +#include "ccm.h"
> > +
> > +#include <algorithm>
> > +#include <chrono>
> > +#include <cmath>
> > +#include <tuple>
> > +#include <vector>
> > +
> > +#include <libcamera/base/log.h>
> > +#include <libcamera/base/utils.h>
> > +
> > +#include <libcamera/ipa/core_ipa_interface.h>
> > +
> > +#include "libcamera/internal/yaml_parser.h"
> > +
> > +#include "../utils.h"
> > +#include "libipa/matrix_interpolator.h"
> > +
> > +/**
> > + * \file ccm.h
> > + */
> > +
> > +namespace libcamera {
> > +
> > +namespace ipa::rkisp1::algorithms {
> > +
> > +/**
> > + * \class Ccm
> > + * \brief A color correction matrix algorithm
> > + */
> > +
> > +LOG_DEFINE_CATEGORY(RkISP1Ccm)
> > +
> > +/**
> > + * \copydoc libcamera::ipa::Algorithm::init
> > + */
> > +int Ccm::init([[maybe_unused]] IPAContext &context, const YamlObject &tuningData)
> > +{
> > + int ret = ccm_.readYaml(tuningData["ccms"], "ct", "ccm");
> > + if (ret < 0) {
> > + LOG(RkISP1Ccm, Warning)
> > + << "Failed to parse 'ccm' "
> > + << "parameter from tuning file; falling back to unit matrix";
> > + ccm_.reset();
> > + }
> > +
> > + ret = offsets_.readYaml(tuningData["ccms"], "ct", "offsets");
> > + if (ret < 0) {
> > + LOG(RkISP1Ccm, Warning)
> > + << "Failed to parse 'offsets' "
> > + << "parameter from tuning file; falling back to zero offsets";
> > + offsets_.reset();
> > + }
>
> If both ccm_ and offsets_ are both a Matrix Interpolator - how do they
> fall back to different defaults?
>
> I would certainly /expect/ offsets to default to 0, and CCM to default
> to Identity/Unit matrix ... but I don't think that's what the above is
> doing ? Or did I miss something?
>
> > +
> > + return 0;
> > +}
> > +
> > +void Ccm::setParameters(rkisp1_params_cfg *params,
> > + const Matrix<double, 3, 3> &matrix,
> > + const Matrix<int16_t, 3, 1> &offsets)
> > +{
> > + struct rkisp1_cif_isp_ctk_config &config = params->others.ctk_config;
> > +
> > + /*
> > + * 4 bit integer and 7 bit fractional, ranging from -8 (0x400) to
> > + * +7.992 (0x3FF)
> > + */
> > + for (unsigned int i = 0; i < 3; i++)
> > + for (unsigned int j = 0; j < 3; j++)
> > + config.coeff[i][j] =
> > + utils::floatingToFixedPoint<4, 7, uint16_t, double>(matrix[i][j]);
> > +
> > + for (unsigned int i = 0; i < 3; i++)
> > + config.ct_offset[i] = offsets[i][0] & 0xFFF;
> > +
> > + LOG(RkISP1Ccm, Debug) << "Setting matrix " << matrix;
> > + LOG(RkISP1Ccm, Debug) << "Setting offsets " << offsets;
> > +
> > + params->module_en_update |= RKISP1_CIF_ISP_MODULE_CTK;
> > + params->module_ens |= RKISP1_CIF_ISP_MODULE_CTK;
> > + params->module_cfg_update |= RKISP1_CIF_ISP_MODULE_CTK;
> > +}
> > +
> > +/**
> > + * \copydoc libcamera::ipa::Algorithm::prepare
> > + */
> > +void Ccm::prepare(IPAContext &context, [[maybe_unused]] const uint32_t frame,
> > + [[maybe_unused]] IPAFrameContext &frameContext,
> > + rkisp1_params_cfg *params)
> > +{
> > + uint32_t ct = context.activeState.awb.temperatureK;
>
> I think I would anticipate that the Ccm would remember the current ct
> in a class private and only perform the below if it's changed.
Ah yeah ok.
>
> Perhaps with a customisable delta value to filter how often this gets
> updated.
I'd avoid this since it's an integer so we don't have to worry about
float comparisons, plus we'd have a fun time if the ct changes barely
less than delta but monotonically and suddently ct is super different
but never changed because it always changed by ct - 1 every frame.
>
>
>
> > + Matrix<double, 3, 3> ccm = ccm_.get(ct);
> > + Matrix<int16_t, 3, 1> offsets = offsets_.get(ct);
> > +
> > + setParameters(params, ccm, offsets);
>
> I think we should report the CCM back in the metadata.
Indeed.
Thanks,
Paul
>
>
> This probably means we should store the Ccm into the frameContext (ccm
> section should be added), and then during process() we set the metadata:
>
>
> metadata.set(controls::ColourCorrectionMatrix, frameContext.ccm);
>
>
> The control reports:
>
> - ColourCorrectionMatrix:
> type: float
> description: |
> The 3x3 matrix that converts camera RGB to sRGB within the
> imaging pipeline. This should describe the matrix that is used
> after pixels have been white-balanced, but before any gamma
> transformation. The 3x3 matrix is stored in conventional reading
> order in an array of 9 floating point values.
>
> size: [3,3]
>
>
> Can we confirm the control description matches the actions in the
> RKISP1?
>
>
>
>
> > +}
> > +
> > +REGISTER_IPA_ALGORITHM(Ccm, "Ccm")
> > +
> > +} /* namespace ipa::rkisp1::algorithms */
> > +
> > +} /* namespace libcamera */
> > diff --git a/src/ipa/rkisp1/algorithms/ccm.h b/src/ipa/rkisp1/algorithms/ccm.h
> > new file mode 100644
> > index 000000000..35c2cf83c
> > --- /dev/null
> > +++ b/src/ipa/rkisp1/algorithms/ccm.h
> > @@ -0,0 +1,44 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2024, Ideas On Board
> > + *
> > + * RkISP1 Color Correction Matrix control algorithm
> > + */
> > +
> > +#pragma once
> > +
> > +#include <linux/rkisp1-config.h>
> > +
> > +#include "libipa/matrix.h"
> > +#include "libipa/matrix_interpolator.h"
> > +
> > +#include "algorithm.h"
> > +
> > +namespace libcamera {
> > +
> > +namespace ipa::rkisp1::algorithms {
> > +
> > +class Ccm : public Algorithm
> > +{
> > +public:
> > + Ccm() {}
> > + ~Ccm() = default;
> > +
> > + int init(IPAContext &context, const YamlObject &tuningData) override;
> > + void prepare(IPAContext &context, const uint32_t frame,
> > + IPAFrameContext &frameContext,
> > + rkisp1_params_cfg *params) override;
> > +
> > +private:
> > + void parseYaml(const YamlObject &tuningData);
> > + void setParameters(rkisp1_params_cfg *params,
> > + const Matrix<double, 3, 3> &matrix,
> > + const Matrix<int16_t, 3, 1> &offsets);
> > +
> > + MatrixInterpolator<double, 3, 3> ccm_;
> > + MatrixInterpolator<int16_t, 3, 1> offsets_;
> > +};
> > +
> > +} /* namespace ipa::rkisp1::algorithms */
> > +
> > +} /* namespace libcamera */
> > diff --git a/src/ipa/rkisp1/algorithms/meson.build b/src/ipa/rkisp1/algorithms/meson.build
> > index 93a483292..16de71332 100644
> > --- a/src/ipa/rkisp1/algorithms/meson.build
> > +++ b/src/ipa/rkisp1/algorithms/meson.build
> > @@ -4,6 +4,7 @@ rkisp1_ipa_algorithms = files([
> > 'agc.cpp',
> > 'awb.cpp',
> > 'blc.cpp',
> > + 'ccm.cpp',
> > 'cproc.cpp',
> > 'dpcc.cpp',
> > 'dpf.cpp',
> > --
> > 2.39.2
> >
More information about the libcamera-devel
mailing list