Message ID | 20200922095018.68434-5-naush@raspberrypi.com |
---|---|
State | Accepted |
Headers | show |
Series |
|
Related | show |
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
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; > >
/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
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>
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
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
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
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
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;
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(-)