[{"id":1430,"web_url":"https://patchwork.libcamera.org/comment/1430/","msgid":"<20190417104915.GC4993@pendragon.ideasonboard.com>","date":"2019-04-17T10:49:15","subject":"Re: [libcamera-devel] [RFC 06/11] libcamera: pipeline_handler: Keep\n\ttrack of MediaDevice","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nThank you for the patch.\n\nOn Sun, Apr 14, 2019 at 03:35:01AM +0200, Niklas Söderlund wrote:\n> Instead of requiring each pipeline handle implementation to keep track\n> of calling release() on its media devices upon deletion keep track of\n> them in the base class. Add a helper which pipeline handlers shall use\n> to acquire a media device instead of directly interacting with the\n> DeviceEnumerator.\n> \n> Centrally keeping track of media devices will also be beneficial\n> implementing pipeline exclusive access across processes.\n> \n> Signed-off-by: Niklas Söderlund \n> ---\n> src/libcamera/include/pipeline_handler.h | 4 ++++\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 20 ++------------------\n> src/libcamera/pipeline/uvcvideo.cpp | 24 +++++++-----------------\n> src/libcamera/pipeline/vimc.cpp | 22 ++++++----------------\n> src/libcamera/pipeline_handler.cpp | 22 ++++++++++++++++++++++\n> 5 files changed, 41 insertions(+), 51 deletions(-)\n> \n> diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h\n> index b6cbd3bae51b08dc..d995a7e56d90f706 100644\n> --- a/src/libcamera/include/pipeline_handler.h\n> +++ b/src/libcamera/include/pipeline_handler.h\n> @@ -21,6 +21,7 @@ class Camera;\n> class CameraConfiguration;\n> class CameraManager;\n> class DeviceEnumerator;\n> +class DeviceMatch;\n> class MediaDevice;\n> class PipelineHandler;\n> class Request;\n> @@ -52,6 +53,8 @@ public:\n> \tvirtual ~PipelineHandler();\n> \n> \tvirtual bool match(DeviceEnumerator *enumerator) = 0;\n> +\tstd::shared_ptr tryAcquire(DeviceEnumerator *enumerator,\n> +\t\t\t\t\t\tconst DeviceMatch &dm);\n\nGiven that this function stores a reference internally in\nPipelineHandler, which is guaranteed to remain valid until the pipeline\nhandler is destroyed, can't we return a MediaDevice * instead of a\nshared pointer ?\n\n> \n> \tvirtual CameraConfiguration\n> \tstreamConfiguration(Camera *camera, const std::vector &usages) = 0;\n> @@ -81,6 +84,7 @@ private:\n> \tvoid mediaDeviceDisconnected(MediaDevice *media);\n> \tvirtual void disconnect();\n> \n> +\tstd::vector> mediaDevices_;\n> \tstd::vector> cameras_;\n> \tstd::map> cameraData_;\n> };\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index a87eff8dfec6d143..77d14533e6bceb80 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -137,7 +137,6 @@ class PipelineHandlerIPU3 : public PipelineHandler\n> {\n> public:\n> \tPipelineHandlerIPU3(CameraManager *manager);\n> -\t~PipelineHandlerIPU3();\n> \n> \tCameraConfiguration\n> \tstreamConfiguration(Camera *camera,\n> @@ -195,15 +194,6 @@ PipelineHandlerIPU3::PipelineHandlerIPU3(CameraManager *manager)\n> {\n> }\n> \n> -PipelineHandlerIPU3::~PipelineHandlerIPU3()\n> -{\n> -\tif (cio2MediaDev_)\n> -\t\tcio2MediaDev_->release();\n> -\n> -\tif (imguMediaDev_)\n> -\t\timguMediaDev_->release();\n> -}\n> -\n> CameraConfiguration\n> PipelineHandlerIPU3::streamConfiguration(Camera *camera,\n> \t\t\t\t\t const std::vector &usages)\n> @@ -451,20 +441,14 @@ bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator)\n> \timgu_dm.add(\"ipu3-imgu 1 viewfinder\");\n> \timgu_dm.add(\"ipu3-imgu 1 3a stat\");\n> \n> -\tcio2MediaDev_ = enumerator->search(cio2_dm);\n> +\tcio2MediaDev_ = tryAcquire(enumerator, cio2_dm);\n> \tif (!cio2MediaDev_)\n> \t\treturn false;\n> \n> -\tif (!cio2MediaDev_->acquire())\n> -\t\treturn false;\n> -\n> -\timguMediaDev_ = enumerator->search(imgu_dm);\n> +\timguMediaDev_ = tryAcquire(enumerator, imgu_dm);\n> \tif (!imguMediaDev_)\n> \t\treturn false;\n> \n> -\tif (!imguMediaDev_->acquire())\n> -\t\treturn false;\n> -\n> \t/*\n> \t * Disable all links that are enabled by default on CIO2, as camera\n> \t * creation enables all valid links it finds.\n> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp\n> index 5214bfd3097b8217..43a6d59670299376 100644\n> --- a/src/libcamera/pipeline/uvcvideo.cpp\n> +++ b/src/libcamera/pipeline/uvcvideo.cpp\n> @@ -24,7 +24,6 @@ class PipelineHandlerUVC : public PipelineHandler\n> {\n> public:\n> \tPipelineHandlerUVC(CameraManager *manager);\n> -\t~PipelineHandlerUVC();\n> \n> \tCameraConfiguration\n> \tstreamConfiguration(Camera *camera,\n> @@ -68,20 +67,13 @@ private:\n> \t\t\tPipelineHandler::cameraData(camera));\n> \t}\n> \n> -\tstd::shared_ptr media_;\n> };\n> \n> PipelineHandlerUVC::PipelineHandlerUVC(CameraManager *manager)\n> -\t: PipelineHandler(manager), media_(nullptr)\n> +\t: PipelineHandler(manager)\n> {\n> }\n> \n> -PipelineHandlerUVC::~PipelineHandlerUVC()\n> -{\n> -\tif (media_)\n> -\t\tmedia_->release();\n> -}\n> -\n> CameraConfiguration\n> PipelineHandlerUVC::streamConfiguration(Camera *camera,\n> \t\t\t\t\tconst std::vector &usages)\n> @@ -178,19 +170,17 @@ int PipelineHandlerUVC::queueRequest(Camera *camera, Request *request)\n> \n> bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator)\n> {\n> +\tstd::shared_ptr media;\n> \tDeviceMatch dm(\"uvcvideo\");\n> \n> -\tmedia_ = enumerator->search(dm);\n> -\tif (!media_)\n> -\t\treturn false;\n> -\n> -\tif (!media_->acquire())\n> +\tmedia = tryAcquire(enumerator, dm);\n> +\tif (!media)\n> \t\treturn false;\n> \n> \tstd::unique_ptr data = utils::make_unique(this);\n> \n> \t/* Locate and open the default video node. */\n> -\tfor (MediaEntity *entity : media_->entities()) {\n> +\tfor (MediaEntity *entity : media->entities()) {\n> \t\tif (entity->flags() & MEDIA_ENT_FL_DEFAULT) {\n> \t\t\tdata->video_ = new V4L2Device(entity);\n> \t\t\tbreak;\n> @@ -209,11 +199,11 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator)\n> \n> \t/* Create and register the camera. */\n> \tstd::set streams{ &data->stream_ };\n> -\tstd::shared_ptr camera = Camera::create(this, media_->model(), streams);\n> +\tstd::shared_ptr camera = Camera::create(this, media->model(), streams);\n> \tregisterCamera(std::move(camera), std::move(data));\n> \n> \t/* Enable hot-unplug notifications. */\n> -\thotplugMediaDevice(media_.get());\n> +\thotplugMediaDevice(media.get());\n> \n> \treturn true;\n> }\n> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp\n> index e5e78ccedd59ae66..a80331e1cd4d1e46 100644\n> --- a/src/libcamera/pipeline/vimc.cpp\n> +++ b/src/libcamera/pipeline/vimc.cpp\n> @@ -24,7 +24,6 @@ class PipelineHandlerVimc : public PipelineHandler\n> {\n> public:\n> \tPipelineHandlerVimc(CameraManager *manager);\n> -\t~PipelineHandlerVimc();\n> \n> \tCameraConfiguration\n> \tstreamConfiguration(Camera *camera,\n> @@ -67,21 +66,13 @@ private:\n> \t\treturn static_cast(\n> \t\t\tPipelineHandler::cameraData(camera));\n> \t}\n> -\n> -\tstd::shared_ptr media_;\n> };\n> \n> PipelineHandlerVimc::PipelineHandlerVimc(CameraManager *manager)\n> -\t: PipelineHandler(manager), media_(nullptr)\n> +\t: PipelineHandler(manager)\n> {\n> }\n> \n> -PipelineHandlerVimc::~PipelineHandlerVimc()\n> -{\n> -\tif (media_)\n> -\t\tmedia_->release();\n> -}\n> -\n> CameraConfiguration\n> PipelineHandlerVimc::streamConfiguration(Camera *camera,\n> \t\t\t\t\t const std::vector &usages)\n> @@ -178,6 +169,8 @@ int PipelineHandlerVimc::queueRequest(Camera *camera, Request *request)\n> \n> bool PipelineHandlerVimc::match(DeviceEnumerator *enumerator)\n> {\n> +\tstd::shared_ptr media;\n> +\n> \tDeviceMatch dm(\"vimc\");\n> \n> \tdm.add(\"Raw Capture 0\");\n> @@ -190,17 +183,14 @@ bool PipelineHandlerVimc::match(DeviceEnumerator *enumerator)\n> \tdm.add(\"RGB/YUV Input\");\n> \tdm.add(\"Scaler\");\n> \n> -\tmedia_ = enumerator->search(dm);\n> -\tif (!media_)\n> -\t\treturn false;\n> -\n> -\tif (!media_->acquire())\n> +\tmedia = tryAcquire(enumerator, dm);\n> +\tif (!media)\n> \t\treturn false;\n> \n> \tstd::unique_ptr data = utils::make_unique(this);\n> \n> \t/* Locate and open the capture video node. */\n> -\tdata->video_ = new V4L2Device(media_->getEntityByName(\"Raw Capture 1\"));\n> +\tdata->video_ = new V4L2Device(media->getEntityByName(\"Raw Capture 1\"));\n> \tif (data->video_->open())\n> \t\treturn false;\n> \n> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> index 43550c0e0210b7b3..7f4035c008f95f91 100644\n> --- a/src/libcamera/pipeline_handler.cpp\n> +++ b/src/libcamera/pipeline_handler.cpp\n> @@ -9,6 +9,7 @@\n> #include \n> #include \n> \n> +#include \"device_enumerator.h\"\n> #include \"log.h\"\n> #include \"media_device.h\"\n> #include \"pipeline_handler.h\"\n> @@ -115,6 +116,8 @@ PipelineHandler::PipelineHandler(CameraManager *manager)\n> \n> PipelineHandler::~PipelineHandler()\n> {\n> +\tfor (std::shared_ptr media : mediaDevices_)\n> +\t\tmedia->release();\n> };\n> \n> /**\n> @@ -148,6 +151,25 @@ PipelineHandler::~PipelineHandler()\n> * created, or false otherwise\n> */\n> \n> +std::shared_ptr\n> +PipelineHandler::tryAcquire(DeviceEnumerator *enumerator, const DeviceMatch &dm)\n\nMissing documentation.\n\n> +{\n> +\tstd::shared_ptr media;\n> +\n> +\tmedia = enumerator->search(dm);\n> +\tif (!media)\n> +\t\tgoto out;\n> +\n> +\tif (!media->acquire()) {\n> +\t\tmedia.reset();\n> +\t\tgoto out;\n> +\t}\n> +\n> +\tmediaDevices_.push_back(media);\n> +out:\n> +\treturn media;\n> +}\n> +\n> /**\n> * \\fn PipelineHandler::streamConfiguration()\n> * \\brief Retrieve a group of stream configurations for a specified camera","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C8F7160DBE\n\tfor ;\n\tWed, 17 Apr 2019 12:49:24 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(dfj612yhrgyx302h3jwwy-3.rev.dnainternet.fi\n\t[IPv6:2001:14ba:21f5:5b00:ce28:277f:58d7:3ca4])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 3DC9F312;\n\tWed, 17 Apr 2019 12:49:24 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1555498164;\n\tbh=STTPg1+7pE51Gojeo/nA0z7fsv256TDrm8Wp6r2uyzE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=cgHsO3qimR/yyIACVOaGGucTuxmrbqnlR20gDq2jqY2ff/8B72DSabnhZN/ARvoIW\n\tNJj70DvmUhIP/sSddpR22jVc+HEeW8dL+VIJPBUxI8tf12II/MDA3PedN+rDGn7fM3\n\tbjEVTH4J9eDxHJEvrRhvDn6s6iD57WyAQsIm+GKo=","Date":"Wed, 17 Apr 2019 13:49:15 +0300","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190417104915.GC4993@pendragon.ideasonboard.com>","References":"<20190414013506.10515-1-niklas.soderlund@ragnatech.se>\n\t<20190414013506.10515-7-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190414013506.10515-7-niklas.soderlund@ragnatech.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [RFC 06/11] libcamera: pipeline_handler: Keep\n\ttrack of MediaDevice","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Wed, 17 Apr 2019 10:49:25 -0000"}},{"id":1536,"web_url":"https://patchwork.libcamera.org/comment/1536/","msgid":"<20190429183018.GL16573@bigcity.dyn.berto.se>","date":"2019-04-29T18:30:18","subject":"Re: [libcamera-devel] [RFC 06/11] libcamera: pipeline_handler: Keep\n\ttrack of MediaDevice","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Laurent,\n\nThanks for your feedback.\n\nOn 2019-04-17 13:49:15 +0300, Laurent Pinchart wrote:\n> > diff --git a/src/libcamera/include/pipeline_handler.h \n> > b/src/libcamera/include/pipeline_handler.h\n> > index b6cbd3bae51b08dc..d995a7e56d90f706 100644\n> > --- a/src/libcamera/include/pipeline_handler.h\n> > +++ b/src/libcamera/include/pipeline_handler.h\n> > @@ -21,6 +21,7 @@ class Camera;\n> > class CameraConfiguration;\n> > class CameraManager;\n> > class DeviceEnumerator;\n> > +class DeviceMatch;\n> > class MediaDevice;\n> > class PipelineHandler;\n> > class Request;\n> > @@ -52,6 +53,8 @@ public:\n> > \tvirtual ~PipelineHandler();\n> > \n> > \tvirtual bool match(DeviceEnumerator *enumerator) = 0;\n> > +\tstd::shared_ptr tryAcquire(DeviceEnumerator *enumerator,\n> > +\t\t\t\t\t\tconst DeviceMatch &dm);\n> \n> Given that this function stores a reference internally in\n> PipelineHandler, which is guaranteed to remain valid until the pipeline\n> handler is destroyed, can't we return a MediaDevice * instead of a\n> shared pointer ?\n\nWe could do that, this would make pipeline handler implementations a tad \nmore simple as they no longer need to store a shared_ptr<> for the media \ndevices it needs to use during it's lifetime. \n\nThanks for pointing this out!","headers":{"Return-Path":"","Received":["from mail-lf1-x136.google.com (mail-lf1-x136.google.com\n\t[IPv6:2a00:1450:4864:20::136])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1C79E600EC\n\tfor ;\n\tMon, 29 Apr 2019 20:30:20 +0200 (CEST)","by mail-lf1-x136.google.com with SMTP id i68so8629585lfi.10\n\tfor ;\n\tMon, 29 Apr 2019 11:30:20 -0700 (PDT)","from localhost (89-233-230-99.cust.bredband2.com. [89.233.230.99])\n\tby smtp.gmail.com with ESMTPSA id\n\tx2sm2185152ljx.13.2019.04.29.11.30.18\n\t(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);\n\tMon, 29 Apr 2019 11:30:18 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to\n\t:user-agent; bh=TviAknBRvxQXyRVU07GsIQxi2+SRggG1ZhEsyOeJc1Y=;\n\tb=Gu1zuOP7EnLhW36A7L5WhIgb5ByhXwolNFcntay4hEihHsPb5F5pJRwxuv/4mdH9RY\n\thVA3hNgq036dkdvZgNyUiKtt/DIQYAz5OMzuTOhwrMqDN7Ic71qw9aVU+5MPPty9IA7L\n\t1tayFYv6gTAGpfkVJwynDHbobvCjLhFkVPIaEDbm+rg7MofHopo+OmYj89VDXKCDah/2\n\to1arQIWs8T7GsQC7Y5MJqySBffE5b+Xwu6ddnZu4Q9v6iDHtOEoALiuejCEvregsgFQs\n\t7IqqzGz2aOTNlTiIT5DM6wKNEtJ5cchssqbaJyz9l/aF6GSJhENfheVkbQLJJKWvoiQF\n\tiuYQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to:user-agent;\n\tbh=TviAknBRvxQXyRVU07GsIQxi2+SRggG1ZhEsyOeJc1Y=;\n\tb=nWGd6o/7R8/YuXVdUT0LsDkbqdJWEaAEOMcENF5zxuGZRdVFVGz9DJSARfDU0LK7PX\n\tH7B3iMaRceDcq7TGqZezOXFmxFO1MXmHZ5IzHGPkNIlukphg/fBxFxFBqwbfKxIPBRSd\n\tjLxevDUG4YZbeMRBpCPwOlby38bp5eoT9ms4sLj0cdxfyQ621KKk8tmaEXe1vhIiOKFe\n\tZ/drlyslCEYKok8D/86QHAieN8YEbEwP+FF9PTjSVmfPS7oBZnoHV8TGdGzw9znQpw8S\n\t+ASmzewrJTG21Auhv09nq1oM3ZCkzByRryiMVwxuzO4Ib0gPlMO6qDu4iYBG/7L4i4yK\n\tUdLA==","X-Gm-Message-State":"APjAAAUFR2iFPHn0Hg/dTC5CLpMC25976uhY2u9XGWRggKcasG86j/eW\n\tMbDoXYbiErEQlt8t33mlKRLZbw==","X-Google-Smtp-Source":"APXvYqxiPnJQIblpbf3MIr60PulCRjPrbpGZOzcdgZaCeMzVRQKUEO6crgAJORE4+gN7MhASV6o94w==","X-Received":"by 2002:a19:6f4d:: with SMTP id\n\tn13mr13716487lfk.57.1556562619565; \n\tMon, 29 Apr 2019 11:30:19 -0700 (PDT)","Date":"Mon, 29 Apr 2019 20:30:18 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190429183018.GL16573@bigcity.dyn.berto.se>","References":"<20190414013506.10515-1-niklas.soderlund@ragnatech.se>\n\t<20190414013506.10515-7-niklas.soderlund@ragnatech.se>\n\t<20190417104915.GC4993@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190417104915.GC4993@pendragon.ideasonboard.com>","User-Agent":"Mutt/1.11.3 (2019-02-01)","Subject":"Re: [libcamera-devel] [RFC 06/11] libcamera: pipeline_handler: Keep\n\ttrack of MediaDevice","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Mon, 29 Apr 2019 18:30:20 -0000"}}]