[{"id":23151,"web_url":"https://patchwork.libcamera.org/comment/23151/","msgid":"","date":"2022-05-25T23:19:39","subject":"Re: [libcamera-devel] [libcamera-devel 2/5] ipa: libipa: Add init()\n\tfunction to the Algorithm class","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Florian,\n\nThank you for the patch.\n\nBy the way, you don't need to add \"libcamera-devel\" manually to the\nsubject line, the [libcamera-devel] tag is added by the mailing list.\n\nOn Mon, May 23, 2022 at 11:24:32AM +0200, Florian Sylvestre via libcamera-devel wrote:\n> Add the init() function that will be called during algorithm initialization\n> to provide each algorithm the list of algorithms tuning data.\n> Each algorithm will be responsible to grab their corresponding parameters.\n> \n> Signed-off-by: Florian Sylvestre \n> ---\n> src/ipa/libipa/algorithm.cpp | 15 +++++++++++++++\n> src/ipa/libipa/algorithm.h | 10 +++++++++-\n> 2 files changed, 24 insertions(+), 1 deletion(-)\n> \n> diff --git a/src/ipa/libipa/algorithm.cpp b/src/ipa/libipa/algorithm.cpp\n> index 398d5372..269a4beb 100644\n> --- a/src/ipa/libipa/algorithm.cpp\n> +++ b/src/ipa/libipa/algorithm.cpp\n> @@ -29,6 +29,21 @@ namespace ipa {\n> * to manage algorithms regardless of their specific type.\n> */\n> \n> +/**\n> + * \\fn Algorithm::init()\n> + * \\brief Configure the Algorithm with default parameters\n> + * \\param[in] context The shared IPA context\n> + * \\param[in] params The initial parameters used to tune algorithms\n> + *\n> + * This function is called once before the camera is running to get default\n> + * algorithm parameters.\n\nThis may be understood as meaning it's called once every time the camera\nis started. I'd make this a bit clearer:\n\n * This function is called once, when the IPA module is initialized, to\n * initialize the algorithm. The \\a params YamlObject contains IPA module\n * parameters, typically tuning data for all algorithms. The Algorithm is\n * responsible for reading the parameters relevant to its configuration.\n\n> + *\n> + * Algorithms are responsible to read the parameters given and extract their\n> + * parameter configuration.\n> + *\n> + * \\return 0 if successful, an error code otherwise\n> + */\n> +\n> /**\n> * \\fn Algorithm::configure()\n> * \\brief Configure the Algorithm given an IPAConfigInfo\n> diff --git a/src/ipa/libipa/algorithm.h b/src/ipa/libipa/algorithm.h\n> index 766aee5d..f5be1caf 100644\n> --- a/src/ipa/libipa/algorithm.h\n> +++ b/src/ipa/libipa/algorithm.h\n> @@ -10,12 +10,20 @@ namespace libcamera {\n> \n> namespace ipa {\n> \n> -template\n> +template +\t typename Config, typename Params, typename Stats>\n> +\n> class Algorithm\n> {\n> public:\n> \tvirtual ~Algorithm() {}\n> \n> +\tvirtual int init([[maybe_unused]] Context &context,\n> +\t\t\t [[maybe_unused]] const TuningData *params)\n> +\t{\n> +\t\treturn 0;\n> +\t}\n> +\n> \tvirtual int configure([[maybe_unused]] Context &context,\n> \t\t\t [[maybe_unused]] const Config &configInfo)\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 291D6BD161\n\tfor ;\n\tWed, 25 May 2022 23:19:47 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8835A65663;\n\tThu, 26 May 2022 01:19:46 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 93B3260422\n\tfor ;\n\tThu, 26 May 2022 01:19:44 +0200 (CEST)","from pendragon.ideasonboard.com (ip-109-40-242-63.web.vodafone.de\n\t[109.40.242.63])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E9F37BB3;\n\tThu, 26 May 2022 01:19:43 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1653520786;\n\tbh=uorngnx1IUPGp6Ocw5X9E2ggtdo8iRdgwijpJx7l2Wg=;\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=ozGcltJVghp3qZP+Y8cPjT0T6MXSdatYWv+0pbIjxQ2n8+GY+BbtLmzZUpDjENedn\n\t6dafqbU5xZcbbFa2VaqhmHMsF/ok7iOiv3nZWRRgnrRm0U73VeSsPZ4XDAxFkwm3Mv\n\t/xOn3cE0oSDaOAeLaVV17Ve/7wJY0Pmhggrp3kHPmQ9SBLKLW97Kmp/1cl8sl1XZaf\n\tuHUuofitg8fBdYcyY6XBHltBd7lqMr3FCPrD7O671AHH9E/8TAiqGAWWv34WycmSp3\n\t97mOOg1aFOtWHG/G5Ru38BG1Fjz9NA0HdgbLeuxwAi8qVP4BDjN6Xmau5/gexRM6Mk\n\tycvjJGYoC2KRw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1653520784;\n\tbh=uorngnx1IUPGp6Ocw5X9E2ggtdo8iRdgwijpJx7l2Wg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Ly3DhvKsnmCAtDGIDyos9iPV6x7IKwz+mBL+8sVj7O0kUNAAuEs5KuGlK9QbzO69m\n\t3y9M3fPJ/Ed5pv7D+BwXnYh6YpR35vtJWdeJZLyRbHwlcn428UCvN4ZrFBJhU+g0id\n\t8cQkbB6dkq7GkeYQPDzGoUQSqaiOSRFouLvgLUkU="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"Ly3DhvKs\"; dkim-atps=neutral","Date":"Thu, 26 May 2022 02:19:39 +0300","To":"Florian Sylvestre ","Message-ID":"","References":"<20220523092435.475510-1-fsylvestre@baylibre.com>\n\t<20220523092435.475510-3-fsylvestre@baylibre.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220523092435.475510-3-fsylvestre@baylibre.com>","Subject":"Re: [libcamera-devel] [libcamera-devel 2/5] ipa: libipa: Add init()\n\tfunction to the Algorithm class","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":23179,"web_url":"https://patchwork.libcamera.org/comment/23179/","msgid":"<20220527064645.GH4117012@pyrite.rasen.tech>","date":"2022-05-27T06:46:45","subject":"Re: [libcamera-devel] [libcamera-devel 2/5] ipa: libipa: Add init()\n\tfunction to the Algorithm class","submitter":{"id":97,"url":"https://patchwork.libcamera.org/api/people/97/","name":"Nicolas Dufresne via libcamera-devel","email":"libcamera-devel@lists.libcamera.org"},"content":"Hi,\n\nOn Thu, May 26, 2022 at 02:19:39AM +0300, Laurent Pinchart via libcamera-devel wrote:\n> Hi Florian,\n> \n> Thank you for the patch.\n> \n> By the way, you don't need to add \"libcamera-devel\" manually to the\n> subject line, the [libcamera-devel] tag is added by the mailing list.\n> \n> On Mon, May 23, 2022 at 11:24:32AM +0200, Florian Sylvestre via libcamera-devel wrote:\n> > Add the init() function that will be called during algorithm initialization\n> > to provide each algorithm the list of algorithms tuning data.\n> > Each algorithm will be responsible to grab their corresponding parameters.\n> > \n> > Signed-off-by: Florian Sylvestre \n> > ---\n> > src/ipa/libipa/algorithm.cpp | 15 +++++++++++++++\n> > src/ipa/libipa/algorithm.h | 10 +++++++++-\n> > 2 files changed, 24 insertions(+), 1 deletion(-)\n> > \n> > diff --git a/src/ipa/libipa/algorithm.cpp b/src/ipa/libipa/algorithm.cpp\n> > index 398d5372..269a4beb 100644\n> > --- a/src/ipa/libipa/algorithm.cpp\n> > +++ b/src/ipa/libipa/algorithm.cpp\n> > @@ -29,6 +29,21 @@ namespace ipa {\n> > * to manage algorithms regardless of their specific type.\n> > */\n> > \n> > +/**\n> > + * \\fn Algorithm::init()\n> > + * \\brief Configure the Algorithm with default parameters\n> > + * \\param[in] context The shared IPA context\n> > + * \\param[in] params The initial parameters used to tune algorithms\n> > + *\n> > + * This function is called once before the camera is running to get default\n> > + * algorithm parameters.\n> \n> This may be understood as meaning it's called once every time the camera\n> is started. I'd make this a bit clearer:\n> \n> * This function is called once, when the IPA module is initialized, to\n> * initialize the algorithm. The \\a params YamlObject contains IPA module\n> * parameters, typically tuning data for all algorithms. The Algorithm is\n> * responsible for reading the parameters relevant to its configuration.\n\nYeah that's a good clarification.\n\n> \n> > + *\n> > + * Algorithms are responsible to read the parameters given and extract their\n> > + * parameter configuration.\n> > + *\n> > + * \\return 0 if successful, an error code otherwise\n> > + */\n> > +\n> > /**\n> > * \\fn Algorithm::configure()\n> > * \\brief Configure the Algorithm given an IPAConfigInfo\n> > diff --git a/src/ipa/libipa/algorithm.h b/src/ipa/libipa/algorithm.h\n> > index 766aee5d..f5be1caf 100644\n> > --- a/src/ipa/libipa/algorithm.h\n> > +++ b/src/ipa/libipa/algorithm.h\n> > @@ -10,12 +10,20 @@ namespace libcamera {\n> > \n> > namespace ipa {\n> > \n> > -template\n> > +template \n> As we're trying to push for YAML usage everywhere through libcamera, I'm\n> tempted to drop the additional template argument and use YamlObject\n> unconditionally in init() instead of TuningData.\n> \n> Does anyone have an opinion on this ?\n\nOn one hand, since all information will be representable in a yaml file,\nit could be good to unify the tuning data into a YamlObject in\nAlgorithm.\n\nOn the other hand, I think that it's not nice for algorithms to have to\ndeal with yaml directly. As in, the parser could convert the yaml values\ninto a \"proper\" data type that can be operated on.\n\nBack to the first hand, the tuning file is only parsed and operated on\nat init time, so the convenience of handling proper data types vs raw\nyaml values isn't too vital.\n\nBack to the second hand, maybe for tuning parameters that are common\nacross platforms could have a unified parser and converter in libipa.\n\nAfter presenting the hands, I suppose I have to make a choice to be my\nopinion... I'd say the second hand. tbh I forsee large tuning\nparameters, and although they don't need to be operated on, I think\nthey'd be converted from yaml to parameters in the same way across\ndifferent platforms. So instead of extracting each value with get<>()\n(like I see in 5/5), we could have a convenience extractor in libipa,\neventually.\n\nThat's just my two cents.\n\nAnd so for that reason, with the wording fix, I think it's good to go:\n\nReviewed-by: Paul Elder \n\n> > +\t typename Config, typename Params, typename Stats>\n> > +\n> > class Algorithm\n> > {\n> > public:\n> > \tvirtual ~Algorithm() {}\n> > \n> > +\tvirtual int init([[maybe_unused]] Context &context,\n> > +\t\t\t [[maybe_unused]] const TuningData *params)\n> > +\t{\n> > +\t\treturn 0;\n> > +\t}\n> > +\n> > \tvirtual int configure([[maybe_unused]] Context &context,\n> > \t\t\t [[maybe_unused]] const Config &configInfo)\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 E65B8BD160\n\tfor ;\n\tFri, 27 May 2022 06:46:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2D632633A3;\n\tFri, 27 May 2022 08:46:55 +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 BE32761FB6\n\tfor ;\n\tFri, 27 May 2022 08:46:53 +0200 (CEST)","from pyrite.rasen.tech (softbank036240126034.bbtec.net\n\t[36.240.126.34])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D7EBF31A;\n\tFri, 27 May 2022 08:46:51 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1653634015;\n\tbh=E2hjMj8uIhcoteXPBZttcq5pFuzw8aS7pwD6DrDzwJw=;\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=OaKCvd2RVqXMl4oezU7Ln+B5Tp39ubWiebY4GHmGHRVYaKSeZRsLzzB3MGm/qJS94\n\tQtfOMDR3+Us+YNWyILCdm3hK3KhmIiztbgT0Gdxz12cBYBYJRn5ovqeELOvVlD63vT\n\tGvuKFImkKKU8j4ZhCCaD2hxbzf4DBP9Nphku6ZpIm3XKIgrt+uOMv1Wh+SZiu8hHJM\n\twAJ/yBHgXQrW6+WOcM2bJh0xr8R4EWApGOZeSxkPr01isc1s4s1AZ+PWCcZVO3Rbi7\n\tJuk5R1fUytOwEUPC7SGr0rneHJr8jo/6DdSaGxcgfKpIqtAOlcOzwrgsdBW0dM0//e\n\tTam7AXEtlOXdg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1653634013;\n\tbh=E2hjMj8uIhcoteXPBZttcq5pFuzw8aS7pwD6DrDzwJw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=PsMLpYvVs/TkHEShLGIWyX7HvO+uo+5SrmrTrp73DkdzrRHuTlDjsjHu7M3aWsO1e\n\tCH7wcPfifBW8wu+tm7JURnD7TE8e1PXE0YsnfYlX3D+UGiZU+M9gqS57/IZDEsbSqt\n\tGNDxsxp6LRWVZ96th0IWFBM/0o45CbhI2yXH0A00="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"PsMLpYvV\"; dkim-atps=neutral","Date":"Fri, 27 May 2022 15:46:45 +0900","To":"Laurent Pinchart ","Message-ID":"<20220527064645.GH4117012@pyrite.rasen.tech>","References":"<20220523092435.475510-1-fsylvestre@baylibre.com>\n\t<20220523092435.475510-3-fsylvestre@baylibre.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [libcamera-devel 2/5] ipa: libipa: Add init()\n\tfunction to the Algorithm class","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":"Paul Elder via libcamera-devel ","Reply-To":"paul.elder@ideasonboard.com","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23361,"web_url":"https://patchwork.libcamera.org/comment/23361/","msgid":"","date":"2022-06-08T22:08:08","subject":"Re: [libcamera-devel] [libcamera-devel 2/5] ipa: libipa: Add init()\n\tfunction to the Algorithm class","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Paul,\n\nOn Fri, May 27, 2022 at 03:46:45PM +0900, paul.elder@ideasonboard.com wrote:\n> On Thu, May 26, 2022 at 02:19:39AM +0300, Laurent Pinchart via libcamera-devel wrote:\n> > Hi Florian,\n> > \n> > Thank you for the patch.\n> > \n> > By the way, you don't need to add \"libcamera-devel\" manually to the\n> > subject line, the [libcamera-devel] tag is added by the mailing list.\n> > \n> > On Mon, May 23, 2022 at 11:24:32AM +0200, Florian Sylvestre via libcamera-devel wrote:\n> > > Add the init() function that will be called during algorithm initialization\n> > > to provide each algorithm the list of algorithms tuning data.\n> > > Each algorithm will be responsible to grab their corresponding parameters.\n> > > \n> > > Signed-off-by: Florian Sylvestre \n> > > ---\n> > > src/ipa/libipa/algorithm.cpp | 15 +++++++++++++++\n> > > src/ipa/libipa/algorithm.h | 10 +++++++++-\n> > > 2 files changed, 24 insertions(+), 1 deletion(-)\n> > > \n> > > diff --git a/src/ipa/libipa/algorithm.cpp b/src/ipa/libipa/algorithm.cpp\n> > > index 398d5372..269a4beb 100644\n> > > --- a/src/ipa/libipa/algorithm.cpp\n> > > +++ b/src/ipa/libipa/algorithm.cpp\n> > > @@ -29,6 +29,21 @@ namespace ipa {\n> > > * to manage algorithms regardless of their specific type.\n> > > */\n> > > \n> > > +/**\n> > > + * \\fn Algorithm::init()\n> > > + * \\brief Configure the Algorithm with default parameters\n> > > + * \\param[in] context The shared IPA context\n> > > + * \\param[in] params The initial parameters used to tune algorithms\n> > > + *\n> > > + * This function is called once before the camera is running to get default\n> > > + * algorithm parameters.\n> > \n> > This may be understood as meaning it's called once every time the camera\n> > is started. I'd make this a bit clearer:\n> > \n> > * This function is called once, when the IPA module is initialized, to\n> > * initialize the algorithm. The \\a params YamlObject contains IPA module\n> > * parameters, typically tuning data for all algorithms. The Algorithm is\n> > * responsible for reading the parameters relevant to its configuration.\n> \n> Yeah that's a good clarification.\n> \n> > > + *\n> > > + * Algorithms are responsible to read the parameters given and extract their\n> > > + * parameter configuration.\n> > > + *\n> > > + * \\return 0 if successful, an error code otherwise\n> > > + */\n> > > +\n> > > /**\n> > > * \\fn Algorithm::configure()\n> > > * \\brief Configure the Algorithm given an IPAConfigInfo\n> > > diff --git a/src/ipa/libipa/algorithm.h b/src/ipa/libipa/algorithm.h\n> > > index 766aee5d..f5be1caf 100644\n> > > --- a/src/ipa/libipa/algorithm.h\n> > > +++ b/src/ipa/libipa/algorithm.h\n> > > @@ -10,12 +10,20 @@ namespace libcamera {\n> > > \n> > > namespace ipa {\n> > > \n> > > -template\n> > > +template > \n> > As we're trying to push for YAML usage everywhere through libcamera, I'm\n> > tempted to drop the additional template argument and use YamlObject\n> > unconditionally in init() instead of TuningData.\n> > \n> > Does anyone have an opinion on this ?\n> \n> On one hand, since all information will be representable in a yaml file,\n> it could be good to unify the tuning data into a YamlObject in\n> Algorithm.\n> \n> On the other hand, I think that it's not nice for algorithms to have to\n> deal with yaml directly. As in, the parser could convert the yaml values\n> into a \"proper\" data type that can be operated on.\n\nHow would you envision this ? A set of structures that would be specific\nto particular algorithms (and to a particular IPA module) ? If so,\nsomeone would need to convert the YAML data to that structure, and\nthat's exactly what the Algorithm::init() function does :-) If we were\nto split that out of the algorithm implementation, wouldn't we end up\nwith code that is algorithm-specific but would live elsewhere ?\n\n> Back to the first hand, the tuning file is only parsed and operated on\n> at init time, so the convenience of handling proper data types vs raw\n> yaml values isn't too vital.\n> \n> Back to the second hand, maybe for tuning parameters that are common\n> across platforms could have a unified parser and converter in libipa.\n\nThat's possible, although I wouldn't expect it to be very common. In any\ncase, the Algorithm::init() function could call helpers from libipa if\nneeded.\n\n> After presenting the hands, I suppose I have to make a choice to be my\n> opinion... I'd say the second hand. tbh I forsee large tuning\n> parameters, and although they don't need to be operated on, I think\n> they'd be converted from yaml to parameters in the same way across\n> different platforms. So instead of extracting each value with get<>()\n> (like I see in 5/5), we could have a convenience extractor in libipa,\n> eventually.\n> \n> That's just my two cents.\n> \n> And so for that reason, with the wording fix, I think it's good to go:\n> \n> Reviewed-by: Paul Elder \n> \n> > > +\t typename Config, typename Params, typename Stats>\n> > > +\n> > > class Algorithm\n> > > {\n> > > public:\n> > > \tvirtual ~Algorithm() {}\n> > > \n> > > +\tvirtual int init([[maybe_unused]] Context &context,\n> > > +\t\t\t [[maybe_unused]] const TuningData *params)\n> > > +\t{\n> > > +\t\treturn 0;\n> > > +\t}\n> > > +\n> > > \tvirtual int configure([[maybe_unused]] Context &context,\n> > > \t\t\t [[maybe_unused]] const Config &configInfo)\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 0A950BD161\n\tfor ;\n\tWed, 8 Jun 2022 22:08:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6502865633;\n\tThu, 9 Jun 2022 00:08:15 +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 371F2633A5\n\tfor ;\n\tThu, 9 Jun 2022 00:08:14 +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 950646CF;\n\tThu, 9 Jun 2022 00:08:13 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1654726095;\n\tbh=OvJVF/aQAlZhmD3uwfVu+6z9E6RmJjGlfkMc2z+59m4=;\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=mQIE4YEom3E7FRVxLAI/KRKBsSuvAaBLhig/WTW7YJXur2xTwcyBphiZzUf0oMJ2d\n\tEj7aZYRssK8BpYZzdtKuJK52PT7r8T//vDtUezKTsnFDqRhR1C9+EuApkydXo7VNqx\n\tqiGnNxVrWdryj4pd0zIbWJSc7a5C5K9wI64Z1kfsJl6S0QlQRkLtSzEwz5UpUcTACQ\n\tx3BP3DQlWNM3ovPKl45jTtJR9x7ER1vBrgnN4PIIKUgmYBevuy+0SUJeQ7Xy+nf+SN\n\tlhrb1rTI2qwqYsZMIOgrcKrkHZuJuq3/3tpQec1fkVWf2lLXJMg6Q+smwPspnEw9/m\n\ttz8BxMiuSsgug==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1654726093;\n\tbh=OvJVF/aQAlZhmD3uwfVu+6z9E6RmJjGlfkMc2z+59m4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Bo9m0kdjpANpEWEvcn340cAJ7WjLLnG/CRlCQ98js/1EWFL2MhtsAIUGGrF6JTwpX\n\tEwWSdJhySd/x3+Px8dXoNHn1qNSTnPlynn9z9O9+9K/lyNzBksTLLH9NBpMCQ9rS8k\n\tdCNQVy0qDVS6QYHLr9zjhYw5MO2AbOaq+qQczQ00="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"Bo9m0kdj\"; dkim-atps=neutral","Date":"Thu, 9 Jun 2022 01:08:08 +0300","To":"paul.elder@ideasonboard.com","Message-ID":"","References":"<20220523092435.475510-1-fsylvestre@baylibre.com>\n\t<20220523092435.475510-3-fsylvestre@baylibre.com>\n\t\n\t<20220527064645.GH4117012@pyrite.rasen.tech>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220527064645.GH4117012@pyrite.rasen.tech>","Subject":"Re: [libcamera-devel] [libcamera-devel 2/5] ipa: libipa: Add init()\n\tfunction to the Algorithm class","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":23426,"web_url":"https://patchwork.libcamera.org/comment/23426/","msgid":"<20220616095157.GA2704142@pyrite.rasen.tech>","date":"2022-06-16T09:51:57","subject":"Re: [libcamera-devel] [libcamera-devel 2/5] ipa: libipa: Add init()\n\tfunction to the Algorithm class","submitter":{"id":97,"url":"https://patchwork.libcamera.org/api/people/97/","name":"Nicolas Dufresne via libcamera-devel","email":"libcamera-devel@lists.libcamera.org"},"content":"Hi Laurent,\n\nOn Thu, Jun 09, 2022 at 01:08:08AM +0300, Laurent Pinchart wrote:\n> Hi Paul,\n> \n> On Fri, May 27, 2022 at 03:46:45PM +0900, paul.elder@ideasonboard.com wrote:\n> > On Thu, May 26, 2022 at 02:19:39AM +0300, Laurent Pinchart via libcamera-devel wrote:\n> > > Hi Florian,\n> > > \n> > > Thank you for the patch.\n> > > \n> > > By the way, you don't need to add \"libcamera-devel\" manually to the\n> > > subject line, the [libcamera-devel] tag is added by the mailing list.\n> > > \n> > > On Mon, May 23, 2022 at 11:24:32AM +0200, Florian Sylvestre via libcamera-devel wrote:\n> > > > Add the init() function that will be called during algorithm initialization\n> > > > to provide each algorithm the list of algorithms tuning data.\n> > > > Each algorithm will be responsible to grab their corresponding parameters.\n> > > > \n> > > > Signed-off-by: Florian Sylvestre \n> > > > ---\n> > > > src/ipa/libipa/algorithm.cpp | 15 +++++++++++++++\n> > > > src/ipa/libipa/algorithm.h | 10 +++++++++-\n> > > > 2 files changed, 24 insertions(+), 1 deletion(-)\n> > > > \n> > > > diff --git a/src/ipa/libipa/algorithm.cpp b/src/ipa/libipa/algorithm.cpp\n> > > > index 398d5372..269a4beb 100644\n> > > > --- a/src/ipa/libipa/algorithm.cpp\n> > > > +++ b/src/ipa/libipa/algorithm.cpp\n> > > > @@ -29,6 +29,21 @@ namespace ipa {\n> > > > * to manage algorithms regardless of their specific type.\n> > > > */\n> > > > \n> > > > +/**\n> > > > + * \\fn Algorithm::init()\n> > > > + * \\brief Configure the Algorithm with default parameters\n> > > > + * \\param[in] context The shared IPA context\n> > > > + * \\param[in] params The initial parameters used to tune algorithms\n> > > > + *\n> > > > + * This function is called once before the camera is running to get default\n> > > > + * algorithm parameters.\n> > > \n> > > This may be understood as meaning it's called once every time the camera\n> > > is started. I'd make this a bit clearer:\n> > > \n> > > * This function is called once, when the IPA module is initialized, to\n> > > * initialize the algorithm. The \\a params YamlObject contains IPA module\n> > > * parameters, typically tuning data for all algorithms. The Algorithm is\n> > > * responsible for reading the parameters relevant to its configuration.\n> > \n> > Yeah that's a good clarification.\n> > \n> > > > + *\n> > > > + * Algorithms are responsible to read the parameters given and extract their\n> > > > + * parameter configuration.\n> > > > + *\n> > > > + * \\return 0 if successful, an error code otherwise\n> > > > + */\n> > > > +\n> > > > /**\n> > > > * \\fn Algorithm::configure()\n> > > > * \\brief Configure the Algorithm given an IPAConfigInfo\n> > > > diff --git a/src/ipa/libipa/algorithm.h b/src/ipa/libipa/algorithm.h\n> > > > index 766aee5d..f5be1caf 100644\n> > > > --- a/src/ipa/libipa/algorithm.h\n> > > > +++ b/src/ipa/libipa/algorithm.h\n> > > > @@ -10,12 +10,20 @@ namespace libcamera {\n> > > > \n> > > > namespace ipa {\n> > > > \n> > > > -template\n> > > > +template > > \n> > > As we're trying to push for YAML usage everywhere through libcamera, I'm\n> > > tempted to drop the additional template argument and use YamlObject\n> > > unconditionally in init() instead of TuningData.\n> > > \n> > > Does anyone have an opinion on this ?\n> > \n> > On one hand, since all information will be representable in a yaml file,\n> > it could be good to unify the tuning data into a YamlObject in\n> > Algorithm.\n> > \n> > On the other hand, I think that it's not nice for algorithms to have to\n> > deal with yaml directly. As in, the parser could convert the yaml values\n> > into a \"proper\" data type that can be operated on.\n> \n> How would you envision this ? A set of structures that would be specific\n> to particular algorithms (and to a particular IPA module) ? If so,\n\ntbh, yeah, that's what I was envisioning.\n\n> someone would need to convert the YAML data to that structure, and\n> that's exactly what the Algorithm::init() function does :-) If we were\n> to split that out of the algorithm implementation, wouldn't we end up\n> with code that is algorithm-specific but would live elsewhere ?\n\nYeah, so probably the first hand is better.\n\n> \n> > Back to the first hand, the tuning file is only parsed and operated on\n> > at init time, so the convenience of handling proper data types vs raw\n> > yaml values isn't too vital.\n> > \n> > Back to the second hand, maybe for tuning parameters that are common\n> > across platforms could have a unified parser and converter in libipa.\n> \n> That's possible, although I wouldn't expect it to be very common. In any\n> case, the Algorithm::init() function could call helpers from libipa if\n> needed.\n\nHm yeah I think you're right.\n\nAlright, let's go for the first hand then.\n\n\nPaul\n\n> \n> > After presenting the hands, I suppose I have to make a choice to be my\n> > opinion... I'd say the second hand. tbh I forsee large tuning\n> > parameters, and although they don't need to be operated on, I think\n> > they'd be converted from yaml to parameters in the same way across\n> > different platforms. So instead of extracting each value with get<>()\n> > (like I see in 5/5), we could have a convenience extractor in libipa,\n> > eventually.\n> > \n> > That's just my two cents.\n> > \n> > And so for that reason, with the wording fix, I think it's good to go:\n> > \n> > Reviewed-by: Paul Elder \n> > \n> > > > +\t typename Config, typename Params, typename Stats>\n> > > > +\n> > > > class Algorithm\n> > > > {\n> > > > public:\n> > > > \tvirtual ~Algorithm() {}\n> > > > \n> > > > +\tvirtual int init([[maybe_unused]] Context &context,\n> > > > +\t\t\t [[maybe_unused]] const TuningData *params)\n> > > > +\t{\n> > > > +\t\treturn 0;\n> > > > +\t}\n> > > > +\n> > > > \tvirtual int configure([[maybe_unused]] Context &context,\n> > > > \t\t\t [[maybe_unused]] const Config &configInfo)\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 CB7D6BD808\n\tfor ;\n\tThu, 16 Jun 2022 09:52:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9076865635;\n\tThu, 16 Jun 2022 11:52:07 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0B25D601F1\n\tfor ;\n\tThu, 16 Jun 2022 11:52:06 +0200 (CEST)","from pyrite.rasen.tech (softbank036240126034.bbtec.net\n\t[36.240.126.34])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 42A9A415;\n\tThu, 16 Jun 2022 11:52:03 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1655373127;\n\tbh=6h67Zh3ysmKChYtiiNLVEn54rS5uTRHUBS/R1zdbkRU=;\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=KPjxNlDz7V5IWPD81bP1twLSq64rAa7SshUIdFHPgPHOhQQ+CzDUdRg7Zut0L5loV\n\tzzLzmfMSl0otuZnvpTSECaWXVLOYtM76ksteQ9Z/eNU3ixjjF/3ry14+859ofub053\n\tvxHqk8Cr5osHMPmAd3P56Avd39aBWC1+P1kCOq9XfFE8FrIfvmX1AG0Gb+C2uRd7J8\n\t8MdAsEei69V9ztI78G7IxNkd9+bbv1JjrJxBBfawKM+EN3mPqtSGsLxW+SyhFM3P39\n\tMZemYs0Ozm1icmhAzAi2GRF1qo/rbRkxXY0Gg0oC11rL9YHM/r7lNOR7SKFtWHBIf9\n\tW0Ua9Kon/8i0g==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1655373125;\n\tbh=6h67Zh3ysmKChYtiiNLVEn54rS5uTRHUBS/R1zdbkRU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=TGBjTVpAHi+oJDiemHb666ebGfn+Ze0z+n+Zx7QtKkH9QKPADK7SIGA/V+nd9kRFY\n\tZSfIGIqiKp3mN8xdzmjM01yKclXSq//1ZCnvJR3CxJu6xTpR06kZ8rmgsDUnA/e+l7\n\tYDE1Q32wORU4xodkrIBFUahWUBR7+cV169pzdkm8="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"TGBjTVpA\"; dkim-atps=neutral","Date":"Thu, 16 Jun 2022 18:51:57 +0900","To":"Laurent Pinchart ","Message-ID":"<20220616095157.GA2704142@pyrite.rasen.tech>","References":"<20220523092435.475510-1-fsylvestre@baylibre.com>\n\t<20220523092435.475510-3-fsylvestre@baylibre.com>\n\t\n\t<20220527064645.GH4117012@pyrite.rasen.tech>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [libcamera-devel 2/5] ipa: libipa: Add init()\n\tfunction to the Algorithm class","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":"Paul Elder via libcamera-devel ","Reply-To":"paul.elder@ideasonboard.com","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]