[libcamera-devel] [PATCH 15/19] libcamera: camera: Centralize state checks in Private class
Niklas Söderlund
niklas.soderlund at ragnatech.se
Wed Jan 22 17:31:28 CET 2020
Hi Laurent,
Thanks for your work.
On 2020-01-20 02:24:33 +0200, Laurent Pinchart wrote:
> 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>
I really like this patch.
> ---
> 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
I wonder if we should document the \retval codes here and then use
\copydetails from callers in Camera to make sure -ENODEV and -EACCESS is
uniformly documented.
Or maybe that is not possible as Doxygen ignores libcamera::*::Private.
With or without this cherry on top,
Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> +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
>
> _______________________________________________
> 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