[PATCH v4 3/4] libcamera: software_isp: Add support for contrast control
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Nov 28 14:22:11 CET 2024
(CC'ing David and Naush)
On Wed, Nov 27, 2024 at 08:07:14PM +0100, Milan Zamazal wrote:
> Stefan Klug <stefan.klug at ideasonboard.com> writes:
> > On Wed, Nov 27, 2024 at 04:51:01PM +0100, Milan Zamazal wrote:
> >> Stefan Klug <stefan.klug at ideasonboard.com> writes:
> >> > On Wed, Nov 27, 2024 at 10:42:57AM +0100, Milan Zamazal wrote:
> >> >> Laurent Pinchart <laurent.pinchart at ideasonboard.com> writes:
> >> >> > 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.
David, Naush, how does this work for Raspberry Pi ? As far as I can see,
contrast is applied through the gamma LUT, and on both the Pi 4 and Pi 5
it's a linear multiplier centered around the middle of the range. Is
that right ? Have you considered other types of contrast adjustments,
like an S curve as done in this patch ?
> >> > 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
That's just the range of the hardware contrast register, which stores a
Q1.7 value. 0x00 maps to 0.0, 0x80 to 1.0, and Oxff to 1.9921875
(255/128).
> >> clarity, perhaps with some internal check (g++/glibc++ seems to be fine
> >> with tan(M_PI/2) but better to prevent possible portability issues).
> >
> > I don't exactly know what the internal formula of the rkisp1 is, but as
> > the image doesn't turn to the full extremes, I don't think a value of 2
> > equals a vertical curve here. But maybe we just need to try it out and
As far as I know, the hardware uses this value as a linear multiplier
for the luma component (contrast is applied in YUV space).
> > compare them. Given the different hardware implementations I suspect it
> > will not be easy to come up with a scale that behaves similarly on each
> > platform.
>
> No problem, I don't think such a unification is necessary. I'm happy as
> long as it behaves sanely in camshark. :-)
>
> > But we can try. The 1.993 is a simple hardware limitation. They use a
> > 1.7 fixed point format to represent the value. And with 7 bits the
> > biggest value you can represent after the dot is 127/128 == 0,99218
I should have read the full e-mail before writing the above :-)
> I see, thank you for explanation.
>
> >> > 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);
> >
> > Oh funny. I didn't expect that :-)
> >
> >> (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.
> >>
> >> >> >> + 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 {
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list