[PATCH v7 3/3] ipa: rkisp1: algorithms: Add crosstalk algorithm
Paul Elder
paul.elder at ideasonboard.com
Thu Jun 13 10:13:28 CEST 2024
On Wed, Jun 12, 2024 at 12:38:34PM +0300, Laurent Pinchart wrote:
> On Wed, Jun 12, 2024 at 05:04:38PM +0900, Paul Elder wrote:
> > On Wed, Jun 12, 2024 at 02:51:20AM +0300, Laurent Pinchart wrote:
> > > On Tue, Jun 11, 2024 at 03:12:44PM +0100, Kieran Bingham wrote:
> > > > Quoting Paul Elder (2024-06-11 15:02:07)
> > > > > 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>
> > > > >
> > > > > ---
> > > > > Changes in v7:
> > > > > - make offsets_ default to zero-matrices as opposed to identity matrices
> > > > > - checkstyle
> > > > > - populate metadata
> > > > > - add ccm to IPAFrameContext
> > > > > - don't update the ccm if the color temperature didn't change
> > > > >
> > > > > 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 | 140 ++++++++++++++++++++++++++
> > > > > src/ipa/rkisp1/algorithms/ccm.h | 50 +++++++++
> > > > > src/ipa/rkisp1/algorithms/meson.build | 1 +
> > > > > src/ipa/rkisp1/ipa_context.h | 5 +
> > > > > 4 files changed, 196 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 000000000000..09fe4b2aa1bc
> > > > > --- /dev/null
> > > > > +++ b/src/ipa/rkisp1/algorithms/ccm.cpp
> > > > > @@ -0,0 +1,140 @@
> > > > > +/* 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/control_ids.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";
> > > > > + /*
> > > > > + * MatrixInterpolator::reset() resets to identity matrices
> > > > > + * while here we need zero matrices so we need to construct it
> > > > > + * ourselves.
> > > > > + */
> > > > > + Matrix<int16_t, 3, 1> m({ 0, 0, 0 });
> > > > > + std::map<unsigned int, Matrix<int16_t, 3, 1>> matrices = { { 0, m } };
> > > > > + offsets_ = MatrixInterpolator<int16_t, 3, 1>(matrices);
> > > > > + }
> > > > > +
> > > > > + 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]);
> > >
> > > Curly braces for outer loop.
> > >
> > > > > +
> > > > > + 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,
> > > > > + IPAFrameContext &frameContext,
> > > > > + rkisp1_params_cfg *params)
> > > > > +{
> > > > > + uint32_t ct = context.activeState.awb.temperatureK;
> > > > > + if (ct == ct_)
> > >
> > > ct_ should be initialized/reset in configure(), to ensure that we'll
> > > always set the CCM matrix on the first frame.
> > >
> > > I expect temperatureK to be fairly noisy, should we add a threshold to
> > > the comparison to avoid updating the CCM parameters on every frame in
> > > practice ?
> > >
> > > > > + return;
> > > >
> > > > Interestingly, I think this is fine (certainly for now) ... but this
> > > > means that the output request metadata will only write the CCM if it
> > > > changes.
> >
> > There was discussion before about "do we need to always write *all*
> > metadata" and iirc the conclusion was no so...
>
> I'm sure we'll revisit the topic at some point.
>
> > > > I think that's actually 'fine' if we imply that any frame that doesn't
> > > > supply metadata is the same as the previous state. Though I think RPi
> > > > currently reports it for every frame regardless.
> > > >
> > > > I'd be curious to see how Stefan's tooling handles this.
> > > >
> > > > We can always update on top, and my comments are addressed so:
> > > >
> > > > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > > >
> > > > > +
> > > > > + ct_ = ct;
> > > > > + Matrix<double, 3, 3> ccm = ccm_.get(ct);
> > > > > + Matrix<int16_t, 3, 1> offsets = offsets_.get(ct);
> > > > > +
> > > > > + frameContext.ccm.ccm = ccm;
> > > > > +
> > > > > + setParameters(params, ccm, offsets);
> > > > > +}
> > > > > +
> > > > > +/**
> > > > > + * \copydoc libcamera::ipa::Algorithm::process
> > > > > + */
> > > > > +void process([[maybe_unused]] IPAContext &context,
> > > > > + [[maybe_unused]] const uint32_t frame,
> > > > > + IPAFrameContext &frameContext,
> > > > > + [[maybe_unused]] const rkisp1_stat_buffer *stats,
> > > > > + ControlList &metadata)
> > > > > +{
> > > > > + float m[9];
> > > > > + for (unsigned int i = 0; i < 3; i++)
> > > > > + for (unsigned int j = 0; j < 3; j++)
> > > > > + m[i] = frameContext.ccm.ccm[i][j];
> > >
> > > Curly braces for outer loop.
> > >
> > > > > + metadata.set(controls::ColourCorrectionMatrix, m);
> > >
> > > It's interesting we're reporting the matrix but not the offsets. Not a
> > > blocker for this series, but should this be addressed ?
> >
> > I wasn't sure if there was demand for reporting the offsets...
> >
> > And actually I remember a link [1] that Stefan sent me a long time ago
> > that said something about them costing more computation and rarely
> > improving results...?
>
> That's what I was wondering too. I wonder if we should drop the offset.
>
I think they should be there to have the /option/ of being set.
Paul
> > [1] https://www.imatest.com/docs/colormatrix/
> >
> > > > > +}
> > > > > +
> > > > > +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 000000000000..09a6801626b4
> > > > > --- /dev/null
> > > > > +++ b/src/ipa/rkisp1/algorithms/ccm.h
> > > > > @@ -0,0 +1,50 @@
> > > > > +/* 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;
> > > > > + void process([[maybe_unused]] IPAContext &context,
> > >
> > > [[maybe_unused]] is not needed in the function declaration.
> > >
> > > > > + [[maybe_unused]] const uint32_t frame,
> > > > > + IPAFrameContext &frameContext,
> > > > > + [[maybe_unused]] const rkisp1_stat_buffer *stats,
> > > > > + ControlList &metadata) 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);
> > > > > +
> > > > > + unsigned int ct_;
> > > > > + MatrixInterpolator<double, 3, 3> ccm_;
> > >
> > > Given that the hardware representation is a 4.7 fixed point, would it be
> > > enough to use floats instead of doubles (here and in the frame context)
> > > ?
> > >
> > > > > + MatrixInterpolator<int16_t, 3, 1> offsets_;
> > >
> > > Why is the matrix stored in floating point values and the offsets as
> > > integers ?
> >
> > Because the offsets are integers in hardware as well while the ccm is
> > fixed-point.
> >
> > > > > +};
> > > > > +
> > > > > +} /* namespace ipa::rkisp1::algorithms */
> > > > > +
> > > > > +} /* namespace libcamera */
> > > > > diff --git a/src/ipa/rkisp1/algorithms/meson.build b/src/ipa/rkisp1/algorithms/meson.build
> > > > > index 6ee71a9b5da3..1734a6675f78 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',
> > > > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> > > > > index 2a994d81ae41..cfb1f9770870 100644
> > > > > --- a/src/ipa/rkisp1/ipa_context.h
> > > > > +++ b/src/ipa/rkisp1/ipa_context.h
> > > > > @@ -16,6 +16,7 @@
> > > > > #include <libcamera/geometry.h>
> > > > >
> > > > > #include <libipa/fc_queue.h>
> > > > > +#include <libipa/matrix.h>
> > > > >
> > > > > namespace libcamera {
> > > > >
> > > > > @@ -155,6 +156,10 @@ struct IPAFrameContext : public FrameContext {
> > > > > uint32_t exposure;
> > > > > double gain;
> > > > > } sensor;
> > > > > +
> > > > > + struct {
> > > > > + Matrix<double, 3, 3> ccm;
> > > > > + } ccm;
> > > > > };
> > > > >
> > > > > struct IPAContext {
>
More information about the libcamera-devel
mailing list