[{"id":129,"web_url":"https://patchwork.libcamera.org/comment/129/","msgid":"<20181230104612.subcgr2uxrmw7lvq@uno.localdomain>","date":"2018-12-30T10:46:12","subject":"Re: [libcamera-devel] [PATCH v2 10/12] libcamera: camera_manager:\n\tadd CameraManager class","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n a few comments you might want to address before or after pushing\nthis patches\n\nOn Sat, Dec 29, 2018 at 04:28:53AM +0100, Niklas Söderlund wrote:\n> Provide a CameraManager class which will handle listing, instancing,\n> destruction and lifetime management of cameras.\n>\n> Signed-off-by: Niklas Söderlund \n> ---\n> include/libcamera/camera_manager.h | 38 +++++++\n> include/libcamera/libcamera.h | 1 +\n> include/libcamera/meson.build | 1 +\n> src/libcamera/camera_manager.cpp | 166 +++++++++++++++++++++++++++++\n> src/libcamera/meson.build | 1 +\n> 5 files changed, 207 insertions(+)\n> create mode 100644 include/libcamera/camera_manager.h\n> create mode 100644 src/libcamera/camera_manager.cpp\n>\n> diff --git a/include/libcamera/camera_manager.h b/include/libcamera/camera_manager.h\n> new file mode 100644\n> index 0000000000000000..9d032d71c4c0841a\n> --- /dev/null\n> +++ b/include/libcamera/camera_manager.h\n> @@ -0,0 +1,38 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2018, Google Inc.\n> + *\n> + * camera_manager.h - Camera management\n> + */\n> +#ifndef __LIBCAMERA_CAMERA_MANAGER_H__\n> +#define __LIBCAMERA_CAMERA_MANAGER_H__\n> +\n> +#include \n> +#include \n> +\n> +namespace libcamera {\n> +\n> +class Camera;\n> +class DeviceEnumerator;\n> +class PipelineHandler;\n> +\n> +class CameraManager\n> +{\n> +public:\n> +\tCameraManager();\n> +\n> +\tint start();\n> +\tvoid stop();\n> +\n> +\tstd::vector list() const;\n> +\tCamera *get(const std::string &name);\n> +\tvoid put(Camera *camera);\n> +\n> +private:\n> +\tDeviceEnumerator *enumerator_;\n> +\tstd::vector pipes_;\n> +};\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_CAMERA_MANAGER_H__ */\n> diff --git a/include/libcamera/libcamera.h b/include/libcamera/libcamera.h\n> index 44c094d92feed5ba..32fb1ff741a7b97a 100644\n> --- a/include/libcamera/libcamera.h\n> +++ b/include/libcamera/libcamera.h\n> @@ -8,6 +8,7 @@\n> #define __LIBCAMERA_LIBCAMERA_H__\n>\n> #include \n> +#include \n>\n> namespace libcamera {\n>\n> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build\n> index 9b266ad926681db9..3e04557d66b1a8f4 100644\n> --- a/include/libcamera/meson.build\n> +++ b/include/libcamera/meson.build\n> @@ -1,5 +1,6 @@\n> libcamera_api = files([\n> 'camera.h',\n> + 'camera_manager.h',\n> 'libcamera.h',\n> ])\n>\n> diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp\n> new file mode 100644\n> index 0000000000000000..1160381b72a08850\n> --- /dev/null\n> +++ b/src/libcamera/camera_manager.cpp\n> @@ -0,0 +1,166 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2018, Google Inc.\n> + *\n> + * camera_manager.h - Camera management\n> + */\n> +\n> +#include \n> +\n> +#include \"device_enumerator.h\"\n> +#include \"pipeline_handler.h\"\n> +\n> +/**\n> + * \\file camera_manager.h\n> + * \\brief Manage all cameras handled by libcamera\n> + *\n> + * The responsibility of the camera manager is to control the lifetime\n> + * management of objects provided by libcamera.\n> + *\n> + * When a user wish to interact with libcamera it creates and starts a\n> + * CameraManager object. Once confirmed the camera manager is running\n> + * the application can list all cameras detected by the library, get\n> + * one or more of the cameras and interact with them.\n> + *\n> + * When the user is done with the camera it should be returned to the\n> + * camera manager. Once all cameras are returned to the camera manager\n> + * the user is free to stop the manager.\n> + *\n> + * \\todo Add ability to add and remove media devices based on\n> + * hot-(un)plug events coming from the device enumerator.\n> + *\n> + * \\todo Add interface to register a notification callback to the user\n> + * to be able to inform it new cameras have been hot-plugged or\n> + * cameras have been removed due to hot-unplug.\n> + */\n> +\n> +namespace libcamera {\n> +\n> +CameraManager::CameraManager()\n> +\t: enumerator_(nullptr)\n> +{\n> +}\n> +\n> +/**\n> + * \\brief Start the camera manager\n> + *\n> + * Start the camera manager and enumerate all devices in the system. Once\n> + * the start have been confirmed the user is free to list and otherwise\n> + * interact with cameras in the system until either the camera manager\n> + * is stopped or the camera is unplugged from the system.\n> + *\n> + * \\return true on successful start false otherwise\n> + */\n> +int CameraManager::start()\n> +{\n> +\tstd::vector handlers;\n> +\n> +\tif (enumerator_)\n> +\t\treturn -ENODEV;\n\nEBUSY?\nDo we want the enumerator be a singleton?\n\n> +\n> +\tenumerator_ = DeviceEnumerator::create();\n> +\n> +\tif (enumerator_->enumerate())\n> +\t\treturn -ENODEV;\n> +\n> +\t/*\n> +\t * TODO: Try to read handlers and order from configuration\n> +\t * file and only fallback on all handlers if there is no\n> +\t * configuration file.\n> +\t */\n> +\tPipelineHandlerFactory::handlers(handlers);\n\nI haven't commented on the PipelineHanderFactory patch, but the\n'handlers()' method, even if static, is a canonical getter. Passing\n'handlers' by reference make it hard to notice this actually a output\nparameters. Might you consider:\n1) passing handlers as pointer to make it clear is a output parameter\n2) have the handlers() method return a vector\n\n> +\n> +\tfor (std::string const &handler : handlers) {\n> +\t\tPipelineHandler *pipe;\n> +\n> +\t\t/*\n> +\t\t * Try each pipeline handler until it exhaust\n> +\t\t * all pipelines it can provide.\n> +\t\t */\n> +\t\tdo {\n> +\t\t\tpipe = PipelineHandlerFactory::create(handler, enumerator_);\n> +\t\t\tif (pipe)\n> +\t\t\t\tpipes_.push_back(pipe);\n> +\t\t} while (pipe);\n> +\t}\n> +\n> +\t/* TODO: register hot-plug callback here */\n> +\n> +\treturn 0;\n> +}\n> +\n> +/**\n> + * \\brief Stop the camera manager\n> + *\n> + * Before stopping the camera manger the caller is responsible for making\n> + * sure all cameras provided by the manager are returned to the manager.\n> + *\n> + * After the manager has been stopped no resource provided by the camera\n> + * manager should be consider valid or functional even if they for one\n> + * reason or another have yet to be deleted.\n> + */\n> +void CameraManager::stop()\n> +{\n> +\t/* TODO: unregister hot-plug callback here */\n> +\n> +\tfor (PipelineHandler *pipe : pipes_)\n> +\t\tdelete pipe;\n> +\n> +\tpipes_.clear();\n> +\n> +\tif (enumerator_)\n> +\t\tdelete enumerator_;\n> +\n> +\tenumerator_ = nullptr;\n> +}\n> +\n> +/**\n> + * \\brief List all detected cameras\n> + *\n> + * Before calling this function the caller is responsible to make sure\n> + * the camera manger is running.\n> + *\n> + * \\return List of names for all detected cameras\n> + */\n> +std::vector CameraManager::list() const\n> +{\n> +\tstd::vector list;\n> +\n> +\tfor (PipelineHandler *pipe : pipes_) {\n> +\t\tfor (unsigned int i = 0; i < pipe->count(); i++) {\n> +\t\t\tCamera *cam = pipe->camera(i);\n> +\t\t\tlist.push_back(cam->name());\n> +\t\t}\n> +\t}\n> +\n> +\treturn list;\n> +}\n> +\n> +/**\n> + * \\brief Get a camera based on name\n> + *\n> + * \\param[in] name Name of camera to get\n> + *\n> + * Before calling this function the caller is responsible to make sure\n\ns/to make sure/of making sure/ ?\nAgain to be upscaled to more English language educated reviewers.\n\n> + * the camera manger is running. A camera fetched this way should be\n> + * release by the user with the put() method of the Camera object once\n\nreleased\n\n> + * it's done using the camera.\n> + *\n> + * \\return Pointer to Camera object or nullptr if camera not found\n> + */\n> +Camera *CameraManager::get(const std::string &name)\n> +{\n> +\tfor (PipelineHandler *pipe : pipes_) {\n> +\t\tfor (unsigned int i = 0; i < pipe->count(); i++) {\n> +\t\t\tCamera *cam = pipe->camera(i);\n> +\t\t\tif (cam->name() == name) {\n> +\t\t\t\tcam->get();\n> +\t\t\t\treturn cam;\n> +\t\t\t}\n> +\t\t}\n> +\t}\n> +\n> +\treturn nullptr;\n> +}\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index 0db648dd3e37156e..a8cb3fdc22784334 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -1,5 +1,6 @@\n> libcamera_sources = files([\n> 'camera.cpp',\n> + 'camera_manager.cpp',\n> 'device_enumerator.cpp',\n> 'log.cpp',\n> 'main.cpp',\n> --\n> 2.20.1\n\nAgain, minor stuff to be addressed later if you want to push this\nseries.\n\nThanks\n j\n\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 relay12.mail.gandi.net (relay12.mail.gandi.net\n\t[217.70.178.232])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6996A60B31\n\tfor ;\n\tSun, 30 Dec 2018 11:46:27 +0100 (CET)","from uno.localdomain (unknown [37.176.180.32])\n\t(Authenticated sender: jacopo@jmondi.org)\n\tby relay12.mail.gandi.net (Postfix) with ESMTPSA id 18666200003;\n\tSun, 30 Dec 2018 10:46:25 +0000 (UTC)"],"Date":"Sun, 30 Dec 2018 11:46:12 +0100","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20181230104612.subcgr2uxrmw7lvq@uno.localdomain>","References":"<20181229032855.26249-1-niklas.soderlund@ragnatech.se>\n\t<20181229032855.26249-11-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"cfnbcfrxr6w6usek\"","Content-Disposition":"inline","In-Reply-To":"<20181229032855.26249-11-niklas.soderlund@ragnatech.se>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH v2 10/12] libcamera: camera_manager:\n\tadd CameraManager class","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":"Sun, 30 Dec 2018 10:46:27 -0000"}},{"id":135,"web_url":"https://patchwork.libcamera.org/comment/135/","msgid":"<20181230203109.GC31866@bigcity.dyn.berto.se>","date":"2018-12-30T20:31:09","subject":"Re: [libcamera-devel] [PATCH v2 10/12] libcamera: camera_manager:\n\tadd CameraManager class","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Jacopo,\n\nThanks for your feedback.\n\nOn 2018-12-30 11:46:12 +0100, Jacopo Mondi wrote:\n> Hi Niklas,\n> a few comments you might want to address before or after pushing\n> this patches\n> \n> On Sat, Dec 29, 2018 at 04:28:53AM +0100, Niklas Söderlund wrote:\n> > Provide a CameraManager class which will handle listing, instancing,\n> > destruction and lifetime management of cameras.\n> >\n> > Signed-off-by: Niklas Söderlund \n> > ---\n> > include/libcamera/camera_manager.h | 38 +++++++\n> > include/libcamera/libcamera.h | 1 +\n> > include/libcamera/meson.build | 1 +\n> > src/libcamera/camera_manager.cpp | 166 +++++++++++++++++++++++++++++\n> > src/libcamera/meson.build | 1 +\n> > 5 files changed, 207 insertions(+)\n> > create mode 100644 include/libcamera/camera_manager.h\n> > create mode 100644 src/libcamera/camera_manager.cpp\n> >\n> > diff --git a/include/libcamera/camera_manager.h b/include/libcamera/camera_manager.h\n> > new file mode 100644\n> > index 0000000000000000..9d032d71c4c0841a\n> > --- /dev/null\n> > +++ b/include/libcamera/camera_manager.h\n> > @@ -0,0 +1,38 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2018, Google Inc.\n> > + *\n> > + * camera_manager.h - Camera management\n> > + */\n> > +#ifndef __LIBCAMERA_CAMERA_MANAGER_H__\n> > +#define __LIBCAMERA_CAMERA_MANAGER_H__\n> > +\n> > +#include \n> > +#include \n> > +\n> > +namespace libcamera {\n> > +\n> > +class Camera;\n> > +class DeviceEnumerator;\n> > +class PipelineHandler;\n> > +\n> > +class CameraManager\n> > +{\n> > +public:\n> > +\tCameraManager();\n> > +\n> > +\tint start();\n> > +\tvoid stop();\n> > +\n> > +\tstd::vector list() const;\n> > +\tCamera *get(const std::string &name);\n> > +\tvoid put(Camera *camera);\n> > +\n> > +private:\n> > +\tDeviceEnumerator *enumerator_;\n> > +\tstd::vector pipes_;\n> > +};\n> > +\n> > +} /* namespace libcamera */\n> > +\n> > +#endif /* __LIBCAMERA_CAMERA_MANAGER_H__ */\n> > diff --git a/include/libcamera/libcamera.h b/include/libcamera/libcamera.h\n> > index 44c094d92feed5ba..32fb1ff741a7b97a 100644\n> > --- a/include/libcamera/libcamera.h\n> > +++ b/include/libcamera/libcamera.h\n> > @@ -8,6 +8,7 @@\n> > #define __LIBCAMERA_LIBCAMERA_H__\n> >\n> > #include \n> > +#include \n> >\n> > namespace libcamera {\n> >\n> > diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build\n> > index 9b266ad926681db9..3e04557d66b1a8f4 100644\n> > --- a/include/libcamera/meson.build\n> > +++ b/include/libcamera/meson.build\n> > @@ -1,5 +1,6 @@\n> > libcamera_api = files([\n> > 'camera.h',\n> > + 'camera_manager.h',\n> > 'libcamera.h',\n> > ])\n> >\n> > diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp\n> > new file mode 100644\n> > index 0000000000000000..1160381b72a08850\n> > --- /dev/null\n> > +++ b/src/libcamera/camera_manager.cpp\n> > @@ -0,0 +1,166 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2018, Google Inc.\n> > + *\n> > + * camera_manager.h - Camera management\n> > + */\n> > +\n> > +#include \n> > +\n> > +#include \"device_enumerator.h\"\n> > +#include \"pipeline_handler.h\"\n> > +\n> > +/**\n> > + * \\file camera_manager.h\n> > + * \\brief Manage all cameras handled by libcamera\n> > + *\n> > + * The responsibility of the camera manager is to control the lifetime\n> > + * management of objects provided by libcamera.\n> > + *\n> > + * When a user wish to interact with libcamera it creates and starts a\n> > + * CameraManager object. Once confirmed the camera manager is running\n> > + * the application can list all cameras detected by the library, get\n> > + * one or more of the cameras and interact with them.\n> > + *\n> > + * When the user is done with the camera it should be returned to the\n> > + * camera manager. Once all cameras are returned to the camera manager\n> > + * the user is free to stop the manager.\n> > + *\n> > + * \\todo Add ability to add and remove media devices based on\n> > + * hot-(un)plug events coming from the device enumerator.\n> > + *\n> > + * \\todo Add interface to register a notification callback to the user\n> > + * to be able to inform it new cameras have been hot-plugged or\n> > + * cameras have been removed due to hot-unplug.\n> > + */\n> > +\n> > +namespace libcamera {\n> > +\n> > +CameraManager::CameraManager()\n> > +\t: enumerator_(nullptr)\n> > +{\n> > +}\n> > +\n> > +/**\n> > + * \\brief Start the camera manager\n> > + *\n> > + * Start the camera manager and enumerate all devices in the system. Once\n> > + * the start have been confirmed the user is free to list and otherwise\n> > + * interact with cameras in the system until either the camera manager\n> > + * is stopped or the camera is unplugged from the system.\n> > + *\n> > + * \\return true on successful start false otherwise\n> > + */\n> > +int CameraManager::start()\n> > +{\n> > +\tstd::vector handlers;\n> > +\n> > +\tif (enumerator_)\n> > +\t\treturn -ENODEV;\n> \n> EBUSY?\n> Do we want the enumerator be a singleton?\n\nYes the CameraManager should be a singleton.\n\n> \n> > +\n> > +\tenumerator_ = DeviceEnumerator::create();\n> > +\n> > +\tif (enumerator_->enumerate())\n> > +\t\treturn -ENODEV;\n> > +\n> > +\t/*\n> > +\t * TODO: Try to read handlers and order from configuration\n> > +\t * file and only fallback on all handlers if there is no\n> > +\t * configuration file.\n> > +\t */\n> > +\tPipelineHandlerFactory::handlers(handlers);\n> \n> I haven't commented on the PipelineHanderFactory patch, but the\n> 'handlers()' method, even if static, is a canonical getter. Passing\n> 'handlers' by reference make it hard to notice this actually a output\n> parameters. Might you consider:\n\nIt is listed as a out parameter in the documentation.\n\n> 1) passing handlers as pointer to make it clear is a output parameter\n\nWould this not be just as unclear as a reference? By using a pointer \nhere all we gain is that we need to add a nullptr check in \nPipelineHandlerFactory::handlers.\n\n> 2) have the handlers() method return a vector\n\nSure it could be done but that would need to be a return by copy. What \ndo rest of you think? I will leave it as is for now.\n\n> \n> > +\n> > +\tfor (std::string const &handler : handlers) {\n> > +\t\tPipelineHandler *pipe;\n> > +\n> > +\t\t/*\n> > +\t\t * Try each pipeline handler until it exhaust\n> > +\t\t * all pipelines it can provide.\n> > +\t\t */\n> > +\t\tdo {\n> > +\t\t\tpipe = PipelineHandlerFactory::create(handler, enumerator_);\n> > +\t\t\tif (pipe)\n> > +\t\t\t\tpipes_.push_back(pipe);\n> > +\t\t} while (pipe);\n> > +\t}\n> > +\n> > +\t/* TODO: register hot-plug callback here */\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +/**\n> > + * \\brief Stop the camera manager\n> > + *\n> > + * Before stopping the camera manger the caller is responsible for making\n> > + * sure all cameras provided by the manager are returned to the manager.\n> > + *\n> > + * After the manager has been stopped no resource provided by the camera\n> > + * manager should be consider valid or functional even if they for one\n> > + * reason or another have yet to be deleted.\n> > + */\n> > +void CameraManager::stop()\n> > +{\n> > +\t/* TODO: unregister hot-plug callback here */\n> > +\n> > +\tfor (PipelineHandler *pipe : pipes_)\n> > +\t\tdelete pipe;\n> > +\n> > +\tpipes_.clear();\n> > +\n> > +\tif (enumerator_)\n> > +\t\tdelete enumerator_;\n> > +\n> > +\tenumerator_ = nullptr;\n> > +}\n> > +\n> > +/**\n> > + * \\brief List all detected cameras\n> > + *\n> > + * Before calling this function the caller is responsible to make sure\n> > + * the camera manger is running.\n> > + *\n> > + * \\return List of names for all detected cameras\n> > + */\n> > +std::vector CameraManager::list() const\n> > +{\n> > +\tstd::vector list;\n> > +\n> > +\tfor (PipelineHandler *pipe : pipes_) {\n> > +\t\tfor (unsigned int i = 0; i < pipe->count(); i++) {\n> > +\t\t\tCamera *cam = pipe->camera(i);\n> > +\t\t\tlist.push_back(cam->name());\n> > +\t\t}\n> > +\t}\n> > +\n> > +\treturn list;\n> > +}\n> > +\n> > +/**\n> > + * \\brief Get a camera based on name\n> > + *\n> > + * \\param[in] name Name of camera to get\n> > + *\n> > + * Before calling this function the caller is responsible to make sure\n> \n> s/to make sure/of making sure/ ?\n> Again to be upscaled to more English language educated reviewers.\n> \n> > + * the camera manger is running. A camera fetched this way should be\n> > + * release by the user with the put() method of the Camera object once\n> \n> released\n> \n\nThanks.\n\n> > + * it's done using the camera.\n> > + *\n> > + * \\return Pointer to Camera object or nullptr if camera not found\n> > + */\n> > +Camera *CameraManager::get(const std::string &name)\n> > +{\n> > +\tfor (PipelineHandler *pipe : pipes_) {\n> > +\t\tfor (unsigned int i = 0; i < pipe->count(); i++) {\n> > +\t\t\tCamera *cam = pipe->camera(i);\n> > +\t\t\tif (cam->name() == name) {\n> > +\t\t\t\tcam->get();\n> > +\t\t\t\treturn cam;\n> > +\t\t\t}\n> > +\t\t}\n> > +\t}\n> > +\n> > +\treturn nullptr;\n> > +}\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> > index 0db648dd3e37156e..a8cb3fdc22784334 100644\n> > --- a/src/libcamera/meson.build\n> > +++ b/src/libcamera/meson.build\n> > @@ -1,5 +1,6 @@\n> > libcamera_sources = files([\n> > 'camera.cpp',\n> > + 'camera_manager.cpp',\n> > 'device_enumerator.cpp',\n> > 'log.cpp',\n> > 'main.cpp',\n> > --\n> > 2.20.1\n> \n> Again, minor stuff to be addressed later if you want to push this\n> series.\n\nWith the above addressed can i add your review tag?\n\n> \n> Thanks\n> j\n> \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 mail-lj1-x242.google.com (mail-lj1-x242.google.com\n\t[IPv6:2a00:1450:4864:20::242])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 569F2600CC\n\tfor ;\n\tSun, 30 Dec 2018 21:31:11 +0100 (CET)","by mail-lj1-x242.google.com with SMTP id k19-v6so22487454lji.11\n\tfor ;\n\tSun, 30 Dec 2018 12:31:11 -0800 (PST)","from localhost (89-233-230-99.cust.bredband2.com. [89.233.230.99])\n\tby smtp.gmail.com with ESMTPSA id\n\tk21-v6sm9676396ljc.15.2018.12.30.12.31.09\n\t(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);\n\tSun, 30 Dec 2018 12:31:09 -0800 (PST)"],"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=8HdROS+SZp8GpYaHIQpj8RkclTmzhZvFKPR1oXMnOds=;\n\tb=sPWKAgmNsvO7n31BXX8tAYKXgON57r9wHNoEhuu8UlhAsUR4ohYaXcuUBe8bK9i6s4\n\t7R8uMpcVAISKlTE92l5BTOzYAI0WQz2ZOrhsb8ZHAMWYQWGuKJjt3aQBvgVlqPvEFkRi\n\txXFjIsMOvi/Oz6ZdREMJ7Qz5DMYZ0D8xum5gyA8AfJMMzIDoNX9FJlJOozuZiwPLJ/GN\n\tzEN3JgQ0xvmmCM4qkfz8nXxCO5c7lX9vQaOQE45/jKetsTK4bEL8ezImDE2wZkCVU4fk\n\tXf5okyS/SyP2P3cuuSQijJHpM72sWaHUAhrPuzgOaA8Y5oKAw4HSCqfFIVWKQibzk4wL\n\t6MjA==","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=8HdROS+SZp8GpYaHIQpj8RkclTmzhZvFKPR1oXMnOds=;\n\tb=KbIDfDj/u9x/zMcRJLxpcnjf3tfcdWKEHd7v4FR027j4sFQ0Ww4WcZE44eRNqd71Sa\n\tVnGoG+WV+3Gk9omJiUtFtfkzkRRhxmRy2kMssh2zI2yGZ7SjZkEzywSPNWFNGbIHt5Qp\n\tFAdQKN+SkIEdiYFi8pxtqaB2zhxhrXa5rizuLiMmzofTDr8Et2xiiFg9b4s05OjVi72A\n\tixIyvAWqwgIxl/47pCevmQzSArxHpg+sNO+QPUWM+ySIxSclQVcEajkLg+9mI3DHehVE\n\tc4JOW3tRTOJ80/6HquXrC/fUhQGyvd6x1jN+ekqhd6gnoxPT00McyaRR/QHeILG+1OaF\n\tw1pg==","X-Gm-Message-State":"AJcUukc5MRGbgkTE/+wxkcC6Zyz9nYgoOjxb99DE0wS6exRjFShM1uJ0\n\tmGtEV+6osuVwVTmICn55tQbH8w==","X-Google-Smtp-Source":"ALg8bN7T4Pfwuh87XF4XlDyS+5DHGh9crABxmQ8Y/nlJbkuLptNDlmE0NIoxJgce/CHfjBr/5/B7yQ==","X-Received":"by 2002:a2e:4942:: with SMTP id\n\tb2-v6mr19677386ljd.168.1546201870527; \n\tSun, 30 Dec 2018 12:31:10 -0800 (PST)","Date":"Sun, 30 Dec 2018 21:31:09 +0100","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20181230203109.GC31866@bigcity.dyn.berto.se>","References":"<20181229032855.26249-1-niklas.soderlund@ragnatech.se>\n\t<20181229032855.26249-11-niklas.soderlund@ragnatech.se>\n\t<20181230104612.subcgr2uxrmw7lvq@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20181230104612.subcgr2uxrmw7lvq@uno.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v2 10/12] libcamera: camera_manager:\n\tadd CameraManager class","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":"Sun, 30 Dec 2018 20:31:11 -0000"}},{"id":140,"web_url":"https://patchwork.libcamera.org/comment/140/","msgid":"<20181230211715.tlcttct4lhmbpzo6@uno.localdomain>","date":"2018-12-30T21:17:15","subject":"Re: [libcamera-devel] [PATCH v2 10/12] libcamera: camera_manager:\n\tadd CameraManager class","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n\nOn Sun, Dec 30, 2018 at 09:31:09PM +0100, Niklas Söderlund wrote:\n> Hi Jacopo,\n>\n> Thanks for your feedback.\n>\n\n[snip]\n\n> On 2018-12-30 11:46:12 +0100, Jacopo Mondi wrote:\n> > > +\t/*\n> > > +\t * TODO: Try to read handlers and order from configuration\n> > > +\t * file and only fallback on all handlers if there is no\n> > > +\t * configuration file.\n> > > +\t */\n> > > +\tPipelineHandlerFactory::handlers(handlers);\n> >\n> > I haven't commented on the PipelineHanderFactory patch, but the\n> > 'handlers()' method, even if static, is a canonical getter. Passing\n> > 'handlers' by reference make it hard to notice this actually a output\n> > parameters. Might you consider:\n>\n> It is listed as a out parameter in the documentation.\n>\n> > 1) passing handlers as pointer to make it clear is a output parameter\n>\n> Would this not be just as unclear as a reference? By using a pointer\n> here all we gain is that we need to add a nullptr check in\n> PipelineHandlerFactory::handlers.\n>\n\nI tend to agree with what is written here:\nhttps://google.github.io/styleguide/cppguide.html#Output_Parameters\n\nReferences are good as const input arguments, but for output\nparameters it's clearer to keep them pointers, to express the\nintent to modify their content. To me a function that returns void\nand a (&handler) parameter clearly states it is going to modify it.\nThe first time I read handlers(handlers) I had to go back and look at\ndocumentation.\n\n> > 2) have the handlers() method return a vector\n>\n> Sure it could be done but that would need to be a return by copy. What\n> do rest of you think? I will leave it as is for now.\n>\n> >\n\nThat would read even more natural to me:\n handlers = PipelineHandlerFactory::handlers();\n\nBut to avoid copying the vector back I would go for the argument\npassed by pointer option.\n\nOr, could the PipelineHandlerFactory store a static vector, and return\na reference to it? I also feel is more 'natural' to return references\nto statically allocated variables, such as member data, and return\npointers to instances/variables dynamically allocated. I have not\nfound that coded in any style guide, so that might just be my\npreference only.\n> > > +\n\n[snip]\n\n> >\n> > Again, minor stuff to be addressed later if you want to push this\n> > series.\n>\n> With the above addressed can i add your review tag?\n>\n\nFor this and other patches where I forgot to do so:\nReviewed-by: Jacopo Mondi \n\nThanks\n j","headers":{"Return-Path":"","Received":["from relay3-d.mail.gandi.net (relay3-d.mail.gandi.net\n\t[217.70.183.195])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E5227600CC\n\tfor ;\n\tSun, 30 Dec 2018 22:17:18 +0100 (CET)","from uno.localdomain\n\t(host54-51-dynamic.16-87-r.retail.telecomitalia.it [87.16.51.54])\n\t(Authenticated sender: jacopo@jmondi.org)\n\tby relay3-d.mail.gandi.net (Postfix) with ESMTPSA id 470C460003;\n\tSun, 30 Dec 2018 21:17:18 +0000 (UTC)"],"X-Originating-IP":"87.16.51.54","Date":"Sun, 30 Dec 2018 22:17:15 +0100","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20181230211715.tlcttct4lhmbpzo6@uno.localdomain>","References":"<20181229032855.26249-1-niklas.soderlund@ragnatech.se>\n\t<20181229032855.26249-11-niklas.soderlund@ragnatech.se>\n\t<20181230104612.subcgr2uxrmw7lvq@uno.localdomain>\n\t<20181230203109.GC31866@bigcity.dyn.berto.se>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"bmmy5dyeec2hyfv6\"","Content-Disposition":"inline","In-Reply-To":"<20181230203109.GC31866@bigcity.dyn.berto.se>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH v2 10/12] libcamera: camera_manager:\n\tadd CameraManager class","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":"Sun, 30 Dec 2018 21:17:19 -0000"}},{"id":143,"web_url":"https://patchwork.libcamera.org/comment/143/","msgid":"<1649090.KfEFg2TjUb@avalon>","date":"2018-12-30T22:39:12","subject":"Re: [libcamera-devel] [PATCH v2 10/12] libcamera: camera_manager:\n\tadd CameraManager class","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello,\n\nOn Sunday, 30 December 2018 23:17:15 EET Jacopo Mondi wrote:\n> On Sun, Dec 30, 2018 at 09:31:09PM +0100, Niklas Söderlund wrote:\n> > Hi Jacopo,\n> > \n> > Thanks for your feedback.\n> \n> [snip]\n> \n> > On 2018-12-30 11:46:12 +0100, Jacopo Mondi wrote:\n> > > > +\t/*\n> > > > +\t * TODO: Try to read handlers and order from configuration\n> > > > +\t * file and only fallback on all handlers if there is no\n> > > > +\t * configuration file.\n> > > > +\t */\n> > > > +\tPipelineHandlerFactory::handlers(handlers);\n> > > \n> > > I haven't commented on the PipelineHanderFactory patch, but the\n> > > 'handlers()' method, even if static, is a canonical getter. Passing\n> > > 'handlers' by reference make it hard to notice this actually a output\n> > > parameters. Might you consider:\n> > \n> > It is listed as a out parameter in the documentation.\n> > \n> > > 1) passing handlers as pointer to make it clear is a output parameter\n> > \n> > Would this not be just as unclear as a reference? By using a pointer\n> > here all we gain is that we need to add a nullptr check in\n> > PipelineHandlerFactory::handlers.\n> \n> I tend to agree with what is written here:\n> https://google.github.io/styleguide/cppguide.html#Output_Parameters\n> \n> References are good as const input arguments, but for output\n> parameters it's clearer to keep them pointers, to express the\n> intent to modify their content. To me a function that returns void\n> and a (&handler) parameter clearly states it is going to modify it.\n> The first time I read handlers(handlers) I had to go back and look at\n> documentation.\n> \n> > > 2) have the handlers() method return a vector\n> > \n> > Sure it could be done but that would need to be a return by copy. What\n> > do rest of you think? I will leave it as is for now.\n> \n> That would read even more natural to me:\n> handlers = PipelineHandlerFactory::handlers();\n> \n> But to avoid copying the vector back I would go for the argument\n> passed by pointer option.\n\n\tstd::vector handlers = PipelineHandlerFactory::handlers();\n\nshould be optimized by the compiler to avoid copies. The std::vector<> \ndeclared on the stack inside the handlers() function should be replaced with \nthe one from the caller transparently.\n\nhttps://en.wikipedia.org/wiki/Copy_elision#Return_value_optimization\n\n> Or, could the PipelineHandlerFactory store a static vector, and return\n> a reference to it? I also feel is more 'natural' to return references\n> to statically allocated variables, such as member data, and return\n> pointers to instances/variables dynamically allocated. I have not\n> found that coded in any style guide, so that might just be my\n> preference only.\n> \n> > > > +\n> \n> [snip]\n> \n> > > Again, minor stuff to be addressed later if you want to push this\n> > > series.\n> > \n> > With the above addressed can i add your review tag?\n> \n> For this and other patches where I forgot to do so:\n> Reviewed-by: Jacopo Mondi ","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 C5E5D600CC\n\tfor ;\n\tSun, 30 Dec 2018 23:38:15 +0100 (CET)","from avalon.localnet (unknown\n\t[IPv6:2a02:a03f:3ad5:900:7d1d:858b:75d5:534d])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 45BE750A;\n\tSun, 30 Dec 2018 23:38:15 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1546209495;\n\tbh=3Cd1FAp6AiUeg1UHbB7ghs6QbZNl71UptmxFkfcm3nw=;\n\th=From:To:Cc:Subject:Date:In-Reply-To:References:From;\n\tb=IMI/fb8Tff5MiYid4ks7E+mQjjcFwm+fMZ3Zwvc3TkHdhiq/q2XhQ7186f0DA4PnL\n\tpiUj3O38EsZS99i4SpIadwbUId53HkgK7HVRaFW1s1OzFYOhtG6Dh0KjJPqQ+UjLPN\n\tmbmVyvYXcVuCmxeyfJFw7158tHhcaYbELk0id0vI=","From":"Laurent Pinchart ","To":"libcamera-devel@lists.libcamera.org","Date":"Mon, 31 Dec 2018 00:39:12 +0200","Message-ID":"<1649090.KfEFg2TjUb@avalon>","Organization":"Ideas on Board Oy","In-Reply-To":"<20181230211715.tlcttct4lhmbpzo6@uno.localdomain>","References":"<20181229032855.26249-1-niklas.soderlund@ragnatech.se>\n\t<20181230203109.GC31866@bigcity.dyn.berto.se>\n\t<20181230211715.tlcttct4lhmbpzo6@uno.localdomain>","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","Content-Type":"text/plain; charset=\"iso-8859-1\"","Subject":"Re: [libcamera-devel] [PATCH v2 10/12] libcamera: camera_manager:\n\tadd CameraManager class","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":"Sun, 30 Dec 2018 22:38:15 -0000"}},{"id":147,"web_url":"https://patchwork.libcamera.org/comment/147/","msgid":"<9669754.P9YqOJaAqb@avalon>","date":"2018-12-30T23:04:08","subject":"Re: [libcamera-devel] [PATCH v2 10/12] libcamera: camera_manager:\n\tadd CameraManager class","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 Saturday, 29 December 2018 05:28:53 EET Niklas Söderlund wrote:\n> Provide a CameraManager class which will handle listing, instancing,\n> destruction and lifetime management of cameras.\n> \n> Signed-off-by: Niklas Söderlund \n> ---\n> include/libcamera/camera_manager.h | 38 +++++++\n> include/libcamera/libcamera.h | 1 +\n> include/libcamera/meson.build | 1 +\n> src/libcamera/camera_manager.cpp | 166 +++++++++++++++++++++++++++++\n> src/libcamera/meson.build | 1 +\n> 5 files changed, 207 insertions(+)\n> create mode 100644 include/libcamera/camera_manager.h\n> create mode 100644 src/libcamera/camera_manager.cpp\n> \n> diff --git a/include/libcamera/camera_manager.h\n> b/include/libcamera/camera_manager.h new file mode 100644\n> index 0000000000000000..9d032d71c4c0841a\n> --- /dev/null\n> +++ b/include/libcamera/camera_manager.h\n> @@ -0,0 +1,38 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2018, Google Inc.\n> + *\n> + * camera_manager.h - Camera management\n> + */\n> +#ifndef __LIBCAMERA_CAMERA_MANAGER_H__\n> +#define __LIBCAMERA_CAMERA_MANAGER_H__\n> +\n> +#include \n> +#include \n> +\n> +namespace libcamera {\n> +\n> +class Camera;\n> +class DeviceEnumerator;\n> +class PipelineHandler;\n> +\n> +class CameraManager\n> +{\n> +public:\n> +\tCameraManager();\n> +\n> +\tint start();\n> +\tvoid stop();\n> +\n> +\tstd::vector list() const;\n> +\tCamera *get(const std::string &name);\n> +\tvoid put(Camera *camera);\n> +\n> +private:\n> +\tDeviceEnumerator *enumerator_;\n> +\tstd::vector pipes_;\n> +};\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_CAMERA_MANAGER_H__ */\n> diff --git a/include/libcamera/libcamera.h b/include/libcamera/libcamera.h\n> index 44c094d92feed5ba..32fb1ff741a7b97a 100644\n> --- a/include/libcamera/libcamera.h\n> +++ b/include/libcamera/libcamera.h\n> @@ -8,6 +8,7 @@\n> #define __LIBCAMERA_LIBCAMERA_H__\n> \n> #include \n> +#include \n> \n> namespace libcamera {\n> \n> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build\n> index 9b266ad926681db9..3e04557d66b1a8f4 100644\n> --- a/include/libcamera/meson.build\n> +++ b/include/libcamera/meson.build\n> @@ -1,5 +1,6 @@\n> libcamera_api = files([\n> 'camera.h',\n> + 'camera_manager.h',\n> 'libcamera.h',\n> ])\n> \n> diff --git a/src/libcamera/camera_manager.cpp\n> b/src/libcamera/camera_manager.cpp new file mode 100644\n> index 0000000000000000..1160381b72a08850\n> --- /dev/null\n> +++ b/src/libcamera/camera_manager.cpp\n> @@ -0,0 +1,166 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2018, Google Inc.\n> + *\n> + * camera_manager.h - Camera management\n> + */\n> +\n> +#include \n> +\n> +#include \"device_enumerator.h\"\n> +#include \"pipeline_handler.h\"\n> +\n> +/**\n> + * \\file camera_manager.h\n> + * \\brief Manage all cameras handled by libcamera\n> + *\n> + * The responsibility of the camera manager is to control the lifetime\n> + * management of objects provided by libcamera.\n> + *\n> + * When a user wish to interact with libcamera it creates and starts a\n> + * CameraManager object. Once confirmed the camera manager is running\n> + * the application can list all cameras detected by the library, get\n> + * one or more of the cameras and interact with them.\n> + *\n> + * When the user is done with the camera it should be returned to the\n> + * camera manager. Once all cameras are returned to the camera manager\n> + * the user is free to stop the manager.\n> + *\n> + * \\todo Add ability to add and remove media devices based on\n> + * hot-(un)plug events coming from the device enumerator.\n> + *\n> + * \\todo Add interface to register a notification callback to the user\n> + * to be able to inform it new cameras have been hot-plugged or\n> + * cameras have been removed due to hot-unplug.\n> + */\n> +\n> +namespace libcamera {\n> +\n> +CameraManager::CameraManager()\n> +\t: enumerator_(nullptr)\n> +{\n> +}\n> +\n> +/**\n> + * \\brief Start the camera manager\n> + *\n> + * Start the camera manager and enumerate all devices in the system. Once\n> + * the start have been confirmed the user is free to list and otherwise\n\ns/have/has/\n\n> + * interact with cameras in the system until either the camera manager\n> + * is stopped or the camera is unplugged from the system.\n> + *\n> + * \\return true on successful start false otherwise\n> + */\n> +int CameraManager::start()\n> +{\n> +\tstd::vector handlers;\n> +\n> +\tif (enumerator_)\n> +\t\treturn -ENODEV;\n> +\n> +\tenumerator_ = DeviceEnumerator::create();\n> +\n\nI'd remove the blank line.\n\n> +\tif (enumerator_->enumerate())\n> +\t\treturn -ENODEV;\n> +\n> +\t/*\n> +\t * TODO: Try to read handlers and order from configuration\n> +\t * file and only fallback on all handlers if there is no\n> +\t * configuration file.\n> +\t */\n> +\tPipelineHandlerFactory::handlers(handlers);\n> +\n> +\tfor (std::string const &handler : handlers) {\n> +\t\tPipelineHandler *pipe;\n> +\n> +\t\t/*\n> +\t\t * Try each pipeline handler until it exhaust\n> +\t\t * all pipelines it can provide.\n> +\t\t */\n> +\t\tdo {\n> +\t\t\tpipe = PipelineHandlerFactory::create(handler, enumerator_);\n> +\t\t\tif (pipe)\n> +\t\t\t\tpipes_.push_back(pipe);\n> +\t\t} while (pipe);\n> +\t}\n> +\n> +\t/* TODO: register hot-plug callback here */\n> +\n> +\treturn 0;\n> +}\n> +\n> +/**\n> + * \\brief Stop the camera manager\n> + *\n> + * Before stopping the camera manger the caller is responsible for making\n> + * sure all cameras provided by the manager are returned to the manager.\n> + *\n> + * After the manager has been stopped no resource provided by the camera\n> + * manager should be consider valid or functional even if they for one\n> + * reason or another have yet to be deleted.\n> + */\n> +void CameraManager::stop()\n> +{\n> +\t/* TODO: unregister hot-plug callback here */\n> +\n> +\tfor (PipelineHandler *pipe : pipes_)\n> +\t\tdelete pipe;\n> +\n> +\tpipes_.clear();\n> +\n> +\tif (enumerator_)\n> +\t\tdelete enumerator_;\n> +\n> +\tenumerator_ = nullptr;\n> +}\n> +\n> +/**\n> + * \\brief List all detected cameras\n> + *\n> + * Before calling this function the caller is responsible to make sure\n> + * the camera manger is running.\n> + *\n> + * \\return List of names for all detected cameras\n> + */\n> +std::vector CameraManager::list() const\n> +{\n> +\tstd::vector list;\n> +\n> +\tfor (PipelineHandler *pipe : pipes_) {\n> +\t\tfor (unsigned int i = 0; i < pipe->count(); i++) {\n> +\t\t\tCamera *cam = pipe->camera(i);\n\nI still think accessing cameras from the pipeline by index isn't a good API, \nbut it can be changed later.\n\n> +\t\t\tlist.push_back(cam->name());\n> +\t\t}\n> +\t}\n> +\n> +\treturn list;\n> +}\n> +\n> +/**\n> + * \\brief Get a camera based on name\n> + *\n> + * \\param[in] name Name of camera to get\n> + *\n> + * Before calling this function the caller is responsible to make sure\n\ns/to make sure/for ensuring that/\n\n> + * the camera manger is running. A camera fetched this way should be\n\ns/should/shall/\n\n> + * release by the user with the put() method of the Camera object once\n> + * it's done using the camera.\n> + *\n> + * \\return Pointer to Camera object or nullptr if camera not found\n> + */\n> +Camera *CameraManager::get(const std::string &name)\n> +{\n> +\tfor (PipelineHandler *pipe : pipes_) {\n> +\t\tfor (unsigned int i = 0; i < pipe->count(); i++) {\n> +\t\t\tCamera *cam = pipe->camera(i);\n> +\t\t\tif (cam->name() == name) {\n> +\t\t\t\tcam->get();\n> +\t\t\t\treturn cam;\n> +\t\t\t}\n> +\t\t}\n> +\t}\n> +\n> +\treturn nullptr;\n> +}\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index 0db648dd3e37156e..a8cb3fdc22784334 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -1,5 +1,6 @@\n> libcamera_sources = files([\n> 'camera.cpp',\n> + 'camera_manager.cpp',\n> 'device_enumerator.cpp',\n> 'log.cpp',\n> 'main.cpp',\n\nReviewed-by: Laurent Pinchart ","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 74B88600CC\n\tfor ;\n\tMon, 31 Dec 2018 00:03:11 +0100 (CET)","from avalon.localnet (unknown\n\t[IPv6:2a02:a03f:3ad5:900:7d1d:858b:75d5:534d])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E95EB50A;\n\tMon, 31 Dec 2018 00:03:10 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1546210991;\n\tbh=1drdzArkTsWZmggjr1O+C8Hh4jTQTk/xsLp7Bk6k+Bc=;\n\th=From:To:Cc:Subject:Date:In-Reply-To:References:From;\n\tb=rxCKuAQuDpPP4jleXkUc742wT+IJmPkWl1IKBdLEErNEFq/G+FOD/G6Ts64ALDBX4\n\tR+JbEozATxp5SbL3qG1X2fF/o3CC1NwECHXgfCQPUNCwrgmaMAkOuBVFMYZsUcmKNy\n\t2qTLhAulRuxDj0Ju0UStM+AJAq4vFtEBJHFkwx4g=","From":"Laurent Pinchart ","To":"libcamera-devel@lists.libcamera.org","Date":"Mon, 31 Dec 2018 01:04:08 +0200","Message-ID":"<9669754.P9YqOJaAqb@avalon>","Organization":"Ideas on Board Oy","In-Reply-To":"<20181229032855.26249-11-niklas.soderlund@ragnatech.se>","References":"<20181229032855.26249-1-niklas.soderlund@ragnatech.se>\n\t<20181229032855.26249-11-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","Content-Type":"text/plain; charset=\"iso-8859-1\"","Subject":"Re: [libcamera-devel] [PATCH v2 10/12] libcamera: camera_manager:\n\tadd CameraManager class","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":"Sun, 30 Dec 2018 23:03:11 -0000"}},{"id":148,"web_url":"https://patchwork.libcamera.org/comment/148/","msgid":"<20181230231235.GC10929@bigcity.dyn.berto.se>","date":"2018-12-30T23:12:35","subject":"Re: [libcamera-devel] [PATCH v2 10/12] libcamera: camera_manager:\n\tadd CameraManager class","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Jacopo, Laurent,\n\nOn 2018-12-31 00:39:12 +0200, Laurent Pinchart wrote:\n> Hello,\n> \n> On Sunday, 30 December 2018 23:17:15 EET Jacopo Mondi wrote:\n> > On Sun, Dec 30, 2018 at 09:31:09PM +0100, Niklas Söderlund wrote:\n> > > Hi Jacopo,\n> > > \n> > > Thanks for your feedback.\n> > \n> > [snip]\n> > \n> > > On 2018-12-30 11:46:12 +0100, Jacopo Mondi wrote:\n> > > > > +\t/*\n> > > > > +\t * TODO: Try to read handlers and order from configuration\n> > > > > +\t * file and only fallback on all handlers if there is no\n> > > > > +\t * configuration file.\n> > > > > +\t */\n> > > > > +\tPipelineHandlerFactory::handlers(handlers);\n> > > > \n> > > > I haven't commented on the PipelineHanderFactory patch, but the\n> > > > 'handlers()' method, even if static, is a canonical getter. Passing\n> > > > 'handlers' by reference make it hard to notice this actually a output\n> > > > parameters. Might you consider:\n> > > \n> > > It is listed as a out parameter in the documentation.\n> > > \n> > > > 1) passing handlers as pointer to make it clear is a output parameter\n> > > \n> > > Would this not be just as unclear as a reference? By using a pointer\n> > > here all we gain is that we need to add a nullptr check in\n> > > PipelineHandlerFactory::handlers.\n> > \n> > I tend to agree with what is written here:\n> > https://google.github.io/styleguide/cppguide.html#Output_Parameters\n> > \n> > References are good as const input arguments, but for output\n> > parameters it's clearer to keep them pointers, to express the\n> > intent to modify their content. To me a function that returns void\n> > and a (&handler) parameter clearly states it is going to modify it.\n> > The first time I read handlers(handlers) I had to go back and look at\n> > documentation.\n> > \n> > > > 2) have the handlers() method return a vector\n> > > \n> > > Sure it could be done but that would need to be a return by copy. What\n> > > do rest of you think? I will leave it as is for now.\n> > \n> > That would read even more natural to me:\n> > handlers = PipelineHandlerFactory::handlers();\n> > \n> > But to avoid copying the vector back I would go for the argument\n> > passed by pointer option.\n> \n> \tstd::vector handlers = PipelineHandlerFactory::handlers();\n> \n> should be optimized by the compiler to avoid copies. The std::vector<> \n> declared on the stack inside the handlers() function should be replaced with \n> the one from the caller transparently.\n> \n> https://en.wikipedia.org/wiki/Copy_elision#Return_value_optimization\n\nI yield :-)\n\n> \n> > Or, could the PipelineHandlerFactory store a static vector, and return\n> > a reference to it? I also feel is more 'natural' to return references\n> > to statically allocated variables, such as member data, and return\n> > pointers to instances/variables dynamically allocated. I have not\n> > found that coded in any style guide, so that might just be my\n> > preference only.\n> > \n> > > > > +\n> > \n> > [snip]\n> > \n> > > > Again, minor stuff to be addressed later if you want to push this\n> > > > series.\n> > > \n> > > With the above addressed can i add your review tag?\n> > \n> > For this and other patches where I forgot to do so:\n> > Reviewed-by: Jacopo Mondi \n\nThanks Jacopo.","headers":{"Return-Path":"","Received":["from mail-lf1-x141.google.com (mail-lf1-x141.google.com\n\t[IPv6:2a00:1450:4864:20::141])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DBDD3600CC\n\tfor ;\n\tMon, 31 Dec 2018 00:12:37 +0100 (CET)","by mail-lf1-x141.google.com with SMTP id a16so17577170lfg.3\n\tfor ;\n\tSun, 30 Dec 2018 15:12:37 -0800 (PST)","from localhost (89-233-230-99.cust.bredband2.com. [89.233.230.99])\n\tby smtp.gmail.com with ESMTPSA id\n\tr4sm9102237lfe.60.2018.12.30.15.12.36\n\t(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);\n\tSun, 30 Dec 2018 15:12:36 -0800 (PST)"],"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=JFLMDjrM06zPnribqV9ysE095sZ3+/CRkYYyOYoC9FY=;\n\tb=PAkCfBbs9cVO8BXrCff03c+WR44ZbFkqADro0t14NBSsUxrM08CVFpxmlg3zwnGJW9\n\tF6UmuqEQ6DaieNtJXVf8vzBGKp6Tz5XxglwnIuZBkbZmXRkSaO/NpaCNm4pp6SONwdzl\n\tb/IKORanWbBn3MKDmoKs+FEK4kBzqQ68BKzMk6ZNI5FOm5Hh9V9gHVNI+akKDexpLakK\n\t3akLP9zy25ZWh0jWv3+CgsoDG9WvgcuRP9OUrbr0HQZW37GuLMCwe1A8n7wPV+/TMYUo\n\tr8sUjqtaYhPX9a5gDewf8CrsFiptSKz9VLN+O+rCXYKTH4c5ZfHVONI+xlc0tyswEb0y\n\tgDEA==","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=JFLMDjrM06zPnribqV9ysE095sZ3+/CRkYYyOYoC9FY=;\n\tb=hapnw2f/Cw/KrgyzfCzc9qcW+hlBxf3CeipbY9U0z+rPUvRJjQ/UxMSXXXdq5m7rMM\n\tUtlXX0zr/pQ2blRU8SkVSTB+3Wd+I4EHQWgFuMhlRpqcaYJDsgp3VvcNN+MUUCAs+Sbt\n\tDDtogns+ReyOPWaxSpaCBrLT25NTi5BulnkT5AUWe8Osq0gH9rH/L7whUdaVfMA7xNy4\n\tfD/KhnYVje1osMSjw3oGMI4a9IUihD+Hkbymidm6h/naM9FUuG/QuSxkgFT/jX/8J+/f\n\t7JjolGzDYE18BHSvhu72CCLEC4dp+kbHmBvO4y0Y6trCUxIrn5iArVkAs9YFTW6ihxZ0\n\tpc6g==","X-Gm-Message-State":"AA+aEWbnOymiZLi8McaBP7u3uQ/KBv+er8MzKIFH421X/W3cgFSRTDqS\n\t4ZqDdqYU0qPGzrURUQu5KnoaBuGFVEQ=","X-Google-Smtp-Source":"AFSGD/XjqnDngk2bKWeyUro5Sfwl0VHrqfLDuP5/RoOheDfQVNNhSBfA1itoy36GfYfm2ddgUhlO1w==","X-Received":"by 2002:a19:910d:: with SMTP id\n\tt13mr16587515lfd.98.1546211557137; \n\tSun, 30 Dec 2018 15:12:37 -0800 (PST)","Date":"Mon, 31 Dec 2018 00:12:35 +0100","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org, Jacopo Mondi ","Message-ID":"<20181230231235.GC10929@bigcity.dyn.berto.se>","References":"<20181229032855.26249-1-niklas.soderlund@ragnatech.se>\n\t<20181230203109.GC31866@bigcity.dyn.berto.se>\n\t<20181230211715.tlcttct4lhmbpzo6@uno.localdomain>\n\t<1649090.KfEFg2TjUb@avalon>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<1649090.KfEFg2TjUb@avalon>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v2 10/12] libcamera: camera_manager:\n\tadd CameraManager class","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":"Sun, 30 Dec 2018 23:12:38 -0000"}},{"id":149,"web_url":"https://patchwork.libcamera.org/comment/149/","msgid":"<20181230232200.GD10929@bigcity.dyn.berto.se>","date":"2018-12-30T23:22:00","subject":"Re: [libcamera-devel] [PATCH v2 10/12] libcamera: camera_manager:\n\tadd CameraManager class","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 2018-12-31 01:04:08 +0200, Laurent Pinchart wrote:\n\n[snip]\n\n> > +/**\n> > + * \\brief List all detected cameras\n> > + *\n> > + * Before calling this function the caller is responsible to make sure\n> > + * the camera manger is running.\n> > + *\n> > + * \\return List of names for all detected cameras\n> > + */\n> > +std::vector CameraManager::list() const\n> > +{\n> > +\tstd::vector list;\n> > +\n> > +\tfor (PipelineHandler *pipe : pipes_) {\n> > +\t\tfor (unsigned int i = 0; i < pipe->count(); i++) {\n> > +\t\t\tCamera *cam = pipe->camera(i);\n> \n> I still think accessing cameras from the pipeline by index isn't a good API, \n> but it can be changed later.\n\nI agree. As the interface between a Camera and the MediaDevice and/or \npipeline manager is not yet defined I thought I keep this dead simple \nfor now as to not create a CameraManager requirement that would take a \nlot of time to change.\n\nAs previously discussed I agree that the best solution would probably be \nfor the Camera to register itself with the CameraManager. Lets see how \nit plays out.","headers":{"Return-Path":"","Received":["from mail-lf1-x12d.google.com (mail-lf1-x12d.google.com\n\t[IPv6:2a00:1450:4864:20::12d])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 850E1600CC\n\tfor ;\n\tMon, 31 Dec 2018 00:22:02 +0100 (CET)","by mail-lf1-x12d.google.com with SMTP id z13so17553497lfe.11\n\tfor ;\n\tSun, 30 Dec 2018 15:22:02 -0800 (PST)","from localhost (89-233-230-99.cust.bredband2.com. [89.233.230.99])\n\tby smtp.gmail.com with ESMTPSA id\n\t12-v6sm9912556ljs.29.2018.12.30.15.22.00\n\t(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);\n\tSun, 30 Dec 2018 15:22:00 -0800 (PST)"],"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=fjePaoClcG4F0J5uBqwYx8HpNvnXciKkQQRR70F37Gk=;\n\tb=jhXYlDLMZKcI4xfHWp3QhE1URNt14UyhDML1wlshtW7BasELu4hpbvIM7rSM++1gwM\n\teDaSGPGF0HBA1j19OQleus+XNtcJDPSlqXw5YcGvZm0QpWxGsRreYIArCeDvRE92sAa1\n\tZH0ogoh92XAuvxo2Zc8hlccGq6VthW/BOaFcXV6xdGpX2dm56Ju/x0Qvjmnk18Ob47YI\n\tbWHgV2ULn+YfV1HtoAm7g+ONzFEO1ZyNkZZTb+6s0XEeoiZQwfuleBFyscR7ZDrzsqmb\n\tVeff3loB19JSNhwFoNpiXOkut4KjmAZlb3dp4HmtRQVW2fAHXR4egWufWuttC53vnvfI\n\ti3rQ==","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=fjePaoClcG4F0J5uBqwYx8HpNvnXciKkQQRR70F37Gk=;\n\tb=LUYGgB8afP6Ef9qFODT+d6HErQRV2+KaqOHNBtdw6Jx7dVJQAUS/5TYyS4ug3vxUa0\n\toHXrWrcK+txTuLjjTk+4eZsRZH+KSOwaSSVnkVcMpVirxU0w/Jq23li+6OerlHgfzGw/\n\tTnxHRLq2xmocHL2XJuvtAj8Nb/UMEwWuF3ZiTBOehqEyw6h+cohM0n5aH5b8CFjNsWjN\n\tba8xYvKU8lhiTExVvzxdQFecTyhuWUrjwBg8iY7RSaY6H8aA8oovIa8zz0+s9zwLnXSP\n\tzWbTNfebaplDz96+avV/yMxRZSqzRWb9GPrscNl3ACALTKzeL/YlhA4f1GMbH+EzFeT+\n\t5CHQ==","X-Gm-Message-State":"AA+aEWbcLpbELMgb1v1jqbOSf3Xup/uF32aORtsnBEDTYgJZxtzXOkCN\n\t+HIvrH6JTrynQ9L4TD5Xw/ORyfaNt8U=","X-Google-Smtp-Source":"AFSGD/V2Z2Cup+nOylWV3qU00HUSzJ9yLQk+1vQ/+zKWUygWhFQXomf/pYFiTh0wjmlNWCYYHiOEsw==","X-Received":"by 2002:a19:574d:: with SMTP id l74mr17350506lfb.5.1546212121496;\n\tSun, 30 Dec 2018 15:22:01 -0800 (PST)","Date":"Mon, 31 Dec 2018 00:22:00 +0100","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20181230232200.GD10929@bigcity.dyn.berto.se>","References":"<20181229032855.26249-1-niklas.soderlund@ragnatech.se>\n\t<20181229032855.26249-11-niklas.soderlund@ragnatech.se>\n\t<9669754.P9YqOJaAqb@avalon>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<9669754.P9YqOJaAqb@avalon>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v2 10/12] libcamera: camera_manager:\n\tadd CameraManager class","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":"Sun, 30 Dec 2018 23:22:03 -0000"}}]