[{"id":31370,"web_url":"https://patchwork.libcamera.org/comment/31370/","msgid":"","date":"2024-09-26T06:58:13","subject":"Re: [PATCH] ipa: rpi: Use std::abs()","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi all,\n\n\nOn Wed, 25 Sept 2024 at 23:12, Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> As explained in the coding style document, usage of std::abs() is\n> preferred over abs() or fabs() as it picks the correct function based on\n> the argument type. Replace calls to abs() and fabs() with std::abs() in\n> the Raspberry Pi algorithms.\n>\n> This fixes a reported warning from clang:\n>\n> ../src/ipa/rpi/controller/rpi/awb.cpp:508:6: error: using integer absolute\n> value function 'abs' when argument is of floating point type\n> [-Werror,-Wabsolute-value]\n> if (abs(denominator) > eps) {\n> ^\n> ../src/ipa/rpi/controller/rpi/awb.cpp:508:6: note: use function 'std::abs'\n> instead\n> if (abs(denominator) > eps) {\n> ^~~\n> std::abs\n>\n> Reported-by: Maarten Lankhorst \n> Signed-off-by: Laurent Pinchart \n> ---\n> Naush, David, I have only compile-tested this patch, could you give it a\n> try to make sure it doesn't break anything ?\n>\n\nI've done some brief testing and nothing seems to go wrong running with\nthese changes.\n\nTested-by: Naushir Patuck \nReviewed-by: Naushir Patuck \n\n---\n> src/ipa/rpi/controller/rpi/alsc.cpp | 18 +++++++++---------\n> src/ipa/rpi/controller/rpi/awb.cpp | 3 ++-\n> 2 files changed, 11 insertions(+), 10 deletions(-)\n>\n> diff --git a/src/ipa/rpi/controller/rpi/alsc.cpp\n> b/src/ipa/rpi/controller/rpi/alsc.cpp\n> index 161fd45526ec..21edb819ad1c 100644\n> --- a/src/ipa/rpi/controller/rpi/alsc.cpp\n> +++ b/src/ipa/rpi/controller/rpi/alsc.cpp\n> @@ -6,8 +6,8 @@\n> */\n>\n> #include \n> +#include \n> #include \n> -#include \n> #include \n> #include \n>\n> @@ -252,12 +252,12 @@ static bool compareModes(CameraMode const &cm0,\n> CameraMode const &cm1)\n> */\n> if (cm0.transform != cm1.transform)\n> return true;\n> - int leftDiff = abs(cm0.cropX - cm1.cropX);\n> - int topDiff = abs(cm0.cropY - cm1.cropY);\n> - int rightDiff = fabs(cm0.cropX + cm0.scaleX * cm0.width -\n> - cm1.cropX - cm1.scaleX * cm1.width);\n> - int bottomDiff = fabs(cm0.cropY + cm0.scaleY * cm0.height -\n> - cm1.cropY - cm1.scaleY * cm1.height);\n> + int leftDiff = std::abs(cm0.cropX - cm1.cropX);\n> + int topDiff = std::abs(cm0.cropY - cm1.cropY);\n> + int rightDiff = std::abs(cm0.cropX + cm0.scaleX * cm0.width -\n> + cm1.cropX - cm1.scaleX * cm1.width);\n> + int bottomDiff = std::abs(cm0.cropY + cm0.scaleY * cm0.height -\n> + cm1.cropY - cm1.scaleY * cm1.height);\n> /*\n> * These thresholds are a rather arbitrary amount chosen to trigger\n> * when carrying on with the previously calculated tables might be\n> @@ -732,7 +732,7 @@ static double gaussSeidel2Sor(const\n> SparseArray &M, double omega,\n> double maxDiff = 0;\n> for (i = 0; i < XY; i++) {\n> lambda[i] = oldLambda[i] + (lambda[i] - oldLambda[i]) *\n> omega;\n> - if (fabs(lambda[i] - oldLambda[i]) > fabs(maxDiff))\n> + if (std::abs(lambda[i] - oldLambda[i]) > std::abs(maxDiff))\n> maxDiff = lambda[i] - oldLambda[i];\n> }\n> return maxDiff;\n> @@ -764,7 +764,7 @@ static void runMatrixIterations(const Array2D\n> &C,\n> constructM(C, W, M);\n> double lastMaxDiff = std::numeric_limits::max();\n> for (unsigned int i = 0; i < nIter; i++) {\n> - double maxDiff = fabs(gaussSeidel2Sor(M, omega, lambda,\n> lambdaBound));\n> + double maxDiff = std::abs(gaussSeidel2Sor(M, omega,\n> lambda, lambdaBound));\n> if (maxDiff < threshold) {\n> LOG(RPiAlsc, Debug)\n> << \"Stop after \" << i + 1 << \" iterations\";\n> diff --git a/src/ipa/rpi/controller/rpi/awb.cpp\n> b/src/ipa/rpi/controller/rpi/awb.cpp\n> index f45525bce2d1..e5d51092605e 100644\n> --- a/src/ipa/rpi/controller/rpi/awb.cpp\n> +++ b/src/ipa/rpi/controller/rpi/awb.cpp\n> @@ -6,6 +6,7 @@\n> */\n>\n> #include \n> +#include \n> #include \n>\n> #include \n> @@ -505,7 +506,7 @@ static double interpolateQuadatric(ipa::Pwl::Point\n> const &a, ipa::Pwl::Point con\n> const double eps = 1e-3;\n> ipa::Pwl::Point ca = c - a, ba = b - a;\n> double denominator = 2 * (ba.y() * ca.x() - ca.y() * ba.x());\n> - if (abs(denominator) > eps) {\n> + if (std::abs(denominator) > eps) {\n> double numerator = ba.y() * ca.x() * ca.x() - ca.y() *\n> ba.x() * ba.x();\n> double result = numerator / denominator + a.x();\n> return std::max(a.x(), std::min(c.x(), result));\n>\n> base-commit: f2088eb91fd6477b152233b9031cb115ca1ae824\n> --\n> Regards,\n>\n> Laurent Pinchart\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 715AFC3257\n\tfor ;\n\tThu, 26 Sep 2024 06:58:45 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 67CA76350E;\n\tThu, 26 Sep 2024 08:58:44 +0200 (CEST)","from mail-yw1-x1132.google.com (mail-yw1-x1132.google.com\n\t[IPv6:2607:f8b0:4864:20::1132])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 55429618D7\n\tfor ;\n\tThu, 26 Sep 2024 08:58:42 +0200 (CEST)","by mail-yw1-x1132.google.com with SMTP id\n\t00721157ae682-6e2317cf8bcso758917b3.0\n\tfor ;\n\tWed, 25 Sep 2024 23:58:42 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"r1cToF9c\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1727333921; x=1727938721;\n\tdarn=lists.libcamera.org; \n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=VfEMUQDjn7zNDo5Hs05H6QguPNWQEV4p1fpr8xt57vM=;\n\tb=r1cToF9c8pxpF0vdsLziAPlx+FlFEoV155f8Y9EBRVOeNpuGFqnIOaKZvynijn5Pos\n\tTuYdQG4IXFAWGZKXFvZl1JOCA/3cSYt/SoqgK9s9LYlU54zzK3R2fWuGXha9IyEhybIm\n\twcXJCykILEltI+MvfDBKYLEuS7HCXr09tL8v6c2D49PSTZAWqTQLQ1gEhrvPibITrwoe\n\tJN15NY0LULODWKm4gNdRo9BJXgXWkONus0IVSjRmGuM3SxMo2w3slH5o2vJGUAsnAHJZ\n\t/bBN00BjIu5Bi4M6easRgmF6TD7wIrtGsLaK/yRbVA9qF9Z2Glz1tLlfWLf1z4nRgwQ9\n\t9XTQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1727333921; x=1727938721;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=VfEMUQDjn7zNDo5Hs05H6QguPNWQEV4p1fpr8xt57vM=;\n\tb=Z4ZK/NCUu3RZKG5Ppfunro1GkIaW/AsGfFXqMOeF2mwlW89xn1N3g1MogNp6NLD4Qy\n\ta38AdghvUbWGjXGwc4C2uBxmRBdu+cawkA9NAPXkleskOkwXqEksWK3kPc9RgHLYCHpv\n\t79xZbcpmDRkiGZXCg/gLOtGx/EqvfpNQrpEtW3m/CklcoUgzd6Yd1zOZdlbu2K0vmLHA\n\txMzf913cHXv8Fe1PtCVyYlfqtjH8qkTQsIYvtG5VHSGks0TbHdGEUDOtLKatBubTlnes\n\tZS/GlwpaHG42ZSLc7MfijT2EMhYA1dafYcJ+soTH8mS6LzclNyTEshb6Dwpa9p2R8e2e\n\tAZyA==","X-Gm-Message-State":"AOJu0YwuxKK3tBb9kLk1Qs3o9TN/XXab0BIgnlKy8DIZ7a5T34G6KC1o\n\tjL6MKKSjOMVp90r2MHOu2yKkvJYxmVfI1Rq1RKhXfFCeuUbDNJqgcBpd7KhhaUD5CVV/tYAk1dt\n\tIuNj4jOY3S6C5fCBVypzGSmBn6OKiGH81j1L2IQ==","X-Google-Smtp-Source":"AGHT+IFbVVxxs+D5U9xv/SXw3KLfuRiOUx9MCzq4My8s+fOC1C1JrBqGPoAdOLut/1nQ9TS8Np5PHsLwhAH5LL/dUQo=","X-Received":"by 2002:a05:690c:490a:b0:6c8:7827:f28c with SMTP id\n\t00721157ae682-6e22fee9ee9mr5933637b3.6.1727333921137; Wed, 25 Sep 2024\n\t23:58:41 -0700 (PDT)","MIME-Version":"1.0","References":"<20240925221210.12331-1-laurent.pinchart@ideasonboard.com>","In-Reply-To":"<20240925221210.12331-1-laurent.pinchart@ideasonboard.com>","From":"Naushir Patuck ","Date":"Thu, 26 Sep 2024 07:58:13 +0100","Message-ID":"","Subject":"Re: [PATCH] ipa: rpi: Use std::abs()","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org, \n\tDavid Plowman ,\n\tMilan Zamazal , Maarten Lankhorst ","Content-Type":"multipart/alternative; boundary=\"000000000000c747d906230044f7\"","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":31395,"web_url":"https://patchwork.libcamera.org/comment/31395/","msgid":"<172734660216.3726802.12500898421521903584@ping.linuxembedded.co.uk>","date":"2024-09-26T10:30:02","subject":"Re: [PATCH] ipa: rpi: Use std::abs()","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Laurent Pinchart (2024-09-25 23:12:10)\n> As explained in the coding style document, usage of std::abs() is\n> preferred over abs() or fabs() as it picks the correct function based on\n> the argument type. Replace calls to abs() and fabs() with std::abs() in\n> the Raspberry Pi algorithms.\n> \n> This fixes a reported warning from clang:\n> \n> ../src/ipa/rpi/controller/rpi/awb.cpp:508:6: error: using integer absolute value function 'abs' when argument is of floating point type [-Werror,-Wabsolute-value]\n> if (abs(denominator) > eps) {\n> ^\n> ../src/ipa/rpi/controller/rpi/awb.cpp:508:6: note: use function 'std::abs' instead\n> if (abs(denominator) > eps) {\n> ^~~\n> std::abs\n> \n> Reported-by: Maarten Lankhorst \n> Signed-off-by: Laurent Pinchart \n> ---\n> Naush, David, I have only compile-tested this patch, could you give it a\n> try to make sure it doesn't break anything ?\n> ---\n> src/ipa/rpi/controller/rpi/alsc.cpp | 18 +++++++++---------\n> src/ipa/rpi/controller/rpi/awb.cpp | 3 ++-\n> 2 files changed, 11 insertions(+), 10 deletions(-)\n> \n> diff --git a/src/ipa/rpi/controller/rpi/alsc.cpp b/src/ipa/rpi/controller/rpi/alsc.cpp\n> index 161fd45526ec..21edb819ad1c 100644\n> --- a/src/ipa/rpi/controller/rpi/alsc.cpp\n> +++ b/src/ipa/rpi/controller/rpi/alsc.cpp\n> @@ -6,8 +6,8 @@\n> */\n> \n> #include \n> +#include \n> #include \n> -#include \n\nOh, I see - this is where we're converting more of the math.h users too.\n\n\n\n> #include \n> #include \n> \n> @@ -252,12 +252,12 @@ static bool compareModes(CameraMode const &cm0, CameraMode const &cm1)\n> */\n> if (cm0.transform != cm1.transform)\n> return true;\n> - int leftDiff = abs(cm0.cropX - cm1.cropX);\n> - int topDiff = abs(cm0.cropY - cm1.cropY);\n> - int rightDiff = fabs(cm0.cropX + cm0.scaleX * cm0.width -\n> - cm1.cropX - cm1.scaleX * cm1.width);\n> - int bottomDiff = fabs(cm0.cropY + cm0.scaleY * cm0.height -\n> - cm1.cropY - cm1.scaleY * cm1.height);\n> + int leftDiff = std::abs(cm0.cropX - cm1.cropX);\n> + int topDiff = std::abs(cm0.cropY - cm1.cropY);\n> + int rightDiff = std::abs(cm0.cropX + cm0.scaleX * cm0.width -\n> + cm1.cropX - cm1.scaleX * cm1.width);\n> + int bottomDiff = std::abs(cm0.cropY + cm0.scaleY * cm0.height -\n> + cm1.cropY - cm1.scaleY * cm1.height);\n> /*\n> * These thresholds are a rather arbitrary amount chosen to trigger\n> * when carrying on with the previously calculated tables might be\n> @@ -732,7 +732,7 @@ static double gaussSeidel2Sor(const SparseArray &M, double omega,\n> double maxDiff = 0;\n> for (i = 0; i < XY; i++) {\n> lambda[i] = oldLambda[i] + (lambda[i] - oldLambda[i]) * omega;\n> - if (fabs(lambda[i] - oldLambda[i]) > fabs(maxDiff))\n> + if (std::abs(lambda[i] - oldLambda[i]) > std::abs(maxDiff))\n> maxDiff = lambda[i] - oldLambda[i];\n> }\n> return maxDiff;\n> @@ -764,7 +764,7 @@ static void runMatrixIterations(const Array2D &C,\n> constructM(C, W, M);\n> double lastMaxDiff = std::numeric_limits::max();\n> for (unsigned int i = 0; i < nIter; i++) {\n> - double maxDiff = fabs(gaussSeidel2Sor(M, omega, lambda, lambdaBound));\n> + double maxDiff = std::abs(gaussSeidel2Sor(M, omega, lambda, lambdaBound));\n> if (maxDiff < threshold) {\n> LOG(RPiAlsc, Debug)\n> << \"Stop after \" << i + 1 << \" iterations\";\n> diff --git a/src/ipa/rpi/controller/rpi/awb.cpp b/src/ipa/rpi/controller/rpi/awb.cpp\n> index f45525bce2d1..e5d51092605e 100644\n> --- a/src/ipa/rpi/controller/rpi/awb.cpp\n> +++ b/src/ipa/rpi/controller/rpi/awb.cpp\n> @@ -6,6 +6,7 @@\n> */\n> \n> #include \n> +#include \n> #include \n> \n> #include \n> @@ -505,7 +506,7 @@ static double interpolateQuadatric(ipa::Pwl::Point const &a, ipa::Pwl::Point con\n> const double eps = 1e-3;\n> ipa::Pwl::Point ca = c - a, ba = b - a;\n> double denominator = 2 * (ba.y() * ca.x() - ca.y() * ba.x());\n> - if (abs(denominator) > eps) {\n> + if (std::abs(denominator) > eps) {\n\nLooks like an easy/straight-forward change.\n\n\nReviewed-by: Kieran Bingham \n\n> double numerator = ba.y() * ca.x() * ca.x() - ca.y() * ba.x() * ba.x();\n> double result = numerator / denominator + a.x();\n> return std::max(a.x(), std::min(c.x(), result));\n> \n> base-commit: f2088eb91fd6477b152233b9031cb115ca1ae824\n> -- \n> Regards,\n> \n> Laurent Pinchart\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 963A2C3257\n\tfor ;\n\tThu, 26 Sep 2024 10:30:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3732663511;\n\tThu, 26 Sep 2024 12:30:08 +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 636EB63502\n\tfor ;\n\tThu, 26 Sep 2024 12:30:06 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 74CFF8D4;\n\tThu, 26 Sep 2024 12:28:37 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"J+7NkLK/\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1727346517;\n\tbh=SaihATakqWThSsjtLwAimgTdnUToO9h88XggigW6QUw=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=J+7NkLK/BOURdNerDKhBl9qhTAHUAvuhChxd9QIGniYzkYxIjbm/bE/8Xv4bM4U+w\n\t1zqNObWiQMjsW+4dTdBdPD9LJGo1qC31+wEopvUZetV9gUMRTMKYKSwM1Uw0QVCZSt\n\teysyUFEEym+7OkgmhodcCWs6DT3Ha5KRT72F3V9U=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20240925221210.12331-1-laurent.pinchart@ideasonboard.com>","References":"<20240925221210.12331-1-laurent.pinchart@ideasonboard.com>","Subject":"Re: [PATCH] ipa: rpi: Use std::abs()","From":"Kieran Bingham ","Cc":"Naushir Patuck ,\n\tDavid Plowman ,\n\tMilan Zamazal , Maarten Lankhorst ","To":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Thu, 26 Sep 2024 11:30:02 +0100","Message-ID":"<172734660216.3726802.12500898421521903584@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]