[{"id":33524,"web_url":"https://patchwork.libcamera.org/comment/33524/","msgid":"<174087235344.3864332.7383361867873488271@ping.linuxembedded.co.uk>","date":"2025-03-01T23:39:13","subject":"Re: [PATCH v5 3/5] pipeline: simple: Connect and disconnect start\n\tframe events","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Stanislaw Gruszka (2025-02-25 16:41:14)\n> Enable frame events on the emitter device during\n> SimplePipelineHandler::start(), move signal connection there\n> from SimplePipelineHandler::configure().\n> \n> Also reset delayed controls on start() to prevent using\n> stale values.\n\nIs that the main cause of https://bugs.libcamera.org/show_bug.cgi?id=241\n? \n\n> \n> Accordingly disable/disconnect on SimplePipelineHandler::stopDevice().\n> \n> This fixes the assertion like below:\n> \n> ../src/libcamera/delayed_controls.cpp:227:\n> libcamera::ControlList libcamera::DelayedControls::get(uint32_t):\n> Assertion `info.type() != ControlTypeNone' failed\n> \n> which can happen rarely at the beginning of streaming when\n> ControlRingBuffer is not yet filled and there are errors on CSI-2.\n> In such rare scenario we can have call to DelayedControls::get()\n> with frame number that exceed number of saved entries. Handing\n> frame start signal assure we do DelayedConntrols::applyControls()\n> with (possibly out of sequence) frame number and later call\n> to DelayedControls::get() will get proper not-empty control entry.\n> \n> Bug: https://bugs.libcamera.org/show_bug.cgi?id=241\n> Co-developed-by: Hans de Goede \n> Signed-off-by: Hans de Goede \n> Signed-off-by: Stanislaw Gruszka \n> ---\n> src/libcamera/pipeline/simple/simple.cpp | 23 +++++++++++++++++++----\n> 1 file changed, 19 insertions(+), 4 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> index 063a098f..774c7824 100644\n> --- a/src/libcamera/pipeline/simple/simple.cpp\n> +++ b/src/libcamera/pipeline/simple/simple.cpp\n> @@ -1229,7 +1229,6 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)\n> static_cast(c);\n> SimpleCameraData *data = cameraData(camera);\n> V4L2VideoDevice *video = data->video_;\n> - V4L2Subdevice *frameStartEmitter = data->frameStartEmitter_;\n> int ret;\n> \n> /*\n> @@ -1300,9 +1299,6 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)\n> data->delayedCtrls_ =\n> std::make_unique(data->sensor_->device(),\n> params);\n> - if (frameStartEmitter)\n> - frameStartEmitter->frameStart.connect(data->delayedCtrls_.get(),\n> - &DelayedControls::applyControls);\n\nAyee, that was definitely wrong before - reconnecting on every\nreconfigure.\n\nI am really surprised we don't have a detection in Signal that reports\nif we connect a same object/slot combination multiple times...\n\n> \n> StreamConfiguration inputCfg;\n> inputCfg.pixelFormat = pipeConfig->captureFormat;\n> @@ -1341,6 +1337,7 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL\n> {\n> SimpleCameraData *data = cameraData(camera);\n> V4L2VideoDevice *video = data->video_;\n> + V4L2Subdevice *frameStartEmitter = data->frameStartEmitter_;\n> int ret;\n> \n> const MediaPad *pad = acquirePipeline(data);\n> @@ -1370,6 +1367,17 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL\n> \n> video->bufferReady.connect(data, &SimpleCameraData::imageBufferReady);\n> \n> + data->delayedCtrls_->reset();\n\nIt's only one line, but it feels like this one line deserves it's own\npatch at the moment. But maybe it's fine. I suspect that's the real\ncause of some of the crashes? in particular #241 - and while handling\nthe frameStart correctly is good - I don't see that as the root cause of\nthe bug?\n\nI could look the other way though as it's all about making sure the\ncontrols are updated at the right timing.\n\n\n> + if (frameStartEmitter) {\n> + ret = frameStartEmitter->setFrameStartEnabled(true);\n> + if (ret) {\n> + stop(camera);\n> + return ret;\n> + }\n> + frameStartEmitter->frameStart.connect(data->delayedCtrls_.get(),\n> + &DelayedControls::applyControls);\n> + }\n> +\n> ret = video->streamOn();\n> if (ret < 0) {\n> stop(camera);\n> @@ -1401,6 +1409,13 @@ void SimplePipelineHandler::stopDevice(Camera *camera)\n> {\n> SimpleCameraData *data = cameraData(camera);\n> V4L2VideoDevice *video = data->video_;\n> + V4L2Subdevice *frameStartEmitter = data->frameStartEmitter_;\n> +\n> + if (frameStartEmitter) {\n> + frameStartEmitter->setFrameStartEnabled(false);\n> + frameStartEmitter->frameStart.disconnect(data->delayedCtrls_.get(),\n> + &DelayedControls::applyControls);\n> + }\n> \n> if (data->useConversion_) {\n> if (data->converter_)\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 68069C3263\n\tfor ;\n\tSat, 1 Mar 2025 23:39:19 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 60437614EB;\n\tSun, 2 Mar 2025 00:39:18 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C7D05614EB\n\tfor ;\n\tSun, 2 Mar 2025 00:39:16 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 66620666;\n\tSun, 2 Mar 2025 00:37:46 +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=\"oFytW0/S\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1740872266;\n\tbh=sBxYV+LZ0EHMOnb5HYXxEtJTlVfP3dpP81DjoeQEdIg=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=oFytW0/SFYe55JTIaNe1sYnOp5Q3vGjPu3rLO+VWNCZjzWu5grZQM8X17FC54XEm+\n\tUs8aUmPaxyNz8d7U8WnRv2klei0MEV4mrmtAlHEkNGqIhmz0V2psqme1GRhuovxfkI\n\tVckzRz98d9av3fXCFqnfJ2u8ARlkbktZa8iAAMCI=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20250225164116.414301-4-stanislaw.gruszka@linux.intel.com>","References":"<20250225164116.414301-1-stanislaw.gruszka@linux.intel.com>\n\t<20250225164116.414301-4-stanislaw.gruszka@linux.intel.com>","Subject":"Re: [PATCH v5 3/5] pipeline: simple: Connect and disconnect start\n\tframe events","From":"Kieran Bingham ","Cc":"Milan Zamazal ,\n\tLaurent Pinchart ,\n\tNaushir Patuck ,\n\tSakari Ailus ,\n\tHans de Goede ","To":"Stanislaw Gruszka ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Sat, 01 Mar 2025 23:39:13 +0000","Message-ID":"<174087235344.3864332.7383361867873488271@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","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":33530,"web_url":"https://patchwork.libcamera.org/comment/33530/","msgid":"<20250302013726.GE18557@pendragon.ideasonboard.com>","date":"2025-03-02T01:37:26","subject":"Re: [PATCH v5 3/5] pipeline: simple: Connect and disconnect start\n\tframe events","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Sat, Mar 01, 2025 at 11:39:13PM +0000, Kieran Bingham wrote:\n> Quoting Stanislaw Gruszka (2025-02-25 16:41:14)\n> > Enable frame events on the emitter device during\n> > SimplePipelineHandler::start(), move signal connection there\n> > from SimplePipelineHandler::configure().\n> > \n> > Also reset delayed controls on start() to prevent using\n> > stale values.\n> \n> Is that the main cause of https://bugs.libcamera.org/show_bug.cgi?id=241\n> ? \n> \n> > Accordingly disable/disconnect on SimplePipelineHandler::stopDevice().\n> > \n> > This fixes the assertion like below:\n> > \n> > ../src/libcamera/delayed_controls.cpp:227:\n> > libcamera::ControlList libcamera::DelayedControls::get(uint32_t):\n> > Assertion `info.type() != ControlTypeNone' failed\n> > \n> > which can happen rarely at the beginning of streaming when\n> > ControlRingBuffer is not yet filled and there are errors on CSI-2.\n> > In such rare scenario we can have call to DelayedControls::get()\n> > with frame number that exceed number of saved entries. Handing\n> > frame start signal assure we do DelayedConntrols::applyControls()\n> > with (possibly out of sequence) frame number and later call\n> > to DelayedControls::get() will get proper not-empty control entry.\n\nThis doesn't explain why connecting the signal is moved to start() time.\nThe commit message needs improvements.\n\n> > Bug: https://bugs.libcamera.org/show_bug.cgi?id=241\n> > Co-developed-by: Hans de Goede \n> > Signed-off-by: Hans de Goede \n> > Signed-off-by: Stanislaw Gruszka \n> > ---\n> > src/libcamera/pipeline/simple/simple.cpp | 23 +++++++++++++++++++----\n> > 1 file changed, 19 insertions(+), 4 deletions(-)\n> > \n> > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> > index 063a098f..774c7824 100644\n> > --- a/src/libcamera/pipeline/simple/simple.cpp\n> > +++ b/src/libcamera/pipeline/simple/simple.cpp\n> > @@ -1229,7 +1229,6 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)\n> > static_cast(c);\n> > SimpleCameraData *data = cameraData(camera);\n> > V4L2VideoDevice *video = data->video_;\n> > - V4L2Subdevice *frameStartEmitter = data->frameStartEmitter_;\n> > int ret;\n> > \n> > /*\n> > @@ -1300,9 +1299,6 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)\n> > data->delayedCtrls_ =\n> > std::make_unique(data->sensor_->device(),\n> > params);\n> > - if (frameStartEmitter)\n> > - frameStartEmitter->frameStart.connect(data->delayedCtrls_.get(),\n> > - &DelayedControls::applyControls);\n> \n> Ayee, that was definitely wrong before - reconnecting on every\n> reconfigure.\n> \n> I am really surprised we don't have a detection in Signal that reports\n> if we connect a same object/slot combination multiple times...\n\nWe have such a check, and it doesn't trigger before the DelayedControls\ninstance is also recreated at every configure().\n\n> > \n> > StreamConfiguration inputCfg;\n> > inputCfg.pixelFormat = pipeConfig->captureFormat;\n> > @@ -1341,6 +1337,7 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL\n> > {\n> > SimpleCameraData *data = cameraData(camera);\n> > V4L2VideoDevice *video = data->video_;\n> > + V4L2Subdevice *frameStartEmitter = data->frameStartEmitter_;\n> > int ret;\n> > \n> > const MediaPad *pad = acquirePipeline(data);\n> > @@ -1370,6 +1367,17 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL\n> > \n> > video->bufferReady.connect(data, &SimpleCameraData::imageBufferReady);\n> > \n> > + data->delayedCtrls_->reset();\n> \n> It's only one line, but it feels like this one line deserves it's own\n> patch at the moment.\n\nAt the very least the reason should be explained in the commit message.\nSplitting this to a separate patch would ensure that.\n\nI'm also concerned about what happens when there is no frame start\nsource. There should be no assertion failure in that case. If the\nreset() call fixes it, then that's great. Otherwise, if the issue gets\nfixed by actually using the frame start event, then we still have a\nproblem.\n\n> But maybe it's fine. I suspect that's the real\n> cause of some of the crashes? in particular #241 - and while handling\n> the frameStart correctly is good - I don't see that as the root cause of\n> the bug?\n> \n> I could look the other way though as it's all about making sure the\n> controls are updated at the right timing.\n> \n> > + if (frameStartEmitter) {\n> > + ret = frameStartEmitter->setFrameStartEnabled(true);\n\nI'm surprised that we currently don't call setFrameStartEnabled(), I had\nmissed that completely. This means this series won't be bisectable :-/\nOn way to fix that would be to split patches differently. You could\nstart with one patch that moves connection/disconnection of the signal\n(without calling setFrameStartEnable()). Here's a candidate for the\ncommit message:\n\n--------\npipeline: simple: Connect/disconnect frameStart signal at start/stop time\n\nThe frameStart signal from the frame start emitter is connected in the\nconfigure() function, and is never disconnected. This means that each\ntime the camera is configured a new connection is made, causing the\nDelayedControls::applyControls() to be called multiple times. Fix it by\nconnecting and disconnecting the signal when starting and stopping the\ncamera.\n--------\n\nYou can then squash the addition of the setFrameStartEnabled() calls\nwith patch 4/5.\n\n--------\npipeline: simple: Enable frame start events\n\nThe simple pipeline handler uses frame start events to apply sensor\ncontrols through the DelayedControls class. The setSensorControls()\nfunction applies the controls directly, which would result in controls\nbeing applied twice, if it wasn't for the fact that the pipeline handler\nforgot to enable the frame start events in the first place. Those two\nissues cancel each other, but cause controls to always be applied\ndirectly.\n\nFix the issue by only applying controls directly in setSensorControls()\nif no frame start event emitter is available, and by enabling the frame\nstart events in startDevice() otherwise. Disable them in stopDevice()\nfor symmetry.\n--------\n\nIf the reset() call is what fixes the assertion failure, it should go to\na separate patch. Otherwise it can be combined with one of the other\npatches, with an appropriate explanation in the commit message.\n\n> > + if (ret) {\n> > + stop(camera);\n\nI'd be surprised if this worked fine. Calling stop() from start() on\nfailure isn't right. At the very least I'd expect a call to stopDevice()\ninstead, but I'm not even sure that would work fine. This isn't a new\nissue though, the stop() function is already called in start(), so I can\nignore this for now. A fix would of course be nice, it can be done on\ntop of this series.\n\n> > + return ret;\n> > + }\n> > + frameStartEmitter->frameStart.connect(data->delayedCtrls_.get(),\n> > + &DelayedControls::applyControls);\n> > + }\n> > +\n> > ret = video->streamOn();\n> > if (ret < 0) {\n> > stop(camera);\n> > @@ -1401,6 +1409,13 @@ void SimplePipelineHandler::stopDevice(Camera *camera)\n> > {\n> > SimpleCameraData *data = cameraData(camera);\n> > V4L2VideoDevice *video = data->video_;\n> > + V4L2Subdevice *frameStartEmitter = data->frameStartEmitter_;\n> > +\n> > + if (frameStartEmitter) {\n> > + frameStartEmitter->setFrameStartEnabled(false);\n> > + frameStartEmitter->frameStart.disconnect(data->delayedCtrls_.get(),\n> > + &DelayedControls::applyControls);\n> > + }\n> > \n> > if (data->useConversion_) {\n> > if (data->converter_)","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 AD213C3259\n\tfor ;\n\tSun, 2 Mar 2025 01:37:47 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D8AE968779;\n\tSun, 2 Mar 2025 02:37:46 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 39AE061833\n\tfor ;\n\tSun, 2 Mar 2025 02:37:45 +0100 (CET)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 956D93E4;\n\tSun, 2 Mar 2025 02:36:14 +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=\"inIlJbk3\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1740879374;\n\tbh=qKMf7/+VS5EKBeAUfCm14ANwyiVtkCv4f1wzA0aiyQY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=inIlJbk37q9f34FKmvieY9EL1kUsCEpt6Uo2DP550GcWmrd28s/qC459JW67pvB3I\n\tUhhOtCJl7paoiIM6zHEeFfOJy9IvkvcWBiIMHteI04f0d5y+SqWpr4OmjevM6A2JYo\n\tFZsEwQThlTFQZo0sexa+DR6gOeWF6N0j9KAdBYas=","Date":"Sun, 2 Mar 2025 03:37:26 +0200","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"Stanislaw Gruszka ,\n\tlibcamera-devel@lists.libcamera.org,\n\tMilan Zamazal , \n\tNaushir Patuck ,\n\tSakari Ailus ,\n\tHans de Goede ","Subject":"Re: [PATCH v5 3/5] pipeline: simple: Connect and disconnect start\n\tframe events","Message-ID":"<20250302013726.GE18557@pendragon.ideasonboard.com>","References":"<20250225164116.414301-1-stanislaw.gruszka@linux.intel.com>\n\t<20250225164116.414301-4-stanislaw.gruszka@linux.intel.com>\n\t<174087235344.3864332.7383361867873488271@ping.linuxembedded.co.uk>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<174087235344.3864332.7383361867873488271@ping.linuxembedded.co.uk>","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":33575,"web_url":"https://patchwork.libcamera.org/comment/33575/","msgid":"","date":"2025-03-04T14:36:35","subject":"Re: [PATCH v5 3/5] pipeline: simple: Connect and disconnect start\n\tframe events","submitter":{"id":211,"url":"https://patchwork.libcamera.org/api/people/211/","name":"Stanislaw Gruszka","email":"stanislaw.gruszka@linux.intel.com"},"content":"Hi Kieran and Laurent,\n\nOn Sun, Mar 02, 2025 at 03:37:26AM +0200, Laurent Pinchart wrote:\n> On Sat, Mar 01, 2025 at 11:39:13PM +0000, Kieran Bingham wrote:\n> > Quoting Stanislaw Gruszka (2025-02-25 16:41:14)\n> > > Enable frame events on the emitter device during\n> > > SimplePipelineHandler::start(), move signal connection there\n> > > from SimplePipelineHandler::configure().\n> > > \n> > > Also reset delayed controls on start() to prevent using\n> > > stale values.\n> > \n> > Is that the main cause of https://bugs.libcamera.org/show_bug.cgi?id=241\n> > ? \n\nIs not. The issue is somewhat complex and require few different conditions\nto happen:\n\n- beginning of processing, i.e. withing less then first 16 frames,\n (DelayedControl::ControlRingBuffer length) to have empty values\n in ControlRingBuffer \n- having frames come with not consecutive numbers (happen when there are CSI\n bus errors)\n- not handling or enabling start frame event\n\nFixing the last point will prevent the problem as we will fill proper frame_nr \nentry in ControlRingBuffer and later DelayedControls::get() will get the right\nvalue instead of None - what triggers the 241 assertion.\n\nAnd yes, the issue will can still happen if we do not handle the event.\n\nActually I think having delayedCtrls_->reset() will make it more reproducible.\nThough alternative is using old/stale ControlValues after every camera restart. \n\n> > > Accordingly disable/disconnect on SimplePipelineHandler::stopDevice().\n> > > \n> > > This fixes the assertion like below:\n> > > \n> > > ../src/libcamera/delayed_controls.cpp:227:\n> > > libcamera::ControlList libcamera::DelayedControls::get(uint32_t):\n> > > Assertion `info.type() != ControlTypeNone' failed\n> > > \n> > > which can happen rarely at the beginning of streaming when\n> > > ControlRingBuffer is not yet filled and there are errors on CSI-2.\n> > > In such rare scenario we can have call to DelayedControls::get()\n> > > with frame number that exceed number of saved entries. Handing\n> > > frame start signal assure we do DelayedConntrols::applyControls()\n> > > with (possibly out of sequence) frame number and later call\n> > > to DelayedControls::get() will get proper not-empty control entry.\n> \n> This doesn't explain why connecting the signal is moved to start() time.\n> The commit message needs improvements.\n> \n> > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=241\n> > > Co-developed-by: Hans de Goede \n> > > Signed-off-by: Hans de Goede \n> > > Signed-off-by: Stanislaw Gruszka \n> > > ---\n> > > src/libcamera/pipeline/simple/simple.cpp | 23 +++++++++++++++++++----\n> > > 1 file changed, 19 insertions(+), 4 deletions(-)\n> > > \n> > > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> > > index 063a098f..774c7824 100644\n> > > --- a/src/libcamera/pipeline/simple/simple.cpp\n> > > +++ b/src/libcamera/pipeline/simple/simple.cpp\n> > > @@ -1229,7 +1229,6 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)\n> > > static_cast(c);\n> > > SimpleCameraData *data = cameraData(camera);\n> > > V4L2VideoDevice *video = data->video_;\n> > > - V4L2Subdevice *frameStartEmitter = data->frameStartEmitter_;\n> > > int ret;\n> > > \n> > > /*\n> > > @@ -1300,9 +1299,6 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)\n> > > data->delayedCtrls_ =\n> > > std::make_unique(data->sensor_->device(),\n> > > params);\n> > > - if (frameStartEmitter)\n> > > - frameStartEmitter->frameStart.connect(data->delayedCtrls_.get(),\n> > > - &DelayedControls::applyControls);\n> > \n> > Ayee, that was definitely wrong before - reconnecting on every\n> > reconfigure.\n> > \n> > I am really surprised we don't have a detection in Signal that reports\n> > if we connect a same object/slot combination multiple times...\n> \n> We have such a check, and it doesn't trigger before the DelayedControls\n> instance is also recreated at every configure().\n> \n> > > \n> > > StreamConfiguration inputCfg;\n> > > inputCfg.pixelFormat = pipeConfig->captureFormat;\n> > > @@ -1341,6 +1337,7 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL\n> > > {\n> > > SimpleCameraData *data = cameraData(camera);\n> > > V4L2VideoDevice *video = data->video_;\n> > > + V4L2Subdevice *frameStartEmitter = data->frameStartEmitter_;\n> > > int ret;\n> > > \n> > > const MediaPad *pad = acquirePipeline(data);\n> > > @@ -1370,6 +1367,17 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL\n> > > \n> > > video->bufferReady.connect(data, &SimpleCameraData::imageBufferReady);\n> > > \n> > > + data->delayedCtrls_->reset();\n> > \n> > It's only one line, but it feels like this one line deserves it's own\n> > patch at the moment.\n> \n> At the very least the reason should be explained in the commit message.\n> Splitting this to a separate patch would ensure that.\n\nI plan to post it as separate patch:\n\ncommit efa05db4b055051fdefcf81b0b3888594c31d9ee\nAuthor: Stanislaw Gruszka \nDate: Mon Mar 3 14:00:50 2025 +0100\n\n pipeline: simple: Reset delayedCtrls at start.\n\n Similar like in other pipelines (IPU3, rpi) avoid using stale\n values of DelayedControls class when the same camera is started\n second time.\n\n Co-developed-by: Hans de Goede \n Signed-off-by: Hans de Goede \n Signed-off-by: Stanislaw Gruszka \n\ndiff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\nindex 58aa3dba..659b1123 100644\n--- a/src/libcamera/pipeline/simple/simple.cpp\n+++ b/src/libcamera/pipeline/simple/simple.cpp\n@@ -1376,6 +1376,7 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL\n\n video->bufferReady.connect(data, &SimpleCameraData::imageBufferReady);\n\n+ data->delayedCtrls_->reset();\n if (frameStartEmitter) {\n ret = frameStartEmitter->setFrameStartEnabled(true);\n if (ret) {\n \n> I'm also concerned about what happens when there is no frame start\n> source. There should be no assertion failure in that case. If the\n> reset() call fixes it, then that's great. Otherwise, if the issue gets\n> fixed by actually using the frame start event, then we still have a\n> problem.\n\nYes, the problem still could happen if there is no startFrameEmitter.\nDo we expect pipelines with no startFrame event devices.\n\n> > But maybe it's fine. I suspect that's the real\n> > cause of some of the crashes? in particular #241 - and while handling\n> > the frameStart correctly is good - I don't see that as the root cause of\n> > the bug?\n\nI tried to address this assertion directly\nhttps://patchwork.libcamera.org/patch/22210/\n\nAnd conclusion FWICT was that assertion did good job as it detected\nreal problem - not handling start frame event in the pipeline. \n\n> > I could look the other way though as it's all about making sure the\n> > controls are updated at the right timing.\n> > \n> > > + if (frameStartEmitter) {\n> > > + ret = frameStartEmitter->setFrameStartEnabled(true);\n> \n> I'm surprised that we currently don't call setFrameStartEnabled(), I had\n> missed that completely. This means this series won't be bisectable :-/\n> On way to fix that would be to split patches differently. You could\n> start with one patch that moves connection/disconnection of the signal\n> (without calling setFrameStartEnable()). Here's a candidate for the\n> commit message:\n> \n> --------\n> pipeline: simple: Connect/disconnect frameStart signal at start/stop time\n> \n> The frameStart signal from the frame start emitter is connected in the\n> configure() function, and is never disconnected. This means that each\n> time the camera is configured a new connection is made, causing the\n> DelayedControls::applyControls() to be called multiple times. Fix it by\n> connecting and disconnecting the signal when starting and stopping the\n> camera.\n> --------\n> \n> You can then squash the addition of the setFrameStartEnabled() calls\n> with patch 4/5.\n> \n> --------\n> pipeline: simple: Enable frame start events\n> \n> The simple pipeline handler uses frame start events to apply sensor\n> controls through the DelayedControls class. The setSensorControls()\n> function applies the controls directly, which would result in controls\n> being applied twice, if it wasn't for the fact that the pipeline handler\n> forgot to enable the frame start events in the first place. Those two\n> issues cancel each other, but cause controls to always be applied\n> directly.\n> \n> Fix the issue by only applying controls directly in setSensorControls()\n> if no frame start event emitter is available, and by enabling the frame\n> start events in startDevice() otherwise. Disable them in stopDevice()\n> for symmetry.\n> --------\n\nThanks for those!\n\n> If the reset() call is what fixes the assertion failure, it should go to\n> a separate patch. Otherwise it can be combined with one of the other\n> patches, with an appropriate explanation in the commit message.\n> \n> > > + if (ret) {\n> > > + stop(camera);\n> \n> I'd be surprised if this worked fine. Calling stop() from start() on\n> failure isn't right. At the very least I'd expect a call to stopDevice()\n> instead, but I'm not even sure that would work fine. This isn't a new\n> issue though, the stop() function is already called in start(), so I can\n> ignore this for now. A fix would of course be nice, it can be done on\n> top of this series.\n\nI've checked that error path and did not notice problem. \nGot \"Failed to start capture\" message and empty qcam window,\nno crash or assertion.\n\nRegards\nStanislaw","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 F360CC32DC\n\tfor ;\n\tTue, 4 Mar 2025 14:36:44 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0FA0B687F0;\n\tTue, 4 Mar 2025 15:36:44 +0100 (CET)","from mgamail.intel.com (mgamail.intel.com [192.198.163.13])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7078268755\n\tfor ;\n\tTue, 4 Mar 2025 15:36:41 +0100 (CET)","from fmviesa004.fm.intel.com ([10.60.135.144])\n\tby fmvoesa107.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384;\n\t04 Mar 2025 06:36:39 -0800","from sgruszka-mobl.ger.corp.intel.com (HELO localhost)\n\t([10.245.97.105]) by fmviesa004-auth.fm.intel.com with\n\tESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 04 Mar 2025 06:36:37 -0800"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=intel.com header.i=@intel.com\n\theader.b=\"l35d1I7k\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple;\n\td=intel.com; i=@intel.com; q=dns/txt; s=Intel;\n\tt=1741099002; x=1772635002;\n\th=date:from:to:cc:subject:message-id:references:\n\tmime-version:in-reply-to;\n\tbh=KaGSxCw65ZAZ0jM0dQ0mwCpugIfwJbNgMoX2KV+Mb80=;\n\tb=l35d1I7kiP7/JDgmOgp99VIgu3jR9UfsE3Gg75hk09RpZhsYtZu2UQ7p\n\txMJ38UIo3pFGEWh1WoqnqoFERZB//n1HSuWoucYmmIB8a2WlOI2iYKsNE\n\tEVnQpryPW2XNB4hn7X1bq6RZr2F0vmQU8tSQGLBgmezCHNHCwNcp22/kT\n\t53FhbCs9Il8LI1RfO3HeQzlNgX6NkXjeWcqEQMDQ+JTNsAEgq1ouwNyM9\n\tWtXAggRBtBPbpDBCXC5g8grRMpuJm2WgdSH+c76PzIoXCo+WzQWnb0MwL\n\tgsCyCopVXpoZxcITH7v/gMouGQFAP7zkD08Ypwj0JfbvegH4ZT6C8lbA4 Q==;","X-CSE-ConnectionGUID":["GPrD5pSkRluTNHYMWD7sMg==","NiI4+qTATjqaVKri/Eyfag=="],"X-CSE-MsgGUID":["rjRO5AM3QH6w4nmDzlOfow==","cqDNysmmRpGJv4C/haCCww=="],"X-IronPort-AV":["E=McAfee;i=\"6700,10204,11363\"; a=\"44826021\"","E=Sophos;i=\"6.14,220,1736841600\"; d=\"scan'208\";a=\"44826021\"","E=Sophos;i=\"6.14,220,1736841600\"; d=\"scan'208\";a=\"123508389\""],"X-ExtLoop1":"1","Date":"Tue, 4 Mar 2025 15:36:35 +0100","From":"Stanislaw Gruszka ","To":"Laurent Pinchart ","Cc":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org,\n\tMilan Zamazal , \n\tNaushir Patuck ,\n\tSakari Ailus ,\n\tHans de Goede ","Subject":"Re: [PATCH v5 3/5] pipeline: simple: Connect and disconnect start\n\tframe events","Message-ID":"","References":"<20250225164116.414301-1-stanislaw.gruszka@linux.intel.com>\n\t<20250225164116.414301-4-stanislaw.gruszka@linux.intel.com>\n\t<174087235344.3864332.7383361867873488271@ping.linuxembedded.co.uk>\n\t<20250302013726.GE18557@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20250302013726.GE18557@pendragon.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\" "}}]