[libcamera-devel,4/4] ipa: raspberrypi: Tidy up variable names to be consistent

Message ID 20200922095018.68434-5-naush@raspberrypi.com
State Accepted
Headers show
Series
  • Code tidy-ups
Related show

Commit Message

Naushir Patuck Sept. 22, 2020, 9:50 a.m. UTC
Change variable names to camel case to be consistent with the rest of
the source files. Remove #define consts and replace with constexpr.

There are no functional changes in this commit.

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
---
 include/libcamera/ipa/raspberrypi.h           |   2 +-
 src/ipa/raspberrypi/raspberrypi.cpp           | 178 +++++++++---------
 .../pipeline/raspberrypi/raspberrypi.cpp      |   2 +-
 3 files changed, 91 insertions(+), 91 deletions(-)

Comments

Jacopo Mondi Sept. 23, 2020, 8 a.m. UTC | #1
Hi Naush,

On Tue, Sep 22, 2020 at 10:50:18AM +0100, Naushir Patuck wrote:
> Change variable names to camel case to be consistent with the rest of
> the source files. Remove #define consts and replace with constexpr.

Very much apreciated! I haven't looked for leftovers, but
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
  j

>
> There are no functional changes in this commit.
>
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> ---
>  include/libcamera/ipa/raspberrypi.h           |   2 +-
>  src/ipa/raspberrypi/raspberrypi.cpp           | 178 +++++++++---------
>  .../pipeline/raspberrypi/raspberrypi.cpp      |   2 +-
>  3 files changed, 91 insertions(+), 91 deletions(-)
>
> diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h
> index c9d4aa81..b3041591 100644
> --- a/include/libcamera/ipa/raspberrypi.h
> +++ b/include/libcamera/ipa/raspberrypi.h
> @@ -41,7 +41,7 @@ enum BufferMask {
>  };
>
>  /* Size of the LS grid allocation. */
> -#define MAX_LS_GRID_SIZE (32 << 10)
> +static constexpr unsigned int MaxLsGridSize = 32 << 10;
>
>  /* List of controls handled by the Raspberry Pi IPA */
>  static const ControlInfoMap Controls = {
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index 0c0dc743..c240eae8 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -55,8 +55,8 @@
>  namespace libcamera {
>
>  /* Configure the sensor with these values initially. */
> -#define DEFAULT_ANALOGUE_GAIN 1.0
> -#define DEFAULT_EXPOSURE_TIME 20000
> +constexpr unsigned int DefaultAnalogueGain = 1.0;
> +constexpr unsigned int DefaultExposureTime = 20000;
>
>  LOG_DEFINE_CATEGORY(IPARPI)
>
> @@ -65,7 +65,7 @@ class IPARPi : public IPAInterface
>  public:
>  	IPARPi()
>  		: lastMode_({}), controller_(), controllerInit_(false),
> -		  frame_count_(0), check_count_(0), mistrust_count_(0),
> +		  frameCount_(0), checkCount_(0), mistrustCount_(0),
>  		  lsTable_(nullptr)
>  	{
>  	}
> @@ -73,7 +73,7 @@ public:
>  	~IPARPi()
>  	{
>  		if (lsTable_)
> -			munmap(lsTable_, MAX_LS_GRID_SIZE);
> +			munmap(lsTable_, RPi::MaxLsGridSize);
>  	}
>
>  	int init(const IPASettings &settings) override;
> @@ -108,13 +108,13 @@ private:
>  	void applySharpen(const struct SharpenStatus *sharpenStatus, ControlList &ctrls);
>  	void applyDPC(const struct DpcStatus *dpcStatus, ControlList &ctrls);
>  	void applyLS(const struct AlscStatus *lsStatus, ControlList &ctrls);
> -	void resampleTable(uint16_t dest[], double const src[12][16], int dest_w, int dest_h);
> +	void resampleTable(uint16_t dest[], double const src[12][16], int destW, int destH);
>
>  	std::map<unsigned int, FrameBuffer> buffers_;
>  	std::map<unsigned int, void *> buffersMemory_;
>
> -	ControlInfoMap unicam_ctrls_;
> -	ControlInfoMap isp_ctrls_;
> +	ControlInfoMap unicamCtrls_;
> +	ControlInfoMap ispCtrls_;
>  	ControlList libcameraMetadata_;
>
>  	/* IPA configuration. */
> @@ -134,11 +134,11 @@ private:
>  	 * We count frames to decide if the frame must be hidden (e.g. from
>  	 * display) or mistrusted (i.e. not given to the control algos).
>  	 */
> -	uint64_t frame_count_;
> +	uint64_t frameCount_;
>  	/* For checking the sequencing of Prepare/Process calls. */
> -	uint64_t check_count_;
> +	uint64_t checkCount_;
>  	/* How many frames we should avoid running control algos on. */
> -	unsigned int mistrust_count_;
> +	unsigned int mistrustCount_;
>  	/* LS table allocation passed in from the pipeline handler. */
>  	FileDescriptor lsTableHandle_;
>  	void *lsTable_;
> @@ -199,8 +199,8 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
>
>  	result->operation = 0;
>
> -	unicam_ctrls_ = entityControls.at(0);
> -	isp_ctrls_ = entityControls.at(1);
> +	unicamCtrls_ = entityControls.at(0);
> +	ispCtrls_ = entityControls.at(1);
>  	/* Setup a metadata ControlList to output metadata. */
>  	libcameraMetadata_ = ControlList(controls::controls);
>
> @@ -238,18 +238,18 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
>  	 *"mistrusted", which depends on whether this is a startup from cold,
>  	 * or merely a mode switch in a running system.
>  	 */
> -	frame_count_ = 0;
> -	check_count_ = 0;
> -	unsigned int drop_frame = 0;
> +	frameCount_ = 0;
> +	checkCount_ = 0;
> +	unsigned int dropFrame = 0;
>  	if (controllerInit_) {
> -		drop_frame = helper_->HideFramesModeSwitch();
> -		mistrust_count_ = helper_->MistrustFramesModeSwitch();
> +		dropFrame = helper_->HideFramesModeSwitch();
> +		mistrustCount_ = helper_->MistrustFramesModeSwitch();
>  	} else {
> -		drop_frame = helper_->HideFramesStartup();
> -		mistrust_count_ = helper_->MistrustFramesStartup();
> +		dropFrame = helper_->HideFramesStartup();
> +		mistrustCount_ = helper_->MistrustFramesStartup();
>  	}
>
> -	result->data.push_back(drop_frame);
> +	result->data.push_back(dropFrame);
>  	result->operation |= RPi::IPA_CONFIG_DROP_FRAMES;
>
>  	struct AgcStatus agcStatus;
> @@ -264,8 +264,8 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
>  		controllerInit_ = true;
>
>  		/* Supply initial values for gain and exposure. */
> -		agcStatus.shutter_time = DEFAULT_EXPOSURE_TIME;
> -		agcStatus.analogue_gain = DEFAULT_ANALOGUE_GAIN;
> +		agcStatus.shutter_time = DefaultExposureTime;
> +		agcStatus.analogue_gain = DefaultAnalogueGain;
>  	}
>
>  	RPiController::Metadata metadata;
> @@ -274,7 +274,7 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
>  	/* SwitchMode may supply updated exposure/gain values to use. */
>  	metadata.Get("agc.status", agcStatus);
>  	if (agcStatus.shutter_time != 0.0 && agcStatus.analogue_gain != 0.0) {
> -		ControlList ctrls(unicam_ctrls_);
> +		ControlList ctrls(unicamCtrls_);
>  		applyAGC(&agcStatus, ctrls);
>  		result->controls.push_back(ctrls);
>
> @@ -287,14 +287,14 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
>  	if (ipaConfig.operation & RPi::IPA_CONFIG_LS_TABLE) {
>  		/* Remove any previous table, if there was one. */
>  		if (lsTable_) {
> -			munmap(lsTable_, MAX_LS_GRID_SIZE);
> +			munmap(lsTable_, RPi::MaxLsGridSize);
>  			lsTable_ = nullptr;
>  		}
>
>  		/* Map the LS table buffer into user space. */
>  		lsTableHandle_ = FileDescriptor(ipaConfig.data[0]);
>  		if (lsTableHandle_.isValid()) {
> -			lsTable_ = mmap(nullptr, MAX_LS_GRID_SIZE, PROT_READ | PROT_WRITE,
> +			lsTable_ = mmap(nullptr, RPi::MaxLsGridSize, PROT_READ | PROT_WRITE,
>  					MAP_SHARED, lsTableHandle_.fd(), 0);
>
>  			if (lsTable_ == MAP_FAILED) {
> @@ -343,9 +343,9 @@ void IPARPi::processEvent(const IPAOperationData &event)
>  	case RPi::IPA_EVENT_SIGNAL_STAT_READY: {
>  		unsigned int bufferId = event.data[0];
>
> -		if (++check_count_ != frame_count_) /* assert here? */
> +		if (++checkCount_ != frameCount_) /* assert here? */
>  			LOG(IPARPI, Error) << "WARNING: Prepare/Process mismatch!!!";
> -		if (frame_count_ > mistrust_count_)
> +		if (frameCount_ > mistrustCount_)
>  			processStats(bufferId);
>
>  		reportMetadata();
> @@ -368,7 +368,7 @@ void IPARPi::processEvent(const IPAOperationData &event)
>  		 * they are "unreliable".
>  		 */
>  		prepareISP(embeddedbufferId);
> -		frame_count_++;
> +		frameCount_++;
>
>  		/* Ready to push the input buffer into the ISP. */
>  		IPAOperationData op;
> @@ -713,7 +713,7 @@ void IPARPi::prepareISP(unsigned int bufferId)
>  	returnEmbeddedBuffer(bufferId);
>
>  	if (success) {
> -		ControlList ctrls(isp_ctrls_);
> +		ControlList ctrls(ispCtrls_);
>
>  		rpiMetadata_.Clear();
>  		rpiMetadata_.Set("device.status", deviceStatus);
> @@ -785,19 +785,19 @@ bool IPARPi::parseEmbeddedData(unsigned int bufferId, struct DeviceStatus &devic
>  	if (status != RPiController::MdParser::Status::OK) {
>  		LOG(IPARPI, Error) << "Embedded Buffer parsing failed, error " << status;
>  	} else {
> -		uint32_t exposure_lines, gain_code;
> -		if (helper_->Parser().GetExposureLines(exposure_lines) != RPiController::MdParser::Status::OK) {
> +		uint32_t exposureLines, gainCode;
> +		if (helper_->Parser().GetExposureLines(exposureLines) != RPiController::MdParser::Status::OK) {
>  			LOG(IPARPI, Error) << "Exposure time failed";
>  			return false;
>  		}
>
> -		deviceStatus.shutter_speed = helper_->Exposure(exposure_lines);
> -		if (helper_->Parser().GetGainCode(gain_code) != RPiController::MdParser::Status::OK) {
> +		deviceStatus.shutter_speed = helper_->Exposure(exposureLines);
> +		if (helper_->Parser().GetGainCode(gainCode) != RPiController::MdParser::Status::OK) {
>  			LOG(IPARPI, Error) << "Gain failed";
>  			return false;
>  		}
>
> -		deviceStatus.analogue_gain = helper_->Gain(gain_code);
> +		deviceStatus.analogue_gain = helper_->Gain(gainCode);
>  		LOG(IPARPI, Debug) << "Metadata - Exposure : "
>  				   << deviceStatus.shutter_speed << " Gain : "
>  				   << deviceStatus.analogue_gain;
> @@ -820,7 +820,7 @@ void IPARPi::processStats(unsigned int bufferId)
>
>  	struct AgcStatus agcStatus;
>  	if (rpiMetadata_.Get("agc.status", agcStatus) == 0) {
> -		ControlList ctrls(unicam_ctrls_);
> +		ControlList ctrls(unicamCtrls_);
>  		applyAGC(&agcStatus, ctrls);
>
>  		IPAOperationData op;
> @@ -832,14 +832,14 @@ void IPARPi::processStats(unsigned int bufferId)
>
>  void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls)
>  {
> -	const auto gainR = isp_ctrls_.find(V4L2_CID_RED_BALANCE);
> -	if (gainR == isp_ctrls_.end()) {
> +	const auto gainR = ispCtrls_.find(V4L2_CID_RED_BALANCE);
> +	if (gainR == ispCtrls_.end()) {
>  		LOG(IPARPI, Error) << "Can't find red gain control";
>  		return;
>  	}
>
> -	const auto gainB = isp_ctrls_.find(V4L2_CID_BLUE_BALANCE);
> -	if (gainB == isp_ctrls_.end()) {
> +	const auto gainB = ispCtrls_.find(V4L2_CID_BLUE_BALANCE);
> +	if (gainB == ispCtrls_.end()) {
>  		LOG(IPARPI, Error) << "Can't find blue gain control";
>  		return;
>  	}
> @@ -855,31 +855,31 @@ void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls)
>
>  void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls)
>  {
> -	int32_t gain_code = helper_->GainCode(agcStatus->analogue_gain);
> -	int32_t exposure_lines = helper_->ExposureLines(agcStatus->shutter_time);
> +	int32_t gainCode = helper_->GainCode(agcStatus->analogue_gain);
> +	int32_t exposureLines = helper_->ExposureLines(agcStatus->shutter_time);
>
> -	if (unicam_ctrls_.find(V4L2_CID_ANALOGUE_GAIN) == unicam_ctrls_.end()) {
> +	if (unicamCtrls_.find(V4L2_CID_ANALOGUE_GAIN) == unicamCtrls_.end()) {
>  		LOG(IPARPI, Error) << "Can't find analogue gain control";
>  		return;
>  	}
>
> -	if (unicam_ctrls_.find(V4L2_CID_EXPOSURE) == unicam_ctrls_.end()) {
> +	if (unicamCtrls_.find(V4L2_CID_EXPOSURE) == unicamCtrls_.end()) {
>  		LOG(IPARPI, Error) << "Can't find exposure control";
>  		return;
>  	}
>
>  	LOG(IPARPI, Debug) << "Applying AGC Exposure: " << agcStatus->shutter_time
> -			   << " (Shutter lines: " << exposure_lines << ") Gain: "
> +			   << " (Shutter lines: " << exposureLines << ") Gain: "
>  			   << agcStatus->analogue_gain << " (Gain Code: "
> -			   << gain_code << ")";
> +			   << gainCode << ")";
>
> -	ctrls.set(V4L2_CID_ANALOGUE_GAIN, gain_code);
> -	ctrls.set(V4L2_CID_EXPOSURE, exposure_lines);
> +	ctrls.set(V4L2_CID_ANALOGUE_GAIN, gainCode);
> +	ctrls.set(V4L2_CID_EXPOSURE, exposureLines);
>  }
>
>  void IPARPi::applyDG(const struct AgcStatus *dgStatus, ControlList &ctrls)
>  {
> -	if (isp_ctrls_.find(V4L2_CID_DIGITAL_GAIN) == isp_ctrls_.end()) {
> +	if (ispCtrls_.find(V4L2_CID_DIGITAL_GAIN) == ispCtrls_.end()) {
>  		LOG(IPARPI, Error) << "Can't find digital gain control";
>  		return;
>  	}
> @@ -890,7 +890,7 @@ void IPARPi::applyDG(const struct AgcStatus *dgStatus, ControlList &ctrls)
>
>  void IPARPi::applyCCM(const struct CcmStatus *ccmStatus, ControlList &ctrls)
>  {
> -	if (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_CC_MATRIX) == isp_ctrls_.end()) {
> +	if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_CC_MATRIX) == ispCtrls_.end()) {
>  		LOG(IPARPI, Error) << "Can't find CCM control";
>  		return;
>  	}
> @@ -911,7 +911,7 @@ void IPARPi::applyCCM(const struct CcmStatus *ccmStatus, ControlList &ctrls)
>
>  void IPARPi::applyGamma(const struct ContrastStatus *contrastStatus, ControlList &ctrls)
>  {
> -	if (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_GAMMA) == isp_ctrls_.end()) {
> +	if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_GAMMA) == ispCtrls_.end()) {
>  		LOG(IPARPI, Error) << "Can't find Gamma control";
>  		return;
>  	}
> @@ -930,7 +930,7 @@ void IPARPi::applyGamma(const struct ContrastStatus *contrastStatus, ControlList
>
>  void IPARPi::applyBlackLevel(const struct BlackLevelStatus *blackLevelStatus, ControlList &ctrls)
>  {
> -	if (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_BLACK_LEVEL) == isp_ctrls_.end()) {
> +	if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_BLACK_LEVEL) == ispCtrls_.end()) {
>  		LOG(IPARPI, Error) << "Can't find black level control";
>  		return;
>  	}
> @@ -948,7 +948,7 @@ void IPARPi::applyBlackLevel(const struct BlackLevelStatus *blackLevelStatus, Co
>
>  void IPARPi::applyGEQ(const struct GeqStatus *geqStatus, ControlList &ctrls)
>  {
> -	if (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_GEQ) == isp_ctrls_.end()) {
> +	if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_GEQ) == ispCtrls_.end()) {
>  		LOG(IPARPI, Error) << "Can't find geq control";
>  		return;
>  	}
> @@ -966,7 +966,7 @@ void IPARPi::applyGEQ(const struct GeqStatus *geqStatus, ControlList &ctrls)
>
>  void IPARPi::applyDenoise(const struct SdnStatus *denoiseStatus, ControlList &ctrls)
>  {
> -	if (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_DENOISE) == isp_ctrls_.end()) {
> +	if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_DENOISE) == ispCtrls_.end()) {
>  		LOG(IPARPI, Error) << "Can't find denoise control";
>  		return;
>  	}
> @@ -986,7 +986,7 @@ void IPARPi::applyDenoise(const struct SdnStatus *denoiseStatus, ControlList &ct
>
>  void IPARPi::applySharpen(const struct SharpenStatus *sharpenStatus, ControlList &ctrls)
>  {
> -	if (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_SHARPEN) == isp_ctrls_.end()) {
> +	if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_SHARPEN) == ispCtrls_.end()) {
>  		LOG(IPARPI, Error) << "Can't find sharpen control";
>  		return;
>  	}
> @@ -1007,7 +1007,7 @@ void IPARPi::applySharpen(const struct SharpenStatus *sharpenStatus, ControlList
>
>  void IPARPi::applyDPC(const struct DpcStatus *dpcStatus, ControlList &ctrls)
>  {
> -	if (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_DPC) == isp_ctrls_.end()) {
> +	if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_DPC) == ispCtrls_.end()) {
>  		LOG(IPARPI, Error) << "Can't find DPC control";
>  		return;
>  	}
> @@ -1023,7 +1023,7 @@ void IPARPi::applyDPC(const struct DpcStatus *dpcStatus, ControlList &ctrls)
>
>  void IPARPi::applyLS(const struct AlscStatus *lsStatus, ControlList &ctrls)
>  {
> -	if (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING) == isp_ctrls_.end()) {
> +	if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING) == ispCtrls_.end()) {
>  		LOG(IPARPI, Error) << "Can't find LS control";
>  		return;
>  	}
> @@ -1032,18 +1032,18 @@ void IPARPi::applyLS(const struct AlscStatus *lsStatus, ControlList &ctrls)
>  	 * Program lens shading tables into pipeline.
>  	 * Choose smallest cell size that won't exceed 63x48 cells.
>  	 */
> -	const int cell_sizes[] = { 16, 32, 64, 128, 256 };
> -	unsigned int num_cells = ARRAY_SIZE(cell_sizes);
> -	unsigned int i, w, h, cell_size;
> -	for (i = 0; i < num_cells; i++) {
> -		cell_size = cell_sizes[i];
> -		w = (mode_.width + cell_size - 1) / cell_size;
> -		h = (mode_.height + cell_size - 1) / cell_size;
> +	const int cellSizes[] = { 16, 32, 64, 128, 256 };
> +	unsigned int numCells = ARRAY_SIZE(cellSizes);
> +	unsigned int i, w, h, cellSize;
> +	for (i = 0; i < numCells; i++) {
> +		cellSize = cellSizes[i];
> +		w = (mode_.width + cellSize - 1) / cellSize;
> +		h = (mode_.height + cellSize - 1) / cellSize;
>  		if (w < 64 && h <= 48)
>  			break;
>  	}
>
> -	if (i == num_cells) {
> +	if (i == numCells) {
>  		LOG(IPARPI, Error) << "Cannot find cell size";
>  		return;
>  	}
> @@ -1052,7 +1052,7 @@ void IPARPi::applyLS(const struct AlscStatus *lsStatus, ControlList &ctrls)
>  	w++, h++;
>  	bcm2835_isp_lens_shading ls = {
>  		.enabled = 1,
> -		.grid_cell_size = cell_size,
> +		.grid_cell_size = cellSize,
>  		.grid_width = w,
>  		.grid_stride = w,
>  		.grid_height = h,
> @@ -1062,7 +1062,7 @@ void IPARPi::applyLS(const struct AlscStatus *lsStatus, ControlList &ctrls)
>  		.gain_format = GAIN_FORMAT_U4P10
>  	};
>
> -	if (!lsTable_ || w * h * 4 * sizeof(uint16_t) > MAX_LS_GRID_SIZE) {
> +	if (!lsTable_ || w * h * 4 * sizeof(uint16_t) > RPi::MaxLsGridSize) {
>  		LOG(IPARPI, Error) << "Do not have a correctly allocate lens shading table!";
>  		return;
>  	}
> @@ -1083,41 +1083,41 @@ void IPARPi::applyLS(const struct AlscStatus *lsStatus, ControlList &ctrls)
>  }
>
>  /*
> - * Resamples a 16x12 table with central sampling to dest_w x dest_h with corner
> + * Resamples a 16x12 table with central sampling to destW x destH with corner
>   * sampling.
>   */
>  void IPARPi::resampleTable(uint16_t dest[], double const src[12][16],
> -			   int dest_w, int dest_h)
> +			   int destW, int destH)
>  {
>  	/*
>  	 * Precalculate and cache the x sampling locations and phases to
>  	 * save recomputing them on every row.
>  	 */
> -	assert(dest_w > 1 && dest_h > 1 && dest_w <= 64);
> -	int x_lo[64], x_hi[64];
> +	assert(destW > 1 && destH > 1 && destW <= 64);
> +	int xLo[64], xHi[64];
>  	double xf[64];
> -	double x = -0.5, x_inc = 16.0 / (dest_w - 1);
> -	for (int i = 0; i < dest_w; i++, x += x_inc) {
> -		x_lo[i] = floor(x);
> -		xf[i] = x - x_lo[i];
> -		x_hi[i] = x_lo[i] < 15 ? x_lo[i] + 1 : 15;
> -		x_lo[i] = x_lo[i] > 0 ? x_lo[i] : 0;
> +	double x = -0.5, xInc = 16.0 / (destW - 1);
> +	for (int i = 0; i < destW; i++, x += xInc) {
> +		xLo[i] = floor(x);
> +		xf[i] = x - xLo[i];
> +		xHi[i] = xLo[i] < 15 ? xLo[i] + 1 : 15;
> +		xLo[i] = xLo[i] > 0 ? xLo[i] : 0;
>  	}
>
>  	/* Now march over the output table generating the new values. */
> -	double y = -0.5, y_inc = 12.0 / (dest_h - 1);
> -	for (int j = 0; j < dest_h; j++, y += y_inc) {
> -		int y_lo = floor(y);
> -		double yf = y - y_lo;
> -		int y_hi = y_lo < 11 ? y_lo + 1 : 11;
> -		y_lo = y_lo > 0 ? y_lo : 0;
> -		double const *row_above = src[y_lo];
> -		double const *row_below = src[y_hi];
> -		for (int i = 0; i < dest_w; i++) {
> -			double above = row_above[x_lo[i]] * (1 - xf[i])
> -				     + row_above[x_hi[i]] * xf[i];
> -			double below = row_below[x_lo[i]] * (1 - xf[i])
> -				     + row_below[x_hi[i]] * xf[i];
> +	double y = -0.5, yInc = 12.0 / (destH - 1);
> +	for (int j = 0; j < destH; j++, y += yInc) {
> +		int yLo = floor(y);
> +		double yf = y - yLo;
> +		int yHi = yLo < 11 ? yLo + 1 : 11;
> +		yLo = yLo > 0 ? yLo : 0;
> +		double const *rowAbove = src[yLo];
> +		double const *rowBelow = src[yHi];
> +		for (int i = 0; i < destW; i++) {
> +			double above = rowAbove[xLo[i]] * (1 - xf[i])
> +				     + rowAbove[xHi[i]] * xf[i];
> +			double below = rowBelow[xLo[i]] * (1 - xf[i])
> +				     + rowBelow[xHi[i]] * xf[i];
>  			int result = floor(1024 * (above * (1 - yf) + below * yf) + .5);
>  			*(dest++) = result > 16383 ? 16383 : result; /* want u4.10 */
>  		}
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 35dbe0fb..8d40b0ed 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -1012,7 +1012,7 @@ int RPiCameraData::configureIPA()
>
>  	/* Allocate the lens shading table via dmaHeap and pass to the IPA. */
>  	if (!lsTable_.isValid()) {
> -		lsTable_ = dmaHeap_.alloc("ls_grid", MAX_LS_GRID_SIZE);
> +		lsTable_ = dmaHeap_.alloc("ls_grid", RPi::MaxLsGridSize);
>  		if (!lsTable_.isValid())
>  			return -ENOMEM;
>
> --
> 2.25.1
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Kieran Bingham Sept. 23, 2020, 11:18 a.m. UTC | #2
Hi Naush,

On 22/09/2020 10:50, Naushir Patuck wrote:
> Change variable names to camel case to be consistent with the rest of
> the source files. Remove #define consts and replace with constexpr.
> 

Sounds good to me...


> There are no functional changes in this commit.
> 
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> ---
>  include/libcamera/ipa/raspberrypi.h           |   2 +-
>  src/ipa/raspberrypi/raspberrypi.cpp           | 178 +++++++++---------
>  .../pipeline/raspberrypi/raspberrypi.cpp      |   2 +-
>  3 files changed, 91 insertions(+), 91 deletions(-)
> 
> diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h
> index c9d4aa81..b3041591 100644
> --- a/include/libcamera/ipa/raspberrypi.h
> +++ b/include/libcamera/ipa/raspberrypi.h
> @@ -41,7 +41,7 @@ enum BufferMask {
>  };
>  
>  /* Size of the LS grid allocation. */
> -#define MAX_LS_GRID_SIZE (32 << 10)
> +static constexpr unsigned int MaxLsGridSize = 32 << 10;

I guess the LS could stay capitalised (MaxLSGridSize), not sure that it
matters, so up to you.

>  /* List of controls handled by the Raspberry Pi IPA */
>  static const ControlInfoMap Controls = {
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index 0c0dc743..c240eae8 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -55,8 +55,8 @@
>  namespace libcamera {
>  
>  /* Configure the sensor with these values initially. */
> -#define DEFAULT_ANALOGUE_GAIN 1.0
> -#define DEFAULT_EXPOSURE_TIME 20000
> +constexpr unsigned int DefaultAnalogueGain = 1.0;
> +constexpr unsigned int DefaultExposureTime = 20000;

Oh nice, This really highlights to me the benefit of constexpr.
Now the values have types!

>  
>  LOG_DEFINE_CATEGORY(IPARPI)
>  
> @@ -65,7 +65,7 @@ class IPARPi : public IPAInterface
>  public:
>  	IPARPi()
>  		: lastMode_({}), controller_(), controllerInit_(false),
> -		  frame_count_(0), check_count_(0), mistrust_count_(0),
> +		  frameCount_(0), checkCount_(0), mistrustCount_(0),
>  		  lsTable_(nullptr)
>  	{
>  	}
> @@ -73,7 +73,7 @@ public:
>  	~IPARPi()
>  	{
>  		if (lsTable_)
> -			munmap(lsTable_, MAX_LS_GRID_SIZE);
> +			munmap(lsTable_, RPi::MaxLsGridSize);
>  	}
>  
>  	int init(const IPASettings &settings) override;
> @@ -108,13 +108,13 @@ private:
>  	void applySharpen(const struct SharpenStatus *sharpenStatus, ControlList &ctrls);
>  	void applyDPC(const struct DpcStatus *dpcStatus, ControlList &ctrls);
>  	void applyLS(const struct AlscStatus *lsStatus, ControlList &ctrls);
> -	void resampleTable(uint16_t dest[], double const src[12][16], int dest_w, int dest_h);
> +	void resampleTable(uint16_t dest[], double const src[12][16], int destW, int destH);
>  
>  	std::map<unsigned int, FrameBuffer> buffers_;
>  	std::map<unsigned int, void *> buffersMemory_;
>  
> -	ControlInfoMap unicam_ctrls_;
> -	ControlInfoMap isp_ctrls_;
> +	ControlInfoMap unicamCtrls_;
> +	ControlInfoMap ispCtrls_;
>  	ControlList libcameraMetadata_;
>  
>  	/* IPA configuration. */
> @@ -134,11 +134,11 @@ private:
>  	 * We count frames to decide if the frame must be hidden (e.g. from
>  	 * display) or mistrusted (i.e. not given to the control algos).
>  	 */
> -	uint64_t frame_count_;
> +	uint64_t frameCount_;

If we're doing clean up - I always prefer to see a newline before a
comment line.

To me it makes the commented block clearer - but I don't know if there's
a 'rule' on it.


>  	/* For checking the sequencing of Prepare/Process calls. */
> -	uint64_t check_count_;
> +	uint64_t checkCount_;

i.e. here too.

>  	/* How many frames we should avoid running control algos on. */
> -	unsigned int mistrust_count_;
> +	unsigned int mistrustCount_;

and so on.

>  	/* LS table allocation passed in from the pipeline handler. */
>  	FileDescriptor lsTableHandle_;
>  	void *lsTable_;
> @@ -199,8 +199,8 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
>  
>  	result->operation = 0;
>  
> -	unicam_ctrls_ = entityControls.at(0);
> -	isp_ctrls_ = entityControls.at(1);
> +	unicamCtrls_ = entityControls.at(0);
> +	ispCtrls_ = entityControls.at(1);
>  	/* Setup a metadata ControlList to output metadata. */
>  	libcameraMetadata_ = ControlList(controls::controls);
>  
> @@ -238,18 +238,18 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
>  	 *"mistrusted", which depends on whether this is a startup from cold,
>  	 * or merely a mode switch in a running system.
>  	 */
> -	frame_count_ = 0;
> -	check_count_ = 0;
> -	unsigned int drop_frame = 0;
> +	frameCount_ = 0;
> +	checkCount_ = 0;
> +	unsigned int dropFrame = 0;
>  	if (controllerInit_) {
> -		drop_frame = helper_->HideFramesModeSwitch();
> -		mistrust_count_ = helper_->MistrustFramesModeSwitch();
> +		dropFrame = helper_->HideFramesModeSwitch();
> +		mistrustCount_ = helper_->MistrustFramesModeSwitch();
>  	} else {
> -		drop_frame = helper_->HideFramesStartup();
> -		mistrust_count_ = helper_->MistrustFramesStartup();
> +		dropFrame = helper_->HideFramesStartup();
> +		mistrustCount_ = helper_->MistrustFramesStartup();
>  	}
>  
> -	result->data.push_back(drop_frame);
> +	result->data.push_back(dropFrame);
>  	result->operation |= RPi::IPA_CONFIG_DROP_FRAMES;
>  
>  	struct AgcStatus agcStatus;
> @@ -264,8 +264,8 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
>  		controllerInit_ = true;
>  
>  		/* Supply initial values for gain and exposure. */
> -		agcStatus.shutter_time = DEFAULT_EXPOSURE_TIME;
> -		agcStatus.analogue_gain = DEFAULT_ANALOGUE_GAIN;
> +		agcStatus.shutter_time = DefaultExposureTime;
> +		agcStatus.analogue_gain = DefaultAnalogueGain;
>  	}
>  
>  	RPiController::Metadata metadata;
> @@ -274,7 +274,7 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
>  	/* SwitchMode may supply updated exposure/gain values to use. */
>  	metadata.Get("agc.status", agcStatus);
>  	if (agcStatus.shutter_time != 0.0 && agcStatus.analogue_gain != 0.0) {
> -		ControlList ctrls(unicam_ctrls_);
> +		ControlList ctrls(unicamCtrls_);
>  		applyAGC(&agcStatus, ctrls);
>  		result->controls.push_back(ctrls);
>  
> @@ -287,14 +287,14 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
>  	if (ipaConfig.operation & RPi::IPA_CONFIG_LS_TABLE) {
>  		/* Remove any previous table, if there was one. */
>  		if (lsTable_) {
> -			munmap(lsTable_, MAX_LS_GRID_SIZE);
> +			munmap(lsTable_, RPi::MaxLsGridSize);
>  			lsTable_ = nullptr;
>  		}
>  
>  		/* Map the LS table buffer into user space. */
>  		lsTableHandle_ = FileDescriptor(ipaConfig.data[0]);
>  		if (lsTableHandle_.isValid()) {
> -			lsTable_ = mmap(nullptr, MAX_LS_GRID_SIZE, PROT_READ | PROT_WRITE,
> +			lsTable_ = mmap(nullptr, RPi::MaxLsGridSize, PROT_READ | PROT_WRITE,
>  					MAP_SHARED, lsTableHandle_.fd(), 0);
>  
>  			if (lsTable_ == MAP_FAILED) {
> @@ -343,9 +343,9 @@ void IPARPi::processEvent(const IPAOperationData &event)
>  	case RPi::IPA_EVENT_SIGNAL_STAT_READY: {
>  		unsigned int bufferId = event.data[0];
>  
> -		if (++check_count_ != frame_count_) /* assert here? */
> +		if (++checkCount_ != frameCount_) /* assert here? */
>  			LOG(IPARPI, Error) << "WARNING: Prepare/Process mismatch!!!";
> -		if (frame_count_ > mistrust_count_)
> +		if (frameCount_ > mistrustCount_)
>  			processStats(bufferId);
>  
>  		reportMetadata();
> @@ -368,7 +368,7 @@ void IPARPi::processEvent(const IPAOperationData &event)
>  		 * they are "unreliable".
>  		 */
>  		prepareISP(embeddedbufferId);
> -		frame_count_++;
> +		frameCount_++;
>  
>  		/* Ready to push the input buffer into the ISP. */
>  		IPAOperationData op;
> @@ -713,7 +713,7 @@ void IPARPi::prepareISP(unsigned int bufferId)
>  	returnEmbeddedBuffer(bufferId);
>  
>  	if (success) {
> -		ControlList ctrls(isp_ctrls_);
> +		ControlList ctrls(ispCtrls_);
>  
>  		rpiMetadata_.Clear();
>  		rpiMetadata_.Set("device.status", deviceStatus);
> @@ -785,19 +785,19 @@ bool IPARPi::parseEmbeddedData(unsigned int bufferId, struct DeviceStatus &devic
>  	if (status != RPiController::MdParser::Status::OK) {
>  		LOG(IPARPI, Error) << "Embedded Buffer parsing failed, error " << status;
>  	} else {
> -		uint32_t exposure_lines, gain_code;
> -		if (helper_->Parser().GetExposureLines(exposure_lines) != RPiController::MdParser::Status::OK) {
> +		uint32_t exposureLines, gainCode;
> +		if (helper_->Parser().GetExposureLines(exposureLines) != RPiController::MdParser::Status::OK) {
>  			LOG(IPARPI, Error) << "Exposure time failed";
>  			return false;
>  		}
>  
> -		deviceStatus.shutter_speed = helper_->Exposure(exposure_lines);
> -		if (helper_->Parser().GetGainCode(gain_code) != RPiController::MdParser::Status::OK) {
> +		deviceStatus.shutter_speed = helper_->Exposure(exposureLines);
> +		if (helper_->Parser().GetGainCode(gainCode) != RPiController::MdParser::Status::OK) {
>  			LOG(IPARPI, Error) << "Gain failed";
>  			return false;
>  		}
>  
> -		deviceStatus.analogue_gain = helper_->Gain(gain_code);
> +		deviceStatus.analogue_gain = helper_->Gain(gainCode);
>  		LOG(IPARPI, Debug) << "Metadata - Exposure : "
>  				   << deviceStatus.shutter_speed << " Gain : "
>  				   << deviceStatus.analogue_gain;
> @@ -820,7 +820,7 @@ void IPARPi::processStats(unsigned int bufferId)
>  
>  	struct AgcStatus agcStatus;
>  	if (rpiMetadata_.Get("agc.status", agcStatus) == 0) {
> -		ControlList ctrls(unicam_ctrls_);
> +		ControlList ctrls(unicamCtrls_);
>  		applyAGC(&agcStatus, ctrls);
>  
>  		IPAOperationData op;
> @@ -832,14 +832,14 @@ void IPARPi::processStats(unsigned int bufferId)
>  
>  void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls)
>  {
> -	const auto gainR = isp_ctrls_.find(V4L2_CID_RED_BALANCE);
> -	if (gainR == isp_ctrls_.end()) {
> +	const auto gainR = ispCtrls_.find(V4L2_CID_RED_BALANCE);
> +	if (gainR == ispCtrls_.end()) {
>  		LOG(IPARPI, Error) << "Can't find red gain control";
>  		return;
>  	}
>  
> -	const auto gainB = isp_ctrls_.find(V4L2_CID_BLUE_BALANCE);
> -	if (gainB == isp_ctrls_.end()) {
> +	const auto gainB = ispCtrls_.find(V4L2_CID_BLUE_BALANCE);
> +	if (gainB == ispCtrls_.end()) {
>  		LOG(IPARPI, Error) << "Can't find blue gain control";
>  		return;
>  	}
> @@ -855,31 +855,31 @@ void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls)
>  
>  void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls)
>  {
> -	int32_t gain_code = helper_->GainCode(agcStatus->analogue_gain);
> -	int32_t exposure_lines = helper_->ExposureLines(agcStatus->shutter_time);
> +	int32_t gainCode = helper_->GainCode(agcStatus->analogue_gain);
> +	int32_t exposureLines = helper_->ExposureLines(agcStatus->shutter_time);
>  
> -	if (unicam_ctrls_.find(V4L2_CID_ANALOGUE_GAIN) == unicam_ctrls_.end()) {
> +	if (unicamCtrls_.find(V4L2_CID_ANALOGUE_GAIN) == unicamCtrls_.end()) {
>  		LOG(IPARPI, Error) << "Can't find analogue gain control";
>  		return;
>  	}
>  
> -	if (unicam_ctrls_.find(V4L2_CID_EXPOSURE) == unicam_ctrls_.end()) {
> +	if (unicamCtrls_.find(V4L2_CID_EXPOSURE) == unicamCtrls_.end()) {
>  		LOG(IPARPI, Error) << "Can't find exposure control";
>  		return;
>  	}
>  
>  	LOG(IPARPI, Debug) << "Applying AGC Exposure: " << agcStatus->shutter_time
> -			   << " (Shutter lines: " << exposure_lines << ") Gain: "
> +			   << " (Shutter lines: " << exposureLines << ") Gain: "
>  			   << agcStatus->analogue_gain << " (Gain Code: "
> -			   << gain_code << ")";
> +			   << gainCode << ")";
>  
> -	ctrls.set(V4L2_CID_ANALOGUE_GAIN, gain_code);
> -	ctrls.set(V4L2_CID_EXPOSURE, exposure_lines);
> +	ctrls.set(V4L2_CID_ANALOGUE_GAIN, gainCode);
> +	ctrls.set(V4L2_CID_EXPOSURE, exposureLines);
>  }
>  
>  void IPARPi::applyDG(const struct AgcStatus *dgStatus, ControlList &ctrls)
>  {
> -	if (isp_ctrls_.find(V4L2_CID_DIGITAL_GAIN) == isp_ctrls_.end()) {
> +	if (ispCtrls_.find(V4L2_CID_DIGITAL_GAIN) == ispCtrls_.end()) {
>  		LOG(IPARPI, Error) << "Can't find digital gain control";
>  		return;
>  	}
> @@ -890,7 +890,7 @@ void IPARPi::applyDG(const struct AgcStatus *dgStatus, ControlList &ctrls)
>  
>  void IPARPi::applyCCM(const struct CcmStatus *ccmStatus, ControlList &ctrls)
>  {
> -	if (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_CC_MATRIX) == isp_ctrls_.end()) {
> +	if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_CC_MATRIX) == ispCtrls_.end()) {
>  		LOG(IPARPI, Error) << "Can't find CCM control";
>  		return;
>  	}
> @@ -911,7 +911,7 @@ void IPARPi::applyCCM(const struct CcmStatus *ccmStatus, ControlList &ctrls)
>  
>  void IPARPi::applyGamma(const struct ContrastStatus *contrastStatus, ControlList &ctrls)
>  {
> -	if (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_GAMMA) == isp_ctrls_.end()) {
> +	if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_GAMMA) == ispCtrls_.end()) {
>  		LOG(IPARPI, Error) << "Can't find Gamma control";
>  		return;
>  	}
> @@ -930,7 +930,7 @@ void IPARPi::applyGamma(const struct ContrastStatus *contrastStatus, ControlList
>  
>  void IPARPi::applyBlackLevel(const struct BlackLevelStatus *blackLevelStatus, ControlList &ctrls)
>  {
> -	if (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_BLACK_LEVEL) == isp_ctrls_.end()) {
> +	if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_BLACK_LEVEL) == ispCtrls_.end()) {
>  		LOG(IPARPI, Error) << "Can't find black level control";
>  		return;
>  	}
> @@ -948,7 +948,7 @@ void IPARPi::applyBlackLevel(const struct BlackLevelStatus *blackLevelStatus, Co
>  
>  void IPARPi::applyGEQ(const struct GeqStatus *geqStatus, ControlList &ctrls)
>  {
> -	if (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_GEQ) == isp_ctrls_.end()) {
> +	if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_GEQ) == ispCtrls_.end()) {
>  		LOG(IPARPI, Error) << "Can't find geq control";
>  		return;
>  	}
> @@ -966,7 +966,7 @@ void IPARPi::applyGEQ(const struct GeqStatus *geqStatus, ControlList &ctrls)
>  
>  void IPARPi::applyDenoise(const struct SdnStatus *denoiseStatus, ControlList &ctrls)
>  {
> -	if (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_DENOISE) == isp_ctrls_.end()) {
> +	if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_DENOISE) == ispCtrls_.end()) {
>  		LOG(IPARPI, Error) << "Can't find denoise control";
>  		return;
>  	}
> @@ -986,7 +986,7 @@ void IPARPi::applyDenoise(const struct SdnStatus *denoiseStatus, ControlList &ct
>  
>  void IPARPi::applySharpen(const struct SharpenStatus *sharpenStatus, ControlList &ctrls)
>  {
> -	if (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_SHARPEN) == isp_ctrls_.end()) {
> +	if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_SHARPEN) == ispCtrls_.end()) {
>  		LOG(IPARPI, Error) << "Can't find sharpen control";
>  		return;
>  	}
> @@ -1007,7 +1007,7 @@ void IPARPi::applySharpen(const struct SharpenStatus *sharpenStatus, ControlList
>  
>  void IPARPi::applyDPC(const struct DpcStatus *dpcStatus, ControlList &ctrls)
>  {
> -	if (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_DPC) == isp_ctrls_.end()) {
> +	if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_DPC) == ispCtrls_.end()) {
>  		LOG(IPARPI, Error) << "Can't find DPC control";
>  		return;
>  	}
> @@ -1023,7 +1023,7 @@ void IPARPi::applyDPC(const struct DpcStatus *dpcStatus, ControlList &ctrls)
>  
>  void IPARPi::applyLS(const struct AlscStatus *lsStatus, ControlList &ctrls)
>  {
> -	if (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING) == isp_ctrls_.end()) {
> +	if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING) == ispCtrls_.end()) {
>  		LOG(IPARPI, Error) << "Can't find LS control";
>  		return;
>  	}
> @@ -1032,18 +1032,18 @@ void IPARPi::applyLS(const struct AlscStatus *lsStatus, ControlList &ctrls)
>  	 * Program lens shading tables into pipeline.
>  	 * Choose smallest cell size that won't exceed 63x48 cells.
>  	 */
> -	const int cell_sizes[] = { 16, 32, 64, 128, 256 };
> -	unsigned int num_cells = ARRAY_SIZE(cell_sizes);
> -	unsigned int i, w, h, cell_size;
> -	for (i = 0; i < num_cells; i++) {
> -		cell_size = cell_sizes[i];
> -		w = (mode_.width + cell_size - 1) / cell_size;
> -		h = (mode_.height + cell_size - 1) / cell_size;
> +	const int cellSizes[] = { 16, 32, 64, 128, 256 };
> +	unsigned int numCells = ARRAY_SIZE(cellSizes);
> +	unsigned int i, w, h, cellSize;
> +	for (i = 0; i < numCells; i++) {
> +		cellSize = cellSizes[i];
> +		w = (mode_.width + cellSize - 1) / cellSize;
> +		h = (mode_.height + cellSize - 1) / cellSize;
>  		if (w < 64 && h <= 48)
>  			break;
>  	}
>  
> -	if (i == num_cells) {
> +	if (i == numCells) {
>  		LOG(IPARPI, Error) << "Cannot find cell size";
>  		return;
>  	}
> @@ -1052,7 +1052,7 @@ void IPARPi::applyLS(const struct AlscStatus *lsStatus, ControlList &ctrls)
>  	w++, h++;
>  	bcm2835_isp_lens_shading ls = {
>  		.enabled = 1,
> -		.grid_cell_size = cell_size,
> +		.grid_cell_size = cellSize,
>  		.grid_width = w,
>  		.grid_stride = w,
>  		.grid_height = h,
> @@ -1062,7 +1062,7 @@ void IPARPi::applyLS(const struct AlscStatus *lsStatus, ControlList &ctrls)
>  		.gain_format = GAIN_FORMAT_U4P10
>  	};
>  
> -	if (!lsTable_ || w * h * 4 * sizeof(uint16_t) > MAX_LS_GRID_SIZE) {
> +	if (!lsTable_ || w * h * 4 * sizeof(uint16_t) > RPi::MaxLsGridSize) {
>  		LOG(IPARPI, Error) << "Do not have a correctly allocate lens shading table!";
>  		return;
>  	}
> @@ -1083,41 +1083,41 @@ void IPARPi::applyLS(const struct AlscStatus *lsStatus, ControlList &ctrls)
>  }
>  
>  /*
> - * Resamples a 16x12 table with central sampling to dest_w x dest_h with corner
> + * Resamples a 16x12 table with central sampling to destW x destH with corner
>   * sampling.
>   */
>  void IPARPi::resampleTable(uint16_t dest[], double const src[12][16],
> -			   int dest_w, int dest_h)
> +			   int destW, int destH)
>  {
>  	/*
>  	 * Precalculate and cache the x sampling locations and phases to
>  	 * save recomputing them on every row.
>  	 */
> -	assert(dest_w > 1 && dest_h > 1 && dest_w <= 64);
> -	int x_lo[64], x_hi[64];
> +	assert(destW > 1 && destH > 1 && destW <= 64);
> +	int xLo[64], xHi[64];
>  	double xf[64];
> -	double x = -0.5, x_inc = 16.0 / (dest_w - 1);
> -	for (int i = 0; i < dest_w; i++, x += x_inc) {
> -		x_lo[i] = floor(x);
> -		xf[i] = x - x_lo[i];
> -		x_hi[i] = x_lo[i] < 15 ? x_lo[i] + 1 : 15;
> -		x_lo[i] = x_lo[i] > 0 ? x_lo[i] : 0;
> +	double x = -0.5, xInc = 16.0 / (destW - 1);
> +	for (int i = 0; i < destW; i++, x += xInc) {
> +		xLo[i] = floor(x);
> +		xf[i] = x - xLo[i];
> +		xHi[i] = xLo[i] < 15 ? xLo[i] + 1 : 15;
> +		xLo[i] = xLo[i] > 0 ? xLo[i] : 0;
>  	}
>  
>  	/* Now march over the output table generating the new values. */
> -	double y = -0.5, y_inc = 12.0 / (dest_h - 1);
> -	for (int j = 0; j < dest_h; j++, y += y_inc) {
> -		int y_lo = floor(y);
> -		double yf = y - y_lo;
> -		int y_hi = y_lo < 11 ? y_lo + 1 : 11;
> -		y_lo = y_lo > 0 ? y_lo : 0;
> -		double const *row_above = src[y_lo];
> -		double const *row_below = src[y_hi];
> -		for (int i = 0; i < dest_w; i++) {
> -			double above = row_above[x_lo[i]] * (1 - xf[i])
> -				     + row_above[x_hi[i]] * xf[i];
> -			double below = row_below[x_lo[i]] * (1 - xf[i])
> -				     + row_below[x_hi[i]] * xf[i];
> +	double y = -0.5, yInc = 12.0 / (destH - 1);
> +	for (int j = 0; j < destH; j++, y += yInc) {
> +		int yLo = floor(y);
> +		double yf = y - yLo;
> +		int yHi = yLo < 11 ? yLo + 1 : 11;
> +		yLo = yLo > 0 ? yLo : 0;
> +		double const *rowAbove = src[yLo];
> +		double const *rowBelow = src[yHi];
> +		for (int i = 0; i < destW; i++) {
> +			double above = rowAbove[xLo[i]] * (1 - xf[i])
> +				     + rowAbove[xHi[i]] * xf[i];
> +			double below = rowBelow[xLo[i]] * (1 - xf[i])
> +				     + rowBelow[xHi[i]] * xf[i];
>  			int result = floor(1024 * (above * (1 - yf) + below * yf) + .5);
>  			*(dest++) = result > 16383 ? 16383 : result; /* want u4.10 */
>  		}
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 35dbe0fb..8d40b0ed 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -1012,7 +1012,7 @@ int RPiCameraData::configureIPA()
>  
>  	/* Allocate the lens shading table via dmaHeap and pass to the IPA. */
>  	if (!lsTable_.isValid()) {
> -		lsTable_ = dmaHeap_.alloc("ls_grid", MAX_LS_GRID_SIZE);
> +		lsTable_ = dmaHeap_.alloc("ls_grid", RPi::MaxLsGridSize);

If we had an IPA namespace, it would be nice to see this as:

  lsTable_ = dmaHeap_.alloc("ls_grid", ipa::RPi::MaxLsGridSize);

As that would make it really clear 'where' this MaxLsGridSize value was
coming from...

Anyway, all my comments above are optional discussion points.

For the patch:

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

>  		if (!lsTable_.isValid())
>  			return -ENOMEM;
>  
>
Jacopo Mondi Sept. 23, 2020, 11:39 a.m. UTC | #3
/me dives into bikeshedding

On Wed, Sep 23, 2020 at 12:18:48PM +0100, Kieran Bingham wrote:
> Hi Naush,
>
> On 22/09/2020 10:50, Naushir Patuck wrote:
> > Change variable names to camel case to be consistent with the rest of
> > the source files. Remove #define consts and replace with constexpr.
> >
>
> Sounds good to me...
>
>
> > There are no functional changes in this commit.
> >
> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > ---
> >  include/libcamera/ipa/raspberrypi.h           |   2 +-
> >  src/ipa/raspberrypi/raspberrypi.cpp           | 178 +++++++++---------
> >  .../pipeline/raspberrypi/raspberrypi.cpp      |   2 +-
> >  3 files changed, 91 insertions(+), 91 deletions(-)
> >
> > diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h
> > index c9d4aa81..b3041591 100644
> > --- a/include/libcamera/ipa/raspberrypi.h
> > +++ b/include/libcamera/ipa/raspberrypi.h
> > @@ -41,7 +41,7 @@ enum BufferMask {
> >  };
> >
> >  /* Size of the LS grid allocation. */
> > -#define MAX_LS_GRID_SIZE (32 << 10)
> > +static constexpr unsigned int MaxLsGridSize = 32 << 10;
>
> I guess the LS could stay capitalised (MaxLSGridSize), not sure that it
> matters, so up to you.
>
> >  /* List of controls handled by the Raspberry Pi IPA */
> >  static const ControlInfoMap Controls = {
> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> > index 0c0dc743..c240eae8 100644
> > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > @@ -55,8 +55,8 @@
> >  namespace libcamera {
> >
> >  /* Configure the sensor with these values initially. */
> > -#define DEFAULT_ANALOGUE_GAIN 1.0
> > -#define DEFAULT_EXPOSURE_TIME 20000
> > +constexpr unsigned int DefaultAnalogueGain = 1.0;
> > +constexpr unsigned int DefaultExposureTime = 20000;
>
> Oh nice, This really highlights to me the benefit of constexpr.
> Now the values have types!
>
> >
> >  LOG_DEFINE_CATEGORY(IPARPI)
> >
> > @@ -65,7 +65,7 @@ class IPARPi : public IPAInterface
> >  public:
> >  	IPARPi()
> >  		: lastMode_({}), controller_(), controllerInit_(false),
> > -		  frame_count_(0), check_count_(0), mistrust_count_(0),
> > +		  frameCount_(0), checkCount_(0), mistrustCount_(0),
> >  		  lsTable_(nullptr)
> >  	{
> >  	}
> > @@ -73,7 +73,7 @@ public:
> >  	~IPARPi()
> >  	{
> >  		if (lsTable_)
> > -			munmap(lsTable_, MAX_LS_GRID_SIZE);
> > +			munmap(lsTable_, RPi::MaxLsGridSize);
> >  	}
> >
> >  	int init(const IPASettings &settings) override;
> > @@ -108,13 +108,13 @@ private:
> >  	void applySharpen(const struct SharpenStatus *sharpenStatus, ControlList &ctrls);
> >  	void applyDPC(const struct DpcStatus *dpcStatus, ControlList &ctrls);
> >  	void applyLS(const struct AlscStatus *lsStatus, ControlList &ctrls);
> > -	void resampleTable(uint16_t dest[], double const src[12][16], int dest_w, int dest_h);
> > +	void resampleTable(uint16_t dest[], double const src[12][16], int destW, int destH);
> >
> >  	std::map<unsigned int, FrameBuffer> buffers_;
> >  	std::map<unsigned int, void *> buffersMemory_;
> >
> > -	ControlInfoMap unicam_ctrls_;
> > -	ControlInfoMap isp_ctrls_;
> > +	ControlInfoMap unicamCtrls_;
> > +	ControlInfoMap ispCtrls_;
> >  	ControlList libcameraMetadata_;
> >
> >  	/* IPA configuration. */
> > @@ -134,11 +134,11 @@ private:
> >  	 * We count frames to decide if the frame must be hidden (e.g. from
> >  	 * display) or mistrusted (i.e. not given to the control algos).
> >  	 */
> > -	uint64_t frame_count_;
> > +	uint64_t frameCount_;
>
> If we're doing clean up - I always prefer to see a newline before a
> comment line.
>
> To me it makes the commented block clearer - but I don't know if there's
> a 'rule' on it.
>

I'm sorry but

        /*
         * A comment describing a variable is better places as close
         * as possible near to that variable, isn't it ?
         */
         int whatAFancyVariable;

         /* Or do you think this variable is not fancy and an empty line is better?  */

         int notAFancyVariable;

Ok, enough useless discussions from me :)

>
> >  	/* For checking the sequencing of Prepare/Process calls. */
> > -	uint64_t check_count_;
> > +	uint64_t checkCount_;
>
> i.e. here too.
>
> >  	/* How many frames we should avoid running control algos on. */
> > -	unsigned int mistrust_count_;
> > +	unsigned int mistrustCount_;
>
> and so on.
>
> >  	/* LS table allocation passed in from the pipeline handler. */
> >  	FileDescriptor lsTableHandle_;
> >  	void *lsTable_;
> > @@ -199,8 +199,8 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
> >
> >  	result->operation = 0;
> >
> > -	unicam_ctrls_ = entityControls.at(0);
> > -	isp_ctrls_ = entityControls.at(1);
> > +	unicamCtrls_ = entityControls.at(0);
> > +	ispCtrls_ = entityControls.at(1);
> >  	/* Setup a metadata ControlList to output metadata. */
> >  	libcameraMetadata_ = ControlList(controls::controls);
> >
> > @@ -238,18 +238,18 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
> >  	 *"mistrusted", which depends on whether this is a startup from cold,
> >  	 * or merely a mode switch in a running system.
> >  	 */
> > -	frame_count_ = 0;
> > -	check_count_ = 0;
> > -	unsigned int drop_frame = 0;
> > +	frameCount_ = 0;
> > +	checkCount_ = 0;
> > +	unsigned int dropFrame = 0;
> >  	if (controllerInit_) {
> > -		drop_frame = helper_->HideFramesModeSwitch();
> > -		mistrust_count_ = helper_->MistrustFramesModeSwitch();
> > +		dropFrame = helper_->HideFramesModeSwitch();
> > +		mistrustCount_ = helper_->MistrustFramesModeSwitch();
> >  	} else {
> > -		drop_frame = helper_->HideFramesStartup();
> > -		mistrust_count_ = helper_->MistrustFramesStartup();
> > +		dropFrame = helper_->HideFramesStartup();
> > +		mistrustCount_ = helper_->MistrustFramesStartup();
> >  	}
> >
> > -	result->data.push_back(drop_frame);
> > +	result->data.push_back(dropFrame);
> >  	result->operation |= RPi::IPA_CONFIG_DROP_FRAMES;
> >
> >  	struct AgcStatus agcStatus;
> > @@ -264,8 +264,8 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
> >  		controllerInit_ = true;
> >
> >  		/* Supply initial values for gain and exposure. */
> > -		agcStatus.shutter_time = DEFAULT_EXPOSURE_TIME;
> > -		agcStatus.analogue_gain = DEFAULT_ANALOGUE_GAIN;
> > +		agcStatus.shutter_time = DefaultExposureTime;
> > +		agcStatus.analogue_gain = DefaultAnalogueGain;
> >  	}
> >
> >  	RPiController::Metadata metadata;
> > @@ -274,7 +274,7 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
> >  	/* SwitchMode may supply updated exposure/gain values to use. */
> >  	metadata.Get("agc.status", agcStatus);
> >  	if (agcStatus.shutter_time != 0.0 && agcStatus.analogue_gain != 0.0) {
> > -		ControlList ctrls(unicam_ctrls_);
> > +		ControlList ctrls(unicamCtrls_);
> >  		applyAGC(&agcStatus, ctrls);
> >  		result->controls.push_back(ctrls);
> >
> > @@ -287,14 +287,14 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
> >  	if (ipaConfig.operation & RPi::IPA_CONFIG_LS_TABLE) {
> >  		/* Remove any previous table, if there was one. */
> >  		if (lsTable_) {
> > -			munmap(lsTable_, MAX_LS_GRID_SIZE);
> > +			munmap(lsTable_, RPi::MaxLsGridSize);
> >  			lsTable_ = nullptr;
> >  		}
> >
> >  		/* Map the LS table buffer into user space. */
> >  		lsTableHandle_ = FileDescriptor(ipaConfig.data[0]);
> >  		if (lsTableHandle_.isValid()) {
> > -			lsTable_ = mmap(nullptr, MAX_LS_GRID_SIZE, PROT_READ | PROT_WRITE,
> > +			lsTable_ = mmap(nullptr, RPi::MaxLsGridSize, PROT_READ | PROT_WRITE,
> >  					MAP_SHARED, lsTableHandle_.fd(), 0);
> >
> >  			if (lsTable_ == MAP_FAILED) {
> > @@ -343,9 +343,9 @@ void IPARPi::processEvent(const IPAOperationData &event)
> >  	case RPi::IPA_EVENT_SIGNAL_STAT_READY: {
> >  		unsigned int bufferId = event.data[0];
> >
> > -		if (++check_count_ != frame_count_) /* assert here? */
> > +		if (++checkCount_ != frameCount_) /* assert here? */
> >  			LOG(IPARPI, Error) << "WARNING: Prepare/Process mismatch!!!";
> > -		if (frame_count_ > mistrust_count_)
> > +		if (frameCount_ > mistrustCount_)
> >  			processStats(bufferId);
> >
> >  		reportMetadata();
> > @@ -368,7 +368,7 @@ void IPARPi::processEvent(const IPAOperationData &event)
> >  		 * they are "unreliable".
> >  		 */
> >  		prepareISP(embeddedbufferId);
> > -		frame_count_++;
> > +		frameCount_++;
> >
> >  		/* Ready to push the input buffer into the ISP. */
> >  		IPAOperationData op;
> > @@ -713,7 +713,7 @@ void IPARPi::prepareISP(unsigned int bufferId)
> >  	returnEmbeddedBuffer(bufferId);
> >
> >  	if (success) {
> > -		ControlList ctrls(isp_ctrls_);
> > +		ControlList ctrls(ispCtrls_);
> >
> >  		rpiMetadata_.Clear();
> >  		rpiMetadata_.Set("device.status", deviceStatus);
> > @@ -785,19 +785,19 @@ bool IPARPi::parseEmbeddedData(unsigned int bufferId, struct DeviceStatus &devic
> >  	if (status != RPiController::MdParser::Status::OK) {
> >  		LOG(IPARPI, Error) << "Embedded Buffer parsing failed, error " << status;
> >  	} else {
> > -		uint32_t exposure_lines, gain_code;
> > -		if (helper_->Parser().GetExposureLines(exposure_lines) != RPiController::MdParser::Status::OK) {
> > +		uint32_t exposureLines, gainCode;
> > +		if (helper_->Parser().GetExposureLines(exposureLines) != RPiController::MdParser::Status::OK) {
> >  			LOG(IPARPI, Error) << "Exposure time failed";
> >  			return false;
> >  		}
> >
> > -		deviceStatus.shutter_speed = helper_->Exposure(exposure_lines);
> > -		if (helper_->Parser().GetGainCode(gain_code) != RPiController::MdParser::Status::OK) {
> > +		deviceStatus.shutter_speed = helper_->Exposure(exposureLines);
> > +		if (helper_->Parser().GetGainCode(gainCode) != RPiController::MdParser::Status::OK) {
> >  			LOG(IPARPI, Error) << "Gain failed";
> >  			return false;
> >  		}
> >
> > -		deviceStatus.analogue_gain = helper_->Gain(gain_code);
> > +		deviceStatus.analogue_gain = helper_->Gain(gainCode);
> >  		LOG(IPARPI, Debug) << "Metadata - Exposure : "
> >  				   << deviceStatus.shutter_speed << " Gain : "
> >  				   << deviceStatus.analogue_gain;
> > @@ -820,7 +820,7 @@ void IPARPi::processStats(unsigned int bufferId)
> >
> >  	struct AgcStatus agcStatus;
> >  	if (rpiMetadata_.Get("agc.status", agcStatus) == 0) {
> > -		ControlList ctrls(unicam_ctrls_);
> > +		ControlList ctrls(unicamCtrls_);
> >  		applyAGC(&agcStatus, ctrls);
> >
> >  		IPAOperationData op;
> > @@ -832,14 +832,14 @@ void IPARPi::processStats(unsigned int bufferId)
> >
> >  void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls)
> >  {
> > -	const auto gainR = isp_ctrls_.find(V4L2_CID_RED_BALANCE);
> > -	if (gainR == isp_ctrls_.end()) {
> > +	const auto gainR = ispCtrls_.find(V4L2_CID_RED_BALANCE);
> > +	if (gainR == ispCtrls_.end()) {
> >  		LOG(IPARPI, Error) << "Can't find red gain control";
> >  		return;
> >  	}
> >
> > -	const auto gainB = isp_ctrls_.find(V4L2_CID_BLUE_BALANCE);
> > -	if (gainB == isp_ctrls_.end()) {
> > +	const auto gainB = ispCtrls_.find(V4L2_CID_BLUE_BALANCE);
> > +	if (gainB == ispCtrls_.end()) {
> >  		LOG(IPARPI, Error) << "Can't find blue gain control";
> >  		return;
> >  	}
> > @@ -855,31 +855,31 @@ void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls)
> >
> >  void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls)
> >  {
> > -	int32_t gain_code = helper_->GainCode(agcStatus->analogue_gain);
> > -	int32_t exposure_lines = helper_->ExposureLines(agcStatus->shutter_time);
> > +	int32_t gainCode = helper_->GainCode(agcStatus->analogue_gain);
> > +	int32_t exposureLines = helper_->ExposureLines(agcStatus->shutter_time);
> >
> > -	if (unicam_ctrls_.find(V4L2_CID_ANALOGUE_GAIN) == unicam_ctrls_.end()) {
> > +	if (unicamCtrls_.find(V4L2_CID_ANALOGUE_GAIN) == unicamCtrls_.end()) {
> >  		LOG(IPARPI, Error) << "Can't find analogue gain control";
> >  		return;
> >  	}
> >
> > -	if (unicam_ctrls_.find(V4L2_CID_EXPOSURE) == unicam_ctrls_.end()) {
> > +	if (unicamCtrls_.find(V4L2_CID_EXPOSURE) == unicamCtrls_.end()) {
> >  		LOG(IPARPI, Error) << "Can't find exposure control";
> >  		return;
> >  	}
> >
> >  	LOG(IPARPI, Debug) << "Applying AGC Exposure: " << agcStatus->shutter_time
> > -			   << " (Shutter lines: " << exposure_lines << ") Gain: "
> > +			   << " (Shutter lines: " << exposureLines << ") Gain: "
> >  			   << agcStatus->analogue_gain << " (Gain Code: "
> > -			   << gain_code << ")";
> > +			   << gainCode << ")";
> >
> > -	ctrls.set(V4L2_CID_ANALOGUE_GAIN, gain_code);
> > -	ctrls.set(V4L2_CID_EXPOSURE, exposure_lines);
> > +	ctrls.set(V4L2_CID_ANALOGUE_GAIN, gainCode);
> > +	ctrls.set(V4L2_CID_EXPOSURE, exposureLines);
> >  }
> >
> >  void IPARPi::applyDG(const struct AgcStatus *dgStatus, ControlList &ctrls)
> >  {
> > -	if (isp_ctrls_.find(V4L2_CID_DIGITAL_GAIN) == isp_ctrls_.end()) {
> > +	if (ispCtrls_.find(V4L2_CID_DIGITAL_GAIN) == ispCtrls_.end()) {
> >  		LOG(IPARPI, Error) << "Can't find digital gain control";
> >  		return;
> >  	}
> > @@ -890,7 +890,7 @@ void IPARPi::applyDG(const struct AgcStatus *dgStatus, ControlList &ctrls)
> >
> >  void IPARPi::applyCCM(const struct CcmStatus *ccmStatus, ControlList &ctrls)
> >  {
> > -	if (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_CC_MATRIX) == isp_ctrls_.end()) {
> > +	if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_CC_MATRIX) == ispCtrls_.end()) {
> >  		LOG(IPARPI, Error) << "Can't find CCM control";
> >  		return;
> >  	}
> > @@ -911,7 +911,7 @@ void IPARPi::applyCCM(const struct CcmStatus *ccmStatus, ControlList &ctrls)
> >
> >  void IPARPi::applyGamma(const struct ContrastStatus *contrastStatus, ControlList &ctrls)
> >  {
> > -	if (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_GAMMA) == isp_ctrls_.end()) {
> > +	if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_GAMMA) == ispCtrls_.end()) {
> >  		LOG(IPARPI, Error) << "Can't find Gamma control";
> >  		return;
> >  	}
> > @@ -930,7 +930,7 @@ void IPARPi::applyGamma(const struct ContrastStatus *contrastStatus, ControlList
> >
> >  void IPARPi::applyBlackLevel(const struct BlackLevelStatus *blackLevelStatus, ControlList &ctrls)
> >  {
> > -	if (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_BLACK_LEVEL) == isp_ctrls_.end()) {
> > +	if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_BLACK_LEVEL) == ispCtrls_.end()) {
> >  		LOG(IPARPI, Error) << "Can't find black level control";
> >  		return;
> >  	}
> > @@ -948,7 +948,7 @@ void IPARPi::applyBlackLevel(const struct BlackLevelStatus *blackLevelStatus, Co
> >
> >  void IPARPi::applyGEQ(const struct GeqStatus *geqStatus, ControlList &ctrls)
> >  {
> > -	if (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_GEQ) == isp_ctrls_.end()) {
> > +	if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_GEQ) == ispCtrls_.end()) {
> >  		LOG(IPARPI, Error) << "Can't find geq control";
> >  		return;
> >  	}
> > @@ -966,7 +966,7 @@ void IPARPi::applyGEQ(const struct GeqStatus *geqStatus, ControlList &ctrls)
> >
> >  void IPARPi::applyDenoise(const struct SdnStatus *denoiseStatus, ControlList &ctrls)
> >  {
> > -	if (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_DENOISE) == isp_ctrls_.end()) {
> > +	if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_DENOISE) == ispCtrls_.end()) {
> >  		LOG(IPARPI, Error) << "Can't find denoise control";
> >  		return;
> >  	}
> > @@ -986,7 +986,7 @@ void IPARPi::applyDenoise(const struct SdnStatus *denoiseStatus, ControlList &ct
> >
> >  void IPARPi::applySharpen(const struct SharpenStatus *sharpenStatus, ControlList &ctrls)
> >  {
> > -	if (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_SHARPEN) == isp_ctrls_.end()) {
> > +	if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_SHARPEN) == ispCtrls_.end()) {
> >  		LOG(IPARPI, Error) << "Can't find sharpen control";
> >  		return;
> >  	}
> > @@ -1007,7 +1007,7 @@ void IPARPi::applySharpen(const struct SharpenStatus *sharpenStatus, ControlList
> >
> >  void IPARPi::applyDPC(const struct DpcStatus *dpcStatus, ControlList &ctrls)
> >  {
> > -	if (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_DPC) == isp_ctrls_.end()) {
> > +	if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_DPC) == ispCtrls_.end()) {
> >  		LOG(IPARPI, Error) << "Can't find DPC control";
> >  		return;
> >  	}
> > @@ -1023,7 +1023,7 @@ void IPARPi::applyDPC(const struct DpcStatus *dpcStatus, ControlList &ctrls)
> >
> >  void IPARPi::applyLS(const struct AlscStatus *lsStatus, ControlList &ctrls)
> >  {
> > -	if (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING) == isp_ctrls_.end()) {
> > +	if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING) == ispCtrls_.end()) {
> >  		LOG(IPARPI, Error) << "Can't find LS control";
> >  		return;
> >  	}
> > @@ -1032,18 +1032,18 @@ void IPARPi::applyLS(const struct AlscStatus *lsStatus, ControlList &ctrls)
> >  	 * Program lens shading tables into pipeline.
> >  	 * Choose smallest cell size that won't exceed 63x48 cells.
> >  	 */
> > -	const int cell_sizes[] = { 16, 32, 64, 128, 256 };
> > -	unsigned int num_cells = ARRAY_SIZE(cell_sizes);
> > -	unsigned int i, w, h, cell_size;
> > -	for (i = 0; i < num_cells; i++) {
> > -		cell_size = cell_sizes[i];
> > -		w = (mode_.width + cell_size - 1) / cell_size;
> > -		h = (mode_.height + cell_size - 1) / cell_size;
> > +	const int cellSizes[] = { 16, 32, 64, 128, 256 };
> > +	unsigned int numCells = ARRAY_SIZE(cellSizes);
> > +	unsigned int i, w, h, cellSize;
> > +	for (i = 0; i < numCells; i++) {
> > +		cellSize = cellSizes[i];
> > +		w = (mode_.width + cellSize - 1) / cellSize;
> > +		h = (mode_.height + cellSize - 1) / cellSize;
> >  		if (w < 64 && h <= 48)
> >  			break;
> >  	}
> >
> > -	if (i == num_cells) {
> > +	if (i == numCells) {
> >  		LOG(IPARPI, Error) << "Cannot find cell size";
> >  		return;
> >  	}
> > @@ -1052,7 +1052,7 @@ void IPARPi::applyLS(const struct AlscStatus *lsStatus, ControlList &ctrls)
> >  	w++, h++;
> >  	bcm2835_isp_lens_shading ls = {
> >  		.enabled = 1,
> > -		.grid_cell_size = cell_size,
> > +		.grid_cell_size = cellSize,
> >  		.grid_width = w,
> >  		.grid_stride = w,
> >  		.grid_height = h,
> > @@ -1062,7 +1062,7 @@ void IPARPi::applyLS(const struct AlscStatus *lsStatus, ControlList &ctrls)
> >  		.gain_format = GAIN_FORMAT_U4P10
> >  	};
> >
> > -	if (!lsTable_ || w * h * 4 * sizeof(uint16_t) > MAX_LS_GRID_SIZE) {
> > +	if (!lsTable_ || w * h * 4 * sizeof(uint16_t) > RPi::MaxLsGridSize) {
> >  		LOG(IPARPI, Error) << "Do not have a correctly allocate lens shading table!";
> >  		return;
> >  	}
> > @@ -1083,41 +1083,41 @@ void IPARPi::applyLS(const struct AlscStatus *lsStatus, ControlList &ctrls)
> >  }
> >
> >  /*
> > - * Resamples a 16x12 table with central sampling to dest_w x dest_h with corner
> > + * Resamples a 16x12 table with central sampling to destW x destH with corner
> >   * sampling.
> >   */
> >  void IPARPi::resampleTable(uint16_t dest[], double const src[12][16],
> > -			   int dest_w, int dest_h)
> > +			   int destW, int destH)
> >  {
> >  	/*
> >  	 * Precalculate and cache the x sampling locations and phases to
> >  	 * save recomputing them on every row.
> >  	 */
> > -	assert(dest_w > 1 && dest_h > 1 && dest_w <= 64);
> > -	int x_lo[64], x_hi[64];
> > +	assert(destW > 1 && destH > 1 && destW <= 64);
> > +	int xLo[64], xHi[64];
> >  	double xf[64];
> > -	double x = -0.5, x_inc = 16.0 / (dest_w - 1);
> > -	for (int i = 0; i < dest_w; i++, x += x_inc) {
> > -		x_lo[i] = floor(x);
> > -		xf[i] = x - x_lo[i];
> > -		x_hi[i] = x_lo[i] < 15 ? x_lo[i] + 1 : 15;
> > -		x_lo[i] = x_lo[i] > 0 ? x_lo[i] : 0;
> > +	double x = -0.5, xInc = 16.0 / (destW - 1);
> > +	for (int i = 0; i < destW; i++, x += xInc) {
> > +		xLo[i] = floor(x);
> > +		xf[i] = x - xLo[i];
> > +		xHi[i] = xLo[i] < 15 ? xLo[i] + 1 : 15;
> > +		xLo[i] = xLo[i] > 0 ? xLo[i] : 0;
> >  	}
> >
> >  	/* Now march over the output table generating the new values. */
> > -	double y = -0.5, y_inc = 12.0 / (dest_h - 1);
> > -	for (int j = 0; j < dest_h; j++, y += y_inc) {
> > -		int y_lo = floor(y);
> > -		double yf = y - y_lo;
> > -		int y_hi = y_lo < 11 ? y_lo + 1 : 11;
> > -		y_lo = y_lo > 0 ? y_lo : 0;
> > -		double const *row_above = src[y_lo];
> > -		double const *row_below = src[y_hi];
> > -		for (int i = 0; i < dest_w; i++) {
> > -			double above = row_above[x_lo[i]] * (1 - xf[i])
> > -				     + row_above[x_hi[i]] * xf[i];
> > -			double below = row_below[x_lo[i]] * (1 - xf[i])
> > -				     + row_below[x_hi[i]] * xf[i];
> > +	double y = -0.5, yInc = 12.0 / (destH - 1);
> > +	for (int j = 0; j < destH; j++, y += yInc) {
> > +		int yLo = floor(y);
> > +		double yf = y - yLo;
> > +		int yHi = yLo < 11 ? yLo + 1 : 11;
> > +		yLo = yLo > 0 ? yLo : 0;
> > +		double const *rowAbove = src[yLo];
> > +		double const *rowBelow = src[yHi];
> > +		for (int i = 0; i < destW; i++) {
> > +			double above = rowAbove[xLo[i]] * (1 - xf[i])
> > +				     + rowAbove[xHi[i]] * xf[i];
> > +			double below = rowBelow[xLo[i]] * (1 - xf[i])
> > +				     + rowBelow[xHi[i]] * xf[i];
> >  			int result = floor(1024 * (above * (1 - yf) + below * yf) + .5);
> >  			*(dest++) = result > 16383 ? 16383 : result; /* want u4.10 */
> >  		}
> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > index 35dbe0fb..8d40b0ed 100644
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > @@ -1012,7 +1012,7 @@ int RPiCameraData::configureIPA()
> >
> >  	/* Allocate the lens shading table via dmaHeap and pass to the IPA. */
> >  	if (!lsTable_.isValid()) {
> > -		lsTable_ = dmaHeap_.alloc("ls_grid", MAX_LS_GRID_SIZE);
> > +		lsTable_ = dmaHeap_.alloc("ls_grid", RPi::MaxLsGridSize);
>
> If we had an IPA namespace, it would be nice to see this as:
>
>   lsTable_ = dmaHeap_.alloc("ls_grid", ipa::RPi::MaxLsGridSize);
>
> As that would make it really clear 'where' this MaxLsGridSize value was
> coming from...
>
> Anyway, all my comments above are optional discussion points.
>
> For the patch:
>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>
> >  		if (!lsTable_.isValid())
> >  			return -ENOMEM;
> >
> >
>
> --
> Regards
> --
> Kieran
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Kieran Bingham Sept. 23, 2020, 11:45 a.m. UTC | #4
Hi Jacopo,

On 23/09/2020 12:39, Jacopo Mondi wrote:
> /me dives into bikeshedding

<snip>

>>> @@ -134,11 +134,11 @@ private:
>>>  	 * We count frames to decide if the frame must be hidden (e.g. from
>>>  	 * display) or mistrusted (i.e. not given to the control algos).
>>>  	 */
>>> -	uint64_t frame_count_;
>>> +	uint64_t frameCount_;
>>
>> If we're doing clean up - I always prefer to see a newline before a
>> comment line.

Note here I state 'before a comment'

>> To me it makes the commented block clearer - but I don't know if there's
>> a 'rule' on it.
>>
> 
> I'm sorry but
> 
>         /*
>          * A comment describing a variable is better places as close
>          * as possible near to that variable, isn't it ?
>          */
>          int whatAFancyVariable;
> 
>          /* Or do you think this variable is not fancy and an empty line is better?  */
> 

Here you seem to be implying 'after' a comment...

>          int notAFancyVariable;
> 
> Ok, enough useless discussions from me :)
> 

I ... 'think' you mis-interpreted what I said, or I was not clear. I was
probably not clear ;-)

I think that this:

	/* A variable group. */
	bool variable;
	int quantity;

	/*
	 * We count frames to decide if the frame must be hidden (e.g.
	 * from display) or mistrusted (i.e. not given to the control
	 * algos).
	 */
	uint64_t frameCount_;

	/* For checking the sequencing of Prepare/Process calls. */
	uint64_t checkCount_;

	/* How many frames we should avoid running control algos on. */
	unsigned int mistrustCount_;

is better than this:


	/* A variable group */
	bool variable;
	int quanty;
	/*
	 * We count frames to decide if the frame must be hidden (e.g.
	 * from display) or mistrusted (i.e. not given to the control
	 * algos).
	 */
	uint64_t frameCount_;
	/* For checking the sequencing of Prepare/Process calls. */
	uint64_t checkCount_;
	/* How many frames we should avoid running control algos on. */
	unsigned int mistrustCount_;

Maybe one day I'll finish my comment formatter in the checkstyle script ;-)


>>
>>>  	/* For checking the sequencing of Prepare/Process calls. */
>>> -	uint64_t check_count_;
>>> +	uint64_t checkCount_;
>>
>> i.e. here too.
>>
>>>  	/* How many frames we should avoid running control algos on. */
>>> -	unsigned int mistrust_count_;
>>> +	unsigned int mistrustCount_;
>>
>> and so on.
>>
>>>  	/* LS table allocation passed in from the pipeline handler. */
>>>  	FileDescriptor lsTableHandle_;
>>>  	void *lsTable_;

<snip>
David Plowman Sept. 23, 2020, 4:31 p.m. UTC | #5
Hi Naush

Thanks for doing all this tidying! One little nit-pick...

On Wed, 23 Sep 2020 at 12:18, Kieran Bingham
<kieran.bingham@ideasonboard.com> wrote:
>
> Hi Naush,
>
> On 22/09/2020 10:50, Naushir Patuck wrote:
> > Change variable names to camel case to be consistent with the rest of
> > the source files. Remove #define consts and replace with constexpr.
> >
>
> Sounds good to me...
>
>
> > There are no functional changes in this commit.
> >
> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > ---
> >  include/libcamera/ipa/raspberrypi.h           |   2 +-
> >  src/ipa/raspberrypi/raspberrypi.cpp           | 178 +++++++++---------
> >  .../pipeline/raspberrypi/raspberrypi.cpp      |   2 +-
> >  3 files changed, 91 insertions(+), 91 deletions(-)
> >
> > diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h
> > index c9d4aa81..b3041591 100644
> > --- a/include/libcamera/ipa/raspberrypi.h
> > +++ b/include/libcamera/ipa/raspberrypi.h
> > @@ -41,7 +41,7 @@ enum BufferMask {
> >  };
> >
> >  /* Size of the LS grid allocation. */
> > -#define MAX_LS_GRID_SIZE (32 << 10)
> > +static constexpr unsigned int MaxLsGridSize = 32 << 10;
>
> I guess the LS could stay capitalised (MaxLSGridSize), not sure that it
> matters, so up to you.
>
> >  /* List of controls handled by the Raspberry Pi IPA */
> >  static const ControlInfoMap Controls = {
> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> > index 0c0dc743..c240eae8 100644
> > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > @@ -55,8 +55,8 @@
> >  namespace libcamera {
> >
> >  /* Configure the sensor with these values initially. */
> > -#define DEFAULT_ANALOGUE_GAIN 1.0
> > -#define DEFAULT_EXPOSURE_TIME 20000
> > +constexpr unsigned int DefaultAnalogueGain = 1.0;
> > +constexpr unsigned int DefaultExposureTime = 20000;
>
> Oh nice, This really highlights to me the benefit of constexpr.
> Now the values have types!

Should DefaultAnalogueGain be a double? Of course it gets cast to a
double when it gets copied into agcStatus.analogue_gain so it probably
contrives to make no difference, but a double might be clearer,
especially if anyone ever changed it.

Thanks!
David

>
> >
> >  LOG_DEFINE_CATEGORY(IPARPI)
> >
> > @@ -65,7 +65,7 @@ class IPARPi : public IPAInterface
> >  public:
> >       IPARPi()
> >               : lastMode_({}), controller_(), controllerInit_(false),
> > -               frame_count_(0), check_count_(0), mistrust_count_(0),
> > +               frameCount_(0), checkCount_(0), mistrustCount_(0),
> >                 lsTable_(nullptr)
> >       {
> >       }
> > @@ -73,7 +73,7 @@ public:
> >       ~IPARPi()
> >       {
> >               if (lsTable_)
> > -                     munmap(lsTable_, MAX_LS_GRID_SIZE);
> > +                     munmap(lsTable_, RPi::MaxLsGridSize);
> >       }
> >
> >       int init(const IPASettings &settings) override;
> > @@ -108,13 +108,13 @@ private:
> >       void applySharpen(const struct SharpenStatus *sharpenStatus, ControlList &ctrls);
> >       void applyDPC(const struct DpcStatus *dpcStatus, ControlList &ctrls);
> >       void applyLS(const struct AlscStatus *lsStatus, ControlList &ctrls);
> > -     void resampleTable(uint16_t dest[], double const src[12][16], int dest_w, int dest_h);
> > +     void resampleTable(uint16_t dest[], double const src[12][16], int destW, int destH);
> >
> >       std::map<unsigned int, FrameBuffer> buffers_;
> >       std::map<unsigned int, void *> buffersMemory_;
> >
> > -     ControlInfoMap unicam_ctrls_;
> > -     ControlInfoMap isp_ctrls_;
> > +     ControlInfoMap unicamCtrls_;
> > +     ControlInfoMap ispCtrls_;
> >       ControlList libcameraMetadata_;
> >
> >       /* IPA configuration. */
> > @@ -134,11 +134,11 @@ private:
> >        * We count frames to decide if the frame must be hidden (e.g. from
> >        * display) or mistrusted (i.e. not given to the control algos).
> >        */
> > -     uint64_t frame_count_;
> > +     uint64_t frameCount_;
>
> If we're doing clean up - I always prefer to see a newline before a
> comment line.
>
> To me it makes the commented block clearer - but I don't know if there's
> a 'rule' on it.
>
>
> >       /* For checking the sequencing of Prepare/Process calls. */
> > -     uint64_t check_count_;
> > +     uint64_t checkCount_;
>
> i.e. here too.
>
> >       /* How many frames we should avoid running control algos on. */
> > -     unsigned int mistrust_count_;
> > +     unsigned int mistrustCount_;
>
> and so on.
>
> >       /* LS table allocation passed in from the pipeline handler. */
> >       FileDescriptor lsTableHandle_;
> >       void *lsTable_;
> > @@ -199,8 +199,8 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
> >
> >       result->operation = 0;
> >
> > -     unicam_ctrls_ = entityControls.at(0);
> > -     isp_ctrls_ = entityControls.at(1);
> > +     unicamCtrls_ = entityControls.at(0);
> > +     ispCtrls_ = entityControls.at(1);
> >       /* Setup a metadata ControlList to output metadata. */
> >       libcameraMetadata_ = ControlList(controls::controls);
> >
> > @@ -238,18 +238,18 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
> >        *"mistrusted", which depends on whether this is a startup from cold,
> >        * or merely a mode switch in a running system.
> >        */
> > -     frame_count_ = 0;
> > -     check_count_ = 0;
> > -     unsigned int drop_frame = 0;
> > +     frameCount_ = 0;
> > +     checkCount_ = 0;
> > +     unsigned int dropFrame = 0;
> >       if (controllerInit_) {
> > -             drop_frame = helper_->HideFramesModeSwitch();
> > -             mistrust_count_ = helper_->MistrustFramesModeSwitch();
> > +             dropFrame = helper_->HideFramesModeSwitch();
> > +             mistrustCount_ = helper_->MistrustFramesModeSwitch();
> >       } else {
> > -             drop_frame = helper_->HideFramesStartup();
> > -             mistrust_count_ = helper_->MistrustFramesStartup();
> > +             dropFrame = helper_->HideFramesStartup();
> > +             mistrustCount_ = helper_->MistrustFramesStartup();
> >       }
> >
> > -     result->data.push_back(drop_frame);
> > +     result->data.push_back(dropFrame);
> >       result->operation |= RPi::IPA_CONFIG_DROP_FRAMES;
> >
> >       struct AgcStatus agcStatus;
> > @@ -264,8 +264,8 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
> >               controllerInit_ = true;
> >
> >               /* Supply initial values for gain and exposure. */
> > -             agcStatus.shutter_time = DEFAULT_EXPOSURE_TIME;
> > -             agcStatus.analogue_gain = DEFAULT_ANALOGUE_GAIN;
> > +             agcStatus.shutter_time = DefaultExposureTime;
> > +             agcStatus.analogue_gain = DefaultAnalogueGain;
> >       }
> >
> >       RPiController::Metadata metadata;
> > @@ -274,7 +274,7 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
> >       /* SwitchMode may supply updated exposure/gain values to use. */
> >       metadata.Get("agc.status", agcStatus);
> >       if (agcStatus.shutter_time != 0.0 && agcStatus.analogue_gain != 0.0) {
> > -             ControlList ctrls(unicam_ctrls_);
> > +             ControlList ctrls(unicamCtrls_);
> >               applyAGC(&agcStatus, ctrls);
> >               result->controls.push_back(ctrls);
> >
> > @@ -287,14 +287,14 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
> >       if (ipaConfig.operation & RPi::IPA_CONFIG_LS_TABLE) {
> >               /* Remove any previous table, if there was one. */
> >               if (lsTable_) {
> > -                     munmap(lsTable_, MAX_LS_GRID_SIZE);
> > +                     munmap(lsTable_, RPi::MaxLsGridSize);
> >                       lsTable_ = nullptr;
> >               }
> >
> >               /* Map the LS table buffer into user space. */
> >               lsTableHandle_ = FileDescriptor(ipaConfig.data[0]);
> >               if (lsTableHandle_.isValid()) {
> > -                     lsTable_ = mmap(nullptr, MAX_LS_GRID_SIZE, PROT_READ | PROT_WRITE,
> > +                     lsTable_ = mmap(nullptr, RPi::MaxLsGridSize, PROT_READ | PROT_WRITE,
> >                                       MAP_SHARED, lsTableHandle_.fd(), 0);
> >
> >                       if (lsTable_ == MAP_FAILED) {
> > @@ -343,9 +343,9 @@ void IPARPi::processEvent(const IPAOperationData &event)
> >       case RPi::IPA_EVENT_SIGNAL_STAT_READY: {
> >               unsigned int bufferId = event.data[0];
> >
> > -             if (++check_count_ != frame_count_) /* assert here? */
> > +             if (++checkCount_ != frameCount_) /* assert here? */
> >                       LOG(IPARPI, Error) << "WARNING: Prepare/Process mismatch!!!";
> > -             if (frame_count_ > mistrust_count_)
> > +             if (frameCount_ > mistrustCount_)
> >                       processStats(bufferId);
> >
> >               reportMetadata();
> > @@ -368,7 +368,7 @@ void IPARPi::processEvent(const IPAOperationData &event)
> >                * they are "unreliable".
> >                */
> >               prepareISP(embeddedbufferId);
> > -             frame_count_++;
> > +             frameCount_++;
> >
> >               /* Ready to push the input buffer into the ISP. */
> >               IPAOperationData op;
> > @@ -713,7 +713,7 @@ void IPARPi::prepareISP(unsigned int bufferId)
> >       returnEmbeddedBuffer(bufferId);
> >
> >       if (success) {
> > -             ControlList ctrls(isp_ctrls_);
> > +             ControlList ctrls(ispCtrls_);
> >
> >               rpiMetadata_.Clear();
> >               rpiMetadata_.Set("device.status", deviceStatus);
> > @@ -785,19 +785,19 @@ bool IPARPi::parseEmbeddedData(unsigned int bufferId, struct DeviceStatus &devic
> >       if (status != RPiController::MdParser::Status::OK) {
> >               LOG(IPARPI, Error) << "Embedded Buffer parsing failed, error " << status;
> >       } else {
> > -             uint32_t exposure_lines, gain_code;
> > -             if (helper_->Parser().GetExposureLines(exposure_lines) != RPiController::MdParser::Status::OK) {
> > +             uint32_t exposureLines, gainCode;
> > +             if (helper_->Parser().GetExposureLines(exposureLines) != RPiController::MdParser::Status::OK) {
> >                       LOG(IPARPI, Error) << "Exposure time failed";
> >                       return false;
> >               }
> >
> > -             deviceStatus.shutter_speed = helper_->Exposure(exposure_lines);
> > -             if (helper_->Parser().GetGainCode(gain_code) != RPiController::MdParser::Status::OK) {
> > +             deviceStatus.shutter_speed = helper_->Exposure(exposureLines);
> > +             if (helper_->Parser().GetGainCode(gainCode) != RPiController::MdParser::Status::OK) {
> >                       LOG(IPARPI, Error) << "Gain failed";
> >                       return false;
> >               }
> >
> > -             deviceStatus.analogue_gain = helper_->Gain(gain_code);
> > +             deviceStatus.analogue_gain = helper_->Gain(gainCode);
> >               LOG(IPARPI, Debug) << "Metadata - Exposure : "
> >                                  << deviceStatus.shutter_speed << " Gain : "
> >                                  << deviceStatus.analogue_gain;
> > @@ -820,7 +820,7 @@ void IPARPi::processStats(unsigned int bufferId)
> >
> >       struct AgcStatus agcStatus;
> >       if (rpiMetadata_.Get("agc.status", agcStatus) == 0) {
> > -             ControlList ctrls(unicam_ctrls_);
> > +             ControlList ctrls(unicamCtrls_);
> >               applyAGC(&agcStatus, ctrls);
> >
> >               IPAOperationData op;
> > @@ -832,14 +832,14 @@ void IPARPi::processStats(unsigned int bufferId)
> >
> >  void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls)
> >  {
> > -     const auto gainR = isp_ctrls_.find(V4L2_CID_RED_BALANCE);
> > -     if (gainR == isp_ctrls_.end()) {
> > +     const auto gainR = ispCtrls_.find(V4L2_CID_RED_BALANCE);
> > +     if (gainR == ispCtrls_.end()) {
> >               LOG(IPARPI, Error) << "Can't find red gain control";
> >               return;
> >       }
> >
> > -     const auto gainB = isp_ctrls_.find(V4L2_CID_BLUE_BALANCE);
> > -     if (gainB == isp_ctrls_.end()) {
> > +     const auto gainB = ispCtrls_.find(V4L2_CID_BLUE_BALANCE);
> > +     if (gainB == ispCtrls_.end()) {
> >               LOG(IPARPI, Error) << "Can't find blue gain control";
> >               return;
> >       }
> > @@ -855,31 +855,31 @@ void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls)
> >
> >  void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls)
> >  {
> > -     int32_t gain_code = helper_->GainCode(agcStatus->analogue_gain);
> > -     int32_t exposure_lines = helper_->ExposureLines(agcStatus->shutter_time);
> > +     int32_t gainCode = helper_->GainCode(agcStatus->analogue_gain);
> > +     int32_t exposureLines = helper_->ExposureLines(agcStatus->shutter_time);
> >
> > -     if (unicam_ctrls_.find(V4L2_CID_ANALOGUE_GAIN) == unicam_ctrls_.end()) {
> > +     if (unicamCtrls_.find(V4L2_CID_ANALOGUE_GAIN) == unicamCtrls_.end()) {
> >               LOG(IPARPI, Error) << "Can't find analogue gain control";
> >               return;
> >       }
> >
> > -     if (unicam_ctrls_.find(V4L2_CID_EXPOSURE) == unicam_ctrls_.end()) {
> > +     if (unicamCtrls_.find(V4L2_CID_EXPOSURE) == unicamCtrls_.end()) {
> >               LOG(IPARPI, Error) << "Can't find exposure control";
> >               return;
> >       }
> >
> >       LOG(IPARPI, Debug) << "Applying AGC Exposure: " << agcStatus->shutter_time
> > -                        << " (Shutter lines: " << exposure_lines << ") Gain: "
> > +                        << " (Shutter lines: " << exposureLines << ") Gain: "
> >                          << agcStatus->analogue_gain << " (Gain Code: "
> > -                        << gain_code << ")";
> > +                        << gainCode << ")";
> >
> > -     ctrls.set(V4L2_CID_ANALOGUE_GAIN, gain_code);
> > -     ctrls.set(V4L2_CID_EXPOSURE, exposure_lines);
> > +     ctrls.set(V4L2_CID_ANALOGUE_GAIN, gainCode);
> > +     ctrls.set(V4L2_CID_EXPOSURE, exposureLines);
> >  }
> >
> >  void IPARPi::applyDG(const struct AgcStatus *dgStatus, ControlList &ctrls)
> >  {
> > -     if (isp_ctrls_.find(V4L2_CID_DIGITAL_GAIN) == isp_ctrls_.end()) {
> > +     if (ispCtrls_.find(V4L2_CID_DIGITAL_GAIN) == ispCtrls_.end()) {
> >               LOG(IPARPI, Error) << "Can't find digital gain control";
> >               return;
> >       }
> > @@ -890,7 +890,7 @@ void IPARPi::applyDG(const struct AgcStatus *dgStatus, ControlList &ctrls)
> >
> >  void IPARPi::applyCCM(const struct CcmStatus *ccmStatus, ControlList &ctrls)
> >  {
> > -     if (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_CC_MATRIX) == isp_ctrls_.end()) {
> > +     if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_CC_MATRIX) == ispCtrls_.end()) {
> >               LOG(IPARPI, Error) << "Can't find CCM control";
> >               return;
> >       }
> > @@ -911,7 +911,7 @@ void IPARPi::applyCCM(const struct CcmStatus *ccmStatus, ControlList &ctrls)
> >
> >  void IPARPi::applyGamma(const struct ContrastStatus *contrastStatus, ControlList &ctrls)
> >  {
> > -     if (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_GAMMA) == isp_ctrls_.end()) {
> > +     if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_GAMMA) == ispCtrls_.end()) {
> >               LOG(IPARPI, Error) << "Can't find Gamma control";
> >               return;
> >       }
> > @@ -930,7 +930,7 @@ void IPARPi::applyGamma(const struct ContrastStatus *contrastStatus, ControlList
> >
> >  void IPARPi::applyBlackLevel(const struct BlackLevelStatus *blackLevelStatus, ControlList &ctrls)
> >  {
> > -     if (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_BLACK_LEVEL) == isp_ctrls_.end()) {
> > +     if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_BLACK_LEVEL) == ispCtrls_.end()) {
> >               LOG(IPARPI, Error) << "Can't find black level control";
> >               return;
> >       }
> > @@ -948,7 +948,7 @@ void IPARPi::applyBlackLevel(const struct BlackLevelStatus *blackLevelStatus, Co
> >
> >  void IPARPi::applyGEQ(const struct GeqStatus *geqStatus, ControlList &ctrls)
> >  {
> > -     if (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_GEQ) == isp_ctrls_.end()) {
> > +     if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_GEQ) == ispCtrls_.end()) {
> >               LOG(IPARPI, Error) << "Can't find geq control";
> >               return;
> >       }
> > @@ -966,7 +966,7 @@ void IPARPi::applyGEQ(const struct GeqStatus *geqStatus, ControlList &ctrls)
> >
> >  void IPARPi::applyDenoise(const struct SdnStatus *denoiseStatus, ControlList &ctrls)
> >  {
> > -     if (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_DENOISE) == isp_ctrls_.end()) {
> > +     if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_DENOISE) == ispCtrls_.end()) {
> >               LOG(IPARPI, Error) << "Can't find denoise control";
> >               return;
> >       }
> > @@ -986,7 +986,7 @@ void IPARPi::applyDenoise(const struct SdnStatus *denoiseStatus, ControlList &ct
> >
> >  void IPARPi::applySharpen(const struct SharpenStatus *sharpenStatus, ControlList &ctrls)
> >  {
> > -     if (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_SHARPEN) == isp_ctrls_.end()) {
> > +     if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_SHARPEN) == ispCtrls_.end()) {
> >               LOG(IPARPI, Error) << "Can't find sharpen control";
> >               return;
> >       }
> > @@ -1007,7 +1007,7 @@ void IPARPi::applySharpen(const struct SharpenStatus *sharpenStatus, ControlList
> >
> >  void IPARPi::applyDPC(const struct DpcStatus *dpcStatus, ControlList &ctrls)
> >  {
> > -     if (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_DPC) == isp_ctrls_.end()) {
> > +     if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_DPC) == ispCtrls_.end()) {
> >               LOG(IPARPI, Error) << "Can't find DPC control";
> >               return;
> >       }
> > @@ -1023,7 +1023,7 @@ void IPARPi::applyDPC(const struct DpcStatus *dpcStatus, ControlList &ctrls)
> >
> >  void IPARPi::applyLS(const struct AlscStatus *lsStatus, ControlList &ctrls)
> >  {
> > -     if (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING) == isp_ctrls_.end()) {
> > +     if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING) == ispCtrls_.end()) {
> >               LOG(IPARPI, Error) << "Can't find LS control";
> >               return;
> >       }
> > @@ -1032,18 +1032,18 @@ void IPARPi::applyLS(const struct AlscStatus *lsStatus, ControlList &ctrls)
> >        * Program lens shading tables into pipeline.
> >        * Choose smallest cell size that won't exceed 63x48 cells.
> >        */
> > -     const int cell_sizes[] = { 16, 32, 64, 128, 256 };
> > -     unsigned int num_cells = ARRAY_SIZE(cell_sizes);
> > -     unsigned int i, w, h, cell_size;
> > -     for (i = 0; i < num_cells; i++) {
> > -             cell_size = cell_sizes[i];
> > -             w = (mode_.width + cell_size - 1) / cell_size;
> > -             h = (mode_.height + cell_size - 1) / cell_size;
> > +     const int cellSizes[] = { 16, 32, 64, 128, 256 };
> > +     unsigned int numCells = ARRAY_SIZE(cellSizes);
> > +     unsigned int i, w, h, cellSize;
> > +     for (i = 0; i < numCells; i++) {
> > +             cellSize = cellSizes[i];
> > +             w = (mode_.width + cellSize - 1) / cellSize;
> > +             h = (mode_.height + cellSize - 1) / cellSize;
> >               if (w < 64 && h <= 48)
> >                       break;
> >       }
> >
> > -     if (i == num_cells) {
> > +     if (i == numCells) {
> >               LOG(IPARPI, Error) << "Cannot find cell size";
> >               return;
> >       }
> > @@ -1052,7 +1052,7 @@ void IPARPi::applyLS(const struct AlscStatus *lsStatus, ControlList &ctrls)
> >       w++, h++;
> >       bcm2835_isp_lens_shading ls = {
> >               .enabled = 1,
> > -             .grid_cell_size = cell_size,
> > +             .grid_cell_size = cellSize,
> >               .grid_width = w,
> >               .grid_stride = w,
> >               .grid_height = h,
> > @@ -1062,7 +1062,7 @@ void IPARPi::applyLS(const struct AlscStatus *lsStatus, ControlList &ctrls)
> >               .gain_format = GAIN_FORMAT_U4P10
> >       };
> >
> > -     if (!lsTable_ || w * h * 4 * sizeof(uint16_t) > MAX_LS_GRID_SIZE) {
> > +     if (!lsTable_ || w * h * 4 * sizeof(uint16_t) > RPi::MaxLsGridSize) {
> >               LOG(IPARPI, Error) << "Do not have a correctly allocate lens shading table!";
> >               return;
> >       }
> > @@ -1083,41 +1083,41 @@ void IPARPi::applyLS(const struct AlscStatus *lsStatus, ControlList &ctrls)
> >  }
> >
> >  /*
> > - * Resamples a 16x12 table with central sampling to dest_w x dest_h with corner
> > + * Resamples a 16x12 table with central sampling to destW x destH with corner
> >   * sampling.
> >   */
> >  void IPARPi::resampleTable(uint16_t dest[], double const src[12][16],
> > -                        int dest_w, int dest_h)
> > +                        int destW, int destH)
> >  {
> >       /*
> >        * Precalculate and cache the x sampling locations and phases to
> >        * save recomputing them on every row.
> >        */
> > -     assert(dest_w > 1 && dest_h > 1 && dest_w <= 64);
> > -     int x_lo[64], x_hi[64];
> > +     assert(destW > 1 && destH > 1 && destW <= 64);
> > +     int xLo[64], xHi[64];
> >       double xf[64];
> > -     double x = -0.5, x_inc = 16.0 / (dest_w - 1);
> > -     for (int i = 0; i < dest_w; i++, x += x_inc) {
> > -             x_lo[i] = floor(x);
> > -             xf[i] = x - x_lo[i];
> > -             x_hi[i] = x_lo[i] < 15 ? x_lo[i] + 1 : 15;
> > -             x_lo[i] = x_lo[i] > 0 ? x_lo[i] : 0;
> > +     double x = -0.5, xInc = 16.0 / (destW - 1);
> > +     for (int i = 0; i < destW; i++, x += xInc) {
> > +             xLo[i] = floor(x);
> > +             xf[i] = x - xLo[i];
> > +             xHi[i] = xLo[i] < 15 ? xLo[i] + 1 : 15;
> > +             xLo[i] = xLo[i] > 0 ? xLo[i] : 0;
> >       }
> >
> >       /* Now march over the output table generating the new values. */
> > -     double y = -0.5, y_inc = 12.0 / (dest_h - 1);
> > -     for (int j = 0; j < dest_h; j++, y += y_inc) {
> > -             int y_lo = floor(y);
> > -             double yf = y - y_lo;
> > -             int y_hi = y_lo < 11 ? y_lo + 1 : 11;
> > -             y_lo = y_lo > 0 ? y_lo : 0;
> > -             double const *row_above = src[y_lo];
> > -             double const *row_below = src[y_hi];
> > -             for (int i = 0; i < dest_w; i++) {
> > -                     double above = row_above[x_lo[i]] * (1 - xf[i])
> > -                                  + row_above[x_hi[i]] * xf[i];
> > -                     double below = row_below[x_lo[i]] * (1 - xf[i])
> > -                                  + row_below[x_hi[i]] * xf[i];
> > +     double y = -0.5, yInc = 12.0 / (destH - 1);
> > +     for (int j = 0; j < destH; j++, y += yInc) {
> > +             int yLo = floor(y);
> > +             double yf = y - yLo;
> > +             int yHi = yLo < 11 ? yLo + 1 : 11;
> > +             yLo = yLo > 0 ? yLo : 0;
> > +             double const *rowAbove = src[yLo];
> > +             double const *rowBelow = src[yHi];
> > +             for (int i = 0; i < destW; i++) {
> > +                     double above = rowAbove[xLo[i]] * (1 - xf[i])
> > +                                  + rowAbove[xHi[i]] * xf[i];
> > +                     double below = rowBelow[xLo[i]] * (1 - xf[i])
> > +                                  + rowBelow[xHi[i]] * xf[i];
> >                       int result = floor(1024 * (above * (1 - yf) + below * yf) + .5);
> >                       *(dest++) = result > 16383 ? 16383 : result; /* want u4.10 */
> >               }
> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > index 35dbe0fb..8d40b0ed 100644
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > @@ -1012,7 +1012,7 @@ int RPiCameraData::configureIPA()
> >
> >       /* Allocate the lens shading table via dmaHeap and pass to the IPA. */
> >       if (!lsTable_.isValid()) {
> > -             lsTable_ = dmaHeap_.alloc("ls_grid", MAX_LS_GRID_SIZE);
> > +             lsTable_ = dmaHeap_.alloc("ls_grid", RPi::MaxLsGridSize);
>
> If we had an IPA namespace, it would be nice to see this as:
>
>   lsTable_ = dmaHeap_.alloc("ls_grid", ipa::RPi::MaxLsGridSize);
>
> As that would make it really clear 'where' this MaxLsGridSize value was
> coming from...
>
> Anyway, all my comments above are optional discussion points.
>
> For the patch:
>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>
> >               if (!lsTable_.isValid())
> >                       return -ENOMEM;
> >
> >
>
> --
> Regards
> --
> Kieran
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Kieran Bingham Sept. 23, 2020, 7:53 p.m. UTC | #6
Hi David, Naush,

On 23/09/2020 17:31, David Plowman wrote:
> Hi Naush
> 
> Thanks for doing all this tidying! One little nit-pick...
> 
> On Wed, 23 Sep 2020 at 12:18, Kieran Bingham
> <kieran.bingham@ideasonboard.com> wrote:
>>
>> Hi Naush,
>>
>> On 22/09/2020 10:50, Naushir Patuck wrote:
>>> Change variable names to camel case to be consistent with the rest of
>>> the source files. Remove #define consts and replace with constexpr.
>>>
>>
>> Sounds good to me...
>>
>>
>>> There are no functional changes in this commit.
>>>
>>> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
>>> ---
>>>  include/libcamera/ipa/raspberrypi.h           |   2 +-
>>>  src/ipa/raspberrypi/raspberrypi.cpp           | 178 +++++++++---------
>>>  .../pipeline/raspberrypi/raspberrypi.cpp      |   2 +-
>>>  3 files changed, 91 insertions(+), 91 deletions(-)
>>>
>>> diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h
>>> index c9d4aa81..b3041591 100644
>>> --- a/include/libcamera/ipa/raspberrypi.h
>>> +++ b/include/libcamera/ipa/raspberrypi.h
>>> @@ -41,7 +41,7 @@ enum BufferMask {
>>>  };
>>>
>>>  /* Size of the LS grid allocation. */
>>> -#define MAX_LS_GRID_SIZE (32 << 10)
>>> +static constexpr unsigned int MaxLsGridSize = 32 << 10;
>>
>> I guess the LS could stay capitalised (MaxLSGridSize), not sure that it
>> matters, so up to you.
>>
>>>  /* List of controls handled by the Raspberry Pi IPA */
>>>  static const ControlInfoMap Controls = {
>>> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
>>> index 0c0dc743..c240eae8 100644
>>> --- a/src/ipa/raspberrypi/raspberrypi.cpp
>>> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
>>> @@ -55,8 +55,8 @@
>>>  namespace libcamera {
>>>
>>>  /* Configure the sensor with these values initially. */
>>> -#define DEFAULT_ANALOGUE_GAIN 1.0
>>> -#define DEFAULT_EXPOSURE_TIME 20000
>>> +constexpr unsigned int DefaultAnalogueGain = 1.0;
>>> +constexpr unsigned int DefaultExposureTime = 20000;
>>
>> Oh nice, This really highlights to me the benefit of constexpr.
>> Now the values have types!
> 
> Should DefaultAnalogueGain be a double? Of course it gets cast to a
> double when it gets copied into agcStatus.analogue_gain so it probably
> contrives to make no difference, but a double might be clearer,
> especially if anyone ever changed it.

Boom, and there's the types! I was so keen on seeing the types, and
noticing the 1.0 - I didn't even notice that it was set as an unsigned
int. :-S

Uhm... What I mean is - I agree with David, and I'm disappointed with
myself ;-)

--
Kieran

> Thanks!
> David
> 
>>
>>>
>>>  LOG_DEFINE_CATEGORY(IPARPI)
>>>
>>> @@ -65,7 +65,7 @@ class IPARPi : public IPAInterface
>>>  public:
>>>       IPARPi()
>>>               : lastMode_({}), controller_(), controllerInit_(false),
>>> -               frame_count_(0), check_count_(0), mistrust_count_(0),
>>> +               frameCount_(0), checkCount_(0), mistrustCount_(0),
>>>                 lsTable_(nullptr)
>>>       {
>>>       }
>>> @@ -73,7 +73,7 @@ public:
>>>       ~IPARPi()
>>>       {
>>>               if (lsTable_)
>>> -                     munmap(lsTable_, MAX_LS_GRID_SIZE);
>>> +                     munmap(lsTable_, RPi::MaxLsGridSize);
>>>       }
>>>
>>>       int init(const IPASettings &settings) override;
>>> @@ -108,13 +108,13 @@ private:
>>>       void applySharpen(const struct SharpenStatus *sharpenStatus, ControlList &ctrls);
>>>       void applyDPC(const struct DpcStatus *dpcStatus, ControlList &ctrls);
>>>       void applyLS(const struct AlscStatus *lsStatus, ControlList &ctrls);
>>> -     void resampleTable(uint16_t dest[], double const src[12][16], int dest_w, int dest_h);
>>> +     void resampleTable(uint16_t dest[], double const src[12][16], int destW, int destH);
>>>
>>>       std::map<unsigned int, FrameBuffer> buffers_;
>>>       std::map<unsigned int, void *> buffersMemory_;
>>>
>>> -     ControlInfoMap unicam_ctrls_;
>>> -     ControlInfoMap isp_ctrls_;
>>> +     ControlInfoMap unicamCtrls_;
>>> +     ControlInfoMap ispCtrls_;
>>>       ControlList libcameraMetadata_;
>>>
>>>       /* IPA configuration. */
>>> @@ -134,11 +134,11 @@ private:
>>>        * We count frames to decide if the frame must be hidden (e.g. from
>>>        * display) or mistrusted (i.e. not given to the control algos).
>>>        */
>>> -     uint64_t frame_count_;
>>> +     uint64_t frameCount_;
>>
>> If we're doing clean up - I always prefer to see a newline before a
>> comment line.
>>
>> To me it makes the commented block clearer - but I don't know if there's
>> a 'rule' on it.
>>
>>
>>>       /* For checking the sequencing of Prepare/Process calls. */
>>> -     uint64_t check_count_;
>>> +     uint64_t checkCount_;
>>
>> i.e. here too.
>>
>>>       /* How many frames we should avoid running control algos on. */
>>> -     unsigned int mistrust_count_;
>>> +     unsigned int mistrustCount_;
>>
>> and so on.
>>
>>>       /* LS table allocation passed in from the pipeline handler. */
>>>       FileDescriptor lsTableHandle_;
>>>       void *lsTable_;
>>> @@ -199,8 +199,8 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
>>>
>>>       result->operation = 0;
>>>
>>> -     unicam_ctrls_ = entityControls.at(0);
>>> -     isp_ctrls_ = entityControls.at(1);
>>> +     unicamCtrls_ = entityControls.at(0);
>>> +     ispCtrls_ = entityControls.at(1);
>>>       /* Setup a metadata ControlList to output metadata. */
>>>       libcameraMetadata_ = ControlList(controls::controls);
>>>
>>> @@ -238,18 +238,18 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
>>>        *"mistrusted", which depends on whether this is a startup from cold,
>>>        * or merely a mode switch in a running system.
>>>        */
>>> -     frame_count_ = 0;
>>> -     check_count_ = 0;
>>> -     unsigned int drop_frame = 0;
>>> +     frameCount_ = 0;
>>> +     checkCount_ = 0;
>>> +     unsigned int dropFrame = 0;
>>>       if (controllerInit_) {
>>> -             drop_frame = helper_->HideFramesModeSwitch();
>>> -             mistrust_count_ = helper_->MistrustFramesModeSwitch();
>>> +             dropFrame = helper_->HideFramesModeSwitch();
>>> +             mistrustCount_ = helper_->MistrustFramesModeSwitch();
>>>       } else {
>>> -             drop_frame = helper_->HideFramesStartup();
>>> -             mistrust_count_ = helper_->MistrustFramesStartup();
>>> +             dropFrame = helper_->HideFramesStartup();
>>> +             mistrustCount_ = helper_->MistrustFramesStartup();
>>>       }
>>>
>>> -     result->data.push_back(drop_frame);
>>> +     result->data.push_back(dropFrame);
>>>       result->operation |= RPi::IPA_CONFIG_DROP_FRAMES;
>>>
>>>       struct AgcStatus agcStatus;
>>> @@ -264,8 +264,8 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
>>>               controllerInit_ = true;
>>>
>>>               /* Supply initial values for gain and exposure. */
>>> -             agcStatus.shutter_time = DEFAULT_EXPOSURE_TIME;
>>> -             agcStatus.analogue_gain = DEFAULT_ANALOGUE_GAIN;
>>> +             agcStatus.shutter_time = DefaultExposureTime;
>>> +             agcStatus.analogue_gain = DefaultAnalogueGain;
>>>       }
>>>
>>>       RPiController::Metadata metadata;
>>> @@ -274,7 +274,7 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
>>>       /* SwitchMode may supply updated exposure/gain values to use. */
>>>       metadata.Get("agc.status", agcStatus);
>>>       if (agcStatus.shutter_time != 0.0 && agcStatus.analogue_gain != 0.0) {
>>> -             ControlList ctrls(unicam_ctrls_);
>>> +             ControlList ctrls(unicamCtrls_);
>>>               applyAGC(&agcStatus, ctrls);
>>>               result->controls.push_back(ctrls);
>>>
>>> @@ -287,14 +287,14 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
>>>       if (ipaConfig.operation & RPi::IPA_CONFIG_LS_TABLE) {
>>>               /* Remove any previous table, if there was one. */
>>>               if (lsTable_) {
>>> -                     munmap(lsTable_, MAX_LS_GRID_SIZE);
>>> +                     munmap(lsTable_, RPi::MaxLsGridSize);
>>>                       lsTable_ = nullptr;
>>>               }
>>>
>>>               /* Map the LS table buffer into user space. */
>>>               lsTableHandle_ = FileDescriptor(ipaConfig.data[0]);
>>>               if (lsTableHandle_.isValid()) {
>>> -                     lsTable_ = mmap(nullptr, MAX_LS_GRID_SIZE, PROT_READ | PROT_WRITE,
>>> +                     lsTable_ = mmap(nullptr, RPi::MaxLsGridSize, PROT_READ | PROT_WRITE,
>>>                                       MAP_SHARED, lsTableHandle_.fd(), 0);
>>>
>>>                       if (lsTable_ == MAP_FAILED) {
>>> @@ -343,9 +343,9 @@ void IPARPi::processEvent(const IPAOperationData &event)
>>>       case RPi::IPA_EVENT_SIGNAL_STAT_READY: {
>>>               unsigned int bufferId = event.data[0];
>>>
>>> -             if (++check_count_ != frame_count_) /* assert here? */
>>> +             if (++checkCount_ != frameCount_) /* assert here? */
>>>                       LOG(IPARPI, Error) << "WARNING: Prepare/Process mismatch!!!";
>>> -             if (frame_count_ > mistrust_count_)
>>> +             if (frameCount_ > mistrustCount_)
>>>                       processStats(bufferId);
>>>
>>>               reportMetadata();
>>> @@ -368,7 +368,7 @@ void IPARPi::processEvent(const IPAOperationData &event)
>>>                * they are "unreliable".
>>>                */
>>>               prepareISP(embeddedbufferId);
>>> -             frame_count_++;
>>> +             frameCount_++;
>>>
>>>               /* Ready to push the input buffer into the ISP. */
>>>               IPAOperationData op;
>>> @@ -713,7 +713,7 @@ void IPARPi::prepareISP(unsigned int bufferId)
>>>       returnEmbeddedBuffer(bufferId);
>>>
>>>       if (success) {
>>> -             ControlList ctrls(isp_ctrls_);
>>> +             ControlList ctrls(ispCtrls_);
>>>
>>>               rpiMetadata_.Clear();
>>>               rpiMetadata_.Set("device.status", deviceStatus);
>>> @@ -785,19 +785,19 @@ bool IPARPi::parseEmbeddedData(unsigned int bufferId, struct DeviceStatus &devic
>>>       if (status != RPiController::MdParser::Status::OK) {
>>>               LOG(IPARPI, Error) << "Embedded Buffer parsing failed, error " << status;
>>>       } else {
>>> -             uint32_t exposure_lines, gain_code;
>>> -             if (helper_->Parser().GetExposureLines(exposure_lines) != RPiController::MdParser::Status::OK) {
>>> +             uint32_t exposureLines, gainCode;
>>> +             if (helper_->Parser().GetExposureLines(exposureLines) != RPiController::MdParser::Status::OK) {
>>>                       LOG(IPARPI, Error) << "Exposure time failed";
>>>                       return false;
>>>               }
>>>
>>> -             deviceStatus.shutter_speed = helper_->Exposure(exposure_lines);
>>> -             if (helper_->Parser().GetGainCode(gain_code) != RPiController::MdParser::Status::OK) {
>>> +             deviceStatus.shutter_speed = helper_->Exposure(exposureLines);
>>> +             if (helper_->Parser().GetGainCode(gainCode) != RPiController::MdParser::Status::OK) {
>>>                       LOG(IPARPI, Error) << "Gain failed";
>>>                       return false;
>>>               }
>>>
>>> -             deviceStatus.analogue_gain = helper_->Gain(gain_code);
>>> +             deviceStatus.analogue_gain = helper_->Gain(gainCode);
>>>               LOG(IPARPI, Debug) << "Metadata - Exposure : "
>>>                                  << deviceStatus.shutter_speed << " Gain : "
>>>                                  << deviceStatus.analogue_gain;
>>> @@ -820,7 +820,7 @@ void IPARPi::processStats(unsigned int bufferId)
>>>
>>>       struct AgcStatus agcStatus;
>>>       if (rpiMetadata_.Get("agc.status", agcStatus) == 0) {
>>> -             ControlList ctrls(unicam_ctrls_);
>>> +             ControlList ctrls(unicamCtrls_);
>>>               applyAGC(&agcStatus, ctrls);
>>>
>>>               IPAOperationData op;
>>> @@ -832,14 +832,14 @@ void IPARPi::processStats(unsigned int bufferId)
>>>
>>>  void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls)
>>>  {
>>> -     const auto gainR = isp_ctrls_.find(V4L2_CID_RED_BALANCE);
>>> -     if (gainR == isp_ctrls_.end()) {
>>> +     const auto gainR = ispCtrls_.find(V4L2_CID_RED_BALANCE);
>>> +     if (gainR == ispCtrls_.end()) {
>>>               LOG(IPARPI, Error) << "Can't find red gain control";
>>>               return;
>>>       }
>>>
>>> -     const auto gainB = isp_ctrls_.find(V4L2_CID_BLUE_BALANCE);
>>> -     if (gainB == isp_ctrls_.end()) {
>>> +     const auto gainB = ispCtrls_.find(V4L2_CID_BLUE_BALANCE);
>>> +     if (gainB == ispCtrls_.end()) {
>>>               LOG(IPARPI, Error) << "Can't find blue gain control";
>>>               return;
>>>       }
>>> @@ -855,31 +855,31 @@ void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls)
>>>
>>>  void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls)
>>>  {
>>> -     int32_t gain_code = helper_->GainCode(agcStatus->analogue_gain);
>>> -     int32_t exposure_lines = helper_->ExposureLines(agcStatus->shutter_time);
>>> +     int32_t gainCode = helper_->GainCode(agcStatus->analogue_gain);
>>> +     int32_t exposureLines = helper_->ExposureLines(agcStatus->shutter_time);
>>>
>>> -     if (unicam_ctrls_.find(V4L2_CID_ANALOGUE_GAIN) == unicam_ctrls_.end()) {
>>> +     if (unicamCtrls_.find(V4L2_CID_ANALOGUE_GAIN) == unicamCtrls_.end()) {
>>>               LOG(IPARPI, Error) << "Can't find analogue gain control";
>>>               return;
>>>       }
>>>
>>> -     if (unicam_ctrls_.find(V4L2_CID_EXPOSURE) == unicam_ctrls_.end()) {
>>> +     if (unicamCtrls_.find(V4L2_CID_EXPOSURE) == unicamCtrls_.end()) {
>>>               LOG(IPARPI, Error) << "Can't find exposure control";
>>>               return;
>>>       }
>>>
>>>       LOG(IPARPI, Debug) << "Applying AGC Exposure: " << agcStatus->shutter_time
>>> -                        << " (Shutter lines: " << exposure_lines << ") Gain: "
>>> +                        << " (Shutter lines: " << exposureLines << ") Gain: "
>>>                          << agcStatus->analogue_gain << " (Gain Code: "
>>> -                        << gain_code << ")";
>>> +                        << gainCode << ")";
>>>
>>> -     ctrls.set(V4L2_CID_ANALOGUE_GAIN, gain_code);
>>> -     ctrls.set(V4L2_CID_EXPOSURE, exposure_lines);
>>> +     ctrls.set(V4L2_CID_ANALOGUE_GAIN, gainCode);
>>> +     ctrls.set(V4L2_CID_EXPOSURE, exposureLines);
>>>  }
>>>
>>>  void IPARPi::applyDG(const struct AgcStatus *dgStatus, ControlList &ctrls)
>>>  {
>>> -     if (isp_ctrls_.find(V4L2_CID_DIGITAL_GAIN) == isp_ctrls_.end()) {
>>> +     if (ispCtrls_.find(V4L2_CID_DIGITAL_GAIN) == ispCtrls_.end()) {
>>>               LOG(IPARPI, Error) << "Can't find digital gain control";
>>>               return;
>>>       }
>>> @@ -890,7 +890,7 @@ void IPARPi::applyDG(const struct AgcStatus *dgStatus, ControlList &ctrls)
>>>
>>>  void IPARPi::applyCCM(const struct CcmStatus *ccmStatus, ControlList &ctrls)
>>>  {
>>> -     if (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_CC_MATRIX) == isp_ctrls_.end()) {
>>> +     if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_CC_MATRIX) == ispCtrls_.end()) {
>>>               LOG(IPARPI, Error) << "Can't find CCM control";
>>>               return;
>>>       }
>>> @@ -911,7 +911,7 @@ void IPARPi::applyCCM(const struct CcmStatus *ccmStatus, ControlList &ctrls)
>>>
>>>  void IPARPi::applyGamma(const struct ContrastStatus *contrastStatus, ControlList &ctrls)
>>>  {
>>> -     if (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_GAMMA) == isp_ctrls_.end()) {
>>> +     if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_GAMMA) == ispCtrls_.end()) {
>>>               LOG(IPARPI, Error) << "Can't find Gamma control";
>>>               return;
>>>       }
>>> @@ -930,7 +930,7 @@ void IPARPi::applyGamma(const struct ContrastStatus *contrastStatus, ControlList
>>>
>>>  void IPARPi::applyBlackLevel(const struct BlackLevelStatus *blackLevelStatus, ControlList &ctrls)
>>>  {
>>> -     if (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_BLACK_LEVEL) == isp_ctrls_.end()) {
>>> +     if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_BLACK_LEVEL) == ispCtrls_.end()) {
>>>               LOG(IPARPI, Error) << "Can't find black level control";
>>>               return;
>>>       }
>>> @@ -948,7 +948,7 @@ void IPARPi::applyBlackLevel(const struct BlackLevelStatus *blackLevelStatus, Co
>>>
>>>  void IPARPi::applyGEQ(const struct GeqStatus *geqStatus, ControlList &ctrls)
>>>  {
>>> -     if (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_GEQ) == isp_ctrls_.end()) {
>>> +     if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_GEQ) == ispCtrls_.end()) {
>>>               LOG(IPARPI, Error) << "Can't find geq control";
>>>               return;
>>>       }
>>> @@ -966,7 +966,7 @@ void IPARPi::applyGEQ(const struct GeqStatus *geqStatus, ControlList &ctrls)
>>>
>>>  void IPARPi::applyDenoise(const struct SdnStatus *denoiseStatus, ControlList &ctrls)
>>>  {
>>> -     if (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_DENOISE) == isp_ctrls_.end()) {
>>> +     if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_DENOISE) == ispCtrls_.end()) {
>>>               LOG(IPARPI, Error) << "Can't find denoise control";
>>>               return;
>>>       }
>>> @@ -986,7 +986,7 @@ void IPARPi::applyDenoise(const struct SdnStatus *denoiseStatus, ControlList &ct
>>>
>>>  void IPARPi::applySharpen(const struct SharpenStatus *sharpenStatus, ControlList &ctrls)
>>>  {
>>> -     if (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_SHARPEN) == isp_ctrls_.end()) {
>>> +     if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_SHARPEN) == ispCtrls_.end()) {
>>>               LOG(IPARPI, Error) << "Can't find sharpen control";
>>>               return;
>>>       }
>>> @@ -1007,7 +1007,7 @@ void IPARPi::applySharpen(const struct SharpenStatus *sharpenStatus, ControlList
>>>
>>>  void IPARPi::applyDPC(const struct DpcStatus *dpcStatus, ControlList &ctrls)
>>>  {
>>> -     if (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_DPC) == isp_ctrls_.end()) {
>>> +     if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_DPC) == ispCtrls_.end()) {
>>>               LOG(IPARPI, Error) << "Can't find DPC control";
>>>               return;
>>>       }
>>> @@ -1023,7 +1023,7 @@ void IPARPi::applyDPC(const struct DpcStatus *dpcStatus, ControlList &ctrls)
>>>
>>>  void IPARPi::applyLS(const struct AlscStatus *lsStatus, ControlList &ctrls)
>>>  {
>>> -     if (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING) == isp_ctrls_.end()) {
>>> +     if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING) == ispCtrls_.end()) {
>>>               LOG(IPARPI, Error) << "Can't find LS control";
>>>               return;
>>>       }
>>> @@ -1032,18 +1032,18 @@ void IPARPi::applyLS(const struct AlscStatus *lsStatus, ControlList &ctrls)
>>>        * Program lens shading tables into pipeline.
>>>        * Choose smallest cell size that won't exceed 63x48 cells.
>>>        */
>>> -     const int cell_sizes[] = { 16, 32, 64, 128, 256 };
>>> -     unsigned int num_cells = ARRAY_SIZE(cell_sizes);
>>> -     unsigned int i, w, h, cell_size;
>>> -     for (i = 0; i < num_cells; i++) {
>>> -             cell_size = cell_sizes[i];
>>> -             w = (mode_.width + cell_size - 1) / cell_size;
>>> -             h = (mode_.height + cell_size - 1) / cell_size;
>>> +     const int cellSizes[] = { 16, 32, 64, 128, 256 };
>>> +     unsigned int numCells = ARRAY_SIZE(cellSizes);
>>> +     unsigned int i, w, h, cellSize;
>>> +     for (i = 0; i < numCells; i++) {
>>> +             cellSize = cellSizes[i];
>>> +             w = (mode_.width + cellSize - 1) / cellSize;
>>> +             h = (mode_.height + cellSize - 1) / cellSize;
>>>               if (w < 64 && h <= 48)
>>>                       break;
>>>       }
>>>
>>> -     if (i == num_cells) {
>>> +     if (i == numCells) {
>>>               LOG(IPARPI, Error) << "Cannot find cell size";
>>>               return;
>>>       }
>>> @@ -1052,7 +1052,7 @@ void IPARPi::applyLS(const struct AlscStatus *lsStatus, ControlList &ctrls)
>>>       w++, h++;
>>>       bcm2835_isp_lens_shading ls = {
>>>               .enabled = 1,
>>> -             .grid_cell_size = cell_size,
>>> +             .grid_cell_size = cellSize,
>>>               .grid_width = w,
>>>               .grid_stride = w,
>>>               .grid_height = h,
>>> @@ -1062,7 +1062,7 @@ void IPARPi::applyLS(const struct AlscStatus *lsStatus, ControlList &ctrls)
>>>               .gain_format = GAIN_FORMAT_U4P10
>>>       };
>>>
>>> -     if (!lsTable_ || w * h * 4 * sizeof(uint16_t) > MAX_LS_GRID_SIZE) {
>>> +     if (!lsTable_ || w * h * 4 * sizeof(uint16_t) > RPi::MaxLsGridSize) {
>>>               LOG(IPARPI, Error) << "Do not have a correctly allocate lens shading table!";
>>>               return;
>>>       }
>>> @@ -1083,41 +1083,41 @@ void IPARPi::applyLS(const struct AlscStatus *lsStatus, ControlList &ctrls)
>>>  }
>>>
>>>  /*
>>> - * Resamples a 16x12 table with central sampling to dest_w x dest_h with corner
>>> + * Resamples a 16x12 table with central sampling to destW x destH with corner
>>>   * sampling.
>>>   */
>>>  void IPARPi::resampleTable(uint16_t dest[], double const src[12][16],
>>> -                        int dest_w, int dest_h)
>>> +                        int destW, int destH)
>>>  {
>>>       /*
>>>        * Precalculate and cache the x sampling locations and phases to
>>>        * save recomputing them on every row.
>>>        */
>>> -     assert(dest_w > 1 && dest_h > 1 && dest_w <= 64);
>>> -     int x_lo[64], x_hi[64];
>>> +     assert(destW > 1 && destH > 1 && destW <= 64);
>>> +     int xLo[64], xHi[64];
>>>       double xf[64];
>>> -     double x = -0.5, x_inc = 16.0 / (dest_w - 1);
>>> -     for (int i = 0; i < dest_w; i++, x += x_inc) {
>>> -             x_lo[i] = floor(x);
>>> -             xf[i] = x - x_lo[i];
>>> -             x_hi[i] = x_lo[i] < 15 ? x_lo[i] + 1 : 15;
>>> -             x_lo[i] = x_lo[i] > 0 ? x_lo[i] : 0;
>>> +     double x = -0.5, xInc = 16.0 / (destW - 1);
>>> +     for (int i = 0; i < destW; i++, x += xInc) {
>>> +             xLo[i] = floor(x);
>>> +             xf[i] = x - xLo[i];
>>> +             xHi[i] = xLo[i] < 15 ? xLo[i] + 1 : 15;
>>> +             xLo[i] = xLo[i] > 0 ? xLo[i] : 0;
>>>       }
>>>
>>>       /* Now march over the output table generating the new values. */
>>> -     double y = -0.5, y_inc = 12.0 / (dest_h - 1);
>>> -     for (int j = 0; j < dest_h; j++, y += y_inc) {
>>> -             int y_lo = floor(y);
>>> -             double yf = y - y_lo;
>>> -             int y_hi = y_lo < 11 ? y_lo + 1 : 11;
>>> -             y_lo = y_lo > 0 ? y_lo : 0;
>>> -             double const *row_above = src[y_lo];
>>> -             double const *row_below = src[y_hi];
>>> -             for (int i = 0; i < dest_w; i++) {
>>> -                     double above = row_above[x_lo[i]] * (1 - xf[i])
>>> -                                  + row_above[x_hi[i]] * xf[i];
>>> -                     double below = row_below[x_lo[i]] * (1 - xf[i])
>>> -                                  + row_below[x_hi[i]] * xf[i];
>>> +     double y = -0.5, yInc = 12.0 / (destH - 1);
>>> +     for (int j = 0; j < destH; j++, y += yInc) {
>>> +             int yLo = floor(y);
>>> +             double yf = y - yLo;
>>> +             int yHi = yLo < 11 ? yLo + 1 : 11;
>>> +             yLo = yLo > 0 ? yLo : 0;
>>> +             double const *rowAbove = src[yLo];
>>> +             double const *rowBelow = src[yHi];
>>> +             for (int i = 0; i < destW; i++) {
>>> +                     double above = rowAbove[xLo[i]] * (1 - xf[i])
>>> +                                  + rowAbove[xHi[i]] * xf[i];
>>> +                     double below = rowBelow[xLo[i]] * (1 - xf[i])
>>> +                                  + rowBelow[xHi[i]] * xf[i];
>>>                       int result = floor(1024 * (above * (1 - yf) + below * yf) + .5);
>>>                       *(dest++) = result > 16383 ? 16383 : result; /* want u4.10 */
>>>               }
>>> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
>>> index 35dbe0fb..8d40b0ed 100644
>>> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
>>> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
>>> @@ -1012,7 +1012,7 @@ int RPiCameraData::configureIPA()
>>>
>>>       /* Allocate the lens shading table via dmaHeap and pass to the IPA. */
>>>       if (!lsTable_.isValid()) {
>>> -             lsTable_ = dmaHeap_.alloc("ls_grid", MAX_LS_GRID_SIZE);
>>> +             lsTable_ = dmaHeap_.alloc("ls_grid", RPi::MaxLsGridSize);
>>
>> If we had an IPA namespace, it would be nice to see this as:
>>
>>   lsTable_ = dmaHeap_.alloc("ls_grid", ipa::RPi::MaxLsGridSize);
>>
>> As that would make it really clear 'where' this MaxLsGridSize value was
>> coming from...
>>
>> Anyway, all my comments above are optional discussion points.
>>
>> For the patch:
>>
>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>>
>>>               if (!lsTable_.isValid())
>>>                       return -ENOMEM;
>>>
>>>
>>
>> --
>> Regards
>> --
>> Kieran
>> _______________________________________________
>> libcamera-devel mailing list
>> libcamera-devel@lists.libcamera.org
>> https://lists.libcamera.org/listinfo/libcamera-devel
Naushir Patuck Sept. 24, 2020, 8:34 a.m. UTC | #7
Hi David,

Thank you for the comments.

On Wed, 23 Sep 2020 at 17:31, David Plowman
<david.plowman@raspberrypi.com> wrote:
>
> Hi Naush
>
> Thanks for doing all this tidying! One little nit-pick...
>
> On Wed, 23 Sep 2020 at 12:18, Kieran Bingham
> <kieran.bingham@ideasonboard.com> wrote:
> >
> > Hi Naush,
> >
> > On 22/09/2020 10:50, Naushir Patuck wrote:
> > > Change variable names to camel case to be consistent with the rest of
> > > the source files. Remove #define consts and replace with constexpr.
> > >
> >
> > Sounds good to me...
> >
> >
> > > There are no functional changes in this commit.
> > >
> > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > > ---
> > >  include/libcamera/ipa/raspberrypi.h           |   2 +-
> > >  src/ipa/raspberrypi/raspberrypi.cpp           | 178 +++++++++---------
> > >  .../pipeline/raspberrypi/raspberrypi.cpp      |   2 +-
> > >  3 files changed, 91 insertions(+), 91 deletions(-)
> > >
> > > diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h
> > > index c9d4aa81..b3041591 100644
> > > --- a/include/libcamera/ipa/raspberrypi.h
> > > +++ b/include/libcamera/ipa/raspberrypi.h
> > > @@ -41,7 +41,7 @@ enum BufferMask {
> > >  };
> > >
> > >  /* Size of the LS grid allocation. */
> > > -#define MAX_LS_GRID_SIZE (32 << 10)
> > > +static constexpr unsigned int MaxLsGridSize = 32 << 10;
> >
> > I guess the LS could stay capitalised (MaxLSGridSize), not sure that it
> > matters, so up to you.
> >
> > >  /* List of controls handled by the Raspberry Pi IPA */
> > >  static const ControlInfoMap Controls = {
> > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> > > index 0c0dc743..c240eae8 100644
> > > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > > @@ -55,8 +55,8 @@
> > >  namespace libcamera {
> > >
> > >  /* Configure the sensor with these values initially. */
> > > -#define DEFAULT_ANALOGUE_GAIN 1.0
> > > -#define DEFAULT_EXPOSURE_TIME 20000
> > > +constexpr unsigned int DefaultAnalogueGain = 1.0;
> > > +constexpr unsigned int DefaultExposureTime = 20000;
> >
> > Oh nice, This really highlights to me the benefit of constexpr.
> > Now the values have types!
>
> Should DefaultAnalogueGain be a double? Of course it gets cast to a
> double when it gets copied into agcStatus.analogue_gain so it probably
> contrives to make no difference, but a double might be clearer,
> especially if anyone ever changed it.

Indeed, this should be a double!  Will fix it in the next version.

Regards,
Naush

>
> Thanks!
> David
>
> >
> > >
> > >  LOG_DEFINE_CATEGORY(IPARPI)
> > >
> > > @@ -65,7 +65,7 @@ class IPARPi : public IPAInterface
> > >  public:
> > >       IPARPi()
> > >               : lastMode_({}), controller_(), controllerInit_(false),
> > > -               frame_count_(0), check_count_(0), mistrust_count_(0),
> > > +               frameCount_(0), checkCount_(0), mistrustCount_(0),
> > >                 lsTable_(nullptr)
> > >       {
> > >       }
> > > @@ -73,7 +73,7 @@ public:
> > >       ~IPARPi()
> > >       {
> > >               if (lsTable_)
> > > -                     munmap(lsTable_, MAX_LS_GRID_SIZE);
> > > +                     munmap(lsTable_, RPi::MaxLsGridSize);
> > >       }
> > >
> > >       int init(const IPASettings &settings) override;
> > > @@ -108,13 +108,13 @@ private:
> > >       void applySharpen(const struct SharpenStatus *sharpenStatus, ControlList &ctrls);
> > >       void applyDPC(const struct DpcStatus *dpcStatus, ControlList &ctrls);
> > >       void applyLS(const struct AlscStatus *lsStatus, ControlList &ctrls);
> > > -     void resampleTable(uint16_t dest[], double const src[12][16], int dest_w, int dest_h);
> > > +     void resampleTable(uint16_t dest[], double const src[12][16], int destW, int destH);
> > >
> > >       std::map<unsigned int, FrameBuffer> buffers_;
> > >       std::map<unsigned int, void *> buffersMemory_;
> > >
> > > -     ControlInfoMap unicam_ctrls_;
> > > -     ControlInfoMap isp_ctrls_;
> > > +     ControlInfoMap unicamCtrls_;
> > > +     ControlInfoMap ispCtrls_;
> > >       ControlList libcameraMetadata_;
> > >
> > >       /* IPA configuration. */
> > > @@ -134,11 +134,11 @@ private:
> > >        * We count frames to decide if the frame must be hidden (e.g. from
> > >        * display) or mistrusted (i.e. not given to the control algos).
> > >        */
> > > -     uint64_t frame_count_;
> > > +     uint64_t frameCount_;
> >
> > If we're doing clean up - I always prefer to see a newline before a
> > comment line.
> >
> > To me it makes the commented block clearer - but I don't know if there's
> > a 'rule' on it.
> >
> >
> > >       /* For checking the sequencing of Prepare/Process calls. */
> > > -     uint64_t check_count_;
> > > +     uint64_t checkCount_;
> >
> > i.e. here too.
> >
> > >       /* How many frames we should avoid running control algos on. */
> > > -     unsigned int mistrust_count_;
> > > +     unsigned int mistrustCount_;
> >
> > and so on.
> >
> > >       /* LS table allocation passed in from the pipeline handler. */
> > >       FileDescriptor lsTableHandle_;
> > >       void *lsTable_;
> > > @@ -199,8 +199,8 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
> > >
> > >       result->operation = 0;
> > >
> > > -     unicam_ctrls_ = entityControls.at(0);
> > > -     isp_ctrls_ = entityControls.at(1);
> > > +     unicamCtrls_ = entityControls.at(0);
> > > +     ispCtrls_ = entityControls.at(1);
> > >       /* Setup a metadata ControlList to output metadata. */
> > >       libcameraMetadata_ = ControlList(controls::controls);
> > >
> > > @@ -238,18 +238,18 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
> > >        *"mistrusted", which depends on whether this is a startup from cold,
> > >        * or merely a mode switch in a running system.
> > >        */
> > > -     frame_count_ = 0;
> > > -     check_count_ = 0;
> > > -     unsigned int drop_frame = 0;
> > > +     frameCount_ = 0;
> > > +     checkCount_ = 0;
> > > +     unsigned int dropFrame = 0;
> > >       if (controllerInit_) {
> > > -             drop_frame = helper_->HideFramesModeSwitch();
> > > -             mistrust_count_ = helper_->MistrustFramesModeSwitch();
> > > +             dropFrame = helper_->HideFramesModeSwitch();
> > > +             mistrustCount_ = helper_->MistrustFramesModeSwitch();
> > >       } else {
> > > -             drop_frame = helper_->HideFramesStartup();
> > > -             mistrust_count_ = helper_->MistrustFramesStartup();
> > > +             dropFrame = helper_->HideFramesStartup();
> > > +             mistrustCount_ = helper_->MistrustFramesStartup();
> > >       }
> > >
> > > -     result->data.push_back(drop_frame);
> > > +     result->data.push_back(dropFrame);
> > >       result->operation |= RPi::IPA_CONFIG_DROP_FRAMES;
> > >
> > >       struct AgcStatus agcStatus;
> > > @@ -264,8 +264,8 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
> > >               controllerInit_ = true;
> > >
> > >               /* Supply initial values for gain and exposure. */
> > > -             agcStatus.shutter_time = DEFAULT_EXPOSURE_TIME;
> > > -             agcStatus.analogue_gain = DEFAULT_ANALOGUE_GAIN;
> > > +             agcStatus.shutter_time = DefaultExposureTime;
> > > +             agcStatus.analogue_gain = DefaultAnalogueGain;
> > >       }
> > >
> > >       RPiController::Metadata metadata;
> > > @@ -274,7 +274,7 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
> > >       /* SwitchMode may supply updated exposure/gain values to use. */
> > >       metadata.Get("agc.status", agcStatus);
> > >       if (agcStatus.shutter_time != 0.0 && agcStatus.analogue_gain != 0.0) {
> > > -             ControlList ctrls(unicam_ctrls_);
> > > +             ControlList ctrls(unicamCtrls_);
> > >               applyAGC(&agcStatus, ctrls);
> > >               result->controls.push_back(ctrls);
> > >
> > > @@ -287,14 +287,14 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
> > >       if (ipaConfig.operation & RPi::IPA_CONFIG_LS_TABLE) {
> > >               /* Remove any previous table, if there was one. */
> > >               if (lsTable_) {
> > > -                     munmap(lsTable_, MAX_LS_GRID_SIZE);
> > > +                     munmap(lsTable_, RPi::MaxLsGridSize);
> > >                       lsTable_ = nullptr;
> > >               }
> > >
> > >               /* Map the LS table buffer into user space. */
> > >               lsTableHandle_ = FileDescriptor(ipaConfig.data[0]);
> > >               if (lsTableHandle_.isValid()) {
> > > -                     lsTable_ = mmap(nullptr, MAX_LS_GRID_SIZE, PROT_READ | PROT_WRITE,
> > > +                     lsTable_ = mmap(nullptr, RPi::MaxLsGridSize, PROT_READ | PROT_WRITE,
> > >                                       MAP_SHARED, lsTableHandle_.fd(), 0);
> > >
> > >                       if (lsTable_ == MAP_FAILED) {
> > > @@ -343,9 +343,9 @@ void IPARPi::processEvent(const IPAOperationData &event)
> > >       case RPi::IPA_EVENT_SIGNAL_STAT_READY: {
> > >               unsigned int bufferId = event.data[0];
> > >
> > > -             if (++check_count_ != frame_count_) /* assert here? */
> > > +             if (++checkCount_ != frameCount_) /* assert here? */
> > >                       LOG(IPARPI, Error) << "WARNING: Prepare/Process mismatch!!!";
> > > -             if (frame_count_ > mistrust_count_)
> > > +             if (frameCount_ > mistrustCount_)
> > >                       processStats(bufferId);
> > >
> > >               reportMetadata();
> > > @@ -368,7 +368,7 @@ void IPARPi::processEvent(const IPAOperationData &event)
> > >                * they are "unreliable".
> > >                */
> > >               prepareISP(embeddedbufferId);
> > > -             frame_count_++;
> > > +             frameCount_++;
> > >
> > >               /* Ready to push the input buffer into the ISP. */
> > >               IPAOperationData op;
> > > @@ -713,7 +713,7 @@ void IPARPi::prepareISP(unsigned int bufferId)
> > >       returnEmbeddedBuffer(bufferId);
> > >
> > >       if (success) {
> > > -             ControlList ctrls(isp_ctrls_);
> > > +             ControlList ctrls(ispCtrls_);
> > >
> > >               rpiMetadata_.Clear();
> > >               rpiMetadata_.Set("device.status", deviceStatus);
> > > @@ -785,19 +785,19 @@ bool IPARPi::parseEmbeddedData(unsigned int bufferId, struct DeviceStatus &devic
> > >       if (status != RPiController::MdParser::Status::OK) {
> > >               LOG(IPARPI, Error) << "Embedded Buffer parsing failed, error " << status;
> > >       } else {
> > > -             uint32_t exposure_lines, gain_code;
> > > -             if (helper_->Parser().GetExposureLines(exposure_lines) != RPiController::MdParser::Status::OK) {
> > > +             uint32_t exposureLines, gainCode;
> > > +             if (helper_->Parser().GetExposureLines(exposureLines) != RPiController::MdParser::Status::OK) {
> > >                       LOG(IPARPI, Error) << "Exposure time failed";
> > >                       return false;
> > >               }
> > >
> > > -             deviceStatus.shutter_speed = helper_->Exposure(exposure_lines);
> > > -             if (helper_->Parser().GetGainCode(gain_code) != RPiController::MdParser::Status::OK) {
> > > +             deviceStatus.shutter_speed = helper_->Exposure(exposureLines);
> > > +             if (helper_->Parser().GetGainCode(gainCode) != RPiController::MdParser::Status::OK) {
> > >                       LOG(IPARPI, Error) << "Gain failed";
> > >                       return false;
> > >               }
> > >
> > > -             deviceStatus.analogue_gain = helper_->Gain(gain_code);
> > > +             deviceStatus.analogue_gain = helper_->Gain(gainCode);
> > >               LOG(IPARPI, Debug) << "Metadata - Exposure : "
> > >                                  << deviceStatus.shutter_speed << " Gain : "
> > >                                  << deviceStatus.analogue_gain;
> > > @@ -820,7 +820,7 @@ void IPARPi::processStats(unsigned int bufferId)
> > >
> > >       struct AgcStatus agcStatus;
> > >       if (rpiMetadata_.Get("agc.status", agcStatus) == 0) {
> > > -             ControlList ctrls(unicam_ctrls_);
> > > +             ControlList ctrls(unicamCtrls_);
> > >               applyAGC(&agcStatus, ctrls);
> > >
> > >               IPAOperationData op;
> > > @@ -832,14 +832,14 @@ void IPARPi::processStats(unsigned int bufferId)
> > >
> > >  void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls)
> > >  {
> > > -     const auto gainR = isp_ctrls_.find(V4L2_CID_RED_BALANCE);
> > > -     if (gainR == isp_ctrls_.end()) {
> > > +     const auto gainR = ispCtrls_.find(V4L2_CID_RED_BALANCE);
> > > +     if (gainR == ispCtrls_.end()) {
> > >               LOG(IPARPI, Error) << "Can't find red gain control";
> > >               return;
> > >       }
> > >
> > > -     const auto gainB = isp_ctrls_.find(V4L2_CID_BLUE_BALANCE);
> > > -     if (gainB == isp_ctrls_.end()) {
> > > +     const auto gainB = ispCtrls_.find(V4L2_CID_BLUE_BALANCE);
> > > +     if (gainB == ispCtrls_.end()) {
> > >               LOG(IPARPI, Error) << "Can't find blue gain control";
> > >               return;
> > >       }
> > > @@ -855,31 +855,31 @@ void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls)
> > >
> > >  void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls)
> > >  {
> > > -     int32_t gain_code = helper_->GainCode(agcStatus->analogue_gain);
> > > -     int32_t exposure_lines = helper_->ExposureLines(agcStatus->shutter_time);
> > > +     int32_t gainCode = helper_->GainCode(agcStatus->analogue_gain);
> > > +     int32_t exposureLines = helper_->ExposureLines(agcStatus->shutter_time);
> > >
> > > -     if (unicam_ctrls_.find(V4L2_CID_ANALOGUE_GAIN) == unicam_ctrls_.end()) {
> > > +     if (unicamCtrls_.find(V4L2_CID_ANALOGUE_GAIN) == unicamCtrls_.end()) {
> > >               LOG(IPARPI, Error) << "Can't find analogue gain control";
> > >               return;
> > >       }
> > >
> > > -     if (unicam_ctrls_.find(V4L2_CID_EXPOSURE) == unicam_ctrls_.end()) {
> > > +     if (unicamCtrls_.find(V4L2_CID_EXPOSURE) == unicamCtrls_.end()) {
> > >               LOG(IPARPI, Error) << "Can't find exposure control";
> > >               return;
> > >       }
> > >
> > >       LOG(IPARPI, Debug) << "Applying AGC Exposure: " << agcStatus->shutter_time
> > > -                        << " (Shutter lines: " << exposure_lines << ") Gain: "
> > > +                        << " (Shutter lines: " << exposureLines << ") Gain: "
> > >                          << agcStatus->analogue_gain << " (Gain Code: "
> > > -                        << gain_code << ")";
> > > +                        << gainCode << ")";
> > >
> > > -     ctrls.set(V4L2_CID_ANALOGUE_GAIN, gain_code);
> > > -     ctrls.set(V4L2_CID_EXPOSURE, exposure_lines);
> > > +     ctrls.set(V4L2_CID_ANALOGUE_GAIN, gainCode);
> > > +     ctrls.set(V4L2_CID_EXPOSURE, exposureLines);
> > >  }
> > >
> > >  void IPARPi::applyDG(const struct AgcStatus *dgStatus, ControlList &ctrls)
> > >  {
> > > -     if (isp_ctrls_.find(V4L2_CID_DIGITAL_GAIN) == isp_ctrls_.end()) {
> > > +     if (ispCtrls_.find(V4L2_CID_DIGITAL_GAIN) == ispCtrls_.end()) {
> > >               LOG(IPARPI, Error) << "Can't find digital gain control";
> > >               return;
> > >       }
> > > @@ -890,7 +890,7 @@ void IPARPi::applyDG(const struct AgcStatus *dgStatus, ControlList &ctrls)
> > >
> > >  void IPARPi::applyCCM(const struct CcmStatus *ccmStatus, ControlList &ctrls)
> > >  {
> > > -     if (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_CC_MATRIX) == isp_ctrls_.end()) {
> > > +     if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_CC_MATRIX) == ispCtrls_.end()) {
> > >               LOG(IPARPI, Error) << "Can't find CCM control";
> > >               return;
> > >       }
> > > @@ -911,7 +911,7 @@ void IPARPi::applyCCM(const struct CcmStatus *ccmStatus, ControlList &ctrls)
> > >
> > >  void IPARPi::applyGamma(const struct ContrastStatus *contrastStatus, ControlList &ctrls)
> > >  {
> > > -     if (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_GAMMA) == isp_ctrls_.end()) {
> > > +     if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_GAMMA) == ispCtrls_.end()) {
> > >               LOG(IPARPI, Error) << "Can't find Gamma control";
> > >               return;
> > >       }
> > > @@ -930,7 +930,7 @@ void IPARPi::applyGamma(const struct ContrastStatus *contrastStatus, ControlList
> > >
> > >  void IPARPi::applyBlackLevel(const struct BlackLevelStatus *blackLevelStatus, ControlList &ctrls)
> > >  {
> > > -     if (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_BLACK_LEVEL) == isp_ctrls_.end()) {
> > > +     if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_BLACK_LEVEL) == ispCtrls_.end()) {
> > >               LOG(IPARPI, Error) << "Can't find black level control";
> > >               return;
> > >       }
> > > @@ -948,7 +948,7 @@ void IPARPi::applyBlackLevel(const struct BlackLevelStatus *blackLevelStatus, Co
> > >
> > >  void IPARPi::applyGEQ(const struct GeqStatus *geqStatus, ControlList &ctrls)
> > >  {
> > > -     if (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_GEQ) == isp_ctrls_.end()) {
> > > +     if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_GEQ) == ispCtrls_.end()) {
> > >               LOG(IPARPI, Error) << "Can't find geq control";
> > >               return;
> > >       }
> > > @@ -966,7 +966,7 @@ void IPARPi::applyGEQ(const struct GeqStatus *geqStatus, ControlList &ctrls)
> > >
> > >  void IPARPi::applyDenoise(const struct SdnStatus *denoiseStatus, ControlList &ctrls)
> > >  {
> > > -     if (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_DENOISE) == isp_ctrls_.end()) {
> > > +     if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_DENOISE) == ispCtrls_.end()) {
> > >               LOG(IPARPI, Error) << "Can't find denoise control";
> > >               return;
> > >       }
> > > @@ -986,7 +986,7 @@ void IPARPi::applyDenoise(const struct SdnStatus *denoiseStatus, ControlList &ct
> > >
> > >  void IPARPi::applySharpen(const struct SharpenStatus *sharpenStatus, ControlList &ctrls)
> > >  {
> > > -     if (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_SHARPEN) == isp_ctrls_.end()) {
> > > +     if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_SHARPEN) == ispCtrls_.end()) {
> > >               LOG(IPARPI, Error) << "Can't find sharpen control";
> > >               return;
> > >       }
> > > @@ -1007,7 +1007,7 @@ void IPARPi::applySharpen(const struct SharpenStatus *sharpenStatus, ControlList
> > >
> > >  void IPARPi::applyDPC(const struct DpcStatus *dpcStatus, ControlList &ctrls)
> > >  {
> > > -     if (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_DPC) == isp_ctrls_.end()) {
> > > +     if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_DPC) == ispCtrls_.end()) {
> > >               LOG(IPARPI, Error) << "Can't find DPC control";
> > >               return;
> > >       }
> > > @@ -1023,7 +1023,7 @@ void IPARPi::applyDPC(const struct DpcStatus *dpcStatus, ControlList &ctrls)
> > >
> > >  void IPARPi::applyLS(const struct AlscStatus *lsStatus, ControlList &ctrls)
> > >  {
> > > -     if (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING) == isp_ctrls_.end()) {
> > > +     if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING) == ispCtrls_.end()) {
> > >               LOG(IPARPI, Error) << "Can't find LS control";
> > >               return;
> > >       }
> > > @@ -1032,18 +1032,18 @@ void IPARPi::applyLS(const struct AlscStatus *lsStatus, ControlList &ctrls)
> > >        * Program lens shading tables into pipeline.
> > >        * Choose smallest cell size that won't exceed 63x48 cells.
> > >        */
> > > -     const int cell_sizes[] = { 16, 32, 64, 128, 256 };
> > > -     unsigned int num_cells = ARRAY_SIZE(cell_sizes);
> > > -     unsigned int i, w, h, cell_size;
> > > -     for (i = 0; i < num_cells; i++) {
> > > -             cell_size = cell_sizes[i];
> > > -             w = (mode_.width + cell_size - 1) / cell_size;
> > > -             h = (mode_.height + cell_size - 1) / cell_size;
> > > +     const int cellSizes[] = { 16, 32, 64, 128, 256 };
> > > +     unsigned int numCells = ARRAY_SIZE(cellSizes);
> > > +     unsigned int i, w, h, cellSize;
> > > +     for (i = 0; i < numCells; i++) {
> > > +             cellSize = cellSizes[i];
> > > +             w = (mode_.width + cellSize - 1) / cellSize;
> > > +             h = (mode_.height + cellSize - 1) / cellSize;
> > >               if (w < 64 && h <= 48)
> > >                       break;
> > >       }
> > >
> > > -     if (i == num_cells) {
> > > +     if (i == numCells) {
> > >               LOG(IPARPI, Error) << "Cannot find cell size";
> > >               return;
> > >       }
> > > @@ -1052,7 +1052,7 @@ void IPARPi::applyLS(const struct AlscStatus *lsStatus, ControlList &ctrls)
> > >       w++, h++;
> > >       bcm2835_isp_lens_shading ls = {
> > >               .enabled = 1,
> > > -             .grid_cell_size = cell_size,
> > > +             .grid_cell_size = cellSize,
> > >               .grid_width = w,
> > >               .grid_stride = w,
> > >               .grid_height = h,
> > > @@ -1062,7 +1062,7 @@ void IPARPi::applyLS(const struct AlscStatus *lsStatus, ControlList &ctrls)
> > >               .gain_format = GAIN_FORMAT_U4P10
> > >       };
> > >
> > > -     if (!lsTable_ || w * h * 4 * sizeof(uint16_t) > MAX_LS_GRID_SIZE) {
> > > +     if (!lsTable_ || w * h * 4 * sizeof(uint16_t) > RPi::MaxLsGridSize) {
> > >               LOG(IPARPI, Error) << "Do not have a correctly allocate lens shading table!";
> > >               return;
> > >       }
> > > @@ -1083,41 +1083,41 @@ void IPARPi::applyLS(const struct AlscStatus *lsStatus, ControlList &ctrls)
> > >  }
> > >
> > >  /*
> > > - * Resamples a 16x12 table with central sampling to dest_w x dest_h with corner
> > > + * Resamples a 16x12 table with central sampling to destW x destH with corner
> > >   * sampling.
> > >   */
> > >  void IPARPi::resampleTable(uint16_t dest[], double const src[12][16],
> > > -                        int dest_w, int dest_h)
> > > +                        int destW, int destH)
> > >  {
> > >       /*
> > >        * Precalculate and cache the x sampling locations and phases to
> > >        * save recomputing them on every row.
> > >        */
> > > -     assert(dest_w > 1 && dest_h > 1 && dest_w <= 64);
> > > -     int x_lo[64], x_hi[64];
> > > +     assert(destW > 1 && destH > 1 && destW <= 64);
> > > +     int xLo[64], xHi[64];
> > >       double xf[64];
> > > -     double x = -0.5, x_inc = 16.0 / (dest_w - 1);
> > > -     for (int i = 0; i < dest_w; i++, x += x_inc) {
> > > -             x_lo[i] = floor(x);
> > > -             xf[i] = x - x_lo[i];
> > > -             x_hi[i] = x_lo[i] < 15 ? x_lo[i] + 1 : 15;
> > > -             x_lo[i] = x_lo[i] > 0 ? x_lo[i] : 0;
> > > +     double x = -0.5, xInc = 16.0 / (destW - 1);
> > > +     for (int i = 0; i < destW; i++, x += xInc) {
> > > +             xLo[i] = floor(x);
> > > +             xf[i] = x - xLo[i];
> > > +             xHi[i] = xLo[i] < 15 ? xLo[i] + 1 : 15;
> > > +             xLo[i] = xLo[i] > 0 ? xLo[i] : 0;
> > >       }
> > >
> > >       /* Now march over the output table generating the new values. */
> > > -     double y = -0.5, y_inc = 12.0 / (dest_h - 1);
> > > -     for (int j = 0; j < dest_h; j++, y += y_inc) {
> > > -             int y_lo = floor(y);
> > > -             double yf = y - y_lo;
> > > -             int y_hi = y_lo < 11 ? y_lo + 1 : 11;
> > > -             y_lo = y_lo > 0 ? y_lo : 0;
> > > -             double const *row_above = src[y_lo];
> > > -             double const *row_below = src[y_hi];
> > > -             for (int i = 0; i < dest_w; i++) {
> > > -                     double above = row_above[x_lo[i]] * (1 - xf[i])
> > > -                                  + row_above[x_hi[i]] * xf[i];
> > > -                     double below = row_below[x_lo[i]] * (1 - xf[i])
> > > -                                  + row_below[x_hi[i]] * xf[i];
> > > +     double y = -0.5, yInc = 12.0 / (destH - 1);
> > > +     for (int j = 0; j < destH; j++, y += yInc) {
> > > +             int yLo = floor(y);
> > > +             double yf = y - yLo;
> > > +             int yHi = yLo < 11 ? yLo + 1 : 11;
> > > +             yLo = yLo > 0 ? yLo : 0;
> > > +             double const *rowAbove = src[yLo];
> > > +             double const *rowBelow = src[yHi];
> > > +             for (int i = 0; i < destW; i++) {
> > > +                     double above = rowAbove[xLo[i]] * (1 - xf[i])
> > > +                                  + rowAbove[xHi[i]] * xf[i];
> > > +                     double below = rowBelow[xLo[i]] * (1 - xf[i])
> > > +                                  + rowBelow[xHi[i]] * xf[i];
> > >                       int result = floor(1024 * (above * (1 - yf) + below * yf) + .5);
> > >                       *(dest++) = result > 16383 ? 16383 : result; /* want u4.10 */
> > >               }
> > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > index 35dbe0fb..8d40b0ed 100644
> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > @@ -1012,7 +1012,7 @@ int RPiCameraData::configureIPA()
> > >
> > >       /* Allocate the lens shading table via dmaHeap and pass to the IPA. */
> > >       if (!lsTable_.isValid()) {
> > > -             lsTable_ = dmaHeap_.alloc("ls_grid", MAX_LS_GRID_SIZE);
> > > +             lsTable_ = dmaHeap_.alloc("ls_grid", RPi::MaxLsGridSize);
> >
> > If we had an IPA namespace, it would be nice to see this as:
> >
> >   lsTable_ = dmaHeap_.alloc("ls_grid", ipa::RPi::MaxLsGridSize);
> >
> > As that would make it really clear 'where' this MaxLsGridSize value was
> > coming from...
> >
> > Anyway, all my comments above are optional discussion points.
> >
> > For the patch:
> >
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> >
> > >               if (!lsTable_.isValid())
> > >                       return -ENOMEM;
> > >
> > >
> >
> > --
> > Regards
> > --
> > Kieran
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel@lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
Naushir Patuck Sept. 24, 2020, 8:37 a.m. UTC | #8
Hi Kieran and Jacopo,

Thank you both for the review comments.

On Wed, 23 Sep 2020 at 12:45, Kieran Bingham
<kieran.bingham@ideasonboard.com> wrote:
>
> Hi Jacopo,
>
> On 23/09/2020 12:39, Jacopo Mondi wrote:
> > /me dives into bikeshedding
>
> <snip>
>
> >>> @@ -134,11 +134,11 @@ private:
> >>>      * We count frames to decide if the frame must be hidden (e.g. from
> >>>      * display) or mistrusted (i.e. not given to the control algos).
> >>>      */
> >>> -   uint64_t frame_count_;
> >>> +   uint64_t frameCount_;
> >>
> >> If we're doing clean up - I always prefer to see a newline before a
> >> comment line.
>
> Note here I state 'before a comment'
>
> >> To me it makes the commented block clearer - but I don't know if there's
> >> a 'rule' on it.
> >>
> >
> > I'm sorry but
> >
> >         /*
> >          * A comment describing a variable is better places as close
> >          * as possible near to that variable, isn't it ?
> >          */
> >          int whatAFancyVariable;
> >
> >          /* Or do you think this variable is not fancy and an empty line is better?  */
> >
>
> Here you seem to be implying 'after' a comment...
>
> >          int notAFancyVariable;
> >
> > Ok, enough useless discussions from me :)
> >
>
> I ... 'think' you mis-interpreted what I said, or I was not clear. I was
> probably not clear ;-)
>
> I think that this:
>
>         /* A variable group. */
>         bool variable;
>         int quantity;
>
>         /*
>          * We count frames to decide if the frame must be hidden (e.g.
>          * from display) or mistrusted (i.e. not given to the control
>          * algos).
>          */
>         uint64_t frameCount_;
>
>         /* For checking the sequencing of Prepare/Process calls. */
>         uint64_t checkCount_;
>
>         /* How many frames we should avoid running control algos on. */
>         unsigned int mistrustCount_;
>
> is better than this:
>
>
>         /* A variable group */
>         bool variable;
>         int quanty;
>         /*
>          * We count frames to decide if the frame must be hidden (e.g.
>          * from display) or mistrusted (i.e. not given to the control
>          * algos).
>          */
>         uint64_t frameCount_;
>         /* For checking the sequencing of Prepare/Process calls. */
>         uint64_t checkCount_;
>         /* How many frames we should avoid running control algos on. */
>         unsigned int mistrustCount_;

Yes, I agree the latter block is less readable than the former.  I
will update this patch with some extra whitespace where needed.

Regards,
Naush


>
> Maybe one day I'll finish my comment formatter in the checkstyle script ;-)
>
>
> >>
> >>>     /* For checking the sequencing of Prepare/Process calls. */
> >>> -   uint64_t check_count_;
> >>> +   uint64_t checkCount_;
> >>
> >> i.e. here too.
> >>
> >>>     /* How many frames we should avoid running control algos on. */
> >>> -   unsigned int mistrust_count_;
> >>> +   unsigned int mistrustCount_;
> >>
> >> and so on.
> >>
> >>>     /* LS table allocation passed in from the pipeline handler. */
> >>>     FileDescriptor lsTableHandle_;
> >>>     void *lsTable_;
>
> <snip>
> --
> Regards
> --
> Kieran

Patch

diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h
index c9d4aa81..b3041591 100644
--- a/include/libcamera/ipa/raspberrypi.h
+++ b/include/libcamera/ipa/raspberrypi.h
@@ -41,7 +41,7 @@  enum BufferMask {
 };
 
 /* Size of the LS grid allocation. */
-#define MAX_LS_GRID_SIZE (32 << 10)
+static constexpr unsigned int MaxLsGridSize = 32 << 10;
 
 /* List of controls handled by the Raspberry Pi IPA */
 static const ControlInfoMap Controls = {
diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
index 0c0dc743..c240eae8 100644
--- a/src/ipa/raspberrypi/raspberrypi.cpp
+++ b/src/ipa/raspberrypi/raspberrypi.cpp
@@ -55,8 +55,8 @@ 
 namespace libcamera {
 
 /* Configure the sensor with these values initially. */
-#define DEFAULT_ANALOGUE_GAIN 1.0
-#define DEFAULT_EXPOSURE_TIME 20000
+constexpr unsigned int DefaultAnalogueGain = 1.0;
+constexpr unsigned int DefaultExposureTime = 20000;
 
 LOG_DEFINE_CATEGORY(IPARPI)
 
@@ -65,7 +65,7 @@  class IPARPi : public IPAInterface
 public:
 	IPARPi()
 		: lastMode_({}), controller_(), controllerInit_(false),
-		  frame_count_(0), check_count_(0), mistrust_count_(0),
+		  frameCount_(0), checkCount_(0), mistrustCount_(0),
 		  lsTable_(nullptr)
 	{
 	}
@@ -73,7 +73,7 @@  public:
 	~IPARPi()
 	{
 		if (lsTable_)
-			munmap(lsTable_, MAX_LS_GRID_SIZE);
+			munmap(lsTable_, RPi::MaxLsGridSize);
 	}
 
 	int init(const IPASettings &settings) override;
@@ -108,13 +108,13 @@  private:
 	void applySharpen(const struct SharpenStatus *sharpenStatus, ControlList &ctrls);
 	void applyDPC(const struct DpcStatus *dpcStatus, ControlList &ctrls);
 	void applyLS(const struct AlscStatus *lsStatus, ControlList &ctrls);
-	void resampleTable(uint16_t dest[], double const src[12][16], int dest_w, int dest_h);
+	void resampleTable(uint16_t dest[], double const src[12][16], int destW, int destH);
 
 	std::map<unsigned int, FrameBuffer> buffers_;
 	std::map<unsigned int, void *> buffersMemory_;
 
-	ControlInfoMap unicam_ctrls_;
-	ControlInfoMap isp_ctrls_;
+	ControlInfoMap unicamCtrls_;
+	ControlInfoMap ispCtrls_;
 	ControlList libcameraMetadata_;
 
 	/* IPA configuration. */
@@ -134,11 +134,11 @@  private:
 	 * We count frames to decide if the frame must be hidden (e.g. from
 	 * display) or mistrusted (i.e. not given to the control algos).
 	 */
-	uint64_t frame_count_;
+	uint64_t frameCount_;
 	/* For checking the sequencing of Prepare/Process calls. */
-	uint64_t check_count_;
+	uint64_t checkCount_;
 	/* How many frames we should avoid running control algos on. */
-	unsigned int mistrust_count_;
+	unsigned int mistrustCount_;
 	/* LS table allocation passed in from the pipeline handler. */
 	FileDescriptor lsTableHandle_;
 	void *lsTable_;
@@ -199,8 +199,8 @@  void IPARPi::configure(const CameraSensorInfo &sensorInfo,
 
 	result->operation = 0;
 
-	unicam_ctrls_ = entityControls.at(0);
-	isp_ctrls_ = entityControls.at(1);
+	unicamCtrls_ = entityControls.at(0);
+	ispCtrls_ = entityControls.at(1);
 	/* Setup a metadata ControlList to output metadata. */
 	libcameraMetadata_ = ControlList(controls::controls);
 
@@ -238,18 +238,18 @@  void IPARPi::configure(const CameraSensorInfo &sensorInfo,
 	 *"mistrusted", which depends on whether this is a startup from cold,
 	 * or merely a mode switch in a running system.
 	 */
-	frame_count_ = 0;
-	check_count_ = 0;
-	unsigned int drop_frame = 0;
+	frameCount_ = 0;
+	checkCount_ = 0;
+	unsigned int dropFrame = 0;
 	if (controllerInit_) {
-		drop_frame = helper_->HideFramesModeSwitch();
-		mistrust_count_ = helper_->MistrustFramesModeSwitch();
+		dropFrame = helper_->HideFramesModeSwitch();
+		mistrustCount_ = helper_->MistrustFramesModeSwitch();
 	} else {
-		drop_frame = helper_->HideFramesStartup();
-		mistrust_count_ = helper_->MistrustFramesStartup();
+		dropFrame = helper_->HideFramesStartup();
+		mistrustCount_ = helper_->MistrustFramesStartup();
 	}
 
-	result->data.push_back(drop_frame);
+	result->data.push_back(dropFrame);
 	result->operation |= RPi::IPA_CONFIG_DROP_FRAMES;
 
 	struct AgcStatus agcStatus;
@@ -264,8 +264,8 @@  void IPARPi::configure(const CameraSensorInfo &sensorInfo,
 		controllerInit_ = true;
 
 		/* Supply initial values for gain and exposure. */
-		agcStatus.shutter_time = DEFAULT_EXPOSURE_TIME;
-		agcStatus.analogue_gain = DEFAULT_ANALOGUE_GAIN;
+		agcStatus.shutter_time = DefaultExposureTime;
+		agcStatus.analogue_gain = DefaultAnalogueGain;
 	}
 
 	RPiController::Metadata metadata;
@@ -274,7 +274,7 @@  void IPARPi::configure(const CameraSensorInfo &sensorInfo,
 	/* SwitchMode may supply updated exposure/gain values to use. */
 	metadata.Get("agc.status", agcStatus);
 	if (agcStatus.shutter_time != 0.0 && agcStatus.analogue_gain != 0.0) {
-		ControlList ctrls(unicam_ctrls_);
+		ControlList ctrls(unicamCtrls_);
 		applyAGC(&agcStatus, ctrls);
 		result->controls.push_back(ctrls);
 
@@ -287,14 +287,14 @@  void IPARPi::configure(const CameraSensorInfo &sensorInfo,
 	if (ipaConfig.operation & RPi::IPA_CONFIG_LS_TABLE) {
 		/* Remove any previous table, if there was one. */
 		if (lsTable_) {
-			munmap(lsTable_, MAX_LS_GRID_SIZE);
+			munmap(lsTable_, RPi::MaxLsGridSize);
 			lsTable_ = nullptr;
 		}
 
 		/* Map the LS table buffer into user space. */
 		lsTableHandle_ = FileDescriptor(ipaConfig.data[0]);
 		if (lsTableHandle_.isValid()) {
-			lsTable_ = mmap(nullptr, MAX_LS_GRID_SIZE, PROT_READ | PROT_WRITE,
+			lsTable_ = mmap(nullptr, RPi::MaxLsGridSize, PROT_READ | PROT_WRITE,
 					MAP_SHARED, lsTableHandle_.fd(), 0);
 
 			if (lsTable_ == MAP_FAILED) {
@@ -343,9 +343,9 @@  void IPARPi::processEvent(const IPAOperationData &event)
 	case RPi::IPA_EVENT_SIGNAL_STAT_READY: {
 		unsigned int bufferId = event.data[0];
 
-		if (++check_count_ != frame_count_) /* assert here? */
+		if (++checkCount_ != frameCount_) /* assert here? */
 			LOG(IPARPI, Error) << "WARNING: Prepare/Process mismatch!!!";
-		if (frame_count_ > mistrust_count_)
+		if (frameCount_ > mistrustCount_)
 			processStats(bufferId);
 
 		reportMetadata();
@@ -368,7 +368,7 @@  void IPARPi::processEvent(const IPAOperationData &event)
 		 * they are "unreliable".
 		 */
 		prepareISP(embeddedbufferId);
-		frame_count_++;
+		frameCount_++;
 
 		/* Ready to push the input buffer into the ISP. */
 		IPAOperationData op;
@@ -713,7 +713,7 @@  void IPARPi::prepareISP(unsigned int bufferId)
 	returnEmbeddedBuffer(bufferId);
 
 	if (success) {
-		ControlList ctrls(isp_ctrls_);
+		ControlList ctrls(ispCtrls_);
 
 		rpiMetadata_.Clear();
 		rpiMetadata_.Set("device.status", deviceStatus);
@@ -785,19 +785,19 @@  bool IPARPi::parseEmbeddedData(unsigned int bufferId, struct DeviceStatus &devic
 	if (status != RPiController::MdParser::Status::OK) {
 		LOG(IPARPI, Error) << "Embedded Buffer parsing failed, error " << status;
 	} else {
-		uint32_t exposure_lines, gain_code;
-		if (helper_->Parser().GetExposureLines(exposure_lines) != RPiController::MdParser::Status::OK) {
+		uint32_t exposureLines, gainCode;
+		if (helper_->Parser().GetExposureLines(exposureLines) != RPiController::MdParser::Status::OK) {
 			LOG(IPARPI, Error) << "Exposure time failed";
 			return false;
 		}
 
-		deviceStatus.shutter_speed = helper_->Exposure(exposure_lines);
-		if (helper_->Parser().GetGainCode(gain_code) != RPiController::MdParser::Status::OK) {
+		deviceStatus.shutter_speed = helper_->Exposure(exposureLines);
+		if (helper_->Parser().GetGainCode(gainCode) != RPiController::MdParser::Status::OK) {
 			LOG(IPARPI, Error) << "Gain failed";
 			return false;
 		}
 
-		deviceStatus.analogue_gain = helper_->Gain(gain_code);
+		deviceStatus.analogue_gain = helper_->Gain(gainCode);
 		LOG(IPARPI, Debug) << "Metadata - Exposure : "
 				   << deviceStatus.shutter_speed << " Gain : "
 				   << deviceStatus.analogue_gain;
@@ -820,7 +820,7 @@  void IPARPi::processStats(unsigned int bufferId)
 
 	struct AgcStatus agcStatus;
 	if (rpiMetadata_.Get("agc.status", agcStatus) == 0) {
-		ControlList ctrls(unicam_ctrls_);
+		ControlList ctrls(unicamCtrls_);
 		applyAGC(&agcStatus, ctrls);
 
 		IPAOperationData op;
@@ -832,14 +832,14 @@  void IPARPi::processStats(unsigned int bufferId)
 
 void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls)
 {
-	const auto gainR = isp_ctrls_.find(V4L2_CID_RED_BALANCE);
-	if (gainR == isp_ctrls_.end()) {
+	const auto gainR = ispCtrls_.find(V4L2_CID_RED_BALANCE);
+	if (gainR == ispCtrls_.end()) {
 		LOG(IPARPI, Error) << "Can't find red gain control";
 		return;
 	}
 
-	const auto gainB = isp_ctrls_.find(V4L2_CID_BLUE_BALANCE);
-	if (gainB == isp_ctrls_.end()) {
+	const auto gainB = ispCtrls_.find(V4L2_CID_BLUE_BALANCE);
+	if (gainB == ispCtrls_.end()) {
 		LOG(IPARPI, Error) << "Can't find blue gain control";
 		return;
 	}
@@ -855,31 +855,31 @@  void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls)
 
 void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls)
 {
-	int32_t gain_code = helper_->GainCode(agcStatus->analogue_gain);
-	int32_t exposure_lines = helper_->ExposureLines(agcStatus->shutter_time);
+	int32_t gainCode = helper_->GainCode(agcStatus->analogue_gain);
+	int32_t exposureLines = helper_->ExposureLines(agcStatus->shutter_time);
 
-	if (unicam_ctrls_.find(V4L2_CID_ANALOGUE_GAIN) == unicam_ctrls_.end()) {
+	if (unicamCtrls_.find(V4L2_CID_ANALOGUE_GAIN) == unicamCtrls_.end()) {
 		LOG(IPARPI, Error) << "Can't find analogue gain control";
 		return;
 	}
 
-	if (unicam_ctrls_.find(V4L2_CID_EXPOSURE) == unicam_ctrls_.end()) {
+	if (unicamCtrls_.find(V4L2_CID_EXPOSURE) == unicamCtrls_.end()) {
 		LOG(IPARPI, Error) << "Can't find exposure control";
 		return;
 	}
 
 	LOG(IPARPI, Debug) << "Applying AGC Exposure: " << agcStatus->shutter_time
-			   << " (Shutter lines: " << exposure_lines << ") Gain: "
+			   << " (Shutter lines: " << exposureLines << ") Gain: "
 			   << agcStatus->analogue_gain << " (Gain Code: "
-			   << gain_code << ")";
+			   << gainCode << ")";
 
-	ctrls.set(V4L2_CID_ANALOGUE_GAIN, gain_code);
-	ctrls.set(V4L2_CID_EXPOSURE, exposure_lines);
+	ctrls.set(V4L2_CID_ANALOGUE_GAIN, gainCode);
+	ctrls.set(V4L2_CID_EXPOSURE, exposureLines);
 }
 
 void IPARPi::applyDG(const struct AgcStatus *dgStatus, ControlList &ctrls)
 {
-	if (isp_ctrls_.find(V4L2_CID_DIGITAL_GAIN) == isp_ctrls_.end()) {
+	if (ispCtrls_.find(V4L2_CID_DIGITAL_GAIN) == ispCtrls_.end()) {
 		LOG(IPARPI, Error) << "Can't find digital gain control";
 		return;
 	}
@@ -890,7 +890,7 @@  void IPARPi::applyDG(const struct AgcStatus *dgStatus, ControlList &ctrls)
 
 void IPARPi::applyCCM(const struct CcmStatus *ccmStatus, ControlList &ctrls)
 {
-	if (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_CC_MATRIX) == isp_ctrls_.end()) {
+	if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_CC_MATRIX) == ispCtrls_.end()) {
 		LOG(IPARPI, Error) << "Can't find CCM control";
 		return;
 	}
@@ -911,7 +911,7 @@  void IPARPi::applyCCM(const struct CcmStatus *ccmStatus, ControlList &ctrls)
 
 void IPARPi::applyGamma(const struct ContrastStatus *contrastStatus, ControlList &ctrls)
 {
-	if (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_GAMMA) == isp_ctrls_.end()) {
+	if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_GAMMA) == ispCtrls_.end()) {
 		LOG(IPARPI, Error) << "Can't find Gamma control";
 		return;
 	}
@@ -930,7 +930,7 @@  void IPARPi::applyGamma(const struct ContrastStatus *contrastStatus, ControlList
 
 void IPARPi::applyBlackLevel(const struct BlackLevelStatus *blackLevelStatus, ControlList &ctrls)
 {
-	if (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_BLACK_LEVEL) == isp_ctrls_.end()) {
+	if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_BLACK_LEVEL) == ispCtrls_.end()) {
 		LOG(IPARPI, Error) << "Can't find black level control";
 		return;
 	}
@@ -948,7 +948,7 @@  void IPARPi::applyBlackLevel(const struct BlackLevelStatus *blackLevelStatus, Co
 
 void IPARPi::applyGEQ(const struct GeqStatus *geqStatus, ControlList &ctrls)
 {
-	if (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_GEQ) == isp_ctrls_.end()) {
+	if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_GEQ) == ispCtrls_.end()) {
 		LOG(IPARPI, Error) << "Can't find geq control";
 		return;
 	}
@@ -966,7 +966,7 @@  void IPARPi::applyGEQ(const struct GeqStatus *geqStatus, ControlList &ctrls)
 
 void IPARPi::applyDenoise(const struct SdnStatus *denoiseStatus, ControlList &ctrls)
 {
-	if (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_DENOISE) == isp_ctrls_.end()) {
+	if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_DENOISE) == ispCtrls_.end()) {
 		LOG(IPARPI, Error) << "Can't find denoise control";
 		return;
 	}
@@ -986,7 +986,7 @@  void IPARPi::applyDenoise(const struct SdnStatus *denoiseStatus, ControlList &ct
 
 void IPARPi::applySharpen(const struct SharpenStatus *sharpenStatus, ControlList &ctrls)
 {
-	if (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_SHARPEN) == isp_ctrls_.end()) {
+	if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_SHARPEN) == ispCtrls_.end()) {
 		LOG(IPARPI, Error) << "Can't find sharpen control";
 		return;
 	}
@@ -1007,7 +1007,7 @@  void IPARPi::applySharpen(const struct SharpenStatus *sharpenStatus, ControlList
 
 void IPARPi::applyDPC(const struct DpcStatus *dpcStatus, ControlList &ctrls)
 {
-	if (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_DPC) == isp_ctrls_.end()) {
+	if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_DPC) == ispCtrls_.end()) {
 		LOG(IPARPI, Error) << "Can't find DPC control";
 		return;
 	}
@@ -1023,7 +1023,7 @@  void IPARPi::applyDPC(const struct DpcStatus *dpcStatus, ControlList &ctrls)
 
 void IPARPi::applyLS(const struct AlscStatus *lsStatus, ControlList &ctrls)
 {
-	if (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING) == isp_ctrls_.end()) {
+	if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING) == ispCtrls_.end()) {
 		LOG(IPARPI, Error) << "Can't find LS control";
 		return;
 	}
@@ -1032,18 +1032,18 @@  void IPARPi::applyLS(const struct AlscStatus *lsStatus, ControlList &ctrls)
 	 * Program lens shading tables into pipeline.
 	 * Choose smallest cell size that won't exceed 63x48 cells.
 	 */
-	const int cell_sizes[] = { 16, 32, 64, 128, 256 };
-	unsigned int num_cells = ARRAY_SIZE(cell_sizes);
-	unsigned int i, w, h, cell_size;
-	for (i = 0; i < num_cells; i++) {
-		cell_size = cell_sizes[i];
-		w = (mode_.width + cell_size - 1) / cell_size;
-		h = (mode_.height + cell_size - 1) / cell_size;
+	const int cellSizes[] = { 16, 32, 64, 128, 256 };
+	unsigned int numCells = ARRAY_SIZE(cellSizes);
+	unsigned int i, w, h, cellSize;
+	for (i = 0; i < numCells; i++) {
+		cellSize = cellSizes[i];
+		w = (mode_.width + cellSize - 1) / cellSize;
+		h = (mode_.height + cellSize - 1) / cellSize;
 		if (w < 64 && h <= 48)
 			break;
 	}
 
-	if (i == num_cells) {
+	if (i == numCells) {
 		LOG(IPARPI, Error) << "Cannot find cell size";
 		return;
 	}
@@ -1052,7 +1052,7 @@  void IPARPi::applyLS(const struct AlscStatus *lsStatus, ControlList &ctrls)
 	w++, h++;
 	bcm2835_isp_lens_shading ls = {
 		.enabled = 1,
-		.grid_cell_size = cell_size,
+		.grid_cell_size = cellSize,
 		.grid_width = w,
 		.grid_stride = w,
 		.grid_height = h,
@@ -1062,7 +1062,7 @@  void IPARPi::applyLS(const struct AlscStatus *lsStatus, ControlList &ctrls)
 		.gain_format = GAIN_FORMAT_U4P10
 	};
 
-	if (!lsTable_ || w * h * 4 * sizeof(uint16_t) > MAX_LS_GRID_SIZE) {
+	if (!lsTable_ || w * h * 4 * sizeof(uint16_t) > RPi::MaxLsGridSize) {
 		LOG(IPARPI, Error) << "Do not have a correctly allocate lens shading table!";
 		return;
 	}
@@ -1083,41 +1083,41 @@  void IPARPi::applyLS(const struct AlscStatus *lsStatus, ControlList &ctrls)
 }
 
 /*
- * Resamples a 16x12 table with central sampling to dest_w x dest_h with corner
+ * Resamples a 16x12 table with central sampling to destW x destH with corner
  * sampling.
  */
 void IPARPi::resampleTable(uint16_t dest[], double const src[12][16],
-			   int dest_w, int dest_h)
+			   int destW, int destH)
 {
 	/*
 	 * Precalculate and cache the x sampling locations and phases to
 	 * save recomputing them on every row.
 	 */
-	assert(dest_w > 1 && dest_h > 1 && dest_w <= 64);
-	int x_lo[64], x_hi[64];
+	assert(destW > 1 && destH > 1 && destW <= 64);
+	int xLo[64], xHi[64];
 	double xf[64];
-	double x = -0.5, x_inc = 16.0 / (dest_w - 1);
-	for (int i = 0; i < dest_w; i++, x += x_inc) {
-		x_lo[i] = floor(x);
-		xf[i] = x - x_lo[i];
-		x_hi[i] = x_lo[i] < 15 ? x_lo[i] + 1 : 15;
-		x_lo[i] = x_lo[i] > 0 ? x_lo[i] : 0;
+	double x = -0.5, xInc = 16.0 / (destW - 1);
+	for (int i = 0; i < destW; i++, x += xInc) {
+		xLo[i] = floor(x);
+		xf[i] = x - xLo[i];
+		xHi[i] = xLo[i] < 15 ? xLo[i] + 1 : 15;
+		xLo[i] = xLo[i] > 0 ? xLo[i] : 0;
 	}
 
 	/* Now march over the output table generating the new values. */
-	double y = -0.5, y_inc = 12.0 / (dest_h - 1);
-	for (int j = 0; j < dest_h; j++, y += y_inc) {
-		int y_lo = floor(y);
-		double yf = y - y_lo;
-		int y_hi = y_lo < 11 ? y_lo + 1 : 11;
-		y_lo = y_lo > 0 ? y_lo : 0;
-		double const *row_above = src[y_lo];
-		double const *row_below = src[y_hi];
-		for (int i = 0; i < dest_w; i++) {
-			double above = row_above[x_lo[i]] * (1 - xf[i])
-				     + row_above[x_hi[i]] * xf[i];
-			double below = row_below[x_lo[i]] * (1 - xf[i])
-				     + row_below[x_hi[i]] * xf[i];
+	double y = -0.5, yInc = 12.0 / (destH - 1);
+	for (int j = 0; j < destH; j++, y += yInc) {
+		int yLo = floor(y);
+		double yf = y - yLo;
+		int yHi = yLo < 11 ? yLo + 1 : 11;
+		yLo = yLo > 0 ? yLo : 0;
+		double const *rowAbove = src[yLo];
+		double const *rowBelow = src[yHi];
+		for (int i = 0; i < destW; i++) {
+			double above = rowAbove[xLo[i]] * (1 - xf[i])
+				     + rowAbove[xHi[i]] * xf[i];
+			double below = rowBelow[xLo[i]] * (1 - xf[i])
+				     + rowBelow[xHi[i]] * xf[i];
 			int result = floor(1024 * (above * (1 - yf) + below * yf) + .5);
 			*(dest++) = result > 16383 ? 16383 : result; /* want u4.10 */
 		}
diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
index 35dbe0fb..8d40b0ed 100644
--- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
+++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
@@ -1012,7 +1012,7 @@  int RPiCameraData::configureIPA()
 
 	/* Allocate the lens shading table via dmaHeap and pass to the IPA. */
 	if (!lsTable_.isValid()) {
-		lsTable_ = dmaHeap_.alloc("ls_grid", MAX_LS_GRID_SIZE);
+		lsTable_ = dmaHeap_.alloc("ls_grid", RPi::MaxLsGridSize);
 		if (!lsTable_.isValid())
 			return -ENOMEM;