[{"id":3588,"web_url":"https://patchwork.libcamera.org/comment/3588/","msgid":"<8fbd15f4-d47e-e88f-41e2-241784cab7cb@ideasonboard.com>","date":"2020-01-23T14:35:54","subject":"Re: [libcamera-devel] [PATCH v2 02/13] libcamera: camera_manager:\n\tMove private data members to private implementation","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 22/01/2020 20:57, Laurent Pinchart wrote:\n> Use the d-pointer idiom ([1], [2]) to hide the private data members from\n> the CameraManager class interface. This will ease maintaining ABI\n> compatibility, and prepares for the implementation of the CameraManager\n> class threading model.\n> \n> [1] https://wiki.qt.io/D-Pointer\n> [2] https://en.cppreference.com/w/cpp/language/pimpl\n> \n> libcamera: camera_manager: Run the camera manager in a thread:q\n\nTrying to escape vim? \":q\" ?\nPerhaps this was a squashed commit or a fixup?\n\nOtherwise, It's certainly interesting moving things to an internal\nprivate class. The public objects then just become lightweight wrappers.\n\nSeems like it might end up generating a lot of boilerplate wrapping\naround classes, but getting some ABI stability will be a good thing...\n\n\nIt looks like the actual code is only moved, so there's no functional\nchanges intended here (except of course for the allocations being\ndifferent and wrapped).\n\nSo a generalised:\n\nReviewed-by: Kieran Bingham \n\n\n\n> Signed-off-by: Laurent Pinchart \n> ---\n> Documentation/Doxyfile.in | 1 +\n> include/libcamera/camera_manager.h | 13 +-\n> src/libcamera/camera_manager.cpp | 218 ++++++++++++++++++-----------\n> 3 files changed, 143 insertions(+), 89 deletions(-)\n> \n> diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in\n> index 8e6fbdbb92b6..1f746393568a 100644\n> --- a/Documentation/Doxyfile.in\n> +++ b/Documentation/Doxyfile.in\n> @@ -877,6 +877,7 @@ EXCLUDE_SYMBOLS = libcamera::BoundMethodArgs \\\n> libcamera::BoundMethodPackBase \\\n> libcamera::BoundMethodStatic \\\n> libcamera::SignalBase \\\n> + libcamera::*::Private \\\n\nI think I saw another patch from you that used ::details for something\nsimilar to this.\n\nWill ::Private be the preferred naming? Or will both end up being used?\n\n> std::*\n> \n> # The EXAMPLE_PATH tag can be used to specify one or more files or directories\n> diff --git a/include/libcamera/camera_manager.h b/include/libcamera/camera_manager.h\n> index 094197668698..068afd58762f 100644\n> --- a/include/libcamera/camera_manager.h\n> +++ b/include/libcamera/camera_manager.h\n> @@ -7,7 +7,6 @@\n> #ifndef __LIBCAMERA_CAMERA_MANAGER_H__\n> #define __LIBCAMERA_CAMERA_MANAGER_H__\n> \n> -#include \n> #include \n> #include \n> #include \n> @@ -18,9 +17,7 @@\n> namespace libcamera {\n> \n> class Camera;\n> -class DeviceEnumerator;\n> class EventDispatcher;\n> -class PipelineHandler;\n> \n> class CameraManager : public Object\n> {\n> @@ -33,7 +30,7 @@ public:\n> \tint start();\n> \tvoid stop();\n> \n> -\tconst std::vector> &cameras() const { return cameras_; }\n> +\tconst std::vector> &cameras() const;\n> \tstd::shared_ptr get(const std::string &name);\n> \tstd::shared_ptr get(dev_t devnum);\n> \n> @@ -46,13 +43,11 @@ public:\n> \tEventDispatcher *eventDispatcher();\n> \n> private:\n> -\tstd::unique_ptr enumerator_;\n> -\tstd::vector> pipes_;\n> -\tstd::vector> cameras_;\n> -\tstd::map> camerasByDevnum_;\n> -\n> \tstatic const std::string version_;\n> \tstatic CameraManager *self_;\n> +\n> +\tclass Private;\n> +\tstd::unique_ptr p_;\n> };\n> \n> } /* namespace libcamera */\n> diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp\n> index a71caf8536bb..e0a07ec557d3 100644\n> --- a/src/libcamera/camera_manager.cpp\n> +++ b/src/libcamera/camera_manager.cpp\n> @@ -7,6 +7,8 @@\n> \n> #include \n> \n> +#include \n> +\n> #include \n> #include \n> \n> @@ -26,6 +28,124 @@ namespace libcamera {\n> \n> LOG_DEFINE_CATEGORY(Camera)\n> \n> +class CameraManager::Private\n> +{\n> +public:\n> +\tPrivate(CameraManager *cm);\n> +\n> +\tint start();\n> +\tvoid stop();\n> +\n> +\tvoid addCamera(std::shared_ptr &camera, dev_t devnum);\n> +\tvoid removeCamera(Camera *camera);\n> +\n> +\tstd::vector> cameras_;\n> +\tstd::map> camerasByDevnum_;\n> +\n> +private:\n> +\tCameraManager *cm_;\n> +\n> +\tstd::vector> pipes_;\n> +\tstd::unique_ptr enumerator_;\n> +};\n> +\n> +CameraManager::Private::Private(CameraManager *cm)\n\nA private private? :-D (I see it's the constructor it just reads oddly\nbecause of the namespacing.)\n\n\n\n> +\t: cm_(cm)\n> +{\n> +}\n> +\n> +int CameraManager::Private::start()\n> +{\n> +\tenumerator_ = DeviceEnumerator::create();\n> +\tif (!enumerator_ || 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> +\tstd::vector &factories = PipelineHandlerFactory::factories();\n> +\n> +\tfor (PipelineHandlerFactory *factory : factories) {\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\twhile (1) {\n> +\t\t\tstd::shared_ptr pipe = factory->create(cm_);\n> +\t\t\tif (!pipe->match(enumerator_.get()))\n> +\t\t\t\tbreak;\n> +\n> +\t\t\tLOG(Camera, Debug)\n> +\t\t\t\t<< \"Pipeline handler \\\"\" << factory->name()\n> +\t\t\t\t<< \"\\\" matched\";\n> +\t\t\tpipes_.push_back(std::move(pipe));\n> +\t\t}\n> +\t}\n> +\n> +\t/* TODO: register hot-plug callback here */\n> +\n> +\treturn 0;\n> +}\n> +\n> +void CameraManager::Private::stop()\n> +{\n> +\t/* TODO: unregister hot-plug callback here */\n> +\n> +\t/*\n> +\t * Release all references to cameras and pipeline handlers to ensure\n> +\t * they all get destroyed before the device enumerator deletes the\n> +\t * media devices.\n> +\t */\n> +\tpipes_.clear();\n> +\tcameras_.clear();\n> +\n> +\tenumerator_.reset(nullptr);\n> +}\n> +\n> +void CameraManager::Private::addCamera(std::shared_ptr &camera,\n> +\t\t\t\t dev_t devnum)\n> +{\n> +\tfor (std::shared_ptr c : cameras_) {\n> +\t\tif (c->name() == camera->name()) {\n> +\t\t\tLOG(Camera, Warning)\n> +\t\t\t\t<< \"Registering camera with duplicate name '\"\n> +\t\t\t\t<< camera->name() << \"'\";\n> +\t\t\tbreak;\n> +\t\t}\n> +\t}\n> +\n> +\tcameras_.push_back(std::move(camera));\n> +\n> +\tif (devnum) {\n> +\t\tunsigned int index = cameras_.size() - 1;\n> +\t\tcamerasByDevnum_[devnum] = cameras_[index];\n> +\t}\n> +}\n> +\n> +void CameraManager::Private::removeCamera(Camera *camera)\n> +{\n> +\tauto iter = std::find_if(cameras_.begin(), cameras_.end(),\n> +\t\t\t\t [camera](std::shared_ptr &c) {\n> +\t\t\t\t\t return c.get() == camera;\n> +\t\t\t\t });\n> +\tif (iter == cameras_.end())\n> +\t\treturn;\n> +\n> +\tLOG(Camera, Debug)\n> +\t\t<< \"Unregistering camera '\" << camera->name() << \"'\";\n> +\n> +\tauto iter_d = std::find_if(camerasByDevnum_.begin(), camerasByDevnum_.end(),\n> +\t\t\t\t [camera](const std::pair> &p) {\n> +\t\t\t\t\t return p.second.lock().get() == camera;\n> +\t\t\t\t });\n> +\tif (iter_d != camerasByDevnum_.end())\n> +\t\tcamerasByDevnum_.erase(iter_d);\n> +\n> +\tcameras_.erase(iter);\n> +}\n> +\n> /**\n> * \\class CameraManager\n> * \\brief Provide access and manage all cameras in the system\n> @@ -62,7 +182,7 @@ LOG_DEFINE_CATEGORY(Camera)\n> CameraManager *CameraManager::self_ = nullptr;\n> \n> CameraManager::CameraManager()\n> -\t: enumerator_(nullptr)\n> +\t: p_(new CameraManager::Private(this))\n\nThe std::unique_ptr<> will take care of free-ing this allocation right?\n\n> {\n> \tif (self_)\n> \t\tLOG(Camera, Fatal)\n> @@ -73,6 +193,8 @@ CameraManager::CameraManager()\n> \n> CameraManager::~CameraManager()\n> {\n> +\tstop();\n> +\n> \tself_ = nullptr;\n> }\n> \n> @@ -88,42 +210,14 @@ CameraManager::~CameraManager()\n> */\n> int CameraManager::start()\n> {\n> -\tif (enumerator_)\n> -\t\treturn -EBUSY;\n> -\n> \tLOG(Camera, Info) << \"libcamera \" << version_;\n> \n> -\tenumerator_ = DeviceEnumerator::create();\n> -\tif (!enumerator_ || 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> -\tstd::vector &factories = PipelineHandlerFactory::factories();\n> +\tint ret = p_->start();\n> +\tif (ret)\n> +\t\tLOG(Camera, Error) << \"Failed to start camera manager: \"\n> +\t\t\t\t << strerror(-ret);\n> \n> -\tfor (PipelineHandlerFactory *factory : factories) {\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\twhile (1) {\n> -\t\t\tstd::shared_ptr pipe = factory->create(this);\n> -\t\t\tif (!pipe->match(enumerator_.get()))\n> -\t\t\t\tbreak;\n> -\n> -\t\t\tLOG(Camera, Debug)\n> -\t\t\t\t<< \"Pipeline handler \\\"\" << factory->name()\n> -\t\t\t\t<< \"\\\" matched\";\n> -\t\t\tpipes_.push_back(std::move(pipe));\n> -\t\t}\n> -\t}\n> -\n> -\t/* TODO: register hot-plug callback here */\n> -\n> -\treturn 0;\n> +\treturn ret;\n> }\n> \n> /**\n> @@ -138,17 +232,7 @@ int CameraManager::start()\n> */\n> void CameraManager::stop()\n> {\n> -\t/* TODO: unregister hot-plug callback here */\n> -\n> -\t/*\n> -\t * Release all references to cameras and pipeline handlers to ensure\n> -\t * they all get destroyed before the device enumerator deletes the\n> -\t * media devices.\n> -\t */\n> -\tpipes_.clear();\n> -\tcameras_.clear();\n> -\n> -\tenumerator_.reset(nullptr);\n> +\tp_->stop();\n> }\n> \n> /**\n> @@ -160,6 +244,10 @@ void CameraManager::stop()\n> *\n> * \\return List of all available cameras\n> */\n> +const std::vector> &CameraManager::cameras() const\n> +{\n> +\treturn p_->cameras_;\n> +}\n> \n> /**\n> * \\brief Get a camera based on name\n> @@ -172,7 +260,7 @@ void CameraManager::stop()\n> */\n> std::shared_ptr CameraManager::get(const std::string &name)\n> {\n> -\tfor (std::shared_ptr camera : cameras_) {\n> +\tfor (std::shared_ptr camera : p_->cameras_) {\n> \t\tif (camera->name() == name)\n> \t\t\treturn camera;\n> \t}\n> @@ -196,8 +284,8 @@ std::shared_ptr CameraManager::get(const std::string &name)\n> */\n> std::shared_ptr CameraManager::get(dev_t devnum)\n> {\n> -\tauto iter = camerasByDevnum_.find(devnum);\n> -\tif (iter == camerasByDevnum_.end())\n> +\tauto iter = p_->camerasByDevnum_.find(devnum);\n> +\tif (iter == p_->camerasByDevnum_.end())\n> \t\treturn nullptr;\n> \n> \treturn iter->second.lock();\n> @@ -217,21 +305,8 @@ std::shared_ptr CameraManager::get(dev_t devnum)\n> */\n> void CameraManager::addCamera(std::shared_ptr camera, dev_t devnum)\n> {\n> -\tfor (std::shared_ptr c : cameras_) {\n> -\t\tif (c->name() == camera->name()) {\n> -\t\t\tLOG(Camera, Warning)\n> -\t\t\t\t<< \"Registering camera with duplicate name '\"\n> -\t\t\t\t<< camera->name() << \"'\";\n> -\t\t\tbreak;\n> -\t\t}\n> -\t}\n> \n> -\tcameras_.push_back(std::move(camera));\n> -\n> -\tif (devnum) {\n> -\t\tunsigned int index = cameras_.size() - 1;\n> -\t\tcamerasByDevnum_[devnum] = cameras_[index];\n> -\t}\n> +\tp_->addCamera(camera, devnum);\n> }\n> \n> /**\n> @@ -244,24 +319,7 @@ void CameraManager::addCamera(std::shared_ptr camera, dev_t devnum)\n> */\n> void CameraManager::removeCamera(Camera *camera)\n> {\n> -\tauto iter = std::find_if(cameras_.begin(), cameras_.end(),\n> -\t\t\t\t [camera](std::shared_ptr &c) {\n> -\t\t\t\t\t return c.get() == camera;\n> -\t\t\t\t });\n> -\tif (iter == cameras_.end())\n> -\t\treturn;\n> -\n> -\tLOG(Camera, Debug)\n> -\t\t<< \"Unregistering camera '\" << camera->name() << \"'\";\n> -\n> -\tauto iter_d = std::find_if(camerasByDevnum_.begin(), camerasByDevnum_.end(),\n> -\t\t\t\t [camera](const std::pair> &p) {\n> -\t\t\t\t\t return p.second.lock().get() == camera;\n> -\t\t\t\t });\n> -\tif (iter_d != camerasByDevnum_.end())\n> -\t\tcamerasByDevnum_.erase(iter_d);\n> -\n> -\tcameras_.erase(iter);\n> +\tp_->removeCamera(camera);\n> }\n> \n> /**\n>","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EF68360455\n\tfor ;\n\tThu, 23 Jan 2020 15:35:58 +0100 (CET)","from [192.168.0.20]\n\t(cpc89242-aztw30-2-0-cust488.18-1.cable.virginm.net [86.31.129.233])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 338682E5;\n\tThu, 23 Jan 2020 15:35:58 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1579790158;\n\tbh=WIi9vCYqcI8uROGFJKvKpC0CEWWlElBhWIgTAA8rWYA=;\n\th=Reply-To:Subject:To:References:From:Date:In-Reply-To:From;\n\tb=p7h50ADL86ZfiGi4miFFhomW7f/R/eZ3rg4lsKAiT+BXa7NzbmLBZidT4X1xnerve\n\tPn3FvCs3Y1xTBOOYDRNX+3g5oJ6osYPoGqV7yqSm0R+VswrhQ2fplbgVssstJq/sQA\n\twDzhBLM4q3YrWioh0mqV0DgBPK6ntfzFeW9bhSWE=","Reply-To":"kieran.bingham@ideasonboard.com","To":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20200122205723.8865-1-laurent.pinchart@ideasonboard.com>\n\t<20200122205723.8865-3-laurent.pinchart@ideasonboard.com>","From":"Kieran Bingham ","Openpgp":"preference=signencrypt","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"<8fbd15f4-d47e-e88f-41e2-241784cab7cb@ideasonboard.com>","Date":"Thu, 23 Jan 2020 14:35:54 +0000","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101\n\tThunderbird/60.9.0","MIME-Version":"1.0","In-Reply-To":"<20200122205723.8865-3-laurent.pinchart@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v2 02/13] libcamera: camera_manager:\n\tMove private data members to private implementation","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":"Thu, 23 Jan 2020 14:35:59 -0000"}},{"id":3590,"web_url":"https://patchwork.libcamera.org/comment/3590/","msgid":"<20200123203034.GA284046@oden.dyn.berto.se>","date":"2020-01-23T20:30:34","subject":"Re: [libcamera-devel] [PATCH v2 02/13] libcamera: camera_manager:\n\tMove private data members to private implementation","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 patch.\n\nOn 2020-01-22 22:57:12 +0200, Laurent Pinchart wrote:\n> Use the d-pointer idiom ([1], [2]) to hide the private data members from\n> the CameraManager class interface. This will ease maintaining ABI\n> compatibility, and prepares for the implementation of the CameraManager\n> class threading model.\n> \n> [1] https://wiki.qt.io/D-Pointer\n> [2] https://en.cppreference.com/w/cpp/language/pimpl\n> \n> libcamera: camera_manager: Run the camera manager in a thread:q\n\nAs Kieran points out I assume this line is not meant to be here :-)\n\n> \n> Signed-off-by: Laurent Pinchart \n\nWith that fixed,\n\nReviewed-by: Niklas Söderlund \n\n> ---\n> Documentation/Doxyfile.in | 1 +\n> include/libcamera/camera_manager.h | 13 +-\n> src/libcamera/camera_manager.cpp | 218 ++++++++++++++++++-----------\n> 3 files changed, 143 insertions(+), 89 deletions(-)\n> \n> diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in\n> index 8e6fbdbb92b6..1f746393568a 100644\n> --- a/Documentation/Doxyfile.in\n> +++ b/Documentation/Doxyfile.in\n> @@ -877,6 +877,7 @@ EXCLUDE_SYMBOLS = libcamera::BoundMethodArgs \\\n> libcamera::BoundMethodPackBase \\\n> libcamera::BoundMethodStatic \\\n> libcamera::SignalBase \\\n> + libcamera::*::Private \\\n> std::*\n> \n> # The EXAMPLE_PATH tag can be used to specify one or more files or directories\n> diff --git a/include/libcamera/camera_manager.h b/include/libcamera/camera_manager.h\n> index 094197668698..068afd58762f 100644\n> --- a/include/libcamera/camera_manager.h\n> +++ b/include/libcamera/camera_manager.h\n> @@ -7,7 +7,6 @@\n> #ifndef __LIBCAMERA_CAMERA_MANAGER_H__\n> #define __LIBCAMERA_CAMERA_MANAGER_H__\n> \n> -#include \n> #include \n> #include \n> #include \n> @@ -18,9 +17,7 @@\n> namespace libcamera {\n> \n> class Camera;\n> -class DeviceEnumerator;\n> class EventDispatcher;\n> -class PipelineHandler;\n> \n> class CameraManager : public Object\n> {\n> @@ -33,7 +30,7 @@ public:\n> \tint start();\n> \tvoid stop();\n> \n> -\tconst std::vector> &cameras() const { return cameras_; }\n> +\tconst std::vector> &cameras() const;\n> \tstd::shared_ptr get(const std::string &name);\n> \tstd::shared_ptr get(dev_t devnum);\n> \n> @@ -46,13 +43,11 @@ public:\n> \tEventDispatcher *eventDispatcher();\n> \n> private:\n> -\tstd::unique_ptr enumerator_;\n> -\tstd::vector> pipes_;\n> -\tstd::vector> cameras_;\n> -\tstd::map> camerasByDevnum_;\n> -\n> \tstatic const std::string version_;\n> \tstatic CameraManager *self_;\n> +\n> +\tclass Private;\n> +\tstd::unique_ptr p_;\n> };\n> \n> } /* namespace libcamera */\n> diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp\n> index a71caf8536bb..e0a07ec557d3 100644\n> --- a/src/libcamera/camera_manager.cpp\n> +++ b/src/libcamera/camera_manager.cpp\n> @@ -7,6 +7,8 @@\n> \n> #include \n> \n> +#include \n> +\n> #include \n> #include \n> \n> @@ -26,6 +28,124 @@ namespace libcamera {\n> \n> LOG_DEFINE_CATEGORY(Camera)\n> \n> +class CameraManager::Private\n> +{\n> +public:\n> +\tPrivate(CameraManager *cm);\n> +\n> +\tint start();\n> +\tvoid stop();\n> +\n> +\tvoid addCamera(std::shared_ptr &camera, dev_t devnum);\n> +\tvoid removeCamera(Camera *camera);\n> +\n> +\tstd::vector> cameras_;\n> +\tstd::map> camerasByDevnum_;\n> +\n> +private:\n> +\tCameraManager *cm_;\n> +\n> +\tstd::vector> pipes_;\n> +\tstd::unique_ptr enumerator_;\n> +};\n> +\n> +CameraManager::Private::Private(CameraManager *cm)\n> +\t: cm_(cm)\n> +{\n> +}\n> +\n> +int CameraManager::Private::start()\n> +{\n> +\tenumerator_ = DeviceEnumerator::create();\n> +\tif (!enumerator_ || 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> +\tstd::vector &factories = PipelineHandlerFactory::factories();\n> +\n> +\tfor (PipelineHandlerFactory *factory : factories) {\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\twhile (1) {\n> +\t\t\tstd::shared_ptr pipe = factory->create(cm_);\n> +\t\t\tif (!pipe->match(enumerator_.get()))\n> +\t\t\t\tbreak;\n> +\n> +\t\t\tLOG(Camera, Debug)\n> +\t\t\t\t<< \"Pipeline handler \\\"\" << factory->name()\n> +\t\t\t\t<< \"\\\" matched\";\n> +\t\t\tpipes_.push_back(std::move(pipe));\n> +\t\t}\n> +\t}\n> +\n> +\t/* TODO: register hot-plug callback here */\n> +\n> +\treturn 0;\n> +}\n> +\n> +void CameraManager::Private::stop()\n> +{\n> +\t/* TODO: unregister hot-plug callback here */\n> +\n> +\t/*\n> +\t * Release all references to cameras and pipeline handlers to ensure\n> +\t * they all get destroyed before the device enumerator deletes the\n> +\t * media devices.\n> +\t */\n> +\tpipes_.clear();\n> +\tcameras_.clear();\n> +\n> +\tenumerator_.reset(nullptr);\n> +}\n> +\n> +void CameraManager::Private::addCamera(std::shared_ptr &camera,\n> +\t\t\t\t dev_t devnum)\n> +{\n> +\tfor (std::shared_ptr c : cameras_) {\n> +\t\tif (c->name() == camera->name()) {\n> +\t\t\tLOG(Camera, Warning)\n> +\t\t\t\t<< \"Registering camera with duplicate name '\"\n> +\t\t\t\t<< camera->name() << \"'\";\n> +\t\t\tbreak;\n> +\t\t}\n> +\t}\n> +\n> +\tcameras_.push_back(std::move(camera));\n> +\n> +\tif (devnum) {\n> +\t\tunsigned int index = cameras_.size() - 1;\n> +\t\tcamerasByDevnum_[devnum] = cameras_[index];\n> +\t}\n> +}\n> +\n> +void CameraManager::Private::removeCamera(Camera *camera)\n> +{\n> +\tauto iter = std::find_if(cameras_.begin(), cameras_.end(),\n> +\t\t\t\t [camera](std::shared_ptr &c) {\n> +\t\t\t\t\t return c.get() == camera;\n> +\t\t\t\t });\n> +\tif (iter == cameras_.end())\n> +\t\treturn;\n> +\n> +\tLOG(Camera, Debug)\n> +\t\t<< \"Unregistering camera '\" << camera->name() << \"'\";\n> +\n> +\tauto iter_d = std::find_if(camerasByDevnum_.begin(), camerasByDevnum_.end(),\n> +\t\t\t\t [camera](const std::pair> &p) {\n> +\t\t\t\t\t return p.second.lock().get() == camera;\n> +\t\t\t\t });\n> +\tif (iter_d != camerasByDevnum_.end())\n> +\t\tcamerasByDevnum_.erase(iter_d);\n> +\n> +\tcameras_.erase(iter);\n> +}\n> +\n> /**\n> * \\class CameraManager\n> * \\brief Provide access and manage all cameras in the system\n> @@ -62,7 +182,7 @@ LOG_DEFINE_CATEGORY(Camera)\n> CameraManager *CameraManager::self_ = nullptr;\n> \n> CameraManager::CameraManager()\n> -\t: enumerator_(nullptr)\n> +\t: p_(new CameraManager::Private(this))\n> {\n> \tif (self_)\n> \t\tLOG(Camera, Fatal)\n> @@ -73,6 +193,8 @@ CameraManager::CameraManager()\n> \n> CameraManager::~CameraManager()\n> {\n> +\tstop();\n> +\n> \tself_ = nullptr;\n> }\n> \n> @@ -88,42 +210,14 @@ CameraManager::~CameraManager()\n> */\n> int CameraManager::start()\n> {\n> -\tif (enumerator_)\n> -\t\treturn -EBUSY;\n> -\n> \tLOG(Camera, Info) << \"libcamera \" << version_;\n> \n> -\tenumerator_ = DeviceEnumerator::create();\n> -\tif (!enumerator_ || 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> -\tstd::vector &factories = PipelineHandlerFactory::factories();\n> +\tint ret = p_->start();\n> +\tif (ret)\n> +\t\tLOG(Camera, Error) << \"Failed to start camera manager: \"\n> +\t\t\t\t << strerror(-ret);\n> \n> -\tfor (PipelineHandlerFactory *factory : factories) {\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\twhile (1) {\n> -\t\t\tstd::shared_ptr pipe = factory->create(this);\n> -\t\t\tif (!pipe->match(enumerator_.get()))\n> -\t\t\t\tbreak;\n> -\n> -\t\t\tLOG(Camera, Debug)\n> -\t\t\t\t<< \"Pipeline handler \\\"\" << factory->name()\n> -\t\t\t\t<< \"\\\" matched\";\n> -\t\t\tpipes_.push_back(std::move(pipe));\n> -\t\t}\n> -\t}\n> -\n> -\t/* TODO: register hot-plug callback here */\n> -\n> -\treturn 0;\n> +\treturn ret;\n> }\n> \n> /**\n> @@ -138,17 +232,7 @@ int CameraManager::start()\n> */\n> void CameraManager::stop()\n> {\n> -\t/* TODO: unregister hot-plug callback here */\n> -\n> -\t/*\n> -\t * Release all references to cameras and pipeline handlers to ensure\n> -\t * they all get destroyed before the device enumerator deletes the\n> -\t * media devices.\n> -\t */\n> -\tpipes_.clear();\n> -\tcameras_.clear();\n> -\n> -\tenumerator_.reset(nullptr);\n> +\tp_->stop();\n> }\n> \n> /**\n> @@ -160,6 +244,10 @@ void CameraManager::stop()\n> *\n> * \\return List of all available cameras\n> */\n> +const std::vector> &CameraManager::cameras() const\n> +{\n> +\treturn p_->cameras_;\n> +}\n> \n> /**\n> * \\brief Get a camera based on name\n> @@ -172,7 +260,7 @@ void CameraManager::stop()\n> */\n> std::shared_ptr CameraManager::get(const std::string &name)\n> {\n> -\tfor (std::shared_ptr camera : cameras_) {\n> +\tfor (std::shared_ptr camera : p_->cameras_) {\n> \t\tif (camera->name() == name)\n> \t\t\treturn camera;\n> \t}\n> @@ -196,8 +284,8 @@ std::shared_ptr CameraManager::get(const std::string &name)\n> */\n> std::shared_ptr CameraManager::get(dev_t devnum)\n> {\n> -\tauto iter = camerasByDevnum_.find(devnum);\n> -\tif (iter == camerasByDevnum_.end())\n> +\tauto iter = p_->camerasByDevnum_.find(devnum);\n> +\tif (iter == p_->camerasByDevnum_.end())\n> \t\treturn nullptr;\n> \n> \treturn iter->second.lock();\n> @@ -217,21 +305,8 @@ std::shared_ptr CameraManager::get(dev_t devnum)\n> */\n> void CameraManager::addCamera(std::shared_ptr camera, dev_t devnum)\n> {\n> -\tfor (std::shared_ptr c : cameras_) {\n> -\t\tif (c->name() == camera->name()) {\n> -\t\t\tLOG(Camera, Warning)\n> -\t\t\t\t<< \"Registering camera with duplicate name '\"\n> -\t\t\t\t<< camera->name() << \"'\";\n> -\t\t\tbreak;\n> -\t\t}\n> -\t}\n> \n> -\tcameras_.push_back(std::move(camera));\n> -\n> -\tif (devnum) {\n> -\t\tunsigned int index = cameras_.size() - 1;\n> -\t\tcamerasByDevnum_[devnum] = cameras_[index];\n> -\t}\n> +\tp_->addCamera(camera, devnum);\n> }\n> \n> /**\n> @@ -244,24 +319,7 @@ void CameraManager::addCamera(std::shared_ptr camera, dev_t devnum)\n> */\n> void CameraManager::removeCamera(Camera *camera)\n> {\n> -\tauto iter = std::find_if(cameras_.begin(), cameras_.end(),\n> -\t\t\t\t [camera](std::shared_ptr &c) {\n> -\t\t\t\t\t return c.get() == camera;\n> -\t\t\t\t });\n> -\tif (iter == cameras_.end())\n> -\t\treturn;\n> -\n> -\tLOG(Camera, Debug)\n> -\t\t<< \"Unregistering camera '\" << camera->name() << \"'\";\n> -\n> -\tauto iter_d = std::find_if(camerasByDevnum_.begin(), camerasByDevnum_.end(),\n> -\t\t\t\t [camera](const std::pair> &p) {\n> -\t\t\t\t\t return p.second.lock().get() == camera;\n> -\t\t\t\t });\n> -\tif (iter_d != camerasByDevnum_.end())\n> -\t\tcamerasByDevnum_.erase(iter_d);\n> -\n> -\tcameras_.erase(iter);\n> +\tp_->removeCamera(camera);\n> }\n> \n> /**\n> -- \n> Regards,\n> \n> Laurent Pinchart\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-lf1-x144.google.com (mail-lf1-x144.google.com\n\t[IPv6:2a00:1450:4864:20::144])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 452C4607EF\n\tfor ;\n\tThu, 23 Jan 2020 21:30:36 +0100 (CET)","by mail-lf1-x144.google.com with SMTP id z26so3327593lfg.13\n\tfor ;\n\tThu, 23 Jan 2020 12:30:36 -0800 (PST)","from localhost (h-93-159.A463.priv.bahnhof.se. [46.59.93.159])\n\tby smtp.gmail.com with ESMTPSA id\n\tu19sm1599483lfu.68.2020.01.23.12.30.34\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tThu, 23 Jan 2020 12:30:34 -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\tbh=ODlC/9CS9s5yiLzaUcYhE1e+uTcwK/1aDO6PjaCEjjo=;\n\tb=J/rlRcnymoerInSAwkEwgAKlVWNwgaqOjaDE/cI8zXH5gpX8Kd0zgc1IWUp9QjcM23\n\tCd5RHUTdG5GCHgE+UlKJL3vESE/LMVycz5b2YUHHFwQ48h9aQdKRkUM9mMnNXDnRSG0z\n\tL6V5lrTYqfwAYOTpJK7hREoaTR5pjTZRD3JV6gJ1uphFGiuFmDHy45kuGXVwDByTtvkW\n\toI1ie7lsW2uZujQop9ypL65ejKszwckCpjenW9DOuHWvGuoJWCBreqDIHiGqK4tbRQ+D\n\tn01uPJ9wpa03cPgTvuag95NaPXsRB3edcfLVd04COefnsP5/I/3z4x7uhOC85eiVFCxq\n\t2Avg==","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;\n\tbh=ODlC/9CS9s5yiLzaUcYhE1e+uTcwK/1aDO6PjaCEjjo=;\n\tb=RNZis00O8CnzMB2czvlVAQQ6gPvcOIOGuShWWEu2P9+OFJdKJWjU7RAS1o9c74nZCH\n\tdqHlNpQ3hiSk7KjqnssafXXtmjGlL+Ur2BquFv2binWTXNE7Q7oEH+KGh3kOVDrdv3Ha\n\tRyZv+l/aULXapQaAF4zpTabpMDdCg4YMJBsSDizBsr9wfQQdmKIk3FYDZm/AZCjwMfKj\n\tKQKemp0AJCJQd7mbyfZuC2iDtRRvl5y19OgQg5JPX8xgIHNp1pz96PRTJZnxfIe/6Nth\n\tbtUzDmtZYzOqr5CAxW3C4TPYS6LhAx3fOSMtlvdwNOVBKPBOU7No/YuBXZw2/eMba/RL\n\tuI9A==","X-Gm-Message-State":"APjAAAVwhVyJRdKESd19jg4s/fqsAjN4tycc8kpffD7rBHegyi3UMjlg\n\tXa8mRXmPbztrDmbuddldiysGiw==","X-Google-Smtp-Source":"APXvYqyqr7HK0RLk9wV3qlCu8mx64Lbw1EJAQLKYkE+QtpOlIhXRGCRnMcDcwDrAAetpuvYP1IzYbg==","X-Received":"by 2002:ac2:46dc:: with SMTP id p28mr5584720lfo.23.1579811435500;\n\tThu, 23 Jan 2020 12:30:35 -0800 (PST)","Date":"Thu, 23 Jan 2020 21:30:34 +0100","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200123203034.GA284046@oden.dyn.berto.se>","References":"<20200122205723.8865-1-laurent.pinchart@ideasonboard.com>\n\t<20200122205723.8865-3-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20200122205723.8865-3-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 02/13] libcamera: camera_manager:\n\tMove private data members to private implementation","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":"Thu, 23 Jan 2020 20:30:36 -0000"}},{"id":3591,"web_url":"https://patchwork.libcamera.org/comment/3591/","msgid":"<20200123203532.GA29589@pendragon.ideasonboard.com>","date":"2020-01-23T20:35:32","subject":"Re: [libcamera-devel] [PATCH v2 02/13] libcamera: camera_manager:\n\tMove private data members to private implementation","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Thu, Jan 23, 2020 at 02:35:54PM +0000, Kieran Bingham wrote:\n> On 22/01/2020 20:57, Laurent Pinchart wrote:\n> > Use the d-pointer idiom ([1], [2]) to hide the private data members from\n> > the CameraManager class interface. This will ease maintaining ABI\n> > compatibility, and prepares for the implementation of the CameraManager\n> > class threading model.\n> > \n> > [1] https://wiki.qt.io/D-Pointer\n> > [2] https://en.cppreference.com/w/cpp/language/pimpl\n> > \n> > libcamera: camera_manager: Run the camera manager in a thread:q\n> \n> Trying to escape vim? \":q\" ?\n\nOops :-)\n\n> Perhaps this was a squashed commit or a fixup?\n> \n> Otherwise, It's certainly interesting moving things to an internal\n> private class. The public objects then just become lightweight wrappers.\n\nIn the CameraManager case I need an internal class inheriting from\nThread for a subsequent patch in this series, so it makes sense to use\nit to hide the details from the public API and help with ABI stability.\n\n> Seems like it might end up generating a lot of boilerplate wrapping\n> around classes, but getting some ABI stability will be a good thing...\n> \n> It looks like the actual code is only moved, so there's no functional\n> changes intended here (except of course for the allocations being\n> different and wrapped).\n\nCorrect.\n\n> So a generalised:\n> \n> Reviewed-by: Kieran Bingham \n> \n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > Documentation/Doxyfile.in | 1 +\n> > include/libcamera/camera_manager.h | 13 +-\n> > src/libcamera/camera_manager.cpp | 218 ++++++++++++++++++-----------\n> > 3 files changed, 143 insertions(+), 89 deletions(-)\n> > \n> > diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in\n> > index 8e6fbdbb92b6..1f746393568a 100644\n> > --- a/Documentation/Doxyfile.in\n> > +++ b/Documentation/Doxyfile.in\n> > @@ -877,6 +877,7 @@ EXCLUDE_SYMBOLS = libcamera::BoundMethodArgs \\\n> > libcamera::BoundMethodPackBase \\\n> > libcamera::BoundMethodStatic \\\n> > libcamera::SignalBase \\\n> > + libcamera::*::Private \\\n> \n> I think I saw another patch from you that used ::details for something\n> similar to this.\n> \n> Will ::Private be the preferred naming? Or will both end up being used?\n\nQt would have used CameraManagerPrivate. I like CameraManager::Private\nbecause it's shorter in contexts where CameraManager is the active name\nspace. details:: come from C++ STL and is used to hide implementation\ndetails from the std:: namespace. I'd rather use it for the same purpose\nwhen needed (to support templates with helpers that need to be present\nin public headers but are not part of the official API), but I'm open to\ndiscussing this. In any case this is a class, so it would need to be\nwrite CameraManager::Details, and in that case I think Private is a\nbetter match (but I may be biased).\n\n> > std::*\n> > \n> > # The EXAMPLE_PATH tag can be used to specify one or more files or directories\n> > diff --git a/include/libcamera/camera_manager.h b/include/libcamera/camera_manager.h\n> > index 094197668698..068afd58762f 100644\n> > --- a/include/libcamera/camera_manager.h\n> > +++ b/include/libcamera/camera_manager.h\n> > @@ -7,7 +7,6 @@\n> > #ifndef __LIBCAMERA_CAMERA_MANAGER_H__\n> > #define __LIBCAMERA_CAMERA_MANAGER_H__\n> > \n> > -#include \n> > #include \n> > #include \n> > #include \n> > @@ -18,9 +17,7 @@\n> > namespace libcamera {\n> > \n> > class Camera;\n> > -class DeviceEnumerator;\n> > class EventDispatcher;\n> > -class PipelineHandler;\n> > \n> > class CameraManager : public Object\n> > {\n> > @@ -33,7 +30,7 @@ public:\n> > \tint start();\n> > \tvoid stop();\n> > \n> > -\tconst std::vector> &cameras() const { return cameras_; }\n> > +\tconst std::vector> &cameras() const;\n> > \tstd::shared_ptr get(const std::string &name);\n> > \tstd::shared_ptr get(dev_t devnum);\n> > \n> > @@ -46,13 +43,11 @@ public:\n> > \tEventDispatcher *eventDispatcher();\n> > \n> > private:\n> > -\tstd::unique_ptr enumerator_;\n> > -\tstd::vector> pipes_;\n> > -\tstd::vector> cameras_;\n> > -\tstd::map> camerasByDevnum_;\n> > -\n> > \tstatic const std::string version_;\n> > \tstatic CameraManager *self_;\n> > +\n> > +\tclass Private;\n> > +\tstd::unique_ptr p_;\n> > };\n> > \n> > } /* namespace libcamera */\n> > diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp\n> > index a71caf8536bb..e0a07ec557d3 100644\n> > --- a/src/libcamera/camera_manager.cpp\n> > +++ b/src/libcamera/camera_manager.cpp\n> > @@ -7,6 +7,8 @@\n> > \n> > #include \n> > \n> > +#include \n> > +\n> > #include \n> > #include \n> > \n> > @@ -26,6 +28,124 @@ namespace libcamera {\n> > \n> > LOG_DEFINE_CATEGORY(Camera)\n> > \n> > +class CameraManager::Private\n> > +{\n> > +public:\n> > +\tPrivate(CameraManager *cm);\n> > +\n> > +\tint start();\n> > +\tvoid stop();\n> > +\n> > +\tvoid addCamera(std::shared_ptr &camera, dev_t devnum);\n> > +\tvoid removeCamera(Camera *camera);\n> > +\n> > +\tstd::vector> cameras_;\n> > +\tstd::map> camerasByDevnum_;\n> > +\n> > +private:\n> > +\tCameraManager *cm_;\n> > +\n> > +\tstd::vector> pipes_;\n> > +\tstd::unique_ptr enumerator_;\n> > +};\n> > +\n> > +CameraManager::Private::Private(CameraManager *cm)\n> \n> A private private? :-D (I see it's the constructor it just reads oddly\n> because of the namespacing.)\n\n:-) I think CameraManagerPrivate::CameraManagerPrivate is a bit worse\n(at the very least it's longer), but I could again be biased.\n\n> > +\t: cm_(cm)\n> > +{\n> > +}\n> > +\n> > +int CameraManager::Private::start()\n> > +{\n> > +\tenumerator_ = DeviceEnumerator::create();\n> > +\tif (!enumerator_ || 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> > +\tstd::vector &factories = PipelineHandlerFactory::factories();\n> > +\n> > +\tfor (PipelineHandlerFactory *factory : factories) {\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\twhile (1) {\n> > +\t\t\tstd::shared_ptr pipe = factory->create(cm_);\n> > +\t\t\tif (!pipe->match(enumerator_.get()))\n> > +\t\t\t\tbreak;\n> > +\n> > +\t\t\tLOG(Camera, Debug)\n> > +\t\t\t\t<< \"Pipeline handler \\\"\" << factory->name()\n> > +\t\t\t\t<< \"\\\" matched\";\n> > +\t\t\tpipes_.push_back(std::move(pipe));\n> > +\t\t}\n> > +\t}\n> > +\n> > +\t/* TODO: register hot-plug callback here */\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +void CameraManager::Private::stop()\n> > +{\n> > +\t/* TODO: unregister hot-plug callback here */\n> > +\n> > +\t/*\n> > +\t * Release all references to cameras and pipeline handlers to ensure\n> > +\t * they all get destroyed before the device enumerator deletes the\n> > +\t * media devices.\n> > +\t */\n> > +\tpipes_.clear();\n> > +\tcameras_.clear();\n> > +\n> > +\tenumerator_.reset(nullptr);\n> > +}\n> > +\n> > +void CameraManager::Private::addCamera(std::shared_ptr &camera,\n> > +\t\t\t\t dev_t devnum)\n> > +{\n> > +\tfor (std::shared_ptr c : cameras_) {\n> > +\t\tif (c->name() == camera->name()) {\n> > +\t\t\tLOG(Camera, Warning)\n> > +\t\t\t\t<< \"Registering camera with duplicate name '\"\n> > +\t\t\t\t<< camera->name() << \"'\";\n> > +\t\t\tbreak;\n> > +\t\t}\n> > +\t}\n> > +\n> > +\tcameras_.push_back(std::move(camera));\n> > +\n> > +\tif (devnum) {\n> > +\t\tunsigned int index = cameras_.size() - 1;\n> > +\t\tcamerasByDevnum_[devnum] = cameras_[index];\n> > +\t}\n> > +}\n> > +\n> > +void CameraManager::Private::removeCamera(Camera *camera)\n> > +{\n> > +\tauto iter = std::find_if(cameras_.begin(), cameras_.end(),\n> > +\t\t\t\t [camera](std::shared_ptr &c) {\n> > +\t\t\t\t\t return c.get() == camera;\n> > +\t\t\t\t });\n> > +\tif (iter == cameras_.end())\n> > +\t\treturn;\n> > +\n> > +\tLOG(Camera, Debug)\n> > +\t\t<< \"Unregistering camera '\" << camera->name() << \"'\";\n> > +\n> > +\tauto iter_d = std::find_if(camerasByDevnum_.begin(), camerasByDevnum_.end(),\n> > +\t\t\t\t [camera](const std::pair> &p) {\n> > +\t\t\t\t\t return p.second.lock().get() == camera;\n> > +\t\t\t\t });\n> > +\tif (iter_d != camerasByDevnum_.end())\n> > +\t\tcamerasByDevnum_.erase(iter_d);\n> > +\n> > +\tcameras_.erase(iter);\n> > +}\n> > +\n> > /**\n> > * \\class CameraManager\n> > * \\brief Provide access and manage all cameras in the system\n> > @@ -62,7 +182,7 @@ LOG_DEFINE_CATEGORY(Camera)\n> > CameraManager *CameraManager::self_ = nullptr;\n> > \n> > CameraManager::CameraManager()\n> > -\t: enumerator_(nullptr)\n> > +\t: p_(new CameraManager::Private(this))\n> \n> The std::unique_ptr<> will take care of free-ing this allocation right?\n\nCorrect.\n\n> > {\n> > \tif (self_)\n> > \t\tLOG(Camera, Fatal)\n> > @@ -73,6 +193,8 @@ CameraManager::CameraManager()\n> > \n> > CameraManager::~CameraManager()\n> > {\n> > +\tstop();\n> > +\n> > \tself_ = nullptr;\n> > }\n> > \n> > @@ -88,42 +210,14 @@ CameraManager::~CameraManager()\n> > */\n> > int CameraManager::start()\n> > {\n> > -\tif (enumerator_)\n> > -\t\treturn -EBUSY;\n> > -\n> > \tLOG(Camera, Info) << \"libcamera \" << version_;\n> > \n> > -\tenumerator_ = DeviceEnumerator::create();\n> > -\tif (!enumerator_ || 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> > -\tstd::vector &factories = PipelineHandlerFactory::factories();\n> > +\tint ret = p_->start();\n> > +\tif (ret)\n> > +\t\tLOG(Camera, Error) << \"Failed to start camera manager: \"\n> > +\t\t\t\t << strerror(-ret);\n> > \n> > -\tfor (PipelineHandlerFactory *factory : factories) {\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\twhile (1) {\n> > -\t\t\tstd::shared_ptr pipe = factory->create(this);\n> > -\t\t\tif (!pipe->match(enumerator_.get()))\n> > -\t\t\t\tbreak;\n> > -\n> > -\t\t\tLOG(Camera, Debug)\n> > -\t\t\t\t<< \"Pipeline handler \\\"\" << factory->name()\n> > -\t\t\t\t<< \"\\\" matched\";\n> > -\t\t\tpipes_.push_back(std::move(pipe));\n> > -\t\t}\n> > -\t}\n> > -\n> > -\t/* TODO: register hot-plug callback here */\n> > -\n> > -\treturn 0;\n> > +\treturn ret;\n> > }\n> > \n> > /**\n> > @@ -138,17 +232,7 @@ int CameraManager::start()\n> > */\n> > void CameraManager::stop()\n> > {\n> > -\t/* TODO: unregister hot-plug callback here */\n> > -\n> > -\t/*\n> > -\t * Release all references to cameras and pipeline handlers to ensure\n> > -\t * they all get destroyed before the device enumerator deletes the\n> > -\t * media devices.\n> > -\t */\n> > -\tpipes_.clear();\n> > -\tcameras_.clear();\n> > -\n> > -\tenumerator_.reset(nullptr);\n> > +\tp_->stop();\n> > }\n> > \n> > /**\n> > @@ -160,6 +244,10 @@ void CameraManager::stop()\n> > *\n> > * \\return List of all available cameras\n> > */\n> > +const std::vector> &CameraManager::cameras() const\n> > +{\n> > +\treturn p_->cameras_;\n> > +}\n> > \n> > /**\n> > * \\brief Get a camera based on name\n> > @@ -172,7 +260,7 @@ void CameraManager::stop()\n> > */\n> > std::shared_ptr CameraManager::get(const std::string &name)\n> > {\n> > -\tfor (std::shared_ptr camera : cameras_) {\n> > +\tfor (std::shared_ptr camera : p_->cameras_) {\n> > \t\tif (camera->name() == name)\n> > \t\t\treturn camera;\n> > \t}\n> > @@ -196,8 +284,8 @@ std::shared_ptr CameraManager::get(const std::string &name)\n> > */\n> > std::shared_ptr CameraManager::get(dev_t devnum)\n> > {\n> > -\tauto iter = camerasByDevnum_.find(devnum);\n> > -\tif (iter == camerasByDevnum_.end())\n> > +\tauto iter = p_->camerasByDevnum_.find(devnum);\n> > +\tif (iter == p_->camerasByDevnum_.end())\n> > \t\treturn nullptr;\n> > \n> > \treturn iter->second.lock();\n> > @@ -217,21 +305,8 @@ std::shared_ptr CameraManager::get(dev_t devnum)\n> > */\n> > void CameraManager::addCamera(std::shared_ptr camera, dev_t devnum)\n> > {\n> > -\tfor (std::shared_ptr c : cameras_) {\n> > -\t\tif (c->name() == camera->name()) {\n> > -\t\t\tLOG(Camera, Warning)\n> > -\t\t\t\t<< \"Registering camera with duplicate name '\"\n> > -\t\t\t\t<< camera->name() << \"'\";\n> > -\t\t\tbreak;\n> > -\t\t}\n> > -\t}\n> > \n> > -\tcameras_.push_back(std::move(camera));\n> > -\n> > -\tif (devnum) {\n> > -\t\tunsigned int index = cameras_.size() - 1;\n> > -\t\tcamerasByDevnum_[devnum] = cameras_[index];\n> > -\t}\n> > +\tp_->addCamera(camera, devnum);\n> > }\n> > \n> > /**\n> > @@ -244,24 +319,7 @@ void CameraManager::addCamera(std::shared_ptr camera, dev_t devnum)\n> > */\n> > void CameraManager::removeCamera(Camera *camera)\n> > {\n> > -\tauto iter = std::find_if(cameras_.begin(), cameras_.end(),\n> > -\t\t\t\t [camera](std::shared_ptr &c) {\n> > -\t\t\t\t\t return c.get() == camera;\n> > -\t\t\t\t });\n> > -\tif (iter == cameras_.end())\n> > -\t\treturn;\n> > -\n> > -\tLOG(Camera, Debug)\n> > -\t\t<< \"Unregistering camera '\" << camera->name() << \"'\";\n> > -\n> > -\tauto iter_d = std::find_if(camerasByDevnum_.begin(), camerasByDevnum_.end(),\n> > -\t\t\t\t [camera](const std::pair> &p) {\n> > -\t\t\t\t\t return p.second.lock().get() == camera;\n> > -\t\t\t\t });\n> > -\tif (iter_d != camerasByDevnum_.end())\n> > -\t\tcamerasByDevnum_.erase(iter_d);\n> > -\n> > -\tcameras_.erase(iter);\n> > +\tp_->removeCamera(camera);\n> > }\n> > \n> > /**","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 61B45607EF\n\tfor ;\n\tThu, 23 Jan 2020 21:35:49 +0100 (CET)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id CC7C12D2;\n\tThu, 23 Jan 2020 21:35:48 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1579811749;\n\tbh=oL6JNEc9wp3zkrU8r8ugyj79QMn//hUMrHea4w3SSIM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=senuiQbD/e0+XDAJNuvK/q2r1zYIjSartj9G/AEBUE1vg83P4pBTYqNRrmF0XnSm5\n\tn/VjHzsBFsXyXB1ucKkk9JQAyeGA0j+PWkq2hJtctEPbnAeceAOQ+6D0PQrHnlUNp9\n\t9RoIePc835I05trQGh6huiHwzYg0qcUh8rRTyv1U=","Date":"Thu, 23 Jan 2020 22:35:32 +0200","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200123203532.GA29589@pendragon.ideasonboard.com>","References":"<20200122205723.8865-1-laurent.pinchart@ideasonboard.com>\n\t<20200122205723.8865-3-laurent.pinchart@ideasonboard.com>\n\t<8fbd15f4-d47e-e88f-41e2-241784cab7cb@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<8fbd15f4-d47e-e88f-41e2-241784cab7cb@ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v2 02/13] libcamera: camera_manager:\n\tMove private data members to private implementation","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":"Thu, 23 Jan 2020 20:35:49 -0000"}}]