[{"id":34028,"web_url":"https://patchwork.libcamera.org/comment/34028/","msgid":"<174549474025.2166729.4580744541516784089@ping.linuxembedded.co.uk>","date":"2025-04-24T11:39:00","subject":"Re: [PATCH v2 2/2] pipeline: simple: Fix matching with empty media\n\tgraphs","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Paul Elder (2025-03-26 08:47:59)\n> The match() function currently reports that it is not possible to create\n> any cameras if it encounters an empty media graph.\n> \n> Fix this by looping over all media graphs and only returning false when\n> all of them fail to create a camera.\n> \n> Signed-off-by: Paul Elder \n> Signed-off-by: Kieran Bingham \n\nI think that SoB only appeared because I might have sent this patch to\nthe list on your behalf in the past as\nhttps://patchwork.libcamera.org/patch/20849/.\n\nBut there's no functional changes in here from me that I'm aware of.\n\nAnyway,\n\nPatch 1/2 in this series addresses Laurents comments on\nhttps://patchwork.libcamera.org/patch/20849/, and I've seen this work on\na complex device so I think it's helpful to land it.\n\n\n> ---\n> Changes in v2:\n> - prevent a mix of valid and invalid media devices from ending up in\n> mediaDevices_\n> ---\n> src/libcamera/pipeline/simple/simple.cpp | 52 ++++++++++++++++--------\n> 1 file changed, 35 insertions(+), 17 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> index 6e039bf35fc1..a6bdd6024a99 100644\n> --- a/src/libcamera/pipeline/simple/simple.cpp\n> +++ b/src/libcamera/pipeline/simple/simple.cpp\n> @@ -373,6 +373,9 @@ private:\n> return static_cast(camera->_d());\n> }\n> \n> + bool matchDevice(MediaDevice *media, const SimplePipelineInfo &info,\n> + DeviceEnumerator *enumerator);\n> +\n> std::vector locateSensors(MediaDevice *media);\n> static int resetRoutingTable(V4L2Subdevice *subdev);\n> \n> @@ -1532,25 +1535,13 @@ int SimplePipelineHandler::resetRoutingTable(V4L2Subdevice *subdev)\n> return 0;\n> }\n> \n> -bool SimplePipelineHandler::match(DeviceEnumerator *enumerator)\n> +bool SimplePipelineHandler::matchDevice(MediaDevice *media,\n> + const SimplePipelineInfo &info,\n> + DeviceEnumerator *enumerator)\n> {\n> - const SimplePipelineInfo *info = nullptr;\n> unsigned int numStreams = 1;\n> - MediaDevice *media;\n> -\n> - for (const SimplePipelineInfo &inf : supportedDevices) {\n> - DeviceMatch dm(inf.driver);\n> - media = acquireMediaDevice(enumerator, dm);\n> - if (media) {\n> - info = &inf;\n> - break;\n> - }\n> - }\n> -\n> - if (!media)\n> - return false;\n> \n> - for (const auto &[name, streams] : info->converters) {\n> + for (const auto &[name, streams] : info.converters) {\n> DeviceMatch converterMatch(name);\n> converter_ = acquireMediaDevice(enumerator, converterMatch);\n> if (converter_) {\n> @@ -1559,7 +1550,7 @@ bool SimplePipelineHandler::match(DeviceEnumerator *enumerator)\n> }\n> }\n> \n> - swIspEnabled_ = info->swIspEnabled;\n> + swIspEnabled_ = info.swIspEnabled;\n> \n> /* Locate the sensors. */\n> std::vector sensors = locateSensors(media);\n> @@ -1678,6 +1669,33 @@ bool SimplePipelineHandler::match(DeviceEnumerator *enumerator)\n> return registered;\n> }\n> \n> +bool SimplePipelineHandler::match(DeviceEnumerator *enumerator)\n> +{\n> + MediaDevice *media;\n> +\n> + for (const SimplePipelineInfo &inf : supportedDevices) {\n> + DeviceMatch dm(inf.driver);\n> + while ((media = acquireMediaDevice(enumerator, dm))) {\n> + /*\n> + * If match succeeds, return true to let match() be\n> + * called again on a new instance of the pipeline\n> + * handler. Otherwise keep looping until we do\n> + * successfully match one (or run out).\n> + */\n> + if (matchDevice(media, inf, enumerator)) {\n> + LOG(SimplePipeline, Debug)\n> + << \"Matched on device: \"\n> + << media->deviceNode();\n> + return true;\n> + }\n> + }\n> +\n> + clearMediaDevices();\n\nHrm, I was about to add a tag, but now I'm doubting myself.\n\nShould clearMediaDevices be here or in the loop above before the next\ncall to acquireMediaDevice() ?\n\nOr does the functionality of acquireMediaDevice rely on the ones we\ndon't support already being acquired so we don't try to acquire them\nagain in an infinite loop ?\n\n(i.e. - is the loop operator based on the enumerator, or something\nimposed by the media devices?)\n\n\n\n> + }\n> +\n> + return false;\n> +}\n> +\n> V4L2VideoDevice *SimplePipelineHandler::video(const MediaEntity *entity)\n> {\n> auto iter = entities_.find(entity);\n> -- \n> 2.47.2\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 C5A48C327D\n\tfor ;\n\tThu, 24 Apr 2025 11:39:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id F20EC68AD0;\n\tThu, 24 Apr 2025 13:39:04 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 79A9868ACC\n\tfor ;\n\tThu, 24 Apr 2025 13:39:03 +0200 (CEST)","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 3503B16A;\n\tThu, 24 Apr 2025 13:39:01 +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=\"KF4XacXw\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1745494741;\n\tbh=Klc9TyxzVQCoiu+n5BHUIbSqYTmZVMTF2vRVxk2VpT8=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=KF4XacXwA+Zn7roAUg1UPHWGFYIMAdBLBZVNMxxE7ro1+oXmzcDKCYuZMSiNnC1y5\n\t0yhJC8ao7aBrc3KuB3Jzi4nHDwuNYSvFlHqcuAj9ZFiDD+WKMNZWCwUbrgFAZ9pqEi\n\tK6mLUzU1OzqMoV5OOGJtT++KGUyxVP/W8a/vJ2v0=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20250326084800.1880530-3-paul.elder@ideasonboard.com>","References":"<20250326084800.1880530-1-paul.elder@ideasonboard.com>\n\t<20250326084800.1880530-3-paul.elder@ideasonboard.com>","Subject":"Re: [PATCH v2 2/2] pipeline: simple: Fix matching with empty media\n\tgraphs","From":"Kieran Bingham ","Cc":"Paul Elder ","To":"Paul Elder ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Thu, 24 Apr 2025 12:39:00 +0100","Message-ID":"<174549474025.2166729.4580744541516784089@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":34035,"web_url":"https://patchwork.libcamera.org/comment/34035/","msgid":"","date":"2025-04-25T08:52:34","subject":"Re: [PATCH v2 2/2] pipeline: simple: Fix matching with empty media\n\tgraphs","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"On Thu, Apr 24, 2025 at 12:39:00PM +0100, Kieran Bingham wrote:\n> Quoting Paul Elder (2025-03-26 08:47:59)\n> > The match() function currently reports that it is not possible to create\n> > any cameras if it encounters an empty media graph.\n> > \n> > Fix this by looping over all media graphs and only returning false when\n> > all of them fail to create a camera.\n> > \n> > Signed-off-by: Paul Elder \n> > Signed-off-by: Kieran Bingham \n> \n> I think that SoB only appeared because I might have sent this patch to\n> the list on your behalf in the past as\n> https://patchwork.libcamera.org/patch/20849/.\n> \n> But there's no functional changes in here from me that I'm aware of.\n> \n> Anyway,\n> \n> Patch 1/2 in this series addresses Laurents comments on\n> https://patchwork.libcamera.org/patch/20849/, and I've seen this work on\n> a complex device so I think it's helpful to land it.\n> \n> \n> > ---\n> > Changes in v2:\n> > - prevent a mix of valid and invalid media devices from ending up in\n> > mediaDevices_\n> > ---\n> > src/libcamera/pipeline/simple/simple.cpp | 52 ++++++++++++++++--------\n> > 1 file changed, 35 insertions(+), 17 deletions(-)\n> > \n> > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> > index 6e039bf35fc1..a6bdd6024a99 100644\n> > --- a/src/libcamera/pipeline/simple/simple.cpp\n> > +++ b/src/libcamera/pipeline/simple/simple.cpp\n> > @@ -373,6 +373,9 @@ private:\n> > return static_cast(camera->_d());\n> > }\n> > \n> > + bool matchDevice(MediaDevice *media, const SimplePipelineInfo &info,\n> > + DeviceEnumerator *enumerator);\n> > +\n> > std::vector locateSensors(MediaDevice *media);\n> > static int resetRoutingTable(V4L2Subdevice *subdev);\n> > \n> > @@ -1532,25 +1535,13 @@ int SimplePipelineHandler::resetRoutingTable(V4L2Subdevice *subdev)\n> > return 0;\n> > }\n> > \n> > -bool SimplePipelineHandler::match(DeviceEnumerator *enumerator)\n> > +bool SimplePipelineHandler::matchDevice(MediaDevice *media,\n> > + const SimplePipelineInfo &info,\n> > + DeviceEnumerator *enumerator)\n> > {\n> > - const SimplePipelineInfo *info = nullptr;\n> > unsigned int numStreams = 1;\n> > - MediaDevice *media;\n> > -\n> > - for (const SimplePipelineInfo &inf : supportedDevices) {\n> > - DeviceMatch dm(inf.driver);\n> > - media = acquireMediaDevice(enumerator, dm);\n> > - if (media) {\n> > - info = &inf;\n> > - break;\n> > - }\n> > - }\n> > -\n> > - if (!media)\n> > - return false;\n> > \n> > - for (const auto &[name, streams] : info->converters) {\n> > + for (const auto &[name, streams] : info.converters) {\n> > DeviceMatch converterMatch(name);\n> > converter_ = acquireMediaDevice(enumerator, converterMatch);\n> > if (converter_) {\n> > @@ -1559,7 +1550,7 @@ bool SimplePipelineHandler::match(DeviceEnumerator *enumerator)\n> > }\n> > }\n> > \n> > - swIspEnabled_ = info->swIspEnabled;\n> > + swIspEnabled_ = info.swIspEnabled;\n> > \n> > /* Locate the sensors. */\n> > std::vector sensors = locateSensors(media);\n> > @@ -1678,6 +1669,33 @@ bool SimplePipelineHandler::match(DeviceEnumerator *enumerator)\n> > return registered;\n> > }\n> > \n> > +bool SimplePipelineHandler::match(DeviceEnumerator *enumerator)\n> > +{\n> > + MediaDevice *media;\n> > +\n> > + for (const SimplePipelineInfo &inf : supportedDevices) {\n> > + DeviceMatch dm(inf.driver);\n> > + while ((media = acquireMediaDevice(enumerator, dm))) {\n> > + /*\n> > + * If match succeeds, return true to let match() be\n> > + * called again on a new instance of the pipeline\n> > + * handler. Otherwise keep looping until we do\n> > + * successfully match one (or run out).\n> > + */\n> > + if (matchDevice(media, inf, enumerator)) {\n> > + LOG(SimplePipeline, Debug)\n> > + << \"Matched on device: \"\n> > + << media->deviceNode();\n> > + return true;\n> > + }\n> > + }\n> > +\n> > + clearMediaDevices();\n> \n> Hrm, I was about to add a tag, but now I'm doubting myself.\n> \n> Should clearMediaDevices be here or in the loop above before the next\n> call to acquireMediaDevice() ?\n\nThe idea is that every time matchDevice() returns false, we need to\nrollback all acquireMediaDevice() calls with clearMediaDevices().\nReally there are only two acquireMediaDevice() calls; one here, and one\ninside matchDevice() to acquire the converter media device. That's the\nmain one that we're concerned about imo.\n\nIf the first acquireMediaDevice() here succeeds and the\nacquireMediaDevice() inside matchDevice() fails, we need to\nclearMediaDevices() before the next attempt of acquireMediaDevice() here\non the next supportedDevices.\n\n(Otherwise mediaDevices_ will have a stray media device from the last\nattempt that succeeded acquire on the main device but failed acquire on\nthe converter)\n\nSo I think clearMediaDevices() being here is correct.\n\nThough I'm thinking that the while loop should probably just be an if.\n\n\nPaul\n\n> \n> Or does the functionality of acquireMediaDevice rely on the ones we\n> don't support already being acquired so we don't try to acquire them\n> again in an infinite loop ?\n> \n> (i.e. - is the loop operator based on the enumerator, or something\n> imposed by the media devices?)\n> \n> \n> \n> > + }\n> > +\n> > + return false;\n> > +}\n> > +\n> > V4L2VideoDevice *SimplePipelineHandler::video(const MediaEntity *entity)\n> > {\n> > auto iter = entities_.find(entity);\n> > -- \n> > 2.47.2\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 8414CBE08B\n\tfor ;\n\tFri, 25 Apr 2025 08:52:44 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 658A268ACD;\n\tFri, 25 Apr 2025 10:52:43 +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 064C160526\n\tfor ;\n\tFri, 25 Apr 2025 10:52:41 +0200 (CEST)","from pyrite.rasen.tech (unknown\n\t[IPv6:2404:7a81:160:2100:cdd6:f8a4:92ff:e16a])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 30006982;\n\tFri, 25 Apr 2025 10:52:37 +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=\"Lk0HFV1j\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1745571159;\n\tbh=a5qRc+tj6y8rXBg/farE780DHsxEdktM1sSEJEeC6rY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Lk0HFV1jO15gUL6Lj2kw1acUsq0ZDUo1odVVIsLaIPEes/6pgnZ86P0eyJKbEU1QB\n\ts+ftpVA3Vbk84jeQNQsxW/C6sVWWaMWJiqpdpn2Qc2eUa7BvyXT/kob4vi99toGA+s\n\tdPKi5vhb0Nrkbkt+ykyJtnkNawFnDS8S4WjezH14=","Date":"Fri, 25 Apr 2025 17:52:34 +0900","From":"Paul Elder ","To":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v2 2/2] pipeline: simple: Fix matching with empty media\n\tgraphs","Message-ID":"","References":"<20250326084800.1880530-1-paul.elder@ideasonboard.com>\n\t<20250326084800.1880530-3-paul.elder@ideasonboard.com>\n\t<174549474025.2166729.4580744541516784089@ping.linuxembedded.co.uk>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<174549474025.2166729.4580744541516784089@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\" "}}]