[{"id":3277,"web_url":"https://patchwork.libcamera.org/comment/3277/","msgid":"<20191227123745.5x2ck3lyfynbs2tt@uno.localdomain>","date":"2019-12-27T12:37:45","subject":"Re: [libcamera-devel] [PATCH v3 2/6] libcamera: pipeline_handler:\n\tAdd map from devnum to cameras","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Paul,\n\nOn Mon, Dec 23, 2019 at 01:26:16AM -0600, Paul Elder wrote:\n> The V4L2 compatibility layer will need a way to map device numbers to\n> libcamera Cameras. As a first step, we add a map from devnum to Cameras\n> to PipelineHandler, as well as an overloaded registerCamera() method to\n> add to this map.\n>\n> Signed-off-by: Paul Elder \n>\n> ---\n> New in v3\n> ---\n> src/libcamera/include/pipeline_handler.h | 6 +++++\n> src/libcamera/pipeline_handler.cpp | 30 ++++++++++++++++++++++++\n> 2 files changed, 36 insertions(+)\n>\n> diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h\n> index f3622631..563de72a 100644\n> --- a/src/libcamera/include/pipeline_handler.h\n> +++ b/src/libcamera/include/pipeline_handler.h\n> @@ -12,6 +12,7 @@\n> #include \n> #include \n> #include \n> +#include \n> #include \n>\n> #include \n> @@ -84,9 +85,13 @@ public:\n>\n> \tconst char *name() const { return name_; }\n>\n> +\tconst std::map> &camerasByDevnum() const { return camerasByDevnum_; }\n> +\n> protected:\n> \tvoid registerCamera(std::shared_ptr camera,\n> \t\t\t std::unique_ptr data);\n> +\tvoid registerCamera(std::shared_ptr camera,\n> +\t\t\t std::unique_ptr data, dev_t devnum);\n\nJust wondering if it would not be better to have a single\nimplementation with devnum = 0 (it can't be 0, right ? )\n\n> \tvoid hotplugMediaDevice(MediaDevice *media);\n>\n> \tvirtual int queueRequestDevice(Camera *camera, Request *request) = 0;\n> @@ -102,6 +107,7 @@ private:\n> \tstd::vector> mediaDevices_;\n> \tstd::vector> cameras_;\n> \tstd::map> cameraData_;\n> +\tstd::map> camerasByDevnum_;\n>\n> \tconst char *name_;\n>\n> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> index 5badf31c..90211f12 100644\n> --- a/src/libcamera/pipeline_handler.cpp\n> +++ b/src/libcamera/pipeline_handler.cpp\n> @@ -7,6 +7,8 @@\n>\n> #include \"pipeline_handler.h\"\n>\n> +#include \n> +\n> #include \n> #include \n> #include \n> @@ -453,6 +455,28 @@ void PipelineHandler::registerCamera(std::shared_ptr camera,\n> \tmanager_->addCamera(std::move(camera));\n> }\n>\n> +/**\n> + * \\brief Register a camera to the camera manager and pipeline handler\n\nNit:\nIf you keep it separate, I would add \"Register a camera associated\nwith \\a devnum to... \"\n\n> + * \\param[in] camera The camera to be added\n> + * \\param[in] data Pipeline-specific data for the camera\n> + * \\param[in] devnum Device number of the camera\n> + *\n> + * This method is called by pipeline handlers to register the cameras they\n> + * handle with the camera manager. It associates the pipeline-specific \\a data\n> + * with the camera, for later retrieval with cameraData(). Ownership of \\a data\n> + * is transferred to the PipelineHandler.\n> + *\n> + * \\a devnum is the device number (as returned by makedev) that the \\a camera\n> + * is to be associated with.\n> + */\n> +void PipelineHandler::registerCamera(std::shared_ptr camera,\n> +\t\t\t\t std::unique_ptr data,\n> +\t\t\t\t dev_t devnum)\n> +{\n> +\tregisterCamera(camera, std::move(data));\n> +\tcamerasByDevnum_[devnum] = camera;\n> +}\n> +\n> /**\n> * \\brief Enable hotplug handling for a media device\n> * \\param[in] media The media device\n> @@ -538,6 +562,12 @@ CameraData *PipelineHandler::cameraData(const Camera *camera)\n> * \\return The pipeline handler name\n> */\n>\n> +/**\n> + * \\fn PipelineHandler::camerasByDevnum()\n> + * \\brief Retrieve the map of devnums to cameras\n> + * \\return The map of devnums to cameras\n\ns/devnums/device numbers/ ?\n\nNit aparts:\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n> + */\n> +\n> /**\n> * \\class PipelineHandlerFactory\n> * \\brief Registration of PipelineHandler classes and creation of instances\n> --\n> 2.23.0\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","Received":["from relay5-d.mail.gandi.net (relay5-d.mail.gandi.net\n\t[217.70.183.197])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A600A6046D\n\tfor ;\n\tFri, 27 Dec 2019 13:35:33 +0100 (CET)","from uno.localdomain (93-34-114-233.ip49.fastwebnet.it\n\t[93.34.114.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay5-d.mail.gandi.net (Postfix) with ESMTPSA id 264961C0007;\n\tFri, 27 Dec 2019 12:35:32 +0000 (UTC)"],"X-Originating-IP":"93.34.114.233","Date":"Fri, 27 Dec 2019 13:37:45 +0100","From":"Jacopo Mondi ","To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20191227123745.5x2ck3lyfynbs2tt@uno.localdomain>","References":"<20191223072620.13022-1-paul.elder@ideasonboard.com>\n\t<20191223072620.13022-3-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"urs4trsj2kal67dl\"","Content-Disposition":"inline","In-Reply-To":"<20191223072620.13022-3-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 2/6] libcamera: pipeline_handler:\n\tAdd map from devnum to cameras","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","X-List-Received-Date":"Fri, 27 Dec 2019 12:35:33 -0000"}},{"id":3281,"web_url":"https://patchwork.libcamera.org/comment/3281/","msgid":"<20191227145353.GC4769@pendragon.ideasonboard.com>","date":"2019-12-27T14:53:53","subject":"Re: [libcamera-devel] [PATCH v3 2/6] libcamera: pipeline_handler:\n\tAdd map from devnum to cameras","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Paul,\n\nThank you for the patch.\n\nOn Fri, Dec 27, 2019 at 01:37:45PM +0100, Jacopo Mondi wrote:\n> On Mon, Dec 23, 2019 at 01:26:16AM -0600, Paul Elder wrote:\n> > The V4L2 compatibility layer will need a way to map device numbers to\n> > libcamera Cameras. As a first step, we add a map from devnum to Cameras\n\nIt's no big deal in the commit message, but we don't pluralize class\nnames (\"Cameras\") in the documentation as Doxygen would then not\nrecognize them. This should then be \"cameras\" or \"Camera instances\" if\nyou want to be consistent here.\n\n> > to PipelineHandler, as well as an overloaded registerCamera() method to\n> > add to this map.\n> >\n> > Signed-off-by: Paul Elder \n> >\n> > ---\n> > New in v3\n> > ---\n> > src/libcamera/include/pipeline_handler.h | 6 +++++\n> > src/libcamera/pipeline_handler.cpp | 30 ++++++++++++++++++++++++\n> > 2 files changed, 36 insertions(+)\n> >\n> > diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h\n> > index f3622631..563de72a 100644\n> > --- a/src/libcamera/include/pipeline_handler.h\n> > +++ b/src/libcamera/include/pipeline_handler.h\n> > @@ -12,6 +12,7 @@\n> > #include \n> > #include \n> > #include \n> > +#include \n> > #include \n> >\n> > #include \n> > @@ -84,9 +85,13 @@ public:\n> >\n> > \tconst char *name() const { return name_; }\n> >\n> > +\tconst std::map> &camerasByDevnum() const { return camerasByDevnum_; }\n> > +\n> > protected:\n> > \tvoid registerCamera(std::shared_ptr camera,\n> > \t\t\t std::unique_ptr data);\n> > +\tvoid registerCamera(std::shared_ptr camera,\n> > +\t\t\t std::unique_ptr data, dev_t devnum);\n> \n> Just wondering if it would not be better to have a single\n> implementation with devnum = 0 (it can't be 0, right ? )\n\nCorrect, 0 is documented as \"reserved as null device number\". I think a\nsingle implementation is indeed better.\n\n> > \tvoid hotplugMediaDevice(MediaDevice *media);\n> >\n> > \tvirtual int queueRequestDevice(Camera *camera, Request *request) = 0;\n> > @@ -102,6 +107,7 @@ private:\n> > \tstd::vector> mediaDevices_;\n> > \tstd::vector> cameras_;\n> > \tstd::map> cameraData_;\n> > +\tstd::map> camerasByDevnum_;\n\nCould we do away with storage of the map in the PipelineHandler, as we\nhave one in the CameraManager ? I think you can just pass the devnum to\nthe CameraManager::addCamera() method in\nPipelineHandler::registerCamera().\n\n> >\n> > \tconst char *name_;\n> >\n> > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> > index 5badf31c..90211f12 100644\n> > --- a/src/libcamera/pipeline_handler.cpp\n> > +++ b/src/libcamera/pipeline_handler.cpp\n> > @@ -7,6 +7,8 @@\n> >\n> > #include \"pipeline_handler.h\"\n> >\n> > +#include \n> > +\n> > #include \n> > #include \n> > #include \n> > @@ -453,6 +455,28 @@ void PipelineHandler::registerCamera(std::shared_ptr camera,\n> > \tmanager_->addCamera(std::move(camera));\n> > }\n> >\n> > +/**\n> > + * \\brief Register a camera to the camera manager and pipeline handler\n> \n> Nit:\n> If you keep it separate, I would add \"Register a camera associated\n> with \\a devnum to... \"\n> \n> > + * \\param[in] camera The camera to be added\n> > + * \\param[in] data Pipeline-specific data for the camera\n> > + * \\param[in] devnum Device number of the camera\n> > + *\n> > + * This method is called by pipeline handlers to register the cameras they\n> > + * handle with the camera manager. It associates the pipeline-specific \\a data\n> > + * with the camera, for later retrieval with cameraData(). Ownership of \\a data\n> > + * is transferred to the PipelineHandler.\n> > + *\n> > + * \\a devnum is the device number (as returned by makedev) that the \\a camera\n> > + * is to be associated with.\n\nLet's expand this slightly, to explain the purpose of the association,\notherwise it's difficult for pipeline handler authors to figure out what\ndevnum to pass. \n\n> > + */\n> > +void PipelineHandler::registerCamera(std::shared_ptr camera,\n> > +\t\t\t\t std::unique_ptr data,\n> > +\t\t\t\t dev_t devnum)\n> > +{\n> > +\tregisterCamera(camera, std::move(data));\n> > +\tcamerasByDevnum_[devnum] = camera;\n> > +}\n> > +\n> > /**\n> > * \\brief Enable hotplug handling for a media device\n> > * \\param[in] media The media device\n> > @@ -538,6 +562,12 @@ CameraData *PipelineHandler::cameraData(const Camera *camera)\n> > * \\return The pipeline handler name\n> > */\n> >\n> > +/**\n> > + * \\fn PipelineHandler::camerasByDevnum()\n> > + * \\brief Retrieve the map of devnums to cameras\n> > + * \\return The map of devnums to cameras\n> \n> s/devnums/device numbers/ ?\n> \n> Nit aparts:\n> Reviewed-by: Jacopo Mondi \n> \n> > + */\n> > +\n> > /**\n> > * \\class PipelineHandlerFactory\n> > * \\brief Registration of PipelineHandler classes and creation of instances","headers":{"Return-Path":"","Received":["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 87E386046D\n\tfor ;\n\tFri, 27 Dec 2019 15:54:05 +0100 (CET)","from pendragon.ideasonboard.com (unknown [91.179.177.25])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id EBF05DD;\n\tFri, 27 Dec 2019 15:54:04 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1577458445;\n\tbh=r8dzhSX6IsbAKEl7Uuce1v0908zd4j3rtmH3sUFw2ng=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=AwVtUBkiV6xevmXTVpI+m0T6cMJ7NFPR9dMwYyGfZSWOl43Tzf7b7e9js+fexV8S4\n\tUOqCOBr1w5ApJY5FuQ/YAi8LXKShz3U5BbIpBvZrvoaXCdiwaMMFPDr3lOquey0EYN\n\t0N/0jU0LU5d2N9rcE2kaCOoj+mp6AEqJ8sRVyZZ4=","Date":"Fri, 27 Dec 2019 16:53:53 +0200","From":"Laurent Pinchart ","To":"Paul Elder ","Cc":"Jacopo Mondi , libcamera-devel@lists.libcamera.org","Message-ID":"<20191227145353.GC4769@pendragon.ideasonboard.com>","References":"<20191223072620.13022-1-paul.elder@ideasonboard.com>\n\t<20191223072620.13022-3-paul.elder@ideasonboard.com>\n\t<20191227123745.5x2ck3lyfynbs2tt@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20191227123745.5x2ck3lyfynbs2tt@uno.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v3 2/6] libcamera: pipeline_handler:\n\tAdd map from devnum to cameras","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","X-List-Received-Date":"Fri, 27 Dec 2019 14:54:05 -0000"}}]