[PATCH v2 1/2] libcamera: software_isp: Add contrast algorithm
Milan Zamazal
mzamazal at redhat.com
Thu Oct 10 10:58:47 CEST 2024
Hi Kieran,
thank you for review.
Kieran Bingham <kieran.bingham at ideasonboard.com> writes:
> 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' ?
Contrast is put before Lut in the algorithm list in uncalibrated.yaml in
the following patch (the algorithm is just defined and not actually used
in this patch).
> 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...
OK, I can put it into Lut.
>> +}
>> +
>> +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.
OK.
> Is the definition of the value for contrast documented by the Control in
> libcamera ? Or is this specific to the implementation detail here ?
Partially, control_ids_core.yaml says:
"Normal contrast is given by the value 1.0; larger values produce
images with more contrast."
The bounds are not defined there.
>> + } knobs;
>> };
>>
>> struct IPAFrameContext : public FrameContext {
>> --
>> 2.44.1
>>
More information about the libcamera-devel
mailing list