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

Milan Zamazal mzamazal at redhat.com
Wed Nov 27 20:07:14 CET 2024


Hi Stefan,

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

> Hi Milan,
>
> On Wed, Nov 27, 2024 at 04:51:01PM +0100, Milan Zamazal wrote:
>> Hi Stefan,
>> 
>> Stefan Klug <stefan.klug at 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 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.
>> 
>> OK, let's stick with an S-curve for now.
>> 
>> >> >> This is a preparation patch without actually providing the control
>> >> >> itself, which is done in the following patch.
>> >> >> 
>> >> >> Signed-off-by: Milan Zamazal <mzamazal at redhat.com>
>> >> >> ---
>> >> >>  src/ipa/simple/algorithms/lut.cpp | 38 +++++++++++++++++++++++++++----
>> >> >>  src/ipa/simple/algorithms/lut.h   |  5 ++++
>> >> >>  src/ipa/simple/ipa_context.h      |  7 ++++++
>> >> >>  3 files changed, 45 insertions(+), 5 deletions(-)
>> >> >> 
>> >> >> diff --git a/src/ipa/simple/algorithms/lut.cpp b/src/ipa/simple/algorithms/lut.cpp
>> >> >> index 9744e773a..ffded0594 100644
>> >> >> --- a/src/ipa/simple/algorithms/lut.cpp
>> >> >> +++ b/src/ipa/simple/algorithms/lut.cpp
>> >> >> @@ -9,14 +9,19 @@
>> >> >>  
>> >> >>  #include <algorithm>
>> >> >>  #include <cmath>
>> >> >> +#include <optional>
>> >> >>  #include <stdint.h>
>> >> >>  
>> >> >>  #include <libcamera/base/log.h>
>> >> >>  
>> >> >>  #include "simple/ipa_context.h"
>> >> >>  
>> >> >> +#include "control_ids.h"
>> >> >> +
>> >> >>  namespace libcamera {
>> >> >>  
>> >> >> +LOG_DEFINE_CATEGORY(IPASoftLut)
>> >> >> +
>> >> >>  namespace ipa::soft::algorithms {
>> >> >>  
>> >> >>  int Lut::configure(IPAContext &context,
>> >> >> @@ -24,24 +29,46 @@ int Lut::configure(IPAContext &context,
>> >> >>  {
>> >> >>  	/* Gamma value is fixed */
>> >> >>  	context.configuration.gamma = 0.5;
>> >> >> +	context.activeState.knobs.contrast = std::optional<double>();
>> >> >>  	updateGammaTable(context);
>> >> >>  
>> >> >>  	return 0;
>> >> >>  }
>> >> >>  
>> >> >> +void Lut::queueRequest(typename Module::Context &context,
>> >> >> +		       [[maybe_unused]] const uint32_t frame,
>> >> >> +		       [[maybe_unused]] typename Module::FrameContext &frameContext,
>> >> >> +		       const ControlList &controls)
>> >> >> +{
>> >> >> +	const auto &contrast = controls.get(controls::Contrast);
>> >> >> +	if (contrast.has_value()) {
>> >> >> +		context.activeState.knobs.contrast = contrast;
>> >> >> +		LOG(IPASoftLut, Debug) << "Setting contrast to " << contrast.value();
>> >> >> +	}
>> >> >> +}
>> >> >> +
>> >> >>  void Lut::updateGammaTable(IPAContext &context)
>> >> >>  {
>> >> >>  	auto &gammaTable = context.activeState.gamma.gammaTable;
>> >> >> -	auto blackLevel = context.activeState.blc.level;
>> >> >> +	const auto blackLevel = context.activeState.blc.level;
>> >> >>  	const unsigned int blackIndex = blackLevel * gammaTable.size() / 256;
>> >> >> +	const auto contrast = context.activeState.knobs.contrast.value_or(1.0);
>> >> >>  
>> >> >>  	std::fill(gammaTable.begin(), gammaTable.begin() + blackIndex, 0);
>> >> >>  	const float divisor = gammaTable.size() - blackIndex - 1.0;
>> >> >> -	for (unsigned int i = blackIndex; i < gammaTable.size(); i++)
>> >> >> -		gammaTable[i] = UINT8_MAX * std::pow((i - blackIndex) / divisor,
>> >> >> -						     context.configuration.gamma);
>> >> >> +	for (unsigned int i = blackIndex; i < gammaTable.size(); i++) {
>> >> >> +		double normalized = (i - blackIndex) / divisor;
>> >> >> +		/* Apply simple S-curve */
>> >> >> +		if (normalized < 0.5)
>> >> >> +			normalized = 0.5 * std::pow(normalized / 0.5, contrast);
>> >> >> +		else
>> >> >> +			normalized = 1.0 - 0.5 * std::pow((1.0 - normalized) / 0.5, contrast);
>> >
>> > One thing that bothers me on that curve is the asymmetry with regards to
>> > the contrast value. So a contrast value of 2 provides some contrast
>> > increase while a contrast value of 0 creates a completely flat response.
>> 
>> Well, the fact that the counterpart of 2 is 1/2 is not exactly asymmetry
>> but it's true that it doesn't play well with linear sliders.
>> 
>> > I believe a value range of -n to n would be easier to understand (0
>> > equals no change in contrast, -1 decrease, 1 increase) but that is a
>> > different topic.
>> 
>> Yes, this would violate the requirement of 1.0 being a normal contrast.
>> 
>> I can see that rpi uses [0, 32] range, while rkisp1 uses [0, 1.993], so
>> we have precedents for both the ways.  But considering that rpi is
>> generally special and GUI sliders are usually linear, I'm in favor of
>> [0, 2] range as you suggest below.
>> 
>> > 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 {
>> >> 
>> 



More information about the libcamera-devel mailing list