[PATCH v3 3/3] ipa: rkisp1: Add GammaOutCorrection algorithm

Stefan Klug stefan.klug at ideasonboard.com
Mon Jun 3 19:21:09 CEST 2024


Hi Kieran,

thanks for the review.

On Mon, Jun 03, 2024 at 04:57:25PM +0100, Kieran Bingham wrote:
> Quoting Stefan Klug (2024-06-03 15:06:30)
> > Add a gamma algorithm for the rkisp1. It defaults to a gamma of 2.2 which
> > closely resembles sRGB.  No seperate sRGB mode was implemented because the pwl
> > that models the gamma curve is so coarse that there is basically no difference
> > between srgb and gamma=2.2. The default can be overridden within the tuning
> > file or set at runtime using the gamma control.
> > 
> > The gamma algorithm is not enabled by default. This will be done in future
> > tuning file updates.
> > 
> > Signed-off-by: Stefan Klug <stefan.klug at ideasonboard.com>
> > ---
> > v2 -> v3:
> > - Squashed with patch 1/4 from previous series
> > - Renamed from Gamma to GammaOutCorrection
> > - Read default gamma value from tuning file
> > - Some stylistic fixes from review
> > 
> >  src/ipa/rkisp1/algorithms/goc.cpp     | 161 ++++++++++++++++++++++++++
> >  src/ipa/rkisp1/algorithms/goc.h       |  48 ++++++++
> >  src/ipa/rkisp1/algorithms/meson.build |   1 +
> >  src/ipa/rkisp1/ipa_context.h          |   4 +
> >  4 files changed, 214 insertions(+)
> >  create mode 100644 src/ipa/rkisp1/algorithms/goc.cpp
> >  create mode 100644 src/ipa/rkisp1/algorithms/goc.h
> > 
> > diff --git a/src/ipa/rkisp1/algorithms/goc.cpp b/src/ipa/rkisp1/algorithms/goc.cpp
> > new file mode 100644
> > index 00000000..875c82e5
> > --- /dev/null
> > +++ b/src/ipa/rkisp1/algorithms/goc.cpp
> > @@ -0,0 +1,161 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2024, Ideas On Board
> > + *
> > + * RkISP1 Gamma out control
> > + */
> > +#include "goc.h"
> > +
> > +#include <cmath>
> > +
> > +#include <libcamera/base/log.h>
> > +#include <libcamera/base/utils.h>
> > +
> > +#include <libcamera/control_ids.h>
> > +
> > +#include "libcamera/internal/yaml_parser.h"
> > +
> > +#include "linux/rkisp1-config.h"
> > +
> > +/**
> > + * \file goc.h
> > + */
> > +
> > +namespace libcamera {
> > +
> > +namespace ipa::rkisp1::algorithms {
> > +
> > +/**
> > + * \class GammaOutCorrection
> > + * \brief RkISP1 Gamma out correction
> > + *
> > + * This algorithm implements the gamma out curve for the RkISP1.
> > + * It defaults to a gamma value of 2.2
> > + * As gamma is internally represented as a piecewise linear function with only
> > + * 17 knots, the difference between gamma=2.2 and sRGB gamma is minimal.
> > + * Therefore sRGB gamma was not implemented as special case.
> > + *
> > + * Useful links:
> > + * https://www.cambridgeincolour.com/tutorials/gamma-correction.htm
> > + * https://en.wikipedia.org/wiki/SRGB
> > + */
> > +
> > +LOG_DEFINE_CATEGORY(RkISP1Gamma)
> > +
> > +const double kDefaultGamma = 2.2;
> > +
> > +GammaOutCorrection::GammaOutCorrection()
> > +       : gamma_(0)
> > +{
> > +}
> > +
> > +/**
> > + * \copydoc libcamera::ipa::Algorithm::init
> > + */
> > +int GammaOutCorrection::init([[maybe_unused]] IPAContext &context,
> > +                            [[maybe_unused]] const YamlObject &tuningData)
> > +{
> > +       context.ctrlMap[&controls::Gamma] = ControlInfo(0.1f, 10.0f, 2.2f);
> > +       defaultGamma_ = tuningData["gamma"].get<double>(kDefaultGamma);
> > +
> > +       if (context.hw->numGammaOutSamples !=
> > +           RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V10) {
> > +               LOG(RkISP1Gamma, Error)
> > +                       << "Gamma is not implemented for RkISP1 V12";
> 
> Is that because the size is different? Are there other differences that
> prevent us supporting it ?

Yes, and I have no documentation on the segment sizes for the
logarithmic case. (And no device to test it).

> 
> > +               return -EINVAL;
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> > +/**
> > + * \brief Configure the Gamma given a configInfo
> > + * \param[in] context The shared IPA context
> > + * \param[in] configInfo The IPA configuration data
> > + *
> > + * \return 0
> > + */
> > +int GammaOutCorrection::configure(IPAContext &context,
> > +                                 [[maybe_unused]] const IPACameraSensorInfo &configInfo)
> > +{
> > +       context.activeState.gamma = defaultGamma_;
> > +       return 0;
> > +}
> > +
> > +/**
> > + * \copydoc libcamera::ipa::Algorithm::queueRequest
> > + */
> > +void GammaOutCorrection::queueRequest([[maybe_unused]] IPAContext &context,
> > +                                     [[maybe_unused]] const uint32_t frame,
> > +                                     IPAFrameContext &frameContext,
> > +                                     const ControlList &controls)
> > +{
> > +       const auto &gamma = controls.get(controls::Gamma);
> > +       if (gamma) {
> > +               /*
> > +                * \todo This is not correct as it updates the current state with a
> > +                * future value. But it does no harm at the moment an allows us to
> > +                * track the last active gamma
> > +                */
> > +               context.activeState.gamma = *gamma;
> > +               LOG(RkISP1Gamma, Debug) << "Set gamma to " << *gamma;
> > +       }
> > +
> > +       frameContext.gamma = context.activeState.gamma;
> > +}
> > +
> > +/**
> > + * \copydoc libcamera::ipa::Algorithm::prepare
> > + */
> > +void GammaOutCorrection::prepare([[maybe_unused]] IPAContext &context,
> > +                                const uint32_t frame,
> > +                                [[maybe_unused]] IPAFrameContext &frameContext,
> > +                                rkisp1_params_cfg *params)
> > +{
> > +       ASSERT(context.hw->numGammaOutSamples ==
> > +              RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V10);
> > +
> > +       /*
> > +        * The logarithmic segments as specified in the reference.
> > +        * Plus an additional 0 to make the loop easier
> 
> Is the '0' still required now that this uses utils::enumerate(segments)?

The segments define the spacing between the kneepoints. So there is one
segment less than kneepoints. Therefore the 0 is still needed.

> 
> > +        */
> > +       std::array<unsigned, RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V10> segments = {
> > +               64, 64, 64, 64, 128, 128, 128, 128, 256,
> > +               256, 256, 512, 512, 512, 512, 512, 0
> > +       };
> 
> Presumably we'd have different segments for V12 ?

Yes, exactly. And I don't know the sizes.

> 
> > +       auto gamma_y = params->others.goc_config.gamma_y;
> > +
> > +       if (frame > 0 && std::fabs(frameContext.gamma - gamma_) < 0.001)
> > +               return;
> > +
> > +       gamma_ = frameContext.gamma;
> > +
> > +       unsigned x = 0;
> > +       for (auto [i, size] : utils::enumerate(segments)) {
> > +               gamma_y[i] = std::pow(x / 4096.0, 1.0 / gamma_) * 1023.0;
> > +               x += size;
> 
> Assuming the '0' is actually valid, as I think the last iteration
> through this loop will still have added x by 512 anyway,
> 

But without the 0 the loop would run one iteration too short and I would
need to duplicate the gamma_y = ... oh wait in our usecase gamma_y[0] is
always zero. I can start with gamma_y[1] and remove the 0. Dang.

> 
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

Thanks :-)

> 
> > +       }
> > +
> > +       params->others.goc_config.mode = RKISP1_CIF_ISP_GOC_MODE_LOGARITHMIC;
> > +       params->module_cfg_update |= RKISP1_CIF_ISP_MODULE_GOC;
> > +       params->module_en_update |= RKISP1_CIF_ISP_MODULE_GOC;
> > +       params->module_ens |= RKISP1_CIF_ISP_MODULE_GOC;
> > +}
> > +
> > +/**
> > + * \copydoc libcamera::ipa::Algorithm::process
> > + */
> > +void GammaOutCorrection::process([[maybe_unused]] IPAContext &context,
> > +                                [[maybe_unused]] const uint32_t frame,
> > +                                IPAFrameContext &frameContext,
> > +                                [[maybe_unused]] const rkisp1_stat_buffer *stats,
> > +                                ControlList &metadata)
> > +{
> > +       metadata.set(controls::Gamma, frameContext.gamma);
> > +}
> > +
> > +REGISTER_IPA_ALGORITHM(GammaOutCorrection, "GammaOutCorrection")
> > +
> > +} /* namespace ipa::rkisp1::algorithms */
> > +
> > +} /* namespace libcamera */
> > diff --git a/src/ipa/rkisp1/algorithms/goc.h b/src/ipa/rkisp1/algorithms/goc.h
> > new file mode 100644
> > index 00000000..d45e4194
> > --- /dev/null
> > +++ b/src/ipa/rkisp1/algorithms/goc.h
> > @@ -0,0 +1,48 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2024, Ideas On Board
> > + *
> > + * RkISP1 Gamma out control
> > + */
> > +
> > +#pragma once
> > +
> > +#include "algorithm.h"
> > +
> > +namespace libcamera {
> > +
> > +namespace ipa::rkisp1::algorithms {
> > +
> > +class GammaOutCorrection : public Algorithm
> > +{
> > +public:
> > +       GammaOutCorrection();
> > +       ~GammaOutCorrection() = 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,
> > +                    rkisp1_params_cfg *params) override;
> > +       void process(IPAContext &context, const uint32_t frame,
> > +                    IPAFrameContext &frameContext,
> > +                    const rkisp1_stat_buffer *stats,
> > +                    ControlList &metadata) override;
> > +
> > +private:
> > +       /*
> > +        * \todo: gamma_ holds the current state of the hardware. context.activeState
> > +        * can not be used as that holds the "future state" after applying all known
> > +        * requests. The intended usage of activeState needs to be clarified.
> > +        */
> > +       double gamma_;
> > +
> > +       double defaultGamma_;
> > +};
> > +
> > +} /* namespace ipa::rkisp1::algorithms */
> > +} /* namespace libcamera */
> > diff --git a/src/ipa/rkisp1/algorithms/meson.build b/src/ipa/rkisp1/algorithms/meson.build
> > index 93a48329..6ee71a9b 100644
> > --- a/src/ipa/rkisp1/algorithms/meson.build
> > +++ b/src/ipa/rkisp1/algorithms/meson.build
> > @@ -8,6 +8,7 @@ rkisp1_ipa_algorithms = files([
> >      'dpcc.cpp',
> >      'dpf.cpp',
> >      'filter.cpp',
> > +    'goc.cpp',
> >      'gsl.cpp',
> >      'lsc.cpp',
> >  ])
> > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> > index bd02a7a2..5252e222 100644
> > --- a/src/ipa/rkisp1/ipa_context.h
> > +++ b/src/ipa/rkisp1/ipa_context.h
> > @@ -104,6 +104,8 @@ struct IPAActiveState {
> >                 uint8_t denoise;
> >                 uint8_t sharpness;
> >         } filter;
> > +
> > +       double gamma;
> >  };
> >  
> >  struct IPAFrameContext : public FrameContext {
> > @@ -146,6 +148,8 @@ struct IPAFrameContext : public FrameContext {
> >                 uint32_t exposure;
> >                 double gain;
> >         } sensor;
> > +
> > +       double gamma;
> >  };
> >  
> >  struct IPAContext {
> > -- 
> > 2.43.0
> >


More information about the libcamera-devel mailing list