[{"id":36423,"web_url":"https://patchwork.libcamera.org/comment/36423/","msgid":"<974aeb0c-7e46-426d-9b6e-113d2c102969@ideasonboard.com>","date":"2025-10-24T09:17:15","subject":"Re: [PATCH v1 10/35] pipeline: rkisp1: Add a frameStart function to\n\thandle DelayedControls::applyControls","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"2025. 10. 24. 10:50 keltezéssel, Stefan Klug írta:\n> Move the call to applyControls into an intermediate function for\n> upcoming modfications.\n> \n> While at it, properly disconnect the signal on failure.\n> \n> Signed-off-by: Stefan Klug \n> ---\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 18 ++++++++++++++++--\n> 1 file changed, 16 insertions(+), 2 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index 17250cbd5ae6..9be9f44db479 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -1540,6 +1540,7 @@ const uint32_t kDefaultExtParamsBlocks = 0xfffff;\n> \n> int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n> {\n> +\tutils::ScopeExitActions actions;\n> \tint ret;\n> \n> \tstd::unique_ptr data =\n> @@ -1570,8 +1571,10 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n> \tdata->delayedCtrls_ =\n> \t\tstd::make_unique(data->sensor_->device(),\n> \t\t\t\t\t\t params);\n> -\tisp_->frameStart.connect(data->delayedCtrls_.get(),\n> -\t\t\t\t &DelayedControls::applyControls);\n> +\tisp_->frameStart.connect(this,\n> +\t\t\t\t &PipelineHandlerRkISP1::frameStart);\n\nI thought the declaration was missing, but it turns out the function is\ndeclared but not defined. (Not removed in 3809fd77463f8f9f30d8da56fc3b103c683fba72.)\n\nI realize that this is kind of unrelated, but why register the signal\nhandler in every `createCamera()` invocation? Is it not sufficient\nto do that once? Especially now that the handler has been completely\ndecoupled from `RkISP1CameraData`?\n\n\n> +\n> +\tactions += [&]() { isp_->frameStart.disconnect(this); };\n> \n> \tuint32_t supportedBlocks = kDefaultExtParamsBlocks;\n> \n> @@ -1603,9 +1606,20 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n> \n> \tregisterCamera(std::move(camera));\n> \n> +\tactions.release();\n> +\n> \treturn 0;\n> }\n> \n> +void PipelineHandlerRkISP1::frameStart(uint32_t sequence)\n> +{\n> +\tif (!activeCamera_)\n> +\t\treturn;\n> +\n> +\tRkISP1CameraData *data = cameraData(activeCamera_);\n> +\tdata->delayedCtrls_->applyControls(sequence);\n> +}\n> +\n> bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)\n> {\n> \tDeviceMatch dm(\"rkisp1\");\n> --\n> 2.48.1\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 4EFBEC3259\n\tfor ;\n\tFri, 24 Oct 2025 09:17:21 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 98AB660895;\n\tFri, 24 Oct 2025 11:17:20 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 868226088F\n\tfor ;\n\tFri, 24 Oct 2025 11:17:18 +0200 (CEST)","from [192.168.33.13] (185.221.141.231.nat.pool.zt.hu\n\t[185.221.141.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 0CC6719E5;\n\tFri, 24 Oct 2025 11:15:33 +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=\"uTUmIf/Y\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1761297333;\n\tbh=jXdw3/+IWf3gZhBZ0P940gYytzSUZG0j5tlz72et11E=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=uTUmIf/Yh0CfitpzBDKVO8l8mgMe72FAARJAFBYWCQDQMeZwF6RX7SXvNjY87EfHI\n\tY0n0/EfmDeXCdxH7Ira6JtVcqztdxw0X7U98lbkXGHuab71gMOevbd/bntl08riSGx\n\tG8ZZSFgpyiwLcKWJ3nIscRwD/SdDuv6FLX8P4HJs=","Message-ID":"<974aeb0c-7e46-426d-9b6e-113d2c102969@ideasonboard.com>","Date":"Fri, 24 Oct 2025 11:17:15 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v1 10/35] pipeline: rkisp1: Add a frameStart function to\n\thandle DelayedControls::applyControls","To":"Stefan Klug ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20251024085130.995967-1-stefan.klug@ideasonboard.com>\n\t<7U_j7YO5lo0LO3fjiwi25JstVP2cGwfCIVRUBD5B2lCHQAn0nW31ZJCAe5cQgmAJusa5lwglWpfMecpanXXY2A==@protonmail.internalid>\n\t<20251024085130.995967-11-stefan.klug@ideasonboard.com>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"<20251024085130.995967-11-stefan.klug@ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","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":37284,"web_url":"https://patchwork.libcamera.org/comment/37284/","msgid":"<176542401572.48638.140318659592113777@localhost>","date":"2025-12-11T03:33:35","subject":"Re: [PATCH v1 10/35] pipeline: rkisp1: Add a frameStart function to\n\thandle DelayedControls::applyControls","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Barnabás,\n\nThank you for the review.\n\nQuoting Barnabás Pőcze (2025-10-24 11:17:15)\n> 2025. 10. 24. 10:50 keltezéssel, Stefan Klug írta:\n> > Move the call to applyControls into an intermediate function for\n> > upcoming modfications.\n> > \n> > While at it, properly disconnect the signal on failure.\n> > \n> > Signed-off-by: Stefan Klug \n> > ---\n> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 18 ++++++++++++++++--\n> > 1 file changed, 16 insertions(+), 2 deletions(-)\n> > \n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > index 17250cbd5ae6..9be9f44db479 100644\n> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > @@ -1540,6 +1540,7 @@ const uint32_t kDefaultExtParamsBlocks = 0xfffff;\n> > \n> > int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n> > {\n> > + utils::ScopeExitActions actions;\n> > int ret;\n> > \n> > std::unique_ptr data =\n> > @@ -1570,8 +1571,10 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n> > data->delayedCtrls_ =\n> > std::make_unique(data->sensor_->device(),\n> > params);\n> > - isp_->frameStart.connect(data->delayedCtrls_.get(),\n> > - &DelayedControls::applyControls);\n> > + isp_->frameStart.connect(this,\n> > + &PipelineHandlerRkISP1::frameStart);\n> \n> I thought the declaration was missing, but it turns out the function is\n> declared but not defined. (Not removed in 3809fd77463f8f9f30d8da56fc3b103c683fba72.)\n> \n> I realize that this is kind of unrelated, but why register the signal\n> handler in every `createCamera()` invocation? Is it not sufficient\n> to do that once? Especially now that the handler has been completely\n> decoupled from `RkISP1CameraData`?\n> \n\nI don't think this is unrelated. I changed it for v2, to connect inside\nthe match() function, where all the other signals get connected.\n\nBest regards,\nStefan\n\n> \n> > +\n> > + actions += [&]() { isp_->frameStart.disconnect(this); };\n> > \n> > uint32_t supportedBlocks = kDefaultExtParamsBlocks;\n> > \n> > @@ -1603,9 +1606,20 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n> > \n> > registerCamera(std::move(camera));\n> > \n> > + actions.release();\n> > +\n> > return 0;\n> > }\n> > \n> > +void PipelineHandlerRkISP1::frameStart(uint32_t sequence)\n> > +{\n> > + if (!activeCamera_)\n> > + return;\n> > +\n> > + RkISP1CameraData *data = cameraData(activeCamera_);\n> > + data->delayedCtrls_->applyControls(sequence);\n> > +}\n> > +\n> > bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)\n> > {\n> > DeviceMatch dm(\"rkisp1\");\n> > --\n> > 2.48.1\n> > \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 1D2A6BD1F1\n\tfor ;\n\tThu, 11 Dec 2025 03:33:45 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6868861506;\n\tThu, 11 Dec 2025 04:33:44 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 506B4606D5\n\tfor ;\n\tThu, 11 Dec 2025 04:33:42 +0100 (CET)","from ideasonboard.com (p99249-ipoefx.ipoe.ocn.ne.jp\n\t[153.246.134.248])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id CA5B3667;\n\tThu, 11 Dec 2025 04:33:39 +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=\"JQrcTh3/\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1765424020;\n\tbh=oB4lWBq0musBaJB9HK7v9SjEsrQ+b31lS6lh5u9Xt8U=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=JQrcTh3/dgObG/IsrKs06miaZUoxJnEXCq5GgE3NjeObXyFtbf/ssM+xsKXH6pv8s\n\tJ56/OZcxql+oXn4Q0eXWjg35SnazkFOzZ8PiZbdZvMUtJJHbKteQ5aaHfQLkBbABBZ\n\tLsD8OwCaYtm+kcMc8XmwiSO1Cb1v7xWJFpuxFwhQ=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<974aeb0c-7e46-426d-9b6e-113d2c102969@ideasonboard.com>","References":"<20251024085130.995967-1-stefan.klug@ideasonboard.com>\n\t<7U_j7YO5lo0LO3fjiwi25JstVP2cGwfCIVRUBD5B2lCHQAn0nW31ZJCAe5cQgmAJusa5lwglWpfMecpanXXY2A==@protonmail.internalid>\n\t<20251024085130.995967-11-stefan.klug@ideasonboard.com>\n\t<974aeb0c-7e46-426d-9b6e-113d2c102969@ideasonboard.com>","Subject":"Re: [PATCH v1 10/35] pipeline: rkisp1: Add a frameStart function to\n\thandle DelayedControls::applyControls","From":"Stefan Klug ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Thu, 11 Dec 2025 04:33:35 +0100","Message-ID":"<176542401572.48638.140318659592113777@localhost>","User-Agent":"alot/0.12.dev8+g2c003385c862.d20250602","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\" "}}]