[libcamera-devel] [PATCH 14/19] libcamera: camera: Move private data members to private implementation
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Jan 22 20:04:23 CET 2020
Hi Niklas,
On Wed, Jan 22, 2020 at 05:21:02PM +0100, Niklas Söderlund wrote:
> 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,
I'll then add a link to https://wiki.qt.io/D-Pointer as [1] doesn't
mention d-pointers.
> 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
More information about the libcamera-devel
mailing list