[PATCH v3 3/3] ipa: rkisp1: Add GammaOutCorrection algorithm
Kieran Bingham
kieran.bingham at ideasonboard.com
Tue Jun 4 09:58:08 CEST 2024
Quoting Stefan Klug (2024-06-04 08:34:55)
> Hi Kieran,
>
> On Mon, Jun 03, 2024 at 07:21:09PM +0200, Stefan Klug wrote:
> > 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.
>
> I did that. The code looks like this:
> ----
>
> /* The logarithmic segments as specified in the reference. */
> 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
> };
> 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;
> gamma_y[0] = 0;
> for (auto [i, size] : utils::enumerate(segments)) {
> x += size;
> gamma_y[i+1] = std::pow(x / 4096.0, 1.0 / gamma_) * 1023.0;
> }
>
> ---
>
> Is that really better, with that speciall handling of gamma_y[0]?
Maybe not, you can choose whichever you prefer ;-)
--
Kieran
More information about the libcamera-devel
mailing list