| Message ID | 20190228185126.32475-4-niklas.soderlund@ragnatech.se |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Niklas, Thank you for the patch. On Thu, Feb 28, 2019 at 07:51:26PM +0100, Niklas Söderlund wrote: > There is a need to better control the order of operations an application > performs 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 states are added; Available, Acquired, > Configured, Prepared 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 may perform. > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > include/libcamera/camera.h | 18 ++- > src/libcamera/camera.cpp | 266 +++++++++++++++++++++++++++++++------ > 2 files changed, 238 insertions(+), 46 deletions(-) > > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h > index 9c8ae01ed5c607f1..e5212cf05d221279 100644 > --- a/include/libcamera/camera.h > +++ b/include/libcamera/camera.h > @@ -39,7 +39,7 @@ public: > Signal<Camera *> disconnected; > > int acquire(); > - void release(); > + int release(); > > const std::set<Stream *> &streams() const; > std::map<Stream *, StreamConfiguration> > @@ -47,7 +47,7 @@ public: > int configureStreams(std::map<Stream *, StreamConfiguration> &config); > > int allocateBuffers(); > - void freeBuffers(); > + int freeBuffers(); > > Request *createRequest(); > int queueRequest(Request *request); > @@ -56,20 +56,30 @@ public: > int stop(); > > private: > + enum State { > + CameraAvailable, > + CameraAcquired, > + CameraConfigured, > + CameraPrepared, > + CameraRunning, > + }; > + > Camera(PipelineHandler *pipe, const std::string &name); > ~Camera(); > > + bool stateBetween(State low, State high) const; > + bool stateIs(State state) const; > + > friend class PipelineHandler; > void disconnect(); > - int exclusiveAccess(); > > std::shared_ptr<PipelineHandler> pipe_; > std::string name_; > std::set<Stream *> streams_; > std::set<Stream *> activeStreams_; > > - bool acquired_; > bool disconnected_; > + State state_; > }; > > } /* namespace libcamera */ > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > index 84b97b5c2ce94ecf..0b06c0d838b356f3 100644 > --- a/src/libcamera/camera.cpp > +++ b/src/libcamera/camera.cpp > @@ -11,6 +11,7 @@ > > #include "log.h" > #include "pipeline_handler.h" > +#include "utils.h" > > /** > * \file camera.h > @@ -42,6 +43,9 @@ LOG_DECLARE_CATEGORY(Camera) > * \class Camera > * \brief Camera device > * > + * \todo Add documentation for camera start timings. What exactly does the > + * camera expect the pipeline handler to do when start() is called? > + * > * The Camera class models a camera capable of producing one or more image > * streams from a single image source. It provides the main interface to > * configuring and controlling the device, and capturing image streams. It is > @@ -52,6 +56,78 @@ LOG_DECLARE_CATEGORY(Camera) > * created with the create() function which returns a shared pointer. The > * Camera constructors and destructor are private, to prevent instances from > * being constructed and destroyed manually. > + * > + * \section camera_operation Operating the Camera > + * > + * An application needs to perform a sequence of operations on a camera before > + * it is ready to process requests. The camera needs to be acquired, configured > + * and resources allocated or imported to prepare the camera for capture. Once > + * started the camera can process requests until it is stopped. When an > + * application is done with a camera all resources allocated needs to be freed s/needs/need/ > + * and the camera released. > + * > + * An application may start and stop a camera multiple times as long as it is > + * not released. The camera may also be reconfigured provided that all > + * resources allocated are freed prior to the reconfiguration. > + * > + * \subsection Camera States > + * > + * To help manage the sequence of operations needed to control the camera a set > + * of states are defined. Each state describes which operations may be performed > + * on the camera. Operations not listed in the state diagram are allowed in all > + * states. > + * > + * \dot > + * digraph camera_state_machine { > + * node [shape = doublecircle ]; Available; > + * node [shape = circle ]; Acquired; > + * node [shape = circle ]; Configured; > + * node [shape = circle ]; Prepared; > + * node [shape = circle ]; Running; > + * > + * Available -> Available [label = "release()"]; > + * Available -> Acquired [label = "acquire()"]; > + * > + * Acquired -> Available [label = "release()"]; > + * Acquired -> Configured [label = "configureStreams()"]; > + * > + * Configured -> Available [label = "release()"]; > + * Configured -> Configured [label = "configureStreams()"]; > + * Configured -> Prepared [label = "allocateBuffers()"]; > + * > + * Prepared -> Configured [label = "freeBuffers()"]; > + * Prepared -> Prepared [label = "createRequest()"]; > + * Prepared -> Running [label = "start()"]; > + * > + * Running -> Prepared [label = "stop()"]; > + * Running -> Running [label = "createRequest(), queueRequest()"]; > + * } > + * \enddot > + * > + * \subsubsection Available > + * The base state of a camera, an application can inspect the properties of the > + * camera to determine if it wishes to use it. If an application wishes to use > + * a camera it should acquire() it to proceed to the Acquired state. > + * > + * \subsubsection Acquired > + * In the acquired state an application has exclusive access to the camera and > + * may modify the camera's parameters to configure it and proceed to the > + * Configured state. > + * > + * \subsubsection Configured > + * The camera is configured and ready for the application to prepare it with > + * resources. The camera may be reconfigured multiple times until resources > + * are provided and the state progresses to Prepared. > + * > + * \subsubsection Prepared > + * The camera has been configured and provided with resources and is ready to be > + * started. The application may free the camera's resources to get back to the > + * Configured state or start it to progress to the Running state. s/start/start()/ > + * > + * \subsubsection Running > + * The camera is running and ready to process requests queued by the > + * application. The camera remains in this state until it is stopped and moved > + * to the Prepared state. > */ > > /** > @@ -116,17 +192,55 @@ 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), disconnected_(false), > + state_(CameraAvailable) > { > } > > Camera::~Camera() > { > - if (acquired_) > + if (!stateIs(CameraAvailable)) > LOG(Camera, Error) << "Removing camera while still in use"; > } > > +static const char *const camera_state_name[] = { Maybe camera_state_names ? > + "Available", > + "Acquired", > + "Configured", > + "Prepared", > + "Running", > +}; > + > +bool Camera::stateBetween(State low, State high) const > +{ > + if (state_ >= low && state_ <= high) > + return true; > + > + ASSERT(static_cast<unsigned int>(low) < ARRAY_SIZE(camera_state_name) && > + static_cast<unsigned int>(high) < ARRAY_SIZE(camera_state_name)); > + > + LOG(Camera, Debug) << "Camera in " << camera_state_name[state_] > + << " state trying operation requiring state between " > + << camera_state_name[low] << " and " > + << camera_state_name[high]; > + > + return false; > +} > + > +bool Camera::stateIs(State state) const > +{ > + if (state_ == state) > + return true; > + > + ASSERT(static_cast<unsigned int>(state) < ARRAY_SIZE(camera_state_name)); > + > + LOG(Camera, Debug) << "Camera in " << camera_state_name[state_] > + << " state trying operation requiring state " > + << camera_state_name[state]; > + > + return false; > +} > + > /** > * \brief Notify camera disconnection > * > @@ -135,11 +249,24 @@ Camera::~Camera() > * instance notifies the application by emitting the #disconnected signal, and > * ensures that all new calls to the application-facing Camera API return an > * error immediately. > + * > + * \todo: Deal with pending requests if the camera is disconnected in a > + * running state. > + * \todo: Update comment about Running state when importing buffers as well as > + * allocating them are supported. No need for colons after any of the \todo tags. > */ > void Camera::disconnect() > { > LOG(Camera, Debug) << "Disconnecting camera " << name_; > > + /* > + * If the camera was running when the hardware was removed force the > + * state to Prepared to allow applications to call freeBuffers() and > + * release() before deleting the camera. > + */ > + if (state_ == CameraRunning) > + state_ = CameraPrepared; > + > disconnected_ = true; > disconnected.emit(this); > } > @@ -155,16 +282,24 @@ void Camera::disconnect() > * Once exclusive access isn't needed anymore, the device should be released > * with a call to the release() function. > * > + * This function effects the state of the camera, see \ref camera_operation. s/effects/affects/ (through the whole file) > + * > * \todo Implement exclusive access across processes. > * > * \return 0 on success or a negative error code otherwise > + * \retval -ENODEV The camera has been disconnected from the system > + * \retval -EBUSY The camera is not free and can't be acquired by the caller > */ > int Camera::acquire() > { > - if (acquired_) > + if (disconnected_) > + return -ENODEV; > + > + if (!stateIs(CameraAvailable)) > return -EBUSY; > > - acquired_ = true; > + state_ = CameraAcquired; > + > return 0; > } > > @@ -173,10 +308,20 @@ int Camera::acquire() > * > * Releasing the camera device allows other users to acquire exclusive access > * with the acquire() function. > + * > + * This function effects the state of the camera, see \ref camera_operation. > + * > + * \return 0 on success or a negative error code otherwise > + * \retval -EBUSY The camera is running and can't be released > */ > -void Camera::release() > +int Camera::release() > { > - acquired_ = false; > + if (!stateBetween(CameraAvailable, CameraConfigured)) > + return -EBUSY; > + > + state_ = CameraAvailable; > + > + return 0; > } > > /** > @@ -235,18 +380,22 @@ Camera::streamConfiguration(std::set<Stream *> &streams) > * Exclusive access to the camera shall be ensured by a call to acquire() prior > * to calling this function, otherwise an -EACCES error will be returned. > * > + * This function effects the state of the camera, see \ref camera_operation. > + * > * \return 0 on success or a negative error code otherwise > - * \retval -ENODEV The camera is not connected to any hardware > - * \retval -EACCES The user has not acquired exclusive access to the camera > + * \retval -ENODEV The camera has been disconnected from the system > + * \retval -EACCES The camera is not in a state where it can be configured > * \retval -EINVAL The configuration is not valid > */ > int Camera::configureStreams(std::map<Stream *, StreamConfiguration> &config) > { > int ret; > > - ret = exclusiveAccess(); > - if (ret) > - return ret; > + if (disconnected_) > + return -ENODEV; > + > + if (!stateBetween(CameraAvailable, CameraConfigured)) > + return -EACCES; > > if (!config.size()) { > LOG(Camera, Error) > @@ -273,20 +422,28 @@ int Camera::configureStreams(std::map<Stream *, StreamConfiguration> &config) > stream->bufferPool().createBuffers(cfg.bufferCount); > } > > + state_ = CameraConfigured; > + > return 0; > } > > /** > * \brief Allocate buffers for all configured streams > + * > + * This function effects the state of the camera, see \ref camera_operation. > + * > * \return 0 on success or a negative error code otherwise > + * \retval -ENODEV The camera has been disconnected from the system > + * \retval -EACCES The camera is not in a state where buffers can be allocated > + * \retval -EINVAL The configuration is not valid > */ > int Camera::allocateBuffers() > { > - int ret; > + if (disconnected_) > + return -ENODEV; > > - ret = exclusiveAccess(); > - if (ret) > - return ret; > + if (!stateIs(CameraConfigured)) > + return -EACCES; > > if (activeStreams_.empty()) { > LOG(Camera, Error) > @@ -295,7 +452,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(); > @@ -303,14 +460,24 @@ int Camera::allocateBuffers() > } > } > > + state_ = CameraPrepared; > + > return 0; > } > > /** > * \brief Release all buffers from allocated pools in each stream > + * > + * This function effects the state of the camera, see \ref camera_operation. > + * > + * \return 0 on success or a negative error code otherwise > + * \retval -EACCES The camera is not in a state where buffers can be freed > */ > -void Camera::freeBuffers() > +int Camera::freeBuffers() > { > + if (!stateIs(CameraPrepared)) > + return -EACCES; > + > for (Stream *stream : activeStreams_) { > if (!stream->bufferPool().count()) > continue; > @@ -318,6 +485,10 @@ void Camera::freeBuffers() > pipe_->freeBuffers(this, stream); > stream->bufferPool().destroyBuffers(); > } > + > + state_ = CameraConfigured; > + > + return 0; > } > > /** > @@ -333,7 +504,7 @@ void Camera::freeBuffers() > */ > Request *Camera::createRequest() Should you update the documentation of this function ? Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > { > - if (exclusiveAccess()) > + if (disconnected_ || !stateBetween(CameraPrepared, CameraRunning)) > return nullptr; > > return new Request(this); > @@ -351,16 +522,18 @@ Request *Camera::createRequest() > * automatically after it completes. > * > * \return 0 on success or a negative error code otherwise > + * \retval -ENODEV The camera has been disconnected from the system > + * \retval -EACCES The camera is not running so requests can't be queued > */ > int Camera::queueRequest(Request *request) > { > - int ret; > + if (disconnected_) > + return -ENODEV; > > - ret = exclusiveAccess(); > - if (ret) > - return ret; > + if (!stateIs(CameraRunning)) > + return -EACCES; > > - ret = request->prepare(); > + int ret = request->prepare(); > if (ret) { > LOG(Camera, Error) << "Failed to prepare request"; > return ret; > @@ -376,17 +549,29 @@ int Camera::queueRequest(Request *request) > * can queue requests to the camera to process and return to the application > * until the capture session is terminated with \a stop(). > * > + * This function effects the state of the camera, see \ref camera_operation. > + * > * \return 0 on success or a negative error code otherwise > + * \retval -ENODEV The camera has been disconnected from the system > + * \retval -EACCES The camera is not in a state where it can be started > */ > int Camera::start() > { > - int ret = exclusiveAccess(); > - if (ret) > - return ret; > + if (disconnected_) > + return -ENODEV; > + > + if (!stateIs(CameraPrepared)) > + return -EACCES; > > LOG(Camera, Debug) << "Starting capture"; > > - return pipe_->start(this); > + int ret = pipe_->start(this); > + if (ret) > + return ret; > + > + state_ = CameraRunning; > + > + return 0; > } > > /** > @@ -395,30 +580,27 @@ int Camera::start() > * This method stops capturing and processing requests immediately. All pending > * requests are cancelled and complete synchronously in an error state. > * > + * This function effects the state of the camera, see \ref camera_operation. > + * > * \return 0 on success or a negative error code otherwise > + * \retval -ENODEV The camera has been disconnected from the system > + * \retval -EACCES The camera is not running so can't be stopped > */ > int Camera::stop() > { > - int ret = exclusiveAccess(); > - if (ret) > - return ret; > + if (disconnected_) > + return -ENODEV; > + > + if (!stateIs(CameraRunning)) > + return -EACCES; > > LOG(Camera, Debug) << "Stopping capture"; > > + state_ = CameraPrepared; > + > pipe_->stop(this); > > return 0; > } > > -int Camera::exclusiveAccess() > -{ > - if (disconnected_) > - return -ENODEV; > - > - if (!acquired_) > - return -EACCES; > - > - return 0; > -} > - > } /* namespace libcamera */
Hi Laurent, Thanks for your feedback. On 2019-02-28 23:39:47 +0200, Laurent Pinchart wrote: > Hi Niklas, > > Thank you for the patch. > > On Thu, Feb 28, 2019 at 07:51:26PM +0100, Niklas Söderlund wrote: > > There is a need to better control the order of operations an application > > performs 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 states are added; Available, Acquired, > > Configured, Prepared 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 may perform. > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > --- > > include/libcamera/camera.h | 18 ++- > > src/libcamera/camera.cpp | 266 +++++++++++++++++++++++++++++++------ > > 2 files changed, 238 insertions(+), 46 deletions(-) > > > > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h > > index 9c8ae01ed5c607f1..e5212cf05d221279 100644 > > --- a/include/libcamera/camera.h > > +++ b/include/libcamera/camera.h > > @@ -39,7 +39,7 @@ public: > > Signal<Camera *> disconnected; > > > > int acquire(); > > - void release(); > > + int release(); > > > > const std::set<Stream *> &streams() const; > > std::map<Stream *, StreamConfiguration> > > @@ -47,7 +47,7 @@ public: > > int configureStreams(std::map<Stream *, StreamConfiguration> &config); > > > > int allocateBuffers(); > > - void freeBuffers(); > > + int freeBuffers(); > > > > Request *createRequest(); > > int queueRequest(Request *request); > > @@ -56,20 +56,30 @@ public: > > int stop(); > > > > private: > > + enum State { > > + CameraAvailable, > > + CameraAcquired, > > + CameraConfigured, > > + CameraPrepared, > > + CameraRunning, > > + }; > > + > > Camera(PipelineHandler *pipe, const std::string &name); > > ~Camera(); > > > > + bool stateBetween(State low, State high) const; > > + bool stateIs(State state) const; > > + > > friend class PipelineHandler; > > void disconnect(); > > - int exclusiveAccess(); > > > > std::shared_ptr<PipelineHandler> pipe_; > > std::string name_; > > std::set<Stream *> streams_; > > std::set<Stream *> activeStreams_; > > > > - bool acquired_; > > bool disconnected_; > > + State state_; > > }; > > > > } /* namespace libcamera */ > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > > index 84b97b5c2ce94ecf..0b06c0d838b356f3 100644 > > --- a/src/libcamera/camera.cpp > > +++ b/src/libcamera/camera.cpp > > @@ -11,6 +11,7 @@ > > > > #include "log.h" > > #include "pipeline_handler.h" > > +#include "utils.h" > > > > /** > > * \file camera.h > > @@ -42,6 +43,9 @@ LOG_DECLARE_CATEGORY(Camera) > > * \class Camera > > * \brief Camera device > > * > > + * \todo Add documentation for camera start timings. What exactly does the > > + * camera expect the pipeline handler to do when start() is called? > > + * > > * The Camera class models a camera capable of producing one or more image > > * streams from a single image source. It provides the main interface to > > * configuring and controlling the device, and capturing image streams. It is > > @@ -52,6 +56,78 @@ LOG_DECLARE_CATEGORY(Camera) > > * created with the create() function which returns a shared pointer. The > > * Camera constructors and destructor are private, to prevent instances from > > * being constructed and destroyed manually. > > + * > > + * \section camera_operation Operating the Camera > > + * > > + * An application needs to perform a sequence of operations on a camera before > > + * it is ready to process requests. The camera needs to be acquired, configured > > + * and resources allocated or imported to prepare the camera for capture. Once > > + * started the camera can process requests until it is stopped. When an > > + * application is done with a camera all resources allocated needs to be freed > > s/needs/need/ > > > + * and the camera released. > > + * > > + * An application may start and stop a camera multiple times as long as it is > > + * not released. The camera may also be reconfigured provided that all > > + * resources allocated are freed prior to the reconfiguration. > > + * > > + * \subsection Camera States > > + * > > + * To help manage the sequence of operations needed to control the camera a set > > + * of states are defined. Each state describes which operations may be performed > > + * on the camera. Operations not listed in the state diagram are allowed in all > > + * states. > > + * > > + * \dot > > + * digraph camera_state_machine { > > + * node [shape = doublecircle ]; Available; > > + * node [shape = circle ]; Acquired; > > + * node [shape = circle ]; Configured; > > + * node [shape = circle ]; Prepared; > > + * node [shape = circle ]; Running; > > + * > > + * Available -> Available [label = "release()"]; > > + * Available -> Acquired [label = "acquire()"]; > > + * > > + * Acquired -> Available [label = "release()"]; > > + * Acquired -> Configured [label = "configureStreams()"]; > > + * > > + * Configured -> Available [label = "release()"]; > > + * Configured -> Configured [label = "configureStreams()"]; > > + * Configured -> Prepared [label = "allocateBuffers()"]; > > + * > > + * Prepared -> Configured [label = "freeBuffers()"]; > > + * Prepared -> Prepared [label = "createRequest()"]; > > + * Prepared -> Running [label = "start()"]; > > + * > > + * Running -> Prepared [label = "stop()"]; > > + * Running -> Running [label = "createRequest(), queueRequest()"]; > > + * } > > + * \enddot > > + * > > + * \subsubsection Available > > + * The base state of a camera, an application can inspect the properties of the > > + * camera to determine if it wishes to use it. If an application wishes to use > > + * a camera it should acquire() it to proceed to the Acquired state. > > + * > > + * \subsubsection Acquired > > + * In the acquired state an application has exclusive access to the camera and > > + * may modify the camera's parameters to configure it and proceed to the > > + * Configured state. > > + * > > + * \subsubsection Configured > > + * The camera is configured and ready for the application to prepare it with > > + * resources. The camera may be reconfigured multiple times until resources > > + * are provided and the state progresses to Prepared. > > + * > > + * \subsubsection Prepared > > + * The camera has been configured and provided with resources and is ready to be > > + * started. The application may free the camera's resources to get back to the > > + * Configured state or start it to progress to the Running state. > > s/start/start()/ > > > + * > > + * \subsubsection Running > > + * The camera is running and ready to process requests queued by the > > + * application. The camera remains in this state until it is stopped and moved > > + * to the Prepared state. > > */ > > > > /** > > @@ -116,17 +192,55 @@ 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), disconnected_(false), > > + state_(CameraAvailable) > > { > > } > > > > Camera::~Camera() > > { > > - if (acquired_) > > + if (!stateIs(CameraAvailable)) > > LOG(Camera, Error) << "Removing camera while still in use"; > > } > > > > +static const char *const camera_state_name[] = { > > Maybe camera_state_names ? > > > + "Available", > > + "Acquired", > > + "Configured", > > + "Prepared", > > + "Running", > > +}; > > + > > +bool Camera::stateBetween(State low, State high) const > > +{ > > + if (state_ >= low && state_ <= high) > > + return true; > > + > > + ASSERT(static_cast<unsigned int>(low) < ARRAY_SIZE(camera_state_name) && > > + static_cast<unsigned int>(high) < ARRAY_SIZE(camera_state_name)); > > + > > + LOG(Camera, Debug) << "Camera in " << camera_state_name[state_] > > + << " state trying operation requiring state between " > > + << camera_state_name[low] << " and " > > + << camera_state_name[high]; > > + > > + return false; > > +} > > + > > +bool Camera::stateIs(State state) const > > +{ > > + if (state_ == state) > > + return true; > > + > > + ASSERT(static_cast<unsigned int>(state) < ARRAY_SIZE(camera_state_name)); > > + > > + LOG(Camera, Debug) << "Camera in " << camera_state_name[state_] > > + << " state trying operation requiring state " > > + << camera_state_name[state]; > > + > > + return false; > > +} > > + > > /** > > * \brief Notify camera disconnection > > * > > @@ -135,11 +249,24 @@ Camera::~Camera() > > * instance notifies the application by emitting the #disconnected signal, and > > * ensures that all new calls to the application-facing Camera API return an > > * error immediately. > > + * > > + * \todo: Deal with pending requests if the camera is disconnected in a > > + * running state. > > + * \todo: Update comment about Running state when importing buffers as well as > > + * allocating them are supported. > > No need for colons after any of the \todo tags. > > > */ > > void Camera::disconnect() > > { > > LOG(Camera, Debug) << "Disconnecting camera " << name_; > > > > + /* > > + * If the camera was running when the hardware was removed force the > > + * state to Prepared to allow applications to call freeBuffers() and > > + * release() before deleting the camera. > > + */ > > + if (state_ == CameraRunning) > > + state_ = CameraPrepared; > > + > > disconnected_ = true; > > disconnected.emit(this); > > } > > @@ -155,16 +282,24 @@ void Camera::disconnect() > > * Once exclusive access isn't needed anymore, the device should be released > > * with a call to the release() function. > > * > > + * This function effects the state of the camera, see \ref camera_operation. > > s/effects/affects/ (through the whole file) > > > + * > > * \todo Implement exclusive access across processes. > > * > > * \return 0 on success or a negative error code otherwise > > + * \retval -ENODEV The camera has been disconnected from the system > > + * \retval -EBUSY The camera is not free and can't be acquired by the caller > > */ > > int Camera::acquire() > > { > > - if (acquired_) > > + if (disconnected_) > > + return -ENODEV; > > + > > + if (!stateIs(CameraAvailable)) > > return -EBUSY; > > > > - acquired_ = true; > > + state_ = CameraAcquired; > > + > > return 0; > > } > > > > @@ -173,10 +308,20 @@ int Camera::acquire() > > * > > * Releasing the camera device allows other users to acquire exclusive access > > * with the acquire() function. > > + * > > + * This function effects the state of the camera, see \ref camera_operation. > > + * > > + * \return 0 on success or a negative error code otherwise > > + * \retval -EBUSY The camera is running and can't be released > > */ > > -void Camera::release() > > +int Camera::release() > > { > > - acquired_ = false; > > + if (!stateBetween(CameraAvailable, CameraConfigured)) > > + return -EBUSY; > > + > > + state_ = CameraAvailable; > > + > > + return 0; > > } > > > > /** > > @@ -235,18 +380,22 @@ Camera::streamConfiguration(std::set<Stream *> &streams) > > * Exclusive access to the camera shall be ensured by a call to acquire() prior > > * to calling this function, otherwise an -EACCES error will be returned. > > * > > + * This function effects the state of the camera, see \ref camera_operation. > > + * > > * \return 0 on success or a negative error code otherwise > > - * \retval -ENODEV The camera is not connected to any hardware > > - * \retval -EACCES The user has not acquired exclusive access to the camera > > + * \retval -ENODEV The camera has been disconnected from the system > > + * \retval -EACCES The camera is not in a state where it can be configured > > * \retval -EINVAL The configuration is not valid > > */ > > int Camera::configureStreams(std::map<Stream *, StreamConfiguration> &config) > > { > > int ret; > > > > - ret = exclusiveAccess(); > > - if (ret) > > - return ret; > > + if (disconnected_) > > + return -ENODEV; > > + > > + if (!stateBetween(CameraAvailable, CameraConfigured)) > > + return -EACCES; > > > > if (!config.size()) { > > LOG(Camera, Error) > > @@ -273,20 +422,28 @@ int Camera::configureStreams(std::map<Stream *, StreamConfiguration> &config) > > stream->bufferPool().createBuffers(cfg.bufferCount); > > } > > > > + state_ = CameraConfigured; > > + > > return 0; > > } > > > > /** > > * \brief Allocate buffers for all configured streams > > + * > > + * This function effects the state of the camera, see \ref camera_operation. > > + * > > * \return 0 on success or a negative error code otherwise > > + * \retval -ENODEV The camera has been disconnected from the system > > + * \retval -EACCES The camera is not in a state where buffers can be allocated > > + * \retval -EINVAL The configuration is not valid > > */ > > int Camera::allocateBuffers() > > { > > - int ret; > > + if (disconnected_) > > + return -ENODEV; > > > > - ret = exclusiveAccess(); > > - if (ret) > > - return ret; > > + if (!stateIs(CameraConfigured)) > > + return -EACCES; > > > > if (activeStreams_.empty()) { > > LOG(Camera, Error) > > @@ -295,7 +452,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(); > > @@ -303,14 +460,24 @@ int Camera::allocateBuffers() > > } > > } > > > > + state_ = CameraPrepared; > > + > > return 0; > > } > > > > /** > > * \brief Release all buffers from allocated pools in each stream > > + * > > + * This function effects the state of the camera, see \ref camera_operation. > > + * > > + * \return 0 on success or a negative error code otherwise > > + * \retval -EACCES The camera is not in a state where buffers can be freed > > */ > > -void Camera::freeBuffers() > > +int Camera::freeBuffers() > > { > > + if (!stateIs(CameraPrepared)) > > + return -EACCES; > > + > > for (Stream *stream : activeStreams_) { > > if (!stream->bufferPool().count()) > > continue; > > @@ -318,6 +485,10 @@ void Camera::freeBuffers() > > pipe_->freeBuffers(this, stream); > > stream->bufferPool().destroyBuffers(); > > } > > + > > + state_ = CameraConfigured; > > + > > + return 0; > > } > > > > /** > > @@ -333,7 +504,7 @@ void Camera::freeBuffers() > > */ > > Request *Camera::createRequest() > > Should you update the documentation of this function ? > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Thanks, I have addressed all your comments, collected your tag and pushed. Thanks for all your hard work reviewing, especially the documentation bits. > > > { > > - if (exclusiveAccess()) > > + if (disconnected_ || !stateBetween(CameraPrepared, CameraRunning)) > > return nullptr; > > > > return new Request(this); > > @@ -351,16 +522,18 @@ Request *Camera::createRequest() > > * automatically after it completes. > > * > > * \return 0 on success or a negative error code otherwise > > + * \retval -ENODEV The camera has been disconnected from the system > > + * \retval -EACCES The camera is not running so requests can't be queued > > */ > > int Camera::queueRequest(Request *request) > > { > > - int ret; > > + if (disconnected_) > > + return -ENODEV; > > > > - ret = exclusiveAccess(); > > - if (ret) > > - return ret; > > + if (!stateIs(CameraRunning)) > > + return -EACCES; > > > > - ret = request->prepare(); > > + int ret = request->prepare(); > > if (ret) { > > LOG(Camera, Error) << "Failed to prepare request"; > > return ret; > > @@ -376,17 +549,29 @@ int Camera::queueRequest(Request *request) > > * can queue requests to the camera to process and return to the application > > * until the capture session is terminated with \a stop(). > > * > > + * This function effects the state of the camera, see \ref camera_operation. > > + * > > * \return 0 on success or a negative error code otherwise > > + * \retval -ENODEV The camera has been disconnected from the system > > + * \retval -EACCES The camera is not in a state where it can be started > > */ > > int Camera::start() > > { > > - int ret = exclusiveAccess(); > > - if (ret) > > - return ret; > > + if (disconnected_) > > + return -ENODEV; > > + > > + if (!stateIs(CameraPrepared)) > > + return -EACCES; > > > > LOG(Camera, Debug) << "Starting capture"; > > > > - return pipe_->start(this); > > + int ret = pipe_->start(this); > > + if (ret) > > + return ret; > > + > > + state_ = CameraRunning; > > + > > + return 0; > > } > > > > /** > > @@ -395,30 +580,27 @@ int Camera::start() > > * This method stops capturing and processing requests immediately. All pending > > * requests are cancelled and complete synchronously in an error state. > > * > > + * This function effects the state of the camera, see \ref camera_operation. > > + * > > * \return 0 on success or a negative error code otherwise > > + * \retval -ENODEV The camera has been disconnected from the system > > + * \retval -EACCES The camera is not running so can't be stopped > > */ > > int Camera::stop() > > { > > - int ret = exclusiveAccess(); > > - if (ret) > > - return ret; > > + if (disconnected_) > > + return -ENODEV; > > + > > + if (!stateIs(CameraRunning)) > > + return -EACCES; > > > > LOG(Camera, Debug) << "Stopping capture"; > > > > + state_ = CameraPrepared; > > + > > pipe_->stop(this); > > > > return 0; > > } > > > > -int Camera::exclusiveAccess() > > -{ > > - if (disconnected_) > > - return -ENODEV; > > - > > - if (!acquired_) > > - return -EACCES; > > - > > - return 0; > > -} > > - > > } /* namespace libcamera */ > > -- > Regards, > > Laurent Pinchart
diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h index 9c8ae01ed5c607f1..e5212cf05d221279 100644 --- a/include/libcamera/camera.h +++ b/include/libcamera/camera.h @@ -39,7 +39,7 @@ public: Signal<Camera *> disconnected; int acquire(); - void release(); + int release(); const std::set<Stream *> &streams() const; std::map<Stream *, StreamConfiguration> @@ -47,7 +47,7 @@ public: int configureStreams(std::map<Stream *, StreamConfiguration> &config); int allocateBuffers(); - void freeBuffers(); + int freeBuffers(); Request *createRequest(); int queueRequest(Request *request); @@ -56,20 +56,30 @@ public: int stop(); private: + enum State { + CameraAvailable, + CameraAcquired, + CameraConfigured, + CameraPrepared, + CameraRunning, + }; + Camera(PipelineHandler *pipe, const std::string &name); ~Camera(); + bool stateBetween(State low, State high) const; + bool stateIs(State state) const; + friend class PipelineHandler; void disconnect(); - int exclusiveAccess(); std::shared_ptr<PipelineHandler> pipe_; std::string name_; std::set<Stream *> streams_; std::set<Stream *> activeStreams_; - bool acquired_; bool disconnected_; + State state_; }; } /* namespace libcamera */ diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp index 84b97b5c2ce94ecf..0b06c0d838b356f3 100644 --- a/src/libcamera/camera.cpp +++ b/src/libcamera/camera.cpp @@ -11,6 +11,7 @@ #include "log.h" #include "pipeline_handler.h" +#include "utils.h" /** * \file camera.h @@ -42,6 +43,9 @@ LOG_DECLARE_CATEGORY(Camera) * \class Camera * \brief Camera device * + * \todo Add documentation for camera start timings. What exactly does the + * camera expect the pipeline handler to do when start() is called? + * * The Camera class models a camera capable of producing one or more image * streams from a single image source. It provides the main interface to * configuring and controlling the device, and capturing image streams. It is @@ -52,6 +56,78 @@ LOG_DECLARE_CATEGORY(Camera) * created with the create() function which returns a shared pointer. The * Camera constructors and destructor are private, to prevent instances from * being constructed and destroyed manually. + * + * \section camera_operation Operating the Camera + * + * An application needs to perform a sequence of operations on a camera before + * it is ready to process requests. The camera needs to be acquired, configured + * and resources allocated or imported to prepare the camera for capture. Once + * started the camera can process requests until it is stopped. When an + * application is done with a camera all resources allocated needs to be freed + * and the camera released. + * + * An application may start and stop a camera multiple times as long as it is + * not released. The camera may also be reconfigured provided that all + * resources allocated are freed prior to the reconfiguration. + * + * \subsection Camera States + * + * To help manage the sequence of operations needed to control the camera a set + * of states are defined. Each state describes which operations may be performed + * on the camera. Operations not listed in the state diagram are allowed in all + * states. + * + * \dot + * digraph camera_state_machine { + * node [shape = doublecircle ]; Available; + * node [shape = circle ]; Acquired; + * node [shape = circle ]; Configured; + * node [shape = circle ]; Prepared; + * node [shape = circle ]; Running; + * + * Available -> Available [label = "release()"]; + * Available -> Acquired [label = "acquire()"]; + * + * Acquired -> Available [label = "release()"]; + * Acquired -> Configured [label = "configureStreams()"]; + * + * Configured -> Available [label = "release()"]; + * Configured -> Configured [label = "configureStreams()"]; + * Configured -> Prepared [label = "allocateBuffers()"]; + * + * Prepared -> Configured [label = "freeBuffers()"]; + * Prepared -> Prepared [label = "createRequest()"]; + * Prepared -> Running [label = "start()"]; + * + * Running -> Prepared [label = "stop()"]; + * Running -> Running [label = "createRequest(), queueRequest()"]; + * } + * \enddot + * + * \subsubsection Available + * The base state of a camera, an application can inspect the properties of the + * camera to determine if it wishes to use it. If an application wishes to use + * a camera it should acquire() it to proceed to the Acquired state. + * + * \subsubsection Acquired + * In the acquired state an application has exclusive access to the camera and + * may modify the camera's parameters to configure it and proceed to the + * Configured state. + * + * \subsubsection Configured + * The camera is configured and ready for the application to prepare it with + * resources. The camera may be reconfigured multiple times until resources + * are provided and the state progresses to Prepared. + * + * \subsubsection Prepared + * The camera has been configured and provided with resources and is ready to be + * started. The application may free the camera's resources to get back to the + * Configured state or start it to progress to the Running state. + * + * \subsubsection Running + * The camera is running and ready to process requests queued by the + * application. The camera remains in this state until it is stopped and moved + * to the Prepared state. */ /** @@ -116,17 +192,55 @@ 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), disconnected_(false), + state_(CameraAvailable) { } Camera::~Camera() { - if (acquired_) + if (!stateIs(CameraAvailable)) LOG(Camera, Error) << "Removing camera while still in use"; } +static const char *const camera_state_name[] = { + "Available", + "Acquired", + "Configured", + "Prepared", + "Running", +}; + +bool Camera::stateBetween(State low, State high) const +{ + if (state_ >= low && state_ <= high) + return true; + + ASSERT(static_cast<unsigned int>(low) < ARRAY_SIZE(camera_state_name) && + static_cast<unsigned int>(high) < ARRAY_SIZE(camera_state_name)); + + LOG(Camera, Debug) << "Camera in " << camera_state_name[state_] + << " state trying operation requiring state between " + << camera_state_name[low] << " and " + << camera_state_name[high]; + + return false; +} + +bool Camera::stateIs(State state) const +{ + if (state_ == state) + return true; + + ASSERT(static_cast<unsigned int>(state) < ARRAY_SIZE(camera_state_name)); + + LOG(Camera, Debug) << "Camera in " << camera_state_name[state_] + << " state trying operation requiring state " + << camera_state_name[state]; + + return false; +} + /** * \brief Notify camera disconnection * @@ -135,11 +249,24 @@ Camera::~Camera() * instance notifies the application by emitting the #disconnected signal, and * ensures that all new calls to the application-facing Camera API return an * error immediately. + * + * \todo: Deal with pending requests if the camera is disconnected in a + * running state. + * \todo: Update comment about Running state when importing buffers as well as + * allocating them are supported. */ void Camera::disconnect() { LOG(Camera, Debug) << "Disconnecting camera " << name_; + /* + * If the camera was running when the hardware was removed force the + * state to Prepared to allow applications to call freeBuffers() and + * release() before deleting the camera. + */ + if (state_ == CameraRunning) + state_ = CameraPrepared; + disconnected_ = true; disconnected.emit(this); } @@ -155,16 +282,24 @@ void Camera::disconnect() * Once exclusive access isn't needed anymore, the device should be released * with a call to the release() function. * + * This function effects the state of the camera, see \ref camera_operation. + * * \todo Implement exclusive access across processes. * * \return 0 on success or a negative error code otherwise + * \retval -ENODEV The camera has been disconnected from the system + * \retval -EBUSY The camera is not free and can't be acquired by the caller */ int Camera::acquire() { - if (acquired_) + if (disconnected_) + return -ENODEV; + + if (!stateIs(CameraAvailable)) return -EBUSY; - acquired_ = true; + state_ = CameraAcquired; + return 0; } @@ -173,10 +308,20 @@ int Camera::acquire() * * Releasing the camera device allows other users to acquire exclusive access * with the acquire() function. + * + * This function effects the state of the camera, see \ref camera_operation. + * + * \return 0 on success or a negative error code otherwise + * \retval -EBUSY The camera is running and can't be released */ -void Camera::release() +int Camera::release() { - acquired_ = false; + if (!stateBetween(CameraAvailable, CameraConfigured)) + return -EBUSY; + + state_ = CameraAvailable; + + return 0; } /** @@ -235,18 +380,22 @@ Camera::streamConfiguration(std::set<Stream *> &streams) * Exclusive access to the camera shall be ensured by a call to acquire() prior * to calling this function, otherwise an -EACCES error will be returned. * + * This function effects the state of the camera, see \ref camera_operation. + * * \return 0 on success or a negative error code otherwise - * \retval -ENODEV The camera is not connected to any hardware - * \retval -EACCES The user has not acquired exclusive access to the camera + * \retval -ENODEV The camera has been disconnected from the system + * \retval -EACCES The camera is not in a state where it can be configured * \retval -EINVAL The configuration is not valid */ int Camera::configureStreams(std::map<Stream *, StreamConfiguration> &config) { int ret; - ret = exclusiveAccess(); - if (ret) - return ret; + if (disconnected_) + return -ENODEV; + + if (!stateBetween(CameraAvailable, CameraConfigured)) + return -EACCES; if (!config.size()) { LOG(Camera, Error) @@ -273,20 +422,28 @@ int Camera::configureStreams(std::map<Stream *, StreamConfiguration> &config) stream->bufferPool().createBuffers(cfg.bufferCount); } + state_ = CameraConfigured; + return 0; } /** * \brief Allocate buffers for all configured streams + * + * This function effects the state of the camera, see \ref camera_operation. + * * \return 0 on success or a negative error code otherwise + * \retval -ENODEV The camera has been disconnected from the system + * \retval -EACCES The camera is not in a state where buffers can be allocated + * \retval -EINVAL The configuration is not valid */ int Camera::allocateBuffers() { - int ret; + if (disconnected_) + return -ENODEV; - ret = exclusiveAccess(); - if (ret) - return ret; + if (!stateIs(CameraConfigured)) + return -EACCES; if (activeStreams_.empty()) { LOG(Camera, Error) @@ -295,7 +452,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(); @@ -303,14 +460,24 @@ int Camera::allocateBuffers() } } + state_ = CameraPrepared; + return 0; } /** * \brief Release all buffers from allocated pools in each stream + * + * This function effects the state of the camera, see \ref camera_operation. + * + * \return 0 on success or a negative error code otherwise + * \retval -EACCES The camera is not in a state where buffers can be freed */ -void Camera::freeBuffers() +int Camera::freeBuffers() { + if (!stateIs(CameraPrepared)) + return -EACCES; + for (Stream *stream : activeStreams_) { if (!stream->bufferPool().count()) continue; @@ -318,6 +485,10 @@ void Camera::freeBuffers() pipe_->freeBuffers(this, stream); stream->bufferPool().destroyBuffers(); } + + state_ = CameraConfigured; + + return 0; } /** @@ -333,7 +504,7 @@ void Camera::freeBuffers() */ Request *Camera::createRequest() { - if (exclusiveAccess()) + if (disconnected_ || !stateBetween(CameraPrepared, CameraRunning)) return nullptr; return new Request(this); @@ -351,16 +522,18 @@ Request *Camera::createRequest() * automatically after it completes. * * \return 0 on success or a negative error code otherwise + * \retval -ENODEV The camera has been disconnected from the system + * \retval -EACCES The camera is not running so requests can't be queued */ int Camera::queueRequest(Request *request) { - int ret; + if (disconnected_) + return -ENODEV; - ret = exclusiveAccess(); - if (ret) - return ret; + if (!stateIs(CameraRunning)) + return -EACCES; - ret = request->prepare(); + int ret = request->prepare(); if (ret) { LOG(Camera, Error) << "Failed to prepare request"; return ret; @@ -376,17 +549,29 @@ int Camera::queueRequest(Request *request) * can queue requests to the camera to process and return to the application * until the capture session is terminated with \a stop(). * + * This function effects the state of the camera, see \ref camera_operation. + * * \return 0 on success or a negative error code otherwise + * \retval -ENODEV The camera has been disconnected from the system + * \retval -EACCES The camera is not in a state where it can be started */ int Camera::start() { - int ret = exclusiveAccess(); - if (ret) - return ret; + if (disconnected_) + return -ENODEV; + + if (!stateIs(CameraPrepared)) + return -EACCES; LOG(Camera, Debug) << "Starting capture"; - return pipe_->start(this); + int ret = pipe_->start(this); + if (ret) + return ret; + + state_ = CameraRunning; + + return 0; } /** @@ -395,30 +580,27 @@ int Camera::start() * This method stops capturing and processing requests immediately. All pending * requests are cancelled and complete synchronously in an error state. * + * This function effects the state of the camera, see \ref camera_operation. + * * \return 0 on success or a negative error code otherwise + * \retval -ENODEV The camera has been disconnected from the system + * \retval -EACCES The camera is not running so can't be stopped */ int Camera::stop() { - int ret = exclusiveAccess(); - if (ret) - return ret; + if (disconnected_) + return -ENODEV; + + if (!stateIs(CameraRunning)) + return -EACCES; LOG(Camera, Debug) << "Stopping capture"; + state_ = CameraPrepared; + pipe_->stop(this); return 0; } -int Camera::exclusiveAccess() -{ - if (disconnected_) - return -ENODEV; - - if (!acquired_) - return -EACCES; - - return 0; -} - } /* namespace libcamera */
There is a need to better control the order of operations an application performs 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 states are added; Available, Acquired, Configured, Prepared 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 may perform. Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> --- include/libcamera/camera.h | 18 ++- src/libcamera/camera.cpp | 266 +++++++++++++++++++++++++++++++------ 2 files changed, 238 insertions(+), 46 deletions(-)