[libcamera-devel] [PATCH 15/19] libcamera: camera: Centralize state checks in Private class
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Jan 20 01:24:33 CET 2020
Move all accesses to the state_ and disconnected_ members to functions
of the Private class. This will make it easier to implement
synchronization, and simplifies the Camera class implementation.
Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
---
src/libcamera/camera.cpp | 163 ++++++++++++++++-------------
src/libcamera/pipeline_handler.cpp | 5 +-
2 files changed, 95 insertions(+), 73 deletions(-)
diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
index 6fe802f375a3..83c2249b8897 100644
--- a/src/libcamera/camera.cpp
+++ b/src/libcamera/camera.cpp
@@ -266,15 +266,21 @@ public:
Private(PipelineHandler *pipe, const std::string &name,
const std::set<Stream *> &streams);
+ ~Private();
- bool stateBetween(State low, State high) const;
- bool stateIs(State state) const;
+ int isAccessAllowed(State state, bool allowDisconnected = false) const;
+ int isAccessAllowed(State low, State high,
+ bool allowDisconnected = false) const;
+
+ void disconnect();
+ void setState(State state);
std::shared_ptr<PipelineHandler> pipe_;
std::string name_;
std::set<Stream *> streams_;
std::set<Stream *> activeStreams_;
+private:
bool disconnected_;
State state_;
};
@@ -286,6 +292,12 @@ Camera::Private::Private(PipelineHandler *pipe, const std::string &name,
{
}
+Camera::Private::~Private()
+{
+ if (state_ != Private::CameraAvailable)
+ LOG(Camera, Error) << "Removing camera while still in use";
+}
+
static constexpr std::array<const char *, 4> camera_state_names = {
"Available",
"Acquired",
@@ -293,10 +305,31 @@ static constexpr std::array<const char *, 4> camera_state_names = {
"Running",
};
-bool Camera::Private::stateBetween(State low, State high) const
+int Camera::Private::isAccessAllowed(State state, bool allowDisconnected) const
{
+ if (!allowDisconnected && disconnected_)
+ return -ENODEV;
+
+ if (state_ == state)
+ return 0;
+
+ 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 -EACCES;
+}
+
+int Camera::Private::isAccessAllowed(State low, State high,
+ bool allowDisconnected) const
+{
+ if (!allowDisconnected && disconnected_)
+ return -ENODEV;
+
if (state_ >= low && state_ <= high)
- return true;
+ return 0;
ASSERT(static_cast<unsigned int>(low) < camera_state_names.size() &&
static_cast<unsigned int>(high) < camera_state_names.size());
@@ -306,21 +339,25 @@ bool Camera::Private::stateBetween(State low, State high) const
<< camera_state_names[low] << " and "
<< camera_state_names[high];
- return false;
+ return -EACCES;
}
-bool Camera::Private::stateIs(State state) const
+void Camera::Private::disconnect()
{
- if (state_ == state)
- return true;
-
- ASSERT(static_cast<unsigned int>(state) < camera_state_names.size());
+ /*
+ * 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_ == Private::CameraRunning)
+ state_ = Private::CameraConfigured;
- LOG(Camera, Debug) << "Camera in " << camera_state_names[state_]
- << " state trying operation requiring state "
- << camera_state_names[state];
+ disconnected_ = true;
+}
- return false;
+void Camera::Private::setState(State state)
+{
+ state_ = state;
}
/**
@@ -468,8 +505,6 @@ Camera::Camera(PipelineHandler *pipe, const std::string &name,
Camera::~Camera()
{
- if (!p_->stateIs(Private::CameraAvailable))
- LOG(Camera, Error) << "Removing camera while still in use";
}
/**
@@ -488,23 +523,16 @@ void Camera::disconnect()
{
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 (p_->state_ == Private::CameraRunning)
- p_->state_ = Private::CameraConfigured;
-
- p_->disconnected_ = true;
+ p_->disconnect();
disconnected.emit(this);
}
int Camera::exportFrameBuffers(Stream *stream,
std::vector<std::unique_ptr<FrameBuffer>> *buffers)
{
- if (!p_->stateIs(Private::CameraConfigured))
- return -EACCES;
+ int ret = p_->isAccessAllowed(Private::CameraConfigured);
+ if (ret < 0)
+ return ret;
if (streams().find(stream) == streams().end())
return -EINVAL;
@@ -517,8 +545,9 @@ int Camera::exportFrameBuffers(Stream *stream,
int Camera::freeFrameBuffers(Stream *stream)
{
- if (!p_->stateIs(Private::CameraConfigured))
- return -EACCES;
+ int ret = p_->isAccessAllowed(Private::CameraConfigured, true);
+ if (ret < 0)
+ return ret;
p_->pipe_->freeFrameBuffers(this, stream);
@@ -550,11 +579,9 @@ int Camera::freeFrameBuffers(Stream *stream)
*/
int Camera::acquire()
{
- if (p_->disconnected_)
- return -ENODEV;
-
- if (!p_->stateIs(Private::CameraAvailable))
- return -EBUSY;
+ int ret = p_->isAccessAllowed(Private::CameraAvailable);
+ if (ret < 0)
+ return ret == -EACCES ? -EBUSY : ret;
if (!p_->pipe_->lock()) {
LOG(Camera, Info)
@@ -562,7 +589,7 @@ int Camera::acquire()
return -EBUSY;
}
- p_->state_ = Private::CameraAcquired;
+ p_->setState(Private::CameraAcquired);
return 0;
}
@@ -580,9 +607,10 @@ int Camera::acquire()
*/
int Camera::release()
{
- if (!p_->stateBetween(Private::CameraAvailable,
- Private::CameraConfigured))
- return -EBUSY;
+ int ret = p_->isAccessAllowed(Private::CameraAvailable,
+ Private::CameraConfigured, true);
+ if (ret < 0)
+ return ret == -EACCES ? -EBUSY : ret;
if (allocator_) {
/*
@@ -596,7 +624,7 @@ int Camera::release()
p_->pipe_->unlock();
- p_->state_ = Private::CameraAvailable;
+ p_->setState(Private::CameraAvailable);
return 0;
}
@@ -643,7 +671,12 @@ const std::set<Stream *> &Camera::streams() const
*/
std::unique_ptr<CameraConfiguration> Camera::generateConfiguration(const StreamRoles &roles)
{
- if (p_->disconnected_ || roles.size() > streams().size())
+ int ret = p_->isAccessAllowed(Private::CameraAvailable,
+ Private::CameraRunning);
+ if (ret < 0)
+ return nullptr;
+
+ if (roles.size() > streams().size())
return nullptr;
CameraConfiguration *config = p_->pipe_->generateConfiguration(this, roles);
@@ -694,14 +727,10 @@ std::unique_ptr<CameraConfiguration> Camera::generateConfiguration(const StreamR
*/
int Camera::configure(CameraConfiguration *config)
{
- int ret;
-
- if (p_->disconnected_)
- return -ENODEV;
-
- if (!p_->stateBetween(Private::CameraAcquired,
- Private::CameraConfigured))
- return -EACCES;
+ int ret = p_->isAccessAllowed(Private::CameraAcquired,
+ Private::CameraConfigured);
+ if (ret < 0)
+ return ret;
if (allocator_ && allocator_->allocated()) {
LOG(Camera, Error)
@@ -740,7 +769,7 @@ int Camera::configure(CameraConfiguration *config)
p_->activeStreams_.insert(stream);
}
- p_->state_ = Private::CameraConfigured;
+ p_->setState(Private::CameraConfigured);
return 0;
}
@@ -767,9 +796,9 @@ int Camera::configure(CameraConfiguration *config)
*/
Request *Camera::createRequest(uint64_t cookie)
{
- if (p_->disconnected_ ||
- !p_->stateBetween(Private::CameraConfigured,
- Private::CameraRunning))
+ int ret = p_->isAccessAllowed(Private::CameraConfigured,
+ Private::CameraRunning);
+ if (ret < 0)
return nullptr;
return new Request(this, cookie);
@@ -799,11 +828,9 @@ Request *Camera::createRequest(uint64_t cookie)
*/
int Camera::queueRequest(Request *request)
{
- if (p_->disconnected_)
- return -ENODEV;
-
- if (!p_->stateIs(Private::CameraRunning))
- return -EACCES;
+ int ret = p_->isAccessAllowed(Private::CameraRunning);
+ if (ret < 0)
+ return ret;
if (request->buffers().empty()) {
LOG(Camera, Error) << "Request contains no buffers";
@@ -837,11 +864,9 @@ int Camera::queueRequest(Request *request)
*/
int Camera::start()
{
- if (p_->disconnected_)
- return -ENODEV;
-
- if (!p_->stateIs(Private::CameraConfigured))
- return -EACCES;
+ int ret = p_->isAccessAllowed(Private::CameraConfigured);
+ if (ret < 0)
+ return ret;
LOG(Camera, Debug) << "Starting capture";
@@ -852,11 +877,11 @@ int Camera::start()
p_->pipe_->importFrameBuffers(this, stream);
}
- int ret = p_->pipe_->start(this);
+ ret = p_->pipe_->start(this);
if (ret)
return ret;
- p_->state_ = Private::CameraRunning;
+ p_->setState(Private::CameraRunning);
return 0;
}
@@ -875,15 +900,13 @@ int Camera::start()
*/
int Camera::stop()
{
- if (p_->disconnected_)
- return -ENODEV;
-
- if (!p_->stateIs(Private::CameraRunning))
- return -EACCES;
+ int ret = p_->isAccessAllowed(Private::CameraRunning);
+ if (ret < 0)
+ return ret;
LOG(Camera, Debug) << "Stopping capture";
- p_->state_ = Private::CameraConfigured;
+ p_->setState(Private::CameraConfigured);
p_->pipe_->stop(this);
diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
index 669097f609ab..3091971d5da0 100644
--- a/src/libcamera/pipeline_handler.cpp
+++ b/src/libcamera/pipeline_handler.cpp
@@ -314,7 +314,7 @@ const ControlInfoMap &PipelineHandler::controls(Camera *camera)
* exportFrameBuffers() and importFrameBuffers() for the streams contained in
* any camera configuration.
*
- * The only intended caller is the FrameBufferAllocator helper.
+ * The only intended caller is Camera::exportFrameBuffers().
*
* \return 0 on success or a negative error code otherwise
*/
@@ -357,8 +357,7 @@ const ControlInfoMap &PipelineHandler::controls(Camera *camera)
* called only after a successful call to either of these two methods, and only
* once per stream.
*
- * The only intended callers are Camera::stop() and the FrameBufferAllocator
- * helper.
+ * The only intended callers are Camera::stop() and Camera::freeFrameBuffers().
*/
/**
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list