[libcamera-devel] [PATCH 15/19] libcamera: camera: Centralize state checks in Private class
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Jan 22 21:49:29 CET 2020
Hi Niklas,
On Wed, Jan 22, 2020 at 05:31:28PM +0100, Niklas Söderlund wrote:
> On 2020-01-20 02:24:33 +0200, Laurent Pinchart wrote:
> > Move all accesses to the state_ and disconnected_ members to functions
> > of the Private class. This will make it easier to implement
> > synchronization, and simplifies the Camera class implementation.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
> I really like this patch.
>
> > ---
> > src/libcamera/camera.cpp | 163 ++++++++++++++++-------------
> > src/libcamera/pipeline_handler.cpp | 5 +-
> > 2 files changed, 95 insertions(+), 73 deletions(-)
> >
> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > index 6fe802f375a3..83c2249b8897 100644
> > --- a/src/libcamera/camera.cpp
> > +++ b/src/libcamera/camera.cpp
> > @@ -266,15 +266,21 @@ public:
> >
> > Private(PipelineHandler *pipe, const std::string &name,
> > const std::set<Stream *> &streams);
> > + ~Private();
> >
> > - bool stateBetween(State low, State high) const;
> > - bool stateIs(State state) const;
> > + int isAccessAllowed(State state, bool allowDisconnected = false) const;
> > + int isAccessAllowed(State low, State high,
> > + bool allowDisconnected = false) const;
> > +
> > + void disconnect();
> > + void setState(State state);
> >
> > std::shared_ptr<PipelineHandler> pipe_;
> > std::string name_;
> > std::set<Stream *> streams_;
> > std::set<Stream *> activeStreams_;
> >
> > +private:
> > bool disconnected_;
> > State state_;
> > };
> > @@ -286,6 +292,12 @@ Camera::Private::Private(PipelineHandler *pipe, const std::string &name,
> > {
> > }
> >
> > +Camera::Private::~Private()
> > +{
> > + if (state_ != Private::CameraAvailable)
> > + LOG(Camera, Error) << "Removing camera while still in use";
> > +}
> > +
> > static constexpr std::array<const char *, 4> camera_state_names = {
> > "Available",
> > "Acquired",
> > @@ -293,10 +305,31 @@ static constexpr std::array<const char *, 4> camera_state_names = {
> > "Running",
> > };
> >
> > -bool Camera::Private::stateBetween(State low, State high) const
>
> I wonder if we should document the \retval codes here and then use
> \copydetails from callers in Camera to make sure -ENODEV and -EACCESS is
> uniformly documented.
>
> Or maybe that is not possible as Doxygen ignores libcamera::*::Private.
Yes, that's the issue :-(
> With or without this cherry on top,
>
> Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
>
> > +int Camera::Private::isAccessAllowed(State state, bool allowDisconnected) const
> > {
> > + if (!allowDisconnected && disconnected_)
> > + return -ENODEV;
> > +
> > + if (state_ == state)
> > + return 0;
> > +
> > + ASSERT(static_cast<unsigned int>(state) < camera_state_names.size());
> > +
> > + LOG(Camera, Debug) << "Camera in " << camera_state_names[state_]
> > + << " state trying operation requiring state "
> > + << camera_state_names[state];
> > +
> > + return -EACCES;
> > +}
> > +
> > +int Camera::Private::isAccessAllowed(State low, State high,
> > + bool allowDisconnected) const
> > +{
> > + if (!allowDisconnected && disconnected_)
> > + return -ENODEV;
> > +
> > if (state_ >= low && state_ <= high)
> > - return true;
> > + return 0;
> >
> > ASSERT(static_cast<unsigned int>(low) < camera_state_names.size() &&
> > static_cast<unsigned int>(high) < camera_state_names.size());
> > @@ -306,21 +339,25 @@ bool Camera::Private::stateBetween(State low, State high) const
> > << camera_state_names[low] << " and "
> > << camera_state_names[high];
> >
> > - return false;
> > + return -EACCES;
> > }
> >
> > -bool Camera::Private::stateIs(State state) const
> > +void Camera::Private::disconnect()
> > {
> > - if (state_ == state)
> > - return true;
> > -
> > - ASSERT(static_cast<unsigned int>(state) < camera_state_names.size());
> > + /*
> > + * If the camera was running when the hardware was removed force the
> > + * state to Configured state to allow applications to free resources
> > + * and call release() before deleting the camera.
> > + */
> > + if (state_ == Private::CameraRunning)
> > + state_ = Private::CameraConfigured;
> >
> > - LOG(Camera, Debug) << "Camera in " << camera_state_names[state_]
> > - << " state trying operation requiring state "
> > - << camera_state_names[state];
> > + disconnected_ = true;
> > +}
> >
> > - return false;
> > +void Camera::Private::setState(State state)
> > +{
> > + state_ = state;
> > }
> >
> > /**
> > @@ -468,8 +505,6 @@ Camera::Camera(PipelineHandler *pipe, const std::string &name,
> >
> > Camera::~Camera()
> > {
> > - if (!p_->stateIs(Private::CameraAvailable))
> > - LOG(Camera, Error) << "Removing camera while still in use";
> > }
> >
> > /**
> > @@ -488,23 +523,16 @@ void Camera::disconnect()
> > {
> > LOG(Camera, Debug) << "Disconnecting camera " << name();
> >
> > - /*
> > - * If the camera was running when the hardware was removed force the
> > - * state to Configured state to allow applications to free resources
> > - * and call release() before deleting the camera.
> > - */
> > - if (p_->state_ == Private::CameraRunning)
> > - p_->state_ = Private::CameraConfigured;
> > -
> > - p_->disconnected_ = true;
> > + p_->disconnect();
> > disconnected.emit(this);
> > }
> >
> > int Camera::exportFrameBuffers(Stream *stream,
> > std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> > {
> > - if (!p_->stateIs(Private::CameraConfigured))
> > - return -EACCES;
> > + int ret = p_->isAccessAllowed(Private::CameraConfigured);
> > + if (ret < 0)
> > + return ret;
> >
> > if (streams().find(stream) == streams().end())
> > return -EINVAL;
> > @@ -517,8 +545,9 @@ int Camera::exportFrameBuffers(Stream *stream,
> >
> > int Camera::freeFrameBuffers(Stream *stream)
> > {
> > - if (!p_->stateIs(Private::CameraConfigured))
> > - return -EACCES;
> > + int ret = p_->isAccessAllowed(Private::CameraConfigured, true);
> > + if (ret < 0)
> > + return ret;
> >
> > p_->pipe_->freeFrameBuffers(this, stream);
> >
> > @@ -550,11 +579,9 @@ int Camera::freeFrameBuffers(Stream *stream)
> > */
> > int Camera::acquire()
> > {
> > - if (p_->disconnected_)
> > - return -ENODEV;
> > -
> > - if (!p_->stateIs(Private::CameraAvailable))
> > - return -EBUSY;
> > + int ret = p_->isAccessAllowed(Private::CameraAvailable);
> > + if (ret < 0)
> > + return ret == -EACCES ? -EBUSY : ret;
> >
> > if (!p_->pipe_->lock()) {
> > LOG(Camera, Info)
> > @@ -562,7 +589,7 @@ int Camera::acquire()
> > return -EBUSY;
> > }
> >
> > - p_->state_ = Private::CameraAcquired;
> > + p_->setState(Private::CameraAcquired);
> >
> > return 0;
> > }
> > @@ -580,9 +607,10 @@ int Camera::acquire()
> > */
> > int Camera::release()
> > {
> > - if (!p_->stateBetween(Private::CameraAvailable,
> > - Private::CameraConfigured))
> > - return -EBUSY;
> > + int ret = p_->isAccessAllowed(Private::CameraAvailable,
> > + Private::CameraConfigured, true);
> > + if (ret < 0)
> > + return ret == -EACCES ? -EBUSY : ret;
> >
> > if (allocator_) {
> > /*
> > @@ -596,7 +624,7 @@ int Camera::release()
> >
> > p_->pipe_->unlock();
> >
> > - p_->state_ = Private::CameraAvailable;
> > + p_->setState(Private::CameraAvailable);
> >
> > return 0;
> > }
> > @@ -643,7 +671,12 @@ const std::set<Stream *> &Camera::streams() const
> > */
> > std::unique_ptr<CameraConfiguration> Camera::generateConfiguration(const StreamRoles &roles)
> > {
> > - if (p_->disconnected_ || roles.size() > streams().size())
> > + int ret = p_->isAccessAllowed(Private::CameraAvailable,
> > + Private::CameraRunning);
> > + if (ret < 0)
> > + return nullptr;
> > +
> > + if (roles.size() > streams().size())
> > return nullptr;
> >
> > CameraConfiguration *config = p_->pipe_->generateConfiguration(this, roles);
> > @@ -694,14 +727,10 @@ std::unique_ptr<CameraConfiguration> Camera::generateConfiguration(const StreamR
> > */
> > int Camera::configure(CameraConfiguration *config)
> > {
> > - int ret;
> > -
> > - if (p_->disconnected_)
> > - return -ENODEV;
> > -
> > - if (!p_->stateBetween(Private::CameraAcquired,
> > - Private::CameraConfigured))
> > - return -EACCES;
> > + int ret = p_->isAccessAllowed(Private::CameraAcquired,
> > + Private::CameraConfigured);
> > + if (ret < 0)
> > + return ret;
> >
> > if (allocator_ && allocator_->allocated()) {
> > LOG(Camera, Error)
> > @@ -740,7 +769,7 @@ int Camera::configure(CameraConfiguration *config)
> > p_->activeStreams_.insert(stream);
> > }
> >
> > - p_->state_ = Private::CameraConfigured;
> > + p_->setState(Private::CameraConfigured);
> >
> > return 0;
> > }
> > @@ -767,9 +796,9 @@ int Camera::configure(CameraConfiguration *config)
> > */
> > Request *Camera::createRequest(uint64_t cookie)
> > {
> > - if (p_->disconnected_ ||
> > - !p_->stateBetween(Private::CameraConfigured,
> > - Private::CameraRunning))
> > + int ret = p_->isAccessAllowed(Private::CameraConfigured,
> > + Private::CameraRunning);
> > + if (ret < 0)
> > return nullptr;
> >
> > return new Request(this, cookie);
> > @@ -799,11 +828,9 @@ Request *Camera::createRequest(uint64_t cookie)
> > */
> > int Camera::queueRequest(Request *request)
> > {
> > - if (p_->disconnected_)
> > - return -ENODEV;
> > -
> > - if (!p_->stateIs(Private::CameraRunning))
> > - return -EACCES;
> > + int ret = p_->isAccessAllowed(Private::CameraRunning);
> > + if (ret < 0)
> > + return ret;
> >
> > if (request->buffers().empty()) {
> > LOG(Camera, Error) << "Request contains no buffers";
> > @@ -837,11 +864,9 @@ int Camera::queueRequest(Request *request)
> > */
> > int Camera::start()
> > {
> > - if (p_->disconnected_)
> > - return -ENODEV;
> > -
> > - if (!p_->stateIs(Private::CameraConfigured))
> > - return -EACCES;
> > + int ret = p_->isAccessAllowed(Private::CameraConfigured);
> > + if (ret < 0)
> > + return ret;
> >
> > LOG(Camera, Debug) << "Starting capture";
> >
> > @@ -852,11 +877,11 @@ int Camera::start()
> > p_->pipe_->importFrameBuffers(this, stream);
> > }
> >
> > - int ret = p_->pipe_->start(this);
> > + ret = p_->pipe_->start(this);
> > if (ret)
> > return ret;
> >
> > - p_->state_ = Private::CameraRunning;
> > + p_->setState(Private::CameraRunning);
> >
> > return 0;
> > }
> > @@ -875,15 +900,13 @@ int Camera::start()
> > */
> > int Camera::stop()
> > {
> > - if (p_->disconnected_)
> > - return -ENODEV;
> > -
> > - if (!p_->stateIs(Private::CameraRunning))
> > - return -EACCES;
> > + int ret = p_->isAccessAllowed(Private::CameraRunning);
> > + if (ret < 0)
> > + return ret;
> >
> > LOG(Camera, Debug) << "Stopping capture";
> >
> > - p_->state_ = Private::CameraConfigured;
> > + p_->setState(Private::CameraConfigured);
> >
> > p_->pipe_->stop(this);
> >
> > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> > index 669097f609ab..3091971d5da0 100644
> > --- a/src/libcamera/pipeline_handler.cpp
> > +++ b/src/libcamera/pipeline_handler.cpp
> > @@ -314,7 +314,7 @@ const ControlInfoMap &PipelineHandler::controls(Camera *camera)
> > * exportFrameBuffers() and importFrameBuffers() for the streams contained in
> > * any camera configuration.
> > *
> > - * The only intended caller is the FrameBufferAllocator helper.
> > + * The only intended caller is Camera::exportFrameBuffers().
> > *
> > * \return 0 on success or a negative error code otherwise
> > */
> > @@ -357,8 +357,7 @@ const ControlInfoMap &PipelineHandler::controls(Camera *camera)
> > * called only after a successful call to either of these two methods, and only
> > * once per stream.
> > *
> > - * The only intended callers are Camera::stop() and the FrameBufferAllocator
> > - * helper.
> > + * The only intended callers are Camera::stop() and Camera::freeFrameBuffers().
> > */
> >
> > /**
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list