[{"id":18551,"web_url":"https://patchwork.libcamera.org/comment/18551/","msgid":"","date":"2021-08-04T10:24:10","subject":"Re: [libcamera-devel] [RFC PATCH v1 0/3] pipeline: isp: The\n\tsoftware ISP Module","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Siyuan,\n\nNice to see your first contribution to libcamera, welcome to the\nproject.\n\nAs this is an RFC series, I'll start by sharing comments on the\narchitecture, without focussing on implementation details.\n\nAs I understand it, the goal of this software ISP implementation is to\nbe used as a reference on platforms that don't have a hardware ISP,\npaving the way for a GPU-based ISP implementation. As such, the simple\npipeline handler is the most interesting target. You've used the\nRaspberry Pi as a test platform due to hardware availability constraints\nif I'm not mistaken. This is fine for test purposes, and we can\nintegrate the software ISP with the simple pipeline handler in a\nsubsequen step.\n\nThis being said, I think that patch 1/3 should not get integrated as-is.\nIt's fine for test purpose, but it will compete with the Rasperry Pi\npipeline handler, and if both are compiled in, may prevent the Raspberry\nPi pipeline handler from being loaded.\n\nI see that the implementation is multi-threaded, which is good, as the\nsoftware processing is heavy. However, the thread should be moved to the\nISP class. Generally speaking, the ISP class should expose an interface\nthat resembles a hardware ISP, with functions called from the pipeline\nhandler to queue work to the ISP, and signals being emitted later when\nthe work completes. The work itself needs to be performed in a separate\nthread.\n\nSpeaking of interfaces, I think we should also create a base abstract\nclass that defines the software ISP API. This CPU-based implementation\nwould then inherit from the base class, and a future GPU-based\nimplementation would also do the same, exposing the same API. This will\nallow switching between the two implementations seamlessly, without any\nchange in the pipeline handler.\n\nI've also looked at the ISP implementation itself, and I wonder if it\nshouldn't be restructure to match what a hardware ISP would do. ISPs\ntypically compute statistics from the image, and process pixels in\nhardware. A CPU-based algorithm (implemented in libcamera as an IPA\nmodule) consumes those statistics and produces parameters for the pixel\nprocessing. Practically speaking, this means that, for instance, the\nautoWhiteBalance() function would need to be split in three parts:\nstatistics calculation, computation of the colour gains from the\nstatistics, and multiplication of each pixel by the colour gains. One\nreason why I think statistics computation should probably be split out\nis that the same statistics could probably be reused in different\nprocessing steps. It's also tempting to move the computation of the\ncolour gains (and other processing parameters) to an IPA module, but\nthat may be overkill, maybe having a clear split but keeping all the\ncode in the ISP class would be better. In any case, different options\nshould be considered, and we should pick the best one and explain the\nreasons.\n\nFinally, a few miscellaneous comments about the implementation of the\nISP class:\n\n- Internal functions should be private, to clearly differentiate between\n the public API and the internal implementation.\n\n- Memory management needs to be considered. You currently allocate a\n large scratch buffer (rgbData) in ISP::processing(), and I wonder if\n it would make more sense to perform processing line by line to only\n require line buffers. This would have an effect on cache usage, which\n needs to be considered too. It's an internal implementation decision\n though, so it won't affect the API.\n\nOn Wed, Aug 04, 2021 at 08:33:44AM +0100, Siyuan Fan wrote:\n> From: Fan Siyuan \n> \n> This patch series adds support for CPU-based software ISP.\n> \n> Patch 1/3 add software ISP-based pipeline handler. Patch 2/3 add \n> software ISP class. Patch 3/3 add all meson configure files.\n> \n> This is a RFC patch because so far the buffer processed by pipeline\n> cannot be output due to lack of metadata information which needs to\n> apply the ISP friend class in class FrameBuffer just like V4L2VideoDevice. \n> \n> \n> Fan Siyuan (3):\n> pipeline: isp: The software ISP-based pipeline handler\n> pipeline: isp: The software ISP class\n> pipeline: isp: All meson configure files\n> \n> meson_options.txt | 2 +-\n> src/libcamera/pipeline/isp/isp.cpp | 323 ++++++++++\n> src/libcamera/pipeline/isp/isp_processing.cpp | 593 ++++++++++++++++++\n> src/libcamera/pipeline/isp/isp_processing.h | 62 ++\n> src/libcamera/pipeline/isp/meson.build | 6 +\n> 5 files changed, 985 insertions(+), 1 deletion(-)\n> create mode 100644 src/libcamera/pipeline/isp/isp.cpp\n> create mode 100644 src/libcamera/pipeline/isp/isp_processing.cpp\n> create mode 100644 src/libcamera/pipeline/isp/isp_processing.h\n> create mode 100644 src/libcamera/pipeline/isp/meson.build","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 7D8D1C3232\n\tfor ;\n\tWed, 4 Aug 2021 10:24:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DAC6F68811;\n\tWed, 4 Aug 2021 12:24:23 +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 062636026E\n\tfor ;\n\tWed, 4 Aug 2021 12:24:23 +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 7E7A724F;\n\tWed, 4 Aug 2021 12:24:22 +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=\"m6GxfgKT\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1628072662;\n\tbh=sIfJWY2Y8tJ4imXgWb9E+w2io61MNdx5uJrjMYJFDRw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=m6GxfgKTpKTLY/+2xpvq73J78TLVJzBn3sUlcY1T6qDmcniz+yVi+2A4nBQ1Xjq5l\n\t6hq05gcP7TMQceGASRUmD+OWk7Eu1vp3O7JDArwlHMcqJ6GbBxjcUIwM5Mch/sxjbt\n\t9GTofC6HICfG1ys6rA9cMWJ8rATS4OIRSohor1Y4=","Date":"Wed, 4 Aug 2021 13:24:10 +0300","From":"Laurent Pinchart ","To":"Siyuan Fan ","Message-ID":"","References":"","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [RFC PATCH v1 0/3] pipeline: isp: The\n\tsoftware ISP Module","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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]