[{"id":12615,"web_url":"https://patchwork.libcamera.org/comment/12615/","msgid":"<20200921095358.GL2692107@oden.dyn.berto.se>","date":"2020-09-21T09:53:58","subject":"Re: [libcamera-devel] [PATCH 4/4] libcamera: camera: Inherit from\n\tExtensible","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 2020-09-21 06:10:57 +0300, Laurent Pinchart wrote:\n> Use the d-pointer infrastructure offered by the Extensible class to\n> replace the custom implementation.\n> \n> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n\nReviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n\n> ---\n>  include/libcamera/camera.h |   9 +--\n>  src/libcamera/camera.cpp   | 123 ++++++++++++++++++++++---------------\n>  2 files changed, 80 insertions(+), 52 deletions(-)\n> \n> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> index 272c12c3c473..77a4402cc766 100644\n> --- a/include/libcamera/camera.h\n> +++ b/include/libcamera/camera.h\n> @@ -13,6 +13,7 @@\n>  #include <string>\n>  \n>  #include <libcamera/controls.h>\n> +#include <libcamera/extensible.h>\n>  #include <libcamera/object.h>\n>  #include <libcamera/request.h>\n>  #include <libcamera/signal.h>\n> @@ -67,8 +68,11 @@ protected:\n>  \tstd::vector<StreamConfiguration> config_;\n>  };\n>  \n> -class Camera final : public Object, public std::enable_shared_from_this<Camera>\n> +class Camera final : public Object, public std::enable_shared_from_this<Camera>,\n> +\t\t     public Extensible\n>  {\n> +\tLIBCAMERA_DECLARE_PRIVATE(Camera)\n> +\n>  public:\n>  \tstatic std::shared_ptr<Camera> create(PipelineHandler *pipe,\n>  \t\t\t\t\t      const std::string &id,\n> @@ -104,9 +108,6 @@ private:\n>  \t       const std::set<Stream *> &streams);\n>  \t~Camera();\n>  \n> -\tclass Private;\n> -\tstd::unique_ptr<Private> p_;\n> -\n>  \tfriend class PipelineHandler;\n>  \tvoid disconnect();\n>  \tvoid requestComplete(Request *request);\n> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> index ae16a64a3b44..4141d63e606c 100644\n> --- a/src/libcamera/camera.cpp\n> +++ b/src/libcamera/camera.cpp\n> @@ -256,8 +256,10 @@ std::size_t CameraConfiguration::size() const\n>   * \\brief The vector of stream configurations\n>   */\n>  \n> -class Camera::Private\n> +class Camera::Private : public Extensible::Private\n>  {\n> +\tLIBCAMERA_DECLARE_PUBLIC(Camera)\n> +\n>  public:\n>  \tenum State {\n>  \t\tCameraAvailable,\n> @@ -266,7 +268,7 @@ public:\n>  \t\tCameraRunning,\n>  \t};\n>  \n> -\tPrivate(PipelineHandler *pipe, const std::string &id,\n> +\tPrivate(Camera *camera, PipelineHandler *pipe, const std::string &id,\n>  \t\tconst std::set<Stream *> &streams);\n>  \t~Private();\n>  \n> @@ -287,10 +289,11 @@ private:\n>  \tstd::atomic<State> state_;\n>  };\n>  \n> -Camera::Private::Private(PipelineHandler *pipe, const std::string &id,\n> +Camera::Private::Private(Camera *camera, PipelineHandler *pipe,\n> +\t\t\t const std::string &id,\n>  \t\t\t const std::set<Stream *> &streams)\n> -\t: pipe_(pipe->shared_from_this()), id_(id), streams_(streams),\n> -\t  disconnected_(false), state_(CameraAvailable)\n> +\t: Extensible::Private(camera), pipe_(pipe->shared_from_this()), id_(id),\n> +\t  streams_(streams), disconnected_(false), state_(CameraAvailable)\n>  {\n>  }\n>  \n> @@ -505,7 +508,8 @@ std::shared_ptr<Camera> Camera::create(PipelineHandler *pipe,\n>   */\n>  const std::string &Camera::id() const\n>  {\n> -\treturn p_->id_;\n> +\tLIBCAMERA_D_PTR(Camera);\n> +\treturn d->id_;\n>  }\n>  \n>  /**\n> @@ -533,7 +537,7 @@ const std::string &Camera::id() const\n>  \n>  Camera::Camera(PipelineHandler *pipe, const std::string &id,\n>  \t       const std::set<Stream *> &streams)\n> -\t: p_(new Private(pipe, id, streams))\n> +\t: Extensible(new Private(this, pipe, id, streams))\n>  {\n>  }\n>  \n> @@ -555,28 +559,32 @@ Camera::~Camera()\n>   */\n>  void Camera::disconnect()\n>  {\n> +\tLIBCAMERA_D_PTR(Camera);\n> +\n>  \tLOG(Camera, Debug) << \"Disconnecting camera \" << id();\n>  \n> -\tp_->disconnect();\n> +\td->disconnect();\n>  \tdisconnected.emit(this);\n>  }\n>  \n>  int Camera::exportFrameBuffers(Stream *stream,\n>  \t\t\t       std::vector<std::unique_ptr<FrameBuffer>> *buffers)\n>  {\n> -\tint ret = p_->isAccessAllowed(Private::CameraConfigured);\n> +\tLIBCAMERA_D_PTR(Camera);\n> +\n> +\tint ret = d->isAccessAllowed(Private::CameraConfigured);\n>  \tif (ret < 0)\n>  \t\treturn ret;\n>  \n>  \tif (streams().find(stream) == streams().end())\n>  \t\treturn -EINVAL;\n>  \n> -\tif (p_->activeStreams_.find(stream) == p_->activeStreams_.end())\n> +\tif (d->activeStreams_.find(stream) == d->activeStreams_.end())\n>  \t\treturn -EINVAL;\n>  \n> -\treturn p_->pipe_->invokeMethod(&PipelineHandler::exportFrameBuffers,\n> -\t\t\t\t       ConnectionTypeBlocking, this, stream,\n> -\t\t\t\t       buffers);\n> +\treturn d->pipe_->invokeMethod(&PipelineHandler::exportFrameBuffers,\n> +\t\t\t\t      ConnectionTypeBlocking, this, stream,\n> +\t\t\t\t      buffers);\n>  }\n>  \n>  /**\n> @@ -605,21 +613,23 @@ int Camera::exportFrameBuffers(Stream *stream,\n>   */\n>  int Camera::acquire()\n>  {\n> +\tLIBCAMERA_D_PTR(Camera);\n> +\n>  \t/*\n>  \t * No manual locking is required as PipelineHandler::lock() is\n>  \t * thread-safe.\n>  \t */\n> -\tint ret = p_->isAccessAllowed(Private::CameraAvailable);\n> +\tint ret = d->isAccessAllowed(Private::CameraAvailable);\n>  \tif (ret < 0)\n>  \t\treturn ret == -EACCES ? -EBUSY : ret;\n>  \n> -\tif (!p_->pipe_->lock()) {\n> +\tif (!d->pipe_->lock()) {\n>  \t\tLOG(Camera, Info)\n>  \t\t\t<< \"Pipeline handler in use by another process\";\n>  \t\treturn -EBUSY;\n>  \t}\n>  \n> -\tp_->setState(Private::CameraAcquired);\n> +\td->setState(Private::CameraAcquired);\n>  \n>  \treturn 0;\n>  }\n> @@ -640,14 +650,16 @@ int Camera::acquire()\n>   */\n>  int Camera::release()\n>  {\n> -\tint ret = p_->isAccessAllowed(Private::CameraAvailable,\n> -\t\t\t\t      Private::CameraConfigured, true);\n> +\tLIBCAMERA_D_PTR(Camera);\n> +\n> +\tint ret = d->isAccessAllowed(Private::CameraAvailable,\n> +\t\t\t\t     Private::CameraConfigured, true);\n>  \tif (ret < 0)\n>  \t\treturn ret == -EACCES ? -EBUSY : ret;\n>  \n> -\tp_->pipe_->unlock();\n> +\td->pipe_->unlock();\n>  \n> -\tp_->setState(Private::CameraAvailable);\n> +\td->setState(Private::CameraAvailable);\n>  \n>  \treturn 0;\n>  }\n> @@ -664,7 +676,8 @@ int Camera::release()\n>   */\n>  const ControlInfoMap &Camera::controls() const\n>  {\n> -\treturn p_->pipe_->controls(this);\n> +\tLIBCAMERA_D_PTR(Camera);\n> +\treturn d->pipe_->controls(this);\n>  }\n>  \n>  /**\n> @@ -677,7 +690,8 @@ const ControlInfoMap &Camera::controls() const\n>   */\n>  const ControlList &Camera::properties() const\n>  {\n> -\treturn p_->pipe_->properties(this);\n> +\tLIBCAMERA_D_PTR(Camera);\n> +\treturn d->pipe_->properties(this);\n>  }\n>  \n>  /**\n> @@ -693,7 +707,8 @@ const ControlList &Camera::properties() const\n>   */\n>  const std::set<Stream *> &Camera::streams() const\n>  {\n> -\treturn p_->streams_;\n> +\tLIBCAMERA_D_PTR(Camera);\n> +\treturn d->streams_;\n>  }\n>  \n>  /**\n> @@ -714,15 +729,17 @@ const std::set<Stream *> &Camera::streams() const\n>   */\n>  std::unique_ptr<CameraConfiguration> Camera::generateConfiguration(const StreamRoles &roles)\n>  {\n> -\tint ret = p_->isAccessAllowed(Private::CameraAvailable,\n> -\t\t\t\t      Private::CameraRunning);\n> +\tLIBCAMERA_D_PTR(Camera);\n> +\n> +\tint ret = d->isAccessAllowed(Private::CameraAvailable,\n> +\t\t\t\t     Private::CameraRunning);\n>  \tif (ret < 0)\n>  \t\treturn nullptr;\n>  \n>  \tif (roles.size() > streams().size())\n>  \t\treturn nullptr;\n>  \n> -\tCameraConfiguration *config = p_->pipe_->generateConfiguration(this, roles);\n> +\tCameraConfiguration *config = d->pipe_->generateConfiguration(this, roles);\n>  \tif (!config) {\n>  \t\tLOG(Camera, Debug)\n>  \t\t\t<< \"Pipeline handler failed to generate configuration\";\n> @@ -773,8 +790,10 @@ std::unique_ptr<CameraConfiguration> Camera::generateConfiguration(const StreamR\n>   */\n>  int Camera::configure(CameraConfiguration *config)\n>  {\n> -\tint ret = p_->isAccessAllowed(Private::CameraAcquired,\n> -\t\t\t\t      Private::CameraConfigured);\n> +\tLIBCAMERA_D_PTR(Camera);\n> +\n> +\tint ret = d->isAccessAllowed(Private::CameraAcquired,\n> +\t\t\t\t     Private::CameraConfigured);\n>  \tif (ret < 0)\n>  \t\treturn ret;\n>  \n> @@ -796,26 +815,26 @@ int Camera::configure(CameraConfiguration *config)\n>  \n>  \tLOG(Camera, Info) << msg.str();\n>  \n> -\tret = p_->pipe_->invokeMethod(&PipelineHandler::configure,\n> -\t\t\t\t      ConnectionTypeBlocking, this, config);\n> +\tret = d->pipe_->invokeMethod(&PipelineHandler::configure,\n> +\t\t\t\t     ConnectionTypeBlocking, this, config);\n>  \tif (ret)\n>  \t\treturn ret;\n>  \n> -\tp_->activeStreams_.clear();\n> +\td->activeStreams_.clear();\n>  \tfor (const StreamConfiguration &cfg : *config) {\n>  \t\tStream *stream = cfg.stream();\n>  \t\tif (!stream) {\n>  \t\t\tLOG(Camera, Fatal)\n>  \t\t\t\t<< \"Pipeline handler failed to update stream configuration\";\n> -\t\t\tp_->activeStreams_.clear();\n> +\t\t\td->activeStreams_.clear();\n>  \t\t\treturn -EINVAL;\n>  \t\t}\n>  \n>  \t\tstream->configuration_ = cfg;\n> -\t\tp_->activeStreams_.insert(stream);\n> +\t\td->activeStreams_.insert(stream);\n>  \t}\n>  \n> -\tp_->setState(Private::CameraConfigured);\n> +\td->setState(Private::CameraConfigured);\n>  \n>  \treturn 0;\n>  }\n> @@ -842,8 +861,10 @@ int Camera::configure(CameraConfiguration *config)\n>   */\n>  Request *Camera::createRequest(uint64_t cookie)\n>  {\n> -\tint ret = p_->isAccessAllowed(Private::CameraConfigured,\n> -\t\t\t\t      Private::CameraRunning);\n> +\tLIBCAMERA_D_PTR(Camera);\n> +\n> +\tint ret = d->isAccessAllowed(Private::CameraConfigured,\n> +\t\t\t\t     Private::CameraRunning);\n>  \tif (ret < 0)\n>  \t\treturn nullptr;\n>  \n> @@ -877,7 +898,9 @@ Request *Camera::createRequest(uint64_t cookie)\n>   */\n>  int Camera::queueRequest(Request *request)\n>  {\n> -\tint ret = p_->isAccessAllowed(Private::CameraRunning);\n> +\tLIBCAMERA_D_PTR(Camera);\n> +\n> +\tint ret = d->isAccessAllowed(Private::CameraRunning);\n>  \tif (ret < 0)\n>  \t\treturn ret;\n>  \n> @@ -895,14 +918,14 @@ int Camera::queueRequest(Request *request)\n>  \tfor (auto const &it : request->buffers()) {\n>  \t\tconst Stream *stream = it.first;\n>  \n> -\t\tif (p_->activeStreams_.find(stream) == p_->activeStreams_.end()) {\n> +\t\tif (d->activeStreams_.find(stream) == d->activeStreams_.end()) {\n>  \t\t\tLOG(Camera, Error) << \"Invalid request\";\n>  \t\t\treturn -EINVAL;\n>  \t\t}\n>  \t}\n>  \n> -\treturn p_->pipe_->invokeMethod(&PipelineHandler::queueRequest,\n> -\t\t\t\t       ConnectionTypeQueued, this, request);\n> +\treturn d->pipe_->invokeMethod(&PipelineHandler::queueRequest,\n> +\t\t\t\t      ConnectionTypeQueued, this, request);\n>  }\n>  \n>  /**\n> @@ -923,18 +946,20 @@ int Camera::queueRequest(Request *request)\n>   */\n>  int Camera::start()\n>  {\n> -\tint ret = p_->isAccessAllowed(Private::CameraConfigured);\n> +\tLIBCAMERA_D_PTR(Camera);\n> +\n> +\tint ret = d->isAccessAllowed(Private::CameraConfigured);\n>  \tif (ret < 0)\n>  \t\treturn ret;\n>  \n>  \tLOG(Camera, Debug) << \"Starting capture\";\n>  \n> -\tret = p_->pipe_->invokeMethod(&PipelineHandler::start,\n> -\t\t\t\t      ConnectionTypeBlocking, this);\n> +\tret = d->pipe_->invokeMethod(&PipelineHandler::start,\n> +\t\t\t\t     ConnectionTypeBlocking, this);\n>  \tif (ret)\n>  \t\treturn ret;\n>  \n> -\tp_->setState(Private::CameraRunning);\n> +\td->setState(Private::CameraRunning);\n>  \n>  \treturn 0;\n>  }\n> @@ -955,16 +980,18 @@ int Camera::start()\n>   */\n>  int Camera::stop()\n>  {\n> -\tint ret = p_->isAccessAllowed(Private::CameraRunning);\n> +\tLIBCAMERA_D_PTR(Camera);\n> +\n> +\tint ret = d->isAccessAllowed(Private::CameraRunning);\n>  \tif (ret < 0)\n>  \t\treturn ret;\n>  \n>  \tLOG(Camera, Debug) << \"Stopping capture\";\n>  \n> -\tp_->setState(Private::CameraConfigured);\n> +\td->setState(Private::CameraConfigured);\n>  \n> -\tp_->pipe_->invokeMethod(&PipelineHandler::stop, ConnectionTypeBlocking,\n> -\t\t\t\tthis);\n> +\td->pipe_->invokeMethod(&PipelineHandler::stop, ConnectionTypeBlocking,\n> +\t\t\t       this);\n>  \n>  \treturn 0;\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":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id BCE74C3B5B\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 21 Sep 2020 09:54:02 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2460262FBF;\n\tMon, 21 Sep 2020 11:54:02 +0200 (CEST)","from mail-lj1-x234.google.com (mail-lj1-x234.google.com\n\t[IPv6:2a00:1450:4864:20::234])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1B56E60367\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 21 Sep 2020 11:54:00 +0200 (CEST)","by mail-lj1-x234.google.com with SMTP id k25so10512047ljg.9\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 21 Sep 2020 02:54:00 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\tq18sm2456514lfr.138.2020.09.21.02.53.58\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tMon, 21 Sep 2020 02:53:58 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com\n\theader.b=\"um9uO0b2\"; dkim-atps=neutral","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=0WOoYadt5CMk8zbs1McHUocLju7btwEA3lF/2ejfxZU=;\n\tb=um9uO0b2FgrKSRT0TacDQdbasxSjbcwq9aZ58CV387DF26vD3AzXCDNDIIzvUnGiHC\n\taZFsR1B4ChvVpjIYnOd6mfuCW0GGscThGte6Rf2FpJ1sPBgq6Nx+xDXFe3xUVxHHl4Or\n\tT01+1vaCHhqzztXGVIcgs7dAuc14ChE/OM7jlSfO/UQMN9vpmKDgU4tcCQMD9ojglEbD\n\t4wyuqkvXIzcauGY82l4Yz7Amo/cq3csCzhJxXL67Ffvru8BLgtcUCCOWypRIWE4eOb/b\n\tRNQeG9hTyE+Rl+SBJ44PeHYAeNEWuZ+UTgDEaYZ0NeRNkc+Y0MBSc9Grnv1Ex3+xlHQQ\n\t1F5Q==","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=0WOoYadt5CMk8zbs1McHUocLju7btwEA3lF/2ejfxZU=;\n\tb=cYCAhIqlSX433lbmIh2sUshRrstaMiAR92r/lgbBnHhlg+b5LBnvsRTFnxLIWrKy81\n\tdA3AkF5u+eOVFz6RocJRtlgAEV9K0Nx3H0YYiSZrx53gvj4je6WhKOmENxrucvh804l9\n\tev+8QKUTW45ccrhKKjW/uIV/Y8XUTCyJchWcK3TTryvP/TcOXKAJGZMoF2RuMo+D7Fqi\n\tpIJRUEEdGvfB8rLEA0oqt2cVY1xNJtzedFLtjFR+drlkXiy8xgDmDF4O6lsjTItLXa41\n\tG2B4H662Fw3F56oNTzek+064MkWD2ZE6yLAmkDM0BIR9XFDvEoEB0/r38A1foPOcfXhb\n\tudUA==","X-Gm-Message-State":"AOAM530E/XjR04ej2jOkaNcUc7K+Z/yP8TFC4kGZzBzgORIFLkQw24oU\n\trjUg4mH2lZRpvfZUKa7QNjNB9k6oTbWXAQ==","X-Google-Smtp-Source":"ABdhPJwkplmKl8BM3romDwu5iB4k/1NtbWog0Xg/BrBmmdVHs8aaokBVySLDmTbePqGILgTNQA+mYQ==","X-Received":"by 2002:a2e:b0f8:: with SMTP id\n\th24mr14857223ljl.165.1600682039338; \n\tMon, 21 Sep 2020 02:53:59 -0700 (PDT)","Date":"Mon, 21 Sep 2020 11:53:58 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= <niklas.soderlund@ragnatech.se>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Message-ID":"<20200921095358.GL2692107@oden.dyn.berto.se>","References":"<20200921031057.3564-1-laurent.pinchart@ideasonboard.com>\n\t<20200921031057.3564-5-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200921031057.3564-5-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 4/4] libcamera: camera: Inherit from\n\tExtensible","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","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>","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"iso-8859-1\"","Content-Transfer-Encoding":"quoted-printable","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]