[{"id":554,"web_url":"https://patchwork.libcamera.org/comment/554/","msgid":"<20190124163800.GV4127@bigcity.dyn.berto.se>","date":"2019-01-24T16:38:00","subject":"Re: [libcamera-devel] [PATCH 01/10] libcamera: pipeline_handler:\n\tStore the camera manager pointer","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 work.\n\nOn 2019-01-24 12:16:42 +0200, Laurent Pinchart wrote:\n> Instead of passing the camera manager pointer to the match() function,\n> and later to more PipelineHandler functions, store it in the\n> PipelineHandler::manager_ member variable at construction time and\n> access it from there.\n> \n> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n\nReviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n\n> ---\n>  src/libcamera/camera_manager.cpp         |  4 +--\n>  src/libcamera/include/pipeline_handler.h | 20 ++++++++-----\n>  src/libcamera/pipeline/ipu3/ipu3.cpp     | 18 ++++++------\n>  src/libcamera/pipeline/uvcvideo.cpp      | 12 ++++----\n>  src/libcamera/pipeline/vimc.cpp          | 12 ++++----\n>  src/libcamera/pipeline_handler.cpp       | 36 ++++++++++++++++++++----\n>  6 files changed, 66 insertions(+), 36 deletions(-)\n> \n> diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp\n> index 4ea7ed44cc31..14410d4dcda7 100644\n> --- a/src/libcamera/camera_manager.cpp\n> +++ b/src/libcamera/camera_manager.cpp\n> @@ -98,8 +98,8 @@ int CameraManager::start()\n>  \t\t * all pipelines it can provide.\n>  \t\t */\n>  \t\twhile (1) {\n> -\t\t\tPipelineHandler *pipe = factory->create();\n> -\t\t\tif (!pipe->match(this, enumerator_.get())) {\n> +\t\t\tPipelineHandler *pipe = factory->create(this);\n> +\t\t\tif (!pipe->match(enumerator_.get())) {\n>  \t\t\t\tdelete pipe;\n>  \t\t\t\tbreak;\n>  \t\t\t}\n> diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h\n> index b03217d5bff8..7bb07d1ec5c7 100644\n> --- a/src/libcamera/include/pipeline_handler.h\n> +++ b/src/libcamera/include/pipeline_handler.h\n> @@ -19,9 +19,13 @@ class DeviceEnumerator;\n>  class PipelineHandler\n>  {\n>  public:\n> -\tvirtual ~PipelineHandler() { };\n> +\tPipelineHandler(CameraManager *manager);\n> +\tvirtual ~PipelineHandler();\n>  \n> -\tvirtual bool match(CameraManager *manager, DeviceEnumerator *enumerator) = 0;\n> +\tvirtual bool match(DeviceEnumerator *enumerator) = 0;\n> +\n> +protected:\n> +\tCameraManager *manager_;\n>  };\n>  \n>  class PipelineHandlerFactory\n> @@ -30,7 +34,7 @@ public:\n>  \tPipelineHandlerFactory(const char *name);\n>  \tvirtual ~PipelineHandlerFactory() { };\n>  \n> -\tvirtual PipelineHandler *create() = 0;\n> +\tvirtual PipelineHandler *create(CameraManager *manager) = 0;\n>  \n>  \tconst std::string &name() const { return name_; }\n>  \n> @@ -42,11 +46,13 @@ private:\n>  };\n>  \n>  #define REGISTER_PIPELINE_HANDLER(handler)\t\t\t\t\\\n> -class handler##Factory : public PipelineHandlerFactory {\t\t\\\n> +class handler##Factory : public PipelineHandlerFactory\t\t\t\\\n> +{\t\t\t\t\t\t\t\t\t\\\n>  public:\t\t\t\t\t\t\t\t\t\\\n> -\thandler##Factory() : PipelineHandlerFactory(#handler) { }\t\\\n> -\tPipelineHandler *create() final {\t\t\t\t\\\n> -\t\treturn new handler();\t\t\t\t\t\\\n> +\thandler##Factory() : PipelineHandlerFactory(#handler) {}\t\\\n> +\tPipelineHandler *create(CameraManager *manager) final\t\t\\\n> +\t{\t\t\t\t\t\t\t\t\\\n> +\t\treturn new handler(manager);\t\t\t\t\\\n>  \t}\t\t\t\t\t\t\t\t\\\n>  };\t\t\t\t\t\t\t\t\t\\\n>  static handler##Factory global_##handler##Factory;\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 8cbc10acfbb5..589b3078f301 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -23,20 +23,20 @@ LOG_DEFINE_CATEGORY(IPU3)\n>  class PipelineHandlerIPU3 : public PipelineHandler\n>  {\n>  public:\n> -\tPipelineHandlerIPU3();\n> +\tPipelineHandlerIPU3(CameraManager *manager);\n>  \t~PipelineHandlerIPU3();\n>  \n> -\tbool match(CameraManager *manager, DeviceEnumerator *enumerator);\n> +\tbool match(DeviceEnumerator *enumerator);\n>  \n>  private:\n>  \tMediaDevice *cio2_;\n>  \tMediaDevice *imgu_;\n>  \n> -\tvoid registerCameras(CameraManager *manager);\n> +\tvoid registerCameras();\n>  };\n>  \n> -PipelineHandlerIPU3::PipelineHandlerIPU3()\n> -\t: cio2_(nullptr), imgu_(nullptr)\n> +PipelineHandlerIPU3::PipelineHandlerIPU3(CameraManager *manager)\n> +\t: PipelineHandler(manager), cio2_(nullptr), imgu_(nullptr)\n>  {\n>  }\n>  \n> @@ -52,7 +52,7 @@ PipelineHandlerIPU3::~PipelineHandlerIPU3()\n>  \timgu_ = nullptr;\n>  }\n>  \n> -bool PipelineHandlerIPU3::match(CameraManager *manager, DeviceEnumerator *enumerator)\n> +bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator)\n>  {\n>  \tDeviceMatch cio2_dm(\"ipu3-cio2\");\n>  \tcio2_dm.add(\"ipu3-csi2 0\");\n> @@ -106,7 +106,7 @@ bool PipelineHandlerIPU3::match(CameraManager *manager, DeviceEnumerator *enumer\n>  \tif (cio2_->disableLinks())\n>  \t\tgoto error_close_cio2;\n>  \n> -\tregisterCameras(manager);\n> +\tregisterCameras();\n>  \n>  \tcio2_->close();\n>  \n> @@ -127,7 +127,7 @@ error_release_mdev:\n>   * media entity with function MEDIA_ENT_F_CAM_SENSOR) to one of the four\n>   * CIO2 CSI-2 receivers.\n>   */\n> -void PipelineHandlerIPU3::registerCameras(CameraManager *manager)\n> +void PipelineHandlerIPU3::registerCameras()\n>  {\n>  \t/*\n>  \t * For each CSI-2 receiver on the IPU3, create a Camera if an\n> @@ -172,7 +172,7 @@ void PipelineHandlerIPU3::registerCameras(CameraManager *manager)\n>  \n>  \t\tstd::string cameraName = sensor->name() + \" \" + std::to_string(id);\n>  \t\tstd::shared_ptr<Camera> camera = Camera::create(cameraName);\n> -\t\tmanager->addCamera(std::move(camera));\n> +\t\tmanager_->addCamera(std::move(camera));\n>  \n>  \t\tLOG(IPU3, Info)\n>  \t\t\t<< \"Registered Camera[\" << numCameras << \"] \\\"\"\n> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp\n> index 3ba69da8b775..92b23da73901 100644\n> --- a/src/libcamera/pipeline/uvcvideo.cpp\n> +++ b/src/libcamera/pipeline/uvcvideo.cpp\n> @@ -17,17 +17,17 @@ namespace libcamera {\n>  class PipelineHandlerUVC : public PipelineHandler\n>  {\n>  public:\n> -\tPipelineHandlerUVC();\n> +\tPipelineHandlerUVC(CameraManager *manager);\n>  \t~PipelineHandlerUVC();\n>  \n> -\tbool match(CameraManager *manager, DeviceEnumerator *enumerator);\n> +\tbool match(DeviceEnumerator *enumerator);\n>  \n>  private:\n>  \tMediaDevice *dev_;\n>  };\n>  \n> -PipelineHandlerUVC::PipelineHandlerUVC()\n> -\t: dev_(nullptr)\n> +PipelineHandlerUVC::PipelineHandlerUVC(CameraManager *manager)\n> +\t: PipelineHandler(manager), dev_(nullptr)\n>  {\n>  }\n>  \n> @@ -37,7 +37,7 @@ PipelineHandlerUVC::~PipelineHandlerUVC()\n>  \t\tdev_->release();\n>  }\n>  \n> -bool PipelineHandlerUVC::match(CameraManager *manager, DeviceEnumerator *enumerator)\n> +bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator)\n>  {\n>  \tDeviceMatch dm(\"uvcvideo\");\n>  \n> @@ -49,7 +49,7 @@ bool PipelineHandlerUVC::match(CameraManager *manager, DeviceEnumerator *enumera\n>  \tdev_->acquire();\n>  \n>  \tstd::shared_ptr<Camera> camera = Camera::create(dev_->model());\n> -\tmanager->addCamera(std::move(camera));\n> +\tmanager_->addCamera(std::move(camera));\n>  \n>  \treturn true;\n>  }\n> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp\n> index 82b9237a3d7d..f12d007cd956 100644\n> --- a/src/libcamera/pipeline/vimc.cpp\n> +++ b/src/libcamera/pipeline/vimc.cpp\n> @@ -17,17 +17,17 @@ namespace libcamera {\n>  class PipeHandlerVimc : public PipelineHandler\n>  {\n>  public:\n> -\tPipeHandlerVimc();\n> +\tPipeHandlerVimc(CameraManager *manager);\n>  \t~PipeHandlerVimc();\n>  \n> -\tbool match(CameraManager *manager, DeviceEnumerator *enumerator);\n> +\tbool match(DeviceEnumerator *enumerator);\n>  \n>  private:\n>  \tMediaDevice *dev_;\n>  };\n>  \n> -PipeHandlerVimc::PipeHandlerVimc()\n> -\t: dev_(nullptr)\n> +PipeHandlerVimc::PipeHandlerVimc(CameraManager *manager)\n> +\t: PipelineHandler(manager), dev_(nullptr)\n>  {\n>  }\n>  \n> @@ -37,7 +37,7 @@ PipeHandlerVimc::~PipeHandlerVimc()\n>  \t\tdev_->release();\n>  }\n>  \n> -bool PipeHandlerVimc::match(CameraManager *manager, DeviceEnumerator *enumerator)\n> +bool PipeHandlerVimc::match(DeviceEnumerator *enumerator)\n>  {\n>  \tDeviceMatch dm(\"vimc\");\n>  \n> @@ -65,7 +65,7 @@ bool PipeHandlerVimc::match(CameraManager *manager, DeviceEnumerator *enumerator\n>  \t * object is modeled.\n>  \t */\n>  \tstd::shared_ptr<Camera> camera = Camera::create(\"Dummy VIMC Camera\");\n> -\tmanager->addCamera(std::move(camera));\n> +\tmanager_->addCamera(std::move(camera));\n>  \n>  \treturn true;\n>  }\n> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> index c24feeafc503..45788487b5c0 100644\n> --- a/src/libcamera/pipeline_handler.cpp\n> +++ b/src/libcamera/pipeline_handler.cpp\n> @@ -34,18 +34,30 @@ LOG_DEFINE_CATEGORY(Pipeline)\n>   * with the pipelines it supports and creates corresponding Camera devices.\n>   */\n>  \n> +/**\n> + * \\brief Construct a PipelineHandler instance\n> + * \\param[in] manager The camera manager\n> + */\n> +PipelineHandler::PipelineHandler(CameraManager *manager)\n> +\t: manager_(manager)\n> +{\n> +}\n> +\n> +PipelineHandler::~PipelineHandler()\n> +{\n> +}\n> +\n>  /**\n>   * \\fn PipelineHandler::match(DeviceEnumerator *enumerator)\n>   * \\brief Match media devices and create camera instances\n> - * \\param manager The camera manager\n>   * \\param enumerator The enumerator providing all media devices found in the\n>   * system\n>   *\n>   * This function is the main entry point of the pipeline handler. It is called\n> - * by the camera manager with the \\a manager and \\a enumerator passed as\n> - * arguments. It shall acquire from the \\a enumerator all the media devices it\n> - * needs for a single pipeline, create one or multiple Camera instances and\n> - * register them with the \\a manager.\n> + * by the camera manager with the \\a enumerator passed as an argument. It shall\n> + * acquire from the \\a enumerator all the media devices it needs for a single\n> + * pipeline, create one or multiple Camera instances and register them with the\n> + * camera manager.\n>   *\n>   * If all media devices needed by the pipeline handler are found, they must all\n>   * be acquired by a call to MediaDevice::acquire(). This function shall then\n> @@ -66,6 +78,15 @@ LOG_DEFINE_CATEGORY(Pipeline)\n>   * created, or false otherwise\n>   */\n>  \n> +/**\n> + * \\var PipelineHandler::manager_\n> + * \\brief The Camera manager associated with the pipeline handler\n> + *\n> + * The camera manager pointer is stored in the pipeline handler for the\n> + * convenience of pipeline handler implementations. It remains valid and\n> + * constant for the whole lifetime of the pipeline handler.\n> + */\n> +\n>  /**\n>   * \\class PipelineHandlerFactory\n>   * \\brief Registration of PipelineHandler classes and creation of instances\n> @@ -96,8 +117,11 @@ PipelineHandlerFactory::PipelineHandlerFactory(const char *name)\n>  /**\n>   * \\fn PipelineHandlerFactory::create()\n>   * \\brief Create an instance of the PipelineHandler corresponding to the factory\n> + * \\param[in] manager The camera manager\n>   *\n> - * This virtual function is implemented by the REGISTER_PIPELINE_HANDLER() macro.\n> + * This virtual function is implemented by the REGISTER_PIPELINE_HANDLER()\n> + * macro. It creates a pipeline handler instance associated with the camera\n> + * \\a manager.\n>   *\n>   * \\return a pointer to a newly constructed instance of the PipelineHandler\n>   * subclass corresponding to the factory\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":"<niklas.soderlund@ragnatech.se>","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 E71A760C82\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu, 24 Jan 2019 17:38:02 +0100 (CET)","by mail-lf1-x144.google.com with SMTP id b20so4771348lfa.12\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu, 24 Jan 2019 08:38: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\te14-v6sm1058191ljl.43.2019.01.24.08.38.01\n\t(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);\n\tThu, 24 Jan 2019 08:38:01 -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=ScMzAa8qW4ch9SvTE1cRkX6sH7MdF1f7DVHlFCnCd7E=;\n\tb=OWoAzA1Hov9WhJcDf6BTZBJiE53zZfx3yPhEPE4AIAKAJhv/oiru3UcdeowVEOL5C1\n\t+twtS+ctrtJF9cD16HsYqCCyVl/4w4qH+Qyxo5eqX4t3az9qwjx57oGUtT+x1KXD4bFr\n\tpONjNWFD0Ti/8iIwaMsrV0b/xG8U4/rHahrQHCM6Z25TJOZVId7am3wsZN1EfdGCziaF\n\tsLJ4E90oSAoOCDYTb4Z5uV8hLQ1hRGv6+YrnFyT6K6BRxJSnAhS7YZWPnYFcwLqBb3nm\n\tZb2hp7FFTOZ8PbaWHp+EUXYzHMYEsgPTeOdQM9/LwFu0IAbJELwrCjbYUT6/IEXXfTRD\n\txu/w==","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=ScMzAa8qW4ch9SvTE1cRkX6sH7MdF1f7DVHlFCnCd7E=;\n\tb=izeR4ANK+yRkHs8swIPGqut4FkRpiWAtO9AeWSF4UF3bssK/CbbUDWKyCKx54WiLb+\n\trbqy1PjLNzeQE8ucrgL+gMg2NrVFw6MxKBqjCTiiNfj/ZoOSwdnTo9ChGUksjN8DTId7\n\tMtvUXhaE3jciKjqoksRRdAOr75ZOXcQtkn82uX7pOQk35XjZ1GJREGMGlF68VyBjzQj1\n\tRQXo58qZfSrdaI25qEycdIGTvam62+HECNcQYSwp38Yyek1ZrviJwOVeUz3/oiNLJHpd\n\ty4quMTz4TTgUVGImcfUfe+7BasfoYHukoJgFcl+/RdNntxspDzWJmWqtNnBdvXq+O4Ww\n\tpl3Q==","X-Gm-Message-State":"AJcUukdcHfsMixgrFi2KNp/9EYd3L8Zda8Pq/aNelNEoN6h5OV7kptnl\n\tm9U7Z2PMl6bbG6xfT2GOBdclTw==","X-Google-Smtp-Source":"ALg8bN474ZMTOPjycREUhXauMzNRfXsKDcpzkl39TnNLT/crJbK+BKJ8zL7v0MB8avc7zCOjksafTw==","X-Received":"by 2002:a19:5d42:: with SMTP id p2mr5658952lfj.83.1548347881873; \n\tThu, 24 Jan 2019 08:38:01 -0800 (PST)","Date":"Thu, 24 Jan 2019 17:38:00 +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":"<20190124163800.GV4127@bigcity.dyn.berto.se>","References":"<20190124101651.9993-1-laurent.pinchart@ideasonboard.com>\n\t<20190124101651.9993-2-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":"<20190124101651.9993-2-laurent.pinchart@ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 01/10] libcamera: pipeline_handler:\n\tStore the camera manager pointer","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":"Thu, 24 Jan 2019 16:38:03 -0000"}}]