[PATCH v5 3/4] libcamera: software_isp: Add support for contrast control
Kieran Bingham
kieran.bingham at ideasonboard.com
Fri Dec 6 13:31:57 CET 2024
Hi Milan,
Quoting Milan Zamazal (2024-12-06 11:36:45)
> Hi Kieran,
>
> thank you for review and testing.
>
> Kieran Bingham <kieran.bingham at ideasonboard.com> writes:
>
> > Quoting Kieran Bingham (2024-12-06 00:22:58)
> >> Quoting Milan Zamazal (2024-11-28 12:52:24)
> >> > 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, 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.
> >> >
> >> > The contrast value range is 0..2 and corresponds to the whole range from
> >> > a completely flat contrast to an infinite contrast, 1.0 being the normal
> >> > value. This makes the user visible range intuitive and easy to use in
> >> > GUI sliders, while complying with Contrast control definition. There is
> >> > no unified range in the hardware pipelines, for example rkisp1 uses
> >> > 0..1.993 range while rpi uses 0..10 range.
> >> >
> >> > 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>
> >>
> >> Well, I've just tested this - and it does indeed affect the image, and
> >> respond correctly to a slider control in camshark ... so that's progress
> >> for the soft-isp from my perspective, and I know more will be built on
> >> top of this:
> >>
> >> Tested-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> >> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> >>
> >> > ---
> >> > src/ipa/simple/algorithms/lut.cpp | 40 +++++++++++++++++++++++++++----
> >> > src/ipa/simple/algorithms/lut.h | 5 ++++
> >> > src/ipa/simple/ipa_context.h | 7 ++++++
> >> > 3 files changed, 47 insertions(+), 5 deletions(-)
> >> >
> >> > diff --git a/src/ipa/simple/algorithms/lut.cpp b/src/ipa/simple/algorithms/lut.cpp
> >> > index 9744e773..dd76e117 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,48 @@ 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;
> >> > + /* Convert 0..2 to 0..infinity; avoid actual inifinity at tan(pi/2) */
> >> > + double contrastExp = tan(std::clamp(contrast * M_PI_4, 0.0, M_PI_2 - 0.00001));
> >> > + /* Apply simple S-curve */
> >> > + if (normalized < 0.5)
> >> > + normalized = 0.5 * std::pow(normalized / 0.5, contrastExp);
> >> > + else
> >> > + normalized = 1.0 - 0.5 * std::pow((1.0 - normalized) / 0.5, contrastExp);
> >> > + gammaTable[i] = UINT8_MAX *
> >> > + std::pow(normalized, context.configuration.gamma);
> >> > + }
> >> >
> >> > context.activeState.gamma.blackLevel = blackLevel;
> >> > + context.activeState.gamma.contrast = contrast;
> >
> > It would be nice if both the black level and the contrast were reported
> > in the metadata of the completed frame.
>
> Yes.
>
> > I guess that can be a patch on top as more work is coming on top of
> > this, but any control that affects the image should be reporting what
> > value was applied when that image completes in the request metadata.
>
> The primary problem is that we don't report metadata at all from
> software ISP. We just can produce it and other possible metadata items
> are also missing I think. It'd be probably best to fix it all at once,
> on top of this series (but separately).
>
> There are your patches from June in this direction, would you like to
> resume work on them or should I take them over?
Aha yes - there's
https://patchwork.libcamera.org/project/libcamera/list/?series=4405
If you're able to take that over I'd be quite happy to see it progress.
Looking at the first patch, I think Laurent asked for it to be updated
to report the values as applied based on the FrameContext - which you
now have in place, but wasn't available back then.
I don't have a lot of time to look at this so I think you would get
there before me!
Equally though - I don't think that blocks this patch here..
>
> > --
> > Kieran
> >
> >
> >> > }
> >> >
> >> > void Lut::prepare(IPAContext &context,
> >> > @@ -55,7 +84,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 b635987d..ef2df147 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 fd7343e9..0c2f7021 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..2 range, 1.0 = normal */
> >> > + std::optional<double> contrast;
> >> > + } knobs;
> >> > };
> >> >
> >> > struct IPAFrameContext : public FrameContext {
> >> > --
> >> > 2.44.2
> >> >
>
More information about the libcamera-devel
mailing list