[PATCH v4 3/4] libcamera: software_isp: Add support for contrast control
Milan Zamazal
mzamazal at redhat.com
Wed Nov 27 16:51:01 CET 2024
Hi Stefan,
Stefan Klug <stefan.klug at ideasonboard.com> writes:
> Hi Milan, hi Laurent,
>
> On Wed, Nov 27, 2024 at 10:42:57AM +0100, Milan Zamazal wrote:
>> Hi Laurent,
>>
>> Laurent Pinchart <laurent.pinchart at ideasonboard.com> writes:
>>
>> > Hi Milan,
>> >
>> > Thank you for the patch.
>> >
>> > On Tue, Nov 26, 2024 at 10:15:04AM +0100, Milan Zamazal wrote:
>> >> 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 support for contrast control, 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.
>> >
>> > Isn't contrast defined as a multiplier ? Applying a different luminance
>> > transformation and calling it contrast will make different cameras
>> > behave in different ways.
>>
>> control_ids_core.yaml says:
>>
>> - Contrast:
>> type: float
>> description: |
>> Specify a fixed contrast parameter.
>>
>> Normal contrast is given by the value 1.0; larger values produce images
>> with more contrast.
>>
>> And V4L2:
>>
>> V4L2_CID_CONTRAST (integer)
>> Picture contrast or luma gain.
>>
>> So nothing specific. I don't know what hardware ISPs usually do with
>> this setting but simply multiplying with clipping (if this is what you
>> mean) wouldn't be very useful regarding image quality.
>
> I agree that a linear contrast curve wouldn't serve image quality
> purposes but is merely for scientific purposes. We could implement
> something like a contrast mode (linear|s-curve), but I believe an
> S-curve is the most useful one to the users.
OK, let's stick with an S-curve for now.
>> >> This is a preparation patch without actually providing the control
>> >> itself, which is done in the following patch.
>> >>
>> >> Signed-off-by: Milan Zamazal <mzamazal at redhat.com>
>> >> ---
>> >> src/ipa/simple/algorithms/lut.cpp | 38 +++++++++++++++++++++++++++----
>> >> src/ipa/simple/algorithms/lut.h | 5 ++++
>> >> src/ipa/simple/ipa_context.h | 7 ++++++
>> >> 3 files changed, 45 insertions(+), 5 deletions(-)
>> >>
>> >> diff --git a/src/ipa/simple/algorithms/lut.cpp b/src/ipa/simple/algorithms/lut.cpp
>> >> index 9744e773a..ffded0594 100644
>> >> --- a/src/ipa/simple/algorithms/lut.cpp
>> >> +++ b/src/ipa/simple/algorithms/lut.cpp
>> >> @@ -9,14 +9,19 @@
>> >>
>> >> #include <algorithm>
>> >> #include <cmath>
>> >> +#include <optional>
>> >> #include <stdint.h>
>> >>
>> >> #include <libcamera/base/log.h>
>> >>
>> >> #include "simple/ipa_context.h"
>> >>
>> >> +#include "control_ids.h"
>> >> +
>> >> namespace libcamera {
>> >>
>> >> +LOG_DEFINE_CATEGORY(IPASoftLut)
>> >> +
>> >> namespace ipa::soft::algorithms {
>> >>
>> >> int Lut::configure(IPAContext &context,
>> >> @@ -24,24 +29,46 @@ int Lut::configure(IPAContext &context,
>> >> {
>> >> /* Gamma value is fixed */
>> >> context.configuration.gamma = 0.5;
>> >> + context.activeState.knobs.contrast = std::optional<double>();
>> >> updateGammaTable(context);
>> >>
>> >> return 0;
>> >> }
>> >>
>> >> +void Lut::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(IPASoftLut, Debug) << "Setting contrast to " << contrast.value();
>> >> + }
>> >> +}
>> >> +
>> >> 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);
>
> One thing that bothers me on that curve is the asymmetry with regards to
> the contrast value. So a contrast value of 2 provides some contrast
> increase while a contrast value of 0 creates a completely flat response.
Well, the fact that the counterpart of 2 is 1/2 is not exactly asymmetry
but it's true that it doesn't play well with linear sliders.
> I believe a value range of -n to n would be easier to understand (0
> equals no change in contrast, -1 decrease, 1 increase) but that is a
> different topic.
Yes, this would violate the requirement of 1.0 being a normal contrast.
I can see that rpi uses [0, 32] range, while rkisp1 uses [0, 1.993], so
we have precedents for both the ways. But considering that rpi is
generally special and GUI sliders are usually linear, I'm in favor of
[0, 2] range as you suggest below.
> I played around with the formula a bit:
> https://www.desmos.com/calculator/5zknbyjpln
>
> The red curve is the one from this patch, the green one is a symmetric
> version.
The modified formula looks nice to linearize the scale, I can use it.
Thank you for this demonstration.
> The steepness is something that might need attention as it goes to
> infinity for a contrast value of 2.
I wonder whether something similar could be the reason why rkisp1 uses
the mysterious value 1.993 as the upper bound (the message of the commit
introducing it doesn't explain it). I'd prefer using 2 in my code for
clarity, perhaps with some internal check (g++/glibc++ seems to be fine
with tan(M_PI/2) but better to prevent possible portability issues).
> To my knowledge there is no "official" s-curve with a corresponding
> defined behavior of the contrast value. We might lookup how gimp does
> it.
At a quick look, GIMP and photo editors (Darktable, Rawtherapee) seem to
transform contrast to curve points. GIMP computes
slant = tan ((config->contrast + 1) * G_PI_4);
(looks familiar, OK) that is then used to set some low and high output
points of the curve.
In our much simpler case, I think we wouldn't be wrong with your
"symmetric" formula.
> Best regards,
> Stefan
>
>> >> + 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 +82,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/lut.h b/src/ipa/simple/algorithms/lut.h
>> >> index b635987d0..ef2df147c 100644
>> >> --- a/src/ipa/simple/algorithms/lut.h
>> >> +++ b/src/ipa/simple/algorithms/lut.h
>> >> @@ -20,6 +20,11 @@ public:
>> >> ~Lut() = default;
>> >>
>> >> int configure(IPAContext &context, const IPAConfigInfo &configInfo) override;
>> >> + void queueRequest(typename Module::Context &context,
>> >> + const uint32_t frame,
>> >> + typename Module::FrameContext &frameContext,
>> >> + const ControlList &controls)
>> >> + override;
>> >> void prepare(IPAContext &context,
>> >> const uint32_t frame,
>> >> IPAFrameContext &frameContext,
>> >> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h
>> >> index fd7343e91..148052207 100644
>> >> --- a/src/ipa/simple/ipa_context.h
>> >> +++ b/src/ipa/simple/ipa_context.h
>> >> @@ -11,6 +11,8 @@
>> >> #include <optional>
>> >> #include <stdint.h>
>> >>
>> >> +#include <libcamera/controls.h>
>> >> +
>> >> #include <libipa/fc_queue.h>
>> >>
>> >> namespace libcamera {
>> >> @@ -48,7 +50,12 @@ struct IPAActiveState {
>> >> struct {
>> >> std::array<double, kGammaLookupSize> gammaTable;
>> >> uint8_t blackLevel;
>> >> + double contrast;
>> >> } gamma;
>> >> + struct {
>> >> + /* 0..inf range, 1.0 = normal */
>> >> + std::optional<double> contrast;
>> >> + } knobs;
>> >> };
>> >>
>> >> struct IPAFrameContext : public FrameContext {
>>
More information about the libcamera-devel
mailing list