[{"id":22421,"web_url":"https://patchwork.libcamera.org/comment/22421/","msgid":"","date":"2022-03-24T01:06:20","subject":"Re: [libcamera-devel] [PATCH v2 5/5] ipa: ipu3: af: Simplify\n\taccumulations of y_items","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nThank you for the patch.\n\nOn Wed, Mar 23, 2022 at 01:56:14PM +0000, Kieran Bingham via libcamera-devel wrote:\n> Simplify the accumulation of the total and variance with a ternary\n> operator.\n> \n> Signed-off-by: Kieran Bingham \n> ---\n> \n> This is optional really, it's only really a stylistic preference.\n> \n> \n> src/ipa/ipu3/algorithms/af.cpp | 14 ++++----------\n> 1 file changed, 4 insertions(+), 10 deletions(-)\n> \n> diff --git a/src/ipa/ipu3/algorithms/af.cpp b/src/ipa/ipu3/algorithms/af.cpp\n> index ff5e9fb5b3c5..940ed68ea14a 100644\n> --- a/src/ipa/ipu3/algorithms/af.cpp\n> +++ b/src/ipa/ipu3/algorithms/af.cpp\n> @@ -342,20 +342,14 @@ double Af::afEstimateVariance(Span y_items, bool isY1)\n> \tdouble mean;\n> \tdouble var_sum = 0;\n> \n> -\tfor (auto y : y_items) {\n> -\t\tif (isY1)\n> -\t\t\ttotal += y.y1_avg;\n> -\t\telse\n> -\t\t\ttotal += y.y2_avg;\n> -\t}\n> +\tfor (auto y : y_items)\n> +\t\ttotal += isY1 ? y.y1_avg : y.y2_avg;\n> \n> \tmean = total / y_items.size();\n> \n> \tfor (auto y : y_items) {\n> -\t\tif (isY1)\n> -\t\t\tvar_sum += pow((y.y1_avg - mean), 2);\n> -\t\telse\n> -\t\t\tvar_sum += pow((y.y2_avg - mean), 2);\n> +\t\tdouble avg = isY1 ? y.y1_avg : y.y2_avg;\n> +\t\tvar_sum += pow((avg - mean), 2);\n\nYou can drop the extra parentheses.\n\nReviewed-by: Laurent Pinchart \n\nBy, this could also be optimized as\n\n\tdouble sum = 0;\n\tdouble sqr_sum = 0;\n\n\tfor (auto y : y_items) {\n\t\tdouble avg = isY1 ? y.y1_avg : y.y2_avg;\n\n\t\tsum += avg;\n\t\tsqr_sum += avg * avg;\n\t}\n\n\tdouble mean = total / y_items.size();\n\treturn sqr_sum / y_items.size() - mean * mean;\n\nwhich should be more efficient, especially with a larger number of\nitems. The algorithm is subject to numerical instability though,\nespecially when the variance is small. The two-pass approach is\nnumerically stable (when the number of items is small).\n\n> \t}\n> \n> \treturn var_sum / static_cast(y_items.size());","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 C2163BD80A\n\tfor ;\n\tThu, 24 Mar 2022 01:06:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 28A2C604DA;\n\tThu, 24 Mar 2022 02:06:23 +0100 (CET)","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 85E7A604C5\n\tfor ;\n\tThu, 24 Mar 2022 02:06:22 +0100 (CET)","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 EBD11FEF;\n\tThu, 24 Mar 2022 02:06:21 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1648083983;\n\tbh=ZGwdHscE7KgZUFWa0xTz0okEy51Bp/1qlAfYx/nDM08=;\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=M6z0OUn+B1zUQZ4cAleDQK7krmz0fz3cQcdAkF0ZeXl1M9Ft5ohI/eCKMfPvtTUne\n\tSIGuJKHog1PDIlD+2ZMqB4WMDjF0hNOc58xjn+8x2jOvvq2TLL+aHxoJSeOmjj00BU\n\tbrL4NUCRPdmtA1/vcZSLXT4dtz0q449/Wq9wpSCLBFIe9QPuU/K9yW02aUXsIw0Pce\n\tZD117Z5Zc0kvaGoaTHLbPjzhvuasv1A5bh2CWae+OffI4W7aWNoPR24ryNQV6xDwrR\n\t7h6tQehALegGSPMSp5R5FlUkBQZSRPFIf3zUuCbXB5JX48jAmCejiMCL/M98u3CMCr\n\tA3kfiRK9+LZPw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1648083982;\n\tbh=ZGwdHscE7KgZUFWa0xTz0okEy51Bp/1qlAfYx/nDM08=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=nCq0NPaPytW9GR46IOD79n6sgq5i9x+RJZKQlJuMIHH/MtaH6fFqSnA7ckFHjfM18\n\tBWrvtYNGPUsKavJDu1jMvqsSSmP+JBeITWAgZCMlr41hNXjGXBp0bLBrk/8JX1mlyt\n\tYg5eaUTxuVxsnf4xCVAeYUUT47C3BOXlgD3E/o4A="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"nCq0NPaP\"; dkim-atps=neutral","Date":"Thu, 24 Mar 2022 03:06:20 +0200","To":"Kieran Bingham ","Message-ID":"","References":"<20220323135614.865252-1-kieran.bingham@ideasonboard.com>\n\t<20220323135614.865252-6-kieran.bingham@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220323135614.865252-6-kieran.bingham@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 5/5] ipa: ipu3: af: Simplify\n\taccumulations of y_items","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22430,"web_url":"https://patchwork.libcamera.org/comment/22430/","msgid":"<164811915856.2130830.16021698783544098120@Monstersaurus>","date":"2022-03-24T10:52:38","subject":"Re: [libcamera-devel] [PATCH v2 5/5] ipa: ipu3: af: Simplify\n\taccumulations of y_items","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Laurent Pinchart (2022-03-24 01:06:20)\n> Hi Kieran,\n> \n> Thank you for the patch.\n> \n> On Wed, Mar 23, 2022 at 01:56:14PM +0000, Kieran Bingham via libcamera-devel wrote:\n> > Simplify the accumulation of the total and variance with a ternary\n> > operator.\n> > \n> > Signed-off-by: Kieran Bingham \n> > ---\n> > \n> > This is optional really, it's only really a stylistic preference.\n> > \n> > \n> > src/ipa/ipu3/algorithms/af.cpp | 14 ++++----------\n> > 1 file changed, 4 insertions(+), 10 deletions(-)\n> > \n> > diff --git a/src/ipa/ipu3/algorithms/af.cpp b/src/ipa/ipu3/algorithms/af.cpp\n> > index ff5e9fb5b3c5..940ed68ea14a 100644\n> > --- a/src/ipa/ipu3/algorithms/af.cpp\n> > +++ b/src/ipa/ipu3/algorithms/af.cpp\n> > @@ -342,20 +342,14 @@ double Af::afEstimateVariance(Span y_items, bool isY1)\n> > double mean;\n> > double var_sum = 0;\n> > \n> > - for (auto y : y_items) {\n> > - if (isY1)\n> > - total += y.y1_avg;\n> > - else\n> > - total += y.y2_avg;\n> > - }\n> > + for (auto y : y_items)\n> > + total += isY1 ? y.y1_avg : y.y2_avg;\n> > \n> > mean = total / y_items.size();\n> > \n> > for (auto y : y_items) {\n> > - if (isY1)\n> > - var_sum += pow((y.y1_avg - mean), 2);\n> > - else\n> > - var_sum += pow((y.y2_avg - mean), 2);\n> > + double avg = isY1 ? y.y1_avg : y.y2_avg;\n> > + var_sum += pow((avg - mean), 2);\n> \n> You can drop the extra parentheses.\n> \n> Reviewed-by: Laurent Pinchart \n> \n> By, this could also be optimized as\n> \n> double sum = 0;\n> double sqr_sum = 0;\n> \n> for (auto y : y_items) {\n> double avg = isY1 ? y.y1_avg : y.y2_avg;\n> \n> sum += avg;\n> sqr_sum += avg * avg;\n> }\n> \n> double mean = total / y_items.size();\n> return sqr_sum / y_items.size() - mean * mean;\n> \n> which should be more efficient, especially with a larger number of\n> items. The algorithm is subject to numerical instability though,\n> especially when the variance is small. The two-pass approach is\n> numerically stable (when the number of items is small).\n\nI did wonder if we could get this in a single loop, but I didn't want to\nbreak any assumptions that were made on precision or overflows etc, by\nmaking fundamental changes to the operation of the code.\n\nI presume that when the variance is small, as a double has 15 decimal\npoints I expect we're way into insignificant numbers that don't affect\nthe overall calculation.\n\nKate, JM, what do you think? Any preference on any of the above?\n\n--\nKieran\n\n\n> \n> > }\n> > \n> > return var_sum / static_cast(y_items.size());\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","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 73261BD80A\n\tfor ;\n\tThu, 24 Mar 2022 10:52:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2AFCC604DA;\n\tThu, 24 Mar 2022 11:52:43 +0100 (CET)","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 7CFE8604C5\n\tfor ;\n\tThu, 24 Mar 2022 11:52:41 +0100 (CET)","from pendragon.ideasonboard.com\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 386D41844;\n\tThu, 24 Mar 2022 11:52:41 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1648119163;\n\tbh=IEcKwFt9MVRFV17C3//Yg7OKMGv7R7meK41Zc4eIvME=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=pGf1+GYEcpH/wnId43wf5UqfJXDk4DkgYqsak5nPz+p3N8zAsSO7Zr2KGpixe9mHC\n\tRksKUkZ3zuG5WpjDD1Xd4MkzeTMG+dQ+1TDhj0A6Sn+Wol7+5+TwqxFLOGMm9gQQ7W\n\tMoLi7T6sYc10+dPMBqXPz8S27vA+Rth15lNlU10HeDhJAzz6orYeSAgmQvm57+DamA\n\t8yvwj5S2iuN/feGJqZc4rF8MmJchMrVNE5kyUeEPrfPanJCepW17Z98AC3V35meP9D\n\tc5q9303pbSm8q0ja04rQ/B7dJzveej8y4R20gAKz43p7W9M6JqqGSGpjTqGEOzNQTs\n\tcrmvDlmvnWP3A==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1648119161;\n\tbh=IEcKwFt9MVRFV17C3//Yg7OKMGv7R7meK41Zc4eIvME=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=rPn+xkdhDdwBj4zoHWYPLI9tJ+oFNcS+FW8Qe+bYlUZgWVGhZ2r24U4ClqfgUFL1c\n\tQgGQACp12uqcvj0LovuTvrCHPpN9VxnKkaJlfHGvxfOLxKzVUjytVTb445inWm8j7c\n\tuUbBOK1dNZdriQUEHnp4C5cEelfXXZwd7sWDiE2c="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"rPn+xkdh\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20220323135614.865252-1-kieran.bingham@ideasonboard.com>\n\t<20220323135614.865252-6-kieran.bingham@ideasonboard.com>\n\t","To":"Laurent Pinchart ","Date":"Thu, 24 Mar 2022 10:52:38 +0000","Message-ID":"<164811915856.2130830.16021698783544098120@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v2 5/5] ipa: ipu3: af: Simplify\n\taccumulations of y_items","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":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22432,"web_url":"https://patchwork.libcamera.org/comment/22432/","msgid":"","date":"2022-03-24T12:24:20","subject":"Re: [libcamera-devel] [PATCH v2 5/5] ipa: ipu3: af: Simplify\n\taccumulations of y_items","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Thu, Mar 24, 2022 at 10:52:38AM +0000, Kieran Bingham wrote:\n> Quoting Laurent Pinchart (2022-03-24 01:06:20)\n> > On Wed, Mar 23, 2022 at 01:56:14PM +0000, Kieran Bingham via libcamera-devel wrote:\n> > > Simplify the accumulation of the total and variance with a ternary\n> > > operator.\n> > > \n> > > Signed-off-by: Kieran Bingham \n> > > ---\n> > > \n> > > This is optional really, it's only really a stylistic preference.\n> > > \n> > > \n> > > src/ipa/ipu3/algorithms/af.cpp | 14 ++++----------\n> > > 1 file changed, 4 insertions(+), 10 deletions(-)\n> > > \n> > > diff --git a/src/ipa/ipu3/algorithms/af.cpp b/src/ipa/ipu3/algorithms/af.cpp\n> > > index ff5e9fb5b3c5..940ed68ea14a 100644\n> > > --- a/src/ipa/ipu3/algorithms/af.cpp\n> > > +++ b/src/ipa/ipu3/algorithms/af.cpp\n> > > @@ -342,20 +342,14 @@ double Af::afEstimateVariance(Span y_items, bool isY1)\n> > > double mean;\n> > > double var_sum = 0;\n> > > \n> > > - for (auto y : y_items) {\n> > > - if (isY1)\n> > > - total += y.y1_avg;\n> > > - else\n> > > - total += y.y2_avg;\n> > > - }\n> > > + for (auto y : y_items)\n> > > + total += isY1 ? y.y1_avg : y.y2_avg;\n> > > \n> > > mean = total / y_items.size();\n> > > \n> > > for (auto y : y_items) {\n> > > - if (isY1)\n> > > - var_sum += pow((y.y1_avg - mean), 2);\n> > > - else\n> > > - var_sum += pow((y.y2_avg - mean), 2);\n> > > + double avg = isY1 ? y.y1_avg : y.y2_avg;\n> > > + var_sum += pow((avg - mean), 2);\n> > \n> > You can drop the extra parentheses.\n> > \n> > Reviewed-by: Laurent Pinchart \n> > \n> > By, this could also be optimized as\n> > \n> > double sum = 0;\n> > double sqr_sum = 0;\n> > \n> > for (auto y : y_items) {\n> > double avg = isY1 ? y.y1_avg : y.y2_avg;\n\nThis could be a uint16_t btw.\n\n> > \n> > sum += avg;\n> > sqr_sum += avg * avg;\n> > }\n> > \n> > double mean = total / y_items.size();\n> > return sqr_sum / y_items.size() - mean * mean;\n> > \n> > which should be more efficient, especially with a larger number of\n> > items. The algorithm is subject to numerical instability though,\n> > especially when the variance is small. The two-pass approach is\n> > numerically stable (when the number of items is small).\n> \n> I did wonder if we could get this in a single loop, but I didn't want to\n> break any assumptions that were made on precision or overflows etc, by\n> making fundamental changes to the operation of the code.\n> \n> I presume that when the variance is small, as a double has 15 decimal\n> points I expect we're way into insignificant numbers that don't affect\n> the overall calculation.\n\nIt's not about the decimal point, see\nhttps://en.wikipedia.org/wiki/Algorithms_for_calculating_variance and\nhttps://en.wikipedia.org/wiki/Catastrophic_cancellation (I like the name\n\"catasrophic cancellation\" :-)).\n\n> Kate, JM, what do you think? Any preference on any of the above?\n> \n> > > }\n> > > \n> > > return var_sum / static_cast(y_items.size());","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 BC1D0BD80A\n\tfor ;\n\tThu, 24 Mar 2022 12:24:25 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 057F2604C5;\n\tThu, 24 Mar 2022 13:24:25 +0100 (CET)","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 9D27B60397\n\tfor ;\n\tThu, 24 Mar 2022 13:24:22 +0100 (CET)","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 13F461852;\n\tThu, 24 Mar 2022 13:24:22 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1648124665;\n\tbh=LzcQz1mUb5wqxbI9j+szHRTYtbc7MBw61jp8Yiy4HYM=;\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=lgSh5EE9AVNx5BGjgkSDfPA2LQMtE6yEYJaRgjprntmOnjFeSbz1MaR3H/+/zMRVJ\n\tvsmhuKAydxynRcH9z4g9j8mN5qi1Ov5B3EwqBF36+Mvxu4zPH7Nco8GscuzhJAPt2y\n\tCOEIe6WJuOXDSAwi3fwo4HgDam6sYoU7aEneVi6MVc1Ksby3XmyEnL6d+MxQNZDk1B\n\tdcil/lKdo3EhWhDd+/worPdC/+MOxQMIpojxOE5x138BzlXlkb9JNWjv4CXsAIbWKt\n\t2/XPvUsWtJgavT3TEx6sFh5EjIG94qcw08cmOTIxUz3y3pQ91Jj4bbvsap4tdAAkuw\n\ty2PYW1N45Ww+Q==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1648124662;\n\tbh=LzcQz1mUb5wqxbI9j+szHRTYtbc7MBw61jp8Yiy4HYM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=GUc02F5Rz88yCiFVWClkqYGw2zsVBVD2qOtHgZxKf0exKLXOhh+BHLyOf6Kx7FQZm\n\tG7Wr/eyVwNktVJhkj5dBkpExlQSBTYz/XzQVo85Vei5yPaqp9Os2A3xGt9KT1AioLI\n\twFQxsyk1CjRdi7jPqASRgD5s/d9Fj9kY+P5001Gg="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"GUc02F5R\"; dkim-atps=neutral","Date":"Thu, 24 Mar 2022 14:24:20 +0200","To":"Kieran Bingham ","Message-ID":"","References":"<20220323135614.865252-1-kieran.bingham@ideasonboard.com>\n\t<20220323135614.865252-6-kieran.bingham@ideasonboard.com>\n\t\n\t<164811915856.2130830.16021698783544098120@Monstersaurus>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<164811915856.2130830.16021698783544098120@Monstersaurus>","Subject":"Re: [libcamera-devel] [PATCH v2 5/5] ipa: ipu3: af: Simplify\n\taccumulations of y_items","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22445,"web_url":"https://patchwork.libcamera.org/comment/22445/","msgid":"","date":"2022-03-25T05:07:15","subject":"Re: [libcamera-devel] [PATCH v2 5/5] ipa: ipu3: af: Simplify\n\taccumulations of y_items","submitter":{"id":105,"url":"https://patchwork.libcamera.org/api/people/105/","name":"Kate Hsuan","email":"hpa@redhat.com"},"content":"Hi Folks,\n\nOn Thu, Mar 24, 2022 at 8:24 PM Laurent Pinchart via libcamera-devel\n wrote:\n>\n> Hi Kieran,\n>\n> On Thu, Mar 24, 2022 at 10:52:38AM +0000, Kieran Bingham wrote:\n> > Quoting Laurent Pinchart (2022-03-24 01:06:20)\n> > > On Wed, Mar 23, 2022 at 01:56:14PM +0000, Kieran Bingham via libcamera-devel wrote:\n> > > > Simplify the accumulation of the total and variance with a ternary\n> > > > operator.\n> > > >\n> > > > Signed-off-by: Kieran Bingham \n> > > > ---\n> > > >\n> > > > This is optional really, it's only really a stylistic preference.\n> > > >\n> > > >\n> > > > src/ipa/ipu3/algorithms/af.cpp | 14 ++++----------\n> > > > 1 file changed, 4 insertions(+), 10 deletions(-)\n> > > >\n> > > > diff --git a/src/ipa/ipu3/algorithms/af.cpp b/src/ipa/ipu3/algorithms/af.cpp\n> > > > index ff5e9fb5b3c5..940ed68ea14a 100644\n> > > > --- a/src/ipa/ipu3/algorithms/af.cpp\n> > > > +++ b/src/ipa/ipu3/algorithms/af.cpp\n> > > > @@ -342,20 +342,14 @@ double Af::afEstimateVariance(Span y_items, bool isY1)\n> > > > double mean;\n> > > > double var_sum = 0;\n> > > >\n> > > > - for (auto y : y_items) {\n> > > > - if (isY1)\n> > > > - total += y.y1_avg;\n> > > > - else\n> > > > - total += y.y2_avg;\n> > > > - }\n> > > > + for (auto y : y_items)\n> > > > + total += isY1 ? y.y1_avg : y.y2_avg;\n> > > >\n> > > > mean = total / y_items.size();\n> > > >\n> > > > for (auto y : y_items) {\n> > > > - if (isY1)\n> > > > - var_sum += pow((y.y1_avg - mean), 2);\n> > > > - else\n> > > > - var_sum += pow((y.y2_avg - mean), 2);\n> > > > + double avg = isY1 ? y.y1_avg : y.y2_avg;\n> > > > + var_sum += pow((avg - mean), 2);\n> > >\n> > > You can drop the extra parentheses.\n> > >\n> > > Reviewed-by: Laurent Pinchart \n> > >\n> > > By, this could also be optimized as\n> > >\n> > > double sum = 0;\n> > > double sqr_sum = 0;\n> > >\n> > > for (auto y : y_items) {\n> > > double avg = isY1 ? y.y1_avg : y.y2_avg;\n>\n> This could be a uint16_t btw.\n>\n> > >\n> > > sum += avg;\n> > > sqr_sum += avg * avg;\n> > > }\n> > >\n> > > double mean = total / y_items.size();\n> > > return sqr_sum / y_items.size() - mean * mean;\n> > >\n> > > which should be more efficient, especially with a larger number of\n> > > items. The algorithm is subject to numerical instability though,\n> > > especially when the variance is small. The two-pass approach is\n> > > numerically stable (when the number of items is small).\n> >\n> > I did wonder if we could get this in a single loop, but I didn't want to\n> > break any assumptions that were made on precision or overflows etc, by\n> > making fundamental changes to the operation of the code.\n> >\n> > I presume that when the variance is small, as a double has 15 decimal\n> > points I expect we're way into insignificant numbers that don't affect\n> > the overall calculation.\n>\n> It's not about the decimal point, see\n> https://en.wikipedia.org/wiki/Algorithms_for_calculating_variance and\n> https://en.wikipedia.org/wiki/Catastrophic_cancellation (I like the name\n> \"catasrophic cancellation\" :-)).\n\nThanks for JM's detailed description of the variance computation\napproaches. The y_table length is determined by the grid width and\nheight. If the maximum values are used, the y_table length will be\n32x24=768. (A fixed number and based on the grid configuration).\nTypically, the grid size is set to 16x16=256. Also, the variance value\ncould be very small (for all white and all black images) or large\n(complicated patterns). A certain level of precision of the\nfloating-point number computation is required.\n\nAccording to the reasons above, I think keeping it simple is good for\nnow since y_table length is not too long. If it still impacts the\nperformance, I can perform some survey on it to see which single pass\nvariance approach is better.\n\n\n>\n> > Kate, JM, what do you think? Any preference on any of the above?\n> >\n> > > > }\n> > > >\n> > > > return var_sum / static_cast(y_items.size());\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\n>\n\n\n--\nBR,\nKate","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 D49ADBD80A\n\tfor ;\n\tFri, 25 Mar 2022 05:07:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 15CB1604D4;\n\tFri, 25 Mar 2022 06:07:35 +0100 (CET)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.129.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1A4BE604C0\n\tfor ;\n\tFri, 25 Mar 2022 06:07:32 +0100 (CET)","from mail-lf1-f70.google.com (mail-lf1-f70.google.com\n\t[209.85.167.70]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id\n\tus-mta-295-Ak_OKWZrNserr1z2srcHog-1; Fri, 25 Mar 2022 01:07:29 -0400","by mail-lf1-f70.google.com with SMTP id\n\tw25-20020a0565120b1900b004489048b5d9so2517995lfu.8\n\tfor ;\n\tThu, 24 Mar 2022 22:07:28 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1648184855;\n\tbh=nI5opU/AWSY7kvECgsW2MV9+ZkwS9cVECfHaywz5tSc=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=JqLzt6rQCRBIOLnb/v/hlckTJw2cxQpJqeym/REpFu/XA9u0cjiRF6hLdu4IwRAT0\n\tIdgOr5y5xSCuuuiUqOSCT3HZ+pP32lXzVSmJv1lGt+//dMsiGmq1qveejWMlMYoIce\n\tfC8sFAhXhR0QMSZ99xLPw2Kn+DBx9AfS0dvQzjxsdv9m76bT+lVm8b8VEPLfdy0cN4\n\tuL//azGe5aGR16d7kV/G+updeHfGt2qVL6KqUJvcTMMEdVDkaNdI5/a4tD0P7nJDD/\n\tVZB9BK5mWnJls1ujxPw/eOoYkJqfPSAS9lvCkn8DoYno1VPsDzA+tArsU0Fcc6EKcd\n\tfv+LgwHLTXN4Q==","v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1648184851;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=NrZC0F3p6gWTGCpT0yjYgTwWsgly9twX20l2ROeO3RY=;\n\tb=P3JrhmwcSvf5BWtllx1XqjG48wmntYuAK9dL+aFKKapaJm+y/BHFbTmroV1JA0mKugPAVV\n\tTn17osoZ7YmIfdPUXUt7M4ZGpCQnmTSNiiikRzKMohQ+/bcogDYJ95fF2owj4OylDCeRqd\n\taYhB7TRo05bw9xB4UPYDQX3eKT8TOIY="],"Authentication-Results":["lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=redhat.com\n\theader.i=@redhat.com header.b=\"P3Jrhmwc\"; \n\tdkim-atps=neutral","relay.mimecast.com;\n\tauth=pass smtp.auth=CUSA124A263 smtp.mailfrom=hpa@redhat.com"],"X-MC-Unique":"Ak_OKWZrNserr1z2srcHog-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=NrZC0F3p6gWTGCpT0yjYgTwWsgly9twX20l2ROeO3RY=;\n\tb=W/t06WZ27XmVus0DlKnF3imn2URuanCI01TPO3+irnO3D3PGtjbnbVLgsGhtjkHh2w\n\txI/BPTwQSTlONtwcMnhCY0xUTCLPWszwIRhL6rMTvR3bTk2bDTGplnaxbvro+25pxSls\n\t+OfgvRKz8zydP2uExgn7sXteN8CurjDbri0MEKNIAbuz3WawDhWpl9elE6rwh5WO+uGU\n\tX8rICUBs3egabQ2WFOJPgXwaMeYfC1QkDvvVEEj4Y49P3VFmb9ActGMeYQ76HojJJxjl\n\t/uUf9D9CIZChxISkdrR7cn3vQHF+2WjNlmgYai5qeg59j68GNXWwFKiji+etG2o3xk8z\n\t3oRA==","X-Gm-Message-State":"AOAM532FgcV6/0Th0pi7nD1VqSgWuC7v6OpRZi7tiq/TS0EIAyK3tq9o\n\taPJ8b6wxC/AH6YCq9A7f33fLDfkw2h1gM2chxnPh1AQT1A3/2SKOhewqjvpEIoCM8k+66HKb8fX\n\tE7js4jESbE0UCJxFeslRu5Nwso/ixqSgQK8hga0//XURXR4vXTQ==","X-Received":["by 2002:a05:6512:4023:b0:44a:6b83:93e with SMTP id\n\tbr35-20020a056512402300b0044a6b83093emr3158692lfb.238.1648184847330; \n\tThu, 24 Mar 2022 22:07:27 -0700 (PDT)","by 2002:a05:6512:4023:b0:44a:6b83:93e with SMTP id\n\tbr35-20020a056512402300b0044a6b83093emr3158679lfb.238.1648184846924;\n\tThu, 24 Mar 2022 22:07:26 -0700 (PDT)"],"X-Google-Smtp-Source":"ABdhPJwwx32Ruj6o6f++wWi3ci9JRwUDktLkRqrYsBbZznUd4B6gvxpDBD6/98fmXn9p5ROJ9pnxRSVYSxlZ6Dsnlvs=","MIME-Version":"1.0","References":"<20220323135614.865252-1-kieran.bingham@ideasonboard.com>\n\t<20220323135614.865252-6-kieran.bingham@ideasonboard.com>\n\t\n\t<164811915856.2130830.16021698783544098120@Monstersaurus>\n\t","In-Reply-To":"","Date":"Fri, 25 Mar 2022 13:07:15 +0800","Message-ID":"","To":"Laurent Pinchart ","X-Mimecast-Spam-Score":"0","X-Mimecast-Originator":"redhat.com","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v2 5/5] ipa: ipu3: af: Simplify\n\taccumulations of y_items","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":"Kate Hsuan via libcamera-devel ","Reply-To":"Kate Hsuan ","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]