[{"id":3576,"web_url":"https://patchwork.libcamera.org/comment/3576/","msgid":"<20200122162102.GV1124294@oden.dyn.berto.se>","date":"2020-01-22T16:21:02","subject":"Re: [libcamera-devel] [PATCH 14/19] libcamera: camera: Move private\n\tdata 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 work.\n\nOn 2020-01-20 02:24:32 +0200, Laurent Pinchart wrote:\n> Use the pimpl paradigm ([1]) to hide the private data members from the\n> Camera class interface. This will ease maintaining ABI compatibility,\n> and prepares for the implementation of the Camera class threading model.\n> \n> The FrameBufferAllocator class accesses the Camera private data members\n> directly. In order to hide them, this pattern is replaced with new\n> private member functions in the Camera class, and the\n> FrameBufferAllocator is updated accordingly.\n> \n> [1] https://en.cppreference.com/w/cpp/language/pimpl\n> \n> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n\nWith s/pimpl/d-pointer/g,\n\nReviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n\n> ---\n>  include/libcamera/camera.h              |  28 +--\n>  src/libcamera/camera.cpp                | 224 +++++++++++++++---------\n>  src/libcamera/framebuffer_allocator.cpp |  47 ++---\n>  3 files changed, 161 insertions(+), 138 deletions(-)\n> \n> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> index 6597ade83288..c37319eda2dc 100644\n> --- a/include/libcamera/camera.h\n> +++ b/include/libcamera/camera.h\n> @@ -98,34 +98,22 @@ public:\n>  \tint stop();\n>  \n>  private:\n> -\tenum State {\n> -\t\tCameraAvailable,\n> -\t\tCameraAcquired,\n> -\t\tCameraConfigured,\n> -\t\tCameraRunning,\n> -\t};\n> -\n> -\tCamera(PipelineHandler *pipe, const std::string &name);\n> +\tCamera(PipelineHandler *pipe, const std::string &name,\n> +\t       const std::set<Stream *> &streams);\n>  \t~Camera();\n>  \n> -\tbool stateBetween(State low, State high) const;\n> -\tbool stateIs(State state) const;\n> +\tclass Private;\n> +\tstd::unique_ptr<Private> p_;\n>  \n>  \tfriend class PipelineHandler;\n>  \tvoid disconnect();\n> -\n>  \tvoid requestComplete(Request *request);\n>  \n> -\tstd::shared_ptr<PipelineHandler> pipe_;\n> -\tstd::string name_;\n> -\tstd::set<Stream *> streams_;\n> -\tstd::set<Stream *> activeStreams_;\n> -\n> -\tbool disconnected_;\n> -\tState state_;\n> -\n> -\t/* Needed to update allocator_ and to read state_ and activeStreams_. */\n>  \tfriend class FrameBufferAllocator;\n> +\tint exportFrameBuffers(Stream *stream,\n> +\t\t\t       std::vector<std::unique_ptr<FrameBuffer>> *buffers);\n> +\tint freeFrameBuffers(Stream *stream);\n> +\t/* \\todo Remove allocator_ from the exposed API */\n>  \tFrameBufferAllocator *allocator_;\n>  };\n>  \n> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> index 3385c08778b8..6fe802f375a3 100644\n> --- a/src/libcamera/camera.cpp\n> +++ b/src/libcamera/camera.cpp\n> @@ -254,6 +254,75 @@ std::size_t CameraConfiguration::size() const\n>   * \\brief The vector of stream configurations\n>   */\n>  \n> +class Camera::Private\n> +{\n> +public:\n> +\tenum State {\n> +\t\tCameraAvailable,\n> +\t\tCameraAcquired,\n> +\t\tCameraConfigured,\n> +\t\tCameraRunning,\n> +\t};\n> +\n> +\tPrivate(PipelineHandler *pipe, const std::string &name,\n> +\t\tconst std::set<Stream *> &streams);\n> +\n> +\tbool stateBetween(State low, State high) const;\n> +\tbool stateIs(State state) const;\n> +\n> +\tstd::shared_ptr<PipelineHandler> pipe_;\n> +\tstd::string name_;\n> +\tstd::set<Stream *> streams_;\n> +\tstd::set<Stream *> activeStreams_;\n> +\n> +\tbool disconnected_;\n> +\tState state_;\n> +};\n> +\n> +Camera::Private::Private(PipelineHandler *pipe, const std::string &name,\n> +\t\t\t const std::set<Stream *> &streams)\n> +\t: pipe_(pipe->shared_from_this()), name_(name), streams_(streams),\n> +\t  disconnected_(false), state_(CameraAvailable)\n> +{\n> +}\n> +\n> +static constexpr std::array<const char *, 4> camera_state_names = {\n> +\t\"Available\",\n> +\t\"Acquired\",\n> +\t\"Configured\",\n> +\t\"Running\",\n> +};\n> +\n> +bool Camera::Private::stateBetween(State low, State high) const\n> +{\n> +\tif (state_ >= low && state_ <= high)\n> +\t\treturn true;\n> +\n> +\tASSERT(static_cast<unsigned int>(low) < camera_state_names.size() &&\n> +\t       static_cast<unsigned int>(high) < camera_state_names.size());\n> +\n> +\tLOG(Camera, Debug) << \"Camera in \" << camera_state_names[state_]\n> +\t\t\t   << \" state trying operation requiring state between \"\n> +\t\t\t   << camera_state_names[low] << \" and \"\n> +\t\t\t   << camera_state_names[high];\n> +\n> +\treturn false;\n> +}\n> +\n> +bool Camera::Private::stateIs(State state) const\n> +{\n> +\tif (state_ == state)\n> +\t\treturn true;\n> +\n> +\tASSERT(static_cast<unsigned int>(state) < camera_state_names.size());\n> +\n> +\tLOG(Camera, Debug) << \"Camera in \" << camera_state_names[state_]\n> +\t\t\t   << \" state trying operation requiring state \"\n> +\t\t\t   << camera_state_names[state];\n> +\n> +\treturn false;\n> +}\n> +\n>  /**\n>   * \\class Camera\n>   * \\brief Camera device\n> @@ -354,8 +423,7 @@ std::shared_ptr<Camera> Camera::create(PipelineHandler *pipe,\n>  \t\t}\n>  \t};\n>  \n> -\tCamera *camera = new Camera(pipe, name);\n> -\tcamera->streams_ = streams;\n> +\tCamera *camera = new Camera(pipe, name, streams);\n>  \n>  \treturn std::shared_ptr<Camera>(camera, Deleter());\n>  }\n> @@ -366,7 +434,7 @@ std::shared_ptr<Camera> Camera::create(PipelineHandler *pipe,\n>   */\n>  const std::string &Camera::name() const\n>  {\n> -\treturn name_;\n> +\treturn p_->name_;\n>  }\n>  \n>  /**\n> @@ -392,55 +460,18 @@ const std::string &Camera::name() const\n>   * application API calls by returning errors immediately.\n>   */\n>  \n> -Camera::Camera(PipelineHandler *pipe, const std::string &name)\n> -\t: pipe_(pipe->shared_from_this()), name_(name), disconnected_(false),\n> -\t  state_(CameraAvailable), allocator_(nullptr)\n> +Camera::Camera(PipelineHandler *pipe, const std::string &name,\n> +\t       const std::set<Stream *> &streams)\n> +\t: p_(new Private(pipe, name, streams)), allocator_(nullptr)\n>  {\n>  }\n>  \n>  Camera::~Camera()\n>  {\n> -\tif (!stateIs(CameraAvailable))\n> +\tif (!p_->stateIs(Private::CameraAvailable))\n>  \t\tLOG(Camera, Error) << \"Removing camera while still in use\";\n>  }\n>  \n> -static constexpr std::array<const char *, 4> camera_state_names = {\n> -\t\"Available\",\n> -\t\"Acquired\",\n> -\t\"Configured\",\n> -\t\"Running\",\n> -};\n> -\n> -bool Camera::stateBetween(State low, State high) const\n> -{\n> -\tif (state_ >= low && state_ <= high)\n> -\t\treturn true;\n> -\n> -\tASSERT(static_cast<unsigned int>(low) < camera_state_names.size() &&\n> -\t       static_cast<unsigned int>(high) < camera_state_names.size());\n> -\n> -\tLOG(Camera, Debug) << \"Camera in \" << camera_state_names[state_]\n> -\t\t\t   << \" state trying operation requiring state between \"\n> -\t\t\t   << camera_state_names[low] << \" and \"\n> -\t\t\t   << camera_state_names[high];\n> -\n> -\treturn false;\n> -}\n> -\n> -bool Camera::stateIs(State state) const\n> -{\n> -\tif (state_ == state)\n> -\t\treturn true;\n> -\n> -\tASSERT(static_cast<unsigned int>(state) < camera_state_names.size());\n> -\n> -\tLOG(Camera, Debug) << \"Camera in \" << camera_state_names[state_]\n> -\t\t\t   << \" state trying operation requiring state \"\n> -\t\t\t   << camera_state_names[state];\n> -\n> -\treturn false;\n> -}\n> -\n>  /**\n>   * \\brief Notify camera disconnection\n>   *\n> @@ -455,20 +486,45 @@ bool Camera::stateIs(State state) const\n>   */\n>  void Camera::disconnect()\n>  {\n> -\tLOG(Camera, Debug) << \"Disconnecting camera \" << name_;\n> +\tLOG(Camera, Debug) << \"Disconnecting camera \" << name();\n>  \n>  \t/*\n>  \t * If the camera was running when the hardware was removed force the\n>  \t * state to Configured state to allow applications to free resources\n>  \t * and call release() before deleting the camera.\n>  \t */\n> -\tif (state_ == CameraRunning)\n> -\t\tstate_ = CameraConfigured;\n> +\tif (p_->state_ == Private::CameraRunning)\n> +\t\tp_->state_ = Private::CameraConfigured;\n>  \n> -\tdisconnected_ = true;\n> +\tp_->disconnected_ = true;\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> +\tif (!p_->stateIs(Private::CameraConfigured))\n> +\t\treturn -EACCES;\n> +\n> +\tif (streams().find(stream) == streams().end())\n> +\t\treturn -EINVAL;\n> +\n> +\tif (p_->activeStreams_.find(stream) == p_->activeStreams_.end())\n> +\t\treturn -EINVAL;\n> +\n> +\treturn p_->pipe_->exportFrameBuffers(this, stream, buffers);\n> +}\n> +\n> +int Camera::freeFrameBuffers(Stream *stream)\n> +{\n> +\tif (!p_->stateIs(Private::CameraConfigured))\n> +\t\treturn -EACCES;\n> +\n> +\tp_->pipe_->freeFrameBuffers(this, stream);\n> +\n> +\treturn 0;\n> +}\n> +\n>  /**\n>   * \\brief Acquire the camera device for exclusive access\n>   *\n> @@ -494,19 +550,19 @@ void Camera::disconnect()\n>   */\n>  int Camera::acquire()\n>  {\n> -\tif (disconnected_)\n> +\tif (p_->disconnected_)\n>  \t\treturn -ENODEV;\n>  \n> -\tif (!stateIs(CameraAvailable))\n> +\tif (!p_->stateIs(Private::CameraAvailable))\n>  \t\treturn -EBUSY;\n>  \n> -\tif (!pipe_->lock()) {\n> +\tif (!p_->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> -\tstate_ = CameraAcquired;\n> +\tp_->state_ = Private::CameraAcquired;\n>  \n>  \treturn 0;\n>  }\n> @@ -524,7 +580,8 @@ int Camera::acquire()\n>   */\n>  int Camera::release()\n>  {\n> -\tif (!stateBetween(CameraAvailable, CameraConfigured))\n> +\tif (!p_->stateBetween(Private::CameraAvailable,\n> +\t\t\t      Private::CameraConfigured))\n>  \t\treturn -EBUSY;\n>  \n>  \tif (allocator_) {\n> @@ -537,9 +594,9 @@ int Camera::release()\n>  \t\treturn -EBUSY;\n>  \t}\n>  \n> -\tpipe_->unlock();\n> +\tp_->pipe_->unlock();\n>  \n> -\tstate_ = CameraAvailable;\n> +\tp_->state_ = Private::CameraAvailable;\n>  \n>  \treturn 0;\n>  }\n> @@ -553,7 +610,7 @@ int Camera::release()\n>   */\n>  const ControlInfoMap &Camera::controls()\n>  {\n> -\treturn pipe_->controls(this);\n> +\treturn p_->pipe_->controls(this);\n>  }\n>  \n>  /**\n> @@ -567,7 +624,7 @@ const ControlInfoMap &Camera::controls()\n>   */\n>  const std::set<Stream *> &Camera::streams() const\n>  {\n> -\treturn streams_;\n> +\treturn p_->streams_;\n>  }\n>  \n>  /**\n> @@ -586,10 +643,10 @@ const std::set<Stream *> &Camera::streams() const\n>   */\n>  std::unique_ptr<CameraConfiguration> Camera::generateConfiguration(const StreamRoles &roles)\n>  {\n> -\tif (disconnected_ || roles.size() > streams_.size())\n> +\tif (p_->disconnected_ || roles.size() > streams().size())\n>  \t\treturn nullptr;\n>  \n> -\tCameraConfiguration *config = pipe_->generateConfiguration(this, roles);\n> +\tCameraConfiguration *config = p_->pipe_->generateConfiguration(this, roles);\n>  \tif (!config) {\n>  \t\tLOG(Camera, Debug)\n>  \t\t\t<< \"Pipeline handler failed to generate configuration\";\n> @@ -639,10 +696,11 @@ int Camera::configure(CameraConfiguration *config)\n>  {\n>  \tint ret;\n>  \n> -\tif (disconnected_)\n> +\tif (p_->disconnected_)\n>  \t\treturn -ENODEV;\n>  \n> -\tif (!stateBetween(CameraAcquired, CameraConfigured))\n> +\tif (!p_->stateBetween(Private::CameraAcquired,\n> +\t\t\t      Private::CameraConfigured))\n>  \t\treturn -EACCES;\n>  \n>  \tif (allocator_ && allocator_->allocated()) {\n> @@ -667,11 +725,11 @@ int Camera::configure(CameraConfiguration *config)\n>  \n>  \tLOG(Camera, Info) << msg.str();\n>  \n> -\tret = pipe_->configure(this, config);\n> +\tret = p_->pipe_->configure(this, config);\n>  \tif (ret)\n>  \t\treturn ret;\n>  \n> -\tactiveStreams_.clear();\n> +\tp_->activeStreams_.clear();\n>  \tfor (const StreamConfiguration &cfg : *config) {\n>  \t\tStream *stream = cfg.stream();\n>  \t\tif (!stream)\n> @@ -679,10 +737,10 @@ int Camera::configure(CameraConfiguration *config)\n>  \t\t\t\t<< \"Pipeline handler failed to update stream configuration\";\n>  \n>  \t\tstream->configuration_ = cfg;\n> -\t\tactiveStreams_.insert(stream);\n> +\t\tp_->activeStreams_.insert(stream);\n>  \t}\n>  \n> -\tstate_ = CameraConfigured;\n> +\tp_->state_ = Private::CameraConfigured;\n>  \n>  \treturn 0;\n>  }\n> @@ -709,7 +767,9 @@ int Camera::configure(CameraConfiguration *config)\n>   */\n>  Request *Camera::createRequest(uint64_t cookie)\n>  {\n> -\tif (disconnected_ || !stateBetween(CameraConfigured, CameraRunning))\n> +\tif (p_->disconnected_ ||\n> +\t    !p_->stateBetween(Private::CameraConfigured,\n> +\t\t\t      Private::CameraRunning))\n>  \t\treturn nullptr;\n>  \n>  \treturn new Request(this, cookie);\n> @@ -739,10 +799,10 @@ Request *Camera::createRequest(uint64_t cookie)\n>   */\n>  int Camera::queueRequest(Request *request)\n>  {\n> -\tif (disconnected_)\n> +\tif (p_->disconnected_)\n>  \t\treturn -ENODEV;\n>  \n> -\tif (!stateIs(CameraRunning))\n> +\tif (!p_->stateIs(Private::CameraRunning))\n>  \t\treturn -EACCES;\n>  \n>  \tif (request->buffers().empty()) {\n> @@ -753,13 +813,13 @@ int Camera::queueRequest(Request *request)\n>  \tfor (auto const &it : request->buffers()) {\n>  \t\tStream *stream = it.first;\n>  \n> -\t\tif (activeStreams_.find(stream) == activeStreams_.end()) {\n> +\t\tif (p_->activeStreams_.find(stream) == p_->activeStreams_.end()) {\n>  \t\t\tLOG(Camera, Error) << \"Invalid request\";\n>  \t\t\treturn -EINVAL;\n>  \t\t}\n>  \t}\n>  \n> -\treturn pipe_->queueRequest(this, request);\n> +\treturn p_->pipe_->queueRequest(this, request);\n>  }\n>  \n>  /**\n> @@ -777,26 +837,26 @@ int Camera::queueRequest(Request *request)\n>   */\n>  int Camera::start()\n>  {\n> -\tif (disconnected_)\n> +\tif (p_->disconnected_)\n>  \t\treturn -ENODEV;\n>  \n> -\tif (!stateIs(CameraConfigured))\n> +\tif (!p_->stateIs(Private::CameraConfigured))\n>  \t\treturn -EACCES;\n>  \n>  \tLOG(Camera, Debug) << \"Starting capture\";\n>  \n> -\tfor (Stream *stream : activeStreams_) {\n> +\tfor (Stream *stream : p_->activeStreams_) {\n>  \t\tif (allocator_ && !allocator_->buffers(stream).empty())\n>  \t\t\tcontinue;\n>  \n> -\t\tpipe_->importFrameBuffers(this, stream);\n> +\t\tp_->pipe_->importFrameBuffers(this, stream);\n>  \t}\n>  \n> -\tint ret = pipe_->start(this);\n> +\tint ret = p_->pipe_->start(this);\n>  \tif (ret)\n>  \t\treturn ret;\n>  \n> -\tstate_ = CameraRunning;\n> +\tp_->state_ = Private::CameraRunning;\n>  \n>  \treturn 0;\n>  }\n> @@ -815,23 +875,23 @@ int Camera::start()\n>   */\n>  int Camera::stop()\n>  {\n> -\tif (disconnected_)\n> +\tif (p_->disconnected_)\n>  \t\treturn -ENODEV;\n>  \n> -\tif (!stateIs(CameraRunning))\n> +\tif (!p_->stateIs(Private::CameraRunning))\n>  \t\treturn -EACCES;\n>  \n>  \tLOG(Camera, Debug) << \"Stopping capture\";\n>  \n> -\tstate_ = CameraConfigured;\n> +\tp_->state_ = Private::CameraConfigured;\n>  \n> -\tpipe_->stop(this);\n> +\tp_->pipe_->stop(this);\n>  \n> -\tfor (Stream *stream : activeStreams_) {\n> +\tfor (Stream *stream : p_->activeStreams_) {\n>  \t\tif (allocator_ && !allocator_->buffers(stream).empty())\n>  \t\t\tcontinue;\n>  \n> -\t\tpipe_->freeFrameBuffers(this, stream);\n> +\t\tp_->pipe_->freeFrameBuffers(this, stream);\n>  \t}\n>  \n>  \treturn 0;\n> diff --git a/src/libcamera/framebuffer_allocator.cpp b/src/libcamera/framebuffer_allocator.cpp\n> index a7588c7fe4c2..4f442d0d92e7 100644\n> --- a/src/libcamera/framebuffer_allocator.cpp\n> +++ b/src/libcamera/framebuffer_allocator.cpp\n> @@ -89,7 +89,7 @@ FrameBufferAllocator::~FrameBufferAllocator()\n>  {\n>  \tfor (auto &value : buffers_) {\n>  \t\tStream *stream = value.first;\n> -\t\tcamera_->pipe_->freeFrameBuffers(camera_.get(), stream);\n> +\t\tcamera_->freeFrameBuffers(stream);\n>  \t}\n>  \n>  \tbuffers_.clear();\n> @@ -116,36 +116,17 @@ FrameBufferAllocator::~FrameBufferAllocator()\n>   */\n>  int FrameBufferAllocator::allocate(Stream *stream)\n>  {\n> -\tif (camera_->state_ != Camera::CameraConfigured) {\n> -\t\tLOG(Allocator, Error)\n> -\t\t\t<< \"Camera must be in the configured state to allocate buffers\";\n> -\t\treturn -EACCES;\n> -\t}\n> -\n> -\tif (camera_->streams().find(stream) == camera_->streams().end()) {\n> -\t\tLOG(Allocator, Error)\n> -\t\t\t<< \"Stream does not belong to \" << camera_->name();\n> -\t\treturn -EINVAL;\n> -\t}\n> -\n> -\tif (camera_->activeStreams_.find(stream) == camera_->activeStreams_.end()) {\n> -\t\tLOG(Allocator, Error)\n> -\t\t\t<< \"Stream is not part of \" << camera_->name()\n> -\t\t\t<< \" active configuration\";\n> -\t\treturn -EINVAL;\n> -\t}\n> -\n>  \tif (buffers_.count(stream)) {\n>  \t\tLOG(Allocator, Error) << \"Buffers already allocated for stream\";\n>  \t\treturn -EBUSY;\n>  \t}\n>  \n> -\tint ret = camera_->pipe_->exportFrameBuffers(camera_.get(), stream,\n> -\t\t\t\t\t\t     &buffers_[stream]);\n> -\tif (ret)\n> -\t\treturn ret;\n> -\n> -\treturn 0;\n> +\tint ret = camera_->exportFrameBuffers(stream, &buffers_[stream]);\n> +\tif (ret == -EINVAL)\n> +\t\tLOG(Allocator, Error)\n> +\t\t\t<< \"Stream is not part of \" << camera_->name()\n> +\t\t\t<< \" active configuration\";\n> +\treturn ret;\n>  }\n>  \n>  /**\n> @@ -162,22 +143,16 @@ int FrameBufferAllocator::allocate(Stream *stream)\n>   */\n>  int FrameBufferAllocator::free(Stream *stream)\n>  {\n> -\tif (camera_->state_ != Camera::CameraConfigured) {\n> -\t\tLOG(Allocator, Error)\n> -\t\t\t<< \"Camera must be in the configured state to free buffers\";\n> -\t\treturn -EACCES;\n> -\t}\n> -\n>  \tauto iter = buffers_.find(stream);\n>  \tif (iter == buffers_.end())\n>  \t\treturn -EINVAL;\n>  \n> -\tstd::vector<std::unique_ptr<FrameBuffer>> &buffers = iter->second;\n> +\tint ret = camera_->freeFrameBuffers(stream);\n> +\tif (ret < 0)\n> +\t\treturn ret;\n>  \n> +\tstd::vector<std::unique_ptr<FrameBuffer>> &buffers = iter->second;\n>  \tbuffers.clear();\n> -\n> -\tcamera_->pipe_->freeFrameBuffers(camera_.get(), stream);\n> -\n>  \tbuffers_.erase(iter);\n>  \n>  \treturn 0;\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-lj1-x233.google.com (mail-lj1-x233.google.com\n\t[IPv6:2a00:1450:4864:20::233])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DAB6960804\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 22 Jan 2020 17:21:04 +0100 (CET)","by mail-lj1-x233.google.com with SMTP id o13so7491634ljg.4\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 22 Jan 2020 08:21:04 -0800 (PST)","from localhost (h-93-159.A463.priv.bahnhof.se. [46.59.93.159])\n\tby smtp.gmail.com with ESMTPSA id\n\t78sm20361378ljj.28.2020.01.22.08.21.03\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tWed, 22 Jan 2020 08:21:03 -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=lhhe9NaUo6OlfmWuDS0podsQq5PjOnaChuBv5q7PJY8=;\n\tb=CRvu60EmQGNqkCz7f1yaBI4RM7zppO+KnncHGJUZjE2j2Q4VA2b3T7kAVFGXf+Ow02\n\t0OP4hE9isJD/p9iGh7mFl07NTWdjjhtHeGIHKY5Flb2L/vCavHph8cq5PWGmKJdcwPB1\n\t59m2JWpFSYmXsJHize4PvSRW/c4HBzTskbd/Lu4GxA+BsaFW5FTyF0dr33q+TpPbnnLf\n\tpZ2SQN0Da5EILgvpLw4VsfHIduP1HmfWdmFR+6Sc0zKwKaNzFhKmJE8l+0VyUhVaTWMr\n\tuKxSUEXABSBMb8Jcv3AC5ki63nzLCiPPbnyZijesa0MPUgazP6L6kG9H9KxoDCoglryt\n\tBthw==","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=lhhe9NaUo6OlfmWuDS0podsQq5PjOnaChuBv5q7PJY8=;\n\tb=sbZoOy3LJQljfnpIot03yq08XhAM9UFU+iFyQDymusBGnXV9/eRvG5PhxdETkInMV3\n\tFtC+oCH8WPEBwUiHqYz5PKpbZ/GrN6von4sETCRJjcapD0gAAEw4Nf/m3GzjNKWMguBB\n\tdLKwikiz1ctUDutcA87ocXDiRAsqep3kR+4qzZ3vcH1R1cjuWKpnX+SfT5Tk7gMMh9Rx\n\tTccDZeLHi99pd2EBtBgaSx2xLho1HY6mwnCt/o2Ji1RRyw9wJ6dOA5lfy4Qu6R+ZPy4/\n\tcTnjjjj+Ab1XHNoojsZjZSWTQCOIRj2YUa3r2gcj6uz7vgM3Rl/HXAdc2CdeWXdBN60Q\n\t6D6w==","X-Gm-Message-State":"APjAAAVns8gRzBpYxV/vSfV0qEvd97YfBu6SuE0hLwU16JyB3oHT4+AY\n\t3fJscp9ukbspHJRH6RnQwS8+cXb7Fpc=","X-Google-Smtp-Source":"APXvYqwmcsyDEJM5CWot4ilrcWXBgFRi1hF6ILpebwAzuWwip7pg3v8JU+PyC6ah2qKk/c1eknFimg==","X-Received":"by 2002:a2e:9d89:: with SMTP id\n\tc9mr19860241ljj.129.1579710063937; \n\tWed, 22 Jan 2020 08:21:03 -0800 (PST)","Date":"Wed, 22 Jan 2020 17:21:02 +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":"<20200122162102.GV1124294@oden.dyn.berto.se>","References":"<20200120002437.6633-1-laurent.pinchart@ideasonboard.com>\n\t<20200120002437.6633-15-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":"<20200120002437.6633-15-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 14/19] libcamera: camera: Move private\n\tdata members to private implementation","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>","X-List-Received-Date":"Wed, 22 Jan 2020 16:21:05 -0000"}},{"id":3582,"web_url":"https://patchwork.libcamera.org/comment/3582/","msgid":"<20200122190423.GE15239@pendragon.ideasonboard.com>","date":"2020-01-22T19:04:23","subject":"Re: [libcamera-devel] [PATCH 14/19] libcamera: camera: Move private\n\tdata 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 Niklas,\n\nOn Wed, Jan 22, 2020 at 05:21:02PM +0100, Niklas Söderlund wrote:\n> On 2020-01-20 02:24:32 +0200, Laurent Pinchart wrote:\n> > Use the pimpl paradigm ([1]) to hide the private data members from the\n> > Camera class interface. This will ease maintaining ABI compatibility,\n> > and prepares for the implementation of the Camera class threading model.\n> > \n> > The FrameBufferAllocator class accesses the Camera private data members\n> > directly. In order to hide them, this pattern is replaced with new\n> > private member functions in the Camera class, and the\n> > FrameBufferAllocator is updated accordingly.\n> > \n> > [1] https://en.cppreference.com/w/cpp/language/pimpl\n> > \n> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> \n> With s/pimpl/d-pointer/g,\n\nI'll then add a link to https://wiki.qt.io/D-Pointer as [1] doesn't\nmention d-pointers.\n\n> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n> \n> > ---\n> >  include/libcamera/camera.h              |  28 +--\n> >  src/libcamera/camera.cpp                | 224 +++++++++++++++---------\n> >  src/libcamera/framebuffer_allocator.cpp |  47 ++---\n> >  3 files changed, 161 insertions(+), 138 deletions(-)\n> > \n> > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> > index 6597ade83288..c37319eda2dc 100644\n> > --- a/include/libcamera/camera.h\n> > +++ b/include/libcamera/camera.h\n> > @@ -98,34 +98,22 @@ public:\n> >  \tint stop();\n> >  \n> >  private:\n> > -\tenum State {\n> > -\t\tCameraAvailable,\n> > -\t\tCameraAcquired,\n> > -\t\tCameraConfigured,\n> > -\t\tCameraRunning,\n> > -\t};\n> > -\n> > -\tCamera(PipelineHandler *pipe, const std::string &name);\n> > +\tCamera(PipelineHandler *pipe, const std::string &name,\n> > +\t       const std::set<Stream *> &streams);\n> >  \t~Camera();\n> >  \n> > -\tbool stateBetween(State low, State high) const;\n> > -\tbool stateIs(State state) const;\n> > +\tclass Private;\n> > +\tstd::unique_ptr<Private> p_;\n> >  \n> >  \tfriend class PipelineHandler;\n> >  \tvoid disconnect();\n> > -\n> >  \tvoid requestComplete(Request *request);\n> >  \n> > -\tstd::shared_ptr<PipelineHandler> pipe_;\n> > -\tstd::string name_;\n> > -\tstd::set<Stream *> streams_;\n> > -\tstd::set<Stream *> activeStreams_;\n> > -\n> > -\tbool disconnected_;\n> > -\tState state_;\n> > -\n> > -\t/* Needed to update allocator_ and to read state_ and activeStreams_. */\n> >  \tfriend class FrameBufferAllocator;\n> > +\tint exportFrameBuffers(Stream *stream,\n> > +\t\t\t       std::vector<std::unique_ptr<FrameBuffer>> *buffers);\n> > +\tint freeFrameBuffers(Stream *stream);\n> > +\t/* \\todo Remove allocator_ from the exposed API */\n> >  \tFrameBufferAllocator *allocator_;\n> >  };\n> >  \n> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > index 3385c08778b8..6fe802f375a3 100644\n> > --- a/src/libcamera/camera.cpp\n> > +++ b/src/libcamera/camera.cpp\n> > @@ -254,6 +254,75 @@ std::size_t CameraConfiguration::size() const\n> >   * \\brief The vector of stream configurations\n> >   */\n> >  \n> > +class Camera::Private\n> > +{\n> > +public:\n> > +\tenum State {\n> > +\t\tCameraAvailable,\n> > +\t\tCameraAcquired,\n> > +\t\tCameraConfigured,\n> > +\t\tCameraRunning,\n> > +\t};\n> > +\n> > +\tPrivate(PipelineHandler *pipe, const std::string &name,\n> > +\t\tconst std::set<Stream *> &streams);\n> > +\n> > +\tbool stateBetween(State low, State high) const;\n> > +\tbool stateIs(State state) const;\n> > +\n> > +\tstd::shared_ptr<PipelineHandler> pipe_;\n> > +\tstd::string name_;\n> > +\tstd::set<Stream *> streams_;\n> > +\tstd::set<Stream *> activeStreams_;\n> > +\n> > +\tbool disconnected_;\n> > +\tState state_;\n> > +};\n> > +\n> > +Camera::Private::Private(PipelineHandler *pipe, const std::string &name,\n> > +\t\t\t const std::set<Stream *> &streams)\n> > +\t: pipe_(pipe->shared_from_this()), name_(name), streams_(streams),\n> > +\t  disconnected_(false), state_(CameraAvailable)\n> > +{\n> > +}\n> > +\n> > +static constexpr std::array<const char *, 4> camera_state_names = {\n> > +\t\"Available\",\n> > +\t\"Acquired\",\n> > +\t\"Configured\",\n> > +\t\"Running\",\n> > +};\n> > +\n> > +bool Camera::Private::stateBetween(State low, State high) const\n> > +{\n> > +\tif (state_ >= low && state_ <= high)\n> > +\t\treturn true;\n> > +\n> > +\tASSERT(static_cast<unsigned int>(low) < camera_state_names.size() &&\n> > +\t       static_cast<unsigned int>(high) < camera_state_names.size());\n> > +\n> > +\tLOG(Camera, Debug) << \"Camera in \" << camera_state_names[state_]\n> > +\t\t\t   << \" state trying operation requiring state between \"\n> > +\t\t\t   << camera_state_names[low] << \" and \"\n> > +\t\t\t   << camera_state_names[high];\n> > +\n> > +\treturn false;\n> > +}\n> > +\n> > +bool Camera::Private::stateIs(State state) const\n> > +{\n> > +\tif (state_ == state)\n> > +\t\treturn true;\n> > +\n> > +\tASSERT(static_cast<unsigned int>(state) < camera_state_names.size());\n> > +\n> > +\tLOG(Camera, Debug) << \"Camera in \" << camera_state_names[state_]\n> > +\t\t\t   << \" state trying operation requiring state \"\n> > +\t\t\t   << camera_state_names[state];\n> > +\n> > +\treturn false;\n> > +}\n> > +\n> >  /**\n> >   * \\class Camera\n> >   * \\brief Camera device\n> > @@ -354,8 +423,7 @@ std::shared_ptr<Camera> Camera::create(PipelineHandler *pipe,\n> >  \t\t}\n> >  \t};\n> >  \n> > -\tCamera *camera = new Camera(pipe, name);\n> > -\tcamera->streams_ = streams;\n> > +\tCamera *camera = new Camera(pipe, name, streams);\n> >  \n> >  \treturn std::shared_ptr<Camera>(camera, Deleter());\n> >  }\n> > @@ -366,7 +434,7 @@ std::shared_ptr<Camera> Camera::create(PipelineHandler *pipe,\n> >   */\n> >  const std::string &Camera::name() const\n> >  {\n> > -\treturn name_;\n> > +\treturn p_->name_;\n> >  }\n> >  \n> >  /**\n> > @@ -392,55 +460,18 @@ const std::string &Camera::name() const\n> >   * application API calls by returning errors immediately.\n> >   */\n> >  \n> > -Camera::Camera(PipelineHandler *pipe, const std::string &name)\n> > -\t: pipe_(pipe->shared_from_this()), name_(name), disconnected_(false),\n> > -\t  state_(CameraAvailable), allocator_(nullptr)\n> > +Camera::Camera(PipelineHandler *pipe, const std::string &name,\n> > +\t       const std::set<Stream *> &streams)\n> > +\t: p_(new Private(pipe, name, streams)), allocator_(nullptr)\n> >  {\n> >  }\n> >  \n> >  Camera::~Camera()\n> >  {\n> > -\tif (!stateIs(CameraAvailable))\n> > +\tif (!p_->stateIs(Private::CameraAvailable))\n> >  \t\tLOG(Camera, Error) << \"Removing camera while still in use\";\n> >  }\n> >  \n> > -static constexpr std::array<const char *, 4> camera_state_names = {\n> > -\t\"Available\",\n> > -\t\"Acquired\",\n> > -\t\"Configured\",\n> > -\t\"Running\",\n> > -};\n> > -\n> > -bool Camera::stateBetween(State low, State high) const\n> > -{\n> > -\tif (state_ >= low && state_ <= high)\n> > -\t\treturn true;\n> > -\n> > -\tASSERT(static_cast<unsigned int>(low) < camera_state_names.size() &&\n> > -\t       static_cast<unsigned int>(high) < camera_state_names.size());\n> > -\n> > -\tLOG(Camera, Debug) << \"Camera in \" << camera_state_names[state_]\n> > -\t\t\t   << \" state trying operation requiring state between \"\n> > -\t\t\t   << camera_state_names[low] << \" and \"\n> > -\t\t\t   << camera_state_names[high];\n> > -\n> > -\treturn false;\n> > -}\n> > -\n> > -bool Camera::stateIs(State state) const\n> > -{\n> > -\tif (state_ == state)\n> > -\t\treturn true;\n> > -\n> > -\tASSERT(static_cast<unsigned int>(state) < camera_state_names.size());\n> > -\n> > -\tLOG(Camera, Debug) << \"Camera in \" << camera_state_names[state_]\n> > -\t\t\t   << \" state trying operation requiring state \"\n> > -\t\t\t   << camera_state_names[state];\n> > -\n> > -\treturn false;\n> > -}\n> > -\n> >  /**\n> >   * \\brief Notify camera disconnection\n> >   *\n> > @@ -455,20 +486,45 @@ bool Camera::stateIs(State state) const\n> >   */\n> >  void Camera::disconnect()\n> >  {\n> > -\tLOG(Camera, Debug) << \"Disconnecting camera \" << name_;\n> > +\tLOG(Camera, Debug) << \"Disconnecting camera \" << name();\n> >  \n> >  \t/*\n> >  \t * If the camera was running when the hardware was removed force the\n> >  \t * state to Configured state to allow applications to free resources\n> >  \t * and call release() before deleting the camera.\n> >  \t */\n> > -\tif (state_ == CameraRunning)\n> > -\t\tstate_ = CameraConfigured;\n> > +\tif (p_->state_ == Private::CameraRunning)\n> > +\t\tp_->state_ = Private::CameraConfigured;\n> >  \n> > -\tdisconnected_ = true;\n> > +\tp_->disconnected_ = true;\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> > +\tif (!p_->stateIs(Private::CameraConfigured))\n> > +\t\treturn -EACCES;\n> > +\n> > +\tif (streams().find(stream) == streams().end())\n> > +\t\treturn -EINVAL;\n> > +\n> > +\tif (p_->activeStreams_.find(stream) == p_->activeStreams_.end())\n> > +\t\treturn -EINVAL;\n> > +\n> > +\treturn p_->pipe_->exportFrameBuffers(this, stream, buffers);\n> > +}\n> > +\n> > +int Camera::freeFrameBuffers(Stream *stream)\n> > +{\n> > +\tif (!p_->stateIs(Private::CameraConfigured))\n> > +\t\treturn -EACCES;\n> > +\n> > +\tp_->pipe_->freeFrameBuffers(this, stream);\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> >  /**\n> >   * \\brief Acquire the camera device for exclusive access\n> >   *\n> > @@ -494,19 +550,19 @@ void Camera::disconnect()\n> >   */\n> >  int Camera::acquire()\n> >  {\n> > -\tif (disconnected_)\n> > +\tif (p_->disconnected_)\n> >  \t\treturn -ENODEV;\n> >  \n> > -\tif (!stateIs(CameraAvailable))\n> > +\tif (!p_->stateIs(Private::CameraAvailable))\n> >  \t\treturn -EBUSY;\n> >  \n> > -\tif (!pipe_->lock()) {\n> > +\tif (!p_->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> > -\tstate_ = CameraAcquired;\n> > +\tp_->state_ = Private::CameraAcquired;\n> >  \n> >  \treturn 0;\n> >  }\n> > @@ -524,7 +580,8 @@ int Camera::acquire()\n> >   */\n> >  int Camera::release()\n> >  {\n> > -\tif (!stateBetween(CameraAvailable, CameraConfigured))\n> > +\tif (!p_->stateBetween(Private::CameraAvailable,\n> > +\t\t\t      Private::CameraConfigured))\n> >  \t\treturn -EBUSY;\n> >  \n> >  \tif (allocator_) {\n> > @@ -537,9 +594,9 @@ int Camera::release()\n> >  \t\treturn -EBUSY;\n> >  \t}\n> >  \n> > -\tpipe_->unlock();\n> > +\tp_->pipe_->unlock();\n> >  \n> > -\tstate_ = CameraAvailable;\n> > +\tp_->state_ = Private::CameraAvailable;\n> >  \n> >  \treturn 0;\n> >  }\n> > @@ -553,7 +610,7 @@ int Camera::release()\n> >   */\n> >  const ControlInfoMap &Camera::controls()\n> >  {\n> > -\treturn pipe_->controls(this);\n> > +\treturn p_->pipe_->controls(this);\n> >  }\n> >  \n> >  /**\n> > @@ -567,7 +624,7 @@ const ControlInfoMap &Camera::controls()\n> >   */\n> >  const std::set<Stream *> &Camera::streams() const\n> >  {\n> > -\treturn streams_;\n> > +\treturn p_->streams_;\n> >  }\n> >  \n> >  /**\n> > @@ -586,10 +643,10 @@ const std::set<Stream *> &Camera::streams() const\n> >   */\n> >  std::unique_ptr<CameraConfiguration> Camera::generateConfiguration(const StreamRoles &roles)\n> >  {\n> > -\tif (disconnected_ || roles.size() > streams_.size())\n> > +\tif (p_->disconnected_ || roles.size() > streams().size())\n> >  \t\treturn nullptr;\n> >  \n> > -\tCameraConfiguration *config = pipe_->generateConfiguration(this, roles);\n> > +\tCameraConfiguration *config = p_->pipe_->generateConfiguration(this, roles);\n> >  \tif (!config) {\n> >  \t\tLOG(Camera, Debug)\n> >  \t\t\t<< \"Pipeline handler failed to generate configuration\";\n> > @@ -639,10 +696,11 @@ int Camera::configure(CameraConfiguration *config)\n> >  {\n> >  \tint ret;\n> >  \n> > -\tif (disconnected_)\n> > +\tif (p_->disconnected_)\n> >  \t\treturn -ENODEV;\n> >  \n> > -\tif (!stateBetween(CameraAcquired, CameraConfigured))\n> > +\tif (!p_->stateBetween(Private::CameraAcquired,\n> > +\t\t\t      Private::CameraConfigured))\n> >  \t\treturn -EACCES;\n> >  \n> >  \tif (allocator_ && allocator_->allocated()) {\n> > @@ -667,11 +725,11 @@ int Camera::configure(CameraConfiguration *config)\n> >  \n> >  \tLOG(Camera, Info) << msg.str();\n> >  \n> > -\tret = pipe_->configure(this, config);\n> > +\tret = p_->pipe_->configure(this, config);\n> >  \tif (ret)\n> >  \t\treturn ret;\n> >  \n> > -\tactiveStreams_.clear();\n> > +\tp_->activeStreams_.clear();\n> >  \tfor (const StreamConfiguration &cfg : *config) {\n> >  \t\tStream *stream = cfg.stream();\n> >  \t\tif (!stream)\n> > @@ -679,10 +737,10 @@ int Camera::configure(CameraConfiguration *config)\n> >  \t\t\t\t<< \"Pipeline handler failed to update stream configuration\";\n> >  \n> >  \t\tstream->configuration_ = cfg;\n> > -\t\tactiveStreams_.insert(stream);\n> > +\t\tp_->activeStreams_.insert(stream);\n> >  \t}\n> >  \n> > -\tstate_ = CameraConfigured;\n> > +\tp_->state_ = Private::CameraConfigured;\n> >  \n> >  \treturn 0;\n> >  }\n> > @@ -709,7 +767,9 @@ int Camera::configure(CameraConfiguration *config)\n> >   */\n> >  Request *Camera::createRequest(uint64_t cookie)\n> >  {\n> > -\tif (disconnected_ || !stateBetween(CameraConfigured, CameraRunning))\n> > +\tif (p_->disconnected_ ||\n> > +\t    !p_->stateBetween(Private::CameraConfigured,\n> > +\t\t\t      Private::CameraRunning))\n> >  \t\treturn nullptr;\n> >  \n> >  \treturn new Request(this, cookie);\n> > @@ -739,10 +799,10 @@ Request *Camera::createRequest(uint64_t cookie)\n> >   */\n> >  int Camera::queueRequest(Request *request)\n> >  {\n> > -\tif (disconnected_)\n> > +\tif (p_->disconnected_)\n> >  \t\treturn -ENODEV;\n> >  \n> > -\tif (!stateIs(CameraRunning))\n> > +\tif (!p_->stateIs(Private::CameraRunning))\n> >  \t\treturn -EACCES;\n> >  \n> >  \tif (request->buffers().empty()) {\n> > @@ -753,13 +813,13 @@ int Camera::queueRequest(Request *request)\n> >  \tfor (auto const &it : request->buffers()) {\n> >  \t\tStream *stream = it.first;\n> >  \n> > -\t\tif (activeStreams_.find(stream) == activeStreams_.end()) {\n> > +\t\tif (p_->activeStreams_.find(stream) == p_->activeStreams_.end()) {\n> >  \t\t\tLOG(Camera, Error) << \"Invalid request\";\n> >  \t\t\treturn -EINVAL;\n> >  \t\t}\n> >  \t}\n> >  \n> > -\treturn pipe_->queueRequest(this, request);\n> > +\treturn p_->pipe_->queueRequest(this, request);\n> >  }\n> >  \n> >  /**\n> > @@ -777,26 +837,26 @@ int Camera::queueRequest(Request *request)\n> >   */\n> >  int Camera::start()\n> >  {\n> > -\tif (disconnected_)\n> > +\tif (p_->disconnected_)\n> >  \t\treturn -ENODEV;\n> >  \n> > -\tif (!stateIs(CameraConfigured))\n> > +\tif (!p_->stateIs(Private::CameraConfigured))\n> >  \t\treturn -EACCES;\n> >  \n> >  \tLOG(Camera, Debug) << \"Starting capture\";\n> >  \n> > -\tfor (Stream *stream : activeStreams_) {\n> > +\tfor (Stream *stream : p_->activeStreams_) {\n> >  \t\tif (allocator_ && !allocator_->buffers(stream).empty())\n> >  \t\t\tcontinue;\n> >  \n> > -\t\tpipe_->importFrameBuffers(this, stream);\n> > +\t\tp_->pipe_->importFrameBuffers(this, stream);\n> >  \t}\n> >  \n> > -\tint ret = pipe_->start(this);\n> > +\tint ret = p_->pipe_->start(this);\n> >  \tif (ret)\n> >  \t\treturn ret;\n> >  \n> > -\tstate_ = CameraRunning;\n> > +\tp_->state_ = Private::CameraRunning;\n> >  \n> >  \treturn 0;\n> >  }\n> > @@ -815,23 +875,23 @@ int Camera::start()\n> >   */\n> >  int Camera::stop()\n> >  {\n> > -\tif (disconnected_)\n> > +\tif (p_->disconnected_)\n> >  \t\treturn -ENODEV;\n> >  \n> > -\tif (!stateIs(CameraRunning))\n> > +\tif (!p_->stateIs(Private::CameraRunning))\n> >  \t\treturn -EACCES;\n> >  \n> >  \tLOG(Camera, Debug) << \"Stopping capture\";\n> >  \n> > -\tstate_ = CameraConfigured;\n> > +\tp_->state_ = Private::CameraConfigured;\n> >  \n> > -\tpipe_->stop(this);\n> > +\tp_->pipe_->stop(this);\n> >  \n> > -\tfor (Stream *stream : activeStreams_) {\n> > +\tfor (Stream *stream : p_->activeStreams_) {\n> >  \t\tif (allocator_ && !allocator_->buffers(stream).empty())\n> >  \t\t\tcontinue;\n> >  \n> > -\t\tpipe_->freeFrameBuffers(this, stream);\n> > +\t\tp_->pipe_->freeFrameBuffers(this, stream);\n> >  \t}\n> >  \n> >  \treturn 0;\n> > diff --git a/src/libcamera/framebuffer_allocator.cpp b/src/libcamera/framebuffer_allocator.cpp\n> > index a7588c7fe4c2..4f442d0d92e7 100644\n> > --- a/src/libcamera/framebuffer_allocator.cpp\n> > +++ b/src/libcamera/framebuffer_allocator.cpp\n> > @@ -89,7 +89,7 @@ FrameBufferAllocator::~FrameBufferAllocator()\n> >  {\n> >  \tfor (auto &value : buffers_) {\n> >  \t\tStream *stream = value.first;\n> > -\t\tcamera_->pipe_->freeFrameBuffers(camera_.get(), stream);\n> > +\t\tcamera_->freeFrameBuffers(stream);\n> >  \t}\n> >  \n> >  \tbuffers_.clear();\n> > @@ -116,36 +116,17 @@ FrameBufferAllocator::~FrameBufferAllocator()\n> >   */\n> >  int FrameBufferAllocator::allocate(Stream *stream)\n> >  {\n> > -\tif (camera_->state_ != Camera::CameraConfigured) {\n> > -\t\tLOG(Allocator, Error)\n> > -\t\t\t<< \"Camera must be in the configured state to allocate buffers\";\n> > -\t\treturn -EACCES;\n> > -\t}\n> > -\n> > -\tif (camera_->streams().find(stream) == camera_->streams().end()) {\n> > -\t\tLOG(Allocator, Error)\n> > -\t\t\t<< \"Stream does not belong to \" << camera_->name();\n> > -\t\treturn -EINVAL;\n> > -\t}\n> > -\n> > -\tif (camera_->activeStreams_.find(stream) == camera_->activeStreams_.end()) {\n> > -\t\tLOG(Allocator, Error)\n> > -\t\t\t<< \"Stream is not part of \" << camera_->name()\n> > -\t\t\t<< \" active configuration\";\n> > -\t\treturn -EINVAL;\n> > -\t}\n> > -\n> >  \tif (buffers_.count(stream)) {\n> >  \t\tLOG(Allocator, Error) << \"Buffers already allocated for stream\";\n> >  \t\treturn -EBUSY;\n> >  \t}\n> >  \n> > -\tint ret = camera_->pipe_->exportFrameBuffers(camera_.get(), stream,\n> > -\t\t\t\t\t\t     &buffers_[stream]);\n> > -\tif (ret)\n> > -\t\treturn ret;\n> > -\n> > -\treturn 0;\n> > +\tint ret = camera_->exportFrameBuffers(stream, &buffers_[stream]);\n> > +\tif (ret == -EINVAL)\n> > +\t\tLOG(Allocator, Error)\n> > +\t\t\t<< \"Stream is not part of \" << camera_->name()\n> > +\t\t\t<< \" active configuration\";\n> > +\treturn ret;\n> >  }\n> >  \n> >  /**\n> > @@ -162,22 +143,16 @@ int FrameBufferAllocator::allocate(Stream *stream)\n> >   */\n> >  int FrameBufferAllocator::free(Stream *stream)\n> >  {\n> > -\tif (camera_->state_ != Camera::CameraConfigured) {\n> > -\t\tLOG(Allocator, Error)\n> > -\t\t\t<< \"Camera must be in the configured state to free buffers\";\n> > -\t\treturn -EACCES;\n> > -\t}\n> > -\n> >  \tauto iter = buffers_.find(stream);\n> >  \tif (iter == buffers_.end())\n> >  \t\treturn -EINVAL;\n> >  \n> > -\tstd::vector<std::unique_ptr<FrameBuffer>> &buffers = iter->second;\n> > +\tint ret = camera_->freeFrameBuffers(stream);\n> > +\tif (ret < 0)\n> > +\t\treturn ret;\n> >  \n> > +\tstd::vector<std::unique_ptr<FrameBuffer>> &buffers = iter->second;\n> >  \tbuffers.clear();\n> > -\n> > -\tcamera_->pipe_->freeFrameBuffers(camera_.get(), stream);\n> > -\n> >  \tbuffers_.erase(iter);\n> >  \n> >  \treturn 0;","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 42C3260804\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 22 Jan 2020 20:04:39 +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 A22B52E5;\n\tWed, 22 Jan 2020 20:04:38 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1579719878;\n\tbh=kbrHvcpPMRPg6RYHC3BUTR0q80CdmXsd9UV/pIrgZpk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=YXzGpC0Js2cfAYJRqlvA6K39Cz/RVn8os2yYK/LfIWR4Uy2znpakduqqHKZyfbWbT\n\tu/DOkYWtQPZFcN6La0PbACZKSTqDjfw09oL3xMrwqW403A5JcjBRzZvuWZaaTbSzUK\n\tDunc1Ch78PbaY44S73xVIVrSVo746TrG5yIaDAfY=","Date":"Wed, 22 Jan 2020 21:04:23 +0200","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Niklas =?utf-8?q?S=C3=B6derlund?= <niklas.soderlund@ragnatech.se>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200122190423.GE15239@pendragon.ideasonboard.com>","References":"<20200120002437.6633-1-laurent.pinchart@ideasonboard.com>\n\t<20200120002437.6633-15-laurent.pinchart@ideasonboard.com>\n\t<20200122162102.GV1124294@oden.dyn.berto.se>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20200122162102.GV1124294@oden.dyn.berto.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 14/19] libcamera: camera: Move private\n\tdata members to private implementation","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>","X-List-Received-Date":"Wed, 22 Jan 2020 19:04:39 -0000"}}]