[{"id":26646,"web_url":"https://patchwork.libcamera.org/comment/26646/","msgid":"<20230313081302.GE2545@pendragon.ideasonboard.com>","date":"2023-03-13T08:13:02","subject":"Re: [libcamera-devel] [PATCH] py: cam: Fix demosaic overflow issue","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Tomi,\n\nThank you for the patch.\n\nOn Mon, Mar 13, 2023 at 09:28:13AM +0200, Tomi Valkeinen via libcamera-devel wrote:\n> The demosaic code first expands the buffer datatype to uint16, and then\n> shifts the data left so that the 8, 10 and 12 bitspp formats all become\n> 16 bitspp.\n> \n> It then, eventually, uses np.einsum to calculate averages, but this\n> averaging sums multiple uint16 values together, and stores them in\n> uint16 storage. As in the first step we shifted the values left,\n> possibly getting values close to the maximum of uint16 range, we, of\n> course, overflow when summing them together. This leads to rather bad\n> looking images.\n> \n> Fix this by dropping the original shift. It serves no purpose, and is\n> probably a remnant of some early testing code. This way the largest\n> numbers we are summing together are 12 bit values, and as we use a 3x3\n> window from which we fetch values, for a single rgb plane, the max\n> number of 12 bit values is 5 (for green). Sum of 5 12 bit values is well\n> below the 16 bit maximum.\n> \n> Signed-off-by: Tomi Valkeinen \n\nReviewed-by: Laurent Pinchart \n\n> ---\n> src/py/cam/helpers.py | 6 ++----\n> 1 file changed, 2 insertions(+), 4 deletions(-)\n> \n> diff --git a/src/py/cam/helpers.py b/src/py/cam/helpers.py\n> index 6b32a134..2d906667 100644\n> --- a/src/py/cam/helpers.py\n> +++ b/src/py/cam/helpers.py\n> @@ -117,14 +117,12 @@ def to_rgb(fmt, size, data):\n> bayer_pattern = fmt[1:5]\n> bitspp = int(fmt[5:])\n> \n> - # \\todo shifting leaves the lowest bits 0\n> if bitspp == 8:\n> data = data.reshape((h, w))\n> - data = data.astype(np.uint16) << 8\n> + data = data.astype(np.uint16)\n> elif bitspp in [10, 12]:\n> data = data.view(np.uint16)\n> data = data.reshape((h, w))\n> - data = data << (16 - bitspp)\n> else:\n> raise Exception('Bad bitspp:' + str(bitspp))\n> \n> @@ -145,7 +143,7 @@ def to_rgb(fmt, size, data):\n> b0 = (idx % 2, idx // 2)\n> \n> rgb = demosaic(data, r0, g0, g1, b0)\n> - rgb = (rgb >> 8).astype(np.uint8)\n> + rgb = (rgb >> (bitspp - 8)).astype(np.uint8)\n> \n> else:\n> rgb = None","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 93C73BD80A\n\tfor ;\n\tMon, 13 Mar 2023 08:13:00 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E773D62709;\n\tMon, 13 Mar 2023 09:12:59 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 10F60626B2\n\tfor ;\n\tMon, 13 Mar 2023 09:12:59 +0100 (CET)","from pendragon.ideasonboard.com (unknown [89.244.118.114])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 6BA75563;\n\tMon, 13 Mar 2023 09:12:58 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1678695179;\n\tbh=6dYqaDPS++m8TblgRgGiZ9kZcCWlFais625YnMEB4dI=;\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=09FhBERiSnUUb4NY7KNf3SyGElLUelYMV3u1sMpwVTPGV8Z21D7dXRU5izryXzK53\n\tK0RnlcE8VPt9hAyDiSuR2xLqLwt9W2ZugPCMGgkgr+q08vW3dSQUKn66c0LbMY+SZV\n\tXvW7DHkK1RBQEkaZRtnceG2KMG5rccF8zBkaVL83iWG3nnaKB7af+PRbadIhtHrl2J\n\tx4FmiwIelwR04aOLnIDNiG132MNonsWhanqS5GObuuawJsE3IgqqA2WalRzRCli99X\n\taCAjQsFbHto1VnLAjRTLQtYz+LfcLO4gcHxqeo0i9fHgojeikVIfzY2jqBpa8TqtFY\n\tzlnz3Vvs+sYdQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1678695178;\n\tbh=6dYqaDPS++m8TblgRgGiZ9kZcCWlFais625YnMEB4dI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=nhEuJrvTRCdPHoFGsGyl2BtcX6+mecXXc0aQs/OH2oLmWmxrOMuPip0bftYGqdV8J\n\tS21S7yrVaZ+eobjLwhqd8wxaYqzJPvosSJe5wcahbuMP0C4+s1Y74IAhecQAIVPoJb\n\tHLgLcYCSWpe69IvhBWCUELJg4qeLLIvaBhB5a2BU="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"nhEuJrvT\"; dkim-atps=neutral","Date":"Mon, 13 Mar 2023 10:13:02 +0200","To":"Tomi Valkeinen ","Message-ID":"<20230313081302.GE2545@pendragon.ideasonboard.com>","References":"<20230313072813.23430-1-tomi.valkeinen@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20230313072813.23430-1-tomi.valkeinen@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH] py: cam: Fix demosaic overflow issue","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\" "}},{"id":26705,"web_url":"https://patchwork.libcamera.org/comment/26705/","msgid":"<167944160141.1593123.8144617977315420263@Monstersaurus>","date":"2023-03-21T23:33:21","subject":"Re: [libcamera-devel] [PATCH] py: cam: Fix demosaic overflow issue","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Tomi Valkeinen via libcamera-devel (2023-03-13 07:28:13)\n> The demosaic code first expands the buffer datatype to uint16, and then\n> shifts the data left so that the 8, 10 and 12 bitspp formats all become\n> 16 bitspp.\n> \n> It then, eventually, uses np.einsum to calculate averages, but this\n> averaging sums multiple uint16 values together, and stores them in\n> uint16 storage. As in the first step we shifted the values left,\n> possibly getting values close to the maximum of uint16 range, we, of\n> course, overflow when summing them together. This leads to rather bad\n> looking images.\n> \n> Fix this by dropping the original shift. It serves no purpose, and is\n> probably a remnant of some early testing code. This way the largest\n> numbers we are summing together are 12 bit values, and as we use a 3x3\n> window from which we fetch values, for a single rgb plane, the max\n> number of 12 bit values is 5 (for green). Sum of 5 12 bit values is well\n> below the 16 bit maximum.\n> \n> Signed-off-by: Tomi Valkeinen \n\nThis looks reasonable to me.\n\nReviewed-by: Kieran Bingham \n\n> ---\n> src/py/cam/helpers.py | 6 ++----\n> 1 file changed, 2 insertions(+), 4 deletions(-)\n> \n> diff --git a/src/py/cam/helpers.py b/src/py/cam/helpers.py\n> index 6b32a134..2d906667 100644\n> --- a/src/py/cam/helpers.py\n> +++ b/src/py/cam/helpers.py\n> @@ -117,14 +117,12 @@ def to_rgb(fmt, size, data):\n> bayer_pattern = fmt[1:5]\n> bitspp = int(fmt[5:])\n> \n> - # \\todo shifting leaves the lowest bits 0\n> if bitspp == 8:\n> data = data.reshape((h, w))\n> - data = data.astype(np.uint16) << 8\n> + data = data.astype(np.uint16)\n> elif bitspp in [10, 12]:\n> data = data.view(np.uint16)\n> data = data.reshape((h, w))\n> - data = data << (16 - bitspp)\n> else:\n> raise Exception('Bad bitspp:' + str(bitspp))\n> \n> @@ -145,7 +143,7 @@ def to_rgb(fmt, size, data):\n> b0 = (idx % 2, idx // 2)\n> \n> rgb = demosaic(data, r0, g0, g1, b0)\n> - rgb = (rgb >> 8).astype(np.uint8)\n> + rgb = (rgb >> (bitspp - 8)).astype(np.uint8)\n> \n> else:\n> rgb = None\n> -- \n> 2.34.1\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 9B1C6C0F2A\n\tfor ;\n\tTue, 21 Mar 2023 23:33:27 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DBE6D626E5;\n\tWed, 22 Mar 2023 00:33:26 +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 20BE361ED3\n\tfor ;\n\tWed, 22 Mar 2023 00:33:25 +0100 (CET)","from pendragon.ideasonboard.com\n\t(aztw-30-b2-v4wan-166917-cust845.vm26.cable.virginm.net\n\t[82.37.23.78])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 8530033F;\n\tWed, 22 Mar 2023 00:33:24 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1679441607;\n\tbh=Rghtw9J2Fs7qvNhKNHceKCsh8od71KOACXaWRSHvvF8=;\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:\n\tFrom;\n\tb=kEwTF95/FTAL+Gi0WBi7Xg/TxYOJNqgM9y+iVDecjFcEe2cGkt2VhhVar55PoZtfk\n\ta1GztYZxI+L1udGcE0YT9lmuX8XMBFpZCOJ/aLfGBn8F9kipOEGDqs0+6XiAe32EwP\n\tXAuFyEOcQVsOoJVPZ8ScfFRi5EgmHeTo3t0qYErFeG1sjhX302ZfgUGNhmM8EQF1yb\n\tAJPDePE0Ltaww2KburqqufvDMqbOqCzZa174t6BfHApyNRtcRCqIN/dHChDlzuBcG1\n\tO7paRZGU57ERt5z9fWzV2SxjhOG6f5Ka+SKJDM+wV8ofBobySjvyW8icZhd9lM5Cp2\n\tJVzgy1DujizsA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1679441604;\n\tbh=Rghtw9J2Fs7qvNhKNHceKCsh8od71KOACXaWRSHvvF8=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=e44FmyPmRmghBitm83uNEWeaAvKHMZt8kX9QGuGWhn20a3KnheX4KoNx8uDBpjp8z\n\tgqSA3NpAvNeOT3Y2JXDUmQW7EoBYTFSX4wIFa0YUNmLeoh+mjR7WQBlof7kxTpn852\n\tGHhB4QooB6M+Ppq27o1JQ1Of3n6l/N+uDNkTFDcQ="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"e44FmyPm\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20230313072813.23430-1-tomi.valkeinen@ideasonboard.com>","References":"<20230313072813.23430-1-tomi.valkeinen@ideasonboard.com>","To":"Tomi Valkeinen ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Tue, 21 Mar 2023 23:33:21 +0000","Message-ID":"<167944160141.1593123.8144617977315420263@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH] py: cam: Fix demosaic overflow issue","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]