[libcamera-devel] [PATCH v3 8/8] android: Implement flush() camera operation
Niklas Söderlund
niklas.soderlund at ragnatech.se
Sat May 22 11:55:36 CEST 2021
Hi Jacopo,
Thanks for your work.
On 2021-05-21 17:42:27 +0200, Jacopo Mondi wrote:
> Implement the flush() camera operation in the CameraDevice class
> and make it available to the camera framework by implementing the
> operation wrapper in camera_ops.cpp.
>
> The flush() implementation stops the Camera and the worker thread and
> waits for all in-flight requests to be returned. Stopping the Camera
> forces all Requests already queued to be returned immediately in error
> state. As flush() has to wait until all of them have been returned, make it
> wait on a newly introduced condition variable which is notified by the
> request completion handler when the queue of pending requests has been
> exhausted.
>
> As flush() can race with processCaptureRequest() protect the requests
> queueing by introducing a new CameraState::CameraFlushing state that
> processCaptureRequest() inspects before queuing the Request to the
> Camera. If flush() has been called while processCaptureRequest() was
> executing, return the current Request immediately in error state.
>
> Protect potentially concurrent calls to close() and configureStreams()
> by inspecting the CameraState, and force a wait for any flush() call
> to complete before proceeding.
>
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
> src/android/camera_device.cpp | 90 +++++++++++++++++++++++++++++++++--
> src/android/camera_device.h | 9 +++-
> src/android/camera_ops.cpp | 8 +++-
> 3 files changed, 100 insertions(+), 7 deletions(-)
>
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index 3fce14035718..899afaa49439 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -750,16 +750,65 @@ int CameraDevice::open(const hw_module_t *hardwareModule)
>
> void CameraDevice::close()
> {
> - streams_.clear();
> + MutexLocker cameraLock(cameraMutex_);
> + if (state_ == CameraFlushing) {
> + flushed_.wait(cameraLock, [&] { return state_ != CameraStopped; });
> + camera_->release();
>
> + return;
> + }
> +
> + streams_.clear();
> stop();
>
> camera_->release();
> }
>
> -void CameraDevice::stop()
> +/*
> + * Flush is similar to stop() but sets the camera state to 'flushing' and wait
> + * until all the in-flight requests have been returned before setting the
> + * camera state to stopped.
> + *
> + * Once flushing is done it unlocks concurrent calls to camera close() and
> + * configureStreams().
> + */
> +void CameraDevice::flush()
> {
> + {
> + MutexLocker cameraLock(cameraMutex_);
> +
> + if (state_ != CameraRunning)
> + return;
> +
> + worker_.stop();
> + camera_->stop();
> + state_ = CameraFlushing;
> + }
> +
> + /*
> + * Now wait for all the in-flight requests to be completed before
> + * continuing. Stopping the Camera guarantees that all in-flight
> + * requests are completed in error state.
> + */
> + {
> + MutexLocker requestsLock(requestsMutex_);
> + flushing_.wait(requestsLock, [&] { return descriptors_.empty(); });
> + }
I'm still uneasy about releasing the cameraMutex_ for this section. In
patch 6/8 you add it to protect the state_ variable but here it's
ignored. I see the ASSERT() added to stop() but the patter of taking the
lock checking state_, releasing the lock and do some work, retake the
lock and update state_ feels like a bad idea. Maybe I'm missing
something and this is not a real problem, if so maybe we can capture
that in the comment here?
> +
> + /*
> + * Set state to stopped and unlock close() or configureStreams() that
> + * might be waiting for flush to be completed.
> + */
> MutexLocker cameraLock(cameraMutex_);
> + state_ = CameraStopped;
> + flushed_.notify_one();
> +}
> +
> +/* Calls to stop() must be protected by cameraMutex_ being held by the caller. */
> +void CameraDevice::stop()
> +{
> + ASSERT(state_ != CameraFlushing);
> +
> if (state_ == CameraStopped)
> return;
>
> @@ -1581,8 +1630,18 @@ PixelFormat CameraDevice::toPixelFormat(int format) const
> */
> int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> {
> - /* Before any configuration attempt, stop the camera. */
> - stop();
> + {
> + /*
> + * If a flush is in progress, wait for it to complete and to
> + * stop the camera, otherwise before any new configuration
> + * attempt we have to stop the camera explictely.
> + */
> + MutexLocker cameraLock(cameraMutex_);
> + if (state_ == CameraFlushing)
> + flushed_.wait(cameraLock, [&] { return state_ != CameraStopped; });
> + else
> + stop();
> + }
>
> if (stream_list->num_streams == 0) {
> LOG(HAL, Error) << "No streams in configuration";
> @@ -1950,6 +2009,25 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> if (ret)
> return ret;
>
> + /*
> + * Just before queuing the request, make sure flush() has not
> + * been called after this function has been executed. In that
> + * case, immediately return the request with errors.
> + */
> + MutexLocker cameraLock(cameraMutex_);
> + if (state_ == CameraFlushing || state_ == CameraStopped) {
> + for (camera3_stream_buffer_t &buffer : descriptor.buffers_) {
> + buffer.status = CAMERA3_BUFFER_STATUS_ERROR;
> + buffer.release_fence = buffer.acquire_fence;
> + }
> +
> + notifyError(descriptor.frameNumber_,
> + descriptor.buffers_[0].stream,
> + CAMERA3_MSG_ERROR_REQUEST);
> +
> + return 0;
> + }
> +
> worker_.queueRequest(descriptor.request_.get());
>
> {
> @@ -1979,6 +2057,10 @@ void CameraDevice::requestComplete(Request *request)
> return;
> }
>
> + /* Release flush if all the pending requests have been completed. */
> + if (descriptors_.empty())
> + flushing_.notify_one();
> +
> node = descriptors_.extract(it);
> }
> Camera3RequestDescriptor &descriptor = node.mapped();
> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> index 7cf8e8370387..e1b3bf7d30f2 100644
> --- a/src/android/camera_device.h
> +++ b/src/android/camera_device.h
> @@ -7,6 +7,7 @@
> #ifndef __ANDROID_CAMERA_DEVICE_H__
> #define __ANDROID_CAMERA_DEVICE_H__
>
> +#include <condition_variable>
> #include <map>
> #include <memory>
> #include <mutex>
> @@ -42,6 +43,7 @@ public:
>
> int open(const hw_module_t *hardwareModule);
> void close();
> + void flush();
>
> unsigned int id() const { return id_; }
> camera3_device_t *camera3Device() { return &camera3Device_; }
> @@ -92,6 +94,7 @@ private:
> enum State {
> CameraStopped,
> CameraRunning,
> + CameraFlushing,
> };
>
> void stop();
> @@ -120,8 +123,9 @@ private:
>
> CameraWorker worker_;
>
> - libcamera::Mutex cameraMutex_; /* Protects access to the camera state. */
> + libcamera::Mutex cameraMutex_; /* Protects the camera state and flushed_. */
> State state_;
> + std::condition_variable flushed_;
>
> std::shared_ptr<libcamera::Camera> camera_;
> std::unique_ptr<libcamera::CameraConfiguration> config_;
> @@ -134,8 +138,9 @@ private:
> std::map<int, libcamera::PixelFormat> formatsMap_;
> std::vector<CameraStream> streams_;
>
> - libcamera::Mutex requestsMutex_; /* Protects descriptors_. */
> + libcamera::Mutex requestsMutex_; /* Protects descriptors_ and flushing_. */
> std::map<uint64_t, Camera3RequestDescriptor> descriptors_;
> + std::condition_variable flushing_;
>
> std::string maker_;
> std::string model_;
> diff --git a/src/android/camera_ops.cpp b/src/android/camera_ops.cpp
> index 696e80436821..8a3cfa175ff5 100644
> --- a/src/android/camera_ops.cpp
> +++ b/src/android/camera_ops.cpp
> @@ -66,8 +66,14 @@ static void hal_dev_dump([[maybe_unused]] const struct camera3_device *dev,
> {
> }
>
> -static int hal_dev_flush([[maybe_unused]] const struct camera3_device *dev)
> +static int hal_dev_flush(const struct camera3_device *dev)
> {
> + if (!dev)
> + return -EINVAL;
> +
> + CameraDevice *camera = reinterpret_cast<CameraDevice *>(dev->priv);
> + camera->flush();
> +
> return 0;
> }
>
> --
> 2.31.1
>
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list