[libcamera-devel] [PATCH 30/30] libcamera: camera: Remove the prepared state
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sun Dec 15 10:18:14 CET 2019
Hi Niklas,
A couple of nother comments.
On Sun, Dec 15, 2019 at 11:12:35AM +0200, Laurent Pinchart wrote:
> On Wed, Nov 27, 2019 at 12:36:20AM +0100, Niklas Söderlund wrote:
> > With the FrameBuffer rework completed there is no reason to keep the
> > camera prepared state around as buffer allocations are now decoupled
> > from the camera state. Remove the camera state simplifying the API.
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > ---
> > include/libcamera/camera.h | 4 --
> > src/android/camera_device.cpp | 10 +---
> > src/cam/capture.cpp | 8 ---
> > src/libcamera/camera.cpp | 104 ++++++++--------------------------
> > src/qcam/main_window.cpp | 9 ---
> > test/camera/buffer_import.cpp | 10 ----
> > test/camera/capture.cpp | 10 ----
> > test/camera/statemachine.cpp | 83 ---------------------------
> > 8 files changed, 26 insertions(+), 212 deletions(-)
> >
> > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> > index 02d047b9649f53d0..9f542aab213c5d47 100644
> > --- a/include/libcamera/camera.h
> > +++ b/include/libcamera/camera.h
> > @@ -90,9 +90,6 @@ public:
> > std::unique_ptr<CameraConfiguration> generateConfiguration(const StreamRoles &roles);
> > int configure(CameraConfiguration *config);
> >
> > - int allocateBuffers();
> > - int freeBuffers();
> > -
> > Request *createRequest(uint64_t cookie = 0);
> > int queueRequest(Request *request);
> >
> > @@ -104,7 +101,6 @@ private:
> > CameraAvailable,
> > CameraAcquired,
> > CameraConfigured,
> > - CameraPrepared,
> > CameraRunning,
> > };
> >
> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > index a6da658bec277933..34c6119228a9ac0e 100644
> > --- a/src/android/camera_device.cpp
> > +++ b/src/android/camera_device.cpp
> > @@ -78,7 +78,6 @@ void CameraDevice::close()
> > {
> > camera_->stop();
> >
>
> I think you can remove this blank line too.
>
> > - camera_->freeBuffers();
> > camera_->release();
> >
> > running_ = false;
> > @@ -690,16 +689,9 @@ void CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reque
> >
> > /* Start the camera if that's the first request we handle. */
> > if (!running_) {
> > - int ret = camera_->allocateBuffers();
> > - if (ret) {
> > - LOG(HAL, Error) << "Failed to allocate buffers";
> > - return;
> > - }
> > -
> > - ret = camera_->start();
> > + int ret = camera_->start();
> > if (ret) {
> > LOG(HAL, Error) << "Failed to start camera";
> > - camera_->freeBuffers();
> > return;
> > }
> >
> > diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp
> > index 154e6e461ee2b96b..835943a7d2b82e79 100644
> > --- a/src/cam/capture.cpp
> > +++ b/src/cam/capture.cpp
> > @@ -42,12 +42,6 @@ int Capture::run(EventLoop *loop, const OptionsParser::Options &options)
> > return ret;
> > }
> >
> > - ret = camera_->allocateBuffers();
> > - if (ret) {
> > - std::cerr << "Failed to allocate buffers" << std::endl;
> > - return ret;
> > - }
> > -
> > camera_->requestCompleted.connect(this, &Capture::requestComplete);
> >
> > if (options.isSet(OptFile)) {
> > @@ -67,8 +61,6 @@ int Capture::run(EventLoop *loop, const OptionsParser::Options &options)
> > writer_ = nullptr;
> > }
> >
> > - camera_->freeBuffers();
> > -
> > return ret;
> > }
> >
> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > index 12878a1c2e6076d3..fca75040ad8b16e7 100644
> > --- a/src/libcamera/camera.cpp
> > +++ b/src/libcamera/camera.cpp
> > @@ -274,15 +274,13 @@ std::size_t CameraConfiguration::size() const
> > * \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 need to be freed
> > - * and the camera released.
> > + * it is ready to process requests. The camera needs to be acquired and
> > + * configured 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 it needs to be released.
>
> "it" as ambiguous here as it could refer to the application or the
> camera. I would write "When an application is done with a camera, the
> camera needs to be 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.
> > + * not released. The camera may also be reconfigured.
> > *
> > * \subsection Camera States
> > *
> > @@ -296,7 +294,6 @@ std::size_t CameraConfiguration::size() const
> > * node [shape = doublecircle ]; Available;
> > * node [shape = circle ]; Acquired;
> > * node [shape = circle ]; Configured;
> > - * node [shape = circle ]; Prepared;
> > * node [shape = circle ]; Running;
> > *
> > * Available -> Available [label = "release()"];
> > @@ -306,14 +303,10 @@ std::size_t CameraConfiguration::size() const
> > * Acquired -> Configured [label = "configure()"];
> > *
> > * Configured -> Available [label = "release()"];
> > - * Configured -> Configured [label = "configure()"];
> > - * Configured -> Prepared [label = "allocateBuffers()"];
> > + * Configured -> Configured [label = "configure(), createRequest()"];
> > + * Configured -> Running [label = "start()"];
> > *
> > - * Prepared -> Configured [label = "freeBuffers()"];
> > - * Prepared -> Prepared [label = "createRequest()"];
> > - * Prepared -> Running [label = "start()"];
> > - *
> > - * Running -> Prepared [label = "stop()"];
> > + * Running -> Configured [label = "stop()"];
> > * Running -> Running [label = "createRequest(), queueRequest()"];
> > * }
> > * \enddot
> > @@ -329,19 +322,14 @@ std::size_t CameraConfiguration::size() const
> > * 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.
> > + * The camera has been configured and is ready to be started. The application
>
> "is configured and ready to be started" to be consistent with the other
> states ?
>
> These minor issues apart,
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
> > + * may release() the camera and to get back to the Available 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.
> > + * to the Configured state.
> > */
> >
> > /**
> > @@ -419,7 +407,6 @@ static const char *const camera_state_names[] = {
> > "Available",
> > "Acquired",
> > "Configured",
> > - "Prepared",
> > "Running",
> > };
> >
> > @@ -473,11 +460,11 @@ void Camera::disconnect()
There's a todo item for this method stating
* \todo Update comment about Running state when importing buffers as well as
* allocating them are supported.
I'm not sure what it means. Is it still valid ?
> >
> > /*
> > * 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.
> > + * state to Configured state to allow applications to free resources
> > + * and release() before deleting the camera.
s/release()/call release()/
and are there any resources to be freed ?
> > */
> > if (state_ == CameraRunning)
> > - state_ = CameraPrepared;
> > + state_ = CameraConfigured;
> >
> > disconnected_ = true;
> > disconnected.emit(this);
> > @@ -685,53 +672,6 @@ int Camera::configure(CameraConfiguration *config)
> > return 0;
> > }
> >
> > -/**
> > - * \brief Allocate buffers for all configured streams
> > - *
> > - * This function affects 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()
> > -{
> > - if (disconnected_)
> > - return -ENODEV;
> > -
> > - if (!stateIs(CameraConfigured))
> > - return -EACCES;
> > -
> > - if (activeStreams_.empty()) {
> > - LOG(Camera, Error)
> > - << "Can't allocate buffers without streams";
> > - return -EINVAL;
> > - }
> > -
> > - state_ = CameraPrepared;
> > -
> > - return 0;
> > -}
> > -
> > -/**
> > - * \brief Release all buffers from allocated pools in each stream
> > - *
> > - * This function affects 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
> > - */
> > -int Camera::freeBuffers()
> > -{
> > - if (!stateIs(CameraPrepared))
> > - return -EACCES;
> > -
> > - state_ = CameraConfigured;
> > -
> > - return 0;
> > -}
> > -
> > /**
> > * \brief Create a request object for the camera
> > * \param[in] cookie Opaque cookie for application use
> > @@ -747,14 +687,14 @@ int Camera::freeBuffers()
> > * The ownership of the returned request is passed to the caller, which is
> > * responsible for either queueing the request or deleting it.
> > *
> > - * This function shall only be called when the camera is in the Prepared
> > + * This function shall only be called when the camera is in the Configured
> > * or Running state, see \ref camera_operation.
> > *
> > * \return A pointer to the newly created request, or nullptr on error
> > */
> > Request *Camera::createRequest(uint64_t cookie)
> > {
> > - if (disconnected_ || !stateBetween(CameraPrepared, CameraRunning))
> > + if (disconnected_ || !stateBetween(CameraConfigured, CameraRunning))
> > return nullptr;
> >
> > return new Request(this, cookie);
> > @@ -828,9 +768,15 @@ int Camera::start()
> > if (disconnected_)
> > return -ENODEV;
> >
> > - if (!stateIs(CameraPrepared))
> > + if (!stateIs(CameraConfigured))
> > return -EACCES;
> >
> > + if (activeStreams_.empty()) {
> > + LOG(Camera, Error)
> > + << "Can't start camera without streams";
> > + return -EINVAL;
> > + }
> > +
> > for (Stream *stream : activeStreams_) {
> > ret = stream->start();
> > if (ret < 0)
> > @@ -870,7 +816,7 @@ int Camera::stop()
> >
> > LOG(Camera, Debug) << "Stopping capture";
> >
> > - state_ = CameraPrepared;
> > + state_ = CameraConfigured;
> >
> > pipe_->stop(this);
> >
> > diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
> > index 9650bd970f89aa41..38cf79deed37a089 100644
> > --- a/src/qcam/main_window.cpp
> > +++ b/src/qcam/main_window.cpp
> > @@ -172,13 +172,6 @@ int MainWindow::startCapture()
> >
> > adjustSize();
> >
> > - ret = camera_->allocateBuffers();
> > - if (ret) {
> > - std::cerr << "Failed to allocate buffers"
> > - << std::endl;
> > - return ret;
> > - }
> > -
> > ret = allocator_->allocate(stream, cfg);
> > if (ret < 0) {
> > std::cerr << "Failed to allocate internal buffers" << std::endl;
> > @@ -230,7 +223,6 @@ error:
> > for (Request *request : requests)
> > delete request;
> >
> > - camera_->freeBuffers();
> > return ret;
> > }
> >
> > @@ -243,7 +235,6 @@ void MainWindow::stopCapture()
> > if (ret)
> > std::cout << "Failed to stop capture" << std::endl;
> >
> > - camera_->freeBuffers();
> > isCapturing_ = false;
> >
> > config_.reset();
> > diff --git a/test/camera/buffer_import.cpp b/test/camera/buffer_import.cpp
> > index c98afeb9597e21c2..9ae364d44d63b4d5 100644
> > --- a/test/camera/buffer_import.cpp
> > +++ b/test/camera/buffer_import.cpp
> > @@ -174,11 +174,6 @@ protected:
> > return TestFail;
> > }
> >
> > - if (camera_->allocateBuffers()) {
> > - cout << "Failed to allocate buffers" << endl;
> > - return TestFail;
> > - }
> > -
> > Stream *stream = cfg.stream();
> >
> > BufferSource source;
> > @@ -246,11 +241,6 @@ protected:
> > return TestFail;
> > }
> >
> > - if (camera_->freeBuffers()) {
> > - cout << "Failed to free buffers" << endl;
> > - return TestFail;
> > - }
> > -
> > return TestPass;
> > }
> >
> > diff --git a/test/camera/capture.cpp b/test/camera/capture.cpp
> > index ee6d0a1bc1fa839d..545496fe2f7655ea 100644
> > --- a/test/camera/capture.cpp
> > +++ b/test/camera/capture.cpp
> > @@ -80,11 +80,6 @@ protected:
> > return TestFail;
> > }
> >
> > - if (camera_->allocateBuffers()) {
> > - cout << "Failed to allocate buffers" << endl;
> > - return TestFail;
> > - }
> > -
> > Stream *stream = cfg.stream();
> >
> > BufferAllocator allocator(camera_);
> > @@ -152,11 +147,6 @@ protected:
> > return TestFail;
> > }
> >
> > - if (camera_->freeBuffers()) {
> > - cout << "Failed to free buffers" << endl;
> > - return TestFail;
> > - }
> > -
> > return TestPass;
> > }
> >
> > diff --git a/test/camera/statemachine.cpp b/test/camera/statemachine.cpp
> > index e9468d806f977a63..7d409ca20689568b 100644
> > --- a/test/camera/statemachine.cpp
> > +++ b/test/camera/statemachine.cpp
> > @@ -29,12 +29,6 @@ protected:
> > if (camera_->configure(defconf_.get()) != -EACCES)
> > return TestFail;
> >
> > - if (camera_->allocateBuffers() != -EACCES)
> > - return TestFail;
> > -
> > - if (camera_->freeBuffers() != -EACCES)
> > - return TestFail;
> > -
> > if (camera_->createRequest())
> > return TestFail;
> >
> > @@ -65,12 +59,6 @@ protected:
> > if (camera_->acquire() != -EBUSY)
> > return TestFail;
> >
> > - if (camera_->allocateBuffers() != -EACCES)
> > - return TestFail;
> > -
> > - if (camera_->freeBuffers() != -EACCES)
> > - return TestFail;
> > -
> > if (camera_->createRequest())
> > return TestFail;
> >
> > @@ -103,57 +91,6 @@ protected:
> > if (camera_->acquire() != -EBUSY)
> > return TestFail;
> >
> > - if (camera_->freeBuffers() != -EACCES)
> > - return TestFail;
> > -
> > - if (camera_->createRequest())
> > - return TestFail;
> > -
> > - Request request(camera_.get());
> > - if (camera_->queueRequest(&request) != -EACCES)
> > - return TestFail;
> > -
> > - if (camera_->start() != -EACCES)
> > - return TestFail;
> > -
> > - if (camera_->stop() != -EACCES)
> > - return TestFail;
> > -
> > - /* Test operations which should pass. */
> > - if (camera_->configure(defconf_.get()))
> > - return TestFail;
> > -
> > - /* Test valid state transitions, end in Prepared state. */
> > - if (camera_->release())
> > - return TestFail;
> > -
> > - if (camera_->acquire())
> > - return TestFail;
> > -
> > - if (camera_->configure(defconf_.get()))
> > - return TestFail;
> > -
> > - if (camera_->allocateBuffers())
> > - return TestFail;
> > -
> > - return TestPass;
> > - }
> > -
> > - int testPrepared()
> > - {
> > - /* Test operations which should fail. */
> > - if (camera_->acquire() != -EBUSY)
> > - return TestFail;
> > -
> > - if (camera_->release() != -EBUSY)
> > - return TestFail;
> > -
> > - if (camera_->configure(defconf_.get()) != -EACCES)
> > - return TestFail;
> > -
> > - if (camera_->allocateBuffers() != -EACCES)
> > - return TestFail;
> > -
> > Request request1(camera_.get());
> > if (camera_->queueRequest(&request1) != -EACCES)
> > return TestFail;
> > @@ -170,9 +107,6 @@ protected:
> > delete request2;
> >
> > /* Test valid state transitions, end in Running state. */
> > - if (camera_->freeBuffers())
> > - return TestFail;
> > -
> > if (camera_->release())
> > return TestFail;
> >
> > @@ -182,9 +116,6 @@ protected:
> > if (camera_->configure(defconf_.get()))
> > return TestFail;
> >
> > - if (camera_->allocateBuffers())
> > - return TestFail;
> > -
> > /* Use internally allocated buffers. */
> > allocator_ = new BufferAllocator(camera_);
> > Stream *stream = *camera_->streams().begin();
> > @@ -209,12 +140,6 @@ protected:
> > if (camera_->configure(defconf_.get()) != -EACCES)
> > return TestFail;
> >
> > - if (camera_->allocateBuffers() != -EACCES)
> > - return TestFail;
> > -
> > - if (camera_->freeBuffers() != -EACCES)
> > - return TestFail;
> > -
> > if (camera_->start() != -EACCES)
> > return TestFail;
> >
> > @@ -236,9 +161,6 @@ protected:
> >
> > delete allocator_;
> >
> > - if (camera_->freeBuffers())
> > - return TestFail;
> > -
> > if (camera_->release())
> > return TestFail;
> >
> > @@ -276,11 +198,6 @@ protected:
> > return TestFail;
> > }
> >
> > - if (testPrepared() != TestPass) {
> > - cout << "State machine in Prepared state failed" << endl;
> > - return TestFail;
> > - }
> > -
> > if (testRuning() != TestPass) {
> > cout << "State machine in Running state failed" << endl;
> > return TestFail;
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list