[libcamera-devel] [PATCH 8/8] android: Implement flush() camera operation

[Niklas Söderlund]

Hi Jacopo,

Thanks for your work.

On 2021-05-10 12:52:35 +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.
> 
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  src/android/camera_device.cpp | 63 +++++++++++++++++++++++++++++++++++
>  src/android/camera_device.h   |  6 ++++
>  src/android/camera_ops.cpp    |  8 ++++-
>  3 files changed, 76 insertions(+), 1 deletion(-)
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index fa12ce5b0133..01b3acd93c4b 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -756,6 +756,42 @@ void CameraDevice::close()
>  	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.
> + */
> +void CameraDevice::flush()
> +{
> +	{
> +		MutexLocker cameraLock(cameraMutex_);
> +
> +		if (state_ != CameraRunning)
> +			return;
> +
> +		state_ = CameraFlushing;
> +
> +		/*
> +		 * Stop the camera and set the state to flushing to prevent any
> +		 * new request to be queued from this point on.
> +		 */
> +		worker_.stop();
> +		camera_->stop();
> +	}

Releasing cameraMutex_ while waiting for the flushMutex_ signal seems 
like a race waiting to happen as the operation is acting in 
synchronization with the state_ statemachine.

I understand you release it as you want to lock it in 
CameraDevice::stop(), is there any other potential race site? If not 
maybe CameraDevice::stop() can be reworked? It only needs to read the 
state so is the mutex really needed, could it be reworked to an atomic?  
Or maybe there is something in the HAL itself that would prevent other 
callbacks to change the state from CameraFlushing -> CameraRunning while 
flush() is executing?

> +
> +	/*
> +	 * Now wait for all the in-flight requests to be completed before
> +	 * returning. Stopping the Camera guarantees that all in-flight requests
> +	 * are completed in error state.
> +	 */
> +	{
> +		MutexLocker flushLock(flushMutex_);
> +		flushing_.wait(flushLock, [&] { return descriptors_.empty(); });
> +	}
> +
> +	MutexLocker cameraLock(cameraMutex_);
> +	state_ = CameraStopped;
> +}
> +
>  void CameraDevice::stop()
>  {
>  	MutexLocker cameraLock(cameraMutex_);
> @@ -2019,6 +2055,26 @@ 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());
>  
>  	{
> @@ -2052,6 +2108,13 @@ void CameraDevice::requestComplete(Request *request)
>  	}
>  	Camera3RequestDescriptor &descriptor = node.mapped();
>  
> +	/* Release flush if all the pending requests have been completed. */
> +	{
> +		MutexLocker flushLock(flushMutex_);
> +		if (descriptors_.empty())
> +			flushing_.notify_one();
> +	}
> +
>  	/*
>  	 * Prepare the capture result for the Android camera stack.
>  	 *
> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> index ed992ae56d5d..4a9ef495b776 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 flushing condition variable. */
> +	std::condition_variable flushing_;
> +
>  	std::shared_ptr<libcamera::Camera> camera_;
>  	std::unique_ptr<libcamera::CameraConfiguration> config_;
>  
> 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

-- 
Regards,
Niklas Söderlund