[libcamera-devel] [PATCH 5/8] libcamera: camera: add state machine to control access from applications
Jacopo Mondi
jacopo at jmondi.org
Tue Feb 26 18:13:43 CET 2019
Hi Niklas,
thanks for this big work
On Tue, Feb 26, 2019 at 03:18:54AM +0100, Niklas Söderlund wrote:
> There is a need to better control the order of operations an application
> perform on a camera for it to function correctly. Add a basic state
> machine to ensure applications perform operations on the camera in good
> order.
>
> Internal to the Camera four states are added; Disconnected, Free,
> Acquired and Running. Each state represents a higher state of
> configuration of the camera ultimately leading to the highest state
> where the camera is capturing frames. Each state supports a subset of
> operations the application can perform.
>
> * Disconnected
> Is the lowest state a camera can be in. It indicates that the camera
> have been disconnected from the system and the only operations an
> application shall do at this point is clean up and release the camera so
> it can be removed from libcamera as well.
>
> * Free
> Is the base state of a camera, an application can inspect the properties
> of the camera to determine if it wish to use it or not. If an
> application wish to use a camera it should acquire it to proceed to the
> next state.
>
> * Acquired
> When an application have acquired a camera it have exclusive access to
> it and can modify the cameras parameters to prepare it for capturing.
> Once the application is done configure the camera it may be started to
> progress to the running state. Once the camera is started it can not be
> reconfigured until it's stopped.
>
> * Running
> In this state the camera is running and able to process requests queued
> to it by the application. Once an application finish capturing the
> camera shall be stopped to change the state to acquired where it can be
> reconfigured or released.
>
Before going into the details of the patch, I wonder if the states you
have described here capture all the possible interactions with a
camera object, in particulare regarding confgiurations of streams.
I would have expected to see a "Configured" state, which unlock access
to the stream start/stop methods and to buffer allocation, and which
might be used to refuse other configurations, if there is an active
one.
My mental model of this is that once a camera's streams have been
configured, the configuration should be deleted (by passing, say, an
empty configuration vector) to make sure applications have an explicit
"unconfigure" step to prevent overriding a configuration (and possibly
centralize buffer release, which should not be done explicitly).
It seems to me here a configuration could be replaced freely, and you
can call free and allocte buffers on a camera which has just been
acquired (but not configured). Is this intentional?
Thanks
j
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
> include/libcamera/camera.h | 14 ++++-
> src/libcamera/camera.cpp | 110 +++++++++++++++++++++++--------------
> 2 files changed, 80 insertions(+), 44 deletions(-)
>
> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> index bf70255a6a5ea364..8c8545b074e8ae13 100644
> --- a/include/libcamera/camera.h
> +++ b/include/libcamera/camera.h
> @@ -55,20 +55,28 @@ public:
> int stop();
>
> private:
> + enum State {
> + Disconnected,
> + Free,
> + Acquired,
> + Running,
> + };
> +
> Camera(PipelineHandler *pipe, const std::string &name);
> ~Camera();
>
> + bool stateIs(State state) const;
> + bool stateIsAtleast(State state) const;
> +
> friend class PipelineHandler;
> void disconnect();
> - int exclusiveAccess();
>
> std::shared_ptr<PipelineHandler> pipe_;
> std::string name_;
> std::vector<Stream *> streams_;
> std::vector<Stream *> activeStreams_;
>
> - bool acquired_;
> - bool disconnected_;
> + State state_;
> };
>
> } /* namespace libcamera */
> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> index d4258fe3c7551af3..c50b14bbd904fc1c 100644
> --- a/src/libcamera/camera.cpp
> +++ b/src/libcamera/camera.cpp
> @@ -116,17 +116,47 @@ const std::string &Camera::name() const
> */
>
> Camera::Camera(PipelineHandler *pipe, const std::string &name)
> - : pipe_(pipe->shared_from_this()), name_(name), acquired_(false),
> - disconnected_(false)
> + : pipe_(pipe->shared_from_this()), name_(name), state_(Free)
> {
> }
>
> Camera::~Camera()
> {
> - if (acquired_)
> + if (state_ > Free)
> LOG(Camera, Error) << "Removing camera while still in use";
> }
>
> +static const std::string stateNames[] = {
> + "Disconnected",
> + "Free",
> + "Acquired",
> + "Running",
> +};
> +
> +bool Camera::stateIs(State state) const
> +{
> + if (state_ == state)
> + return true;
> +
> + LOG(Camera, Error) << "Camera in " << stateNames[state_]
> + << " state trying operation requiering "
> + << stateNames[state] << " state";
> +
> + return false;
> +}
> +
> +bool Camera::stateIsAtleast(State state) const
> +{
> + if (state_ >= state)
> + return true;
> +
> + LOG(Camera, Error) << "Camera in " << stateNames[state_]
> + << " state trying operation requiering at least "
> + << stateNames[state] << " state";
> +
> + return false;
> +}
> +
> /**
> * \brief Notify camera disconnection
> *
> @@ -140,7 +170,7 @@ void Camera::disconnect()
> {
> LOG(Camera, Debug) << "Disconnecting camera " << name_;
>
> - disconnected_ = true;
> + state_ = Disconnected;
> disconnected.emit(this);
> }
>
> @@ -162,10 +192,11 @@ void Camera::disconnect()
> */
> int Camera::acquire()
> {
> - if (acquired_)
> + if (!stateIs(Free))
> return -EBUSY;
>
> - acquired_ = true;
> + state_ = Acquired;
> +
> return 0;
> }
>
> @@ -177,7 +208,10 @@ int Camera::acquire()
> */
> void Camera::release()
> {
> - acquired_ = false;
> + if (!stateIs(Acquired))
> + return;
> +
> + state_ = Free;
> }
>
> /**
> @@ -191,6 +225,9 @@ void Camera::release()
> */
> const std::vector<Stream *> &Camera::streams() const
> {
> + if (!stateIsAtleast(Free))
> + std::vector<Stream *>{};
> +
> return streams_;
> }
>
> @@ -213,7 +250,7 @@ const std::vector<Stream *> &Camera::streams() const
> std::map<Stream *, StreamConfiguration>
> Camera::streamConfiguration(std::vector<Stream *> &streams)
> {
> - if (disconnected_ || !streams.size())
> + if (!stateIsAtleast(Free) || !streams.size())
> return std::map<Stream *, StreamConfiguration>{};
>
> return pipe_->streamConfiguration(this, streams);
> @@ -244,9 +281,8 @@ int Camera::configureStreams(std::map<Stream *, StreamConfiguration> &config)
> {
> int ret;
>
> - ret = exclusiveAccess();
> - if (ret)
> - return ret;
> + if (!stateIs(Acquired))
> + return -EACCES;
>
> if (!config.size()) {
> LOG(Camera, Error)
> @@ -284,11 +320,8 @@ int Camera::configureStreams(std::map<Stream *, StreamConfiguration> &config)
> */
> int Camera::allocateBuffers()
> {
> - int ret;
> -
> - ret = exclusiveAccess();
> - if (ret)
> - return ret;
> + if (!stateIs(Acquired))
> + return -EACCES;
>
> if (activeStreams_.empty()) {
> LOG(Camera, Error)
> @@ -297,7 +330,7 @@ int Camera::allocateBuffers()
> }
>
> for (Stream *stream : activeStreams_) {
> - ret = pipe_->allocateBuffers(this, stream);
> + int ret = pipe_->allocateBuffers(this, stream);
> if (ret) {
> LOG(Camera, Error) << "Failed to allocate buffers";
> freeBuffers();
> @@ -313,6 +346,9 @@ int Camera::allocateBuffers()
> */
> void Camera::freeBuffers()
> {
> + if (!stateIs(Acquired))
> + return;
> +
> for (Stream *stream : activeStreams_) {
> if (!stream->bufferPool().count())
> continue;
> @@ -336,7 +372,7 @@ void Camera::freeBuffers()
> */
> Request *Camera::createRequest()
> {
> - if (exclusiveAccess())
> + if (!stateIsAtleast(Acquired))
> return nullptr;
>
> return new Request(this);
> @@ -358,13 +394,10 @@ Request *Camera::createRequest()
> */
> int Camera::queueRequest(Request *request)
> {
> - int ret;
> + if (!stateIs(Running))
> + return -EACCES;
>
> - ret = exclusiveAccess();
> - if (ret)
> - return ret;
> -
> - ret = request->prepare();
> + int ret = request->prepare();
> if (ret) {
> LOG(Camera, Error) << "Failed to prepare request";
> return ret;
> @@ -385,13 +418,18 @@ int Camera::queueRequest(Request *request)
> */
> int Camera::start()
> {
> - int ret = exclusiveAccess();
> - if (ret)
> - return ret;
> + if (!stateIs(Acquired))
> + return -EACCES;
>
> LOG(Camera, Debug) << "Starting capture";
>
> - return pipe_->start(this);
> + int ret = pipe_->start(this);
> + if (ret)
> + return ret;
> +
> + state_ = Running;
> +
> + return 0;
> }
>
> /**
> @@ -405,24 +443,14 @@ int Camera::start()
> */
> int Camera::stop()
> {
> - int ret = exclusiveAccess();
> - if (ret)
> - return ret;
> + if (!stateIs(Running))
> + return -EACCES;
>
> LOG(Camera, Debug) << "Stopping capture";
>
> pipe_->stop(this);
>
> - return 0;
> -}
> -
> -int Camera::exclusiveAccess()
> -{
> - if (disconnected_)
> - return -ENODEV;
> -
> - if (!acquired_)
> - return -EACCES;
> + state_ = Acquired;
>
> return 0;
> }
> --
> 2.20.1
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20190226/04acca0f/attachment.sig>
More information about the libcamera-devel
mailing list