[PATCH v2 11/17] ipa: rkisp1: Implement manual ColourCorrectionMatrix control
Stefan Klug
stefan.klug at ideasonboard.com
Wed Apr 2 16:45:34 CEST 2025
Hi Laurent,
Thank you for the review.
On Wed, Apr 02, 2025 at 12:30:05AM +0300, Laurent Pinchart wrote:
> On Mon, Mar 31, 2025 at 06:39:48PM +0100, Kieran Bingham wrote:
> > Quoting Stefan Klug (2025-03-19 16:11:16)
> > > Add a manual ColourCorrectionMatrix control. This was already discussed
> > > while implementing manual colour temperature but was never implemented.
> > > The control allows to manually specify the CCM when AwbEnable is false.
> > >
> > > Signed-off-by: Stefan Klug <stefan.klug at ideasonboard.com>
> > >
> > > ---
> > >
> > > Changes in v2:
> > > - None
> > > ---
> > > src/ipa/rkisp1/algorithms/ccm.cpp | 59 ++++++++++++++++++++++++++++---
> > > src/ipa/rkisp1/algorithms/ccm.h | 6 ++++
> > > src/ipa/rkisp1/ipa_context.h | 3 +-
> > > 3 files changed, 62 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/src/ipa/rkisp1/algorithms/ccm.cpp b/src/ipa/rkisp1/algorithms/ccm.cpp
> > > index 2e5e91006b55..303ac3dd2fe2 100644
> > > --- a/src/ipa/rkisp1/algorithms/ccm.cpp
> > > +++ b/src/ipa/rkisp1/algorithms/ccm.cpp
> > > @@ -36,17 +36,25 @@ namespace ipa::rkisp1::algorithms {
> > >
> > > LOG_DEFINE_CATEGORY(RkISP1Ccm)
> > >
> > > +constexpr Matrix<float, 3, 3> kIdentity3x3 = Matrix<float, 3, 3>::identity();
> > > +
> > > /**
> > > * \copydoc libcamera::ipa::Algorithm::init
> > > */
> > > int Ccm::init([[maybe_unused]] IPAContext &context, const YamlObject &tuningData)
> > > {
> > > + auto &cmap = context.ctrlMap;
> > > + cmap[&controls::ColourCorrectionMatrix] = ControlInfo(
> > > + ControlValue(-8.0f),
> > > + ControlValue(7.993f),
> > > + ControlValue(kIdentity3x3.data()));
> > > +
> > > 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_.setData({ { 0, Matrix<float, 3, 3>::identity() } });
> > > + ccm_.setData({ { 0, kIdentity3x3 } });
> > > }
> > >
> > > ret = offsets_.readYaml(tuningData["ccms"], "ct", "offsets");
> > > @@ -61,13 +69,48 @@ int Ccm::init([[maybe_unused]] IPAContext &context, const YamlObject &tuningData
> > > return 0;
> > > }
> > >
> > > +/**
> > > + * \copydoc libcamera::ipa::Algorithm::configure
> > > + */
> > > +int Ccm::configure(IPAContext &context,
> > > + [[maybe_unused]] const IPACameraSensorInfo &configInfo)
> > > +{
> > > + auto &as = context.activeState;
> > > + as.ccm.manual = kIdentity3x3;
> > > + as.ccm.automatic = ccm_.getInterpolated(as.awb.automatic.temperatureK);
> > > + LOG(RkISP1Ccm, Debug) << "init matrix " << as.ccm.manual;
> >
> > I'm not sure that debug line adds much value...
> >
> > But aside from that, it seems to match the control documentation.
> >
> >
> > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> >
> >
> > > + return 0;
> > > +}
> > > +
> > > +void Ccm::queueRequest(IPAContext &context,
> > > + [[maybe_unused]] const uint32_t frame,
> > > + IPAFrameContext &frameContext,
> > > + const ControlList &controls)
> > > +{
> > > + /* Nothing to do here, the ccm will be calculated in prepare() */
> > > + if (frameContext.awb.autoEnabled)
> > > + return;
> > > +
> > > + auto &ccm = context.activeState.ccm;
> > > +
> > > + const auto &colourTemperature = controls.get(controls::ColourTemperature);
> > > + const auto &ccmMatrix = controls.get(controls::ColourCorrectionMatrix);
> > > + if (ccmMatrix)
> > > + ccm.manual = Matrix<float, 3, 3>(*ccmMatrix);
> > > + else if (colourTemperature)
> > > + ccm.manual = ccm_.getInterpolated(*colourTemperature);
> > > +
> > > + LOG(RkISP1Ccm, Debug) << "queueRequest matrix " << ccm.manual;
>
> To make the log less noisy, should this be restricted to when a CT or
> CCM matrix control was set ? Or maybe also indicate where the CCM came
> from ?
>
> if (ccmMatrix) {
> ccm.manual = Matrix<float, 3, 3>(*ccmMatrix);
> LOG(RkISP1Ccm, Debug)
> << "Setting manual CCM from CCM control to " << ccm.manual;
> } else if (colourTemperature) {
> ccm.manual = ccm_.getInterpolated(*colourTemperature);
> LOG(RkISP1Ccm, Debug)
> << "Setting manual CCM from CT control to " << ccm.manual;
> }
Yes, that is better. Integrated.
>
> or something similar.
>
> > > + frameContext.ccm.ccm = ccm.manual;
> > > +}
> > > +
> > > void Ccm::setParameters(struct rkisp1_cif_isp_ctk_config &config,
> > > const Matrix<float, 3, 3> &matrix,
> > > const Matrix<int16_t, 3, 1> &offsets)
> > > {
> > > /*
> > > * 4 bit integer and 7 bit fractional, ranging from -8 (0x400) to
> > > - * +7.992 (0x3ff)
> > > + * +7.9921875 (0x3ff)
>
> Someone likes precision :-)
I wanted a way to explain why I used 7.993 as control limit :-)
>
> > > */
> > > for (unsigned int i = 0; i < 3; i++) {
> > > for (unsigned int j = 0; j < 3; j++)
> > > @@ -88,14 +131,20 @@ void Ccm::setParameters(struct rkisp1_cif_isp_ctk_config &config,
> > > void Ccm::prepare(IPAContext &context, const uint32_t frame,
> > > IPAFrameContext &frameContext, RkISP1Params *params)
> > > {
> > > - uint32_t ct = frameContext.awb.temperatureK;
> > > + if (!frameContext.awb.autoEnabled) {
> > > + auto config = params->block<BlockType::Ctk>();
> > > + config.setEnabled(true);
> > > + setParameters(*config, frameContext.ccm.ccm, Matrix<int16_t, 3, 1>());
> > > + return;
> > > + }
> > >
> > > + uint32_t ct = frameContext.awb.temperatureK;
> > > /*
> > > * \todo The colour temperature will likely be noisy, add filtering to
> > > * avoid updating the CCM matrix all the time.
> > > */
> > > if (frame > 0 && ct == ct_) {
> > > - frameContext.ccm.ccm = context.activeState.ccm.ccm;
> > > + frameContext.ccm.ccm = context.activeState.ccm.automatic;
> > > return;
> > > }
> > >
> > > @@ -103,7 +152,7 @@ void Ccm::prepare(IPAContext &context, const uint32_t frame,
> > > Matrix<float, 3, 3> ccm = ccm_.getInterpolated(ct);
> > > Matrix<int16_t, 3, 1> offsets = offsets_.getInterpolated(ct);
> > >
> > > - context.activeState.ccm.ccm = ccm;
> > > + context.activeState.ccm.automatic = ccm;
> > > frameContext.ccm.ccm = ccm;
> > >
> > > auto config = params->block<BlockType::Ctk>();
> > > diff --git a/src/ipa/rkisp1/algorithms/ccm.h b/src/ipa/rkisp1/algorithms/ccm.h
> > > index a5d9a9a45e5d..c301e6e531c8 100644
> > > --- a/src/ipa/rkisp1/algorithms/ccm.h
> > > +++ b/src/ipa/rkisp1/algorithms/ccm.h
> > > @@ -26,6 +26,12 @@ public:
> > > ~Ccm() = default;
> > >
> > > int init(IPAContext &context, const YamlObject &tuningData) override;
> > > + int configure(IPAContext &context,
> > > + const IPACameraSensorInfo &configInfo) override;
> > > + void queueRequest(IPAContext &context,
> > > + const uint32_t frame,
> > > + IPAFrameContext &frameContext,
> > > + const ControlList &controls) override;
> > > void prepare(IPAContext &context, const uint32_t frame,
> > > IPAFrameContext &frameContext,
> > > RkISP1Params *params) override;
> > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> > > index 769e9f114e23..f0d504215d34 100644
> > > --- a/src/ipa/rkisp1/ipa_context.h
> > > +++ b/src/ipa/rkisp1/ipa_context.h
> > > @@ -101,7 +101,8 @@ struct IPAActiveState {
> > > } awb;
> > >
> > > struct {
> > > - Matrix<float, 3, 3> ccm;
> > > + Matrix<float, 3, 3> manual;
> > > + Matrix<float, 3, 3> automatic;
>
> Missing documentation updates.
Ahrgh, we should really add rkisp1 to doxygen. But that is a bit of
pain....
Regards,
Stefan
>
> > > } ccm;
> > >
> > > struct {
>
> --
> Regards,
>
> Laurent Pinchart
More information about the libcamera-devel
mailing list