[{"id":24169,"web_url":"https://patchwork.libcamera.org/comment/24169/","msgid":"","date":"2022-07-27T02:02:15","subject":"Re: [libcamera-devel] [PATCH 16/17] ipa: raspberrypi: Remove\n\t#define constants","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nThank you for the patch.\n\nOn Tue, Jul 26, 2022 at 01:45:48PM +0100, Naushir Patuck via libcamera-devel wrote:\n> Replace all #define constant values with equivalent constexpr definitions.\n> As a drive-by, remove the CAMERA_MODE_NAME_LEN constant as it is unused.\n> \n> Signed-off-by: Naushir Patuck \n\nReviewed-by: Laurent Pinchart \n\n> ---\n> src/ipa/raspberrypi/controller/alsc_status.h | 10 ++++-----\n> src/ipa/raspberrypi/controller/camera_mode.h | 2 --\n> .../raspberrypi/controller/contrast_status.h | 4 ++--\n> src/ipa/raspberrypi/controller/rpi/agc.cpp | 22 +++++++++----------\n> src/ipa/raspberrypi/controller/rpi/agc.h | 4 ++--\n> src/ipa/raspberrypi/controller/rpi/alsc.cpp | 4 ++--\n> src/ipa/raspberrypi/controller/rpi/alsc.h | 22 +++++++++----------\n> src/ipa/raspberrypi/controller/rpi/awb.cpp | 8 +++----\n> .../raspberrypi/controller/rpi/contrast.cpp | 6 ++---\n> src/ipa/raspberrypi/raspberrypi.cpp | 2 +-\n> 10 files changed, 41 insertions(+), 43 deletions(-)\n> \n> diff --git a/src/ipa/raspberrypi/controller/alsc_status.h b/src/ipa/raspberrypi/controller/alsc_status.h\n> index 498880daf2d1..e5aa7e37c330 100644\n> --- a/src/ipa/raspberrypi/controller/alsc_status.h\n> +++ b/src/ipa/raspberrypi/controller/alsc_status.h\n> @@ -11,11 +11,11 @@\n> * \"alsc.status\" metadata.\n> */\n> \n> -#define ALSC_CELLS_X 16\n> -#define ALSC_CELLS_Y 12\n> +constexpr unsigned int AlscCellsX = 16;\n> +constexpr unsigned int AlscCellsY = 12;\n> \n> struct AlscStatus {\n> -\tdouble r[ALSC_CELLS_Y][ALSC_CELLS_X];\n> -\tdouble g[ALSC_CELLS_Y][ALSC_CELLS_X];\n> -\tdouble b[ALSC_CELLS_Y][ALSC_CELLS_X];\n> +\tdouble r[AlscCellsY][AlscCellsX];\n> +\tdouble g[AlscCellsY][AlscCellsX];\n> +\tdouble b[AlscCellsY][AlscCellsX];\n> };\n> diff --git a/src/ipa/raspberrypi/controller/camera_mode.h b/src/ipa/raspberrypi/controller/camera_mode.h\n> index 0ac6c07fb7bf..a6ccf8c1c600 100644\n> --- a/src/ipa/raspberrypi/controller/camera_mode.h\n> +++ b/src/ipa/raspberrypi/controller/camera_mode.h\n> @@ -16,8 +16,6 @@\n> * including binning, scaling, cropping etc.\n> */\n> \n> -#define CAMERA_MODE_NAME_LEN 32\n> -\n> struct CameraMode {\n> \t/* bit depth of the raw camera output */\n> \tuint32_t bitdepth;\n> diff --git a/src/ipa/raspberrypi/controller/contrast_status.h b/src/ipa/raspberrypi/controller/contrast_status.h\n> index 11d55295963b..ef2a7c680fc2 100644\n> --- a/src/ipa/raspberrypi/controller/contrast_status.h\n> +++ b/src/ipa/raspberrypi/controller/contrast_status.h\n> @@ -11,7 +11,7 @@\n> * of contrast stretching based on the AGC histogram.\n> */\n> \n> -#define CONTRAST_NUM_POINTS 33\n> +constexpr unsigned int ContrastNumPoints = 33;\n> \n> struct ContrastPoint {\n> \tuint16_t x;\n> @@ -19,7 +19,7 @@ struct ContrastPoint {\n> };\n> \n> struct ContrastStatus {\n> -\tstruct ContrastPoint points[CONTRAST_NUM_POINTS];\n> +\tstruct ContrastPoint points[ContrastNumPoints];\n> \tdouble brightness;\n> \tdouble contrast;\n> };\n> diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> index e9a945e3a630..e0c174b6580d 100644\n> --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> @@ -28,17 +28,17 @@ LOG_DEFINE_CATEGORY(RPiAgc)\n> \n> #define NAME \"rpi.agc\"\n> \n> -#define PIPELINE_BITS 13 /* seems to be a 13-bit pipeline */\n> +static constexpr unsigned int PipelineBits = 13; /* seems to be a 13-bit pipeline */\n> \n> void AgcMeteringMode::read(boost::property_tree::ptree const ¶ms)\n> {\n> \tint num = 0;\n> \tfor (auto &p : params.get_child(\"weights\")) {\n> -\t\tif (num == AGC_STATS_SIZE)\n> +\t\tif (num == AgcStatsSize)\n> \t\t\tLOG(RPiAgc, Fatal) << \"AgcConfig: too many weights\";\n> \t\tweights[num++] = p.second.get_value();\n> \t}\n> -\tif (num != AGC_STATS_SIZE)\n> +\tif (num != AgcStatsSize)\n> \t\tLOG(RPiAgc, Fatal) << \"AgcConfig: insufficient weights\";\n> }\n> \n> @@ -525,11 +525,11 @@ static double computeInitialY(bcm2835_isp_stats *stats, AwbStatus const &awb,\n> \t * \"average\" metering (i.e. all pixels equally important).\n> \t */\n> \tdouble rSum = 0, gSum = 0, bSum = 0, pixelSum = 0;\n> -\tfor (int i = 0; i < AGC_STATS_SIZE; i++) {\n> +\tfor (unsigned int i = 0; i < AgcStatsSize; i++) {\n> \t\tdouble counted = regions[i].counted;\n> -\t\tdouble rAcc = std::min(regions[i].r_sum * gain, ((1 << PIPELINE_BITS) - 1) * counted);\n> -\t\tdouble gAcc = std::min(regions[i].g_sum * gain, ((1 << PIPELINE_BITS) - 1) * counted);\n> -\t\tdouble bAcc = std::min(regions[i].b_sum * gain, ((1 << PIPELINE_BITS) - 1) * counted);\n> +\t\tdouble rAcc = std::min(regions[i].r_sum * gain, ((1 << PipelineBits) - 1) * counted);\n> +\t\tdouble gAcc = std::min(regions[i].g_sum * gain, ((1 << PipelineBits) - 1) * counted);\n> +\t\tdouble bAcc = std::min(regions[i].b_sum * gain, ((1 << PipelineBits) - 1) * counted);\n> \t\trSum += rAcc * weights[i];\n> \t\tgSum += gAcc * weights[i];\n> \t\tbSum += bAcc * weights[i];\n> @@ -542,7 +542,7 @@ static double computeInitialY(bcm2835_isp_stats *stats, AwbStatus const &awb,\n> \tdouble ySum = rSum * awb.gainR * .299 +\n> \t\t gSum * awb.gainG * .587 +\n> \t\t bSum * awb.gainB * .114;\n> -\treturn ySum / pixelSum / (1 << PIPELINE_BITS);\n> +\treturn ySum / pixelSum / (1 << PipelineBits);\n> }\n> \n> /*\n> @@ -553,13 +553,13 @@ static double computeInitialY(bcm2835_isp_stats *stats, AwbStatus const &awb,\n> * (contrived) cases.\n> */\n> \n> -#define EV_GAIN_Y_TARGET_LIMIT 0.9\n> +static constexpr double EvGainYTargetLimit = 0.9;\n> \n> static double constraintComputeGain(AgcConstraint &c, Histogram &h, double lux,\n> \t\t\t\t double evGain, double &targetY)\n> {\n> \ttargetY = c.yTarget.eval(c.yTarget.domain().clip(lux));\n> -\ttargetY = std::min(EV_GAIN_Y_TARGET_LIMIT, targetY * evGain);\n> +\ttargetY = std::min(EvGainYTargetLimit, targetY * evGain);\n> \tdouble iqm = h.interQuantileMean(c.qLo, c.qHi);\n> \treturn (targetY * NUM_HISTOGRAM_BINS) / iqm;\n> }\n> @@ -578,7 +578,7 @@ void Agc::computeGain(bcm2835_isp_stats *statistics, Metadata *imageMetadata,\n> \t * that we consider the histogram constraints.\n> \t */\n> \ttargetY = config_.yTarget.eval(config_.yTarget.domain().clip(lux.lux));\n> -\ttargetY = std::min(EV_GAIN_Y_TARGET_LIMIT, targetY * evGain);\n> +\ttargetY = std::min(EvGainYTargetLimit, targetY * evGain);\n> \n> \t/*\n> \t * Do this calculation a few times as brightness increase can be\n> diff --git a/src/ipa/raspberrypi/controller/rpi/agc.h b/src/ipa/raspberrypi/controller/rpi/agc.h\n> index 48b33a10c73a..f57afa6dc80c 100644\n> --- a/src/ipa/raspberrypi/controller/rpi/agc.h\n> +++ b/src/ipa/raspberrypi/controller/rpi/agc.h\n> @@ -22,12 +22,12 @@\n> * number (which is 16).\n> */\n> \n> -#define AGC_STATS_SIZE 15\n> +constexpr unsigned int AgcStatsSize = 15;\n> \n> namespace RPiController {\n> \n> struct AgcMeteringMode {\n> -\tdouble weights[AGC_STATS_SIZE];\n> +\tdouble weights[AgcStatsSize];\n> \tvoid read(boost::property_tree::ptree const ¶ms);\n> };\n> \n> diff --git a/src/ipa/raspberrypi/controller/rpi/alsc.cpp b/src/ipa/raspberrypi/controller/rpi/alsc.cpp\n> index 7df89445711a..03ae33501dc0 100644\n> --- a/src/ipa/raspberrypi/controller/rpi/alsc.cpp\n> +++ b/src/ipa/raspberrypi/controller/rpi/alsc.cpp\n> @@ -23,8 +23,8 @@ LOG_DEFINE_CATEGORY(RPiAlsc)\n> \n> #define NAME \"rpi.alsc\"\n> \n> -static const int X = ALSC_CELLS_X;\n> -static const int Y = ALSC_CELLS_Y;\n> +static const int X = AlscCellsX;\n> +static const int Y = AlscCellsY;\n> static const int XY = X * Y;\n> static const double InsufficientData = -1.0;\n> \n> diff --git a/src/ipa/raspberrypi/controller/rpi/alsc.h b/src/ipa/raspberrypi/controller/rpi/alsc.h\n> index e17f2fe93379..4e9a715ae0ab 100644\n> --- a/src/ipa/raspberrypi/controller/rpi/alsc.h\n> +++ b/src/ipa/raspberrypi/controller/rpi/alsc.h\n> @@ -19,7 +19,7 @@ namespace RPiController {\n> \n> struct AlscCalibration {\n> \tdouble ct;\n> -\tdouble table[ALSC_CELLS_X * ALSC_CELLS_Y];\n> +\tdouble table[AlscCellsX * AlscCellsY];\n> };\n> \n> struct AlscConfig {\n> @@ -35,7 +35,7 @@ struct AlscConfig {\n> \tuint16_t minG;\n> \tdouble omega;\n> \tuint32_t nIter;\n> -\tdouble luminanceLut[ALSC_CELLS_X * ALSC_CELLS_Y];\n> +\tdouble luminanceLut[AlscCellsX * AlscCellsY];\n> \tdouble luminanceStrength;\n> \tstd::vector calibrationsCr;\n> \tstd::vector calibrationsCb;\n> @@ -61,7 +61,7 @@ private:\n> \tAlscConfig config_;\n> \tbool firstTime_;\n> \tCameraMode cameraMode_;\n> -\tdouble luminanceTable_[ALSC_CELLS_X * ALSC_CELLS_Y];\n> +\tdouble luminanceTable_[AlscCellsX * AlscCellsY];\n> \tstd::thread asyncThread_;\n> \tvoid asyncFunc(); /* asynchronous thread function */\n> \tstd::mutex mutex_;\n> @@ -87,8 +87,8 @@ private:\n> \tint frameCount_;\n> \t/* counts up to startupFrames for Process function */\n> \tint frameCount2_;\n> -\tdouble syncResults_[3][ALSC_CELLS_Y][ALSC_CELLS_X];\n> -\tdouble prevSyncResults_[3][ALSC_CELLS_Y][ALSC_CELLS_X];\n> +\tdouble syncResults_[3][AlscCellsY][AlscCellsX];\n> +\tdouble prevSyncResults_[3][AlscCellsY][AlscCellsX];\n> \tvoid waitForAysncThread();\n> \t/*\n> \t * The following are for the asynchronous thread to use, though the main\n> @@ -98,13 +98,13 @@ private:\n> \t/* copy out the results from the async thread so that it can be restarted */\n> \tvoid fetchAsyncResults();\n> \tdouble ct_;\n> -\tbcm2835_isp_stats_region statistics_[ALSC_CELLS_Y * ALSC_CELLS_X];\n> -\tdouble asyncResults_[3][ALSC_CELLS_Y][ALSC_CELLS_X];\n> -\tdouble asyncLambdaR_[ALSC_CELLS_X * ALSC_CELLS_Y];\n> -\tdouble asyncLambdaB_[ALSC_CELLS_X * ALSC_CELLS_Y];\n> +\tbcm2835_isp_stats_region statistics_[AlscCellsY * AlscCellsX];\n> +\tdouble asyncResults_[3][AlscCellsY][AlscCellsX];\n> +\tdouble asyncLambdaR_[AlscCellsX * AlscCellsY];\n> +\tdouble asyncLambdaB_[AlscCellsX * AlscCellsY];\n> \tvoid doAlsc();\n> -\tdouble lambdaR_[ALSC_CELLS_X * ALSC_CELLS_Y];\n> -\tdouble lambdaB_[ALSC_CELLS_X * ALSC_CELLS_Y];\n> +\tdouble lambdaR_[AlscCellsX * AlscCellsY];\n> +\tdouble lambdaB_[AlscCellsX * AlscCellsY];\n> };\n> \n> } /* namespace RPiController */\n> diff --git a/src/ipa/raspberrypi/controller/rpi/awb.cpp b/src/ipa/raspberrypi/controller/rpi/awb.cpp\n> index c379e6b92649..ad75d55f0976 100644\n> --- a/src/ipa/raspberrypi/controller/rpi/awb.cpp\n> +++ b/src/ipa/raspberrypi/controller/rpi/awb.cpp\n> @@ -18,8 +18,8 @@ LOG_DEFINE_CATEGORY(RPiAwb)\n> \n> #define NAME \"rpi.awb\"\n> \n> -#define AWB_STATS_SIZE_X DEFAULT_AWB_REGIONS_X\n> -#define AWB_STATS_SIZE_Y DEFAULT_AWB_REGIONS_Y\n> +static constexpr unsigned int AwbStatsSizeX = DEFAULT_AWB_REGIONS_X;\n> +static constexpr unsigned int AwbStatsSizeY = DEFAULT_AWB_REGIONS_Y;\n> \n> /*\n> * todo - the locking in this algorithm needs some tidying up as has been done\n> @@ -357,7 +357,7 @@ static void generateStats(std::vector &zones,\n> \t\t\t bcm2835_isp_stats_region *stats, double minPixels,\n> \t\t\t double minG)\n> {\n> -\tfor (int i = 0; i < AWB_STATS_SIZE_X * AWB_STATS_SIZE_Y; i++) {\n> +\tfor (unsigned int i = 0; i < AwbStatsSizeX * AwbStatsSizeY; i++) {\n> \t\tAwb::RGB zone;\n> \t\tdouble counted = stats[i].counted;\n> \t\tif (counted >= minPixels) {\n> @@ -599,7 +599,7 @@ void Awb::awbBayes()\n> \t * valid... not entirely sure about this.\n> \t */\n> \tPwl prior = interpolatePrior();\n> -\tprior *= zones_.size() / (double)(AWB_STATS_SIZE_X * AWB_STATS_SIZE_Y);\n> +\tprior *= zones_.size() / (double)(AwbStatsSizeX * AwbStatsSizeY);\n> \tprior.map([](double x, double y) {\n> \t\tLOG(RPiAwb, Debug) << \"(\" << x << \",\" << y << \")\";\n> \t});\n> diff --git a/src/ipa/raspberrypi/controller/rpi/contrast.cpp b/src/ipa/raspberrypi/controller/rpi/contrast.cpp\n> index 04aeb91e4d61..9e60dc5d47e7 100644\n> --- a/src/ipa/raspberrypi/controller/rpi/contrast.cpp\n> +++ b/src/ipa/raspberrypi/controller/rpi/contrast.cpp\n> @@ -70,15 +70,15 @@ static void fillInStatus(ContrastStatus &status, double brightness,\n> {\n> \tstatus.brightness = brightness;\n> \tstatus.contrast = contrast;\n> -\tfor (int i = 0; i < CONTRAST_NUM_POINTS - 1; i++) {\n> +\tfor (unsigned int i = 0; i < ContrastNumPoints - 1; i++) {\n> \t\tint x = i < 16 ? i * 1024\n> \t\t\t : (i < 24 ? (i - 16) * 2048 + 16384\n> \t\t\t\t\t : (i - 24) * 4096 + 32768);\n> \t\tstatus.points[i].x = x;\n> \t\tstatus.points[i].y = std::min(65535.0, gammaCurve.eval(x));\n> \t}\n> -\tstatus.points[CONTRAST_NUM_POINTS - 1].x = 65535;\n> -\tstatus.points[CONTRAST_NUM_POINTS - 1].y = 65535;\n> +\tstatus.points[ContrastNumPoints - 1].x = 65535;\n> +\tstatus.points[ContrastNumPoints - 1].y = 65535;\n> }\n> \n> void Contrast::initialise()\n> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> index 951d8c65abfd..9d550354d78e 100644\n> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> @@ -1235,7 +1235,7 @@ void IPARPi::applyGamma(const struct ContrastStatus *contrastStatus, ControlList\n> \tstruct bcm2835_isp_gamma gamma;\n> \n> \tgamma.enabled = 1;\n> -\tfor (int i = 0; i < CONTRAST_NUM_POINTS; i++) {\n> +\tfor (unsigned int i = 0; i < ContrastNumPoints; i++) {\n> \t\tgamma.x[i] = contrastStatus->points[i].x;\n> \t\tgamma.y[i] = contrastStatus->points[i].y;\n> \t}","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 9BB0BC3275\n\tfor ;\n\tWed, 27 Jul 2022 02:02:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 21BDC63312;\n\tWed, 27 Jul 2022 04:02:18 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 71C41603E8\n\tfor ;\n\tWed, 27 Jul 2022 04:02:17 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id DDE8E56D;\n\tWed, 27 Jul 2022 04:02:16 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1658887338;\n\tbh=vDfDgTvYFbtMKe+RLXgtEORP/EFQIO2M2wiHFni7xBQ=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=DOWLfXGv5uQz+KMJHMjwXsBu80Ch3jQeLRCneuix9vVfWXtf2HzzciEB/VL8qREgq\n\t/fw1CML2VamuiMVB1gp17SilUA8W1aSANfXuoiMCUM/dF8Nw1Hx6Pmz9nZ5aR63NYY\n\tURZ9PgbdGFUdWbK8DcSNaBpNf2QlxxmwWSvwLXYv4ywB3ugXD4oA0LZo7dSrlRmHV3\n\tozeg/I9f0Q0Y7S0UiNb04o24FC+C0frF9GVFUJMgl1eYkgf6HLIbX0/LNzRlx57Fm4\n\t7H75nNXLAXPXlcDJsFaJnIhKIdDikkxKPYTK6nvv0GYp1ZLpbjRF48LbtX2/wobnYB\n\tb+s7swuqTEBmg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1658887337;\n\tbh=vDfDgTvYFbtMKe+RLXgtEORP/EFQIO2M2wiHFni7xBQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ITYFezp1nklp4C140ggqYtj3uLzYTPwLOdV7VmLfbCp4NGfpBHvOBK2orBKngl4LH\n\ty8lau04SLm1g2ErUBNhRa9CifglIDFBixDikmc+GyttGnX4CCi88CvgLX5AvxyS4xm\n\tzW7+VI7RTmxiTqkLNtOXvTCM4mvx69wo/9X2Ajdw="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"ITYFezp1\"; dkim-atps=neutral","Date":"Wed, 27 Jul 2022 05:02:15 +0300","To":"Naushir Patuck ","Message-ID":"","References":"<20220726124549.1646-1-naush@raspberrypi.com>\n\t<20220726124549.1646-17-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220726124549.1646-17-naush@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH 16/17] ipa: raspberrypi: Remove\n\t#define constants","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","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]