[{"id":12651,"web_url":"https://patchwork.libcamera.org/comment/12651/","msgid":"<20200923080005.kdaoitkxcg6tuvzr@uno.localdomain>","date":"2020-09-23T08:00:05","subject":"Re: [libcamera-devel] [PATCH 4/4] ipa: raspberrypi: Tidy up\n\tvariable names to be consistent","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Naush,\n\nOn Tue, Sep 22, 2020 at 10:50:18AM +0100, Naushir Patuck wrote:\n> Change variable names to camel case to be consistent with the rest of\n> the source files. Remove #define consts and replace with constexpr.\n\nVery much apreciated! I haven't looked for leftovers, but\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n>\n> There are no functional changes in this commit.\n>\n> Signed-off-by: Naushir Patuck \n> ---\n> include/libcamera/ipa/raspberrypi.h | 2 +-\n> src/ipa/raspberrypi/raspberrypi.cpp | 178 +++++++++---------\n> .../pipeline/raspberrypi/raspberrypi.cpp | 2 +-\n> 3 files changed, 91 insertions(+), 91 deletions(-)\n>\n> diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h\n> index c9d4aa81..b3041591 100644\n> --- a/include/libcamera/ipa/raspberrypi.h\n> +++ b/include/libcamera/ipa/raspberrypi.h\n> @@ -41,7 +41,7 @@ enum BufferMask {\n> };\n>\n> /* Size of the LS grid allocation. */\n> -#define MAX_LS_GRID_SIZE (32 << 10)\n> +static constexpr unsigned int MaxLsGridSize = 32 << 10;\n>\n> /* List of controls handled by the Raspberry Pi IPA */\n> static const ControlInfoMap Controls = {\n> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> index 0c0dc743..c240eae8 100644\n> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> @@ -55,8 +55,8 @@\n> namespace libcamera {\n>\n> /* Configure the sensor with these values initially. */\n> -#define DEFAULT_ANALOGUE_GAIN 1.0\n> -#define DEFAULT_EXPOSURE_TIME 20000\n> +constexpr unsigned int DefaultAnalogueGain = 1.0;\n> +constexpr unsigned int DefaultExposureTime = 20000;\n>\n> LOG_DEFINE_CATEGORY(IPARPI)\n>\n> @@ -65,7 +65,7 @@ class IPARPi : public IPAInterface\n> public:\n> \tIPARPi()\n> \t\t: lastMode_({}), controller_(), controllerInit_(false),\n> -\t\t frame_count_(0), check_count_(0), mistrust_count_(0),\n> +\t\t frameCount_(0), checkCount_(0), mistrustCount_(0),\n> \t\t lsTable_(nullptr)\n> \t{\n> \t}\n> @@ -73,7 +73,7 @@ public:\n> \t~IPARPi()\n> \t{\n> \t\tif (lsTable_)\n> -\t\t\tmunmap(lsTable_, MAX_LS_GRID_SIZE);\n> +\t\t\tmunmap(lsTable_, RPi::MaxLsGridSize);\n> \t}\n>\n> \tint init(const IPASettings &settings) override;\n> @@ -108,13 +108,13 @@ private:\n> \tvoid applySharpen(const struct SharpenStatus *sharpenStatus, ControlList &ctrls);\n> \tvoid applyDPC(const struct DpcStatus *dpcStatus, ControlList &ctrls);\n> \tvoid applyLS(const struct AlscStatus *lsStatus, ControlList &ctrls);\n> -\tvoid resampleTable(uint16_t dest[], double const src[12][16], int dest_w, int dest_h);\n> +\tvoid resampleTable(uint16_t dest[], double const src[12][16], int destW, int destH);\n>\n> \tstd::map buffers_;\n> \tstd::map buffersMemory_;\n>\n> -\tControlInfoMap unicam_ctrls_;\n> -\tControlInfoMap isp_ctrls_;\n> +\tControlInfoMap unicamCtrls_;\n> +\tControlInfoMap ispCtrls_;\n> \tControlList libcameraMetadata_;\n>\n> \t/* IPA configuration. */\n> @@ -134,11 +134,11 @@ private:\n> \t * We count frames to decide if the frame must be hidden (e.g. from\n> \t * display) or mistrusted (i.e. not given to the control algos).\n> \t */\n> -\tuint64_t frame_count_;\n> +\tuint64_t frameCount_;\n> \t/* For checking the sequencing of Prepare/Process calls. */\n> -\tuint64_t check_count_;\n> +\tuint64_t checkCount_;\n> \t/* How many frames we should avoid running control algos on. */\n> -\tunsigned int mistrust_count_;\n> +\tunsigned int mistrustCount_;\n> \t/* LS table allocation passed in from the pipeline handler. */\n> \tFileDescriptor lsTableHandle_;\n> \tvoid *lsTable_;\n> @@ -199,8 +199,8 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n>\n> \tresult->operation = 0;\n>\n> -\tunicam_ctrls_ = entityControls.at(0);\n> -\tisp_ctrls_ = entityControls.at(1);\n> +\tunicamCtrls_ = entityControls.at(0);\n> +\tispCtrls_ = entityControls.at(1);\n> \t/* Setup a metadata ControlList to output metadata. */\n> \tlibcameraMetadata_ = ControlList(controls::controls);\n>\n> @@ -238,18 +238,18 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> \t *\"mistrusted\", which depends on whether this is a startup from cold,\n> \t * or merely a mode switch in a running system.\n> \t */\n> -\tframe_count_ = 0;\n> -\tcheck_count_ = 0;\n> -\tunsigned int drop_frame = 0;\n> +\tframeCount_ = 0;\n> +\tcheckCount_ = 0;\n> +\tunsigned int dropFrame = 0;\n> \tif (controllerInit_) {\n> -\t\tdrop_frame = helper_->HideFramesModeSwitch();\n> -\t\tmistrust_count_ = helper_->MistrustFramesModeSwitch();\n> +\t\tdropFrame = helper_->HideFramesModeSwitch();\n> +\t\tmistrustCount_ = helper_->MistrustFramesModeSwitch();\n> \t} else {\n> -\t\tdrop_frame = helper_->HideFramesStartup();\n> -\t\tmistrust_count_ = helper_->MistrustFramesStartup();\n> +\t\tdropFrame = helper_->HideFramesStartup();\n> +\t\tmistrustCount_ = helper_->MistrustFramesStartup();\n> \t}\n>\n> -\tresult->data.push_back(drop_frame);\n> +\tresult->data.push_back(dropFrame);\n> \tresult->operation |= RPi::IPA_CONFIG_DROP_FRAMES;\n>\n> \tstruct AgcStatus agcStatus;\n> @@ -264,8 +264,8 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> \t\tcontrollerInit_ = true;\n>\n> \t\t/* Supply initial values for gain and exposure. */\n> -\t\tagcStatus.shutter_time = DEFAULT_EXPOSURE_TIME;\n> -\t\tagcStatus.analogue_gain = DEFAULT_ANALOGUE_GAIN;\n> +\t\tagcStatus.shutter_time = DefaultExposureTime;\n> +\t\tagcStatus.analogue_gain = DefaultAnalogueGain;\n> \t}\n>\n> \tRPiController::Metadata metadata;\n> @@ -274,7 +274,7 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> \t/* SwitchMode may supply updated exposure/gain values to use. */\n> \tmetadata.Get(\"agc.status\", agcStatus);\n> \tif (agcStatus.shutter_time != 0.0 && agcStatus.analogue_gain != 0.0) {\n> -\t\tControlList ctrls(unicam_ctrls_);\n> +\t\tControlList ctrls(unicamCtrls_);\n> \t\tapplyAGC(&agcStatus, ctrls);\n> \t\tresult->controls.push_back(ctrls);\n>\n> @@ -287,14 +287,14 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> \tif (ipaConfig.operation & RPi::IPA_CONFIG_LS_TABLE) {\n> \t\t/* Remove any previous table, if there was one. */\n> \t\tif (lsTable_) {\n> -\t\t\tmunmap(lsTable_, MAX_LS_GRID_SIZE);\n> +\t\t\tmunmap(lsTable_, RPi::MaxLsGridSize);\n> \t\t\tlsTable_ = nullptr;\n> \t\t}\n>\n> \t\t/* Map the LS table buffer into user space. */\n> \t\tlsTableHandle_ = FileDescriptor(ipaConfig.data[0]);\n> \t\tif (lsTableHandle_.isValid()) {\n> -\t\t\tlsTable_ = mmap(nullptr, MAX_LS_GRID_SIZE, PROT_READ | PROT_WRITE,\n> +\t\t\tlsTable_ = mmap(nullptr, RPi::MaxLsGridSize, PROT_READ | PROT_WRITE,\n> \t\t\t\t\tMAP_SHARED, lsTableHandle_.fd(), 0);\n>\n> \t\t\tif (lsTable_ == MAP_FAILED) {\n> @@ -343,9 +343,9 @@ void IPARPi::processEvent(const IPAOperationData &event)\n> \tcase RPi::IPA_EVENT_SIGNAL_STAT_READY: {\n> \t\tunsigned int bufferId = event.data[0];\n>\n> -\t\tif (++check_count_ != frame_count_) /* assert here? */\n> +\t\tif (++checkCount_ != frameCount_) /* assert here? */\n> \t\t\tLOG(IPARPI, Error) << \"WARNING: Prepare/Process mismatch!!!\";\n> -\t\tif (frame_count_ > mistrust_count_)\n> +\t\tif (frameCount_ > mistrustCount_)\n> \t\t\tprocessStats(bufferId);\n>\n> \t\treportMetadata();\n> @@ -368,7 +368,7 @@ void IPARPi::processEvent(const IPAOperationData &event)\n> \t\t * they are \"unreliable\".\n> \t\t */\n> \t\tprepareISP(embeddedbufferId);\n> -\t\tframe_count_++;\n> +\t\tframeCount_++;\n>\n> \t\t/* Ready to push the input buffer into the ISP. */\n> \t\tIPAOperationData op;\n> @@ -713,7 +713,7 @@ void IPARPi::prepareISP(unsigned int bufferId)\n> \treturnEmbeddedBuffer(bufferId);\n>\n> \tif (success) {\n> -\t\tControlList ctrls(isp_ctrls_);\n> +\t\tControlList ctrls(ispCtrls_);\n>\n> \t\trpiMetadata_.Clear();\n> \t\trpiMetadata_.Set(\"device.status\", deviceStatus);\n> @@ -785,19 +785,19 @@ bool IPARPi::parseEmbeddedData(unsigned int bufferId, struct DeviceStatus &devic\n> \tif (status != RPiController::MdParser::Status::OK) {\n> \t\tLOG(IPARPI, Error) << \"Embedded Buffer parsing failed, error \" << status;\n> \t} else {\n> -\t\tuint32_t exposure_lines, gain_code;\n> -\t\tif (helper_->Parser().GetExposureLines(exposure_lines) != RPiController::MdParser::Status::OK) {\n> +\t\tuint32_t exposureLines, gainCode;\n> +\t\tif (helper_->Parser().GetExposureLines(exposureLines) != RPiController::MdParser::Status::OK) {\n> \t\t\tLOG(IPARPI, Error) << \"Exposure time failed\";\n> \t\t\treturn false;\n> \t\t}\n>\n> -\t\tdeviceStatus.shutter_speed = helper_->Exposure(exposure_lines);\n> -\t\tif (helper_->Parser().GetGainCode(gain_code) != RPiController::MdParser::Status::OK) {\n> +\t\tdeviceStatus.shutter_speed = helper_->Exposure(exposureLines);\n> +\t\tif (helper_->Parser().GetGainCode(gainCode) != RPiController::MdParser::Status::OK) {\n> \t\t\tLOG(IPARPI, Error) << \"Gain failed\";\n> \t\t\treturn false;\n> \t\t}\n>\n> -\t\tdeviceStatus.analogue_gain = helper_->Gain(gain_code);\n> +\t\tdeviceStatus.analogue_gain = helper_->Gain(gainCode);\n> \t\tLOG(IPARPI, Debug) << \"Metadata - Exposure : \"\n> \t\t\t\t << deviceStatus.shutter_speed << \" Gain : \"\n> \t\t\t\t << deviceStatus.analogue_gain;\n> @@ -820,7 +820,7 @@ void IPARPi::processStats(unsigned int bufferId)\n>\n> \tstruct AgcStatus agcStatus;\n> \tif (rpiMetadata_.Get(\"agc.status\", agcStatus) == 0) {\n> -\t\tControlList ctrls(unicam_ctrls_);\n> +\t\tControlList ctrls(unicamCtrls_);\n> \t\tapplyAGC(&agcStatus, ctrls);\n>\n> \t\tIPAOperationData op;\n> @@ -832,14 +832,14 @@ void IPARPi::processStats(unsigned int bufferId)\n>\n> void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls)\n> {\n> -\tconst auto gainR = isp_ctrls_.find(V4L2_CID_RED_BALANCE);\n> -\tif (gainR == isp_ctrls_.end()) {\n> +\tconst auto gainR = ispCtrls_.find(V4L2_CID_RED_BALANCE);\n> +\tif (gainR == ispCtrls_.end()) {\n> \t\tLOG(IPARPI, Error) << \"Can't find red gain control\";\n> \t\treturn;\n> \t}\n>\n> -\tconst auto gainB = isp_ctrls_.find(V4L2_CID_BLUE_BALANCE);\n> -\tif (gainB == isp_ctrls_.end()) {\n> +\tconst auto gainB = ispCtrls_.find(V4L2_CID_BLUE_BALANCE);\n> +\tif (gainB == ispCtrls_.end()) {\n> \t\tLOG(IPARPI, Error) << \"Can't find blue gain control\";\n> \t\treturn;\n> \t}\n> @@ -855,31 +855,31 @@ void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls)\n>\n> void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls)\n> {\n> -\tint32_t gain_code = helper_->GainCode(agcStatus->analogue_gain);\n> -\tint32_t exposure_lines = helper_->ExposureLines(agcStatus->shutter_time);\n> +\tint32_t gainCode = helper_->GainCode(agcStatus->analogue_gain);\n> +\tint32_t exposureLines = helper_->ExposureLines(agcStatus->shutter_time);\n>\n> -\tif (unicam_ctrls_.find(V4L2_CID_ANALOGUE_GAIN) == unicam_ctrls_.end()) {\n> +\tif (unicamCtrls_.find(V4L2_CID_ANALOGUE_GAIN) == unicamCtrls_.end()) {\n> \t\tLOG(IPARPI, Error) << \"Can't find analogue gain control\";\n> \t\treturn;\n> \t}\n>\n> -\tif (unicam_ctrls_.find(V4L2_CID_EXPOSURE) == unicam_ctrls_.end()) {\n> +\tif (unicamCtrls_.find(V4L2_CID_EXPOSURE) == unicamCtrls_.end()) {\n> \t\tLOG(IPARPI, Error) << \"Can't find exposure control\";\n> \t\treturn;\n> \t}\n>\n> \tLOG(IPARPI, Debug) << \"Applying AGC Exposure: \" << agcStatus->shutter_time\n> -\t\t\t << \" (Shutter lines: \" << exposure_lines << \") Gain: \"\n> +\t\t\t << \" (Shutter lines: \" << exposureLines << \") Gain: \"\n> \t\t\t << agcStatus->analogue_gain << \" (Gain Code: \"\n> -\t\t\t << gain_code << \")\";\n> +\t\t\t << gainCode << \")\";\n>\n> -\tctrls.set(V4L2_CID_ANALOGUE_GAIN, gain_code);\n> -\tctrls.set(V4L2_CID_EXPOSURE, exposure_lines);\n> +\tctrls.set(V4L2_CID_ANALOGUE_GAIN, gainCode);\n> +\tctrls.set(V4L2_CID_EXPOSURE, exposureLines);\n> }\n>\n> void IPARPi::applyDG(const struct AgcStatus *dgStatus, ControlList &ctrls)\n> {\n> -\tif (isp_ctrls_.find(V4L2_CID_DIGITAL_GAIN) == isp_ctrls_.end()) {\n> +\tif (ispCtrls_.find(V4L2_CID_DIGITAL_GAIN) == ispCtrls_.end()) {\n> \t\tLOG(IPARPI, Error) << \"Can't find digital gain control\";\n> \t\treturn;\n> \t}\n> @@ -890,7 +890,7 @@ void IPARPi::applyDG(const struct AgcStatus *dgStatus, ControlList &ctrls)\n>\n> void IPARPi::applyCCM(const struct CcmStatus *ccmStatus, ControlList &ctrls)\n> {\n> -\tif (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_CC_MATRIX) == isp_ctrls_.end()) {\n> +\tif (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_CC_MATRIX) == ispCtrls_.end()) {\n> \t\tLOG(IPARPI, Error) << \"Can't find CCM control\";\n> \t\treturn;\n> \t}\n> @@ -911,7 +911,7 @@ void IPARPi::applyCCM(const struct CcmStatus *ccmStatus, ControlList &ctrls)\n>\n> void IPARPi::applyGamma(const struct ContrastStatus *contrastStatus, ControlList &ctrls)\n> {\n> -\tif (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_GAMMA) == isp_ctrls_.end()) {\n> +\tif (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_GAMMA) == ispCtrls_.end()) {\n> \t\tLOG(IPARPI, Error) << \"Can't find Gamma control\";\n> \t\treturn;\n> \t}\n> @@ -930,7 +930,7 @@ void IPARPi::applyGamma(const struct ContrastStatus *contrastStatus, ControlList\n>\n> void IPARPi::applyBlackLevel(const struct BlackLevelStatus *blackLevelStatus, ControlList &ctrls)\n> {\n> -\tif (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_BLACK_LEVEL) == isp_ctrls_.end()) {\n> +\tif (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_BLACK_LEVEL) == ispCtrls_.end()) {\n> \t\tLOG(IPARPI, Error) << \"Can't find black level control\";\n> \t\treturn;\n> \t}\n> @@ -948,7 +948,7 @@ void IPARPi::applyBlackLevel(const struct BlackLevelStatus *blackLevelStatus, Co\n>\n> void IPARPi::applyGEQ(const struct GeqStatus *geqStatus, ControlList &ctrls)\n> {\n> -\tif (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_GEQ) == isp_ctrls_.end()) {\n> +\tif (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_GEQ) == ispCtrls_.end()) {\n> \t\tLOG(IPARPI, Error) << \"Can't find geq control\";\n> \t\treturn;\n> \t}\n> @@ -966,7 +966,7 @@ void IPARPi::applyGEQ(const struct GeqStatus *geqStatus, ControlList &ctrls)\n>\n> void IPARPi::applyDenoise(const struct SdnStatus *denoiseStatus, ControlList &ctrls)\n> {\n> -\tif (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_DENOISE) == isp_ctrls_.end()) {\n> +\tif (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_DENOISE) == ispCtrls_.end()) {\n> \t\tLOG(IPARPI, Error) << \"Can't find denoise control\";\n> \t\treturn;\n> \t}\n> @@ -986,7 +986,7 @@ void IPARPi::applyDenoise(const struct SdnStatus *denoiseStatus, ControlList &ct\n>\n> void IPARPi::applySharpen(const struct SharpenStatus *sharpenStatus, ControlList &ctrls)\n> {\n> -\tif (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_SHARPEN) == isp_ctrls_.end()) {\n> +\tif (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_SHARPEN) == ispCtrls_.end()) {\n> \t\tLOG(IPARPI, Error) << \"Can't find sharpen control\";\n> \t\treturn;\n> \t}\n> @@ -1007,7 +1007,7 @@ void IPARPi::applySharpen(const struct SharpenStatus *sharpenStatus, ControlList\n>\n> void IPARPi::applyDPC(const struct DpcStatus *dpcStatus, ControlList &ctrls)\n> {\n> -\tif (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_DPC) == isp_ctrls_.end()) {\n> +\tif (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_DPC) == ispCtrls_.end()) {\n> \t\tLOG(IPARPI, Error) << \"Can't find DPC control\";\n> \t\treturn;\n> \t}\n> @@ -1023,7 +1023,7 @@ void IPARPi::applyDPC(const struct DpcStatus *dpcStatus, ControlList &ctrls)\n>\n> void IPARPi::applyLS(const struct AlscStatus *lsStatus, ControlList &ctrls)\n> {\n> -\tif (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING) == isp_ctrls_.end()) {\n> +\tif (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING) == ispCtrls_.end()) {\n> \t\tLOG(IPARPI, Error) << \"Can't find LS control\";\n> \t\treturn;\n> \t}\n> @@ -1032,18 +1032,18 @@ void IPARPi::applyLS(const struct AlscStatus *lsStatus, ControlList &ctrls)\n> \t * Program lens shading tables into pipeline.\n> \t * Choose smallest cell size that won't exceed 63x48 cells.\n> \t */\n> -\tconst int cell_sizes[] = { 16, 32, 64, 128, 256 };\n> -\tunsigned int num_cells = ARRAY_SIZE(cell_sizes);\n> -\tunsigned int i, w, h, cell_size;\n> -\tfor (i = 0; i < num_cells; i++) {\n> -\t\tcell_size = cell_sizes[i];\n> -\t\tw = (mode_.width + cell_size - 1) / cell_size;\n> -\t\th = (mode_.height + cell_size - 1) / cell_size;\n> +\tconst int cellSizes[] = { 16, 32, 64, 128, 256 };\n> +\tunsigned int numCells = ARRAY_SIZE(cellSizes);\n> +\tunsigned int i, w, h, cellSize;\n> +\tfor (i = 0; i < numCells; i++) {\n> +\t\tcellSize = cellSizes[i];\n> +\t\tw = (mode_.width + cellSize - 1) / cellSize;\n> +\t\th = (mode_.height + cellSize - 1) / cellSize;\n> \t\tif (w < 64 && h <= 48)\n> \t\t\tbreak;\n> \t}\n>\n> -\tif (i == num_cells) {\n> +\tif (i == numCells) {\n> \t\tLOG(IPARPI, Error) << \"Cannot find cell size\";\n> \t\treturn;\n> \t}\n> @@ -1052,7 +1052,7 @@ void IPARPi::applyLS(const struct AlscStatus *lsStatus, ControlList &ctrls)\n> \tw++, h++;\n> \tbcm2835_isp_lens_shading ls = {\n> \t\t.enabled = 1,\n> -\t\t.grid_cell_size = cell_size,\n> +\t\t.grid_cell_size = cellSize,\n> \t\t.grid_width = w,\n> \t\t.grid_stride = w,\n> \t\t.grid_height = h,\n> @@ -1062,7 +1062,7 @@ void IPARPi::applyLS(const struct AlscStatus *lsStatus, ControlList &ctrls)\n> \t\t.gain_format = GAIN_FORMAT_U4P10\n> \t};\n>\n> -\tif (!lsTable_ || w * h * 4 * sizeof(uint16_t) > MAX_LS_GRID_SIZE) {\n> +\tif (!lsTable_ || w * h * 4 * sizeof(uint16_t) > RPi::MaxLsGridSize) {\n> \t\tLOG(IPARPI, Error) << \"Do not have a correctly allocate lens shading table!\";\n> \t\treturn;\n> \t}\n> @@ -1083,41 +1083,41 @@ void IPARPi::applyLS(const struct AlscStatus *lsStatus, ControlList &ctrls)\n> }\n>\n> /*\n> - * Resamples a 16x12 table with central sampling to dest_w x dest_h with corner\n> + * Resamples a 16x12 table with central sampling to destW x destH with corner\n> * sampling.\n> */\n> void IPARPi::resampleTable(uint16_t dest[], double const src[12][16],\n> -\t\t\t int dest_w, int dest_h)\n> +\t\t\t int destW, int destH)\n> {\n> \t/*\n> \t * Precalculate and cache the x sampling locations and phases to\n> \t * save recomputing them on every row.\n> \t */\n> -\tassert(dest_w > 1 && dest_h > 1 && dest_w <= 64);\n> -\tint x_lo[64], x_hi[64];\n> +\tassert(destW > 1 && destH > 1 && destW <= 64);\n> +\tint xLo[64], xHi[64];\n> \tdouble xf[64];\n> -\tdouble x = -0.5, x_inc = 16.0 / (dest_w - 1);\n> -\tfor (int i = 0; i < dest_w; i++, x += x_inc) {\n> -\t\tx_lo[i] = floor(x);\n> -\t\txf[i] = x - x_lo[i];\n> -\t\tx_hi[i] = x_lo[i] < 15 ? x_lo[i] + 1 : 15;\n> -\t\tx_lo[i] = x_lo[i] > 0 ? x_lo[i] : 0;\n> +\tdouble x = -0.5, xInc = 16.0 / (destW - 1);\n> +\tfor (int i = 0; i < destW; i++, x += xInc) {\n> +\t\txLo[i] = floor(x);\n> +\t\txf[i] = x - xLo[i];\n> +\t\txHi[i] = xLo[i] < 15 ? xLo[i] + 1 : 15;\n> +\t\txLo[i] = xLo[i] > 0 ? xLo[i] : 0;\n> \t}\n>\n> \t/* Now march over the output table generating the new values. */\n> -\tdouble y = -0.5, y_inc = 12.0 / (dest_h - 1);\n> -\tfor (int j = 0; j < dest_h; j++, y += y_inc) {\n> -\t\tint y_lo = floor(y);\n> -\t\tdouble yf = y - y_lo;\n> -\t\tint y_hi = y_lo < 11 ? y_lo + 1 : 11;\n> -\t\ty_lo = y_lo > 0 ? y_lo : 0;\n> -\t\tdouble const *row_above = src[y_lo];\n> -\t\tdouble const *row_below = src[y_hi];\n> -\t\tfor (int i = 0; i < dest_w; i++) {\n> -\t\t\tdouble above = row_above[x_lo[i]] * (1 - xf[i])\n> -\t\t\t\t + row_above[x_hi[i]] * xf[i];\n> -\t\t\tdouble below = row_below[x_lo[i]] * (1 - xf[i])\n> -\t\t\t\t + row_below[x_hi[i]] * xf[i];\n> +\tdouble y = -0.5, yInc = 12.0 / (destH - 1);\n> +\tfor (int j = 0; j < destH; j++, y += yInc) {\n> +\t\tint yLo = floor(y);\n> +\t\tdouble yf = y - yLo;\n> +\t\tint yHi = yLo < 11 ? yLo + 1 : 11;\n> +\t\tyLo = yLo > 0 ? yLo : 0;\n> +\t\tdouble const *rowAbove = src[yLo];\n> +\t\tdouble const *rowBelow = src[yHi];\n> +\t\tfor (int i = 0; i < destW; i++) {\n> +\t\t\tdouble above = rowAbove[xLo[i]] * (1 - xf[i])\n> +\t\t\t\t + rowAbove[xHi[i]] * xf[i];\n> +\t\t\tdouble below = rowBelow[xLo[i]] * (1 - xf[i])\n> +\t\t\t\t + rowBelow[xHi[i]] * xf[i];\n> \t\t\tint result = floor(1024 * (above * (1 - yf) + below * yf) + .5);\n> \t\t\t*(dest++) = result > 16383 ? 16383 : result; /* want u4.10 */\n> \t\t}\n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index 35dbe0fb..8d40b0ed 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -1012,7 +1012,7 @@ int RPiCameraData::configureIPA()\n>\n> \t/* Allocate the lens shading table via dmaHeap and pass to the IPA. */\n> \tif (!lsTable_.isValid()) {\n> -\t\tlsTable_ = dmaHeap_.alloc(\"ls_grid\", MAX_LS_GRID_SIZE);\n> +\t\tlsTable_ = dmaHeap_.alloc(\"ls_grid\", RPi::MaxLsGridSize);\n> \t\tif (!lsTable_.isValid())\n> \t\t\treturn -ENOMEM;\n>\n> --\n> 2.25.1\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 15C77C3B5C\n\tfor ;\n\tWed, 23 Sep 2020 07:56:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8C28762FDC;\n\tWed, 23 Sep 2020 09:56:14 +0200 (CEST)","from relay1-d.mail.gandi.net (relay1-d.mail.gandi.net\n\t[217.70.183.193])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3E37E60363\n\tfor ;\n\tWed, 23 Sep 2020 09:56:13 +0200 (CEST)","from uno.localdomain (93-34-118-233.ip49.fastwebnet.it\n\t[93.34.118.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay1-d.mail.gandi.net (Postfix) with ESMTPSA id A89F5240008;\n\tWed, 23 Sep 2020 07:56:12 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Wed, 23 Sep 2020 10:00:05 +0200","From":"Jacopo Mondi ","To":"Naushir Patuck ","Message-ID":"<20200923080005.kdaoitkxcg6tuvzr@uno.localdomain>","References":"<20200922095018.68434-1-naush@raspberrypi.com>\n\t<20200922095018.68434-5-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200922095018.68434-5-naush@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH 4/4] ipa: raspberrypi: Tidy up\n\tvariable names to be consistent","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":12665,"web_url":"https://patchwork.libcamera.org/comment/12665/","msgid":"","date":"2020-09-23T11:18:48","subject":"Re: [libcamera-devel] [PATCH 4/4] ipa: raspberrypi: Tidy up\n\tvariable names to be consistent","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Naush,\n\nOn 22/09/2020 10:50, Naushir Patuck wrote:\n> Change variable names to camel case to be consistent with the rest of\n> the source files. Remove #define consts and replace with constexpr.\n> \n\nSounds good to me...\n\n\n> There are no functional changes in this commit.\n> \n> Signed-off-by: Naushir Patuck \n> ---\n> include/libcamera/ipa/raspberrypi.h | 2 +-\n> src/ipa/raspberrypi/raspberrypi.cpp | 178 +++++++++---------\n> .../pipeline/raspberrypi/raspberrypi.cpp | 2 +-\n> 3 files changed, 91 insertions(+), 91 deletions(-)\n> \n> diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h\n> index c9d4aa81..b3041591 100644\n> --- a/include/libcamera/ipa/raspberrypi.h\n> +++ b/include/libcamera/ipa/raspberrypi.h\n> @@ -41,7 +41,7 @@ enum BufferMask {\n> };\n> \n> /* Size of the LS grid allocation. */\n> -#define MAX_LS_GRID_SIZE (32 << 10)\n> +static constexpr unsigned int MaxLsGridSize = 32 << 10;\n\nI guess the LS could stay capitalised (MaxLSGridSize), not sure that it\nmatters, so up to you.\n\n> /* List of controls handled by the Raspberry Pi IPA */\n> static const ControlInfoMap Controls = {\n> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> index 0c0dc743..c240eae8 100644\n> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> @@ -55,8 +55,8 @@\n> namespace libcamera {\n> \n> /* Configure the sensor with these values initially. */\n> -#define DEFAULT_ANALOGUE_GAIN 1.0\n> -#define DEFAULT_EXPOSURE_TIME 20000\n> +constexpr unsigned int DefaultAnalogueGain = 1.0;\n> +constexpr unsigned int DefaultExposureTime = 20000;\n\nOh nice, This really highlights to me the benefit of constexpr.\nNow the values have types!\n\n> \n> LOG_DEFINE_CATEGORY(IPARPI)\n> \n> @@ -65,7 +65,7 @@ class IPARPi : public IPAInterface\n> public:\n> \tIPARPi()\n> \t\t: lastMode_({}), controller_(), controllerInit_(false),\n> -\t\t frame_count_(0), check_count_(0), mistrust_count_(0),\n> +\t\t frameCount_(0), checkCount_(0), mistrustCount_(0),\n> \t\t lsTable_(nullptr)\n> \t{\n> \t}\n> @@ -73,7 +73,7 @@ public:\n> \t~IPARPi()\n> \t{\n> \t\tif (lsTable_)\n> -\t\t\tmunmap(lsTable_, MAX_LS_GRID_SIZE);\n> +\t\t\tmunmap(lsTable_, RPi::MaxLsGridSize);\n> \t}\n> \n> \tint init(const IPASettings &settings) override;\n> @@ -108,13 +108,13 @@ private:\n> \tvoid applySharpen(const struct SharpenStatus *sharpenStatus, ControlList &ctrls);\n> \tvoid applyDPC(const struct DpcStatus *dpcStatus, ControlList &ctrls);\n> \tvoid applyLS(const struct AlscStatus *lsStatus, ControlList &ctrls);\n> -\tvoid resampleTable(uint16_t dest[], double const src[12][16], int dest_w, int dest_h);\n> +\tvoid resampleTable(uint16_t dest[], double const src[12][16], int destW, int destH);\n> \n> \tstd::map buffers_;\n> \tstd::map buffersMemory_;\n> \n> -\tControlInfoMap unicam_ctrls_;\n> -\tControlInfoMap isp_ctrls_;\n> +\tControlInfoMap unicamCtrls_;\n> +\tControlInfoMap ispCtrls_;\n> \tControlList libcameraMetadata_;\n> \n> \t/* IPA configuration. */\n> @@ -134,11 +134,11 @@ private:\n> \t * We count frames to decide if the frame must be hidden (e.g. from\n> \t * display) or mistrusted (i.e. not given to the control algos).\n> \t */\n> -\tuint64_t frame_count_;\n> +\tuint64_t frameCount_;\n\nIf we're doing clean up - I always prefer to see a newline before a\ncomment line.\n\nTo me it makes the commented block clearer - but I don't know if there's\na 'rule' on it.\n\n\n> \t/* For checking the sequencing of Prepare/Process calls. */\n> -\tuint64_t check_count_;\n> +\tuint64_t checkCount_;\n\ni.e. here too.\n\n> \t/* How many frames we should avoid running control algos on. */\n> -\tunsigned int mistrust_count_;\n> +\tunsigned int mistrustCount_;\n\nand so on.\n\n> \t/* LS table allocation passed in from the pipeline handler. */\n> \tFileDescriptor lsTableHandle_;\n> \tvoid *lsTable_;\n> @@ -199,8 +199,8 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> \n> \tresult->operation = 0;\n> \n> -\tunicam_ctrls_ = entityControls.at(0);\n> -\tisp_ctrls_ = entityControls.at(1);\n> +\tunicamCtrls_ = entityControls.at(0);\n> +\tispCtrls_ = entityControls.at(1);\n> \t/* Setup a metadata ControlList to output metadata. */\n> \tlibcameraMetadata_ = ControlList(controls::controls);\n> \n> @@ -238,18 +238,18 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> \t *\"mistrusted\", which depends on whether this is a startup from cold,\n> \t * or merely a mode switch in a running system.\n> \t */\n> -\tframe_count_ = 0;\n> -\tcheck_count_ = 0;\n> -\tunsigned int drop_frame = 0;\n> +\tframeCount_ = 0;\n> +\tcheckCount_ = 0;\n> +\tunsigned int dropFrame = 0;\n> \tif (controllerInit_) {\n> -\t\tdrop_frame = helper_->HideFramesModeSwitch();\n> -\t\tmistrust_count_ = helper_->MistrustFramesModeSwitch();\n> +\t\tdropFrame = helper_->HideFramesModeSwitch();\n> +\t\tmistrustCount_ = helper_->MistrustFramesModeSwitch();\n> \t} else {\n> -\t\tdrop_frame = helper_->HideFramesStartup();\n> -\t\tmistrust_count_ = helper_->MistrustFramesStartup();\n> +\t\tdropFrame = helper_->HideFramesStartup();\n> +\t\tmistrustCount_ = helper_->MistrustFramesStartup();\n> \t}\n> \n> -\tresult->data.push_back(drop_frame);\n> +\tresult->data.push_back(dropFrame);\n> \tresult->operation |= RPi::IPA_CONFIG_DROP_FRAMES;\n> \n> \tstruct AgcStatus agcStatus;\n> @@ -264,8 +264,8 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> \t\tcontrollerInit_ = true;\n> \n> \t\t/* Supply initial values for gain and exposure. */\n> -\t\tagcStatus.shutter_time = DEFAULT_EXPOSURE_TIME;\n> -\t\tagcStatus.analogue_gain = DEFAULT_ANALOGUE_GAIN;\n> +\t\tagcStatus.shutter_time = DefaultExposureTime;\n> +\t\tagcStatus.analogue_gain = DefaultAnalogueGain;\n> \t}\n> \n> \tRPiController::Metadata metadata;\n> @@ -274,7 +274,7 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> \t/* SwitchMode may supply updated exposure/gain values to use. */\n> \tmetadata.Get(\"agc.status\", agcStatus);\n> \tif (agcStatus.shutter_time != 0.0 && agcStatus.analogue_gain != 0.0) {\n> -\t\tControlList ctrls(unicam_ctrls_);\n> +\t\tControlList ctrls(unicamCtrls_);\n> \t\tapplyAGC(&agcStatus, ctrls);\n> \t\tresult->controls.push_back(ctrls);\n> \n> @@ -287,14 +287,14 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> \tif (ipaConfig.operation & RPi::IPA_CONFIG_LS_TABLE) {\n> \t\t/* Remove any previous table, if there was one. */\n> \t\tif (lsTable_) {\n> -\t\t\tmunmap(lsTable_, MAX_LS_GRID_SIZE);\n> +\t\t\tmunmap(lsTable_, RPi::MaxLsGridSize);\n> \t\t\tlsTable_ = nullptr;\n> \t\t}\n> \n> \t\t/* Map the LS table buffer into user space. */\n> \t\tlsTableHandle_ = FileDescriptor(ipaConfig.data[0]);\n> \t\tif (lsTableHandle_.isValid()) {\n> -\t\t\tlsTable_ = mmap(nullptr, MAX_LS_GRID_SIZE, PROT_READ | PROT_WRITE,\n> +\t\t\tlsTable_ = mmap(nullptr, RPi::MaxLsGridSize, PROT_READ | PROT_WRITE,\n> \t\t\t\t\tMAP_SHARED, lsTableHandle_.fd(), 0);\n> \n> \t\t\tif (lsTable_ == MAP_FAILED) {\n> @@ -343,9 +343,9 @@ void IPARPi::processEvent(const IPAOperationData &event)\n> \tcase RPi::IPA_EVENT_SIGNAL_STAT_READY: {\n> \t\tunsigned int bufferId = event.data[0];\n> \n> -\t\tif (++check_count_ != frame_count_) /* assert here? */\n> +\t\tif (++checkCount_ != frameCount_) /* assert here? */\n> \t\t\tLOG(IPARPI, Error) << \"WARNING: Prepare/Process mismatch!!!\";\n> -\t\tif (frame_count_ > mistrust_count_)\n> +\t\tif (frameCount_ > mistrustCount_)\n> \t\t\tprocessStats(bufferId);\n> \n> \t\treportMetadata();\n> @@ -368,7 +368,7 @@ void IPARPi::processEvent(const IPAOperationData &event)\n> \t\t * they are \"unreliable\".\n> \t\t */\n> \t\tprepareISP(embeddedbufferId);\n> -\t\tframe_count_++;\n> +\t\tframeCount_++;\n> \n> \t\t/* Ready to push the input buffer into the ISP. */\n> \t\tIPAOperationData op;\n> @@ -713,7 +713,7 @@ void IPARPi::prepareISP(unsigned int bufferId)\n> \treturnEmbeddedBuffer(bufferId);\n> \n> \tif (success) {\n> -\t\tControlList ctrls(isp_ctrls_);\n> +\t\tControlList ctrls(ispCtrls_);\n> \n> \t\trpiMetadata_.Clear();\n> \t\trpiMetadata_.Set(\"device.status\", deviceStatus);\n> @@ -785,19 +785,19 @@ bool IPARPi::parseEmbeddedData(unsigned int bufferId, struct DeviceStatus &devic\n> \tif (status != RPiController::MdParser::Status::OK) {\n> \t\tLOG(IPARPI, Error) << \"Embedded Buffer parsing failed, error \" << status;\n> \t} else {\n> -\t\tuint32_t exposure_lines, gain_code;\n> -\t\tif (helper_->Parser().GetExposureLines(exposure_lines) != RPiController::MdParser::Status::OK) {\n> +\t\tuint32_t exposureLines, gainCode;\n> +\t\tif (helper_->Parser().GetExposureLines(exposureLines) != RPiController::MdParser::Status::OK) {\n> \t\t\tLOG(IPARPI, Error) << \"Exposure time failed\";\n> \t\t\treturn false;\n> \t\t}\n> \n> -\t\tdeviceStatus.shutter_speed = helper_->Exposure(exposure_lines);\n> -\t\tif (helper_->Parser().GetGainCode(gain_code) != RPiController::MdParser::Status::OK) {\n> +\t\tdeviceStatus.shutter_speed = helper_->Exposure(exposureLines);\n> +\t\tif (helper_->Parser().GetGainCode(gainCode) != RPiController::MdParser::Status::OK) {\n> \t\t\tLOG(IPARPI, Error) << \"Gain failed\";\n> \t\t\treturn false;\n> \t\t}\n> \n> -\t\tdeviceStatus.analogue_gain = helper_->Gain(gain_code);\n> +\t\tdeviceStatus.analogue_gain = helper_->Gain(gainCode);\n> \t\tLOG(IPARPI, Debug) << \"Metadata - Exposure : \"\n> \t\t\t\t << deviceStatus.shutter_speed << \" Gain : \"\n> \t\t\t\t << deviceStatus.analogue_gain;\n> @@ -820,7 +820,7 @@ void IPARPi::processStats(unsigned int bufferId)\n> \n> \tstruct AgcStatus agcStatus;\n> \tif (rpiMetadata_.Get(\"agc.status\", agcStatus) == 0) {\n> -\t\tControlList ctrls(unicam_ctrls_);\n> +\t\tControlList ctrls(unicamCtrls_);\n> \t\tapplyAGC(&agcStatus, ctrls);\n> \n> \t\tIPAOperationData op;\n> @@ -832,14 +832,14 @@ void IPARPi::processStats(unsigned int bufferId)\n> \n> void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls)\n> {\n> -\tconst auto gainR = isp_ctrls_.find(V4L2_CID_RED_BALANCE);\n> -\tif (gainR == isp_ctrls_.end()) {\n> +\tconst auto gainR = ispCtrls_.find(V4L2_CID_RED_BALANCE);\n> +\tif (gainR == ispCtrls_.end()) {\n> \t\tLOG(IPARPI, Error) << \"Can't find red gain control\";\n> \t\treturn;\n> \t}\n> \n> -\tconst auto gainB = isp_ctrls_.find(V4L2_CID_BLUE_BALANCE);\n> -\tif (gainB == isp_ctrls_.end()) {\n> +\tconst auto gainB = ispCtrls_.find(V4L2_CID_BLUE_BALANCE);\n> +\tif (gainB == ispCtrls_.end()) {\n> \t\tLOG(IPARPI, Error) << \"Can't find blue gain control\";\n> \t\treturn;\n> \t}\n> @@ -855,31 +855,31 @@ void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls)\n> \n> void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls)\n> {\n> -\tint32_t gain_code = helper_->GainCode(agcStatus->analogue_gain);\n> -\tint32_t exposure_lines = helper_->ExposureLines(agcStatus->shutter_time);\n> +\tint32_t gainCode = helper_->GainCode(agcStatus->analogue_gain);\n> +\tint32_t exposureLines = helper_->ExposureLines(agcStatus->shutter_time);\n> \n> -\tif (unicam_ctrls_.find(V4L2_CID_ANALOGUE_GAIN) == unicam_ctrls_.end()) {\n> +\tif (unicamCtrls_.find(V4L2_CID_ANALOGUE_GAIN) == unicamCtrls_.end()) {\n> \t\tLOG(IPARPI, Error) << \"Can't find analogue gain control\";\n> \t\treturn;\n> \t}\n> \n> -\tif (unicam_ctrls_.find(V4L2_CID_EXPOSURE) == unicam_ctrls_.end()) {\n> +\tif (unicamCtrls_.find(V4L2_CID_EXPOSURE) == unicamCtrls_.end()) {\n> \t\tLOG(IPARPI, Error) << \"Can't find exposure control\";\n> \t\treturn;\n> \t}\n> \n> \tLOG(IPARPI, Debug) << \"Applying AGC Exposure: \" << agcStatus->shutter_time\n> -\t\t\t << \" (Shutter lines: \" << exposure_lines << \") Gain: \"\n> +\t\t\t << \" (Shutter lines: \" << exposureLines << \") Gain: \"\n> \t\t\t << agcStatus->analogue_gain << \" (Gain Code: \"\n> -\t\t\t << gain_code << \")\";\n> +\t\t\t << gainCode << \")\";\n> \n> -\tctrls.set(V4L2_CID_ANALOGUE_GAIN, gain_code);\n> -\tctrls.set(V4L2_CID_EXPOSURE, exposure_lines);\n> +\tctrls.set(V4L2_CID_ANALOGUE_GAIN, gainCode);\n> +\tctrls.set(V4L2_CID_EXPOSURE, exposureLines);\n> }\n> \n> void IPARPi::applyDG(const struct AgcStatus *dgStatus, ControlList &ctrls)\n> {\n> -\tif (isp_ctrls_.find(V4L2_CID_DIGITAL_GAIN) == isp_ctrls_.end()) {\n> +\tif (ispCtrls_.find(V4L2_CID_DIGITAL_GAIN) == ispCtrls_.end()) {\n> \t\tLOG(IPARPI, Error) << \"Can't find digital gain control\";\n> \t\treturn;\n> \t}\n> @@ -890,7 +890,7 @@ void IPARPi::applyDG(const struct AgcStatus *dgStatus, ControlList &ctrls)\n> \n> void IPARPi::applyCCM(const struct CcmStatus *ccmStatus, ControlList &ctrls)\n> {\n> -\tif (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_CC_MATRIX) == isp_ctrls_.end()) {\n> +\tif (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_CC_MATRIX) == ispCtrls_.end()) {\n> \t\tLOG(IPARPI, Error) << \"Can't find CCM control\";\n> \t\treturn;\n> \t}\n> @@ -911,7 +911,7 @@ void IPARPi::applyCCM(const struct CcmStatus *ccmStatus, ControlList &ctrls)\n> \n> void IPARPi::applyGamma(const struct ContrastStatus *contrastStatus, ControlList &ctrls)\n> {\n> -\tif (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_GAMMA) == isp_ctrls_.end()) {\n> +\tif (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_GAMMA) == ispCtrls_.end()) {\n> \t\tLOG(IPARPI, Error) << \"Can't find Gamma control\";\n> \t\treturn;\n> \t}\n> @@ -930,7 +930,7 @@ void IPARPi::applyGamma(const struct ContrastStatus *contrastStatus, ControlList\n> \n> void IPARPi::applyBlackLevel(const struct BlackLevelStatus *blackLevelStatus, ControlList &ctrls)\n> {\n> -\tif (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_BLACK_LEVEL) == isp_ctrls_.end()) {\n> +\tif (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_BLACK_LEVEL) == ispCtrls_.end()) {\n> \t\tLOG(IPARPI, Error) << \"Can't find black level control\";\n> \t\treturn;\n> \t}\n> @@ -948,7 +948,7 @@ void IPARPi::applyBlackLevel(const struct BlackLevelStatus *blackLevelStatus, Co\n> \n> void IPARPi::applyGEQ(const struct GeqStatus *geqStatus, ControlList &ctrls)\n> {\n> -\tif (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_GEQ) == isp_ctrls_.end()) {\n> +\tif (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_GEQ) == ispCtrls_.end()) {\n> \t\tLOG(IPARPI, Error) << \"Can't find geq control\";\n> \t\treturn;\n> \t}\n> @@ -966,7 +966,7 @@ void IPARPi::applyGEQ(const struct GeqStatus *geqStatus, ControlList &ctrls)\n> \n> void IPARPi::applyDenoise(const struct SdnStatus *denoiseStatus, ControlList &ctrls)\n> {\n> -\tif (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_DENOISE) == isp_ctrls_.end()) {\n> +\tif (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_DENOISE) == ispCtrls_.end()) {\n> \t\tLOG(IPARPI, Error) << \"Can't find denoise control\";\n> \t\treturn;\n> \t}\n> @@ -986,7 +986,7 @@ void IPARPi::applyDenoise(const struct SdnStatus *denoiseStatus, ControlList &ct\n> \n> void IPARPi::applySharpen(const struct SharpenStatus *sharpenStatus, ControlList &ctrls)\n> {\n> -\tif (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_SHARPEN) == isp_ctrls_.end()) {\n> +\tif (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_SHARPEN) == ispCtrls_.end()) {\n> \t\tLOG(IPARPI, Error) << \"Can't find sharpen control\";\n> \t\treturn;\n> \t}\n> @@ -1007,7 +1007,7 @@ void IPARPi::applySharpen(const struct SharpenStatus *sharpenStatus, ControlList\n> \n> void IPARPi::applyDPC(const struct DpcStatus *dpcStatus, ControlList &ctrls)\n> {\n> -\tif (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_DPC) == isp_ctrls_.end()) {\n> +\tif (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_DPC) == ispCtrls_.end()) {\n> \t\tLOG(IPARPI, Error) << \"Can't find DPC control\";\n> \t\treturn;\n> \t}\n> @@ -1023,7 +1023,7 @@ void IPARPi::applyDPC(const struct DpcStatus *dpcStatus, ControlList &ctrls)\n> \n> void IPARPi::applyLS(const struct AlscStatus *lsStatus, ControlList &ctrls)\n> {\n> -\tif (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING) == isp_ctrls_.end()) {\n> +\tif (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING) == ispCtrls_.end()) {\n> \t\tLOG(IPARPI, Error) << \"Can't find LS control\";\n> \t\treturn;\n> \t}\n> @@ -1032,18 +1032,18 @@ void IPARPi::applyLS(const struct AlscStatus *lsStatus, ControlList &ctrls)\n> \t * Program lens shading tables into pipeline.\n> \t * Choose smallest cell size that won't exceed 63x48 cells.\n> \t */\n> -\tconst int cell_sizes[] = { 16, 32, 64, 128, 256 };\n> -\tunsigned int num_cells = ARRAY_SIZE(cell_sizes);\n> -\tunsigned int i, w, h, cell_size;\n> -\tfor (i = 0; i < num_cells; i++) {\n> -\t\tcell_size = cell_sizes[i];\n> -\t\tw = (mode_.width + cell_size - 1) / cell_size;\n> -\t\th = (mode_.height + cell_size - 1) / cell_size;\n> +\tconst int cellSizes[] = { 16, 32, 64, 128, 256 };\n> +\tunsigned int numCells = ARRAY_SIZE(cellSizes);\n> +\tunsigned int i, w, h, cellSize;\n> +\tfor (i = 0; i < numCells; i++) {\n> +\t\tcellSize = cellSizes[i];\n> +\t\tw = (mode_.width + cellSize - 1) / cellSize;\n> +\t\th = (mode_.height + cellSize - 1) / cellSize;\n> \t\tif (w < 64 && h <= 48)\n> \t\t\tbreak;\n> \t}\n> \n> -\tif (i == num_cells) {\n> +\tif (i == numCells) {\n> \t\tLOG(IPARPI, Error) << \"Cannot find cell size\";\n> \t\treturn;\n> \t}\n> @@ -1052,7 +1052,7 @@ void IPARPi::applyLS(const struct AlscStatus *lsStatus, ControlList &ctrls)\n> \tw++, h++;\n> \tbcm2835_isp_lens_shading ls = {\n> \t\t.enabled = 1,\n> -\t\t.grid_cell_size = cell_size,\n> +\t\t.grid_cell_size = cellSize,\n> \t\t.grid_width = w,\n> \t\t.grid_stride = w,\n> \t\t.grid_height = h,\n> @@ -1062,7 +1062,7 @@ void IPARPi::applyLS(const struct AlscStatus *lsStatus, ControlList &ctrls)\n> \t\t.gain_format = GAIN_FORMAT_U4P10\n> \t};\n> \n> -\tif (!lsTable_ || w * h * 4 * sizeof(uint16_t) > MAX_LS_GRID_SIZE) {\n> +\tif (!lsTable_ || w * h * 4 * sizeof(uint16_t) > RPi::MaxLsGridSize) {\n> \t\tLOG(IPARPI, Error) << \"Do not have a correctly allocate lens shading table!\";\n> \t\treturn;\n> \t}\n> @@ -1083,41 +1083,41 @@ void IPARPi::applyLS(const struct AlscStatus *lsStatus, ControlList &ctrls)\n> }\n> \n> /*\n> - * Resamples a 16x12 table with central sampling to dest_w x dest_h with corner\n> + * Resamples a 16x12 table with central sampling to destW x destH with corner\n> * sampling.\n> */\n> void IPARPi::resampleTable(uint16_t dest[], double const src[12][16],\n> -\t\t\t int dest_w, int dest_h)\n> +\t\t\t int destW, int destH)\n> {\n> \t/*\n> \t * Precalculate and cache the x sampling locations and phases to\n> \t * save recomputing them on every row.\n> \t */\n> -\tassert(dest_w > 1 && dest_h > 1 && dest_w <= 64);\n> -\tint x_lo[64], x_hi[64];\n> +\tassert(destW > 1 && destH > 1 && destW <= 64);\n> +\tint xLo[64], xHi[64];\n> \tdouble xf[64];\n> -\tdouble x = -0.5, x_inc = 16.0 / (dest_w - 1);\n> -\tfor (int i = 0; i < dest_w; i++, x += x_inc) {\n> -\t\tx_lo[i] = floor(x);\n> -\t\txf[i] = x - x_lo[i];\n> -\t\tx_hi[i] = x_lo[i] < 15 ? x_lo[i] + 1 : 15;\n> -\t\tx_lo[i] = x_lo[i] > 0 ? x_lo[i] : 0;\n> +\tdouble x = -0.5, xInc = 16.0 / (destW - 1);\n> +\tfor (int i = 0; i < destW; i++, x += xInc) {\n> +\t\txLo[i] = floor(x);\n> +\t\txf[i] = x - xLo[i];\n> +\t\txHi[i] = xLo[i] < 15 ? xLo[i] + 1 : 15;\n> +\t\txLo[i] = xLo[i] > 0 ? xLo[i] : 0;\n> \t}\n> \n> \t/* Now march over the output table generating the new values. */\n> -\tdouble y = -0.5, y_inc = 12.0 / (dest_h - 1);\n> -\tfor (int j = 0; j < dest_h; j++, y += y_inc) {\n> -\t\tint y_lo = floor(y);\n> -\t\tdouble yf = y - y_lo;\n> -\t\tint y_hi = y_lo < 11 ? y_lo + 1 : 11;\n> -\t\ty_lo = y_lo > 0 ? y_lo : 0;\n> -\t\tdouble const *row_above = src[y_lo];\n> -\t\tdouble const *row_below = src[y_hi];\n> -\t\tfor (int i = 0; i < dest_w; i++) {\n> -\t\t\tdouble above = row_above[x_lo[i]] * (1 - xf[i])\n> -\t\t\t\t + row_above[x_hi[i]] * xf[i];\n> -\t\t\tdouble below = row_below[x_lo[i]] * (1 - xf[i])\n> -\t\t\t\t + row_below[x_hi[i]] * xf[i];\n> +\tdouble y = -0.5, yInc = 12.0 / (destH - 1);\n> +\tfor (int j = 0; j < destH; j++, y += yInc) {\n> +\t\tint yLo = floor(y);\n> +\t\tdouble yf = y - yLo;\n> +\t\tint yHi = yLo < 11 ? yLo + 1 : 11;\n> +\t\tyLo = yLo > 0 ? yLo : 0;\n> +\t\tdouble const *rowAbove = src[yLo];\n> +\t\tdouble const *rowBelow = src[yHi];\n> +\t\tfor (int i = 0; i < destW; i++) {\n> +\t\t\tdouble above = rowAbove[xLo[i]] * (1 - xf[i])\n> +\t\t\t\t + rowAbove[xHi[i]] * xf[i];\n> +\t\t\tdouble below = rowBelow[xLo[i]] * (1 - xf[i])\n> +\t\t\t\t + rowBelow[xHi[i]] * xf[i];\n> \t\t\tint result = floor(1024 * (above * (1 - yf) + below * yf) + .5);\n> \t\t\t*(dest++) = result > 16383 ? 16383 : result; /* want u4.10 */\n> \t\t}\n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index 35dbe0fb..8d40b0ed 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -1012,7 +1012,7 @@ int RPiCameraData::configureIPA()\n> \n> \t/* Allocate the lens shading table via dmaHeap and pass to the IPA. */\n> \tif (!lsTable_.isValid()) {\n> -\t\tlsTable_ = dmaHeap_.alloc(\"ls_grid\", MAX_LS_GRID_SIZE);\n> +\t\tlsTable_ = dmaHeap_.alloc(\"ls_grid\", RPi::MaxLsGridSize);\n\nIf we had an IPA namespace, it would be nice to see this as:\n\n lsTable_ = dmaHeap_.alloc(\"ls_grid\", ipa::RPi::MaxLsGridSize);\n\nAs that would make it really clear 'where' this MaxLsGridSize value was\ncoming from...\n\nAnyway, all my comments above are optional discussion points.\n\nFor the patch:\n\nReviewed-by: Kieran Bingham \n\n> \t\tif (!lsTable_.isValid())\n> \t\t\treturn -ENOMEM;\n> \n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 27349C3B5B\n\tfor ;\n\tWed, 23 Sep 2020 11:18:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AEFA462FD8;\n\tWed, 23 Sep 2020 13:18:52 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id F27EF62FD2\n\tfor ;\n\tWed, 23 Sep 2020 13:18:50 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 41524555;\n\tWed, 23 Sep 2020 13:18:50 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"SDhmELaD\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1600859930;\n\tbh=InUrXgUqLBQjQUJkTc3Z+LTNxLK/OKLHgFUcfxpuKIA=;\n\th=Reply-To:Subject:To:References:From:Date:In-Reply-To:From;\n\tb=SDhmELaDpIPvedLL98SgeshbS6/zcmlLpho6WVUWq3WNNse+TFWx3SvRhuaqOcg4r\n\tT9PR1GVq/lWnNIEI717H6YCQtmvmKX1XD8RnF1v+N7jwCXGdD0Hz0pnDCNS3OFLg26\n\t1udxtp0ITaCBSCuAthgtn/PktRPO9BDhN6mCcC9k=","To":"Naushir Patuck ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20200922095018.68434-1-naush@raspberrypi.com>\n\t<20200922095018.68434-5-naush@raspberrypi.com>","From":"Kieran Bingham ","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"","Date":"Wed, 23 Sep 2020 12:18:48 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.10.0","MIME-Version":"1.0","In-Reply-To":"<20200922095018.68434-5-naush@raspberrypi.com>","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH 4/4] ipa: raspberrypi: Tidy up\n\tvariable names to be consistent","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Reply-To":"kieran.bingham@ideasonboard.com","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":12666,"web_url":"https://patchwork.libcamera.org/comment/12666/","msgid":"<20200923113913.a6rmjylpqwbumo2s@uno.localdomain>","date":"2020-09-23T11:39:13","subject":"Re: [libcamera-devel] [PATCH 4/4] ipa: raspberrypi: Tidy up\n\tvariable names to be consistent","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"/me dives into bikeshedding\n\nOn Wed, Sep 23, 2020 at 12:18:48PM +0100, Kieran Bingham wrote:\n> Hi Naush,\n>\n> On 22/09/2020 10:50, Naushir Patuck wrote:\n> > Change variable names to camel case to be consistent with the rest of\n> > the source files. Remove #define consts and replace with constexpr.\n> >\n>\n> Sounds good to me...\n>\n>\n> > There are no functional changes in this commit.\n> >\n> > Signed-off-by: Naushir Patuck \n> > ---\n> > include/libcamera/ipa/raspberrypi.h | 2 +-\n> > src/ipa/raspberrypi/raspberrypi.cpp | 178 +++++++++---------\n> > .../pipeline/raspberrypi/raspberrypi.cpp | 2 +-\n> > 3 files changed, 91 insertions(+), 91 deletions(-)\n> >\n> > diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h\n> > index c9d4aa81..b3041591 100644\n> > --- a/include/libcamera/ipa/raspberrypi.h\n> > +++ b/include/libcamera/ipa/raspberrypi.h\n> > @@ -41,7 +41,7 @@ enum BufferMask {\n> > };\n> >\n> > /* Size of the LS grid allocation. */\n> > -#define MAX_LS_GRID_SIZE (32 << 10)\n> > +static constexpr unsigned int MaxLsGridSize = 32 << 10;\n>\n> I guess the LS could stay capitalised (MaxLSGridSize), not sure that it\n> matters, so up to you.\n>\n> > /* List of controls handled by the Raspberry Pi IPA */\n> > static const ControlInfoMap Controls = {\n> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> > index 0c0dc743..c240eae8 100644\n> > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > @@ -55,8 +55,8 @@\n> > namespace libcamera {\n> >\n> > /* Configure the sensor with these values initially. */\n> > -#define DEFAULT_ANALOGUE_GAIN 1.0\n> > -#define DEFAULT_EXPOSURE_TIME 20000\n> > +constexpr unsigned int DefaultAnalogueGain = 1.0;\n> > +constexpr unsigned int DefaultExposureTime = 20000;\n>\n> Oh nice, This really highlights to me the benefit of constexpr.\n> Now the values have types!\n>\n> >\n> > LOG_DEFINE_CATEGORY(IPARPI)\n> >\n> > @@ -65,7 +65,7 @@ class IPARPi : public IPAInterface\n> > public:\n> > \tIPARPi()\n> > \t\t: lastMode_({}), controller_(), controllerInit_(false),\n> > -\t\t frame_count_(0), check_count_(0), mistrust_count_(0),\n> > +\t\t frameCount_(0), checkCount_(0), mistrustCount_(0),\n> > \t\t lsTable_(nullptr)\n> > \t{\n> > \t}\n> > @@ -73,7 +73,7 @@ public:\n> > \t~IPARPi()\n> > \t{\n> > \t\tif (lsTable_)\n> > -\t\t\tmunmap(lsTable_, MAX_LS_GRID_SIZE);\n> > +\t\t\tmunmap(lsTable_, RPi::MaxLsGridSize);\n> > \t}\n> >\n> > \tint init(const IPASettings &settings) override;\n> > @@ -108,13 +108,13 @@ private:\n> > \tvoid applySharpen(const struct SharpenStatus *sharpenStatus, ControlList &ctrls);\n> > \tvoid applyDPC(const struct DpcStatus *dpcStatus, ControlList &ctrls);\n> > \tvoid applyLS(const struct AlscStatus *lsStatus, ControlList &ctrls);\n> > -\tvoid resampleTable(uint16_t dest[], double const src[12][16], int dest_w, int dest_h);\n> > +\tvoid resampleTable(uint16_t dest[], double const src[12][16], int destW, int destH);\n> >\n> > \tstd::map buffers_;\n> > \tstd::map buffersMemory_;\n> >\n> > -\tControlInfoMap unicam_ctrls_;\n> > -\tControlInfoMap isp_ctrls_;\n> > +\tControlInfoMap unicamCtrls_;\n> > +\tControlInfoMap ispCtrls_;\n> > \tControlList libcameraMetadata_;\n> >\n> > \t/* IPA configuration. */\n> > @@ -134,11 +134,11 @@ private:\n> > \t * We count frames to decide if the frame must be hidden (e.g. from\n> > \t * display) or mistrusted (i.e. not given to the control algos).\n> > \t */\n> > -\tuint64_t frame_count_;\n> > +\tuint64_t frameCount_;\n>\n> If we're doing clean up - I always prefer to see a newline before a\n> comment line.\n>\n> To me it makes the commented block clearer - but I don't know if there's\n> a 'rule' on it.\n>\n\nI'm sorry but\n\n /*\n * A comment describing a variable is better places as close\n * as possible near to that variable, isn't it ?\n */\n int whatAFancyVariable;\n\n /* Or do you think this variable is not fancy and an empty line is better? */\n\n int notAFancyVariable;\n\nOk, enough useless discussions from me :)\n\n>\n> > \t/* For checking the sequencing of Prepare/Process calls. */\n> > -\tuint64_t check_count_;\n> > +\tuint64_t checkCount_;\n>\n> i.e. here too.\n>\n> > \t/* How many frames we should avoid running control algos on. */\n> > -\tunsigned int mistrust_count_;\n> > +\tunsigned int mistrustCount_;\n>\n> and so on.\n>\n> > \t/* LS table allocation passed in from the pipeline handler. */\n> > \tFileDescriptor lsTableHandle_;\n> > \tvoid *lsTable_;\n> > @@ -199,8 +199,8 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> >\n> > \tresult->operation = 0;\n> >\n> > -\tunicam_ctrls_ = entityControls.at(0);\n> > -\tisp_ctrls_ = entityControls.at(1);\n> > +\tunicamCtrls_ = entityControls.at(0);\n> > +\tispCtrls_ = entityControls.at(1);\n> > \t/* Setup a metadata ControlList to output metadata. */\n> > \tlibcameraMetadata_ = ControlList(controls::controls);\n> >\n> > @@ -238,18 +238,18 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> > \t *\"mistrusted\", which depends on whether this is a startup from cold,\n> > \t * or merely a mode switch in a running system.\n> > \t */\n> > -\tframe_count_ = 0;\n> > -\tcheck_count_ = 0;\n> > -\tunsigned int drop_frame = 0;\n> > +\tframeCount_ = 0;\n> > +\tcheckCount_ = 0;\n> > +\tunsigned int dropFrame = 0;\n> > \tif (controllerInit_) {\n> > -\t\tdrop_frame = helper_->HideFramesModeSwitch();\n> > -\t\tmistrust_count_ = helper_->MistrustFramesModeSwitch();\n> > +\t\tdropFrame = helper_->HideFramesModeSwitch();\n> > +\t\tmistrustCount_ = helper_->MistrustFramesModeSwitch();\n> > \t} else {\n> > -\t\tdrop_frame = helper_->HideFramesStartup();\n> > -\t\tmistrust_count_ = helper_->MistrustFramesStartup();\n> > +\t\tdropFrame = helper_->HideFramesStartup();\n> > +\t\tmistrustCount_ = helper_->MistrustFramesStartup();\n> > \t}\n> >\n> > -\tresult->data.push_back(drop_frame);\n> > +\tresult->data.push_back(dropFrame);\n> > \tresult->operation |= RPi::IPA_CONFIG_DROP_FRAMES;\n> >\n> > \tstruct AgcStatus agcStatus;\n> > @@ -264,8 +264,8 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> > \t\tcontrollerInit_ = true;\n> >\n> > \t\t/* Supply initial values for gain and exposure. */\n> > -\t\tagcStatus.shutter_time = DEFAULT_EXPOSURE_TIME;\n> > -\t\tagcStatus.analogue_gain = DEFAULT_ANALOGUE_GAIN;\n> > +\t\tagcStatus.shutter_time = DefaultExposureTime;\n> > +\t\tagcStatus.analogue_gain = DefaultAnalogueGain;\n> > \t}\n> >\n> > \tRPiController::Metadata metadata;\n> > @@ -274,7 +274,7 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> > \t/* SwitchMode may supply updated exposure/gain values to use. */\n> > \tmetadata.Get(\"agc.status\", agcStatus);\n> > \tif (agcStatus.shutter_time != 0.0 && agcStatus.analogue_gain != 0.0) {\n> > -\t\tControlList ctrls(unicam_ctrls_);\n> > +\t\tControlList ctrls(unicamCtrls_);\n> > \t\tapplyAGC(&agcStatus, ctrls);\n> > \t\tresult->controls.push_back(ctrls);\n> >\n> > @@ -287,14 +287,14 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> > \tif (ipaConfig.operation & RPi::IPA_CONFIG_LS_TABLE) {\n> > \t\t/* Remove any previous table, if there was one. */\n> > \t\tif (lsTable_) {\n> > -\t\t\tmunmap(lsTable_, MAX_LS_GRID_SIZE);\n> > +\t\t\tmunmap(lsTable_, RPi::MaxLsGridSize);\n> > \t\t\tlsTable_ = nullptr;\n> > \t\t}\n> >\n> > \t\t/* Map the LS table buffer into user space. */\n> > \t\tlsTableHandle_ = FileDescriptor(ipaConfig.data[0]);\n> > \t\tif (lsTableHandle_.isValid()) {\n> > -\t\t\tlsTable_ = mmap(nullptr, MAX_LS_GRID_SIZE, PROT_READ | PROT_WRITE,\n> > +\t\t\tlsTable_ = mmap(nullptr, RPi::MaxLsGridSize, PROT_READ | PROT_WRITE,\n> > \t\t\t\t\tMAP_SHARED, lsTableHandle_.fd(), 0);\n> >\n> > \t\t\tif (lsTable_ == MAP_FAILED) {\n> > @@ -343,9 +343,9 @@ void IPARPi::processEvent(const IPAOperationData &event)\n> > \tcase RPi::IPA_EVENT_SIGNAL_STAT_READY: {\n> > \t\tunsigned int bufferId = event.data[0];\n> >\n> > -\t\tif (++check_count_ != frame_count_) /* assert here? */\n> > +\t\tif (++checkCount_ != frameCount_) /* assert here? */\n> > \t\t\tLOG(IPARPI, Error) << \"WARNING: Prepare/Process mismatch!!!\";\n> > -\t\tif (frame_count_ > mistrust_count_)\n> > +\t\tif (frameCount_ > mistrustCount_)\n> > \t\t\tprocessStats(bufferId);\n> >\n> > \t\treportMetadata();\n> > @@ -368,7 +368,7 @@ void IPARPi::processEvent(const IPAOperationData &event)\n> > \t\t * they are \"unreliable\".\n> > \t\t */\n> > \t\tprepareISP(embeddedbufferId);\n> > -\t\tframe_count_++;\n> > +\t\tframeCount_++;\n> >\n> > \t\t/* Ready to push the input buffer into the ISP. */\n> > \t\tIPAOperationData op;\n> > @@ -713,7 +713,7 @@ void IPARPi::prepareISP(unsigned int bufferId)\n> > \treturnEmbeddedBuffer(bufferId);\n> >\n> > \tif (success) {\n> > -\t\tControlList ctrls(isp_ctrls_);\n> > +\t\tControlList ctrls(ispCtrls_);\n> >\n> > \t\trpiMetadata_.Clear();\n> > \t\trpiMetadata_.Set(\"device.status\", deviceStatus);\n> > @@ -785,19 +785,19 @@ bool IPARPi::parseEmbeddedData(unsigned int bufferId, struct DeviceStatus &devic\n> > \tif (status != RPiController::MdParser::Status::OK) {\n> > \t\tLOG(IPARPI, Error) << \"Embedded Buffer parsing failed, error \" << status;\n> > \t} else {\n> > -\t\tuint32_t exposure_lines, gain_code;\n> > -\t\tif (helper_->Parser().GetExposureLines(exposure_lines) != RPiController::MdParser::Status::OK) {\n> > +\t\tuint32_t exposureLines, gainCode;\n> > +\t\tif (helper_->Parser().GetExposureLines(exposureLines) != RPiController::MdParser::Status::OK) {\n> > \t\t\tLOG(IPARPI, Error) << \"Exposure time failed\";\n> > \t\t\treturn false;\n> > \t\t}\n> >\n> > -\t\tdeviceStatus.shutter_speed = helper_->Exposure(exposure_lines);\n> > -\t\tif (helper_->Parser().GetGainCode(gain_code) != RPiController::MdParser::Status::OK) {\n> > +\t\tdeviceStatus.shutter_speed = helper_->Exposure(exposureLines);\n> > +\t\tif (helper_->Parser().GetGainCode(gainCode) != RPiController::MdParser::Status::OK) {\n> > \t\t\tLOG(IPARPI, Error) << \"Gain failed\";\n> > \t\t\treturn false;\n> > \t\t}\n> >\n> > -\t\tdeviceStatus.analogue_gain = helper_->Gain(gain_code);\n> > +\t\tdeviceStatus.analogue_gain = helper_->Gain(gainCode);\n> > \t\tLOG(IPARPI, Debug) << \"Metadata - Exposure : \"\n> > \t\t\t\t << deviceStatus.shutter_speed << \" Gain : \"\n> > \t\t\t\t << deviceStatus.analogue_gain;\n> > @@ -820,7 +820,7 @@ void IPARPi::processStats(unsigned int bufferId)\n> >\n> > \tstruct AgcStatus agcStatus;\n> > \tif (rpiMetadata_.Get(\"agc.status\", agcStatus) == 0) {\n> > -\t\tControlList ctrls(unicam_ctrls_);\n> > +\t\tControlList ctrls(unicamCtrls_);\n> > \t\tapplyAGC(&agcStatus, ctrls);\n> >\n> > \t\tIPAOperationData op;\n> > @@ -832,14 +832,14 @@ void IPARPi::processStats(unsigned int bufferId)\n> >\n> > void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls)\n> > {\n> > -\tconst auto gainR = isp_ctrls_.find(V4L2_CID_RED_BALANCE);\n> > -\tif (gainR == isp_ctrls_.end()) {\n> > +\tconst auto gainR = ispCtrls_.find(V4L2_CID_RED_BALANCE);\n> > +\tif (gainR == ispCtrls_.end()) {\n> > \t\tLOG(IPARPI, Error) << \"Can't find red gain control\";\n> > \t\treturn;\n> > \t}\n> >\n> > -\tconst auto gainB = isp_ctrls_.find(V4L2_CID_BLUE_BALANCE);\n> > -\tif (gainB == isp_ctrls_.end()) {\n> > +\tconst auto gainB = ispCtrls_.find(V4L2_CID_BLUE_BALANCE);\n> > +\tif (gainB == ispCtrls_.end()) {\n> > \t\tLOG(IPARPI, Error) << \"Can't find blue gain control\";\n> > \t\treturn;\n> > \t}\n> > @@ -855,31 +855,31 @@ void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls)\n> >\n> > void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls)\n> > {\n> > -\tint32_t gain_code = helper_->GainCode(agcStatus->analogue_gain);\n> > -\tint32_t exposure_lines = helper_->ExposureLines(agcStatus->shutter_time);\n> > +\tint32_t gainCode = helper_->GainCode(agcStatus->analogue_gain);\n> > +\tint32_t exposureLines = helper_->ExposureLines(agcStatus->shutter_time);\n> >\n> > -\tif (unicam_ctrls_.find(V4L2_CID_ANALOGUE_GAIN) == unicam_ctrls_.end()) {\n> > +\tif (unicamCtrls_.find(V4L2_CID_ANALOGUE_GAIN) == unicamCtrls_.end()) {\n> > \t\tLOG(IPARPI, Error) << \"Can't find analogue gain control\";\n> > \t\treturn;\n> > \t}\n> >\n> > -\tif (unicam_ctrls_.find(V4L2_CID_EXPOSURE) == unicam_ctrls_.end()) {\n> > +\tif (unicamCtrls_.find(V4L2_CID_EXPOSURE) == unicamCtrls_.end()) {\n> > \t\tLOG(IPARPI, Error) << \"Can't find exposure control\";\n> > \t\treturn;\n> > \t}\n> >\n> > \tLOG(IPARPI, Debug) << \"Applying AGC Exposure: \" << agcStatus->shutter_time\n> > -\t\t\t << \" (Shutter lines: \" << exposure_lines << \") Gain: \"\n> > +\t\t\t << \" (Shutter lines: \" << exposureLines << \") Gain: \"\n> > \t\t\t << agcStatus->analogue_gain << \" (Gain Code: \"\n> > -\t\t\t << gain_code << \")\";\n> > +\t\t\t << gainCode << \")\";\n> >\n> > -\tctrls.set(V4L2_CID_ANALOGUE_GAIN, gain_code);\n> > -\tctrls.set(V4L2_CID_EXPOSURE, exposure_lines);\n> > +\tctrls.set(V4L2_CID_ANALOGUE_GAIN, gainCode);\n> > +\tctrls.set(V4L2_CID_EXPOSURE, exposureLines);\n> > }\n> >\n> > void IPARPi::applyDG(const struct AgcStatus *dgStatus, ControlList &ctrls)\n> > {\n> > -\tif (isp_ctrls_.find(V4L2_CID_DIGITAL_GAIN) == isp_ctrls_.end()) {\n> > +\tif (ispCtrls_.find(V4L2_CID_DIGITAL_GAIN) == ispCtrls_.end()) {\n> > \t\tLOG(IPARPI, Error) << \"Can't find digital gain control\";\n> > \t\treturn;\n> > \t}\n> > @@ -890,7 +890,7 @@ void IPARPi::applyDG(const struct AgcStatus *dgStatus, ControlList &ctrls)\n> >\n> > void IPARPi::applyCCM(const struct CcmStatus *ccmStatus, ControlList &ctrls)\n> > {\n> > -\tif (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_CC_MATRIX) == isp_ctrls_.end()) {\n> > +\tif (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_CC_MATRIX) == ispCtrls_.end()) {\n> > \t\tLOG(IPARPI, Error) << \"Can't find CCM control\";\n> > \t\treturn;\n> > \t}\n> > @@ -911,7 +911,7 @@ void IPARPi::applyCCM(const struct CcmStatus *ccmStatus, ControlList &ctrls)\n> >\n> > void IPARPi::applyGamma(const struct ContrastStatus *contrastStatus, ControlList &ctrls)\n> > {\n> > -\tif (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_GAMMA) == isp_ctrls_.end()) {\n> > +\tif (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_GAMMA) == ispCtrls_.end()) {\n> > \t\tLOG(IPARPI, Error) << \"Can't find Gamma control\";\n> > \t\treturn;\n> > \t}\n> > @@ -930,7 +930,7 @@ void IPARPi::applyGamma(const struct ContrastStatus *contrastStatus, ControlList\n> >\n> > void IPARPi::applyBlackLevel(const struct BlackLevelStatus *blackLevelStatus, ControlList &ctrls)\n> > {\n> > -\tif (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_BLACK_LEVEL) == isp_ctrls_.end()) {\n> > +\tif (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_BLACK_LEVEL) == ispCtrls_.end()) {\n> > \t\tLOG(IPARPI, Error) << \"Can't find black level control\";\n> > \t\treturn;\n> > \t}\n> > @@ -948,7 +948,7 @@ void IPARPi::applyBlackLevel(const struct BlackLevelStatus *blackLevelStatus, Co\n> >\n> > void IPARPi::applyGEQ(const struct GeqStatus *geqStatus, ControlList &ctrls)\n> > {\n> > -\tif (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_GEQ) == isp_ctrls_.end()) {\n> > +\tif (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_GEQ) == ispCtrls_.end()) {\n> > \t\tLOG(IPARPI, Error) << \"Can't find geq control\";\n> > \t\treturn;\n> > \t}\n> > @@ -966,7 +966,7 @@ void IPARPi::applyGEQ(const struct GeqStatus *geqStatus, ControlList &ctrls)\n> >\n> > void IPARPi::applyDenoise(const struct SdnStatus *denoiseStatus, ControlList &ctrls)\n> > {\n> > -\tif (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_DENOISE) == isp_ctrls_.end()) {\n> > +\tif (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_DENOISE) == ispCtrls_.end()) {\n> > \t\tLOG(IPARPI, Error) << \"Can't find denoise control\";\n> > \t\treturn;\n> > \t}\n> > @@ -986,7 +986,7 @@ void IPARPi::applyDenoise(const struct SdnStatus *denoiseStatus, ControlList &ct\n> >\n> > void IPARPi::applySharpen(const struct SharpenStatus *sharpenStatus, ControlList &ctrls)\n> > {\n> > -\tif (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_SHARPEN) == isp_ctrls_.end()) {\n> > +\tif (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_SHARPEN) == ispCtrls_.end()) {\n> > \t\tLOG(IPARPI, Error) << \"Can't find sharpen control\";\n> > \t\treturn;\n> > \t}\n> > @@ -1007,7 +1007,7 @@ void IPARPi::applySharpen(const struct SharpenStatus *sharpenStatus, ControlList\n> >\n> > void IPARPi::applyDPC(const struct DpcStatus *dpcStatus, ControlList &ctrls)\n> > {\n> > -\tif (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_DPC) == isp_ctrls_.end()) {\n> > +\tif (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_DPC) == ispCtrls_.end()) {\n> > \t\tLOG(IPARPI, Error) << \"Can't find DPC control\";\n> > \t\treturn;\n> > \t}\n> > @@ -1023,7 +1023,7 @@ void IPARPi::applyDPC(const struct DpcStatus *dpcStatus, ControlList &ctrls)\n> >\n> > void IPARPi::applyLS(const struct AlscStatus *lsStatus, ControlList &ctrls)\n> > {\n> > -\tif (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING) == isp_ctrls_.end()) {\n> > +\tif (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING) == ispCtrls_.end()) {\n> > \t\tLOG(IPARPI, Error) << \"Can't find LS control\";\n> > \t\treturn;\n> > \t}\n> > @@ -1032,18 +1032,18 @@ void IPARPi::applyLS(const struct AlscStatus *lsStatus, ControlList &ctrls)\n> > \t * Program lens shading tables into pipeline.\n> > \t * Choose smallest cell size that won't exceed 63x48 cells.\n> > \t */\n> > -\tconst int cell_sizes[] = { 16, 32, 64, 128, 256 };\n> > -\tunsigned int num_cells = ARRAY_SIZE(cell_sizes);\n> > -\tunsigned int i, w, h, cell_size;\n> > -\tfor (i = 0; i < num_cells; i++) {\n> > -\t\tcell_size = cell_sizes[i];\n> > -\t\tw = (mode_.width + cell_size - 1) / cell_size;\n> > -\t\th = (mode_.height + cell_size - 1) / cell_size;\n> > +\tconst int cellSizes[] = { 16, 32, 64, 128, 256 };\n> > +\tunsigned int numCells = ARRAY_SIZE(cellSizes);\n> > +\tunsigned int i, w, h, cellSize;\n> > +\tfor (i = 0; i < numCells; i++) {\n> > +\t\tcellSize = cellSizes[i];\n> > +\t\tw = (mode_.width + cellSize - 1) / cellSize;\n> > +\t\th = (mode_.height + cellSize - 1) / cellSize;\n> > \t\tif (w < 64 && h <= 48)\n> > \t\t\tbreak;\n> > \t}\n> >\n> > -\tif (i == num_cells) {\n> > +\tif (i == numCells) {\n> > \t\tLOG(IPARPI, Error) << \"Cannot find cell size\";\n> > \t\treturn;\n> > \t}\n> > @@ -1052,7 +1052,7 @@ void IPARPi::applyLS(const struct AlscStatus *lsStatus, ControlList &ctrls)\n> > \tw++, h++;\n> > \tbcm2835_isp_lens_shading ls = {\n> > \t\t.enabled = 1,\n> > -\t\t.grid_cell_size = cell_size,\n> > +\t\t.grid_cell_size = cellSize,\n> > \t\t.grid_width = w,\n> > \t\t.grid_stride = w,\n> > \t\t.grid_height = h,\n> > @@ -1062,7 +1062,7 @@ void IPARPi::applyLS(const struct AlscStatus *lsStatus, ControlList &ctrls)\n> > \t\t.gain_format = GAIN_FORMAT_U4P10\n> > \t};\n> >\n> > -\tif (!lsTable_ || w * h * 4 * sizeof(uint16_t) > MAX_LS_GRID_SIZE) {\n> > +\tif (!lsTable_ || w * h * 4 * sizeof(uint16_t) > RPi::MaxLsGridSize) {\n> > \t\tLOG(IPARPI, Error) << \"Do not have a correctly allocate lens shading table!\";\n> > \t\treturn;\n> > \t}\n> > @@ -1083,41 +1083,41 @@ void IPARPi::applyLS(const struct AlscStatus *lsStatus, ControlList &ctrls)\n> > }\n> >\n> > /*\n> > - * Resamples a 16x12 table with central sampling to dest_w x dest_h with corner\n> > + * Resamples a 16x12 table with central sampling to destW x destH with corner\n> > * sampling.\n> > */\n> > void IPARPi::resampleTable(uint16_t dest[], double const src[12][16],\n> > -\t\t\t int dest_w, int dest_h)\n> > +\t\t\t int destW, int destH)\n> > {\n> > \t/*\n> > \t * Precalculate and cache the x sampling locations and phases to\n> > \t * save recomputing them on every row.\n> > \t */\n> > -\tassert(dest_w > 1 && dest_h > 1 && dest_w <= 64);\n> > -\tint x_lo[64], x_hi[64];\n> > +\tassert(destW > 1 && destH > 1 && destW <= 64);\n> > +\tint xLo[64], xHi[64];\n> > \tdouble xf[64];\n> > -\tdouble x = -0.5, x_inc = 16.0 / (dest_w - 1);\n> > -\tfor (int i = 0; i < dest_w; i++, x += x_inc) {\n> > -\t\tx_lo[i] = floor(x);\n> > -\t\txf[i] = x - x_lo[i];\n> > -\t\tx_hi[i] = x_lo[i] < 15 ? x_lo[i] + 1 : 15;\n> > -\t\tx_lo[i] = x_lo[i] > 0 ? x_lo[i] : 0;\n> > +\tdouble x = -0.5, xInc = 16.0 / (destW - 1);\n> > +\tfor (int i = 0; i < destW; i++, x += xInc) {\n> > +\t\txLo[i] = floor(x);\n> > +\t\txf[i] = x - xLo[i];\n> > +\t\txHi[i] = xLo[i] < 15 ? xLo[i] + 1 : 15;\n> > +\t\txLo[i] = xLo[i] > 0 ? xLo[i] : 0;\n> > \t}\n> >\n> > \t/* Now march over the output table generating the new values. */\n> > -\tdouble y = -0.5, y_inc = 12.0 / (dest_h - 1);\n> > -\tfor (int j = 0; j < dest_h; j++, y += y_inc) {\n> > -\t\tint y_lo = floor(y);\n> > -\t\tdouble yf = y - y_lo;\n> > -\t\tint y_hi = y_lo < 11 ? y_lo + 1 : 11;\n> > -\t\ty_lo = y_lo > 0 ? y_lo : 0;\n> > -\t\tdouble const *row_above = src[y_lo];\n> > -\t\tdouble const *row_below = src[y_hi];\n> > -\t\tfor (int i = 0; i < dest_w; i++) {\n> > -\t\t\tdouble above = row_above[x_lo[i]] * (1 - xf[i])\n> > -\t\t\t\t + row_above[x_hi[i]] * xf[i];\n> > -\t\t\tdouble below = row_below[x_lo[i]] * (1 - xf[i])\n> > -\t\t\t\t + row_below[x_hi[i]] * xf[i];\n> > +\tdouble y = -0.5, yInc = 12.0 / (destH - 1);\n> > +\tfor (int j = 0; j < destH; j++, y += yInc) {\n> > +\t\tint yLo = floor(y);\n> > +\t\tdouble yf = y - yLo;\n> > +\t\tint yHi = yLo < 11 ? yLo + 1 : 11;\n> > +\t\tyLo = yLo > 0 ? yLo : 0;\n> > +\t\tdouble const *rowAbove = src[yLo];\n> > +\t\tdouble const *rowBelow = src[yHi];\n> > +\t\tfor (int i = 0; i < destW; i++) {\n> > +\t\t\tdouble above = rowAbove[xLo[i]] * (1 - xf[i])\n> > +\t\t\t\t + rowAbove[xHi[i]] * xf[i];\n> > +\t\t\tdouble below = rowBelow[xLo[i]] * (1 - xf[i])\n> > +\t\t\t\t + rowBelow[xHi[i]] * xf[i];\n> > \t\t\tint result = floor(1024 * (above * (1 - yf) + below * yf) + .5);\n> > \t\t\t*(dest++) = result > 16383 ? 16383 : result; /* want u4.10 */\n> > \t\t}\n> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > index 35dbe0fb..8d40b0ed 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > @@ -1012,7 +1012,7 @@ int RPiCameraData::configureIPA()\n> >\n> > \t/* Allocate the lens shading table via dmaHeap and pass to the IPA. */\n> > \tif (!lsTable_.isValid()) {\n> > -\t\tlsTable_ = dmaHeap_.alloc(\"ls_grid\", MAX_LS_GRID_SIZE);\n> > +\t\tlsTable_ = dmaHeap_.alloc(\"ls_grid\", RPi::MaxLsGridSize);\n>\n> If we had an IPA namespace, it would be nice to see this as:\n>\n> lsTable_ = dmaHeap_.alloc(\"ls_grid\", ipa::RPi::MaxLsGridSize);\n>\n> As that would make it really clear 'where' this MaxLsGridSize value was\n> coming from...\n>\n> Anyway, all my comments above are optional discussion points.\n>\n> For the patch:\n>\n> Reviewed-by: Kieran Bingham \n>\n> > \t\tif (!lsTable_.isValid())\n> > \t\t\treturn -ENOMEM;\n> >\n> >\n>\n> --\n> Regards\n> --\n> Kieran\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 3D03AC3B5C\n\tfor ;\n\tWed, 23 Sep 2020 11:35:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D437762FD8;\n\tWed, 23 Sep 2020 13:35:22 +0200 (CEST)","from relay3-d.mail.gandi.net (relay3-d.mail.gandi.net\n\t[217.70.183.195])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 92B2762FD2\n\tfor ;\n\tWed, 23 Sep 2020 13:35:21 +0200 (CEST)","from uno.localdomain (93-34-118-233.ip49.fastwebnet.it\n\t[93.34.118.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay3-d.mail.gandi.net (Postfix) with ESMTPSA id C141860005;\n\tWed, 23 Sep 2020 11:35:20 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Wed, 23 Sep 2020 13:39:13 +0200","From":"Jacopo Mondi ","To":"Kieran Bingham ","Message-ID":"<20200923113913.a6rmjylpqwbumo2s@uno.localdomain>","References":"<20200922095018.68434-1-naush@raspberrypi.com>\n\t<20200922095018.68434-5-naush@raspberrypi.com>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 4/4] ipa: raspberrypi: Tidy up\n\tvariable names to be consistent","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":12667,"web_url":"https://patchwork.libcamera.org/comment/12667/","msgid":"","date":"2020-09-23T11:45:33","subject":"Re: [libcamera-devel] [PATCH 4/4] ipa: raspberrypi: Tidy up\n\tvariable names to be consistent","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn 23/09/2020 12:39, Jacopo Mondi wrote:\n> /me dives into bikeshedding\n\n\n\n>>> @@ -134,11 +134,11 @@ private:\n>>> \t * We count frames to decide if the frame must be hidden (e.g. from\n>>> \t * display) or mistrusted (i.e. not given to the control algos).\n>>> \t */\n>>> -\tuint64_t frame_count_;\n>>> +\tuint64_t frameCount_;\n>>\n>> If we're doing clean up - I always prefer to see a newline before a\n>> comment line.\n\nNote here I state 'before a comment'\n\n>> To me it makes the commented block clearer - but I don't know if there's\n>> a 'rule' on it.\n>>\n> \n> I'm sorry but\n> \n> /*\n> * A comment describing a variable is better places as close\n> * as possible near to that variable, isn't it ?\n> */\n> int whatAFancyVariable;\n> \n> /* Or do you think this variable is not fancy and an empty line is better? */\n> \n\nHere you seem to be implying 'after' a comment...\n\n> int notAFancyVariable;\n> \n> Ok, enough useless discussions from me :)\n> \n\nI ... 'think' you mis-interpreted what I said, or I was not clear. I was\nprobably not clear ;-)\n\nI think that this:\n\n\t/* A variable group. */\n\tbool variable;\n\tint quantity;\n\n\t/*\n\t * We count frames to decide if the frame must be hidden (e.g.\n\t * from display) or mistrusted (i.e. not given to the control\n\t * algos).\n\t */\n\tuint64_t frameCount_;\n\n\t/* For checking the sequencing of Prepare/Process calls. */\n\tuint64_t checkCount_;\n\n\t/* How many frames we should avoid running control algos on. */\n\tunsigned int mistrustCount_;\n\nis better than this:\n\n\n\t/* A variable group */\n\tbool variable;\n\tint quanty;\n\t/*\n\t * We count frames to decide if the frame must be hidden (e.g.\n\t * from display) or mistrusted (i.e. not given to the control\n\t * algos).\n\t */\n\tuint64_t frameCount_;\n\t/* For checking the sequencing of Prepare/Process calls. */\n\tuint64_t checkCount_;\n\t/* How many frames we should avoid running control algos on. */\n\tunsigned int mistrustCount_;\n\nMaybe one day I'll finish my comment formatter in the checkstyle script ;-)\n\n\n>>\n>>> \t/* For checking the sequencing of Prepare/Process calls. */\n>>> -\tuint64_t check_count_;\n>>> +\tuint64_t checkCount_;\n>>\n>> i.e. here too.\n>>\n>>> \t/* How many frames we should avoid running control algos on. */\n>>> -\tunsigned int mistrust_count_;\n>>> +\tunsigned int mistrustCount_;\n>>\n>> and so on.\n>>\n>>> \t/* LS table allocation passed in from the pipeline handler. */\n>>> \tFileDescriptor lsTableHandle_;\n>>> \tvoid *lsTable_;\n\n","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 47E58C3B5B\n\tfor ;\n\tWed, 23 Sep 2020 11:45:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C599962FDE;\n\tWed, 23 Sep 2020 13:45:42 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D5E6562FD2\n\tfor ;\n\tWed, 23 Sep 2020 13:45:41 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 0A7F92FD;\n\tWed, 23 Sep 2020 13:45:36 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"uyeLoBo/\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1600861537;\n\tbh=vViIJBYk4GUDYN+2wF87NguU9AcQiAK6MIhENMK5JiI=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=uyeLoBo/XvbjOJHLTd206S5JjZ6b5d6CrlM/N2UKewqN2em7eAt5YMUOGNUZ9heJW\n\tKFwDEc4Slbi7M0yiS3c8yQtHMR3jio7dnF8ehqFzLIGgK+bR01f/irwP9DqmVO3+W6\n\t/V/qF9ylA81Gp5puDPXuzyeOtmu0ErWe+5JzevVI=","To":"Jacopo Mondi ","References":"<20200922095018.68434-1-naush@raspberrypi.com>\n\t<20200922095018.68434-5-naush@raspberrypi.com>\n\t\n\t<20200923113913.a6rmjylpqwbumo2s@uno.localdomain>","From":"Kieran Bingham ","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"","Date":"Wed, 23 Sep 2020 12:45:33 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.10.0","MIME-Version":"1.0","In-Reply-To":"<20200923113913.a6rmjylpqwbumo2s@uno.localdomain>","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH 4/4] ipa: raspberrypi: Tidy up\n\tvariable names to be consistent","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Reply-To":"kieran.bingham@ideasonboard.com","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":12681,"web_url":"https://patchwork.libcamera.org/comment/12681/","msgid":"","date":"2020-09-23T16:31:43","subject":"Re: [libcamera-devel] [PATCH 4/4] ipa: raspberrypi: Tidy up\n\tvariable names to be consistent","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Naush\n\nThanks for doing all this tidying! One little nit-pick...\n\nOn Wed, 23 Sep 2020 at 12:18, Kieran Bingham\n wrote:\n>\n> Hi Naush,\n>\n> On 22/09/2020 10:50, Naushir Patuck wrote:\n> > Change variable names to camel case to be consistent with the rest of\n> > the source files. Remove #define consts and replace with constexpr.\n> >\n>\n> Sounds good to me...\n>\n>\n> > There are no functional changes in this commit.\n> >\n> > Signed-off-by: Naushir Patuck \n> > ---\n> > include/libcamera/ipa/raspberrypi.h | 2 +-\n> > src/ipa/raspberrypi/raspberrypi.cpp | 178 +++++++++---------\n> > .../pipeline/raspberrypi/raspberrypi.cpp | 2 +-\n> > 3 files changed, 91 insertions(+), 91 deletions(-)\n> >\n> > diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h\n> > index c9d4aa81..b3041591 100644\n> > --- a/include/libcamera/ipa/raspberrypi.h\n> > +++ b/include/libcamera/ipa/raspberrypi.h\n> > @@ -41,7 +41,7 @@ enum BufferMask {\n> > };\n> >\n> > /* Size of the LS grid allocation. */\n> > -#define MAX_LS_GRID_SIZE (32 << 10)\n> > +static constexpr unsigned int MaxLsGridSize = 32 << 10;\n>\n> I guess the LS could stay capitalised (MaxLSGridSize), not sure that it\n> matters, so up to you.\n>\n> > /* List of controls handled by the Raspberry Pi IPA */\n> > static const ControlInfoMap Controls = {\n> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> > index 0c0dc743..c240eae8 100644\n> > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > @@ -55,8 +55,8 @@\n> > namespace libcamera {\n> >\n> > /* Configure the sensor with these values initially. */\n> > -#define DEFAULT_ANALOGUE_GAIN 1.0\n> > -#define DEFAULT_EXPOSURE_TIME 20000\n> > +constexpr unsigned int DefaultAnalogueGain = 1.0;\n> > +constexpr unsigned int DefaultExposureTime = 20000;\n>\n> Oh nice, This really highlights to me the benefit of constexpr.\n> Now the values have types!\n\nShould DefaultAnalogueGain be a double? Of course it gets cast to a\ndouble when it gets copied into agcStatus.analogue_gain so it probably\ncontrives to make no difference, but a double might be clearer,\nespecially if anyone ever changed it.\n\nThanks!\nDavid\n\n>\n> >\n> > LOG_DEFINE_CATEGORY(IPARPI)\n> >\n> > @@ -65,7 +65,7 @@ class IPARPi : public IPAInterface\n> > public:\n> > IPARPi()\n> > : lastMode_({}), controller_(), controllerInit_(false),\n> > - frame_count_(0), check_count_(0), mistrust_count_(0),\n> > + frameCount_(0), checkCount_(0), mistrustCount_(0),\n> > lsTable_(nullptr)\n> > {\n> > }\n> > @@ -73,7 +73,7 @@ public:\n> > ~IPARPi()\n> > {\n> > if (lsTable_)\n> > - munmap(lsTable_, MAX_LS_GRID_SIZE);\n> > + munmap(lsTable_, RPi::MaxLsGridSize);\n> > }\n> >\n> > int init(const IPASettings &settings) override;\n> > @@ -108,13 +108,13 @@ private:\n> > void applySharpen(const struct SharpenStatus *sharpenStatus, ControlList &ctrls);\n> > void applyDPC(const struct DpcStatus *dpcStatus, ControlList &ctrls);\n> > void applyLS(const struct AlscStatus *lsStatus, ControlList &ctrls);\n> > - void resampleTable(uint16_t dest[], double const src[12][16], int dest_w, int dest_h);\n> > + void resampleTable(uint16_t dest[], double const src[12][16], int destW, int destH);\n> >\n> > std::map buffers_;\n> > std::map buffersMemory_;\n> >\n> > - ControlInfoMap unicam_ctrls_;\n> > - ControlInfoMap isp_ctrls_;\n> > + ControlInfoMap unicamCtrls_;\n> > + ControlInfoMap ispCtrls_;\n> > ControlList libcameraMetadata_;\n> >\n> > /* IPA configuration. */\n> > @@ -134,11 +134,11 @@ private:\n> > * We count frames to decide if the frame must be hidden (e.g. from\n> > * display) or mistrusted (i.e. not given to the control algos).\n> > */\n> > - uint64_t frame_count_;\n> > + uint64_t frameCount_;\n>\n> If we're doing clean up - I always prefer to see a newline before a\n> comment line.\n>\n> To me it makes the commented block clearer - but I don't know if there's\n> a 'rule' on it.\n>\n>\n> > /* For checking the sequencing of Prepare/Process calls. */\n> > - uint64_t check_count_;\n> > + uint64_t checkCount_;\n>\n> i.e. here too.\n>\n> > /* How many frames we should avoid running control algos on. */\n> > - unsigned int mistrust_count_;\n> > + unsigned int mistrustCount_;\n>\n> and so on.\n>\n> > /* LS table allocation passed in from the pipeline handler. */\n> > FileDescriptor lsTableHandle_;\n> > void *lsTable_;\n> > @@ -199,8 +199,8 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> >\n> > result->operation = 0;\n> >\n> > - unicam_ctrls_ = entityControls.at(0);\n> > - isp_ctrls_ = entityControls.at(1);\n> > + unicamCtrls_ = entityControls.at(0);\n> > + ispCtrls_ = entityControls.at(1);\n> > /* Setup a metadata ControlList to output metadata. */\n> > libcameraMetadata_ = ControlList(controls::controls);\n> >\n> > @@ -238,18 +238,18 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> > *\"mistrusted\", which depends on whether this is a startup from cold,\n> > * or merely a mode switch in a running system.\n> > */\n> > - frame_count_ = 0;\n> > - check_count_ = 0;\n> > - unsigned int drop_frame = 0;\n> > + frameCount_ = 0;\n> > + checkCount_ = 0;\n> > + unsigned int dropFrame = 0;\n> > if (controllerInit_) {\n> > - drop_frame = helper_->HideFramesModeSwitch();\n> > - mistrust_count_ = helper_->MistrustFramesModeSwitch();\n> > + dropFrame = helper_->HideFramesModeSwitch();\n> > + mistrustCount_ = helper_->MistrustFramesModeSwitch();\n> > } else {\n> > - drop_frame = helper_->HideFramesStartup();\n> > - mistrust_count_ = helper_->MistrustFramesStartup();\n> > + dropFrame = helper_->HideFramesStartup();\n> > + mistrustCount_ = helper_->MistrustFramesStartup();\n> > }\n> >\n> > - result->data.push_back(drop_frame);\n> > + result->data.push_back(dropFrame);\n> > result->operation |= RPi::IPA_CONFIG_DROP_FRAMES;\n> >\n> > struct AgcStatus agcStatus;\n> > @@ -264,8 +264,8 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> > controllerInit_ = true;\n> >\n> > /* Supply initial values for gain and exposure. */\n> > - agcStatus.shutter_time = DEFAULT_EXPOSURE_TIME;\n> > - agcStatus.analogue_gain = DEFAULT_ANALOGUE_GAIN;\n> > + agcStatus.shutter_time = DefaultExposureTime;\n> > + agcStatus.analogue_gain = DefaultAnalogueGain;\n> > }\n> >\n> > RPiController::Metadata metadata;\n> > @@ -274,7 +274,7 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> > /* SwitchMode may supply updated exposure/gain values to use. */\n> > metadata.Get(\"agc.status\", agcStatus);\n> > if (agcStatus.shutter_time != 0.0 && agcStatus.analogue_gain != 0.0) {\n> > - ControlList ctrls(unicam_ctrls_);\n> > + ControlList ctrls(unicamCtrls_);\n> > applyAGC(&agcStatus, ctrls);\n> > result->controls.push_back(ctrls);\n> >\n> > @@ -287,14 +287,14 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> > if (ipaConfig.operation & RPi::IPA_CONFIG_LS_TABLE) {\n> > /* Remove any previous table, if there was one. */\n> > if (lsTable_) {\n> > - munmap(lsTable_, MAX_LS_GRID_SIZE);\n> > + munmap(lsTable_, RPi::MaxLsGridSize);\n> > lsTable_ = nullptr;\n> > }\n> >\n> > /* Map the LS table buffer into user space. */\n> > lsTableHandle_ = FileDescriptor(ipaConfig.data[0]);\n> > if (lsTableHandle_.isValid()) {\n> > - lsTable_ = mmap(nullptr, MAX_LS_GRID_SIZE, PROT_READ | PROT_WRITE,\n> > + lsTable_ = mmap(nullptr, RPi::MaxLsGridSize, PROT_READ | PROT_WRITE,\n> > MAP_SHARED, lsTableHandle_.fd(), 0);\n> >\n> > if (lsTable_ == MAP_FAILED) {\n> > @@ -343,9 +343,9 @@ void IPARPi::processEvent(const IPAOperationData &event)\n> > case RPi::IPA_EVENT_SIGNAL_STAT_READY: {\n> > unsigned int bufferId = event.data[0];\n> >\n> > - if (++check_count_ != frame_count_) /* assert here? */\n> > + if (++checkCount_ != frameCount_) /* assert here? */\n> > LOG(IPARPI, Error) << \"WARNING: Prepare/Process mismatch!!!\";\n> > - if (frame_count_ > mistrust_count_)\n> > + if (frameCount_ > mistrustCount_)\n> > processStats(bufferId);\n> >\n> > reportMetadata();\n> > @@ -368,7 +368,7 @@ void IPARPi::processEvent(const IPAOperationData &event)\n> > * they are \"unreliable\".\n> > */\n> > prepareISP(embeddedbufferId);\n> > - frame_count_++;\n> > + frameCount_++;\n> >\n> > /* Ready to push the input buffer into the ISP. */\n> > IPAOperationData op;\n> > @@ -713,7 +713,7 @@ void IPARPi::prepareISP(unsigned int bufferId)\n> > returnEmbeddedBuffer(bufferId);\n> >\n> > if (success) {\n> > - ControlList ctrls(isp_ctrls_);\n> > + ControlList ctrls(ispCtrls_);\n> >\n> > rpiMetadata_.Clear();\n> > rpiMetadata_.Set(\"device.status\", deviceStatus);\n> > @@ -785,19 +785,19 @@ bool IPARPi::parseEmbeddedData(unsigned int bufferId, struct DeviceStatus &devic\n> > if (status != RPiController::MdParser::Status::OK) {\n> > LOG(IPARPI, Error) << \"Embedded Buffer parsing failed, error \" << status;\n> > } else {\n> > - uint32_t exposure_lines, gain_code;\n> > - if (helper_->Parser().GetExposureLines(exposure_lines) != RPiController::MdParser::Status::OK) {\n> > + uint32_t exposureLines, gainCode;\n> > + if (helper_->Parser().GetExposureLines(exposureLines) != RPiController::MdParser::Status::OK) {\n> > LOG(IPARPI, Error) << \"Exposure time failed\";\n> > return false;\n> > }\n> >\n> > - deviceStatus.shutter_speed = helper_->Exposure(exposure_lines);\n> > - if (helper_->Parser().GetGainCode(gain_code) != RPiController::MdParser::Status::OK) {\n> > + deviceStatus.shutter_speed = helper_->Exposure(exposureLines);\n> > + if (helper_->Parser().GetGainCode(gainCode) != RPiController::MdParser::Status::OK) {\n> > LOG(IPARPI, Error) << \"Gain failed\";\n> > return false;\n> > }\n> >\n> > - deviceStatus.analogue_gain = helper_->Gain(gain_code);\n> > + deviceStatus.analogue_gain = helper_->Gain(gainCode);\n> > LOG(IPARPI, Debug) << \"Metadata - Exposure : \"\n> > << deviceStatus.shutter_speed << \" Gain : \"\n> > << deviceStatus.analogue_gain;\n> > @@ -820,7 +820,7 @@ void IPARPi::processStats(unsigned int bufferId)\n> >\n> > struct AgcStatus agcStatus;\n> > if (rpiMetadata_.Get(\"agc.status\", agcStatus) == 0) {\n> > - ControlList ctrls(unicam_ctrls_);\n> > + ControlList ctrls(unicamCtrls_);\n> > applyAGC(&agcStatus, ctrls);\n> >\n> > IPAOperationData op;\n> > @@ -832,14 +832,14 @@ void IPARPi::processStats(unsigned int bufferId)\n> >\n> > void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls)\n> > {\n> > - const auto gainR = isp_ctrls_.find(V4L2_CID_RED_BALANCE);\n> > - if (gainR == isp_ctrls_.end()) {\n> > + const auto gainR = ispCtrls_.find(V4L2_CID_RED_BALANCE);\n> > + if (gainR == ispCtrls_.end()) {\n> > LOG(IPARPI, Error) << \"Can't find red gain control\";\n> > return;\n> > }\n> >\n> > - const auto gainB = isp_ctrls_.find(V4L2_CID_BLUE_BALANCE);\n> > - if (gainB == isp_ctrls_.end()) {\n> > + const auto gainB = ispCtrls_.find(V4L2_CID_BLUE_BALANCE);\n> > + if (gainB == ispCtrls_.end()) {\n> > LOG(IPARPI, Error) << \"Can't find blue gain control\";\n> > return;\n> > }\n> > @@ -855,31 +855,31 @@ void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls)\n> >\n> > void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls)\n> > {\n> > - int32_t gain_code = helper_->GainCode(agcStatus->analogue_gain);\n> > - int32_t exposure_lines = helper_->ExposureLines(agcStatus->shutter_time);\n> > + int32_t gainCode = helper_->GainCode(agcStatus->analogue_gain);\n> > + int32_t exposureLines = helper_->ExposureLines(agcStatus->shutter_time);\n> >\n> > - if (unicam_ctrls_.find(V4L2_CID_ANALOGUE_GAIN) == unicam_ctrls_.end()) {\n> > + if (unicamCtrls_.find(V4L2_CID_ANALOGUE_GAIN) == unicamCtrls_.end()) {\n> > LOG(IPARPI, Error) << \"Can't find analogue gain control\";\n> > return;\n> > }\n> >\n> > - if (unicam_ctrls_.find(V4L2_CID_EXPOSURE) == unicam_ctrls_.end()) {\n> > + if (unicamCtrls_.find(V4L2_CID_EXPOSURE) == unicamCtrls_.end()) {\n> > LOG(IPARPI, Error) << \"Can't find exposure control\";\n> > return;\n> > }\n> >\n> > LOG(IPARPI, Debug) << \"Applying AGC Exposure: \" << agcStatus->shutter_time\n> > - << \" (Shutter lines: \" << exposure_lines << \") Gain: \"\n> > + << \" (Shutter lines: \" << exposureLines << \") Gain: \"\n> > << agcStatus->analogue_gain << \" (Gain Code: \"\n> > - << gain_code << \")\";\n> > + << gainCode << \")\";\n> >\n> > - ctrls.set(V4L2_CID_ANALOGUE_GAIN, gain_code);\n> > - ctrls.set(V4L2_CID_EXPOSURE, exposure_lines);\n> > + ctrls.set(V4L2_CID_ANALOGUE_GAIN, gainCode);\n> > + ctrls.set(V4L2_CID_EXPOSURE, exposureLines);\n> > }\n> >\n> > void IPARPi::applyDG(const struct AgcStatus *dgStatus, ControlList &ctrls)\n> > {\n> > - if (isp_ctrls_.find(V4L2_CID_DIGITAL_GAIN) == isp_ctrls_.end()) {\n> > + if (ispCtrls_.find(V4L2_CID_DIGITAL_GAIN) == ispCtrls_.end()) {\n> > LOG(IPARPI, Error) << \"Can't find digital gain control\";\n> > return;\n> > }\n> > @@ -890,7 +890,7 @@ void IPARPi::applyDG(const struct AgcStatus *dgStatus, ControlList &ctrls)\n> >\n> > void IPARPi::applyCCM(const struct CcmStatus *ccmStatus, ControlList &ctrls)\n> > {\n> > - if (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_CC_MATRIX) == isp_ctrls_.end()) {\n> > + if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_CC_MATRIX) == ispCtrls_.end()) {\n> > LOG(IPARPI, Error) << \"Can't find CCM control\";\n> > return;\n> > }\n> > @@ -911,7 +911,7 @@ void IPARPi::applyCCM(const struct CcmStatus *ccmStatus, ControlList &ctrls)\n> >\n> > void IPARPi::applyGamma(const struct ContrastStatus *contrastStatus, ControlList &ctrls)\n> > {\n> > - if (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_GAMMA) == isp_ctrls_.end()) {\n> > + if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_GAMMA) == ispCtrls_.end()) {\n> > LOG(IPARPI, Error) << \"Can't find Gamma control\";\n> > return;\n> > }\n> > @@ -930,7 +930,7 @@ void IPARPi::applyGamma(const struct ContrastStatus *contrastStatus, ControlList\n> >\n> > void IPARPi::applyBlackLevel(const struct BlackLevelStatus *blackLevelStatus, ControlList &ctrls)\n> > {\n> > - if (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_BLACK_LEVEL) == isp_ctrls_.end()) {\n> > + if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_BLACK_LEVEL) == ispCtrls_.end()) {\n> > LOG(IPARPI, Error) << \"Can't find black level control\";\n> > return;\n> > }\n> > @@ -948,7 +948,7 @@ void IPARPi::applyBlackLevel(const struct BlackLevelStatus *blackLevelStatus, Co\n> >\n> > void IPARPi::applyGEQ(const struct GeqStatus *geqStatus, ControlList &ctrls)\n> > {\n> > - if (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_GEQ) == isp_ctrls_.end()) {\n> > + if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_GEQ) == ispCtrls_.end()) {\n> > LOG(IPARPI, Error) << \"Can't find geq control\";\n> > return;\n> > }\n> > @@ -966,7 +966,7 @@ void IPARPi::applyGEQ(const struct GeqStatus *geqStatus, ControlList &ctrls)\n> >\n> > void IPARPi::applyDenoise(const struct SdnStatus *denoiseStatus, ControlList &ctrls)\n> > {\n> > - if (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_DENOISE) == isp_ctrls_.end()) {\n> > + if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_DENOISE) == ispCtrls_.end()) {\n> > LOG(IPARPI, Error) << \"Can't find denoise control\";\n> > return;\n> > }\n> > @@ -986,7 +986,7 @@ void IPARPi::applyDenoise(const struct SdnStatus *denoiseStatus, ControlList &ct\n> >\n> > void IPARPi::applySharpen(const struct SharpenStatus *sharpenStatus, ControlList &ctrls)\n> > {\n> > - if (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_SHARPEN) == isp_ctrls_.end()) {\n> > + if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_SHARPEN) == ispCtrls_.end()) {\n> > LOG(IPARPI, Error) << \"Can't find sharpen control\";\n> > return;\n> > }\n> > @@ -1007,7 +1007,7 @@ void IPARPi::applySharpen(const struct SharpenStatus *sharpenStatus, ControlList\n> >\n> > void IPARPi::applyDPC(const struct DpcStatus *dpcStatus, ControlList &ctrls)\n> > {\n> > - if (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_DPC) == isp_ctrls_.end()) {\n> > + if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_DPC) == ispCtrls_.end()) {\n> > LOG(IPARPI, Error) << \"Can't find DPC control\";\n> > return;\n> > }\n> > @@ -1023,7 +1023,7 @@ void IPARPi::applyDPC(const struct DpcStatus *dpcStatus, ControlList &ctrls)\n> >\n> > void IPARPi::applyLS(const struct AlscStatus *lsStatus, ControlList &ctrls)\n> > {\n> > - if (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING) == isp_ctrls_.end()) {\n> > + if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING) == ispCtrls_.end()) {\n> > LOG(IPARPI, Error) << \"Can't find LS control\";\n> > return;\n> > }\n> > @@ -1032,18 +1032,18 @@ void IPARPi::applyLS(const struct AlscStatus *lsStatus, ControlList &ctrls)\n> > * Program lens shading tables into pipeline.\n> > * Choose smallest cell size that won't exceed 63x48 cells.\n> > */\n> > - const int cell_sizes[] = { 16, 32, 64, 128, 256 };\n> > - unsigned int num_cells = ARRAY_SIZE(cell_sizes);\n> > - unsigned int i, w, h, cell_size;\n> > - for (i = 0; i < num_cells; i++) {\n> > - cell_size = cell_sizes[i];\n> > - w = (mode_.width + cell_size - 1) / cell_size;\n> > - h = (mode_.height + cell_size - 1) / cell_size;\n> > + const int cellSizes[] = { 16, 32, 64, 128, 256 };\n> > + unsigned int numCells = ARRAY_SIZE(cellSizes);\n> > + unsigned int i, w, h, cellSize;\n> > + for (i = 0; i < numCells; i++) {\n> > + cellSize = cellSizes[i];\n> > + w = (mode_.width + cellSize - 1) / cellSize;\n> > + h = (mode_.height + cellSize - 1) / cellSize;\n> > if (w < 64 && h <= 48)\n> > break;\n> > }\n> >\n> > - if (i == num_cells) {\n> > + if (i == numCells) {\n> > LOG(IPARPI, Error) << \"Cannot find cell size\";\n> > return;\n> > }\n> > @@ -1052,7 +1052,7 @@ void IPARPi::applyLS(const struct AlscStatus *lsStatus, ControlList &ctrls)\n> > w++, h++;\n> > bcm2835_isp_lens_shading ls = {\n> > .enabled = 1,\n> > - .grid_cell_size = cell_size,\n> > + .grid_cell_size = cellSize,\n> > .grid_width = w,\n> > .grid_stride = w,\n> > .grid_height = h,\n> > @@ -1062,7 +1062,7 @@ void IPARPi::applyLS(const struct AlscStatus *lsStatus, ControlList &ctrls)\n> > .gain_format = GAIN_FORMAT_U4P10\n> > };\n> >\n> > - if (!lsTable_ || w * h * 4 * sizeof(uint16_t) > MAX_LS_GRID_SIZE) {\n> > + if (!lsTable_ || w * h * 4 * sizeof(uint16_t) > RPi::MaxLsGridSize) {\n> > LOG(IPARPI, Error) << \"Do not have a correctly allocate lens shading table!\";\n> > return;\n> > }\n> > @@ -1083,41 +1083,41 @@ void IPARPi::applyLS(const struct AlscStatus *lsStatus, ControlList &ctrls)\n> > }\n> >\n> > /*\n> > - * Resamples a 16x12 table with central sampling to dest_w x dest_h with corner\n> > + * Resamples a 16x12 table with central sampling to destW x destH with corner\n> > * sampling.\n> > */\n> > void IPARPi::resampleTable(uint16_t dest[], double const src[12][16],\n> > - int dest_w, int dest_h)\n> > + int destW, int destH)\n> > {\n> > /*\n> > * Precalculate and cache the x sampling locations and phases to\n> > * save recomputing them on every row.\n> > */\n> > - assert(dest_w > 1 && dest_h > 1 && dest_w <= 64);\n> > - int x_lo[64], x_hi[64];\n> > + assert(destW > 1 && destH > 1 && destW <= 64);\n> > + int xLo[64], xHi[64];\n> > double xf[64];\n> > - double x = -0.5, x_inc = 16.0 / (dest_w - 1);\n> > - for (int i = 0; i < dest_w; i++, x += x_inc) {\n> > - x_lo[i] = floor(x);\n> > - xf[i] = x - x_lo[i];\n> > - x_hi[i] = x_lo[i] < 15 ? x_lo[i] + 1 : 15;\n> > - x_lo[i] = x_lo[i] > 0 ? x_lo[i] : 0;\n> > + double x = -0.5, xInc = 16.0 / (destW - 1);\n> > + for (int i = 0; i < destW; i++, x += xInc) {\n> > + xLo[i] = floor(x);\n> > + xf[i] = x - xLo[i];\n> > + xHi[i] = xLo[i] < 15 ? xLo[i] + 1 : 15;\n> > + xLo[i] = xLo[i] > 0 ? xLo[i] : 0;\n> > }\n> >\n> > /* Now march over the output table generating the new values. */\n> > - double y = -0.5, y_inc = 12.0 / (dest_h - 1);\n> > - for (int j = 0; j < dest_h; j++, y += y_inc) {\n> > - int y_lo = floor(y);\n> > - double yf = y - y_lo;\n> > - int y_hi = y_lo < 11 ? y_lo + 1 : 11;\n> > - y_lo = y_lo > 0 ? y_lo : 0;\n> > - double const *row_above = src[y_lo];\n> > - double const *row_below = src[y_hi];\n> > - for (int i = 0; i < dest_w; i++) {\n> > - double above = row_above[x_lo[i]] * (1 - xf[i])\n> > - + row_above[x_hi[i]] * xf[i];\n> > - double below = row_below[x_lo[i]] * (1 - xf[i])\n> > - + row_below[x_hi[i]] * xf[i];\n> > + double y = -0.5, yInc = 12.0 / (destH - 1);\n> > + for (int j = 0; j < destH; j++, y += yInc) {\n> > + int yLo = floor(y);\n> > + double yf = y - yLo;\n> > + int yHi = yLo < 11 ? yLo + 1 : 11;\n> > + yLo = yLo > 0 ? yLo : 0;\n> > + double const *rowAbove = src[yLo];\n> > + double const *rowBelow = src[yHi];\n> > + for (int i = 0; i < destW; i++) {\n> > + double above = rowAbove[xLo[i]] * (1 - xf[i])\n> > + + rowAbove[xHi[i]] * xf[i];\n> > + double below = rowBelow[xLo[i]] * (1 - xf[i])\n> > + + rowBelow[xHi[i]] * xf[i];\n> > int result = floor(1024 * (above * (1 - yf) + below * yf) + .5);\n> > *(dest++) = result > 16383 ? 16383 : result; /* want u4.10 */\n> > }\n> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > index 35dbe0fb..8d40b0ed 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > @@ -1012,7 +1012,7 @@ int RPiCameraData::configureIPA()\n> >\n> > /* Allocate the lens shading table via dmaHeap and pass to the IPA. */\n> > if (!lsTable_.isValid()) {\n> > - lsTable_ = dmaHeap_.alloc(\"ls_grid\", MAX_LS_GRID_SIZE);\n> > + lsTable_ = dmaHeap_.alloc(\"ls_grid\", RPi::MaxLsGridSize);\n>\n> If we had an IPA namespace, it would be nice to see this as:\n>\n> lsTable_ = dmaHeap_.alloc(\"ls_grid\", ipa::RPi::MaxLsGridSize);\n>\n> As that would make it really clear 'where' this MaxLsGridSize value was\n> coming from...\n>\n> Anyway, all my comments above are optional discussion points.\n>\n> For the patch:\n>\n> Reviewed-by: Kieran Bingham \n>\n> > if (!lsTable_.isValid())\n> > return -ENOMEM;\n> >\n> >\n>\n> --\n> Regards\n> --\n> Kieran\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 3D19DC3B5C\n\tfor ;\n\tWed, 23 Sep 2020 16:31:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AA20462FE3;\n\tWed, 23 Sep 2020 18:31:58 +0200 (CEST)","from mail-oi1-x241.google.com (mail-oi1-x241.google.com\n\t[IPv6:2607:f8b0:4864:20::241])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EC56860576\n\tfor ;\n\tWed, 23 Sep 2020 18:31:56 +0200 (CEST)","by mail-oi1-x241.google.com with SMTP id c13so490088oiy.6\n\tfor ;\n\tWed, 23 Sep 2020 09:31:56 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"ZEW72yeN\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=6E+C7Q+RHHdYLuf6KYtZ5W0ihH4Jf06zGkurVGsm8/Y=;\n\tb=ZEW72yeNWm+JlFt+4UaTCjYuBudbPcWosLEMv59crhAfvkpC9nKjV9rsCp/H2uXusH\n\tQdzzaDnvarv4C7UAwhxKM8ARj9qdG+pNoozUegKhDrQ3RLh1xmd1mUkBByHmwqehFF2r\n\tGGKNAPw6TO9jvHBicGUs8SgHvuEeoOZyOeeCRgSfIyNm0QfTORBUKRbopGAxGFb/Y3Z4\n\tC0OVRPfGBzV2pQFqHKbTcL00KWE2nZ0rBj/xeGty0rPr96UonokdUCt5Yn5++t36bY+f\n\tlfWvC8Lwv9GwicBAuLAzohO3ZE4vZNS14JGFfIyvH/G5q2oP6QWPbFrvjgI1mn4iiTYb\n\tyB6A==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=6E+C7Q+RHHdYLuf6KYtZ5W0ihH4Jf06zGkurVGsm8/Y=;\n\tb=Cwq6r3+fJDc7E0xJlt8Hr4g2hBokLIeWPVbXR4F46tGCFpKZjHw2AyK+lLtq1qnkHQ\n\tn5OElfFqGJ2qM5fajIxl+47tC0lzsKmBP5Wst0ghWAW0JbId1mkC3pAV0ARFpk3rEFUw\n\t9NkkRugbMCe73Vx/sPnpvi5Lhdl7FKxXR8dDgRPzmLU/ByKIyqQ9zrhoX1S0FfbyNH6E\n\tjMxZ1bFwBVFtArOI1JOJDhpGnkz4pqQ4gLd/R1bSmpWfA9JqvY6uYk1hZYkN8zfqMnTR\n\t8QD30kmCOS8ddtAk1daFNCQ02//hqoP3TZrQd4aeQMMVt4bW/m6tPX/6EGK2zC5DT5qv\n\t4Djg==","X-Gm-Message-State":"AOAM532CYbZB7WkzB+LI1aCsZWjqDL6P/aYqMJCWCF0vVyo9mD2V1Yu6\n\tO6jnJIHyePoDIyZ7nBQPVAde+EnKK3kLec6KuIHQqsw/LC8=","X-Google-Smtp-Source":"ABdhPJwSpEqp17msn1afoC8CCE8ZbB2HRjPz3+nz1CP60wrJwmFnrHZfzaHN4M5jdDraxNgJ20VayvRKnIFNgulPGSc=","X-Received":"by 2002:aca:42c2:: with SMTP id p185mr187034oia.55.1600878715138;\n\tWed, 23 Sep 2020 09:31:55 -0700 (PDT)","MIME-Version":"1.0","References":"<20200922095018.68434-1-naush@raspberrypi.com>\n\t<20200922095018.68434-5-naush@raspberrypi.com>\n\t","In-Reply-To":"","From":"David Plowman ","Date":"Wed, 23 Sep 2020 17:31:43 +0100","Message-ID":"","To":"Kieran Bingham ","Subject":"Re: [libcamera-devel] [PATCH 4/4] ipa: raspberrypi: Tidy up\n\tvariable names to be consistent","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":12683,"web_url":"https://patchwork.libcamera.org/comment/12683/","msgid":"","date":"2020-09-23T19:53:13","subject":"Re: [libcamera-devel] [PATCH 4/4] ipa: raspberrypi: Tidy up\n\tvariable names to be consistent","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi David, Naush,\n\nOn 23/09/2020 17:31, David Plowman wrote:\n> Hi Naush\n> \n> Thanks for doing all this tidying! One little nit-pick...\n> \n> On Wed, 23 Sep 2020 at 12:18, Kieran Bingham\n> wrote:\n>>\n>> Hi Naush,\n>>\n>> On 22/09/2020 10:50, Naushir Patuck wrote:\n>>> Change variable names to camel case to be consistent with the rest of\n>>> the source files. Remove #define consts and replace with constexpr.\n>>>\n>>\n>> Sounds good to me...\n>>\n>>\n>>> There are no functional changes in this commit.\n>>>\n>>> Signed-off-by: Naushir Patuck \n>>> ---\n>>> include/libcamera/ipa/raspberrypi.h | 2 +-\n>>> src/ipa/raspberrypi/raspberrypi.cpp | 178 +++++++++---------\n>>> .../pipeline/raspberrypi/raspberrypi.cpp | 2 +-\n>>> 3 files changed, 91 insertions(+), 91 deletions(-)\n>>>\n>>> diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h\n>>> index c9d4aa81..b3041591 100644\n>>> --- a/include/libcamera/ipa/raspberrypi.h\n>>> +++ b/include/libcamera/ipa/raspberrypi.h\n>>> @@ -41,7 +41,7 @@ enum BufferMask {\n>>> };\n>>>\n>>> /* Size of the LS grid allocation. */\n>>> -#define MAX_LS_GRID_SIZE (32 << 10)\n>>> +static constexpr unsigned int MaxLsGridSize = 32 << 10;\n>>\n>> I guess the LS could stay capitalised (MaxLSGridSize), not sure that it\n>> matters, so up to you.\n>>\n>>> /* List of controls handled by the Raspberry Pi IPA */\n>>> static const ControlInfoMap Controls = {\n>>> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n>>> index 0c0dc743..c240eae8 100644\n>>> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n>>> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n>>> @@ -55,8 +55,8 @@\n>>> namespace libcamera {\n>>>\n>>> /* Configure the sensor with these values initially. */\n>>> -#define DEFAULT_ANALOGUE_GAIN 1.0\n>>> -#define DEFAULT_EXPOSURE_TIME 20000\n>>> +constexpr unsigned int DefaultAnalogueGain = 1.0;\n>>> +constexpr unsigned int DefaultExposureTime = 20000;\n>>\n>> Oh nice, This really highlights to me the benefit of constexpr.\n>> Now the values have types!\n> \n> Should DefaultAnalogueGain be a double? Of course it gets cast to a\n> double when it gets copied into agcStatus.analogue_gain so it probably\n> contrives to make no difference, but a double might be clearer,\n> especially if anyone ever changed it.\n\nBoom, and there's the types! I was so keen on seeing the types, and\nnoticing the 1.0 - I didn't even notice that it was set as an unsigned\nint. :-S\n\nUhm... What I mean is - I agree with David, and I'm disappointed with\nmyself ;-)\n\n--\nKieran\n\n> Thanks!\n> David\n> \n>>\n>>>\n>>> LOG_DEFINE_CATEGORY(IPARPI)\n>>>\n>>> @@ -65,7 +65,7 @@ class IPARPi : public IPAInterface\n>>> public:\n>>> IPARPi()\n>>> : lastMode_({}), controller_(), controllerInit_(false),\n>>> - frame_count_(0), check_count_(0), mistrust_count_(0),\n>>> + frameCount_(0), checkCount_(0), mistrustCount_(0),\n>>> lsTable_(nullptr)\n>>> {\n>>> }\n>>> @@ -73,7 +73,7 @@ public:\n>>> ~IPARPi()\n>>> {\n>>> if (lsTable_)\n>>> - munmap(lsTable_, MAX_LS_GRID_SIZE);\n>>> + munmap(lsTable_, RPi::MaxLsGridSize);\n>>> }\n>>>\n>>> int init(const IPASettings &settings) override;\n>>> @@ -108,13 +108,13 @@ private:\n>>> void applySharpen(const struct SharpenStatus *sharpenStatus, ControlList &ctrls);\n>>> void applyDPC(const struct DpcStatus *dpcStatus, ControlList &ctrls);\n>>> void applyLS(const struct AlscStatus *lsStatus, ControlList &ctrls);\n>>> - void resampleTable(uint16_t dest[], double const src[12][16], int dest_w, int dest_h);\n>>> + void resampleTable(uint16_t dest[], double const src[12][16], int destW, int destH);\n>>>\n>>> std::map buffers_;\n>>> std::map buffersMemory_;\n>>>\n>>> - ControlInfoMap unicam_ctrls_;\n>>> - ControlInfoMap isp_ctrls_;\n>>> + ControlInfoMap unicamCtrls_;\n>>> + ControlInfoMap ispCtrls_;\n>>> ControlList libcameraMetadata_;\n>>>\n>>> /* IPA configuration. */\n>>> @@ -134,11 +134,11 @@ private:\n>>> * We count frames to decide if the frame must be hidden (e.g. from\n>>> * display) or mistrusted (i.e. not given to the control algos).\n>>> */\n>>> - uint64_t frame_count_;\n>>> + uint64_t frameCount_;\n>>\n>> If we're doing clean up - I always prefer to see a newline before a\n>> comment line.\n>>\n>> To me it makes the commented block clearer - but I don't know if there's\n>> a 'rule' on it.\n>>\n>>\n>>> /* For checking the sequencing of Prepare/Process calls. */\n>>> - uint64_t check_count_;\n>>> + uint64_t checkCount_;\n>>\n>> i.e. here too.\n>>\n>>> /* How many frames we should avoid running control algos on. */\n>>> - unsigned int mistrust_count_;\n>>> + unsigned int mistrustCount_;\n>>\n>> and so on.\n>>\n>>> /* LS table allocation passed in from the pipeline handler. */\n>>> FileDescriptor lsTableHandle_;\n>>> void *lsTable_;\n>>> @@ -199,8 +199,8 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n>>>\n>>> result->operation = 0;\n>>>\n>>> - unicam_ctrls_ = entityControls.at(0);\n>>> - isp_ctrls_ = entityControls.at(1);\n>>> + unicamCtrls_ = entityControls.at(0);\n>>> + ispCtrls_ = entityControls.at(1);\n>>> /* Setup a metadata ControlList to output metadata. */\n>>> libcameraMetadata_ = ControlList(controls::controls);\n>>>\n>>> @@ -238,18 +238,18 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n>>> *\"mistrusted\", which depends on whether this is a startup from cold,\n>>> * or merely a mode switch in a running system.\n>>> */\n>>> - frame_count_ = 0;\n>>> - check_count_ = 0;\n>>> - unsigned int drop_frame = 0;\n>>> + frameCount_ = 0;\n>>> + checkCount_ = 0;\n>>> + unsigned int dropFrame = 0;\n>>> if (controllerInit_) {\n>>> - drop_frame = helper_->HideFramesModeSwitch();\n>>> - mistrust_count_ = helper_->MistrustFramesModeSwitch();\n>>> + dropFrame = helper_->HideFramesModeSwitch();\n>>> + mistrustCount_ = helper_->MistrustFramesModeSwitch();\n>>> } else {\n>>> - drop_frame = helper_->HideFramesStartup();\n>>> - mistrust_count_ = helper_->MistrustFramesStartup();\n>>> + dropFrame = helper_->HideFramesStartup();\n>>> + mistrustCount_ = helper_->MistrustFramesStartup();\n>>> }\n>>>\n>>> - result->data.push_back(drop_frame);\n>>> + result->data.push_back(dropFrame);\n>>> result->operation |= RPi::IPA_CONFIG_DROP_FRAMES;\n>>>\n>>> struct AgcStatus agcStatus;\n>>> @@ -264,8 +264,8 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n>>> controllerInit_ = true;\n>>>\n>>> /* Supply initial values for gain and exposure. */\n>>> - agcStatus.shutter_time = DEFAULT_EXPOSURE_TIME;\n>>> - agcStatus.analogue_gain = DEFAULT_ANALOGUE_GAIN;\n>>> + agcStatus.shutter_time = DefaultExposureTime;\n>>> + agcStatus.analogue_gain = DefaultAnalogueGain;\n>>> }\n>>>\n>>> RPiController::Metadata metadata;\n>>> @@ -274,7 +274,7 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n>>> /* SwitchMode may supply updated exposure/gain values to use. */\n>>> metadata.Get(\"agc.status\", agcStatus);\n>>> if (agcStatus.shutter_time != 0.0 && agcStatus.analogue_gain != 0.0) {\n>>> - ControlList ctrls(unicam_ctrls_);\n>>> + ControlList ctrls(unicamCtrls_);\n>>> applyAGC(&agcStatus, ctrls);\n>>> result->controls.push_back(ctrls);\n>>>\n>>> @@ -287,14 +287,14 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n>>> if (ipaConfig.operation & RPi::IPA_CONFIG_LS_TABLE) {\n>>> /* Remove any previous table, if there was one. */\n>>> if (lsTable_) {\n>>> - munmap(lsTable_, MAX_LS_GRID_SIZE);\n>>> + munmap(lsTable_, RPi::MaxLsGridSize);\n>>> lsTable_ = nullptr;\n>>> }\n>>>\n>>> /* Map the LS table buffer into user space. */\n>>> lsTableHandle_ = FileDescriptor(ipaConfig.data[0]);\n>>> if (lsTableHandle_.isValid()) {\n>>> - lsTable_ = mmap(nullptr, MAX_LS_GRID_SIZE, PROT_READ | PROT_WRITE,\n>>> + lsTable_ = mmap(nullptr, RPi::MaxLsGridSize, PROT_READ | PROT_WRITE,\n>>> MAP_SHARED, lsTableHandle_.fd(), 0);\n>>>\n>>> if (lsTable_ == MAP_FAILED) {\n>>> @@ -343,9 +343,9 @@ void IPARPi::processEvent(const IPAOperationData &event)\n>>> case RPi::IPA_EVENT_SIGNAL_STAT_READY: {\n>>> unsigned int bufferId = event.data[0];\n>>>\n>>> - if (++check_count_ != frame_count_) /* assert here? */\n>>> + if (++checkCount_ != frameCount_) /* assert here? */\n>>> LOG(IPARPI, Error) << \"WARNING: Prepare/Process mismatch!!!\";\n>>> - if (frame_count_ > mistrust_count_)\n>>> + if (frameCount_ > mistrustCount_)\n>>> processStats(bufferId);\n>>>\n>>> reportMetadata();\n>>> @@ -368,7 +368,7 @@ void IPARPi::processEvent(const IPAOperationData &event)\n>>> * they are \"unreliable\".\n>>> */\n>>> prepareISP(embeddedbufferId);\n>>> - frame_count_++;\n>>> + frameCount_++;\n>>>\n>>> /* Ready to push the input buffer into the ISP. */\n>>> IPAOperationData op;\n>>> @@ -713,7 +713,7 @@ void IPARPi::prepareISP(unsigned int bufferId)\n>>> returnEmbeddedBuffer(bufferId);\n>>>\n>>> if (success) {\n>>> - ControlList ctrls(isp_ctrls_);\n>>> + ControlList ctrls(ispCtrls_);\n>>>\n>>> rpiMetadata_.Clear();\n>>> rpiMetadata_.Set(\"device.status\", deviceStatus);\n>>> @@ -785,19 +785,19 @@ bool IPARPi::parseEmbeddedData(unsigned int bufferId, struct DeviceStatus &devic\n>>> if (status != RPiController::MdParser::Status::OK) {\n>>> LOG(IPARPI, Error) << \"Embedded Buffer parsing failed, error \" << status;\n>>> } else {\n>>> - uint32_t exposure_lines, gain_code;\n>>> - if (helper_->Parser().GetExposureLines(exposure_lines) != RPiController::MdParser::Status::OK) {\n>>> + uint32_t exposureLines, gainCode;\n>>> + if (helper_->Parser().GetExposureLines(exposureLines) != RPiController::MdParser::Status::OK) {\n>>> LOG(IPARPI, Error) << \"Exposure time failed\";\n>>> return false;\n>>> }\n>>>\n>>> - deviceStatus.shutter_speed = helper_->Exposure(exposure_lines);\n>>> - if (helper_->Parser().GetGainCode(gain_code) != RPiController::MdParser::Status::OK) {\n>>> + deviceStatus.shutter_speed = helper_->Exposure(exposureLines);\n>>> + if (helper_->Parser().GetGainCode(gainCode) != RPiController::MdParser::Status::OK) {\n>>> LOG(IPARPI, Error) << \"Gain failed\";\n>>> return false;\n>>> }\n>>>\n>>> - deviceStatus.analogue_gain = helper_->Gain(gain_code);\n>>> + deviceStatus.analogue_gain = helper_->Gain(gainCode);\n>>> LOG(IPARPI, Debug) << \"Metadata - Exposure : \"\n>>> << deviceStatus.shutter_speed << \" Gain : \"\n>>> << deviceStatus.analogue_gain;\n>>> @@ -820,7 +820,7 @@ void IPARPi::processStats(unsigned int bufferId)\n>>>\n>>> struct AgcStatus agcStatus;\n>>> if (rpiMetadata_.Get(\"agc.status\", agcStatus) == 0) {\n>>> - ControlList ctrls(unicam_ctrls_);\n>>> + ControlList ctrls(unicamCtrls_);\n>>> applyAGC(&agcStatus, ctrls);\n>>>\n>>> IPAOperationData op;\n>>> @@ -832,14 +832,14 @@ void IPARPi::processStats(unsigned int bufferId)\n>>>\n>>> void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls)\n>>> {\n>>> - const auto gainR = isp_ctrls_.find(V4L2_CID_RED_BALANCE);\n>>> - if (gainR == isp_ctrls_.end()) {\n>>> + const auto gainR = ispCtrls_.find(V4L2_CID_RED_BALANCE);\n>>> + if (gainR == ispCtrls_.end()) {\n>>> LOG(IPARPI, Error) << \"Can't find red gain control\";\n>>> return;\n>>> }\n>>>\n>>> - const auto gainB = isp_ctrls_.find(V4L2_CID_BLUE_BALANCE);\n>>> - if (gainB == isp_ctrls_.end()) {\n>>> + const auto gainB = ispCtrls_.find(V4L2_CID_BLUE_BALANCE);\n>>> + if (gainB == ispCtrls_.end()) {\n>>> LOG(IPARPI, Error) << \"Can't find blue gain control\";\n>>> return;\n>>> }\n>>> @@ -855,31 +855,31 @@ void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls)\n>>>\n>>> void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls)\n>>> {\n>>> - int32_t gain_code = helper_->GainCode(agcStatus->analogue_gain);\n>>> - int32_t exposure_lines = helper_->ExposureLines(agcStatus->shutter_time);\n>>> + int32_t gainCode = helper_->GainCode(agcStatus->analogue_gain);\n>>> + int32_t exposureLines = helper_->ExposureLines(agcStatus->shutter_time);\n>>>\n>>> - if (unicam_ctrls_.find(V4L2_CID_ANALOGUE_GAIN) == unicam_ctrls_.end()) {\n>>> + if (unicamCtrls_.find(V4L2_CID_ANALOGUE_GAIN) == unicamCtrls_.end()) {\n>>> LOG(IPARPI, Error) << \"Can't find analogue gain control\";\n>>> return;\n>>> }\n>>>\n>>> - if (unicam_ctrls_.find(V4L2_CID_EXPOSURE) == unicam_ctrls_.end()) {\n>>> + if (unicamCtrls_.find(V4L2_CID_EXPOSURE) == unicamCtrls_.end()) {\n>>> LOG(IPARPI, Error) << \"Can't find exposure control\";\n>>> return;\n>>> }\n>>>\n>>> LOG(IPARPI, Debug) << \"Applying AGC Exposure: \" << agcStatus->shutter_time\n>>> - << \" (Shutter lines: \" << exposure_lines << \") Gain: \"\n>>> + << \" (Shutter lines: \" << exposureLines << \") Gain: \"\n>>> << agcStatus->analogue_gain << \" (Gain Code: \"\n>>> - << gain_code << \")\";\n>>> + << gainCode << \")\";\n>>>\n>>> - ctrls.set(V4L2_CID_ANALOGUE_GAIN, gain_code);\n>>> - ctrls.set(V4L2_CID_EXPOSURE, exposure_lines);\n>>> + ctrls.set(V4L2_CID_ANALOGUE_GAIN, gainCode);\n>>> + ctrls.set(V4L2_CID_EXPOSURE, exposureLines);\n>>> }\n>>>\n>>> void IPARPi::applyDG(const struct AgcStatus *dgStatus, ControlList &ctrls)\n>>> {\n>>> - if (isp_ctrls_.find(V4L2_CID_DIGITAL_GAIN) == isp_ctrls_.end()) {\n>>> + if (ispCtrls_.find(V4L2_CID_DIGITAL_GAIN) == ispCtrls_.end()) {\n>>> LOG(IPARPI, Error) << \"Can't find digital gain control\";\n>>> return;\n>>> }\n>>> @@ -890,7 +890,7 @@ void IPARPi::applyDG(const struct AgcStatus *dgStatus, ControlList &ctrls)\n>>>\n>>> void IPARPi::applyCCM(const struct CcmStatus *ccmStatus, ControlList &ctrls)\n>>> {\n>>> - if (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_CC_MATRIX) == isp_ctrls_.end()) {\n>>> + if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_CC_MATRIX) == ispCtrls_.end()) {\n>>> LOG(IPARPI, Error) << \"Can't find CCM control\";\n>>> return;\n>>> }\n>>> @@ -911,7 +911,7 @@ void IPARPi::applyCCM(const struct CcmStatus *ccmStatus, ControlList &ctrls)\n>>>\n>>> void IPARPi::applyGamma(const struct ContrastStatus *contrastStatus, ControlList &ctrls)\n>>> {\n>>> - if (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_GAMMA) == isp_ctrls_.end()) {\n>>> + if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_GAMMA) == ispCtrls_.end()) {\n>>> LOG(IPARPI, Error) << \"Can't find Gamma control\";\n>>> return;\n>>> }\n>>> @@ -930,7 +930,7 @@ void IPARPi::applyGamma(const struct ContrastStatus *contrastStatus, ControlList\n>>>\n>>> void IPARPi::applyBlackLevel(const struct BlackLevelStatus *blackLevelStatus, ControlList &ctrls)\n>>> {\n>>> - if (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_BLACK_LEVEL) == isp_ctrls_.end()) {\n>>> + if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_BLACK_LEVEL) == ispCtrls_.end()) {\n>>> LOG(IPARPI, Error) << \"Can't find black level control\";\n>>> return;\n>>> }\n>>> @@ -948,7 +948,7 @@ void IPARPi::applyBlackLevel(const struct BlackLevelStatus *blackLevelStatus, Co\n>>>\n>>> void IPARPi::applyGEQ(const struct GeqStatus *geqStatus, ControlList &ctrls)\n>>> {\n>>> - if (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_GEQ) == isp_ctrls_.end()) {\n>>> + if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_GEQ) == ispCtrls_.end()) {\n>>> LOG(IPARPI, Error) << \"Can't find geq control\";\n>>> return;\n>>> }\n>>> @@ -966,7 +966,7 @@ void IPARPi::applyGEQ(const struct GeqStatus *geqStatus, ControlList &ctrls)\n>>>\n>>> void IPARPi::applyDenoise(const struct SdnStatus *denoiseStatus, ControlList &ctrls)\n>>> {\n>>> - if (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_DENOISE) == isp_ctrls_.end()) {\n>>> + if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_DENOISE) == ispCtrls_.end()) {\n>>> LOG(IPARPI, Error) << \"Can't find denoise control\";\n>>> return;\n>>> }\n>>> @@ -986,7 +986,7 @@ void IPARPi::applyDenoise(const struct SdnStatus *denoiseStatus, ControlList &ct\n>>>\n>>> void IPARPi::applySharpen(const struct SharpenStatus *sharpenStatus, ControlList &ctrls)\n>>> {\n>>> - if (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_SHARPEN) == isp_ctrls_.end()) {\n>>> + if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_SHARPEN) == ispCtrls_.end()) {\n>>> LOG(IPARPI, Error) << \"Can't find sharpen control\";\n>>> return;\n>>> }\n>>> @@ -1007,7 +1007,7 @@ void IPARPi::applySharpen(const struct SharpenStatus *sharpenStatus, ControlList\n>>>\n>>> void IPARPi::applyDPC(const struct DpcStatus *dpcStatus, ControlList &ctrls)\n>>> {\n>>> - if (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_DPC) == isp_ctrls_.end()) {\n>>> + if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_DPC) == ispCtrls_.end()) {\n>>> LOG(IPARPI, Error) << \"Can't find DPC control\";\n>>> return;\n>>> }\n>>> @@ -1023,7 +1023,7 @@ void IPARPi::applyDPC(const struct DpcStatus *dpcStatus, ControlList &ctrls)\n>>>\n>>> void IPARPi::applyLS(const struct AlscStatus *lsStatus, ControlList &ctrls)\n>>> {\n>>> - if (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING) == isp_ctrls_.end()) {\n>>> + if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING) == ispCtrls_.end()) {\n>>> LOG(IPARPI, Error) << \"Can't find LS control\";\n>>> return;\n>>> }\n>>> @@ -1032,18 +1032,18 @@ void IPARPi::applyLS(const struct AlscStatus *lsStatus, ControlList &ctrls)\n>>> * Program lens shading tables into pipeline.\n>>> * Choose smallest cell size that won't exceed 63x48 cells.\n>>> */\n>>> - const int cell_sizes[] = { 16, 32, 64, 128, 256 };\n>>> - unsigned int num_cells = ARRAY_SIZE(cell_sizes);\n>>> - unsigned int i, w, h, cell_size;\n>>> - for (i = 0; i < num_cells; i++) {\n>>> - cell_size = cell_sizes[i];\n>>> - w = (mode_.width + cell_size - 1) / cell_size;\n>>> - h = (mode_.height + cell_size - 1) / cell_size;\n>>> + const int cellSizes[] = { 16, 32, 64, 128, 256 };\n>>> + unsigned int numCells = ARRAY_SIZE(cellSizes);\n>>> + unsigned int i, w, h, cellSize;\n>>> + for (i = 0; i < numCells; i++) {\n>>> + cellSize = cellSizes[i];\n>>> + w = (mode_.width + cellSize - 1) / cellSize;\n>>> + h = (mode_.height + cellSize - 1) / cellSize;\n>>> if (w < 64 && h <= 48)\n>>> break;\n>>> }\n>>>\n>>> - if (i == num_cells) {\n>>> + if (i == numCells) {\n>>> LOG(IPARPI, Error) << \"Cannot find cell size\";\n>>> return;\n>>> }\n>>> @@ -1052,7 +1052,7 @@ void IPARPi::applyLS(const struct AlscStatus *lsStatus, ControlList &ctrls)\n>>> w++, h++;\n>>> bcm2835_isp_lens_shading ls = {\n>>> .enabled = 1,\n>>> - .grid_cell_size = cell_size,\n>>> + .grid_cell_size = cellSize,\n>>> .grid_width = w,\n>>> .grid_stride = w,\n>>> .grid_height = h,\n>>> @@ -1062,7 +1062,7 @@ void IPARPi::applyLS(const struct AlscStatus *lsStatus, ControlList &ctrls)\n>>> .gain_format = GAIN_FORMAT_U4P10\n>>> };\n>>>\n>>> - if (!lsTable_ || w * h * 4 * sizeof(uint16_t) > MAX_LS_GRID_SIZE) {\n>>> + if (!lsTable_ || w * h * 4 * sizeof(uint16_t) > RPi::MaxLsGridSize) {\n>>> LOG(IPARPI, Error) << \"Do not have a correctly allocate lens shading table!\";\n>>> return;\n>>> }\n>>> @@ -1083,41 +1083,41 @@ void IPARPi::applyLS(const struct AlscStatus *lsStatus, ControlList &ctrls)\n>>> }\n>>>\n>>> /*\n>>> - * Resamples a 16x12 table with central sampling to dest_w x dest_h with corner\n>>> + * Resamples a 16x12 table with central sampling to destW x destH with corner\n>>> * sampling.\n>>> */\n>>> void IPARPi::resampleTable(uint16_t dest[], double const src[12][16],\n>>> - int dest_w, int dest_h)\n>>> + int destW, int destH)\n>>> {\n>>> /*\n>>> * Precalculate and cache the x sampling locations and phases to\n>>> * save recomputing them on every row.\n>>> */\n>>> - assert(dest_w > 1 && dest_h > 1 && dest_w <= 64);\n>>> - int x_lo[64], x_hi[64];\n>>> + assert(destW > 1 && destH > 1 && destW <= 64);\n>>> + int xLo[64], xHi[64];\n>>> double xf[64];\n>>> - double x = -0.5, x_inc = 16.0 / (dest_w - 1);\n>>> - for (int i = 0; i < dest_w; i++, x += x_inc) {\n>>> - x_lo[i] = floor(x);\n>>> - xf[i] = x - x_lo[i];\n>>> - x_hi[i] = x_lo[i] < 15 ? x_lo[i] + 1 : 15;\n>>> - x_lo[i] = x_lo[i] > 0 ? x_lo[i] : 0;\n>>> + double x = -0.5, xInc = 16.0 / (destW - 1);\n>>> + for (int i = 0; i < destW; i++, x += xInc) {\n>>> + xLo[i] = floor(x);\n>>> + xf[i] = x - xLo[i];\n>>> + xHi[i] = xLo[i] < 15 ? xLo[i] + 1 : 15;\n>>> + xLo[i] = xLo[i] > 0 ? xLo[i] : 0;\n>>> }\n>>>\n>>> /* Now march over the output table generating the new values. */\n>>> - double y = -0.5, y_inc = 12.0 / (dest_h - 1);\n>>> - for (int j = 0; j < dest_h; j++, y += y_inc) {\n>>> - int y_lo = floor(y);\n>>> - double yf = y - y_lo;\n>>> - int y_hi = y_lo < 11 ? y_lo + 1 : 11;\n>>> - y_lo = y_lo > 0 ? y_lo : 0;\n>>> - double const *row_above = src[y_lo];\n>>> - double const *row_below = src[y_hi];\n>>> - for (int i = 0; i < dest_w; i++) {\n>>> - double above = row_above[x_lo[i]] * (1 - xf[i])\n>>> - + row_above[x_hi[i]] * xf[i];\n>>> - double below = row_below[x_lo[i]] * (1 - xf[i])\n>>> - + row_below[x_hi[i]] * xf[i];\n>>> + double y = -0.5, yInc = 12.0 / (destH - 1);\n>>> + for (int j = 0; j < destH; j++, y += yInc) {\n>>> + int yLo = floor(y);\n>>> + double yf = y - yLo;\n>>> + int yHi = yLo < 11 ? yLo + 1 : 11;\n>>> + yLo = yLo > 0 ? yLo : 0;\n>>> + double const *rowAbove = src[yLo];\n>>> + double const *rowBelow = src[yHi];\n>>> + for (int i = 0; i < destW; i++) {\n>>> + double above = rowAbove[xLo[i]] * (1 - xf[i])\n>>> + + rowAbove[xHi[i]] * xf[i];\n>>> + double below = rowBelow[xLo[i]] * (1 - xf[i])\n>>> + + rowBelow[xHi[i]] * xf[i];\n>>> int result = floor(1024 * (above * (1 - yf) + below * yf) + .5);\n>>> *(dest++) = result > 16383 ? 16383 : result; /* want u4.10 */\n>>> }\n>>> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n>>> index 35dbe0fb..8d40b0ed 100644\n>>> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n>>> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n>>> @@ -1012,7 +1012,7 @@ int RPiCameraData::configureIPA()\n>>>\n>>> /* Allocate the lens shading table via dmaHeap and pass to the IPA. */\n>>> if (!lsTable_.isValid()) {\n>>> - lsTable_ = dmaHeap_.alloc(\"ls_grid\", MAX_LS_GRID_SIZE);\n>>> + lsTable_ = dmaHeap_.alloc(\"ls_grid\", RPi::MaxLsGridSize);\n>>\n>> If we had an IPA namespace, it would be nice to see this as:\n>>\n>> lsTable_ = dmaHeap_.alloc(\"ls_grid\", ipa::RPi::MaxLsGridSize);\n>>\n>> As that would make it really clear 'where' this MaxLsGridSize value was\n>> coming from...\n>>\n>> Anyway, all my comments above are optional discussion points.\n>>\n>> For the patch:\n>>\n>> Reviewed-by: Kieran Bingham \n>>\n>>> if (!lsTable_.isValid())\n>>> return -ENOMEM;\n>>>\n>>>\n>>\n>> --\n>> Regards\n>> --\n>> Kieran\n>> _______________________________________________\n>> libcamera-devel mailing list\n>> libcamera-devel@lists.libcamera.org\n>> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 1ABE1C3B5C\n\tfor ;\n\tWed, 23 Sep 2020 19:53:21 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 756E962FE3;\n\tWed, 23 Sep 2020 21:53:20 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 01A1D60576\n\tfor ;\n\tWed, 23 Sep 2020 21:53:18 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B572B555;\n\tWed, 23 Sep 2020 21:53:17 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"PDg1P9ax\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1600890798;\n\tbh=EIig2wc61O1boQP0AFlG6H9poMqYab66OCyZdgP6NW8=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=PDg1P9axM3Lq9lN8oxw9djzJH4ZgyDYrdu1as2ufi7fn6J6ZjeMjZfodUPPw7eM+H\n\tA8LgsWZXquN3VdwYsxPLc+ydlFuRPVYOemp0l0Dr6q0zbIoN46LlU7SyywdosOSP2n\n\ti/P9DdHiYlysn/OJfg412tXVjrcsH6/k+X4FWxmU=","To":"David Plowman ","References":"<20200922095018.68434-1-naush@raspberrypi.com>\n\t<20200922095018.68434-5-naush@raspberrypi.com>\n\t\n\t","From":"Kieran Bingham ","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"","Date":"Wed, 23 Sep 2020 20:53:13 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.10.0","MIME-Version":"1.0","In-Reply-To":"","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH 4/4] ipa: raspberrypi: Tidy up\n\tvariable names to be consistent","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Reply-To":"kieran.bingham@ideasonboard.com","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":12713,"web_url":"https://patchwork.libcamera.org/comment/12713/","msgid":"","date":"2020-09-24T08:34:09","subject":"Re: [libcamera-devel] [PATCH 4/4] ipa: raspberrypi: Tidy up\n\tvariable names to be consistent","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi David,\n\nThank you for the comments.\n\nOn Wed, 23 Sep 2020 at 17:31, David Plowman\n wrote:\n>\n> Hi Naush\n>\n> Thanks for doing all this tidying! One little nit-pick...\n>\n> On Wed, 23 Sep 2020 at 12:18, Kieran Bingham\n> wrote:\n> >\n> > Hi Naush,\n> >\n> > On 22/09/2020 10:50, Naushir Patuck wrote:\n> > > Change variable names to camel case to be consistent with the rest of\n> > > the source files. Remove #define consts and replace with constexpr.\n> > >\n> >\n> > Sounds good to me...\n> >\n> >\n> > > There are no functional changes in this commit.\n> > >\n> > > Signed-off-by: Naushir Patuck \n> > > ---\n> > > include/libcamera/ipa/raspberrypi.h | 2 +-\n> > > src/ipa/raspberrypi/raspberrypi.cpp | 178 +++++++++---------\n> > > .../pipeline/raspberrypi/raspberrypi.cpp | 2 +-\n> > > 3 files changed, 91 insertions(+), 91 deletions(-)\n> > >\n> > > diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h\n> > > index c9d4aa81..b3041591 100644\n> > > --- a/include/libcamera/ipa/raspberrypi.h\n> > > +++ b/include/libcamera/ipa/raspberrypi.h\n> > > @@ -41,7 +41,7 @@ enum BufferMask {\n> > > };\n> > >\n> > > /* Size of the LS grid allocation. */\n> > > -#define MAX_LS_GRID_SIZE (32 << 10)\n> > > +static constexpr unsigned int MaxLsGridSize = 32 << 10;\n> >\n> > I guess the LS could stay capitalised (MaxLSGridSize), not sure that it\n> > matters, so up to you.\n> >\n> > > /* List of controls handled by the Raspberry Pi IPA */\n> > > static const ControlInfoMap Controls = {\n> > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > index 0c0dc743..c240eae8 100644\n> > > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > @@ -55,8 +55,8 @@\n> > > namespace libcamera {\n> > >\n> > > /* Configure the sensor with these values initially. */\n> > > -#define DEFAULT_ANALOGUE_GAIN 1.0\n> > > -#define DEFAULT_EXPOSURE_TIME 20000\n> > > +constexpr unsigned int DefaultAnalogueGain = 1.0;\n> > > +constexpr unsigned int DefaultExposureTime = 20000;\n> >\n> > Oh nice, This really highlights to me the benefit of constexpr.\n> > Now the values have types!\n>\n> Should DefaultAnalogueGain be a double? Of course it gets cast to a\n> double when it gets copied into agcStatus.analogue_gain so it probably\n> contrives to make no difference, but a double might be clearer,\n> especially if anyone ever changed it.\n\nIndeed, this should be a double! Will fix it in the next version.\n\nRegards,\nNaush\n\n>\n> Thanks!\n> David\n>\n> >\n> > >\n> > > LOG_DEFINE_CATEGORY(IPARPI)\n> > >\n> > > @@ -65,7 +65,7 @@ class IPARPi : public IPAInterface\n> > > public:\n> > > IPARPi()\n> > > : lastMode_({}), controller_(), controllerInit_(false),\n> > > - frame_count_(0), check_count_(0), mistrust_count_(0),\n> > > + frameCount_(0), checkCount_(0), mistrustCount_(0),\n> > > lsTable_(nullptr)\n> > > {\n> > > }\n> > > @@ -73,7 +73,7 @@ public:\n> > > ~IPARPi()\n> > > {\n> > > if (lsTable_)\n> > > - munmap(lsTable_, MAX_LS_GRID_SIZE);\n> > > + munmap(lsTable_, RPi::MaxLsGridSize);\n> > > }\n> > >\n> > > int init(const IPASettings &settings) override;\n> > > @@ -108,13 +108,13 @@ private:\n> > > void applySharpen(const struct SharpenStatus *sharpenStatus, ControlList &ctrls);\n> > > void applyDPC(const struct DpcStatus *dpcStatus, ControlList &ctrls);\n> > > void applyLS(const struct AlscStatus *lsStatus, ControlList &ctrls);\n> > > - void resampleTable(uint16_t dest[], double const src[12][16], int dest_w, int dest_h);\n> > > + void resampleTable(uint16_t dest[], double const src[12][16], int destW, int destH);\n> > >\n> > > std::map buffers_;\n> > > std::map buffersMemory_;\n> > >\n> > > - ControlInfoMap unicam_ctrls_;\n> > > - ControlInfoMap isp_ctrls_;\n> > > + ControlInfoMap unicamCtrls_;\n> > > + ControlInfoMap ispCtrls_;\n> > > ControlList libcameraMetadata_;\n> > >\n> > > /* IPA configuration. */\n> > > @@ -134,11 +134,11 @@ private:\n> > > * We count frames to decide if the frame must be hidden (e.g. from\n> > > * display) or mistrusted (i.e. not given to the control algos).\n> > > */\n> > > - uint64_t frame_count_;\n> > > + uint64_t frameCount_;\n> >\n> > If we're doing clean up - I always prefer to see a newline before a\n> > comment line.\n> >\n> > To me it makes the commented block clearer - but I don't know if there's\n> > a 'rule' on it.\n> >\n> >\n> > > /* For checking the sequencing of Prepare/Process calls. */\n> > > - uint64_t check_count_;\n> > > + uint64_t checkCount_;\n> >\n> > i.e. here too.\n> >\n> > > /* How many frames we should avoid running control algos on. */\n> > > - unsigned int mistrust_count_;\n> > > + unsigned int mistrustCount_;\n> >\n> > and so on.\n> >\n> > > /* LS table allocation passed in from the pipeline handler. */\n> > > FileDescriptor lsTableHandle_;\n> > > void *lsTable_;\n> > > @@ -199,8 +199,8 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> > >\n> > > result->operation = 0;\n> > >\n> > > - unicam_ctrls_ = entityControls.at(0);\n> > > - isp_ctrls_ = entityControls.at(1);\n> > > + unicamCtrls_ = entityControls.at(0);\n> > > + ispCtrls_ = entityControls.at(1);\n> > > /* Setup a metadata ControlList to output metadata. */\n> > > libcameraMetadata_ = ControlList(controls::controls);\n> > >\n> > > @@ -238,18 +238,18 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> > > *\"mistrusted\", which depends on whether this is a startup from cold,\n> > > * or merely a mode switch in a running system.\n> > > */\n> > > - frame_count_ = 0;\n> > > - check_count_ = 0;\n> > > - unsigned int drop_frame = 0;\n> > > + frameCount_ = 0;\n> > > + checkCount_ = 0;\n> > > + unsigned int dropFrame = 0;\n> > > if (controllerInit_) {\n> > > - drop_frame = helper_->HideFramesModeSwitch();\n> > > - mistrust_count_ = helper_->MistrustFramesModeSwitch();\n> > > + dropFrame = helper_->HideFramesModeSwitch();\n> > > + mistrustCount_ = helper_->MistrustFramesModeSwitch();\n> > > } else {\n> > > - drop_frame = helper_->HideFramesStartup();\n> > > - mistrust_count_ = helper_->MistrustFramesStartup();\n> > > + dropFrame = helper_->HideFramesStartup();\n> > > + mistrustCount_ = helper_->MistrustFramesStartup();\n> > > }\n> > >\n> > > - result->data.push_back(drop_frame);\n> > > + result->data.push_back(dropFrame);\n> > > result->operation |= RPi::IPA_CONFIG_DROP_FRAMES;\n> > >\n> > > struct AgcStatus agcStatus;\n> > > @@ -264,8 +264,8 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> > > controllerInit_ = true;\n> > >\n> > > /* Supply initial values for gain and exposure. */\n> > > - agcStatus.shutter_time = DEFAULT_EXPOSURE_TIME;\n> > > - agcStatus.analogue_gain = DEFAULT_ANALOGUE_GAIN;\n> > > + agcStatus.shutter_time = DefaultExposureTime;\n> > > + agcStatus.analogue_gain = DefaultAnalogueGain;\n> > > }\n> > >\n> > > RPiController::Metadata metadata;\n> > > @@ -274,7 +274,7 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> > > /* SwitchMode may supply updated exposure/gain values to use. */\n> > > metadata.Get(\"agc.status\", agcStatus);\n> > > if (agcStatus.shutter_time != 0.0 && agcStatus.analogue_gain != 0.0) {\n> > > - ControlList ctrls(unicam_ctrls_);\n> > > + ControlList ctrls(unicamCtrls_);\n> > > applyAGC(&agcStatus, ctrls);\n> > > result->controls.push_back(ctrls);\n> > >\n> > > @@ -287,14 +287,14 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> > > if (ipaConfig.operation & RPi::IPA_CONFIG_LS_TABLE) {\n> > > /* Remove any previous table, if there was one. */\n> > > if (lsTable_) {\n> > > - munmap(lsTable_, MAX_LS_GRID_SIZE);\n> > > + munmap(lsTable_, RPi::MaxLsGridSize);\n> > > lsTable_ = nullptr;\n> > > }\n> > >\n> > > /* Map the LS table buffer into user space. */\n> > > lsTableHandle_ = FileDescriptor(ipaConfig.data[0]);\n> > > if (lsTableHandle_.isValid()) {\n> > > - lsTable_ = mmap(nullptr, MAX_LS_GRID_SIZE, PROT_READ | PROT_WRITE,\n> > > + lsTable_ = mmap(nullptr, RPi::MaxLsGridSize, PROT_READ | PROT_WRITE,\n> > > MAP_SHARED, lsTableHandle_.fd(), 0);\n> > >\n> > > if (lsTable_ == MAP_FAILED) {\n> > > @@ -343,9 +343,9 @@ void IPARPi::processEvent(const IPAOperationData &event)\n> > > case RPi::IPA_EVENT_SIGNAL_STAT_READY: {\n> > > unsigned int bufferId = event.data[0];\n> > >\n> > > - if (++check_count_ != frame_count_) /* assert here? */\n> > > + if (++checkCount_ != frameCount_) /* assert here? */\n> > > LOG(IPARPI, Error) << \"WARNING: Prepare/Process mismatch!!!\";\n> > > - if (frame_count_ > mistrust_count_)\n> > > + if (frameCount_ > mistrustCount_)\n> > > processStats(bufferId);\n> > >\n> > > reportMetadata();\n> > > @@ -368,7 +368,7 @@ void IPARPi::processEvent(const IPAOperationData &event)\n> > > * they are \"unreliable\".\n> > > */\n> > > prepareISP(embeddedbufferId);\n> > > - frame_count_++;\n> > > + frameCount_++;\n> > >\n> > > /* Ready to push the input buffer into the ISP. */\n> > > IPAOperationData op;\n> > > @@ -713,7 +713,7 @@ void IPARPi::prepareISP(unsigned int bufferId)\n> > > returnEmbeddedBuffer(bufferId);\n> > >\n> > > if (success) {\n> > > - ControlList ctrls(isp_ctrls_);\n> > > + ControlList ctrls(ispCtrls_);\n> > >\n> > > rpiMetadata_.Clear();\n> > > rpiMetadata_.Set(\"device.status\", deviceStatus);\n> > > @@ -785,19 +785,19 @@ bool IPARPi::parseEmbeddedData(unsigned int bufferId, struct DeviceStatus &devic\n> > > if (status != RPiController::MdParser::Status::OK) {\n> > > LOG(IPARPI, Error) << \"Embedded Buffer parsing failed, error \" << status;\n> > > } else {\n> > > - uint32_t exposure_lines, gain_code;\n> > > - if (helper_->Parser().GetExposureLines(exposure_lines) != RPiController::MdParser::Status::OK) {\n> > > + uint32_t exposureLines, gainCode;\n> > > + if (helper_->Parser().GetExposureLines(exposureLines) != RPiController::MdParser::Status::OK) {\n> > > LOG(IPARPI, Error) << \"Exposure time failed\";\n> > > return false;\n> > > }\n> > >\n> > > - deviceStatus.shutter_speed = helper_->Exposure(exposure_lines);\n> > > - if (helper_->Parser().GetGainCode(gain_code) != RPiController::MdParser::Status::OK) {\n> > > + deviceStatus.shutter_speed = helper_->Exposure(exposureLines);\n> > > + if (helper_->Parser().GetGainCode(gainCode) != RPiController::MdParser::Status::OK) {\n> > > LOG(IPARPI, Error) << \"Gain failed\";\n> > > return false;\n> > > }\n> > >\n> > > - deviceStatus.analogue_gain = helper_->Gain(gain_code);\n> > > + deviceStatus.analogue_gain = helper_->Gain(gainCode);\n> > > LOG(IPARPI, Debug) << \"Metadata - Exposure : \"\n> > > << deviceStatus.shutter_speed << \" Gain : \"\n> > > << deviceStatus.analogue_gain;\n> > > @@ -820,7 +820,7 @@ void IPARPi::processStats(unsigned int bufferId)\n> > >\n> > > struct AgcStatus agcStatus;\n> > > if (rpiMetadata_.Get(\"agc.status\", agcStatus) == 0) {\n> > > - ControlList ctrls(unicam_ctrls_);\n> > > + ControlList ctrls(unicamCtrls_);\n> > > applyAGC(&agcStatus, ctrls);\n> > >\n> > > IPAOperationData op;\n> > > @@ -832,14 +832,14 @@ void IPARPi::processStats(unsigned int bufferId)\n> > >\n> > > void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls)\n> > > {\n> > > - const auto gainR = isp_ctrls_.find(V4L2_CID_RED_BALANCE);\n> > > - if (gainR == isp_ctrls_.end()) {\n> > > + const auto gainR = ispCtrls_.find(V4L2_CID_RED_BALANCE);\n> > > + if (gainR == ispCtrls_.end()) {\n> > > LOG(IPARPI, Error) << \"Can't find red gain control\";\n> > > return;\n> > > }\n> > >\n> > > - const auto gainB = isp_ctrls_.find(V4L2_CID_BLUE_BALANCE);\n> > > - if (gainB == isp_ctrls_.end()) {\n> > > + const auto gainB = ispCtrls_.find(V4L2_CID_BLUE_BALANCE);\n> > > + if (gainB == ispCtrls_.end()) {\n> > > LOG(IPARPI, Error) << \"Can't find blue gain control\";\n> > > return;\n> > > }\n> > > @@ -855,31 +855,31 @@ void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls)\n> > >\n> > > void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls)\n> > > {\n> > > - int32_t gain_code = helper_->GainCode(agcStatus->analogue_gain);\n> > > - int32_t exposure_lines = helper_->ExposureLines(agcStatus->shutter_time);\n> > > + int32_t gainCode = helper_->GainCode(agcStatus->analogue_gain);\n> > > + int32_t exposureLines = helper_->ExposureLines(agcStatus->shutter_time);\n> > >\n> > > - if (unicam_ctrls_.find(V4L2_CID_ANALOGUE_GAIN) == unicam_ctrls_.end()) {\n> > > + if (unicamCtrls_.find(V4L2_CID_ANALOGUE_GAIN) == unicamCtrls_.end()) {\n> > > LOG(IPARPI, Error) << \"Can't find analogue gain control\";\n> > > return;\n> > > }\n> > >\n> > > - if (unicam_ctrls_.find(V4L2_CID_EXPOSURE) == unicam_ctrls_.end()) {\n> > > + if (unicamCtrls_.find(V4L2_CID_EXPOSURE) == unicamCtrls_.end()) {\n> > > LOG(IPARPI, Error) << \"Can't find exposure control\";\n> > > return;\n> > > }\n> > >\n> > > LOG(IPARPI, Debug) << \"Applying AGC Exposure: \" << agcStatus->shutter_time\n> > > - << \" (Shutter lines: \" << exposure_lines << \") Gain: \"\n> > > + << \" (Shutter lines: \" << exposureLines << \") Gain: \"\n> > > << agcStatus->analogue_gain << \" (Gain Code: \"\n> > > - << gain_code << \")\";\n> > > + << gainCode << \")\";\n> > >\n> > > - ctrls.set(V4L2_CID_ANALOGUE_GAIN, gain_code);\n> > > - ctrls.set(V4L2_CID_EXPOSURE, exposure_lines);\n> > > + ctrls.set(V4L2_CID_ANALOGUE_GAIN, gainCode);\n> > > + ctrls.set(V4L2_CID_EXPOSURE, exposureLines);\n> > > }\n> > >\n> > > void IPARPi::applyDG(const struct AgcStatus *dgStatus, ControlList &ctrls)\n> > > {\n> > > - if (isp_ctrls_.find(V4L2_CID_DIGITAL_GAIN) == isp_ctrls_.end()) {\n> > > + if (ispCtrls_.find(V4L2_CID_DIGITAL_GAIN) == ispCtrls_.end()) {\n> > > LOG(IPARPI, Error) << \"Can't find digital gain control\";\n> > > return;\n> > > }\n> > > @@ -890,7 +890,7 @@ void IPARPi::applyDG(const struct AgcStatus *dgStatus, ControlList &ctrls)\n> > >\n> > > void IPARPi::applyCCM(const struct CcmStatus *ccmStatus, ControlList &ctrls)\n> > > {\n> > > - if (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_CC_MATRIX) == isp_ctrls_.end()) {\n> > > + if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_CC_MATRIX) == ispCtrls_.end()) {\n> > > LOG(IPARPI, Error) << \"Can't find CCM control\";\n> > > return;\n> > > }\n> > > @@ -911,7 +911,7 @@ void IPARPi::applyCCM(const struct CcmStatus *ccmStatus, ControlList &ctrls)\n> > >\n> > > void IPARPi::applyGamma(const struct ContrastStatus *contrastStatus, ControlList &ctrls)\n> > > {\n> > > - if (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_GAMMA) == isp_ctrls_.end()) {\n> > > + if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_GAMMA) == ispCtrls_.end()) {\n> > > LOG(IPARPI, Error) << \"Can't find Gamma control\";\n> > > return;\n> > > }\n> > > @@ -930,7 +930,7 @@ void IPARPi::applyGamma(const struct ContrastStatus *contrastStatus, ControlList\n> > >\n> > > void IPARPi::applyBlackLevel(const struct BlackLevelStatus *blackLevelStatus, ControlList &ctrls)\n> > > {\n> > > - if (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_BLACK_LEVEL) == isp_ctrls_.end()) {\n> > > + if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_BLACK_LEVEL) == ispCtrls_.end()) {\n> > > LOG(IPARPI, Error) << \"Can't find black level control\";\n> > > return;\n> > > }\n> > > @@ -948,7 +948,7 @@ void IPARPi::applyBlackLevel(const struct BlackLevelStatus *blackLevelStatus, Co\n> > >\n> > > void IPARPi::applyGEQ(const struct GeqStatus *geqStatus, ControlList &ctrls)\n> > > {\n> > > - if (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_GEQ) == isp_ctrls_.end()) {\n> > > + if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_GEQ) == ispCtrls_.end()) {\n> > > LOG(IPARPI, Error) << \"Can't find geq control\";\n> > > return;\n> > > }\n> > > @@ -966,7 +966,7 @@ void IPARPi::applyGEQ(const struct GeqStatus *geqStatus, ControlList &ctrls)\n> > >\n> > > void IPARPi::applyDenoise(const struct SdnStatus *denoiseStatus, ControlList &ctrls)\n> > > {\n> > > - if (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_DENOISE) == isp_ctrls_.end()) {\n> > > + if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_DENOISE) == ispCtrls_.end()) {\n> > > LOG(IPARPI, Error) << \"Can't find denoise control\";\n> > > return;\n> > > }\n> > > @@ -986,7 +986,7 @@ void IPARPi::applyDenoise(const struct SdnStatus *denoiseStatus, ControlList &ct\n> > >\n> > > void IPARPi::applySharpen(const struct SharpenStatus *sharpenStatus, ControlList &ctrls)\n> > > {\n> > > - if (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_SHARPEN) == isp_ctrls_.end()) {\n> > > + if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_SHARPEN) == ispCtrls_.end()) {\n> > > LOG(IPARPI, Error) << \"Can't find sharpen control\";\n> > > return;\n> > > }\n> > > @@ -1007,7 +1007,7 @@ void IPARPi::applySharpen(const struct SharpenStatus *sharpenStatus, ControlList\n> > >\n> > > void IPARPi::applyDPC(const struct DpcStatus *dpcStatus, ControlList &ctrls)\n> > > {\n> > > - if (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_DPC) == isp_ctrls_.end()) {\n> > > + if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_DPC) == ispCtrls_.end()) {\n> > > LOG(IPARPI, Error) << \"Can't find DPC control\";\n> > > return;\n> > > }\n> > > @@ -1023,7 +1023,7 @@ void IPARPi::applyDPC(const struct DpcStatus *dpcStatus, ControlList &ctrls)\n> > >\n> > > void IPARPi::applyLS(const struct AlscStatus *lsStatus, ControlList &ctrls)\n> > > {\n> > > - if (isp_ctrls_.find(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING) == isp_ctrls_.end()) {\n> > > + if (ispCtrls_.find(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING) == ispCtrls_.end()) {\n> > > LOG(IPARPI, Error) << \"Can't find LS control\";\n> > > return;\n> > > }\n> > > @@ -1032,18 +1032,18 @@ void IPARPi::applyLS(const struct AlscStatus *lsStatus, ControlList &ctrls)\n> > > * Program lens shading tables into pipeline.\n> > > * Choose smallest cell size that won't exceed 63x48 cells.\n> > > */\n> > > - const int cell_sizes[] = { 16, 32, 64, 128, 256 };\n> > > - unsigned int num_cells = ARRAY_SIZE(cell_sizes);\n> > > - unsigned int i, w, h, cell_size;\n> > > - for (i = 0; i < num_cells; i++) {\n> > > - cell_size = cell_sizes[i];\n> > > - w = (mode_.width + cell_size - 1) / cell_size;\n> > > - h = (mode_.height + cell_size - 1) / cell_size;\n> > > + const int cellSizes[] = { 16, 32, 64, 128, 256 };\n> > > + unsigned int numCells = ARRAY_SIZE(cellSizes);\n> > > + unsigned int i, w, h, cellSize;\n> > > + for (i = 0; i < numCells; i++) {\n> > > + cellSize = cellSizes[i];\n> > > + w = (mode_.width + cellSize - 1) / cellSize;\n> > > + h = (mode_.height + cellSize - 1) / cellSize;\n> > > if (w < 64 && h <= 48)\n> > > break;\n> > > }\n> > >\n> > > - if (i == num_cells) {\n> > > + if (i == numCells) {\n> > > LOG(IPARPI, Error) << \"Cannot find cell size\";\n> > > return;\n> > > }\n> > > @@ -1052,7 +1052,7 @@ void IPARPi::applyLS(const struct AlscStatus *lsStatus, ControlList &ctrls)\n> > > w++, h++;\n> > > bcm2835_isp_lens_shading ls = {\n> > > .enabled = 1,\n> > > - .grid_cell_size = cell_size,\n> > > + .grid_cell_size = cellSize,\n> > > .grid_width = w,\n> > > .grid_stride = w,\n> > > .grid_height = h,\n> > > @@ -1062,7 +1062,7 @@ void IPARPi::applyLS(const struct AlscStatus *lsStatus, ControlList &ctrls)\n> > > .gain_format = GAIN_FORMAT_U4P10\n> > > };\n> > >\n> > > - if (!lsTable_ || w * h * 4 * sizeof(uint16_t) > MAX_LS_GRID_SIZE) {\n> > > + if (!lsTable_ || w * h * 4 * sizeof(uint16_t) > RPi::MaxLsGridSize) {\n> > > LOG(IPARPI, Error) << \"Do not have a correctly allocate lens shading table!\";\n> > > return;\n> > > }\n> > > @@ -1083,41 +1083,41 @@ void IPARPi::applyLS(const struct AlscStatus *lsStatus, ControlList &ctrls)\n> > > }\n> > >\n> > > /*\n> > > - * Resamples a 16x12 table with central sampling to dest_w x dest_h with corner\n> > > + * Resamples a 16x12 table with central sampling to destW x destH with corner\n> > > * sampling.\n> > > */\n> > > void IPARPi::resampleTable(uint16_t dest[], double const src[12][16],\n> > > - int dest_w, int dest_h)\n> > > + int destW, int destH)\n> > > {\n> > > /*\n> > > * Precalculate and cache the x sampling locations and phases to\n> > > * save recomputing them on every row.\n> > > */\n> > > - assert(dest_w > 1 && dest_h > 1 && dest_w <= 64);\n> > > - int x_lo[64], x_hi[64];\n> > > + assert(destW > 1 && destH > 1 && destW <= 64);\n> > > + int xLo[64], xHi[64];\n> > > double xf[64];\n> > > - double x = -0.5, x_inc = 16.0 / (dest_w - 1);\n> > > - for (int i = 0; i < dest_w; i++, x += x_inc) {\n> > > - x_lo[i] = floor(x);\n> > > - xf[i] = x - x_lo[i];\n> > > - x_hi[i] = x_lo[i] < 15 ? x_lo[i] + 1 : 15;\n> > > - x_lo[i] = x_lo[i] > 0 ? x_lo[i] : 0;\n> > > + double x = -0.5, xInc = 16.0 / (destW - 1);\n> > > + for (int i = 0; i < destW; i++, x += xInc) {\n> > > + xLo[i] = floor(x);\n> > > + xf[i] = x - xLo[i];\n> > > + xHi[i] = xLo[i] < 15 ? xLo[i] + 1 : 15;\n> > > + xLo[i] = xLo[i] > 0 ? xLo[i] : 0;\n> > > }\n> > >\n> > > /* Now march over the output table generating the new values. */\n> > > - double y = -0.5, y_inc = 12.0 / (dest_h - 1);\n> > > - for (int j = 0; j < dest_h; j++, y += y_inc) {\n> > > - int y_lo = floor(y);\n> > > - double yf = y - y_lo;\n> > > - int y_hi = y_lo < 11 ? y_lo + 1 : 11;\n> > > - y_lo = y_lo > 0 ? y_lo : 0;\n> > > - double const *row_above = src[y_lo];\n> > > - double const *row_below = src[y_hi];\n> > > - for (int i = 0; i < dest_w; i++) {\n> > > - double above = row_above[x_lo[i]] * (1 - xf[i])\n> > > - + row_above[x_hi[i]] * xf[i];\n> > > - double below = row_below[x_lo[i]] * (1 - xf[i])\n> > > - + row_below[x_hi[i]] * xf[i];\n> > > + double y = -0.5, yInc = 12.0 / (destH - 1);\n> > > + for (int j = 0; j < destH; j++, y += yInc) {\n> > > + int yLo = floor(y);\n> > > + double yf = y - yLo;\n> > > + int yHi = yLo < 11 ? yLo + 1 : 11;\n> > > + yLo = yLo > 0 ? yLo : 0;\n> > > + double const *rowAbove = src[yLo];\n> > > + double const *rowBelow = src[yHi];\n> > > + for (int i = 0; i < destW; i++) {\n> > > + double above = rowAbove[xLo[i]] * (1 - xf[i])\n> > > + + rowAbove[xHi[i]] * xf[i];\n> > > + double below = rowBelow[xLo[i]] * (1 - xf[i])\n> > > + + rowBelow[xHi[i]] * xf[i];\n> > > int result = floor(1024 * (above * (1 - yf) + below * yf) + .5);\n> > > *(dest++) = result > 16383 ? 16383 : result; /* want u4.10 */\n> > > }\n> > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > index 35dbe0fb..8d40b0ed 100644\n> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > @@ -1012,7 +1012,7 @@ int RPiCameraData::configureIPA()\n> > >\n> > > /* Allocate the lens shading table via dmaHeap and pass to the IPA. */\n> > > if (!lsTable_.isValid()) {\n> > > - lsTable_ = dmaHeap_.alloc(\"ls_grid\", MAX_LS_GRID_SIZE);\n> > > + lsTable_ = dmaHeap_.alloc(\"ls_grid\", RPi::MaxLsGridSize);\n> >\n> > If we had an IPA namespace, it would be nice to see this as:\n> >\n> > lsTable_ = dmaHeap_.alloc(\"ls_grid\", ipa::RPi::MaxLsGridSize);\n> >\n> > As that would make it really clear 'where' this MaxLsGridSize value was\n> > coming from...\n> >\n> > Anyway, all my comments above are optional discussion points.\n> >\n> > For the patch:\n> >\n> > Reviewed-by: Kieran Bingham \n> >\n> > > if (!lsTable_.isValid())\n> > > return -ENOMEM;\n> > >\n> > >\n> >\n> > --\n> > Regards\n> > --\n> > Kieran\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id CA695C3B5B\n\tfor ;\n\tThu, 24 Sep 2020 08:34:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4683562FDE;\n\tThu, 24 Sep 2020 10:34:28 +0200 (CEST)","from mail-lf1-x144.google.com (mail-lf1-x144.google.com\n\t[IPv6:2a00:1450:4864:20::144])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7E28B60362\n\tfor ;\n\tThu, 24 Sep 2020 10:34:26 +0200 (CEST)","by mail-lf1-x144.google.com with SMTP id b12so2925528lfp.9\n\tfor ;\n\tThu, 24 Sep 2020 01:34:26 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"tE6oaLWm\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=qoJJsfkVuFdCWgn/NELH6Kf0fiHW33LKCP6VtCKKguw=;\n\tb=tE6oaLWmEpRigPFcuCS6GKMvdc8s8I61T4MXLHwH74Xpq+hXXws7gKaYQgxnOSz3TS\n\tqTcfoQRJ93+p0IixonLHISjQK4TT55Exs6uM4Q+awr6FwTOgMEJm+/MOse8k3AXmJoQQ\n\twyruXkbfvG814aorjcbE3gxm0kIMMzgsgaeHiQFZpWhkr+0a72fp02yLEWGuq6LVaNWI\n\t1+CovA3x7L8MV4/82yaNDajFSvBTbK1+zEIZtUiW5nkTe2qWo+BRwbSDi6yHTHOrWxMW\n\tj2EmGOc7d7WG103Zs6L0kdW83i4xs8gam5HT4gQDYA0YOj1U0LeFvHD1x52MRXHTZ4Qq\n\tuBsQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=qoJJsfkVuFdCWgn/NELH6Kf0fiHW33LKCP6VtCKKguw=;\n\tb=CLc2M92LMq8AFWr6n9rgKeL2ZAEA/kQztmYY9pjGv0IcXndYPxtNC5Hyry8ICOqgpz\n\tvNsqHfvgil2GyN8RbG1TFtN7qC9/uf94ZP6fKSka4Dcj+GcO4ebEkck70I1B1MKrWMfH\n\t/xCQqI+sCQMefQeEkHUnI/5xrENCsVAPzPwculROTfr4kYypQv/RXZBgCT+6gZ5cT2Aw\n\tqMPeRpsAgXcmQ96498v10nx7T2a7Enf8HksG0EuthXK0CdbVuVhXJA9iiHc/SkwF4GHq\n\tdsE/vf3NCp1n4j4MBj8WPiWQaFflWKdjSlHRIYmMA4dLZZoTY2QHl9dAvQvmmLwy+THI\n\tg0cA==","X-Gm-Message-State":"AOAM533N67LA/2MsZHNH9oTDwtbSG9ANXoYQkm+iHVDeqe2E2XJ+2u0R\n\tmJ0Lc9C/q803eqyosdDIlOn/yOzdr7c4mshruWFO1PTHrGQ6ug==","X-Google-Smtp-Source":"ABdhPJxK4I8GVNeQNugV2zxGeLDkxVcAxsJ4nyi9l6JkSsj7gb9gwLQOfCAKHzdxj0gEcC35nNmgHklaJGIdnc79CQU=","X-Received":"by 2002:a19:8542:: with SMTP id\n\th63mr1131437lfd.177.1600936465625; \n\tThu, 24 Sep 2020 01:34:25 -0700 (PDT)","MIME-Version":"1.0","References":"<20200922095018.68434-1-naush@raspberrypi.com>\n\t<20200922095018.68434-5-naush@raspberrypi.com>\n\t\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Thu, 24 Sep 2020 09:34:09 +0100","Message-ID":"","To":"David Plowman ","Subject":"Re: [libcamera-devel] [PATCH 4/4] ipa: raspberrypi: Tidy up\n\tvariable names to be consistent","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":12714,"web_url":"https://patchwork.libcamera.org/comment/12714/","msgid":"","date":"2020-09-24T08:37:19","subject":"Re: [libcamera-devel] [PATCH 4/4] ipa: raspberrypi: Tidy up\n\tvariable names to be consistent","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Kieran and Jacopo,\n\nThank you both for the review comments.\n\nOn Wed, 23 Sep 2020 at 12:45, Kieran Bingham\n wrote:\n>\n> Hi Jacopo,\n>\n> On 23/09/2020 12:39, Jacopo Mondi wrote:\n> > /me dives into bikeshedding\n>\n> \n>\n> >>> @@ -134,11 +134,11 @@ private:\n> >>> * We count frames to decide if the frame must be hidden (e.g. from\n> >>> * display) or mistrusted (i.e. not given to the control algos).\n> >>> */\n> >>> - uint64_t frame_count_;\n> >>> + uint64_t frameCount_;\n> >>\n> >> If we're doing clean up - I always prefer to see a newline before a\n> >> comment line.\n>\n> Note here I state 'before a comment'\n>\n> >> To me it makes the commented block clearer - but I don't know if there's\n> >> a 'rule' on it.\n> >>\n> >\n> > I'm sorry but\n> >\n> > /*\n> > * A comment describing a variable is better places as close\n> > * as possible near to that variable, isn't it ?\n> > */\n> > int whatAFancyVariable;\n> >\n> > /* Or do you think this variable is not fancy and an empty line is better? */\n> >\n>\n> Here you seem to be implying 'after' a comment...\n>\n> > int notAFancyVariable;\n> >\n> > Ok, enough useless discussions from me :)\n> >\n>\n> I ... 'think' you mis-interpreted what I said, or I was not clear. I was\n> probably not clear ;-)\n>\n> I think that this:\n>\n> /* A variable group. */\n> bool variable;\n> int quantity;\n>\n> /*\n> * We count frames to decide if the frame must be hidden (e.g.\n> * from display) or mistrusted (i.e. not given to the control\n> * algos).\n> */\n> uint64_t frameCount_;\n>\n> /* For checking the sequencing of Prepare/Process calls. */\n> uint64_t checkCount_;\n>\n> /* How many frames we should avoid running control algos on. */\n> unsigned int mistrustCount_;\n>\n> is better than this:\n>\n>\n> /* A variable group */\n> bool variable;\n> int quanty;\n> /*\n> * We count frames to decide if the frame must be hidden (e.g.\n> * from display) or mistrusted (i.e. not given to the control\n> * algos).\n> */\n> uint64_t frameCount_;\n> /* For checking the sequencing of Prepare/Process calls. */\n> uint64_t checkCount_;\n> /* How many frames we should avoid running control algos on. */\n> unsigned int mistrustCount_;\n\nYes, I agree the latter block is less readable than the former. I\nwill update this patch with some extra whitespace where needed.\n\nRegards,\nNaush\n\n\n>\n> Maybe one day I'll finish my comment formatter in the checkstyle script ;-)\n>\n>\n> >>\n> >>> /* For checking the sequencing of Prepare/Process calls. */\n> >>> - uint64_t check_count_;\n> >>> + uint64_t checkCount_;\n> >>\n> >> i.e. here too.\n> >>\n> >>> /* How many frames we should avoid running control algos on. */\n> >>> - unsigned int mistrust_count_;\n> >>> + unsigned int mistrustCount_;\n> >>\n> >> and so on.\n> >>\n> >>> /* LS table allocation passed in from the pipeline handler. */\n> >>> FileDescriptor lsTableHandle_;\n> >>> void *lsTable_;\n>\n> \n> --\n> Regards\n> --\n> Kieran","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id BF96CC3B5C\n\tfor ;\n\tThu, 24 Sep 2020 08:37:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3804662FE6;\n\tThu, 24 Sep 2020 10:37:37 +0200 (CEST)","from mail-lj1-x242.google.com (mail-lj1-x242.google.com\n\t[IPv6:2a00:1450:4864:20::242])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0935160362\n\tfor ;\n\tThu, 24 Sep 2020 10:37:36 +0200 (CEST)","by mail-lj1-x242.google.com with SMTP id u21so2009541ljl.6\n\tfor ;\n\tThu, 24 Sep 2020 01:37:35 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"T4nBK+8b\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=Lt4pBq6L0VsfBWJXk82OEV7cZiZJl2+JqUrToZma0LA=;\n\tb=T4nBK+8bi16Y2wAucE/FqVJ0a2etKKoPPgfnw8qDUQaH+oBeP8j3AQpVz3Xzrrxk1Y\n\tS6D95ehf0dlWBSJe3k4b6Q8u+3UrOJQ3thXTL7xLL+VcjPLf8dv71rmXP9XxlOB/zGM/\n\tWOks5XxQhwwzqv6UQiRHmeYI253HjvnJ3LiFXHhgrTmoYYJqOSpC8OwKSNocmuYUZ8fG\n\tTvxeSDgzxVOWsRwY8zElLwb/keNy6eQAgeizqRN2RtYCkc7gOVe89KtEoHxUjvqDd9PR\n\tO/Nhv+FDZGKAqnTBUFa++w7vPx3tZxo1Nvh7RT6+WEWzsVZ/FTLcBMrxGUpdrQgDyYj3\n\tMhKg==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=Lt4pBq6L0VsfBWJXk82OEV7cZiZJl2+JqUrToZma0LA=;\n\tb=j4nWvJVlm83+oc4v/l01gN3h1GZH46Yir8COQlEgo0uQcP+nytNmKT1cD/mddc2tQo\n\tPhdYk4H+D7SD3VmM2t83vDrWirsaNUSgtBMURo9HIf0En1+3snoWA9FHkOuQ1D4RF/MV\n\tsAHhO3Bl36gbEyB3j3uTb1YPA8VPCXBB710L1b6yprEzooqJwyVx5V3HuQGP02tbjiAB\n\t6mizcYlSjRSvsHFkPthwQrf84/4WKis/3NjRh8KRrLmHX4KePqidK/ivh1oA3QioKCRv\n\tTwy1BE2+AmxXSpkO96aiOHhUx7XpVj1S1nXkca7KEBYfYfJBI9WBtfuZHEenXjWAeUno\n\tqVpg==","X-Gm-Message-State":"AOAM533WTCI80ahheVyw8LfeoIrw8/0fJ5qJnxwPGSgZ2qQLSYPdtkhd\n\tXkjdT0sXdZ2VY22ry251pXp3uMT+H0hkr9Zw89tiHg==","X-Google-Smtp-Source":"ABdhPJyS15RDERpN3SFwKTvuXskq3/q07npRmTj4q88UVExFjD+c7c5B74WYXk3srjp8FgdqA9j4zqO9GzWdiu0phMs=","X-Received":"by 2002:a2e:9b97:: with SMTP id z23mr1175357lji.1.1600936655365; \n\tThu, 24 Sep 2020 01:37:35 -0700 (PDT)","MIME-Version":"1.0","References":"<20200922095018.68434-1-naush@raspberrypi.com>\n\t<20200922095018.68434-5-naush@raspberrypi.com>\n\t\n\t<20200923113913.a6rmjylpqwbumo2s@uno.localdomain>\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Thu, 24 Sep 2020 09:37:19 +0100","Message-ID":"","To":"Kieran Bingham ","Subject":"Re: [libcamera-devel] [PATCH 4/4] ipa: raspberrypi: Tidy up\n\tvariable names to be consistent","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]