[libcamera-devel] [PATCH 4/4] ipa: raspberrypi: Tidy up variable names to be consistent
Naushir Patuck
naush at raspberrypi.com
Thu Sep 24 10:34:09 CEST 2020
Hi David,
Thank you for the comments.
On Wed, 23 Sep 2020 at 17:31, David Plowman
<david.plowman at 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 at 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 at 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 at ideasonboard.com>
> >
> > > if (!lsTable_.isValid())
> > > return -ENOMEM;
> > >
> > >
> >
> > --
> > Regards
> > --
> > Kieran
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
More information about the libcamera-devel
mailing list