[PATCH v4 3/4] libcamera: software_isp: Add support for contrast control

[Stefan Klug]

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.

> 
> >> 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.

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.

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 steepness is something that might need attention as it goes
to infinity for a contrast value of 2.

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.

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 {
>