[libcamera-devel] [PATCH v2 8/8] android: Implement flush() camera operation
Hirokazu Honda
hiroh at chromium.org
Mon May 17 07:51:45 CEST 2021
Hi Jacopo, thank you for the patch.
On Thu, May 13, 2021 at 6:22 PM Jacopo Mondi <jacopo at jmondi.org> 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.
>
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
> src/android/camera_device.cpp | 91 +++++++++++++++++++++++++++++++++--
> src/android/camera_device.h | 9 +++-
> src/android/camera_ops.cpp | 8 ++-
> 3 files changed, 102 insertions(+), 6 deletions(-)
>
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index 7f8c9bd7832d..6d6730a50ec7 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -750,16 +750,64 @@ int CameraDevice::open(const hw_module_t
*hardwareModule)
>
> void CameraDevice::close()
> {
> - streams_.clear();
> + MutexLocker cameraLock(cameraMutex_);
> + if (state_ == CameraFlushing) {
> + MutexLocker flushLock(flushMutex_);
> + flushed_.wait(flushLock, [&] { return state_ !=
CameraStopped; });
> + camera_->release();
>
> + return;
> + }
> +
> + streams_.clear();
> stop();
>
> camera_->release();
> }
>
> +/*
> + * 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 camera close() calls or
new
> + * camera configurations.
> + */
> +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(); });
> + }
> +
> + /*
> + * Unlock close() or configureStreams() that might be waiting for
> + * flush to be completed.
> + */
> + MutexLocker flushLocker(flushMutex_);
I found state_ is touched without cameraMutex_ here and the condition
variable of flushed_.wait().
I suspect that flushMutex_ is unnecessary and should be replaced with
cameraMutex_.
>
> + state_ = CameraStopped;
> + flushed_.notify_one();
> +}
> +
> +/* Calls to stop() must be protected by cameraMutex_ being held by the
caller. */
> void CameraDevice::stop()
> {
> - MutexLocker cameraLock(cameraMutex_);
> if (state_ == CameraStopped)
> return;
>
> @@ -1652,8 +1700,20 @@ 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) {
> + MutexLocker flushLock(flushMutex_);
> + flushed_.wait(flushLock, [&] { return state_ !=
CameraStopped; });
> + } else {
> + stop();
> + }
> + }
I wonder if we should hold cameraMutex_ entirely in configureStreams()
because we are touching streams_ and camera_, which are also accessed by
flush() and stop().
>
>
> if (stream_list->num_streams == 0) {
> LOG(HAL, Error) << "No streams in configuration";
CaptureRequest, which is constructed through Camera3RequestDescriptor.
CaptureRequest::queue() calls camera_->queueRequest(). It is necessary to
make sure camera_ is valid at that time.
It seems to be difficult because it is a different thread and therefore we
don't have any idea when it is called.
Although I think https://patchwork.libcamera.org/patch/12089 helps our
situation.
>
> @@ -2021,6 +2081,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());
>
> {
> @@ -2050,6 +2129,10 @@ void CameraDevice::requestComplete(Request
*request)
> return;
> }
>
> + /* Release flush if all the pending requests have been
completed. */
> + if (descriptors_.empty())
> + flushing_.notify_one();
> +
The flush() document states
flush() should only return when there are no more outstanding buffers or
requests left in the HAL.
Is it okay that flush() returns before notifyError() and
process_capture_result() is called?
-Hiro
>
> node = descriptors_.extract(it);
> }
> Camera3RequestDescriptor &descriptor = node.mapped();
> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> index ed992ae56d5d..9c55cc2674b7 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();
> @@ -124,6 +127,9 @@ private:
> libcamera::Mutex cameraMutex_; /* Protects access to the camera
state. */
> State state_;
>
> + libcamera::Mutex flushMutex_; /* Protects the flushed_ condition
variable. */
> + std::condition_variable flushed_;
> +
> std::shared_ptr<libcamera::Camera> camera_;
> std::unique_ptr<libcamera::CameraConfiguration> config_;
>
> @@ -135,8 +141,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
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210517/c77e148e/attachment-0001.htm>
More information about the libcamera-devel
mailing list