[{"id":39634,"web_url":"https://patchwork.libcamera.org/comment/39634/","msgid":"<85a4s1el70.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","date":"2026-07-08T12:10:27","subject":"Re: [PATCH v4 11/35] ipa: libipa: lsc_polynomial: Don't inline\n\tValueNode accessor","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Hi,\n\nJacopo Mondi writes:\n\n> From: Laurent Pinchart \n>\n> There's no need to inline the ValueNode::Accessor::get() function for\n> LscPolynomial. Move it to from lsc_polynomial.h to lsc_polynomial.cpp.\n>\n> While at it, replace C++-style comments with C-style comments according\n> to the libcamera coding style.\n>\n> Signed-off-by: Laurent Pinchart \n> Reviewed-by: Kieran Bingham \n> Reviewed-by: Jacopo Mondi \n> Signed-off-by: Jacopo Mondi \n> ---\n> src/ipa/libipa/lsc_polynomial.cpp | 26 ++++++++++++++++++++++++--\n> src/ipa/libipa/lsc_polynomial.h | 24 ------------------------\n> 2 files changed, 24 insertions(+), 26 deletions(-)\n>\n> diff --git a/src/ipa/libipa/lsc_polynomial.cpp b/src/ipa/libipa/lsc_polynomial.cpp\n> index f607d86c54c3..00569c2fd160 100644\n> --- a/src/ipa/libipa/lsc_polynomial.cpp\n> +++ b/src/ipa/libipa/lsc_polynomial.cpp\n> @@ -77,5 +77,27 @@ namespace ipa {\n> * \\param size The size of the reference image\n> */\n> \n> -} // namespace ipa\n> -} // namespace libcamera\n> +} /* namespace ipa */\n> +\n> +#ifndef __DOXYGEN__\n> +template<>\n> +std::optional\n> +ValueNode::Accessor::get(const ValueNode &obj) const\n> +{\n> +\tstd::optional cx = obj[\"cx\"].get();\n> +\tstd::optional cy = obj[\"cy\"].get();\n> +\tstd::optional k0 = obj[\"k0\"].get();\n> +\tstd::optional k1 = obj[\"k1\"].get();\n> +\tstd::optional k2 = obj[\"k2\"].get();\n> +\tstd::optional k3 = obj[\"k3\"].get();\n> +\tstd::optional k4 = obj[\"k4\"].get();\n> +\n> +\tif (!(cx && cy && k0 && k1 && k2 && k3 && k4))\n> +\t\tLOG(LscPolynomial, Error)\n> +\t\t\t<< \"Polynomial is missing a parameter\";\n\nThis should return an empty value or do something about the missing\nparameters, otherwise the error is followed by a crash in the return\nstatement below.\n\nThis is not a fault of this patch but it would be worth to fix, here or elsewhere.\n\n> +\n> +\treturn ipa::LscPolynomial(*cx, *cy, *k0, *k1, *k2, *k3, *k4);\n> +}\n> +#endif /* __DOXYGEN__ */\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/ipa/libipa/lsc_polynomial.h b/src/ipa/libipa/lsc_polynomial.h\n> index d7d9ae42e360..d52ec86946b1 100644\n> --- a/src/ipa/libipa/lsc_polynomial.h\n> +++ b/src/ipa/libipa/lsc_polynomial.h\n> @@ -80,28 +80,4 @@ private:\n> \n> } /* namespace ipa */\n> \n> -#ifndef __DOXYGEN__\n> -\n> -template<>\n> -struct ValueNode::Accessor {\n> -\tstd::optional get(const ValueNode &obj) const\n> -\t{\n> -\t\tstd::optional cx = obj[\"cx\"].get();\n> -\t\tstd::optional cy = obj[\"cy\"].get();\n> -\t\tstd::optional k0 = obj[\"k0\"].get();\n> -\t\tstd::optional k1 = obj[\"k1\"].get();\n> -\t\tstd::optional k2 = obj[\"k2\"].get();\n> -\t\tstd::optional k3 = obj[\"k3\"].get();\n> -\t\tstd::optional k4 = obj[\"k4\"].get();\n> -\n> -\t\tif (!(cx && cy && k0 && k1 && k2 && k3 && k4))\n> -\t\t\tLOG(LscPolynomial, Error)\n> -\t\t\t\t<< \"Polynomial is missing a parameter\";\n> -\n> -\t\treturn ipa::LscPolynomial(*cx, *cy, *k0, *k1, *k2, *k3, *k4);\n> -\t}\n> -};\n> -\n> -#endif\n> -\n> } /* namespace libcamera */","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 88CFAC3306\n\tfor ;\n\tWed, 8 Jul 2026 12:10:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7585766084;\n\tWed, 8 Jul 2026 14:10:36 +0200 (CEST)","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 3BD4F66003\n\tfor ;\n\tWed, 8 Jul 2026 14:10:35 +0200 (CEST)","from mail-wm1-f70.google.com (mail-wm1-f70.google.com\n\t[209.85.128.70]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-303-lW7IppUTOQatI0LsFdTcKg-1; Wed, 08 Jul 2026 08:10:33 -0400","by mail-wm1-f70.google.com with SMTP id\n\t5b1f17b1804b1-493b786d550so6085755e9.0\n\tfor ;\n\tWed, 08 Jul 2026 05:10:32 -0700 (PDT)","from mzamazal-thinkpadp1gen7.tpbc.csb\n\t(ip-77-48-47-4.net.vodafone.cz. [77.48.47.4])\n\tby smtp.gmail.com with ESMTPSA id\n\t5b1f17b1804b1-493e0f40d50sm156749985e9.5.2026.07.08.05.10.30\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tWed, 08 Jul 2026 05:10:30 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"SBPDWvUs\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1783512634;\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=0tiHGYWdlNjDpSb5fYrW7P+FjIoetMlbNkwxH0pwA34=;\n\tb=SBPDWvUsv/+SNj91ZK5AN/l0u2UV61DZ4SkSDTg/Ndy6MmK/+HYu7Gssbfls0V0dsky4OU\n\t/XLFcebBDv1uNRx92dZbAy1Y/YX5lUMLEmaqDpJSqrMLOVEZ3YTkbaXU6zoX6v1UrOpGfG\n\tyHk3HmMz6xBs0JjwaRBaBdjdxtFLTV0=","X-MC-Unique":"lW7IppUTOQatI0LsFdTcKg-1","X-Mimecast-MFC-AGG-ID":"lW7IppUTOQatI0LsFdTcKg_1783512632","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20251104; t=1783512632; x=1784117432;\n\th=mime-version:user-agent:message-id:date:references:in-reply-to\n\t:subject:cc:to:from:x-gm-gg:x-gm-message-state:from:to:cc:subject\n\t:date:message-id:reply-to;\n\tbh=0tiHGYWdlNjDpSb5fYrW7P+FjIoetMlbNkwxH0pwA34=;\n\tb=qW/2UXbmh1VlReZloqETaSHf7+TqtADfGLa/92SjJFd+yvGUFOr4L9ft0ukPLZXCvv\n\thVzijopR8/RyuRE3SlROz5ymASdPNmukQU4Z6D7/eWhszAYtiTTkziCg4fh8knpfnWmG\n\tcaqPSBXjPJ+PrU3DtzpMx73Zh3pX5+lW9zxF1xAupVHlASlUz92f5VPTP4d8qIO7DzDR\n\tNlUb2k8LcFGb0fxHK/jSn1CKzxc2XJwekZpqWXvGoDVoBPsrhkxuKotm8Vngw5dCjwTW\n\tfWNn6Ysk99XMra4L64K0zt+3Lb86KLrtC8yyYdA8Vaj5CSghJLd/aolJlxeUz8UE2/IT\n\tSL1g==","X-Gm-Message-State":"AOJu0YwDjTqPwXBypULBFAh9z+PNKJFm/dxeLMbccQYZOx4siOCw+DUQ\n\tNCuWWLiF+9nfSyCbhqrSUzTgEiM1gQeaTqZFq8VJL/Cb2IrgyBN+XwnuQQ0DMvPp0ntOqjsOEfS\n\taBqtQk8vtBYDgIk8Y3VrGBRQ5d5YUPiEmtxpAxo5jPj2QSDcZQUYVXh9fqFbvE8TMVoqyMv4qgU\n\tY=","X-Gm-Gg":"AfdE7cnVAU5+skVOVNIt4Tb04MGTtSHZs/qtghsb3vqD99m1eWPXGM4zs7NPL7svtWH\n\tgA6axu1bWkpICSe6XhQGOwqZuvd4TRdOeIwKutSpkCi541iRxk5l0KM66ScNOTlz5DhKriMAxtx\n\tsFrG4aRRiH2MJcuZN8HK4fzADbmyWDP+DxS+wn9w2pAoLhwXy40xJK61rZB99MOn+9rJLCm7wKM\n\tCf0Ut22694ApxCwvQOHnu/lmQ4PBYJQJG8XfPfdq05YG5kdRtdQpoBYul6i2BTOReMi+hE+Qjk5\n\tDpO0NrT+BRLmo80wLA4Li1E6SsMkznRMx2N4sDUWSTa29wd0JneW0HJUS7y20DoLXaLWNfQWesq\n\twkieN1CN60CjYrkYUKViOWajqFoF8rHwPTOlvnCwZ/4DbBMZWeA==","X-Received":["by 2002:a05:600c:3b10:b0:493:a7fd:15d6 with SMTP id\n\t5b1f17b1804b1-493e689b2d8mr26210535e9.9.1783512631717; \n\tWed, 08 Jul 2026 05:10:31 -0700 (PDT)","by 2002:a05:600c:3b10:b0:493:a7fd:15d6 with SMTP id\n\t5b1f17b1804b1-493e689b2d8mr26210295e9.9.1783512631336; \n\tWed, 08 Jul 2026 05:10:31 -0700 (PDT)"],"From":"Milan Zamazal ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org, Stefan Klug\n\t, Laurent Pinchart\n\t, Kieran Bingham\n\t","Subject":"Re: [PATCH v4 11/35] ipa: libipa: lsc_polynomial: Don't inline\n\tValueNode accessor","In-Reply-To":"<20260708-libipa-algorithms-v4-11-f0e99f035294@ideasonboard.com>\n\t(Jacopo Mondi's message of \"Wed, 08 Jul 2026 11:24:58 +0200\")","References":"<20260708-libipa-algorithms-v4-0-f0e99f035294@ideasonboard.com>\n\t<20260708-libipa-algorithms-v4-11-f0e99f035294@ideasonboard.com>","Date":"Wed, 08 Jul 2026 14:10:27 +0200","Message-ID":"<85a4s1el70.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","User-Agent":"Gnus/5.13 (Gnus v5.13)","MIME-Version":"1.0","X-Mimecast-Spam-Score":"0","X-Mimecast-MFC-PROC-ID":"U-4tErl1VfNlQ7FmgKYyJfb1aS079GQt98bm38iAHwY_1783512632","X-Mimecast-Originator":"redhat.com","Content-Type":"text/plain","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":39638,"web_url":"https://patchwork.libcamera.org/comment/39638/","msgid":"","date":"2026-07-08T13:46:08","subject":"Re: [PATCH v4 11/35] ipa: libipa: lsc_polynomial: Don't inline\n\tValueNode accessor","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Milan\n\nOn Wed, Jul 08, 2026 at 02:10:27PM +0200, Milan Zamazal wrote:\n> Hi,\n>\n> Jacopo Mondi writes:\n>\n> > From: Laurent Pinchart \n> >\n> > There's no need to inline the ValueNode::Accessor::get() function for\n> > LscPolynomial. Move it to from lsc_polynomial.h to lsc_polynomial.cpp.\n> >\n> > While at it, replace C++-style comments with C-style comments according\n> > to the libcamera coding style.\n> >\n> > Signed-off-by: Laurent Pinchart \n> > Reviewed-by: Kieran Bingham \n> > Reviewed-by: Jacopo Mondi \n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/ipa/libipa/lsc_polynomial.cpp | 26 ++++++++++++++++++++++++--\n> > src/ipa/libipa/lsc_polynomial.h | 24 ------------------------\n> > 2 files changed, 24 insertions(+), 26 deletions(-)\n> >\n> > diff --git a/src/ipa/libipa/lsc_polynomial.cpp b/src/ipa/libipa/lsc_polynomial.cpp\n> > index f607d86c54c3..00569c2fd160 100644\n> > --- a/src/ipa/libipa/lsc_polynomial.cpp\n> > +++ b/src/ipa/libipa/lsc_polynomial.cpp\n> > @@ -77,5 +77,27 @@ namespace ipa {\n> > * \\param size The size of the reference image\n> > */\n> >\n> > -} // namespace ipa\n> > -} // namespace libcamera\n> > +} /* namespace ipa */\n> > +\n> > +#ifndef __DOXYGEN__\n> > +template<>\n> > +std::optional\n> > +ValueNode::Accessor::get(const ValueNode &obj) const\n> > +{\n> > +\tstd::optional cx = obj[\"cx\"].get();\n> > +\tstd::optional cy = obj[\"cy\"].get();\n> > +\tstd::optional k0 = obj[\"k0\"].get();\n> > +\tstd::optional k1 = obj[\"k1\"].get();\n> > +\tstd::optional k2 = obj[\"k2\"].get();\n> > +\tstd::optional k3 = obj[\"k3\"].get();\n> > +\tstd::optional k4 = obj[\"k4\"].get();\n> > +\n> > +\tif (!(cx && cy && k0 && k1 && k2 && k3 && k4))\n> > +\t\tLOG(LscPolynomial, Error)\n> > +\t\t\t<< \"Polynomial is missing a parameter\";\n>\n> This should return an empty value or do something about the missing\n> parameters, otherwise the error is followed by a crash in the return\n> statement below.\n>\n> This is not a fault of this patch but it would be worth to fix, here or elsewhere.\n>\n\nThanks for pointing this out\n\nIt should be enough to:\n\n--- a/src/ipa/libipa/lsc_polynomial.cpp\n+++ b/src/ipa/libipa/lsc_polynomial.cpp\n@@ -229,9 +229,11 @@ ValueNode::Accessor::get(const ValueNode &obj) const\n std::optional k3 = obj[\"k3\"].get();\n std::optional k4 = obj[\"k4\"].get();\n\n- if (!(cx && cy && k0 && k1 && k2 && k3 && k4))\n+ if (!(cx && cy && k0 && k1 && k2 && k3 && k4)) {\n LOG(LscPolynomial, Error)\n << \"Polynomial is missing a parameter\";\n+ return std::nullopt;\n+ }\n\n return ipa::lsc::Polynomial(*cx, *cy, *k0, *k1, *k2, *k3, *k4);\n }\n\nThe caller already checks\n\n auto polynomial = set[k.c_str()].get();\n if (!polynomial) {\n LOG(LscPolynomial, Error)\n << \"Missing polynomial for component \"\n << k;\n return -EINVAL;\n }\n\nWhich should guarantee parsing fails if a polynomial component is\nmissing.\n\nI'll add a patch to the series\n\nThanks\n j\n\n> > +\n> > +\treturn ipa::LscPolynomial(*cx, *cy, *k0, *k1, *k2, *k3, *k4);\n> > +}\n> > +#endif /* __DOXYGEN__ */\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/src/ipa/libipa/lsc_polynomial.h b/src/ipa/libipa/lsc_polynomial.h\n> > index d7d9ae42e360..d52ec86946b1 100644\n> > --- a/src/ipa/libipa/lsc_polynomial.h\n> > +++ b/src/ipa/libipa/lsc_polynomial.h\n> > @@ -80,28 +80,4 @@ private:\n> >\n> > } /* namespace ipa */\n> >\n> > -#ifndef __DOXYGEN__\n> > -\n> > -template<>\n> > -struct ValueNode::Accessor {\n> > -\tstd::optional get(const ValueNode &obj) const\n> > -\t{\n> > -\t\tstd::optional cx = obj[\"cx\"].get();\n> > -\t\tstd::optional cy = obj[\"cy\"].get();\n> > -\t\tstd::optional k0 = obj[\"k0\"].get();\n> > -\t\tstd::optional k1 = obj[\"k1\"].get();\n> > -\t\tstd::optional k2 = obj[\"k2\"].get();\n> > -\t\tstd::optional k3 = obj[\"k3\"].get();\n> > -\t\tstd::optional k4 = obj[\"k4\"].get();\n> > -\n> > -\t\tif (!(cx && cy && k0 && k1 && k2 && k3 && k4))\n> > -\t\t\tLOG(LscPolynomial, Error)\n> > -\t\t\t\t<< \"Polynomial is missing a parameter\";\n> > -\n> > -\t\treturn ipa::LscPolynomial(*cx, *cy, *k0, *k1, *k2, *k3, *k4);\n> > -\t}\n> > -};\n> > -\n> > -#endif\n> > -\n> > } /* namespace libcamera */\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 8C4CBC3306\n\tfor ;\n\tWed, 8 Jul 2026 13:46:13 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 85A4666089;\n\tWed, 8 Jul 2026 15:46:12 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7317965FF1\n\tfor ;\n\tWed, 8 Jul 2026 15:46:11 +0200 (CEST)","from ideasonboard.com (93-46-82-201.ip106.fastwebnet.it\n\t[93.46.82.201])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 40023EAA;\n\tWed, 8 Jul 2026 15:45:21 +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=\"jHXXIMtG\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1783518321;\n\tbh=rYgcn6CMZQ4AptGgu+FlQWa3qOsXYpLnSGzzrL6V0nw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=jHXXIMtGRqZL4jqAeNO9UFn35i0wjzuAorGfZW+HzTogJ2vFfLaXKlOfOyxARlME0\n\tb2u+FGUAsD1hONqMiYDf4SrxGoWbmeY9Q0E6YuCPlRtjr+7d/LpyM2sL9Rw2JdMTrA\n\tKpuXbNa4GlOkjzz3kFpFre1xluXrtVULhVki3330=","Date":"Wed, 8 Jul 2026 15:46:08 +0200","From":"Jacopo Mondi ","To":"Milan Zamazal ","Cc":"Jacopo Mondi , \n\tlibcamera-devel@lists.libcamera.org,\n\tStefan Klug , \n\tLaurent Pinchart ,\n\tKieran Bingham ","Subject":"Re: [PATCH v4 11/35] ipa: libipa: lsc_polynomial: Don't inline\n\tValueNode accessor","Message-ID":"","References":"<20260708-libipa-algorithms-v4-0-f0e99f035294@ideasonboard.com>\n\t<20260708-libipa-algorithms-v4-11-f0e99f035294@ideasonboard.com>\n\t<85a4s1el70.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<85a4s1el70.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]