[{"id":109,"web_url":"https://patchwork.libcamera.org/comment/109/","msgid":"<8141132.q9shXpV6BA@avalon>","date":"2018-12-29T00:20:41","subject":"Re: [libcamera-devel] [PATCH 10/12] libcamera: cameramanager: add\n\tCameraManager 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 Sunday, 23 December 2018 01:00:39 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 <niklas.soderlund@ragnatech.se>\n> ---\n>  include/libcamera/cameramanager.h |  38 +++++++\n>  include/libcamera/libcamera.h     |   1 +\n>  include/libcamera/meson.build     |   1 +\n>  src/libcamera/cameramanager.cpp   | 173 ++++++++++++++++++++++++++++++\n>  src/libcamera/meson.build         |   1 +\n>  5 files changed, 214 insertions(+)\n>  create mode 100644 include/libcamera/cameramanager.h\n>  create mode 100644 src/libcamera/cameramanager.cpp\n> \n> diff --git a/include/libcamera/cameramanager.h\n> b/include/libcamera/cameramanager.h new file mode 100644\n> index 0000000000000000..be078a75f9e5aefc\n> --- /dev/null\n> +++ b/include/libcamera/cameramanager.h\n> @@ -0,0 +1,38 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2018, Google Inc.\n> + *\n> + * cameramanager.h - Camera management\n> + */\n> +#ifndef __LIBCAMERA_CAMERAMANAGER_H__\n> +#define __LIBCAMERA_CAMERAMANAGER_H__\n> +\n> +#include <vector>\n> +#include <string>\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> +\tbool start();\n> +\tvoid stop();\n> +\n> +\tstd::vector<std::string> list() const;\n> +\tCamera *get(const std::string &name);\n> +\tvoid put(Camera *camera);\n> +\n> +private:\n> +\tDeviceEnumerator *enumerator_;\n> +\tstd::vector<PipelineHandler *> pipes_;\n> +};\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_CAMERAMANAGER_H__ */\n> diff --git a/include/libcamera/libcamera.h b/include/libcamera/libcamera.h\n> index 44c094d92feed5ba..286b3d4d28083b01 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 <libcamera/camera.h>\n> +#include <libcamera/cameramanager.h>\n> \n>  namespace libcamera {\n> \n> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build\n> index 9b266ad926681db9..b7ba1f124aada003 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> +    'cameramanager.h',\n>      'libcamera.h',\n>  ])\n> \n> diff --git a/src/libcamera/cameramanager.cpp\n> b/src/libcamera/cameramanager.cpp new file mode 100644\n> index 0000000000000000..53209e32d88e3ed3\n> --- /dev/null\n> +++ b/src/libcamera/cameramanager.cpp\n> @@ -0,0 +1,173 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2018, Google Inc.\n> + *\n> + * cameramanager.h - Camera management\n> + */\n> +\n> +#include <libcamera/cameramanager.h>\n> +\n> +#include \"deviceenumerator.h\"\n> +#include \"pipelinehandler.h\"\n> +\n> +/**\n> + * \\file cameramanager.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_(NULL)\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 stat have been confirmed the user is free to list and otherwise\n\ns/stat/start/\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> +bool CameraManager::start()\n\nShould we standardize on integer status (0 for success and negative error code \notherwise) ?\n\n> +{\n> +\tstd::vector<std::string> handlers;\n> +\n> +\tif (enumerator_)\n> +\t\treturn false;\n> +\n> +\tenumerator_ = DeviceEnumerator::create();\n> +\n> +\tif (enumerator_->enumerate())\n> +\t\treturn false;\n> +\n> +\t/* TODO: Try to read handlers and order from configuration\n\n/* on its own line.\n\n> +\t * file and only fallback on all handlers if there is no\n> +\t * configuration file.\n> +\t */\n> +\tPipelineHandler::handlers(handlers);\n> +\n> +\tfor (std::string const &handler : handlers) {\n\nInstead of getting a list of pipeline handler names and trying to create \npipelines by name with a lookup in PipelineHandler::create(), how about \naccessing the factories map directly ?\n\n> +\t\tPipelineHandler *pipe;\n> +\n> +\t\t/* Try each pipeline handler until it exhaust\n> +\t\t * all pipelines it can provide. */\n\nComment block format.\n\n> +\t\tdo {\n> +\t\t\tpipe = PipelineHandler::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 true;\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 have been stopped no resource provided my the camera\n\ns/have/has/\ns/my/by/\n\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> +\n> +\tif (enumerator_)\n> +\t\tdelete enumerator_;\n> +\n> +\tenumerator_ = NULL;\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<std::string> CameraManager::list() const\n> +{\n> +\tstd::vector<std::string> 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\nWould it make sense to construct the list at start() time and cache it ? \nAnother option would be to make the PipelineManager register the Camera \ninstances it creates with the CameraManager. I quite like the second option \nbut I haven't thought it through yet.\n\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> + * the camera manger is running. A camera fetched this way should be\n> + * returned to the camera manger once the caller is done with it.\n> + *\n> + * \\return Pointer to Camera object or NULL if camera not found\n> + */\n> +Camera *CameraManager::get(const std::string &name)\n\nThis requires camera names to be unique. While that's not a bad idea, I think \nthis interface could be improved. Let's do so on top of this series.\n\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 NULL;\n> +}\n> +\n> +/**\n> + * \\brief Return camera to the camera manager\n> + *\n> + * \\param[in] camera Camera to return to the manager\n> + *\n> + * After the camera have been returned to the camera manager the caller\n> + * should not use the pointer to the camera object it has returned.\n> + */\n> +void CameraManager::put(Camera *camera)\n> +{\n> +\tcamera->put();\n> +}\n\nI don't think we need this method. The user should call camera->put() \ndirectly. This should be documented in CameraManager::get().\n\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index 0776643d3064129d..8457e57939b862ed 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> +    'cameramanager.cpp',\n>      'deviceenumerator.cpp',\n>      'log.cpp',\n>      'main.cpp',","headers":{"Return-Path":"<laurent.pinchart@ideasonboard.com>","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 C8F7A60B2E\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSat, 29 Dec 2018 01:19:44 +0100 (CET)","from avalon.localnet (unknown\n\t[IPv6:2a02:2788:66a:3eb:2624:a446:f4b7:b19d])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5CA63DF;\n\tSat, 29 Dec 2018 01:19:44 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1546042784;\n\tbh=jj6iMaa5S5vScFHFDX7vymGEEiu8EEsG7CYQ8CUwqsQ=;\n\th=From:To:Cc:Subject:Date:In-Reply-To:References:From;\n\tb=NvDu/0MiomDVtZdUDSZVzINhgEeMyPDHkVLHpN3x9qxxz6Ct6WSznRad/GyUPA6G0\n\t15LqvxELBaKnQW1dXjdoJ11/AiLnQevHtT32xejpcq1qk8iHp+e2MAPhMthd++cssD\n\tdWN2cB/TMK72ASMQXfuDwyFBIRk22Ayv3H3xPVsg=","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"libcamera-devel@lists.libcamera.org","Date":"Sat, 29 Dec 2018 02:20:41 +0200","Message-ID":"<8141132.q9shXpV6BA@avalon>","Organization":"Ideas on Board Oy","In-Reply-To":"<20181222230041.29999-11-niklas.soderlund@ragnatech.se>","References":"<20181222230041.29999-1-niklas.soderlund@ragnatech.se>\n\t<20181222230041.29999-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 10/12] libcamera: cameramanager: add\n\tCameraManager class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Sat, 29 Dec 2018 00:19:45 -0000"}},{"id":119,"web_url":"https://patchwork.libcamera.org/comment/119/","msgid":"<20181229030437.GE19796@bigcity.dyn.berto.se>","date":"2018-12-29T03:04:37","subject":"Re: [libcamera-devel] [PATCH 10/12] libcamera: cameramanager: add\n\tCameraManager 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 comments.\n\nOn 2018-12-29 02:20:41 +0200, Laurent Pinchart wrote:\n> Hi Niklas,\n> \n> Thank you for the patch.\n> \n> On Sunday, 23 December 2018 01:00:39 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 <niklas.soderlund@ragnatech.se>\n> > ---\n> >  include/libcamera/cameramanager.h |  38 +++++++\n> >  include/libcamera/libcamera.h     |   1 +\n> >  include/libcamera/meson.build     |   1 +\n> >  src/libcamera/cameramanager.cpp   | 173 ++++++++++++++++++++++++++++++\n> >  src/libcamera/meson.build         |   1 +\n> >  5 files changed, 214 insertions(+)\n> >  create mode 100644 include/libcamera/cameramanager.h\n> >  create mode 100644 src/libcamera/cameramanager.cpp\n> > \n> > diff --git a/include/libcamera/cameramanager.h\n> > b/include/libcamera/cameramanager.h new file mode 100644\n> > index 0000000000000000..be078a75f9e5aefc\n> > --- /dev/null\n> > +++ b/include/libcamera/cameramanager.h\n> > @@ -0,0 +1,38 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2018, Google Inc.\n> > + *\n> > + * cameramanager.h - Camera management\n> > + */\n> > +#ifndef __LIBCAMERA_CAMERAMANAGER_H__\n> > +#define __LIBCAMERA_CAMERAMANAGER_H__\n> > +\n> > +#include <vector>\n> > +#include <string>\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> > +\tbool start();\n> > +\tvoid stop();\n> > +\n> > +\tstd::vector<std::string> list() const;\n> > +\tCamera *get(const std::string &name);\n> > +\tvoid put(Camera *camera);\n> > +\n> > +private:\n> > +\tDeviceEnumerator *enumerator_;\n> > +\tstd::vector<PipelineHandler *> pipes_;\n> > +};\n> > +\n> > +} /* namespace libcamera */\n> > +\n> > +#endif /* __LIBCAMERA_CAMERAMANAGER_H__ */\n> > diff --git a/include/libcamera/libcamera.h b/include/libcamera/libcamera.h\n> > index 44c094d92feed5ba..286b3d4d28083b01 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 <libcamera/camera.h>\n> > +#include <libcamera/cameramanager.h>\n> > \n> >  namespace libcamera {\n> > \n> > diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build\n> > index 9b266ad926681db9..b7ba1f124aada003 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> > +    'cameramanager.h',\n> >      'libcamera.h',\n> >  ])\n> > \n> > diff --git a/src/libcamera/cameramanager.cpp\n> > b/src/libcamera/cameramanager.cpp new file mode 100644\n> > index 0000000000000000..53209e32d88e3ed3\n> > --- /dev/null\n> > +++ b/src/libcamera/cameramanager.cpp\n> > @@ -0,0 +1,173 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2018, Google Inc.\n> > + *\n> > + * cameramanager.h - Camera management\n> > + */\n> > +\n> > +#include <libcamera/cameramanager.h>\n> > +\n> > +#include \"deviceenumerator.h\"\n> > +#include \"pipelinehandler.h\"\n> > +\n> > +/**\n> > + * \\file cameramanager.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_(NULL)\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 stat have been confirmed the user is free to list and otherwise\n> \n> s/stat/start/\n\nThanks.\n\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> > +bool CameraManager::start()\n> \n> Should we standardize on integer status (0 for success and negative error code \n> otherwise) ?\n\nLets, I felt bool was more C++ but would be happy to switch to the \nscheme you suggest here.\n\n> \n> > +{\n> > +\tstd::vector<std::string> handlers;\n> > +\n> > +\tif (enumerator_)\n> > +\t\treturn false;\n> > +\n> > +\tenumerator_ = DeviceEnumerator::create();\n> > +\n> > +\tif (enumerator_->enumerate())\n> > +\t\treturn false;\n> > +\n> > +\t/* TODO: Try to read handlers and order from configuration\n> \n> /* on its own line.\n\nDone.\n\n> \n> > +\t * file and only fallback on all handlers if there is no\n> > +\t * configuration file.\n> > +\t */\n> > +\tPipelineHandler::handlers(handlers);\n> > +\n> > +\tfor (std::string const &handler : handlers) {\n> \n> Instead of getting a list of pipeline handler names and trying to create \n> pipelines by name with a lookup in PipelineHandler::create(), how about \n> accessing the factories map directly ?\n\nI'm open to the idea but would like to implement the configuration file \nfor 'active' pipeline handlers before we make the switch.\n\n> \n> > +\t\tPipelineHandler *pipe;\n> > +\n> > +\t\t/* Try each pipeline handler until it exhaust\n> > +\t\t * all pipelines it can provide. */\n> \n> Comment block format.\n\nDone.\n\n> \n> > +\t\tdo {\n> > +\t\t\tpipe = PipelineHandler::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 true;\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 have been stopped no resource provided my the camera\n> \n> s/have/has/\n> s/my/by/\n\nThanks.\n\n> \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\nGood catch!\n\n> > +\n> > +\tif (enumerator_)\n> > +\t\tdelete enumerator_;\n> > +\n> > +\tenumerator_ = NULL;\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<std::string> CameraManager::list() const\n> > +{\n> > +\tstd::vector<std::string> 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> Would it make sense to construct the list at start() time and cache it ? \n> Another option would be to make the PipelineManager register the Camera \n> instances it creates with the CameraManager. I quite like the second option \n> but I haven't thought it through yet.\n\nI also like your second idea. I added it to the task list so we can fix \nthis on-top.\n\n> \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> > + * the camera manger is running. A camera fetched this way should be\n> > + * returned to the camera manger once the caller is done with it.\n> > + *\n> > + * \\return Pointer to Camera object or NULL if camera not found\n> > + */\n> > +Camera *CameraManager::get(const std::string &name)\n> \n> This requires camera names to be unique. While that's not a bad idea, I think \n> this interface could be improved. Let's do so on top of this series.\n\nI agree, this is one limitation of this series which should be solved \ngoing forward.\n\n> \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 NULL;\n> > +}\n> > +\n> > +/**\n> > + * \\brief Return camera to the camera manager\n> > + *\n> > + * \\param[in] camera Camera to return to the manager\n> > + *\n> > + * After the camera have been returned to the camera manager the caller\n> > + * should not use the pointer to the camera object it has returned.\n> > + */\n> > +void CameraManager::put(Camera *camera)\n> > +{\n> > +\tcamera->put();\n> > +}\n> \n> I don't think we need this method. The user should call camera->put() \n> directly. This should be documented in CameraManager::get().\n\nI like it!\n\n> \n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> > index 0776643d3064129d..8457e57939b862ed 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> > +    'cameramanager.cpp',\n> >      'deviceenumerator.cpp',\n> >      'log.cpp',\n> >      'main.cpp',\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart\n> \n> \n>","headers":{"Return-Path":"<niklas.soderlund@ragnatech.se>","Received":["from mail-lj1-x22e.google.com (mail-lj1-x22e.google.com\n\t[IPv6:2a00:1450:4864:20::22e])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8399360B30\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSat, 29 Dec 2018 04:04:39 +0100 (CET)","by mail-lj1-x22e.google.com with SMTP id t18-v6so20004225ljd.4\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 28 Dec 2018 19:04:39 -0800 (PST)","from localhost (89-233-230-99.cust.bredband2.com. [89.233.230.99])\n\tby smtp.gmail.com with ESMTPSA id\n\te19-v6sm8913546ljf.67.2018.12.28.19.04.37\n\t(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);\n\tFri, 28 Dec 2018 19:04:37 -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=Ff4BNn63IZGTlL//kHewCLQGEkg4aFFd2z66F+sKjQ8=;\n\tb=CBBU0+PgUS87LPmogKYjtC2Y6vi/XaqFG8gONPyfM+mscWMgOpWZXBP/a5rFqNHK8W\n\tqHGa/C0EDaNEU0cMHWrijJHtLSbzaG1LfGJ6M2IsFOMz5Jm9wTFPbox5VCymQQ35yli3\n\tFPLCmf1GWQmEXU1HxQSyBinn2ImihlI2gQEerRpcvsZYfZUvo99G5tMaz6fzpwaTXsP7\n\tSbXJPZZP/OsB4cRPbQRg0EwEZ/0xfbE23rOUZAufvyLaACXOGH8Kr1Fz3laXPHfn7BXp\n\tYGmFexJkFH2wy7GrQyi3VkXanC7R5So3+f8f3U26Qc8BADGiQb6qslyaAYyh2r3tIXs7\n\tB3UA==","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=Ff4BNn63IZGTlL//kHewCLQGEkg4aFFd2z66F+sKjQ8=;\n\tb=IQLtyMPEKmFy1pp2pFiJPtnEnRvjsTqvoHW6wiVAFASW85bAZ7HHV+C8G6bSeY8Y1b\n\tGarATwZ/MWaJ/qQMKFZPvxqdZuPyBZ48VYENkH8mt1PcfD9ZYdYGl+poz0Vunw10T/sM\n\tAAhUy4u5vsSFvZ+rpXulKVBBURBOI/h/7QMcfRP+PmpZbRGBEBNclraptupqzA/gcg7a\n\tGfRR6G/lLFzoO5ubTlz0GEl118xaY51TLxlAqkvXHQSzU0wknQMSv0YSfsuCV6YY8LQB\n\t90+91wrSKtR5biF/eQKqMXrRGRyyONtYz2WasKTTix7EH+wjRT6j2tB3s0bK5/qBYpnQ\n\tz0Uw==","X-Gm-Message-State":"AJcUukcP1Qk83xUTBxRNTi7qQQzFrDR6x9begqAgyNocdOwcumHkMjoq\n\tisnIrJFybgOeKPHdChkoB3G1VA==","X-Google-Smtp-Source":"ALg8bN5MNW8otjg3U7RtEDFSLtdcLeMjZ+7FtCB2QkJ1F17H96zRsAPblOeWvSKY4wv3FxYy5zptwQ==","X-Received":"by 2002:a2e:5703:: with SMTP id\n\tl3-v6mr17993438ljb.106.1546052678779; \n\tFri, 28 Dec 2018 19:04:38 -0800 (PST)","Date":"Sat, 29 Dec 2018 04:04:37 +0100","From":"Niklas =?iso-8859-1?q?S=F6derlund?= <niklas.soderlund@ragnatech.se>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20181229030437.GE19796@bigcity.dyn.berto.se>","References":"<20181222230041.29999-1-niklas.soderlund@ragnatech.se>\n\t<20181222230041.29999-11-niklas.soderlund@ragnatech.se>\n\t<8141132.q9shXpV6BA@avalon>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<8141132.q9shXpV6BA@avalon>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 10/12] libcamera: cameramanager: add\n\tCameraManager class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Sat, 29 Dec 2018 03:04:39 -0000"}}]