[PATCH v2 1/2] libcamera: software_isp: Add contrast algorithm
Kieran Bingham
kieran.bingham at ideasonboard.com
Thu Oct 10 00:15:25 CEST 2024
Quoting Milan Zamazal (2024-10-09 20:33:16)
> Software ISP is currently fully automatic and doesn't allow image
> modifications by explicitly set control values. The user has no means
> to make the image looking better.
>
> This patch introduces contrast control algorithm, which can improve
> e.g. a flat looking image. Based on the provided contrast value with a
> range 0..infinity and 1.0 being the normal value, it applies a simple
> S-curve modification to the image. The contrast algorithm just handles
> the provided values, while the S-curve is applied in the gamma algorithm
> on the computed gamma curve whenever the contrast value changes. Since
> the algorithm is applied only on the lookup table already present, its
> overhead is negligible.
>
> This is a preparation patch without actually activating the contrast
> algorithm, which will be done in the following patch.
>
> Signed-off-by: Milan Zamazal <mzamazal at redhat.com>
> ---
> src/ipa/simple/algorithms/contrast.cpp | 45 ++++++++++++++++++++++++++
> src/ipa/simple/algorithms/contrast.h | 37 +++++++++++++++++++++
> src/ipa/simple/algorithms/lut.cpp | 20 +++++++++---
> src/ipa/simple/algorithms/meson.build | 1 +
> src/ipa/simple/ipa_context.h | 7 ++++
> 5 files changed, 105 insertions(+), 5 deletions(-)
> create mode 100644 src/ipa/simple/algorithms/contrast.cpp
> create mode 100644 src/ipa/simple/algorithms/contrast.h
>
> diff --git a/src/ipa/simple/algorithms/contrast.cpp b/src/ipa/simple/algorithms/contrast.cpp
> new file mode 100644
> index 000000000..1a2c14cf9
> --- /dev/null
> +++ b/src/ipa/simple/algorithms/contrast.cpp
> @@ -0,0 +1,45 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2024, Red Hat Inc.
> + *
> + * Contrast adjustment
> + */
> +
> +#include "contrast.h"
> +
> +#include <optional>
> +
> +#include <libcamera/base/log.h>
> +
> +#include "control_ids.h"
> +
> +namespace libcamera {
> +
> +LOG_DEFINE_CATEGORY(IPASoftContrast)
> +
> +namespace ipa::soft::algorithms {
> +
> +int Contrast::configure(typename Module::Context &context,
> + [[maybe_unused]] const typename Module::Config &configInfo)
> +{
> + context.activeState.knobs.contrast = std::optional<double>();
> + return 0;
> +}
> +
> +void Contrast::queueRequest(typename Module::Context &context,
> + [[maybe_unused]] const uint32_t frame,
> + [[maybe_unused]] typename Module::FrameContext &frameContext,
> + const ControlList &controls)
> +{
> + const auto &contrast = controls.get(controls::Contrast);
> + if (contrast.has_value()) {
> + context.activeState.knobs.contrast = contrast;
> + LOG(IPASoftContrast, Debug) << "Setting contrast to " << contrast.value();
> + }
Is this only working by luck that 'Contrast' is alphabetically before
'LUT' ?
Or in fact, maybe it doesn't matter so much if it's being set here in
queueRequest() but it's a separate call in process() anyway to actually
consume it.
But if this impacts the Lut algorithm, it makes me think this is really
a control for that algorithm? Or maybe it doesn't matter... But I don't
think we would have 'one algorithm per control'. It's reasonable to have
a colorprocessing algorithm that would manage the LUT +
Contrast+Saturation or such for example...
> +}
> +
> +REGISTER_IPA_ALGORITHM(Contrast, "Contrast")
> +
> +} /* namespace ipa::soft::algorithms */
> +
> +} /* namespace libcamera */
> diff --git a/src/ipa/simple/algorithms/contrast.h b/src/ipa/simple/algorithms/contrast.h
> new file mode 100644
> index 000000000..0b3933099
> --- /dev/null
> +++ b/src/ipa/simple/algorithms/contrast.h
> @@ -0,0 +1,37 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2024, Red Hat Inc.
> + *
> + * Contrast adjustment
> + */
> +
> +#pragma once
> +
> +#include <libcamera/controls.h>
> +
> +#include "algorithm.h"
> +
> +namespace libcamera {
> +
> +namespace ipa::soft::algorithms {
> +
> +class Contrast : public Algorithm
> +{
> +public:
> + Contrast() = default;
> + ~Contrast() = default;
> +
> + int configure(typename Module::Context &context,
> + const typename Module::Config &configInfo)
> + override;
> +
> + void queueRequest(typename Module::Context &context,
> + const uint32_t frame,
> + typename Module::FrameContext &frameContext,
> + const ControlList &controls)
> + override;
> +};
> +
> +} /* namespace ipa::soft::algorithms */
> +
> +} /* namespace libcamera */
> diff --git a/src/ipa/simple/algorithms/lut.cpp b/src/ipa/simple/algorithms/lut.cpp
> index 9744e773a..8ebc824ce 100644
> --- a/src/ipa/simple/algorithms/lut.cpp
> +++ b/src/ipa/simple/algorithms/lut.cpp
> @@ -32,16 +32,25 @@ int Lut::configure(IPAContext &context,
> void Lut::updateGammaTable(IPAContext &context)
> {
> auto &gammaTable = context.activeState.gamma.gammaTable;
> - auto blackLevel = context.activeState.blc.level;
> + const auto blackLevel = context.activeState.blc.level;
> const unsigned int blackIndex = blackLevel * gammaTable.size() / 256;
> + const auto contrast = context.activeState.knobs.contrast.value_or(1.0);
>
> std::fill(gammaTable.begin(), gammaTable.begin() + blackIndex, 0);
> const float divisor = gammaTable.size() - blackIndex - 1.0;
> - for (unsigned int i = blackIndex; i < gammaTable.size(); i++)
> - gammaTable[i] = UINT8_MAX * std::pow((i - blackIndex) / divisor,
> - context.configuration.gamma);
> + for (unsigned int i = blackIndex; i < gammaTable.size(); i++) {
> + double normalized = (i - blackIndex) / divisor;
> + /* Apply simple S-curve */
> + if (normalized < 0.5)
> + normalized = 0.5 * std::pow(normalized / 0.5, contrast);
> + else
> + normalized = 1.0 - 0.5 * std::pow((1.0 - normalized) / 0.5, contrast);
> + gammaTable[i] = UINT8_MAX *
> + std::pow(normalized, context.configuration.gamma);
> + }
>
> context.activeState.gamma.blackLevel = blackLevel;
> + context.activeState.gamma.contrast = contrast;
> }
>
> void Lut::prepare(IPAContext &context,
> @@ -55,7 +64,8 @@ void Lut::prepare(IPAContext &context,
> * observed, it's not permanently prone to minor fluctuations or
> * rounding errors.
> */
> - if (context.activeState.gamma.blackLevel != context.activeState.blc.level)
> + if (context.activeState.gamma.blackLevel != context.activeState.blc.level ||
> + context.activeState.gamma.contrast != context.activeState.knobs.contrast)
> updateGammaTable(context);
>
> auto &gains = context.activeState.gains;
> diff --git a/src/ipa/simple/algorithms/meson.build b/src/ipa/simple/algorithms/meson.build
> index 37a2eb534..d75a7b2a1 100644
> --- a/src/ipa/simple/algorithms/meson.build
> +++ b/src/ipa/simple/algorithms/meson.build
> @@ -4,5 +4,6 @@ soft_simple_ipa_algorithms = files([
> 'awb.cpp',
> 'agc.cpp',
> 'blc.cpp',
> + 'contrast.cpp',
> 'lut.cpp',
> ])
> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h
> index 3519f20f6..5118d6abf 100644
> --- a/src/ipa/simple/ipa_context.h
> +++ b/src/ipa/simple/ipa_context.h
> @@ -8,8 +8,11 @@
> #pragma once
>
> #include <array>
> +#include <optional>
> #include <stdint.h>
>
> +#include <libcamera/controls.h>
> +
> #include <libipa/fc_queue.h>
>
> namespace libcamera {
> @@ -44,7 +47,11 @@ struct IPAActiveState {
> struct {
> std::array<double, kGammaLookupSize> gammaTable;
> uint8_t blackLevel;
> + double contrast;
> } gamma;
> + struct {
> + std::optional<double> contrast; // 0..inf, 1 = neutral
We avoid C++ commments throughout the project.
Is the definition of the value for contrast documented by the Control in
libcamera ? Or is this specific to the implementation detail here ?
> + } knobs;
> };
>
> struct IPAFrameContext : public FrameContext {
> --
> 2.44.1
>
More information about the libcamera-devel
mailing list