[{"id":19662,"web_url":"https://patchwork.libcamera.org/comment/19662/","msgid":"","date":"2021-09-14T04:56:08","subject":"Re: [libcamera-devel] [PATCH 11/11] ipa: ipu3: tonemapping: Add the\n\tdocumentation for ToneMapping","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jean-Michel,\n\nThank you for the patch.\n\nOn Mon, Sep 13, 2021 at 04:58:10PM +0200, Jean-Michel Hautbois wrote:\n> The Tonemapping algorithm is currently undocumented.\n\ns/Tonemapping/tone mapping/\n\n> Provide an introduction and overview to the implementation as the class\n> definition and document how the algorithm operates in the process and\n> prepare methods.\n> \n> Signed-off-by: Jean-Michel Hautbois \n> Signed-off-by: Kieran Bingham \n> ---\n> src/ipa/ipu3/algorithms/tone_mapping.cpp | 35 +++++++++++++++++++++++-\n> 1 file changed, 34 insertions(+), 1 deletion(-)\n> \n> diff --git a/src/ipa/ipu3/algorithms/tone_mapping.cpp b/src/ipa/ipu3/algorithms/tone_mapping.cpp\n> index 40337f9d..15e9ab3f 100644\n> --- a/src/ipa/ipu3/algorithms/tone_mapping.cpp\n> +++ b/src/ipa/ipu3/algorithms/tone_mapping.cpp\n> @@ -10,15 +10,40 @@\n> #include \n> #include \n> \n> +/**\n> + * \\file tone_mapping.h\n> + */\n> +\n> namespace libcamera {\n> \n> namespace ipa::ipu3::algorithms {\n> \n> +/**\n> + * \\class ToneMapping\n> + * \\brief A class to handle tone mapping based on gamma\n> + *\n> + * This algorithm improves the image dynamic using a look-up table which is\n> + * generated based on a gamma parameter.\n> + *\n> + * Gamma values less than one have the effect of compressing the image histogram\n> + * while values over 1 will expand it.\n\nI think this is misleading. The standard gamma correction formula is\nV_{out} = A V_{in}^\\gamma, but we implement inverse gamma. I'm also not\nsure that \"compressing\" or \"expanding\" the histogram is accurate. 0.0\nstays 0.0 and 1.0 stays 1.0, so the histogram is not really stretched when\n\"expanding\". Saying that it provides better overall contrast in that\ncase doesn't seem accurate to me.\n\n> + *\n> + * Expanding the histogram has the effect of providing better overall contrast.\n> + */\n> +\n> ToneMapping::ToneMapping()\n> \t: gamma_(1.0)\n> {\n> }\n> \n> +/**\n> + * \\brief Fill in the parameter structure, and enable gamma control\n> + * \\param context The shared IPA context\n> + * \\param params The IPU3 parameters\n> + *\n> + * Populate the IPU3 parameter structure with our gamma correction table, and\n> + * enable the gamma control module in the accelerator cluster.\n\nSame comment as in other patches about accelerator cluster.\n\n> + */\n> void ToneMapping::prepare([[maybe_unused]] IPAContext &context,\n> \t\t\t ipu3_uapi_params *params)\n> {\n> @@ -33,7 +58,15 @@ void ToneMapping::prepare([[maybe_unused]] IPAContext &context,\n> \tparams->acc_param.gamma.gc_ctrl.enable = 1;\n> }\n> \n> -void ToneMapping::process([[maybe_unused]] IPAContext &context,\n> +/**\n> + * \\brief Calculate the Gamma curve\n\n\"Calculate the tone mapping look up table\"\n\nWe should consider the algorithm as handling tone mapping, with the\ncurrent implementation using a fixed inverse gamma function (the\ndecision to name the corresponding ImgU data structure gamma is a bit\nunfortunate as it's misleading). I'd update the rest of the\ndocumentation accordingly.\n\n> + * \\param context The shared IPA context\n> + * \\param stats The IPU3 statistics and ISP results\n> + *\n> + * The gamma correction look up table is generated as an inverse power curve\n> + * from our gamma setting.\n> + */\n> +void ToneMapping::process(IPAContext &context,\n> \t\t\t [[maybe_unused]] const ipu3_uapi_stats_3a *stats)\n> {\n> \t/*","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 51335BDB1D\n\tfor ;\n\tTue, 14 Sep 2021 04:56:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 86A9969187;\n\tTue, 14 Sep 2021 06:56:34 +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 F082C6916F\n\tfor ;\n\tTue, 14 Sep 2021 06:56:32 +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 627312A5;\n\tTue, 14 Sep 2021 06:56:32 +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=\"ABYGEhlu\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1631595392;\n\tbh=X+9PTMbt2zAHjXT+JfyeIblHXC7YIk+I0TwCgx2LMOg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ABYGEhlu5m2zhdYZQqe9qt8qwPf7TqGVFS7ZId5Df2IliExZE8S7q/pzyQ/EO7qVi\n\tUVZ/PuYcWHxSGp2WUYyeBUrFYYIPUKtTMkUSYXveypfZqyAhM8yVQX30m+QXdjf97k\n\tIYtpP61WCQHc7Zf66aahGW3u6GHglkBPZpIJ4Udk=","Date":"Tue, 14 Sep 2021 07:56:08 +0300","From":"Laurent Pinchart ","To":"Jean-Michel Hautbois ","Message-ID":"","References":"<20210913145810.66515-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210913145810.66515-12-jeanmichel.hautbois@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210913145810.66515-12-jeanmichel.hautbois@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 11/11] ipa: ipu3: tonemapping: Add the\n\tdocumentation for ToneMapping","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\" "}}]