[PATCH v6 3/5] libcamera: software_isp: Move color mappings out of debayering

Milan Zamazal mzamazal at redhat.com
Mon Jun 3 09:52:24 CEST 2024


Laurent Pinchart <laurent.pinchart at ideasonboard.com> writes:

> Hi Milan,
>
> Thank you for the patch.

Hi Laurent,

thank you for review.

> On Fri, May 31, 2024 at 02:38:38PM +0200, Milan Zamazal wrote:
>> Constructing the color mapping tables is related to stats rather than
>> debayering, where they are applied.  Let's move the corresponding code
>> to stats processing.
>> 
>> The same applies to the auxiliary gamma table.  As the gamma value is
>> currently fixed and used in a single place, with the temporary exception
>> mentioned below, there is no need to share it anywhere anymore.
>> 
>> It's necessary to initialize SoftwareIsp::debayerParams_ to default
>> values.  These initial values are used for the first two frames, before
>> they are changed based on determined stats.  To avoid sharing the gamma
>> value constant in artificial ways, we use 0.5 directly in the
>> initialization.  This all is not a particularly elegant thing to do,
>> such a code belongs conceptually to the similar code in stats
>> processing, but doing better is left for larger refactoring.
>> 
>> This is a preliminary step towards building this functionality on top of
>> libipa/algorithm.h, which should follow.
>> 
>> Signed-off-by: Milan Zamazal <mzamazal at redhat.com>
>> Reviewed-by: Andrei Konovalov <andrey.konovalov.ynk at gmail.com>
>> ---
>>  .../internal/software_isp/debayer_params.h    | 18 +++----
>>  src/ipa/simple/soft_simple.cpp                | 51 +++++++++++++++----
>>  src/libcamera/software_isp/debayer.cpp        | 29 +++++------
>>  src/libcamera/software_isp/debayer_cpu.cpp    | 43 +++-------------
>>  src/libcamera/software_isp/debayer_cpu.h      | 11 ++--
>>  src/libcamera/software_isp/software_isp.cpp   | 23 +++++++--
>>  6 files changed, 95 insertions(+), 80 deletions(-)
>> 
>> diff --git a/include/libcamera/internal/software_isp/debayer_params.h b/include/libcamera/internal/software_isp/debayer_params.h
>> index ce1b5945..463d24b3 100644
>> --- a/include/libcamera/internal/software_isp/debayer_params.h
>> +++ b/include/libcamera/internal/software_isp/debayer_params.h
>> @@ -1,6 +1,6 @@
>>  /* SPDX-License-Identifier: LGPL-2.1-or-later */
>>  /*
>> - * Copyright (C) 2023, Red Hat Inc.
>> + * Copyright (C) 2023, 2024 Red Hat Inc.
>>   *
>>   * Authors:
>>   * Hans de Goede <hdegoede at redhat.com>
>> @@ -10,20 +10,20 @@
>>  
>>  #pragma once
>>  
>> +#include <array>
>> +#include <stdint.h>
>> +
>>  namespace libcamera {
>>  
>>  struct DebayerParams {
>>  	static constexpr unsigned int kGain10 = 256;
>> +	static constexpr unsigned int kRGBLookupSize = 256;
>>  
>> -	unsigned int gainR;
>> -	unsigned int gainG;
>> -	unsigned int gainB;
>> +	using ColorLookupTable = std::array<uint8_t, kRGBLookupSize>;
>>  
>> -	float gamma;
>> -	/**
>> -	 * \brief Level of the black point, 0..255, 0 is no correction.
>> -	 */
>> -	unsigned int blackLevel;
>> +	ColorLookupTable red;
>> +	ColorLookupTable green;
>> +	ColorLookupTable blue;
>>  };
>>  
>>  } /* namespace libcamera */
>> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp
>> index f383f994..3388686f 100644
>> --- a/src/ipa/simple/soft_simple.cpp
>> +++ b/src/ipa/simple/soft_simple.cpp
>> @@ -5,6 +5,7 @@
>>   * Simple Software Image Processing Algorithm module
>>   */
>>  
>> +#include <cmath>
>>  #include <numeric>
>>  #include <stdint.h>
>>  #include <sys/mman.h>
>> @@ -84,6 +85,10 @@ private:
>>  	ControlInfoMap sensorInfoMap_;
>>  	BlackLevel blackLevel_;
>>  
>> +	static constexpr unsigned int kGammaLookupSize = 1024;
>> +	std::array<uint8_t, kGammaLookupSize> gammaTable_;
>> +	int lastBlackLevel_ = -1;
>> +
>>  	int32_t exposureMin_, exposureMax_;
>>  	int32_t exposure_;
>>  	double againMin_, againMax_, againMinStep_;
>> @@ -246,7 +251,6 @@ void IPASoftSimple::processStats(const ControlList &sensorControls)
>>  	if (ignoreUpdates_ > 0)
>>  		blackLevel_.update(histogram);
>>  	const uint8_t blackLevel = blackLevel_.get();
>> -	params_->blackLevel = blackLevel;
>>  
>>  	/*
>>  	 * Black level must be subtracted to get the correct AWB ratios,
>> @@ -263,13 +267,42 @@ void IPASoftSimple::processStats(const ControlList &sensorControls)
>>  	/*
>>  	 * Calculate red and blue gains for AWB.
>>  	 * Clamp max gain at 4.0, this also avoids 0 division.
>> +	 * Gain: 128 = 0.5, 256 = 1.0, 512 = 2.0, etc.
>>  	 */
>> -	params_->gainR = sumR <= sumG / 4 ? 1024 : 256 * sumG / sumR;
>> -	params_->gainB = sumB <= sumG / 4 ? 1024 : 256 * sumG / sumB;
>> -
>> +	const unsigned int gainR = sumR <= sumG / 4 ? 1024 : 256 * sumG / sumR;
>> +	const unsigned int gainB = sumB <= sumG / 4 ? 1024 : 256 * sumG / sumB;
>>  	/* Green gain and gamma values are fixed */
>> -	params_->gainG = 256;
>> -	params_->gamma = 0.5;
>> +	constexpr unsigned int gainG = 256;
>> +
>> +	/* Update the gamma table if needed */
>> +	if (blackLevel != lastBlackLevel_) {
>> +		constexpr float gamma = 0.5;
>> +		const unsigned int blackIndex = blackLevel * kGammaLookupSize / 256;
>> +		std::fill(gammaTable_.begin(), gammaTable_.begin() + blackIndex, 0);
>> +		const float divisor = kGammaLookupSize - blackIndex - 1.0;
>> +		for (unsigned int i = blackIndex; i < kGammaLookupSize; i++)
>> +			gammaTable_[i] = UINT8_MAX *
>> +					 std::pow((i - blackIndex) / divisor, gamma);
>> +
>> +		lastBlackLevel_ = blackLevel;
>> +	}
>> +
>> +	for (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++) {
>> +		constexpr unsigned int div =
>> +			DebayerParams::kRGBLookupSize * DebayerParams::kGain10 /
>> +			kGammaLookupSize;
>> +		unsigned int idx;
>> +
>> +		/* Apply gamma after gain! */
>> +		idx = std::min({ i * gainR / div, (kGammaLookupSize - 1) });
>> +		params_->red[i] = gammaTable_[idx];
>> +
>> +		idx = std::min({ i * gainG / div, (kGammaLookupSize - 1) });
>> +		params_->green[i] = gammaTable_[idx];
>> +
>> +		idx = std::min({ i * gainB / div, (kGammaLookupSize - 1) });
>> +		params_->blue[i] = gammaTable_[idx];
>> +	}
>>  
>>  	setIspParams.emit();
>>  
>> @@ -290,7 +323,7 @@ void IPASoftSimple::processStats(const ControlList &sensorControls)
>>  	 * https://www.araa.asn.au/acra/acra2007/papers/paper84final.pdf
>>  	 */
>>  	const unsigned int blackLevelHistIdx =
>> -		params_->blackLevel / (256 / SwIspStats::kYHistogramSize);
>> +		blackLevel / (256 / SwIspStats::kYHistogramSize);
>>  	const unsigned int histogramSize =
>>  		SwIspStats::kYHistogramSize - blackLevelHistIdx;
>>  	const unsigned int yHistValsPerBin = histogramSize / kExposureBinsCount;
>> @@ -338,8 +371,8 @@ void IPASoftSimple::processStats(const ControlList &sensorControls)
>>  
>>  	LOG(IPASoft, Debug) << "exposureMSV " << exposureMSV
>>  			    << " exp " << exposure_ << " again " << again_
>> -			    << " gain R/B " << params_->gainR << "/" << params_->gainB
>> -			    << " black level " << params_->blackLevel;
>> +			    << " gain R/B " << gainR << "/" << gainB
>> +			    << " black level " << static_cast<unsigned int>(blackLevel);
>>  }
>>  
>>  void IPASoftSimple::updateExposure(double exposureMSV)
>> diff --git a/src/libcamera/software_isp/debayer.cpp b/src/libcamera/software_isp/debayer.cpp
>> index efe75ea8..3f3969f7 100644
>> --- a/src/libcamera/software_isp/debayer.cpp
>> +++ b/src/libcamera/software_isp/debayer.cpp
>> @@ -1,7 +1,7 @@
>>  /* SPDX-License-Identifier: LGPL-2.1-or-later */
>>  /*
>>   * Copyright (C) 2023, Linaro Ltd
>> - * Copyright (C) 2023, Red Hat Inc.
>> + * Copyright (C) 2023, 2024 Red Hat Inc.
>>   *
>>   * Authors:
>>   * Hans de Goede <hdegoede at redhat.com>
>> @@ -24,29 +24,28 @@ namespace libcamera {
>>   */
>>  
>>  /**
>> - * \var DebayerParams::gainR
>> - * \brief Red gain
>> - *
>> - * 128 = 0.5, 256 = 1.0, 512 = 2.0, etc.
>> + * \var DebayerParams::kRGBLookupSize
>> + * \brief Size of a color lookup table
>>   */
>>  
>>  /**
>> - * \var DebayerParams::gainG
>> - * \brief Green gain
>> - *
>> - * 128 = 0.5, 256 = 1.0, 512 = 2.0, etc.
>> + * \typedef DebayerParams::ColorLookupTable
>> + * \brief Type of the lookup tables for red, green, blue values
>>   */
>>  
>>  /**
>> - * \var DebayerParams::gainB
>> - * \brief Blue gain
>> - *
>> - * 128 = 0.5, 256 = 1.0, 512 = 2.0, etc.
>> + * \var DebayerParams::red
>> + * \brief Lookup table for red color, mapping input values to output values
>> + */
>> +
>> +/**
>> + * \var DebayerParams::green
>> + * \brief Lookup table for green color, mapping input values to output values
>>   */
>>  
>>  /**
>> - * \var DebayerParams::gamma
>> - * \brief Gamma correction, 1.0 is no correction
>> + * \var DebayerParams::blue
>> + * \brief Lookup table for blue color, mapping input values to output values
>>   */
>>  
>>  /**
>> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp
>> index 8254bbe9..c038eed4 100644
>> --- a/src/libcamera/software_isp/debayer_cpu.cpp
>> +++ b/src/libcamera/software_isp/debayer_cpu.cpp
>> @@ -11,7 +11,6 @@
>>  
>>  #include "debayer_cpu.h"
>>  
>> -#include <math.h>
>>  #include <stdlib.h>
>>  #include <time.h>
>>  
>> @@ -35,7 +34,7 @@ namespace libcamera {
>>   * \param[in] stats Pointer to the stats object to use
>>   */
>>  DebayerCpu::DebayerCpu(std::unique_ptr<SwStatsCpu> stats)
>> -	: stats_(std::move(stats)), gammaCorrection_(1.0), blackLevel_(0)
>> +	: stats_(std::move(stats))
>>  {
>>  	/*
>>  	 * Reading from uncached buffers may be very slow.
>> @@ -47,9 +46,9 @@ DebayerCpu::DebayerCpu(std::unique_ptr<SwStatsCpu> stats)
>>  	 */
>>  	enableInputMemcpy_ = true;
>>  
>> -	/* Initialize gamma to 1.0 curve */
>> -	for (unsigned int i = 0; i < kGammaLookupSize; i++)
>> -		gamma_[i] = i / (kGammaLookupSize / kRGBLookupSize);
>> +	/* Initialize color lookup tables */
>> +	for (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++)
>> +		red_[i] = green_[i] = blue_[i] = i;
>>  
>>  	for (unsigned int i = 0; i < kMaxLineBuffers; i++)
>>  		lineBuffers_[i] = nullptr;
>> @@ -698,37 +697,9 @@ void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams
>>  		clock_gettime(CLOCK_MONOTONIC_RAW, &frameStartTime);
>>  	}
>>  
>> -	/* Apply DebayerParams */
>> -	if (params.gamma != gammaCorrection_ || params.blackLevel != blackLevel_) {
>> -		const unsigned int blackIndex =
>> -			params.blackLevel * kGammaLookupSize / 256;
>> -		std::fill(gamma_.begin(), gamma_.begin() + blackIndex, 0);
>> -		const float divisor = kGammaLookupSize - blackIndex - 1.0;
>> -		for (unsigned int i = blackIndex; i < kGammaLookupSize; i++)
>> -			gamma_[i] = UINT8_MAX * powf((i - blackIndex) / divisor, params.gamma);
>> -
>> -		gammaCorrection_ = params.gamma;
>> -		blackLevel_ = params.blackLevel;
>> -	}
>> -
>> -	if (swapRedBlueGains_)
>> -		std::swap(params.gainR, params.gainB);
>> -
>> -	for (unsigned int i = 0; i < kRGBLookupSize; i++) {
>> -		constexpr unsigned int div =
>> -			kRGBLookupSize * DebayerParams::kGain10 / kGammaLookupSize;
>> -		unsigned int idx;
>> -
>> -		/* Apply gamma after gain! */
>> -		idx = std::min({ i * params.gainR / div, (kGammaLookupSize - 1) });
>> -		red_[i] = gamma_[idx];
>> -
>> -		idx = std::min({ i * params.gainG / div, (kGammaLookupSize - 1) });
>> -		green_[i] = gamma_[idx];
>> -
>> -		idx = std::min({ i * params.gainB / div, (kGammaLookupSize - 1) });
>> -		blue_[i] = gamma_[idx];
>> -	}
>> +	green_ = params.green;
>> +	red_ = swapRedBlueGains_ ? params.blue : params.red;
>> +	blue_ = swapRedBlueGains_ ? params.red : params.blue;
>>  
>>  	/* Copy metadata from the input buffer */
>>  	FrameMetadata &metadata = output->_d()->metadata();
>> diff --git a/src/libcamera/software_isp/debayer_cpu.h b/src/libcamera/software_isp/debayer_cpu.h
>> index de216fe3..be7dcdca 100644
>> --- a/src/libcamera/software_isp/debayer_cpu.h
>> +++ b/src/libcamera/software_isp/debayer_cpu.h
>> @@ -122,15 +122,12 @@ private:
>>  	void process2(const uint8_t *src, uint8_t *dst);
>>  	void process4(const uint8_t *src, uint8_t *dst);
>>  
>> -	static constexpr unsigned int kGammaLookupSize = 1024;
>> -	static constexpr unsigned int kRGBLookupSize = 256;
>>  	/* Max. supported Bayer pattern height is 4, debayering this requires 5 lines */
>>  	static constexpr unsigned int kMaxLineBuffers = 5;
>>  
>> -	std::array<uint8_t, kGammaLookupSize> gamma_;
>> -	std::array<uint8_t, kRGBLookupSize> red_;
>> -	std::array<uint8_t, kRGBLookupSize> green_;
>> -	std::array<uint8_t, kRGBLookupSize> blue_;
>> +	DebayerParams::ColorLookupTable red_;
>> +	DebayerParams::ColorLookupTable green_;
>> +	DebayerParams::ColorLookupTable blue_;
>>  	debayerFn debayer0_;
>>  	debayerFn debayer1_;
>>  	debayerFn debayer2_;
>> @@ -146,8 +143,6 @@ private:
>>  	unsigned int xShift_; /* Offset of 0/1 applied to window_.x */
>>  	bool enableInputMemcpy_;
>>  	bool swapRedBlueGains_;
>> -	float gammaCorrection_;
>> -	unsigned int blackLevel_;
>>  	unsigned int measuredFrames_;
>>  	int64_t frameProcessTime_;
>>  	/* Skip 30 frames for things to stabilize then measure 30 frames */
>> diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp
>> index c9b6be56..ccd703bb 100644
>> --- a/src/libcamera/software_isp/software_isp.cpp
>> +++ b/src/libcamera/software_isp/software_isp.cpp
>> @@ -7,6 +7,8 @@
>>  
>>  #include "libcamera/internal/software_isp/software_isp.h"
>>  
>> +#include <cmath>
>> +#include <stdint.h>
>>  #include <sys/mman.h>
>>  #include <sys/types.h>
>>  #include <unistd.h>
>> @@ -18,6 +20,7 @@
>>  #include "libcamera/internal/framebuffer.h"
>>  #include "libcamera/internal/ipa_manager.h"
>>  #include "libcamera/internal/mapped_framebuffer.h"
>> +#include "libcamera/internal/software_isp/debayer_params.h"
>>  
>>  #include "debayer_cpu.h"
>>  
>> @@ -63,10 +66,24 @@ LOG_DEFINE_CATEGORY(SoftwareIsp)
>>   * handler
>>   */
>>  SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor)
>> -	: debayerParams_{ DebayerParams::kGain10, DebayerParams::kGain10,
>> -			  DebayerParams::kGain10, 0.5f, 0 },
>> -	  dmaHeap_(DmaHeap::DmaHeapFlag::Cma | DmaHeap::DmaHeapFlag::System)
>> +	: dmaHeap_(DmaHeap::DmaHeapFlag::Cma | DmaHeap::DmaHeapFlag::System)
>>  {
>> +	/*
>> +	 * debayerParams_ must be initialized because the initial value is used for
>> +	 * the first two frames, i.e. until stats processing starts providing its
>> +	 * own parameters.
>> +	 *
>> +	 * \todo This should be handled in the same place as the related operations.
>
> I would write
>
> 	 * \todo This should be handled in the same place as the related
> 	 * operations, in the IPA module.

Yes, OK.

> No need to resubmit just for this, I can update the comment when
> applying.

Thank you.

> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
>> +	 */
>> +	std::array<uint8_t, 256> gammaTable;
>> +	for (unsigned int i = 0; i < 256; i++)
>> +		gammaTable[i] = UINT8_MAX * std::pow(i / 256.0, 0.5);
>> +	for (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++) {
>> +		debayerParams_.red[i] = gammaTable[i];
>> +		debayerParams_.green[i] = gammaTable[i];
>> +		debayerParams_.blue[i] = gammaTable[i];
>> +	}
>> +
>>  	if (!dmaHeap_.isValid()) {
>>  		LOG(SoftwareIsp, Error) << "Failed to create DmaHeap object";
>>  		return;



More information about the libcamera-devel mailing list