[libcamera-devel] [PATCH 30/30] libcamera: camera: Remove the prepared state
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sun Dec 15 10:12:35 CET 2019
Hi Niklas,
Thank you for the patch.
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()
>
> /*
> * 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.
> */
> 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