| Message ID | 20190226021857.28255-6-niklas.soderlund@ragnatech.se |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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@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@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Jacopo, Thanks for your feedback. On 2019-02-26 18:13:43 +0100, Jacopo Mondi wrote: > 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. In my mind it captures all current interactions with the camera and covers our discussions in Brussels. I agree it might be extended in the future to also include a configured state. For now I feel this covers the behavior discussed and agreed upon. > > 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). This seems messy and hard for applications to use. We might need it at some point, but for now all this change is make the current behavior a bit more strict. If we need this I say we should do this on top of this change. > > 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? Yes. However with all patches in this series applied more checks are added then just simply converting the existing ones to the state machine logic. But as you point out above even more checks might be needed and added in the future. > > Thanks > j > > > Signed-off-by: Niklas Söderlund <niklas.soderlund@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@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Niklas, Thank you for the patch. 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 s/perform/performs/ > 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 s/have/has/ > application shall do at this point is clean up and release the camera so > it can be removed from libcamera as well. I think you'll have to keep the connection status separate from the state, otherwise an application won't be able to free buffers for a disconnected camera. Disconnection should be separate from the state machine related to the operations performed by the application, but of course needs to be taken into account when checking whether a function can be called or not. I know that the current implementation doesn't handle this correctly either, but I think we should fix it instead of moving the incorrect behaviour to the new state machine implementation. > * 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 s/wish/wishes/ > application wish to use a camera it should acquire it to proceed to the s/wish/wishes/ > next state. > > * Acquired > When an application have acquired a camera it have exclusive access to s/have/has/g > it and can modify the cameras parameters to prepare it for capturing. s/cameras/camera's/ > Once the application is done configure the camera it may be started to s/configure/configuring/ > 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 s/finish/finishes/ > camera shall be stopped to change the state to acquired where it can be > reconfigured or released. All this should be captured in the documentation, with a state diagram (probably as part of the Camera \class documentation). You can find an example of how to embed dot diagrams in Doxygen at http://www.doxygen.nl/manual/commands.html#cmddot. > Signed-off-by: Niklas Söderlund <niklas.soderlund@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, That's pretty generic and could lead to namespace collision. How about prefixing these state names with Camera ? > + }; > + > 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 " s/requiering/requiring/ Is this an error, or could it be a warning ? Or possibly even a debug mesage ? Are there cases when an application could try to call an operation to check whether access is possible during the course of normal operation ? I think that would be the case of the acquire() function in particular. Same comment for the next function. It would make sense to make the camera Loggable to print its name, but that would require exposing the log.h header, which is likely not a good idea. I think we can postpone this until we implement a Camera facade object. > + << 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 " s/requiering/requiring/ > + << 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; Would it make sense to allow release() to be called on a Free camera, to simplify error handling in the application ? > } > > /** > @@ -191,6 +225,9 @@ void Camera::release() > */ > const std::vector<Stream *> &Camera::streams() const > { > + if (!stateIsAtleast(Free)) > + std::vector<Stream *>{}; Missing return ? This will lead you to another problem, which is that you return a reference to a local object. There are multiple ways to solve this, but I think you can drop the state check for this function, as streams have the same lifetime as the camera, there's no need to restrict the stream() function. > + > 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; I agree with Jacopo that a Configured state is likely needed. > > 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; > + This allows calling freeBuffers() even when no buffers have been allocated. I think it's a good idea, but it calls for allowing release() on a released camera, as mentioned above. > 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; > }
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; }
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. Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> --- include/libcamera/camera.h | 14 ++++- src/libcamera/camera.cpp | 110 +++++++++++++++++++++++-------------- 2 files changed, 80 insertions(+), 44 deletions(-)