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

[Milan Zamazal]

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.

>> 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);
>> +		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 {