| Message ID | 20191126233620.1695316-31-niklas.soderlund@ragnatech.se |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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@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@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;
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@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@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;
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(); - 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. * * 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 + * 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;
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@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(-)