[libcamera-devel] [PATCH 14/19] libcamera: camera: Move private data members to private implementation
Niklas Söderlund
niklas.soderlund at ragnatech.se
Wed Jan 22 17:21:02 CET 2020
Hi Laurent,
Thanks for your work.
On 2020-01-20 02:24:32 +0200, Laurent Pinchart wrote:
> Use the pimpl paradigm ([1]) to hide the private data members from the
> Camera class interface. This will ease maintaining ABI compatibility,
> and prepares for the implementation of the Camera class threading model.
>
> The FrameBufferAllocator class accesses the Camera private data members
> directly. In order to hide them, this pattern is replaced with new
> private member functions in the Camera class, and the
> FrameBufferAllocator is updated accordingly.
>
> [1] https://en.cppreference.com/w/cpp/language/pimpl
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
With s/pimpl/d-pointer/g,
Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
> include/libcamera/camera.h | 28 +--
> src/libcamera/camera.cpp | 224 +++++++++++++++---------
> src/libcamera/framebuffer_allocator.cpp | 47 ++---
> 3 files changed, 161 insertions(+), 138 deletions(-)
>
> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> index 6597ade83288..c37319eda2dc 100644
> --- a/include/libcamera/camera.h
> +++ b/include/libcamera/camera.h
> @@ -98,34 +98,22 @@ public:
> int stop();
>
> private:
> - enum State {
> - CameraAvailable,
> - CameraAcquired,
> - CameraConfigured,
> - CameraRunning,
> - };
> -
> - Camera(PipelineHandler *pipe, const std::string &name);
> + Camera(PipelineHandler *pipe, const std::string &name,
> + const std::set<Stream *> &streams);
> ~Camera();
>
> - bool stateBetween(State low, State high) const;
> - bool stateIs(State state) const;
> + class Private;
> + std::unique_ptr<Private> p_;
>
> friend class PipelineHandler;
> void disconnect();
> -
> void requestComplete(Request *request);
>
> - std::shared_ptr<PipelineHandler> pipe_;
> - std::string name_;
> - std::set<Stream *> streams_;
> - std::set<Stream *> activeStreams_;
> -
> - bool disconnected_;
> - State state_;
> -
> - /* Needed to update allocator_ and to read state_ and activeStreams_. */
> friend class FrameBufferAllocator;
> + int exportFrameBuffers(Stream *stream,
> + std::vector<std::unique_ptr<FrameBuffer>> *buffers);
> + int freeFrameBuffers(Stream *stream);
> + /* \todo Remove allocator_ from the exposed API */
> FrameBufferAllocator *allocator_;
> };
>
> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> index 3385c08778b8..6fe802f375a3 100644
> --- a/src/libcamera/camera.cpp
> +++ b/src/libcamera/camera.cpp
> @@ -254,6 +254,75 @@ std::size_t CameraConfiguration::size() const
> * \brief The vector of stream configurations
> */
>
> +class Camera::Private
> +{
> +public:
> + enum State {
> + CameraAvailable,
> + CameraAcquired,
> + CameraConfigured,
> + CameraRunning,
> + };
> +
> + Private(PipelineHandler *pipe, const std::string &name,
> + const std::set<Stream *> &streams);
> +
> + bool stateBetween(State low, State high) const;
> + bool stateIs(State state) const;
> +
> + std::shared_ptr<PipelineHandler> pipe_;
> + std::string name_;
> + std::set<Stream *> streams_;
> + std::set<Stream *> activeStreams_;
> +
> + bool disconnected_;
> + State state_;
> +};
> +
> +Camera::Private::Private(PipelineHandler *pipe, const std::string &name,
> + const std::set<Stream *> &streams)
> + : pipe_(pipe->shared_from_this()), name_(name), streams_(streams),
> + disconnected_(false), state_(CameraAvailable)
> +{
> +}
> +
> +static constexpr std::array<const char *, 4> camera_state_names = {
> + "Available",
> + "Acquired",
> + "Configured",
> + "Running",
> +};
> +
> +bool Camera::Private::stateBetween(State low, State high) const
> +{
> + if (state_ >= low && state_ <= high)
> + return true;
> +
> + ASSERT(static_cast<unsigned int>(low) < camera_state_names.size() &&
> + static_cast<unsigned int>(high) < camera_state_names.size());
> +
> + LOG(Camera, Debug) << "Camera in " << camera_state_names[state_]
> + << " state trying operation requiring state between "
> + << camera_state_names[low] << " and "
> + << camera_state_names[high];
> +
> + return false;
> +}
> +
> +bool Camera::Private::stateIs(State state) const
> +{
> + if (state_ == state)
> + return true;
> +
> + ASSERT(static_cast<unsigned int>(state) < camera_state_names.size());
> +
> + LOG(Camera, Debug) << "Camera in " << camera_state_names[state_]
> + << " state trying operation requiring state "
> + << camera_state_names[state];
> +
> + return false;
> +}
> +
> /**
> * \class Camera
> * \brief Camera device
> @@ -354,8 +423,7 @@ std::shared_ptr<Camera> Camera::create(PipelineHandler *pipe,
> }
> };
>
> - Camera *camera = new Camera(pipe, name);
> - camera->streams_ = streams;
> + Camera *camera = new Camera(pipe, name, streams);
>
> return std::shared_ptr<Camera>(camera, Deleter());
> }
> @@ -366,7 +434,7 @@ std::shared_ptr<Camera> Camera::create(PipelineHandler *pipe,
> */
> const std::string &Camera::name() const
> {
> - return name_;
> + return p_->name_;
> }
>
> /**
> @@ -392,55 +460,18 @@ const std::string &Camera::name() const
> * application API calls by returning errors immediately.
> */
>
> -Camera::Camera(PipelineHandler *pipe, const std::string &name)
> - : pipe_(pipe->shared_from_this()), name_(name), disconnected_(false),
> - state_(CameraAvailable), allocator_(nullptr)
> +Camera::Camera(PipelineHandler *pipe, const std::string &name,
> + const std::set<Stream *> &streams)
> + : p_(new Private(pipe, name, streams)), allocator_(nullptr)
> {
> }
>
> Camera::~Camera()
> {
> - if (!stateIs(CameraAvailable))
> + if (!p_->stateIs(Private::CameraAvailable))
> LOG(Camera, Error) << "Removing camera while still in use";
> }
>
> -static constexpr std::array<const char *, 4> camera_state_names = {
> - "Available",
> - "Acquired",
> - "Configured",
> - "Running",
> -};
> -
> -bool Camera::stateBetween(State low, State high) const
> -{
> - if (state_ >= low && state_ <= high)
> - return true;
> -
> - ASSERT(static_cast<unsigned int>(low) < camera_state_names.size() &&
> - static_cast<unsigned int>(high) < camera_state_names.size());
> -
> - LOG(Camera, Debug) << "Camera in " << camera_state_names[state_]
> - << " state trying operation requiring state between "
> - << camera_state_names[low] << " and "
> - << camera_state_names[high];
> -
> - return false;
> -}
> -
> -bool Camera::stateIs(State state) const
> -{
> - if (state_ == state)
> - return true;
> -
> - ASSERT(static_cast<unsigned int>(state) < camera_state_names.size());
> -
> - LOG(Camera, Debug) << "Camera in " << camera_state_names[state_]
> - << " state trying operation requiring state "
> - << camera_state_names[state];
> -
> - return false;
> -}
> -
> /**
> * \brief Notify camera disconnection
> *
> @@ -455,20 +486,45 @@ bool Camera::stateIs(State state) const
> */
> void Camera::disconnect()
> {
> - LOG(Camera, Debug) << "Disconnecting camera " << name_;
> + LOG(Camera, Debug) << "Disconnecting camera " << name();
>
> /*
> * If the camera was running when the hardware was removed force the
> * state to Configured state to allow applications to free resources
> * and call release() before deleting the camera.
> */
> - if (state_ == CameraRunning)
> - state_ = CameraConfigured;
> + if (p_->state_ == Private::CameraRunning)
> + p_->state_ = Private::CameraConfigured;
>
> - disconnected_ = true;
> + p_->disconnected_ = true;
> disconnected.emit(this);
> }
>
> +int Camera::exportFrameBuffers(Stream *stream,
> + std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> +{
> + if (!p_->stateIs(Private::CameraConfigured))
> + return -EACCES;
> +
> + if (streams().find(stream) == streams().end())
> + return -EINVAL;
> +
> + if (p_->activeStreams_.find(stream) == p_->activeStreams_.end())
> + return -EINVAL;
> +
> + return p_->pipe_->exportFrameBuffers(this, stream, buffers);
> +}
> +
> +int Camera::freeFrameBuffers(Stream *stream)
> +{
> + if (!p_->stateIs(Private::CameraConfigured))
> + return -EACCES;
> +
> + p_->pipe_->freeFrameBuffers(this, stream);
> +
> + return 0;
> +}
> +
> /**
> * \brief Acquire the camera device for exclusive access
> *
> @@ -494,19 +550,19 @@ void Camera::disconnect()
> */
> int Camera::acquire()
> {
> - if (disconnected_)
> + if (p_->disconnected_)
> return -ENODEV;
>
> - if (!stateIs(CameraAvailable))
> + if (!p_->stateIs(Private::CameraAvailable))
> return -EBUSY;
>
> - if (!pipe_->lock()) {
> + if (!p_->pipe_->lock()) {
> LOG(Camera, Info)
> << "Pipeline handler in use by another process";
> return -EBUSY;
> }
>
> - state_ = CameraAcquired;
> + p_->state_ = Private::CameraAcquired;
>
> return 0;
> }
> @@ -524,7 +580,8 @@ int Camera::acquire()
> */
> int Camera::release()
> {
> - if (!stateBetween(CameraAvailable, CameraConfigured))
> + if (!p_->stateBetween(Private::CameraAvailable,
> + Private::CameraConfigured))
> return -EBUSY;
>
> if (allocator_) {
> @@ -537,9 +594,9 @@ int Camera::release()
> return -EBUSY;
> }
>
> - pipe_->unlock();
> + p_->pipe_->unlock();
>
> - state_ = CameraAvailable;
> + p_->state_ = Private::CameraAvailable;
>
> return 0;
> }
> @@ -553,7 +610,7 @@ int Camera::release()
> */
> const ControlInfoMap &Camera::controls()
> {
> - return pipe_->controls(this);
> + return p_->pipe_->controls(this);
> }
>
> /**
> @@ -567,7 +624,7 @@ const ControlInfoMap &Camera::controls()
> */
> const std::set<Stream *> &Camera::streams() const
> {
> - return streams_;
> + return p_->streams_;
> }
>
> /**
> @@ -586,10 +643,10 @@ const std::set<Stream *> &Camera::streams() const
> */
> std::unique_ptr<CameraConfiguration> Camera::generateConfiguration(const StreamRoles &roles)
> {
> - if (disconnected_ || roles.size() > streams_.size())
> + if (p_->disconnected_ || roles.size() > streams().size())
> return nullptr;
>
> - CameraConfiguration *config = pipe_->generateConfiguration(this, roles);
> + CameraConfiguration *config = p_->pipe_->generateConfiguration(this, roles);
> if (!config) {
> LOG(Camera, Debug)
> << "Pipeline handler failed to generate configuration";
> @@ -639,10 +696,11 @@ int Camera::configure(CameraConfiguration *config)
> {
> int ret;
>
> - if (disconnected_)
> + if (p_->disconnected_)
> return -ENODEV;
>
> - if (!stateBetween(CameraAcquired, CameraConfigured))
> + if (!p_->stateBetween(Private::CameraAcquired,
> + Private::CameraConfigured))
> return -EACCES;
>
> if (allocator_ && allocator_->allocated()) {
> @@ -667,11 +725,11 @@ int Camera::configure(CameraConfiguration *config)
>
> LOG(Camera, Info) << msg.str();
>
> - ret = pipe_->configure(this, config);
> + ret = p_->pipe_->configure(this, config);
> if (ret)
> return ret;
>
> - activeStreams_.clear();
> + p_->activeStreams_.clear();
> for (const StreamConfiguration &cfg : *config) {
> Stream *stream = cfg.stream();
> if (!stream)
> @@ -679,10 +737,10 @@ int Camera::configure(CameraConfiguration *config)
> << "Pipeline handler failed to update stream configuration";
>
> stream->configuration_ = cfg;
> - activeStreams_.insert(stream);
> + p_->activeStreams_.insert(stream);
> }
>
> - state_ = CameraConfigured;
> + p_->state_ = Private::CameraConfigured;
>
> return 0;
> }
> @@ -709,7 +767,9 @@ int Camera::configure(CameraConfiguration *config)
> */
> Request *Camera::createRequest(uint64_t cookie)
> {
> - if (disconnected_ || !stateBetween(CameraConfigured, CameraRunning))
> + if (p_->disconnected_ ||
> + !p_->stateBetween(Private::CameraConfigured,
> + Private::CameraRunning))
> return nullptr;
>
> return new Request(this, cookie);
> @@ -739,10 +799,10 @@ Request *Camera::createRequest(uint64_t cookie)
> */
> int Camera::queueRequest(Request *request)
> {
> - if (disconnected_)
> + if (p_->disconnected_)
> return -ENODEV;
>
> - if (!stateIs(CameraRunning))
> + if (!p_->stateIs(Private::CameraRunning))
> return -EACCES;
>
> if (request->buffers().empty()) {
> @@ -753,13 +813,13 @@ int Camera::queueRequest(Request *request)
> for (auto const &it : request->buffers()) {
> Stream *stream = it.first;
>
> - if (activeStreams_.find(stream) == activeStreams_.end()) {
> + if (p_->activeStreams_.find(stream) == p_->activeStreams_.end()) {
> LOG(Camera, Error) << "Invalid request";
> return -EINVAL;
> }
> }
>
> - return pipe_->queueRequest(this, request);
> + return p_->pipe_->queueRequest(this, request);
> }
>
> /**
> @@ -777,26 +837,26 @@ int Camera::queueRequest(Request *request)
> */
> int Camera::start()
> {
> - if (disconnected_)
> + if (p_->disconnected_)
> return -ENODEV;
>
> - if (!stateIs(CameraConfigured))
> + if (!p_->stateIs(Private::CameraConfigured))
> return -EACCES;
>
> LOG(Camera, Debug) << "Starting capture";
>
> - for (Stream *stream : activeStreams_) {
> + for (Stream *stream : p_->activeStreams_) {
> if (allocator_ && !allocator_->buffers(stream).empty())
> continue;
>
> - pipe_->importFrameBuffers(this, stream);
> + p_->pipe_->importFrameBuffers(this, stream);
> }
>
> - int ret = pipe_->start(this);
> + int ret = p_->pipe_->start(this);
> if (ret)
> return ret;
>
> - state_ = CameraRunning;
> + p_->state_ = Private::CameraRunning;
>
> return 0;
> }
> @@ -815,23 +875,23 @@ int Camera::start()
> */
> int Camera::stop()
> {
> - if (disconnected_)
> + if (p_->disconnected_)
> return -ENODEV;
>
> - if (!stateIs(CameraRunning))
> + if (!p_->stateIs(Private::CameraRunning))
> return -EACCES;
>
> LOG(Camera, Debug) << "Stopping capture";
>
> - state_ = CameraConfigured;
> + p_->state_ = Private::CameraConfigured;
>
> - pipe_->stop(this);
> + p_->pipe_->stop(this);
>
> - for (Stream *stream : activeStreams_) {
> + for (Stream *stream : p_->activeStreams_) {
> if (allocator_ && !allocator_->buffers(stream).empty())
> continue;
>
> - pipe_->freeFrameBuffers(this, stream);
> + p_->pipe_->freeFrameBuffers(this, stream);
> }
>
> return 0;
> diff --git a/src/libcamera/framebuffer_allocator.cpp b/src/libcamera/framebuffer_allocator.cpp
> index a7588c7fe4c2..4f442d0d92e7 100644
> --- a/src/libcamera/framebuffer_allocator.cpp
> +++ b/src/libcamera/framebuffer_allocator.cpp
> @@ -89,7 +89,7 @@ FrameBufferAllocator::~FrameBufferAllocator()
> {
> for (auto &value : buffers_) {
> Stream *stream = value.first;
> - camera_->pipe_->freeFrameBuffers(camera_.get(), stream);
> + camera_->freeFrameBuffers(stream);
> }
>
> buffers_.clear();
> @@ -116,36 +116,17 @@ FrameBufferAllocator::~FrameBufferAllocator()
> */
> int FrameBufferAllocator::allocate(Stream *stream)
> {
> - if (camera_->state_ != Camera::CameraConfigured) {
> - LOG(Allocator, Error)
> - << "Camera must be in the configured state to allocate buffers";
> - return -EACCES;
> - }
> -
> - if (camera_->streams().find(stream) == camera_->streams().end()) {
> - LOG(Allocator, Error)
> - << "Stream does not belong to " << camera_->name();
> - return -EINVAL;
> - }
> -
> - if (camera_->activeStreams_.find(stream) == camera_->activeStreams_.end()) {
> - LOG(Allocator, Error)
> - << "Stream is not part of " << camera_->name()
> - << " active configuration";
> - return -EINVAL;
> - }
> -
> if (buffers_.count(stream)) {
> LOG(Allocator, Error) << "Buffers already allocated for stream";
> return -EBUSY;
> }
>
> - int ret = camera_->pipe_->exportFrameBuffers(camera_.get(), stream,
> - &buffers_[stream]);
> - if (ret)
> - return ret;
> -
> - return 0;
> + int ret = camera_->exportFrameBuffers(stream, &buffers_[stream]);
> + if (ret == -EINVAL)
> + LOG(Allocator, Error)
> + << "Stream is not part of " << camera_->name()
> + << " active configuration";
> + return ret;
> }
>
> /**
> @@ -162,22 +143,16 @@ int FrameBufferAllocator::allocate(Stream *stream)
> */
> int FrameBufferAllocator::free(Stream *stream)
> {
> - if (camera_->state_ != Camera::CameraConfigured) {
> - LOG(Allocator, Error)
> - << "Camera must be in the configured state to free buffers";
> - return -EACCES;
> - }
> -
> auto iter = buffers_.find(stream);
> if (iter == buffers_.end())
> return -EINVAL;
>
> - std::vector<std::unique_ptr<FrameBuffer>> &buffers = iter->second;
> + int ret = camera_->freeFrameBuffers(stream);
> + if (ret < 0)
> + return ret;
>
> + std::vector<std::unique_ptr<FrameBuffer>> &buffers = iter->second;
> buffers.clear();
> -
> - camera_->pipe_->freeFrameBuffers(camera_.get(), stream);
> -
> buffers_.erase(iter);
>
> return 0;
> --
> Regards,
>
> Laurent Pinchart
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list