[{"id":3278,"web_url":"https://patchwork.libcamera.org/comment/3278/","msgid":"<20191227125805.elgtw2kxev2mw4nr@uno.localdomain>","date":"2019-12-27T12:58:05","subject":"Re: [libcamera-devel] [PATCH v3 4/6] libcamera: camera_manager:\n\tallow retrieving cameras by devnum","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:18AM -0600, Paul Elder wrote:\n> Expose a method to retrieve Cameras by devnum. The map of devnum to\n\nCamera instances by device number ?\n\n> Camera is gathered from all PipelineHandlers. This method is mainly for\n\nipeline handlers\n\n> the V4L2 compatibility layer, in order to match V4L2 devices to\n> libcamera Cameras.\n\nCamera\n\nDo not pluralize class names :)\n\n>\n> Signed-off-by: Paul Elder \n>\n> ---\n> New in v3\n> ---\n> include/libcamera/camera_manager.h | 3 +++\n> src/libcamera/camera_manager.cpp | 24 ++++++++++++++++++++++++\n> 2 files changed, 27 insertions(+)\n>\n> diff --git a/include/libcamera/camera_manager.h b/include/libcamera/camera_manager.h\n> index 8331898c..6ce63912 100644\n> --- a/include/libcamera/camera_manager.h\n> +++ b/include/libcamera/camera_manager.h\n> @@ -7,6 +7,7 @@\n> #ifndef __LIBCAMERA_CAMERA_MANAGER_H__\n> #define __LIBCAMERA_CAMERA_MANAGER_H__\n>\n> +#include \n> #include \n> #include \n> #include \n> @@ -33,6 +34,7 @@ public:\n>\n> \tconst std::vector> &cameras() const { return cameras_; }\n> \tstd::shared_ptr get(const std::string &name);\n> +\tstd::shared_ptr get(dev_t devnum);\n>\n> \tvoid addCamera(std::shared_ptr camera);\n> \tvoid removeCamera(Camera *camera);\n> @@ -46,6 +48,7 @@ private:\n> \tstd::unique_ptr enumerator_;\n> \tstd::vector> pipes_;\n> \tstd::vector> cameras_;\n> +\tstd::map> camerasByDevnum_;\n\nDo you need to store these as shared pointers ?\nI don't think it's a big deal, but CameraManager already has a vector\nof Camera shared_ptr, adding one reference (for some cameras only, the\nones created with a devnum associated) might not be needed. I would\nstore them as weak_ptr and return iter->second.lock() in the get()\nimplementation.\n\nThe implications are minor, so it's really up to you.\n\n>\n> \tstatic const std::string version_;\n> \tstatic CameraManager *self_;\n> diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp\n> index 7c6f72bb..1354df4c 100644\n> --- a/src/libcamera/camera_manager.cpp\n> +++ b/src/libcamera/camera_manager.cpp\n> @@ -121,6 +121,11 @@ int CameraManager::start()\n> \t\t}\n> \t}\n>\n> +\tfor (std::shared_ptr pipe : pipes_) {\n> +\t\tconst std::map> devMap = pipe->camerasByDevnum();\n\n&devMap\n\n> +\t\tcamerasByDevnum_.insert(devMap.begin(), devMap.end());\n> +\t}\n> +\n> \t/* TODO: register hot-plug callback here */\n>\n> \treturn 0;\n> @@ -180,6 +185,25 @@ std::shared_ptr CameraManager::get(const std::string &name)\n> \treturn nullptr;\n> }\n>\n> +/**\n> + * \\brief Get a camera based on devnum\n\n\"Retrieve a camera\" ? Even if the other get() impmentation has this\nvery same documentation\n\n> + * \\param[in] devnum Device number of camera to get\n> + *\n> + * Before calling this function the caller is responsible for ensuring that\n> + * the camera manager is running.\n> + *\n> + * \\return Shared pointer to Camera object or nullptr if camera not found\n> + */\n> +std::shared_ptr CameraManager::get(dev_t devnum)\n> +{\n> +\tauto iter = camerasByDevnum_.find(devnum);\n> +\n> +\tif (iter == camerasByDevnum_.end())\n> +\t\treturn nullptr;\n> +\n> +\treturn iter->second;\n> +}\n> +\n\nMinors apart:\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n> /**\n> * \\brief Add a camera to the camera manager\n> * \\param[in] camera The camera to be added\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 relay10.mail.gandi.net (relay10.mail.gandi.net\n\t[217.70.178.230])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 235EB6046D\n\tfor ;\n\tFri, 27 Dec 2019 13:55:52 +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 relay10.mail.gandi.net (Postfix) with ESMTPSA id 7A811240009;\n\tFri, 27 Dec 2019 12:55:51 +0000 (UTC)"],"Date":"Fri, 27 Dec 2019 13:58:05 +0100","From":"Jacopo Mondi ","To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20191227125805.elgtw2kxev2mw4nr@uno.localdomain>","References":"<20191223072620.13022-1-paul.elder@ideasonboard.com>\n\t<20191223072620.13022-5-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"m4lmochv4woy6dds\"","Content-Disposition":"inline","In-Reply-To":"<20191223072620.13022-5-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 4/6] libcamera: camera_manager:\n\tallow retrieving cameras by devnum","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:55:52 -0000"}},{"id":3284,"web_url":"https://patchwork.libcamera.org/comment/3284/","msgid":"<20191227222931.GB5747@pendragon.ideasonboard.com>","date":"2019-12-27T22:29:31","subject":"Re: [libcamera-devel] [PATCH v3 4/6] libcamera: camera_manager:\n\tallow retrieving cameras by devnum","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:58:05PM +0100, Jacopo Mondi wrote:\n> On Mon, Dec 23, 2019 at 01:26:18AM -0600, Paul Elder wrote:\n> > Expose a method to retrieve Cameras by devnum. The map of devnum to\n> \n> Camera instances by device number ?\n> \n> > Camera is gathered from all PipelineHandlers. This method is mainly for\n> \n> ipeline handlers\n> \n> > the V4L2 compatibility layer, in order to match V4L2 devices to\n> > libcamera Cameras.\n> \n> Camera\n> \n> Do not pluralize class names :)\n\nAs noted in my review of 2/6, I agree :-) It's not the end of the world\nin commit messages, but interacts badly with Doxygen in the code, so we\ncan as well extend the good habit to commit messages.\n\n> > Signed-off-by: Paul Elder \n> >\n> > ---\n> > New in v3\n> > ---\n> > include/libcamera/camera_manager.h | 3 +++\n> > src/libcamera/camera_manager.cpp | 24 ++++++++++++++++++++++++\n> > 2 files changed, 27 insertions(+)\n> >\n> > diff --git a/include/libcamera/camera_manager.h b/include/libcamera/camera_manager.h\n> > index 8331898c..6ce63912 100644\n> > --- a/include/libcamera/camera_manager.h\n> > +++ b/include/libcamera/camera_manager.h\n> > @@ -7,6 +7,7 @@\n> > #ifndef __LIBCAMERA_CAMERA_MANAGER_H__\n> > #define __LIBCAMERA_CAMERA_MANAGER_H__\n> >\n> > +#include \n> > #include \n> > #include \n> > #include \n> > @@ -33,6 +34,7 @@ public:\n> >\n> > \tconst std::vector> &cameras() const { return cameras_; }\n> > \tstd::shared_ptr get(const std::string &name);\n> > +\tstd::shared_ptr get(dev_t devnum);\n> >\n> > \tvoid addCamera(std::shared_ptr camera);\n> > \tvoid removeCamera(Camera *camera);\n> > @@ -46,6 +48,7 @@ private:\n> > \tstd::unique_ptr enumerator_;\n> > \tstd::vector> pipes_;\n> > \tstd::vector> cameras_;\n> > +\tstd::map> camerasByDevnum_;\n> \n> Do you need to store these as shared pointers ?\n> I don't think it's a big deal, but CameraManager already has a vector\n> of Camera shared_ptr, adding one reference (for some cameras only, the\n> ones created with a devnum associated) might not be needed. I would\n> store them as weak_ptr and return iter->second.lock() in the get()\n> implementation.\n> \n> The implications are minor, so it's really up to you.\n> \n> > \tstatic const std::string version_;\n> > \tstatic CameraManager *self_;\n> > diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp\n> > index 7c6f72bb..1354df4c 100644\n> > --- a/src/libcamera/camera_manager.cpp\n> > +++ b/src/libcamera/camera_manager.cpp\n> > @@ -121,6 +121,11 @@ int CameraManager::start()\n> > \t\t}\n> > \t}\n> >\n> > +\tfor (std::shared_ptr pipe : pipes_) {\n> > +\t\tconst std::map> devMap = pipe->camerasByDevnum();\n> \n> &devMap\n> \n> > +\t\tcamerasByDevnum_.insert(devMap.begin(), devMap.end());\n> > +\t}\n> > +\n\nAs commented for 2/6, I think we should insert the camera in\ncamerasByDevnum_ when the camera is registered, bypassing the map in\nPipelineHandler. This patch will then become quite small, and I think\nyou should squash it with 2/6.\n\n> > \t/* TODO: register hot-plug callback here */\n> >\n> > \treturn 0;\n> > @@ -180,6 +185,25 @@ std::shared_ptr CameraManager::get(const std::string &name)\n> > \treturn nullptr;\n> > }\n> >\n> > +/**\n> > + * \\brief Get a camera based on devnum\n> \n> \"Retrieve a camera\" ? Even if the other get() impmentation has this\n> very same documentation\n> \n> > + * \\param[in] devnum Device number of camera to get\n> > + *\n> > + * Before calling this function the caller is responsible for ensuring that\n> > + * the camera manager is running.\n\nYou need to expand the documentation here to explain what retrieving a\ncamera by devnum is meant for and what it means, as commented in patch\n2/6.\n\n> > + *\n> > + * \\return Shared pointer to Camera object or nullptr if camera not found\n\nTechnically speaking, it's not a nullptr but an empty shared pointer.\nI'd write \"Shared pointer to Camera object, which is empty if the camera\nis not found\".\n\n> > + */\n> > +std::shared_ptr CameraManager::get(dev_t devnum)\n> > +{\n> > +\tauto iter = camerasByDevnum_.find(devnum);\n> > +\n> > +\tif (iter == camerasByDevnum_.end())\n> > +\t\treturn nullptr;\n> > +\n> > +\treturn iter->second;\n> > +}\n> > +\n> \n> Minors apart:\n> Reviewed-by: Jacopo Mondi \n> \n> > /**\n> > * \\brief Add a camera to the camera manager\n> > * \\param[in] camera The camera to be added","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 13B816046A\n\tfor ;\n\tFri, 27 Dec 2019 23:29:44 +0100 (CET)","from pendragon.ideasonboard.com\n\t(host-149-154-202-187.dynamic.voo.be [149.154.202.187])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 7A1BFDD;\n\tFri, 27 Dec 2019 23:29:43 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1577485783;\n\tbh=kBUnZxAIwcyTPxPsFhKiY4LAAsG8Kt+aFt13al3ZwUo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Nts5w48Bsj/gSdZdTq0+WQ9rrkljuN373uDAt3p1mYATEWctEgt4YY/FRcy2j64Wq\n\tmY4j0V+NFtvg39Rt7efpjT9EDxYJR316E1E3tB40RpKJz1MTTDjO2PN/20eKJ469vb\n\tW6QTjMdRMr60n4IvBPrib/CMXNoTKH+p98KY5ffc=","Date":"Sat, 28 Dec 2019 00:29:31 +0200","From":"Laurent Pinchart ","To":"Paul Elder ","Cc":"Jacopo Mondi , libcamera-devel@lists.libcamera.org","Message-ID":"<20191227222931.GB5747@pendragon.ideasonboard.com>","References":"<20191223072620.13022-1-paul.elder@ideasonboard.com>\n\t<20191223072620.13022-5-paul.elder@ideasonboard.com>\n\t<20191227125805.elgtw2kxev2mw4nr@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20191227125805.elgtw2kxev2mw4nr@uno.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v3 4/6] libcamera: camera_manager:\n\tallow retrieving cameras by devnum","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 22:29:44 -0000"}}]