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

22094 diff mbox series

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.

This is a preparation patch without actually providing the control
itself, which is done in the following patch.

Signed-off-by: Milan Zamazal <mzamazal@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(-)

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.

> This is a preparation patch without actually providing the control
> itself, which is done in the following patch.
> 
> Signed-off-by: Milan Zamazal <mzamazal@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 {

Hi Laurent,

Laurent Pinchart <laurent.pinchart@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@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 {

Hi Milan, hi Laurent,

On Wed, Nov 27, 2024 at 10:42:57AM +0100, Milan Zamazal wrote:
> Hi Laurent,
> 
> Laurent Pinchart <laurent.pinchart@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@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 {
>

Hi Stefan,

Stefan Klug <stefan.klug@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@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@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 {
>>

Hi Milan,

On Wed, Nov 27, 2024 at 04:51:01PM +0100, Milan Zamazal wrote:
> Hi Stefan,
> 
> Stefan Klug <stefan.klug@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@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@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).

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

> 
> > 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 :-)

Cheers,
Stefan

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

Hi Stefan,

Stefan Klug <stefan.klug@ideasonboard.com> writes:

> Hi Milan,
>
> On Wed, Nov 27, 2024 at 04:51:01PM +0100, Milan Zamazal wrote:
>> Hi Stefan,
>> 
>> Stefan Klug <stefan.klug@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@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@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).
>
> 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
> 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 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 :-)
>
> Cheers,
> Stefan
>
>> 
>> (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 {
>> >> 
>>

(CC'ing David and Naush)

On Wed, Nov 27, 2024 at 08:07:14PM +0100, Milan Zamazal wrote:
> Stefan Klug <stefan.klug@ideasonboard.com> writes:
> > On Wed, Nov 27, 2024 at 04:51:01PM +0100, Milan Zamazal wrote:
> >> Stefan Klug <stefan.klug@ideasonboard.com> writes:
> >> > On Wed, Nov 27, 2024 at 10:42:57AM +0100, Milan Zamazal wrote:
> >> >> Laurent Pinchart <laurent.pinchart@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@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 {

Hi Laurent

On Thu, 28 Nov 2024 at 13:22, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> (CC'ing David and Naush)
>
> On Wed, Nov 27, 2024 at 08:07:14PM +0100, Milan Zamazal wrote:
> > Stefan Klug <stefan.klug@ideasonboard.com> writes:
> > > On Wed, Nov 27, 2024 at 04:51:01PM +0100, Milan Zamazal wrote:
> > >> Stefan Klug <stefan.klug@ideasonboard.com> writes:
> > >> > On Wed, Nov 27, 2024 at 10:42:57AM +0100, Milan Zamazal wrote:
> > >> >> Laurent Pinchart <laurent.pinchart@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@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 ?

Yes, that's what it does. Historically, it's what we've always done,
and none of our users seem to ask about it. To be honest, I've always
felt that this control is only there because people "expect" it, it's
almost never the right thing to use.

For example, if you're changing the values at runtime to try and
improve the images, well, then the tuning/control algorithms should
probably be working better. If you're always applying the same values
because that seems better, well, then the tuning is probably still
wrong!!

So I don't think we really see any need to revisit this for the moment.

David

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