[{"id":29990,"web_url":"https://patchwork.libcamera.org/comment/29990/","msgid":"","date":"2024-06-17T10:14:29","subject":"Re: [PATCH 08/12] ipa: rkisp1: agc: Simplify predivider calculation","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"On Sun, Jun 16, 2024 at 07:39:06PM +0300, Laurent Pinchart wrote:\n> The condition\n> \n> \tif (std::pow(std::floor(root), 2) < factor)\n> \t\tpredivider = static_cast(std::ceil(root));\n> \telse\n> \t\tpredivider = static_cast(std::floor(root));\n> \n> can only be false when the factor's root is an integer. In that case,\n> std::ceil(root) and std::floor(root) will be equal. The computation can\n> thus be simplified by always rounding up.\n> \n> Signed-off-by: Laurent Pinchart \n\nReviewed-by: Paul Elder \n\n> ---\n> src/ipa/rkisp1/algorithms/agc.cpp | 7 +------\n> 1 file changed, 1 insertion(+), 6 deletions(-)\n> \n> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> index 9f3b59b45f95..a61201bb05c9 100644\n> --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> @@ -115,12 +115,7 @@ uint8_t Agc::computeHistogramPredivider(const Size &size,\n> \tint count = mode == RKISP1_CIF_ISP_HISTOGRAM_MODE_RGB_COMBINED ? 3 : 1;\n> \tdouble factor = size.width * size.height * count / 65536.0;\n> \tdouble root = std::sqrt(factor);\n> -\tuint8_t predivider;\n> -\n> -\tif (std::pow(std::floor(root), 2) < factor)\n> -\t\tpredivider = static_cast(std::ceil(root));\n> -\telse\n> -\t\tpredivider = static_cast(std::floor(root));\n> +\tuint8_t predivider = static_cast(std::ceil(root));\n> \n> \treturn std::clamp(predivider, 3, 127);\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 EA73CC3237\n\tfor ;\n\tMon, 17 Jun 2024 10:14:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E89A06549C;\n\tMon, 17 Jun 2024 12:14:36 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E866865498\n\tfor ;\n\tMon, 17 Jun 2024 12:14:35 +0200 (CEST)","from pyrite.rasen.tech (h175-177-049-156.catv02.itscom.jp\n\t[175.177.49.156])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 3B8A82D5;\n\tMon, 17 Jun 2024 12:14:18 +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=\"n55S9f00\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1718619259;\n\tbh=wPxECkYx2q/DAkBtcM/gBjrVUMzSbVqlOtDr2BWprwg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=n55S9f00WVLpHmYjqhU/nW6DN29BbNTPIGMdPwMhj1E9O0/G2W/wbbGrBpo2gPnNv\n\tmjW9P/mFe84sqf3mL2RIkNT/uhOQXb+U6fHJZyiEQwWpv4tkU0p6JDx2blqxv88M44\n\tCM1IPy/PMhLxO/tAz1hfqqXkyjS7lD2UiHxjwmBo=","Date":"Mon, 17 Jun 2024 19:14:29 +0900","From":"Paul Elder ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 08/12] ipa: rkisp1: agc: Simplify predivider calculation","Message-ID":"","References":"<20240616163910.5506-1-laurent.pinchart@ideasonboard.com>\n\t<20240616163910.5506-9-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20240616163910.5506-9-laurent.pinchart@ideasonboard.com>","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":30000,"web_url":"https://patchwork.libcamera.org/comment/30000/","msgid":"<171862958722.2248009.2889018271818995615@ping.linuxembedded.co.uk>","date":"2024-06-17T13:06:27","subject":"Re: [PATCH 08/12] ipa: rkisp1: agc: Simplify predivider calculation","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Laurent Pinchart (2024-06-16 17:39:06)\n> The condition\n> \n> if (std::pow(std::floor(root), 2) < factor)\n> predivider = static_cast(std::ceil(root));\n> else\n> predivider = static_cast(std::floor(root));\n> \n> can only be false when the factor's root is an integer. In that case,\n> std::ceil(root) and std::floor(root) will be equal. The computation can\n> thus be simplified by always rounding up.\n> \n> Signed-off-by: Laurent Pinchart \n\nThis sounds fine to me. I hesitated as I wasn't sure if this was\npapering over some floating point issue ... but without conjouring up a\nfull test suite to prove this ... I'm just going to say:\n\n\nReviewed-by: Kieran Bingham \n\n\n> ---\n> src/ipa/rkisp1/algorithms/agc.cpp | 7 +------\n> 1 file changed, 1 insertion(+), 6 deletions(-)\n> \n> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> index 9f3b59b45f95..a61201bb05c9 100644\n> --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> @@ -115,12 +115,7 @@ uint8_t Agc::computeHistogramPredivider(const Size &size,\n> int count = mode == RKISP1_CIF_ISP_HISTOGRAM_MODE_RGB_COMBINED ? 3 : 1;\n> double factor = size.width * size.height * count / 65536.0;\n> double root = std::sqrt(factor);\n> - uint8_t predivider;\n> -\n> - if (std::pow(std::floor(root), 2) < factor)\n> - predivider = static_cast(std::ceil(root));\n> - else\n> - predivider = static_cast(std::floor(root));\n> + uint8_t predivider = static_cast(std::ceil(root));\n> \n> return std::clamp(predivider, 3, 127);\n> }\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 D637FBD87C\n\tfor ;\n\tMon, 17 Jun 2024 13:06:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EFFAE65499;\n\tMon, 17 Jun 2024 15:06:31 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4A13B65491\n\tfor ;\n\tMon, 17 Jun 2024 15:06:30 +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 4760539F;\n\tMon, 17 Jun 2024 15:06:13 +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=\"D8cA1Xkj\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1718629573;\n\tbh=yE2h1se4XcGD5BJ/bNSlqiEVq1wwm01VHmvNN00nRsg=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=D8cA1Xkjr6cBl8M9Q7b9Fmug2p2HqVlN+13qzEsnzj9OB7j/u7Zf2C02yzCcQpTSL\n\t/jD9sSMaiZKeSaoDWpZomzkJSHLvsJW8riYkcHOijxgbce3pPVeqPqVVnhvU5dJVzs\n\tZvJpCOzt+r8Za6P3Jz1RvFexJ5yjZGZSeqp4yajA=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20240616163910.5506-9-laurent.pinchart@ideasonboard.com>","References":"<20240616163910.5506-1-laurent.pinchart@ideasonboard.com>\n\t<20240616163910.5506-9-laurent.pinchart@ideasonboard.com>","Subject":"Re: [PATCH 08/12] ipa: rkisp1: agc: Simplify predivider calculation","From":"Kieran Bingham ","To":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Mon, 17 Jun 2024 14:06:27 +0100","Message-ID":"<171862958722.2248009.2889018271818995615@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\" "}}]