[{"id":21656,"web_url":"https://patchwork.libcamera.org/comment/21656/","msgid":"","date":"2021-12-08T08:26:46","subject":"Re: [libcamera-devel] [PATCH v3 3/9] libcamera: media_device:\n\tHandle ancillary links in populateLinks()","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Dan,\n\nThank you for the patch.\n\nOn Tue, Dec 07, 2021 at 10:45:06PM +0000, Daniel Scally wrote:\n> The populateLinks() function can't currently handle ancillary links\n> which causes an error to be thrown in process() when the MediaObject\n> cannot be cast to a MediaPad.\n> \n> Add explicit handling for the different link types, creating either\n> pad-2-pad links or else storing the pointer to the ancillary device\n> MediaEntity in the ancillaryEntities_ member of the primary.\n> \n> Signed-off-by: Daniel Scally \n> ---\n> Changes in v3:\n> \n> \t- Split out the new macro\n> \t- Fixed some style errors and comments\n> \t- Added a default case\n> \n> src/libcamera/media_device.cpp | 55 ++++++++++++++++++++++++----------\n> 1 file changed, 39 insertions(+), 16 deletions(-)\n\nI'm afraid this patch now conflicts, could you please rebase it ?\n\n> diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp\n> index 4df0a27f..fb332445 100644\n> --- a/src/libcamera/media_device.cpp\n> +++ b/src/libcamera/media_device.cpp\n> @@ -693,45 +693,68 @@ bool MediaDevice::populateLinks(const struct media_v2_topology &topology)\n> {\n> \tstruct media_v2_link *mediaLinks = reinterpret_cast\n> \t\t\t\t\t (topology.ptr_links);\n> +\tMediaEntity *ancillary;\n> +\tMediaEntity *primary;\n> +\tMediaLink *link;\n> \n> \tfor (unsigned int i = 0; i < topology.num_links; ++i) {\n> \t\t/*\n> -\t\t * Skip links between entities and interfaces: we only care\n> -\t\t * about pad-2-pad links here.\n> +\t\t * Skip links between entities and interfaces: interfaces are\n> +\t\t * not created as MediaObjects at this time, so the source and\n> +\t\t * sink pointers would never be found.\n> \t\t */\n> \t\tif ((mediaLinks[i].flags & MEDIA_LNK_FL_LINK_TYPE) ==\n> \t\t MEDIA_LNK_FL_INTERFACE_LINK)\n> \t\t\tcontinue;\n> \n> -\t\t/* Store references to source and sink pads in the link. */\n> +\t\t/* Look up the source and sink objects. */\n> \t\tunsigned int source_id = mediaLinks[i].source_id;\n> -\t\tMediaPad *source = dynamic_cast\n> -\t\t\t\t (object(source_id));\n> +\t\tMediaObject *source = object(source_id);\n> \t\tif (!source) {\n> \t\t\tLOG(MediaDevice, Error)\n> -\t\t\t\t<< \"Failed to find pad with id: \"\n> +\t\t\t\t<< \"Failed to find MediaObject with id: \"\n> \t\t\t\t<< source_id;\n> \t\t\treturn false;\n> \t\t}\n> \n> \t\tunsigned int sink_id = mediaLinks[i].sink_id;\n> -\t\tMediaPad *sink = dynamic_cast\n> -\t\t\t\t (object(sink_id));\n> +\t\tMediaObject *sink = object(sink_id);\n> \t\tif (!sink) {\n> \t\t\tLOG(MediaDevice, Error)\n> -\t\t\t\t<< \"Failed to find pad with id: \"\n> +\t\t\t\t<< \"Failed to find MediaObject with id: \"\n> \t\t\t\t<< sink_id;\n> \t\t\treturn false;\n> \t\t}\n> \n> -\t\tMediaLink *link = new MediaLink(&mediaLinks[i], source, sink);\n> -\t\tif (!addObject(link)) {\n> -\t\t\tdelete link;\n> -\t\t\treturn false;\n> -\t\t}\n> +\t\tswitch (mediaLinks[i].flags & MEDIA_LNK_FL_LINK_TYPE) {\n> +\t\tcase MEDIA_LNK_FL_DATA_LINK:\n> +\t\t\tlink = new MediaLink(&mediaLinks[i],\n> +\t\t\t\t\t dynamic_cast(source),\n> +\t\t\t\t\t dynamic_cast(sink));\n\nWe'll crash if the source or sink isn't a pad (dynamic_cast will return\nnullptr in that case). This shouldn't happen unless the kernel reports\nan incorrect topology, but it would still be nice to protect against\nthat. How about the following (untested) ?\n\n\t\tcase MEDIA_LNK_FL_DATA_LINK: {\n\t\t\tMediaPad *sourcePad = dynamic_cast(source);\n\t\t\tif (!sourcePad) {\n\t\t\t\tLOG(MediaDevice, Error)\n\t\t\t\t\t<< \"Source MediaObject \" << source_id\n\t\t\t\t\t<< \" is not a pad\";\n\t\t\t\treturn false;\n\t\t\t}\n\n\t\t\tMediaPad *sinkPad = dynamic_cast(sink);\n\t\t\tif (!sinkPad) {\n\t\t\t\tLOG(MediaDevice, Error)\n\t\t\t\t\t<< \"Sink MediaObject \" << sink_id\n\t\t\t\t\t<< \" is not a pad\";\n\t\t\t\treturn false;\n\t\t\t}\n\n\t\t\tMediaLink *link = new MediaLink(&mediaLinks[i],\n\t\t\t\t\t\t\tsourcePad, sinkPad);\n\t\t\tif (!addObject(link)) {\n\t\t\t\tdelete link;\n\t\t\t\treturn false;\n\t\t\t}\n\n\t\t\tlink->source()->addLink(link);\n\t\t\tlink->sink()->addLink(link);\n\n\t\t\tbreak;\n\t\t}\n\n\t\tcase MEDIA_LNK_FL_ANCILLARY_LINK: {\n\t\t\tMediaEntity *primary = dynamic_cast(source);\n\t\t\tif (!primary) {\n\t\t\t\tLOG(MediaDevice, Error)\n\t\t\t\t\t<< \"Source MediaObject \" << source_id\n\t\t\t\t\t<< \" is not an entity\";\n\t\t\t\treturn false;\n\t\t\t}\n\n\t\t\tMediaEntity *ancillary = dynamic_cast(sink);\n\t\t\tif (!ancillary) {\n\t\t\t\tLOG(MediaDevice, Error)\n\t\t\t\t\t<< \"Sink MediaObject \" << sink_id\n\t\t\t\t\t<< \" is not an entity\";\n\t\t\t\treturn false;\n\t\t\t}\n\n\t\t\tprimary->addAncillaryEntity(ancillary);\n\t\t\tbreak;\n\t\t}\n\nOr is it overkill ?\n\n> +\t\t\tif (!addObject(link)) {\n> +\t\t\t\tdelete link;\n> +\t\t\t\treturn false;\n> +\t\t\t}\n> +\n> +\t\t\tlink->source()->addLink(link);\n> +\t\t\tlink->sink()->addLink(link);\n> +\n> +\t\t\tbreak;\n> +\n> +\t\tcase MEDIA_LNK_FL_ANCILLARY_LINK:\n> +\t\t\tprimary = dynamic_cast(source);\n> +\t\t\tancillary = dynamic_cast(sink);\n> \n> -\t\tsource->addLink(link);\n> -\t\tsink->addLink(link);\n> +\t\t\tprimary->addAncillaryEntity(ancillary);\n> +\n> +\t\t\tbreak;\n> +\n> +\t\tdefault:\n> +\t\t\tLOG(MediaDevice, Warning)\n> +\t\t\t\t<< \"Unknown media link type\";\n> +\n> +\t\t\tbreak;\n> +\t\t}\n> \t}\n> \n> \treturn true;","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 77215BF415\n\tfor ;\n\tWed, 8 Dec 2021 08:27:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C08AB6086C;\n\tWed, 8 Dec 2021 09:27:17 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C086760225\n\tfor ;\n\tWed, 8 Dec 2021 09:27:16 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 2DD9A8BB;\n\tWed, 8 Dec 2021 09:27:16 +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=\"cRBivvOe\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1638952036;\n\tbh=QSIHCZo7GJHdVFo8aBFNbauc0O+G/2Jcf9SM+g0RyvY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=cRBivvOeiMzfxKd6GztdDx/cedCse23Ntcuz/kdRk6REB9jxBQoGg0uyJMl+ZOtQ7\n\tVzWksYpxs9CB0+bQvFT9tVX2lKSj9+U90mL7MNmHg6n0yNSvIJrSL2V3UgEP8cV2wN\n\tCoDUTI5/mdF/yd8RyYzpEVPq6ux9fRcOOKX7DsS0=","Date":"Wed, 8 Dec 2021 10:26:46 +0200","From":"Laurent Pinchart ","To":"Daniel Scally ","Message-ID":"","References":"<20211207224512.753979-1-djrscally@gmail.com>\n\t<20211207224512.753979-4-djrscally@gmail.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211207224512.753979-4-djrscally@gmail.com>","Subject":"Re: [libcamera-devel] [PATCH v3 3/9] libcamera: media_device:\n\tHandle ancillary links in populateLinks()","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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":21682,"web_url":"https://patchwork.libcamera.org/comment/21682/","msgid":"<539a8a46-ad3d-82c6-73bb-499111ad2c62@gmail.com>","date":"2021-12-08T22:10:08","subject":"Re: [libcamera-devel] [PATCH v3 3/9] libcamera: media_device:\n\tHandle ancillary links in populateLinks()","submitter":{"id":90,"url":"https://patchwork.libcamera.org/api/people/90/","name":"Daniel Scally","email":"djrscally@gmail.com"},"content":"Hi Laurent\n\nOn 08/12/2021 08:26, Laurent Pinchart wrote:\n> Hi Dan,\n> \n> Thank you for the patch.\n> \n> On Tue, Dec 07, 2021 at 10:45:06PM +0000, Daniel Scally wrote:\n>> The populateLinks() function can't currently handle ancillary links\n>> which causes an error to be thrown in process() when the MediaObject\n>> cannot be cast to a MediaPad.\n>>\n>> Add explicit handling for the different link types, creating either\n>> pad-2-pad links or else storing the pointer to the ancillary device\n>> MediaEntity in the ancillaryEntities_ member of the primary.\n>>\n>> Signed-off-by: Daniel Scally \n>> ---\n>> Changes in v3:\n>>\n>> \t- Split out the new macro\n>> \t- Fixed some style errors and comments\n>> \t- Added a default case\n>>\n>> src/libcamera/media_device.cpp | 55 ++++++++++++++++++++++++----------\n>> 1 file changed, 39 insertions(+), 16 deletions(-)\n> \n> I'm afraid this patch now conflicts, could you please rebase it ?\n> \n>> diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp\n>> index 4df0a27f..fb332445 100644\n>> --- a/src/libcamera/media_device.cpp\n>> +++ b/src/libcamera/media_device.cpp\n>> @@ -693,45 +693,68 @@ bool MediaDevice::populateLinks(const struct media_v2_topology &topology)\n>> {\n>> \tstruct media_v2_link *mediaLinks = reinterpret_cast\n>> \t\t\t\t\t (topology.ptr_links);\n>> +\tMediaEntity *ancillary;\n>> +\tMediaEntity *primary;\n>> +\tMediaLink *link;\n>> \n>> \tfor (unsigned int i = 0; i < topology.num_links; ++i) {\n>> \t\t/*\n>> -\t\t * Skip links between entities and interfaces: we only care\n>> -\t\t * about pad-2-pad links here.\n>> +\t\t * Skip links between entities and interfaces: interfaces are\n>> +\t\t * not created as MediaObjects at this time, so the source and\n>> +\t\t * sink pointers would never be found.\n>> \t\t */\n>> \t\tif ((mediaLinks[i].flags & MEDIA_LNK_FL_LINK_TYPE) ==\n>> \t\t MEDIA_LNK_FL_INTERFACE_LINK)\n>> \t\t\tcontinue;\n>> \n>> -\t\t/* Store references to source and sink pads in the link. */\n>> +\t\t/* Look up the source and sink objects. */\n>> \t\tunsigned int source_id = mediaLinks[i].source_id;\n>> -\t\tMediaPad *source = dynamic_cast\n>> -\t\t\t\t (object(source_id));\n>> +\t\tMediaObject *source = object(source_id);\n>> \t\tif (!source) {\n>> \t\t\tLOG(MediaDevice, Error)\n>> -\t\t\t\t<< \"Failed to find pad with id: \"\n>> +\t\t\t\t<< \"Failed to find MediaObject with id: \"\n>> \t\t\t\t<< source_id;\n>> \t\t\treturn false;\n>> \t\t}\n>> \n>> \t\tunsigned int sink_id = mediaLinks[i].sink_id;\n>> -\t\tMediaPad *sink = dynamic_cast\n>> -\t\t\t\t (object(sink_id));\n>> +\t\tMediaObject *sink = object(sink_id);\n>> \t\tif (!sink) {\n>> \t\t\tLOG(MediaDevice, Error)\n>> -\t\t\t\t<< \"Failed to find pad with id: \"\n>> +\t\t\t\t<< \"Failed to find MediaObject with id: \"\n>> \t\t\t\t<< sink_id;\n>> \t\t\treturn false;\n>> \t\t}\n>> \n>> -\t\tMediaLink *link = new MediaLink(&mediaLinks[i], source, sink);\n>> -\t\tif (!addObject(link)) {\n>> -\t\t\tdelete link;\n>> -\t\t\treturn false;\n>> -\t\t}\n>> +\t\tswitch (mediaLinks[i].flags & MEDIA_LNK_FL_LINK_TYPE) {\n>> +\t\tcase MEDIA_LNK_FL_DATA_LINK:\n>> +\t\t\tlink = new MediaLink(&mediaLinks[i],\n>> +\t\t\t\t\t dynamic_cast(source),\n>> +\t\t\t\t\t dynamic_cast(sink));\n> \n> We'll crash if the source or sink isn't a pad (dynamic_cast will return\n> nullptr in that case). This shouldn't happen unless the kernel reports\n> an incorrect topology, but it would still be nice to protect against\n> that. How about the following (untested) ?\n> \n> \t\tcase MEDIA_LNK_FL_DATA_LINK: {\n> \t\t\tMediaPad *sourcePad = dynamic_cast(source);\n> \t\t\tif (!sourcePad) {\n> \t\t\t\tLOG(MediaDevice, Error)\n> \t\t\t\t\t<< \"Source MediaObject \" << source_id\n> \t\t\t\t\t<< \" is not a pad\";\n> \t\t\t\treturn false;\n> \t\t\t}\n> \n> \t\t\tMediaPad *sinkPad = dynamic_cast(sink);\n> \t\t\tif (!sinkPad) {\n> \t\t\t\tLOG(MediaDevice, Error)\n> \t\t\t\t\t<< \"Sink MediaObject \" << sink_id\n> \t\t\t\t\t<< \" is not a pad\";\n> \t\t\t\treturn false;\n> \t\t\t}\n> \n> \t\t\tMediaLink *link = new MediaLink(&mediaLinks[i],\n> \t\t\t\t\t\t\tsourcePad, sinkPad);\n> \t\t\tif (!addObject(link)) {\n> \t\t\t\tdelete link;\n> \t\t\t\treturn false;\n> \t\t\t}\n> \n> \t\t\tlink->source()->addLink(link);\n> \t\t\tlink->sink()->addLink(link);\n> \n> \t\t\tbreak;\n> \t\t}\n> \n> \t\tcase MEDIA_LNK_FL_ANCILLARY_LINK: {\n> \t\t\tMediaEntity *primary = dynamic_cast(source);\n> \t\t\tif (!primary) {\n> \t\t\t\tLOG(MediaDevice, Error)\n> \t\t\t\t\t<< \"Source MediaObject \" << source_id\n> \t\t\t\t\t<< \" is not an entity\";\n> \t\t\t\treturn false;\n> \t\t\t}\n> \n> \t\t\tMediaEntity *ancillary = dynamic_cast(sink);\n> \t\t\tif (!ancillary) {\n> \t\t\t\tLOG(MediaDevice, Error)\n> \t\t\t\t\t<< \"Sink MediaObject \" << sink_id\n> \t\t\t\t\t<< \" is not an entity\";\n> \t\t\t\treturn false;\n> \t\t\t}\n> \n> \t\t\tprimary->addAncillaryEntity(ancillary);\n> \t\t\tbreak;\n> \t\t}\n> \n> Or is it overkill ?\n\nI lean towards overkill to be honest; since creating data links on the\nkernel side is through media_create_pad_link(), which explicitly sets\nthe source and sink to pads. So someone would have to invent a new\nfunction to add the link in a way that would be broken.\n\nHappy to do it though if you prefer.\n\n>> +\t\t\tif (!addObject(link)) {\n>> +\t\t\t\tdelete link;\n>> +\t\t\t\treturn false;\n>> +\t\t\t}\n>> +\n>> +\t\t\tlink->source()->addLink(link);\n>> +\t\t\tlink->sink()->addLink(link);\n>> +\n>> +\t\t\tbreak;\n>> +\n>> +\t\tcase MEDIA_LNK_FL_ANCILLARY_LINK:\n>> +\t\t\tprimary = dynamic_cast(source);\n>> +\t\t\tancillary = dynamic_cast(sink);\n>> \n>> -\t\tsource->addLink(link);\n>> -\t\tsink->addLink(link);\n>> +\t\t\tprimary->addAncillaryEntity(ancillary);\n>> +\n>> +\t\t\tbreak;\n>> +\n>> +\t\tdefault:\n>> +\t\t\tLOG(MediaDevice, Warning)\n>> +\t\t\t\t<< \"Unknown media link type\";\n>> +\n>> +\t\t\tbreak;\n>> +\t\t}\n>> \t}\n>> \n>> \treturn true;\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 69690BF415\n\tfor ;\n\tWed, 8 Dec 2021 22:10:13 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 70EE76087A;\n\tWed, 8 Dec 2021 23:10:12 +0100 (CET)","from mail-wm1-x336.google.com (mail-wm1-x336.google.com\n\t[IPv6:2a00:1450:4864:20::336])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EEE9560225\n\tfor ;\n\tWed, 8 Dec 2021 23:10:10 +0100 (CET)","by mail-wm1-x336.google.com with SMTP id i12so2820224wmq.4\n\tfor ;\n\tWed, 08 Dec 2021 14:10:10 -0800 (PST)","from [192.168.0.16]\n\t(cpc141996-chfd3-2-0-cust928.12-3.cable.virginm.net. [86.13.91.161])\n\tby smtp.gmail.com with ESMTPSA id\n\td8sm3781450wrm.76.2021.12.08.14.10.09\n\t(version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128);\n\tWed, 08 Dec 2021 14:10:09 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"dGOm3fU+\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112;\n\th=subject:to:cc:references:from:message-id:date:user-agent\n\t:mime-version:in-reply-to:content-language:content-transfer-encoding; \n\tbh=w7w4akYp1yf+5Hf6OKCo8TYhGKE0p1odchoGAmlTnc4=;\n\tb=dGOm3fU+4DexQy53tTyMSnnkYudYknK8TzKDMt3mCReLb9EoTvaTO5YumLc81+BsfS\n\thPaR9hcudwQ7WO/N/LyA0/IWmT09WUAPuZodY8zr4UsdhwsYMQrdyahifpfllFpKjFzk\n\tsvNgxvoNrL0hpPAL4tfsX1XtECYmVy87f+U1POLUqisB0N915dyvx0+PuM4Wwau8xAJF\n\tG4bmwHocfpPXZGt8CEOl3Rp24qX492d8MEuAxtwp9nmPPKu7yaal9U7VUriUyJxi8OPW\n\tkvflPd4oVJFD24opXhSWfY7lv2Tst+YO/HhW2cf7tozfOhJIvY4b3iSvDPgfPyt0lZD+\n\t2BsQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:subject:to:cc:references:from:message-id:date\n\t:user-agent:mime-version:in-reply-to:content-language\n\t:content-transfer-encoding;\n\tbh=w7w4akYp1yf+5Hf6OKCo8TYhGKE0p1odchoGAmlTnc4=;\n\tb=R0OTJoUtiaylrvS5iekQouvVy5yNan5mhBO4N3KL+WqhXipQB/TKsW0PqDi0mQMZun\n\tKJ7IqkiZDbKXk5CE7qHYdTgYWR9uk1n1W0Vz1ZVWzCbv7RGTjP5hUtlhQ/sLMzjK2g4A\n\t70Pa9KmJ/lGgoQEtDJyNZ5EPLcjDWzrNbl1PRZyj65iMPKdEfm17Bwh1R2p4SSDW78b1\n\tzNovlMQIi+LI0C8heTU3pbY7MumzOWFIPlGggRss+1UUuVbeQAh+LBJgnhL2UxF0++j5\n\t0hFK6BRkLbLcBjHtjcxP29yX3DSWs7LfQTyJ4djJ3vZGkffRe0cZEgXuOmW6VFHELneJ\n\tBbRQ==","X-Gm-Message-State":"AOAM533n1I3x/jw7R5roAifmNN5IMjy7hvtuu6KLYQiIuxzUXv59LeV7\n\tmoLBFHY3jzORxhas+7noGs/P2HjnMlE=","X-Google-Smtp-Source":"ABdhPJxYy/Y0eFHXtm67k30wx1T3ojhJZIIqt0IrD8i6tJIxATNz2chyHPnRNE0+yMIjENEP/gYKIQ==","X-Received":"by 2002:a05:600c:a49:: with SMTP id\n\tc9mr1761470wmq.172.1639001410485; \n\tWed, 08 Dec 2021 14:10:10 -0800 (PST)","To":"Laurent Pinchart ","References":"<20211207224512.753979-1-djrscally@gmail.com>\n\t<20211207224512.753979-4-djrscally@gmail.com>\n\t","From":"Daniel Scally ","Message-ID":"<539a8a46-ad3d-82c6-73bb-499111ad2c62@gmail.com>","Date":"Wed, 8 Dec 2021 22:10:08 +0000","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.14.0","MIME-Version":"1.0","In-Reply-To":"","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH v3 3/9] libcamera: media_device:\n\tHandle ancillary links in populateLinks()","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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":21732,"web_url":"https://patchwork.libcamera.org/comment/21732/","msgid":"","date":"2021-12-10T00:59:37","subject":"Re: [libcamera-devel] [PATCH v3 3/9] libcamera: media_device:\n\tHandle ancillary links in populateLinks()","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Dan,\n\nOn Wed, Dec 08, 2021 at 10:10:08PM +0000, Daniel Scally wrote:\n> On 08/12/2021 08:26, Laurent Pinchart wrote:\n> > On Tue, Dec 07, 2021 at 10:45:06PM +0000, Daniel Scally wrote:\n> >> The populateLinks() function can't currently handle ancillary links\n> >> which causes an error to be thrown in process() when the MediaObject\n> >> cannot be cast to a MediaPad.\n> >>\n> >> Add explicit handling for the different link types, creating either\n> >> pad-2-pad links or else storing the pointer to the ancillary device\n> >> MediaEntity in the ancillaryEntities_ member of the primary.\n> >>\n> >> Signed-off-by: Daniel Scally \n> >> ---\n> >> Changes in v3:\n> >>\n> >> \t- Split out the new macro\n> >> \t- Fixed some style errors and comments\n> >> \t- Added a default case\n> >>\n> >> src/libcamera/media_device.cpp | 55 ++++++++++++++++++++++++----------\n> >> 1 file changed, 39 insertions(+), 16 deletions(-)\n> > \n> > I'm afraid this patch now conflicts, could you please rebase it ?\n> > \n> >> diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp\n> >> index 4df0a27f..fb332445 100644\n> >> --- a/src/libcamera/media_device.cpp\n> >> +++ b/src/libcamera/media_device.cpp\n> >> @@ -693,45 +693,68 @@ bool MediaDevice::populateLinks(const struct media_v2_topology &topology)\n> >> {\n> >> \tstruct media_v2_link *mediaLinks = reinterpret_cast\n> >> \t\t\t\t\t (topology.ptr_links);\n> >> +\tMediaEntity *ancillary;\n> >> +\tMediaEntity *primary;\n> >> +\tMediaLink *link;\n> >> \n> >> \tfor (unsigned int i = 0; i < topology.num_links; ++i) {\n> >> \t\t/*\n> >> -\t\t * Skip links between entities and interfaces: we only care\n> >> -\t\t * about pad-2-pad links here.\n> >> +\t\t * Skip links between entities and interfaces: interfaces are\n> >> +\t\t * not created as MediaObjects at this time, so the source and\n> >> +\t\t * sink pointers would never be found.\n> >> \t\t */\n> >> \t\tif ((mediaLinks[i].flags & MEDIA_LNK_FL_LINK_TYPE) ==\n> >> \t\t MEDIA_LNK_FL_INTERFACE_LINK)\n> >> \t\t\tcontinue;\n> >> \n> >> -\t\t/* Store references to source and sink pads in the link. */\n> >> +\t\t/* Look up the source and sink objects. */\n> >> \t\tunsigned int source_id = mediaLinks[i].source_id;\n> >> -\t\tMediaPad *source = dynamic_cast\n> >> -\t\t\t\t (object(source_id));\n> >> +\t\tMediaObject *source = object(source_id);\n> >> \t\tif (!source) {\n> >> \t\t\tLOG(MediaDevice, Error)\n> >> -\t\t\t\t<< \"Failed to find pad with id: \"\n> >> +\t\t\t\t<< \"Failed to find MediaObject with id: \"\n> >> \t\t\t\t<< source_id;\n> >> \t\t\treturn false;\n> >> \t\t}\n> >> \n> >> \t\tunsigned int sink_id = mediaLinks[i].sink_id;\n> >> -\t\tMediaPad *sink = dynamic_cast\n> >> -\t\t\t\t (object(sink_id));\n> >> +\t\tMediaObject *sink = object(sink_id);\n> >> \t\tif (!sink) {\n> >> \t\t\tLOG(MediaDevice, Error)\n> >> -\t\t\t\t<< \"Failed to find pad with id: \"\n> >> +\t\t\t\t<< \"Failed to find MediaObject with id: \"\n> >> \t\t\t\t<< sink_id;\n> >> \t\t\treturn false;\n> >> \t\t}\n> >> \n> >> -\t\tMediaLink *link = new MediaLink(&mediaLinks[i], source, sink);\n> >> -\t\tif (!addObject(link)) {\n> >> -\t\t\tdelete link;\n> >> -\t\t\treturn false;\n> >> -\t\t}\n> >> +\t\tswitch (mediaLinks[i].flags & MEDIA_LNK_FL_LINK_TYPE) {\n> >> +\t\tcase MEDIA_LNK_FL_DATA_LINK:\n> >> +\t\t\tlink = new MediaLink(&mediaLinks[i],\n> >> +\t\t\t\t\t dynamic_cast(source),\n> >> +\t\t\t\t\t dynamic_cast(sink));\n> > \n> > We'll crash if the source or sink isn't a pad (dynamic_cast will return\n> > nullptr in that case). This shouldn't happen unless the kernel reports\n> > an incorrect topology, but it would still be nice to protect against\n> > that. How about the following (untested) ?\n> > \n> > \t\tcase MEDIA_LNK_FL_DATA_LINK: {\n> > \t\t\tMediaPad *sourcePad = dynamic_cast(source);\n> > \t\t\tif (!sourcePad) {\n> > \t\t\t\tLOG(MediaDevice, Error)\n> > \t\t\t\t\t<< \"Source MediaObject \" << source_id\n> > \t\t\t\t\t<< \" is not a pad\";\n> > \t\t\t\treturn false;\n> > \t\t\t}\n> > \n> > \t\t\tMediaPad *sinkPad = dynamic_cast(sink);\n> > \t\t\tif (!sinkPad) {\n> > \t\t\t\tLOG(MediaDevice, Error)\n> > \t\t\t\t\t<< \"Sink MediaObject \" << sink_id\n> > \t\t\t\t\t<< \" is not a pad\";\n> > \t\t\t\treturn false;\n> > \t\t\t}\n> > \n> > \t\t\tMediaLink *link = new MediaLink(&mediaLinks[i],\n> > \t\t\t\t\t\t\tsourcePad, sinkPad);\n> > \t\t\tif (!addObject(link)) {\n> > \t\t\t\tdelete link;\n> > \t\t\t\treturn false;\n> > \t\t\t}\n> > \n> > \t\t\tlink->source()->addLink(link);\n> > \t\t\tlink->sink()->addLink(link);\n> > \n> > \t\t\tbreak;\n> > \t\t}\n> > \n> > \t\tcase MEDIA_LNK_FL_ANCILLARY_LINK: {\n> > \t\t\tMediaEntity *primary = dynamic_cast(source);\n> > \t\t\tif (!primary) {\n> > \t\t\t\tLOG(MediaDevice, Error)\n> > \t\t\t\t\t<< \"Source MediaObject \" << source_id\n> > \t\t\t\t\t<< \" is not an entity\";\n> > \t\t\t\treturn false;\n> > \t\t\t}\n> > \n> > \t\t\tMediaEntity *ancillary = dynamic_cast(sink);\n> > \t\t\tif (!ancillary) {\n> > \t\t\t\tLOG(MediaDevice, Error)\n> > \t\t\t\t\t<< \"Sink MediaObject \" << sink_id\n> > \t\t\t\t\t<< \" is not an entity\";\n> > \t\t\t\treturn false;\n> > \t\t\t}\n> > \n> > \t\t\tprimary->addAncillaryEntity(ancillary);\n> > \t\t\tbreak;\n> > \t\t}\n> > \n> > Or is it overkill ?\n> \n> I lean towards overkill to be honest; since creating data links on the\n> kernel side is through media_create_pad_link(), which explicitly sets\n> the source and sink to pads. So someone would have to invent a new\n> function to add the link in a way that would be broken.\n> \n> Happy to do it though if you prefer.\n\nI agree that having detailed messages is overkill, but I'd like to at\nleast avoid crashing in case something goes wrong (during kernel\ndevelopment for instance). Maybe the following ?\n\n \t\tcase MEDIA_LNK_FL_DATA_LINK: {\n \t\t\tMediaPad *sourcePad = dynamic_cast(source);\n \t\t\tMediaPad *sinkPad = dynamic_cast(sink);\n \t\t\tif (!sourcePad || !sinkPad) {\n \t\t\t\tLOG(MediaDevice, Error)\n \t\t\t\t\t<< \"Source or sink is not a pad\";\n \t\t\t\treturn false;\n \t\t\t}\n \n \t\t\tMediaLink *link = new MediaLink(&mediaLinks[i],\n \t\t\t\t\t\t\tsourcePad, sinkPad);\n \t\t\tif (!addObject(link)) {\n \t\t\t\tdelete link;\n \t\t\t\treturn false;\n \t\t\t}\n \n \t\t\tlink->source()->addLink(link);\n \t\t\tlink->sink()->addLink(link);\n \n \t\t\tbreak;\n \t\t}\n \n \t\tcase MEDIA_LNK_FL_ANCILLARY_LINK: {\n \t\t\tMediaEntity *primary = dynamic_cast(source);\n \t\t\tMediaEntity *ancillary = dynamic_cast(sink);\n \t\t\tif (!primary || !ancillary) {\n \t\t\t\tLOG(MediaDevice, Error)\n \t\t\t\t\t<< \"Source or sink is not an entity\";\n \t\t\t\treturn false;\n \t\t\t}\n \n \t\t\tprimary->addAncillaryEntity(ancillary);\n \t\t\tbreak;\n \t\t}\n\n> >> +\t\t\tif (!addObject(link)) {\n> >> +\t\t\t\tdelete link;\n> >> +\t\t\t\treturn false;\n> >> +\t\t\t}\n> >> +\n> >> +\t\t\tlink->source()->addLink(link);\n> >> +\t\t\tlink->sink()->addLink(link);\n> >> +\n> >> +\t\t\tbreak;\n> >> +\n> >> +\t\tcase MEDIA_LNK_FL_ANCILLARY_LINK:\n> >> +\t\t\tprimary = dynamic_cast(source);\n> >> +\t\t\tancillary = dynamic_cast(sink);\n> >> \n> >> -\t\tsource->addLink(link);\n> >> -\t\tsink->addLink(link);\n> >> +\t\t\tprimary->addAncillaryEntity(ancillary);\n> >> +\n> >> +\t\t\tbreak;\n> >> +\n> >> +\t\tdefault:\n> >> +\t\t\tLOG(MediaDevice, Warning)\n> >> +\t\t\t\t<< \"Unknown media link type\";\n> >> +\n> >> +\t\t\tbreak;\n> >> +\t\t}\n> >> \t}\n> >> \n> >> \treturn true;","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 CA236BDB13\n\tfor ;\n\tFri, 10 Dec 2021 01:00:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 105CC6086A;\n\tFri, 10 Dec 2021 02:00:10 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E657760113\n\tfor ;\n\tFri, 10 Dec 2021 02:00:08 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 57B2890E;\n\tFri, 10 Dec 2021 02:00:08 +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=\"FQnrO7uo\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1639098008;\n\tbh=9gyY6XQrMcLZyjK/tyEl9EkBYuuqbGUCut01w2xCcGk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=FQnrO7uoQ2bi0D6+WXBu85HFAS3vs+40Y9ww4jZygv7r5lEPF4PiKyq6pEwdNqJw1\n\tdEnf/1F3g405+INADABFjE2UYpqdj8qyNz9o+60ouwZpEIFuVMzOH6IM1tsCoKknEK\n\t1k8UT+sCLknAiNK+pTILGw5MIBTgKo1O080DXOeo=","Date":"Fri, 10 Dec 2021 02:59:37 +0200","From":"Laurent Pinchart ","To":"Daniel Scally ","Message-ID":"","References":"<20211207224512.753979-1-djrscally@gmail.com>\n\t<20211207224512.753979-4-djrscally@gmail.com>\n\t\n\t<539a8a46-ad3d-82c6-73bb-499111ad2c62@gmail.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<539a8a46-ad3d-82c6-73bb-499111ad2c62@gmail.com>","Subject":"Re: [libcamera-devel] [PATCH v3 3/9] libcamera: media_device:\n\tHandle ancillary links in populateLinks()","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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]