[{"id":32782,"web_url":"https://patchwork.libcamera.org/comment/32782/","msgid":"<75yxziw6oyqlfpyy4kn6awa5gsbcadmrewgmi4qin4gfq4azlr@6dcefgysdzni>","date":"2024-12-16T18:32:24","subject":"Re: [PATCH v4 15/20] pipeline: rkisp1: Enable the dewarper\n\tunconditionally","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Stefan\n\nOn Mon, Dec 16, 2024 at 04:40:55PM +0100, Stefan Klug wrote:\n> In configure() and in the future in generateConfiguration() the\n> calculated stream sizes and crop rectangles depend on the dewarper being\n> used or not. It is therefore not possible to postpone that decision\n> until the dewarper gets configured. Enable the dewarper unconditionally\n> if it is found and the stream type is not RAW.\n\nAm I wrong or the commit message doesn't match the patch ?\n\n>\n> Signed-off-by: Stefan Klug \n> Signed-off-by: Jacopo Mondi \n> Reviewed-by: Paul Elder \n>\n> ---\n>\n> Changes in v4:\n> - Collected tags\n> ---\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 5 ++++-\n> 1 file changed, 4 insertions(+), 1 deletion(-)\n>\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index 18038226912a..14d4bb9a929b 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -865,7 +865,10 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n> \t\t\tif (dewarper_ && !isRaw_) {\n> \t\t\t\toutputCfgs.push_back(const_cast(cfg));\n> \t\t\t\tret = dewarper_->configure(cfg, outputCfgs);\n> -\t\t\t\tuseDewarper_ = ret ? false : true;\n> +\t\t\t\tif (ret)\n> +\t\t\t\t\treturn ret;\n> +\n> +\t\t\t\tuseDewarper_ = true;\n>\n> \t\t\t\t/*\n> \t\t\t\t * Calculate the crop rectangle of the data\n> --\n> 2.43.0\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 4534CC326C\n\tfor ;\n\tMon, 16 Dec 2024 18:32:30 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 88CE267FA4;\n\tMon, 16 Dec 2024 19:32:29 +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 C91D767F93\n\tfor ;\n\tMon, 16 Dec 2024 19:32:27 +0100 (CET)","from ideasonboard.com (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id DAD8F160;\n\tMon, 16 Dec 2024 19:31:50 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"h4q6rZBd\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1734373911;\n\tbh=VN99TzYM9fn/n7xFaBSWrsjY0SGLdHyqzz8a9lg+LmU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=h4q6rZBd4BIE30E+X2wUhOwpcOZVm9ln4TiplLzAo4JIJeIUbk3h3O/Yelghj9S2X\n\tzKaIt1PGfxgfgjA8L4lEQviWXGdXhdkWKOpxQUyHI2hE/E4WUw2Xgfax0MOON25Ufv\n\toDGGsi/yiitA12l3FUzyWlQhw3AmmdIgnZnsTf6w=","Date":"Mon, 16 Dec 2024 19:32:24 +0100","From":"Jacopo Mondi ","To":"Stefan Klug ","Cc":"libcamera-devel@lists.libcamera.org, \n\tJacopo Mondi ,\n\tPaul Elder ","Subject":"Re: [PATCH v4 15/20] pipeline: rkisp1: Enable the dewarper\n\tunconditionally","Message-ID":"<75yxziw6oyqlfpyy4kn6awa5gsbcadmrewgmi4qin4gfq4azlr@6dcefgysdzni>","References":"<20241216154124.203650-1-stefan.klug@ideasonboard.com>\n\t<20241216154124.203650-16-stefan.klug@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20241216154124.203650-16-stefan.klug@ideasonboard.com>","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":32788,"web_url":"https://patchwork.libcamera.org/comment/32788/","msgid":"","date":"2024-12-16T20:30:15","subject":"Re: [PATCH v4 15/20] pipeline: rkisp1: Enable the dewarper\n\tunconditionally","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the review. \n\nOn Mon, Dec 16, 2024 at 07:32:24PM +0100, Jacopo Mondi wrote:\n> Hi Stefan\n> \n> On Mon, Dec 16, 2024 at 04:40:55PM +0100, Stefan Klug wrote:\n> > In configure() and in the future in generateConfiguration() the\n> > calculated stream sizes and crop rectangles depend on the dewarper being\n> > used or not. It is therefore not possible to postpone that decision\n> > until the dewarper gets configured. Enable the dewarper unconditionally\n> > if it is found and the stream type is not RAW.\n> \n> Am I wrong or the commit message doesn't match the patch ?\n\nHm, no that was really the message for that change. To me it explains\nthe reasoning of the change. This patch was slimmed down by the internal\nfixup dd7f3d5a2436 (\"fixup! libcamera: rkisp1: Enable the dewarper\nunconditionally\") that you pushed to our common branch (which was\nabsolutely valid).\n\nDid I miss something?\n\nBest regards,\nStefan\n\n> \n> >\n> > Signed-off-by: Stefan Klug \n> > Signed-off-by: Jacopo Mondi \n> > Reviewed-by: Paul Elder \n> >\n> > ---\n> >\n> > Changes in v4:\n> > - Collected tags\n> > ---\n> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 5 ++++-\n> > 1 file changed, 4 insertions(+), 1 deletion(-)\n> >\n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > index 18038226912a..14d4bb9a929b 100644\n> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > @@ -865,7 +865,10 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n> > \t\t\tif (dewarper_ && !isRaw_) {\n> > \t\t\t\toutputCfgs.push_back(const_cast(cfg));\n> > \t\t\t\tret = dewarper_->configure(cfg, outputCfgs);\n> > -\t\t\t\tuseDewarper_ = ret ? false : true;\n> > +\t\t\t\tif (ret)\n> > +\t\t\t\t\treturn ret;\n> > +\n> > +\t\t\t\tuseDewarper_ = true;\n> >\n> > \t\t\t\t/*\n> > \t\t\t\t * Calculate the crop rectangle of the data\n> > --\n> > 2.43.0\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 7E486C32F6\n\tfor ;\n\tMon, 16 Dec 2024 20:30:19 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AE11167FAD;\n\tMon, 16 Dec 2024 21:30:18 +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 DE28062C8B\n\tfor ;\n\tMon, 16 Dec 2024 21:30:17 +0100 (CET)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:15e9:bfc7:2fd9:3f8a])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id ED833675;\n\tMon, 16 Dec 2024 21:29:40 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"ZVsA4OwK\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1734380981;\n\tbh=1HOXAontxxn5jvSWDROd0Dba3faq3KpEkqJvBvNp5PU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ZVsA4OwK+pGgSWSWQrmVXrL6yRS+hPNrCGYoptKmW/xGfhVCdHDn453Btk6ZfeAYO\n\tXj1wQfCtHmurh414x+ayEgfc6+7e4T/k3aBGVUHBOS2Nc1m297JnEC1o9Cg3nUJdTR\n\t79FQR0WN6bOaLeg1m2CQM+efKR4QDzyaZN5yTGS4=","Date":"Mon, 16 Dec 2024 21:30:15 +0100","From":"Stefan Klug ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org, \n\tPaul Elder ","Subject":"Re: [PATCH v4 15/20] pipeline: rkisp1: Enable the dewarper\n\tunconditionally","Message-ID":"","References":"<20241216154124.203650-1-stefan.klug@ideasonboard.com>\n\t<20241216154124.203650-16-stefan.klug@ideasonboard.com>\n\t<75yxziw6oyqlfpyy4kn6awa5gsbcadmrewgmi4qin4gfq4azlr@6dcefgysdzni>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<75yxziw6oyqlfpyy4kn6awa5gsbcadmrewgmi4qin4gfq4azlr@6dcefgysdzni>","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":32804,"web_url":"https://patchwork.libcamera.org/comment/32804/","msgid":"","date":"2024-12-17T07:56:18","subject":"Re: [PATCH v4 15/20] pipeline: rkisp1: Enable the dewarper\n\tunconditionally","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Stefan\n\nOn Mon, Dec 16, 2024 at 09:30:15PM +0100, Stefan Klug wrote:\n> Hi Jacopo,\n>\n> Thank you for the review.\n>\n> On Mon, Dec 16, 2024 at 07:32:24PM +0100, Jacopo Mondi wrote:\n> > Hi Stefan\n> >\n> > On Mon, Dec 16, 2024 at 04:40:55PM +0100, Stefan Klug wrote:\n> > > In configure() and in the future in generateConfiguration() the\n> > > calculated stream sizes and crop rectangles depend on the dewarper being\n> > > used or not. It is therefore not possible to postpone that decision\n> > > until the dewarper gets configured. Enable the dewarper unconditionally\n> > > if it is found and the stream type is not RAW.\n> >\n> > Am I wrong or the commit message doesn't match the patch ?\n>\n> Hm, no that was really the message for that change. To me it explains\n> the reasoning of the change. This patch was slimmed down by the internal\n> fixup dd7f3d5a2436 (\"fixup! libcamera: rkisp1: Enable the dewarper\n> unconditionally\") that you pushed to our common branch (which was\n> absolutely valid).\n\nThis was a partial revert, and what's left in the commit is\n\n>\n> Did I miss something?\n>\n> Best regards,\n> Stefan\n>\n> >\n> > >\n> > > Signed-off-by: Stefan Klug \n> > > Signed-off-by: Jacopo Mondi \n> > > Reviewed-by: Paul Elder \n> > >\n> > > ---\n> > >\n> > > Changes in v4:\n> > > - Collected tags\n> > > ---\n> > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 5 ++++-\n> > > 1 file changed, 4 insertions(+), 1 deletion(-)\n> > >\n> > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > index 18038226912a..14d4bb9a929b 100644\n> > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > @@ -865,7 +865,10 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n> > > \t\t\tif (dewarper_ && !isRaw_) {\n> > > \t\t\t\toutputCfgs.push_back(const_cast(cfg));\n> > > \t\t\t\tret = dewarper_->configure(cfg, outputCfgs);\n> > > -\t\t\t\tuseDewarper_ = ret ? false : true;\n> > > +\t\t\t\tif (ret)\n> > > +\t\t\t\t\treturn ret;\n> > > +\n> > > +\t\t\t\tuseDewarper_ = true;\n\nDo not enable the dewarper if its configuration fails.\nYou'll enable the dewarper earlier in a later patch as noted.\n\nJust change the commit message to highlight what this patch does\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n> > >\n> > > \t\t\t\t/*\n> > > \t\t\t\t * Calculate the crop rectangle of the data\n> > > --\n> > > 2.43.0\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 87DA9C32F6\n\tfor ;\n\tTue, 17 Dec 2024 07:56:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9D11C67FC0;\n\tTue, 17 Dec 2024 08:56:23 +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 6C47F67FB8\n\tfor ;\n\tTue, 17 Dec 2024 08:56:22 +0100 (CET)","from ideasonboard.com (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 315893E;\n\tTue, 17 Dec 2024 08:55:45 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"sM8xCiwS\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1734422145;\n\tbh=btrlg2voEZQKalqKM/ECm1fhF3vEXeCMiq4wDnIjKLQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=sM8xCiwSWxM6MuR1wFAQH6dOsP5Au/LUKxzBsXrynRMboer+kuHP/ucsdakjk2XoI\n\tWvR+q0ria7V+yi5Q/NhOAxf4x2oWg3dI6Tfd2gJxhTEVBT8UJAitbVUfK0AYKDPc3X\n\tIUlCtMMZXDJDvimYWhiNqCruI4j/evFrqj5pRe7Y=","Date":"Tue, 17 Dec 2024 08:56:18 +0100","From":"Jacopo Mondi ","To":"Stefan Klug ","Cc":"Jacopo Mondi , \n\tlibcamera-devel@lists.libcamera.org,\n\tPaul Elder ","Subject":"Re: [PATCH v4 15/20] pipeline: rkisp1: Enable the dewarper\n\tunconditionally","Message-ID":"","References":"<20241216154124.203650-1-stefan.klug@ideasonboard.com>\n\t<20241216154124.203650-16-stefan.klug@ideasonboard.com>\n\t<75yxziw6oyqlfpyy4kn6awa5gsbcadmrewgmi4qin4gfq4azlr@6dcefgysdzni>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","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\" "}}]