[libcamera-devel] [PATCH v2 1/2] android: CameraDevice: Prevent race due to concurrent HAL calls

[Hirokazu Honda]

HAL API functions might be called by different threads in Android
Camera HAL3 API, while process_capture_request() must be called
by a single thread. Furthermore, close() and flush() can be
called any time. Therefore, races to variables can occur among
the HAL API functions.
This prevents the races by enclosing HAL calls by mutex. It
might not be the best in terms of the performance, but the
simplicity is good as a first step and also further development.

Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
---
 src/android/camera_device.cpp | 70 +++++++++++++++++++++--------------
 src/android/camera_device.h   |  9 ++++-
 2 files changed, 50 insertions(+), 29 deletions(-)

diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
index a71aee2f..c74dc0e7 100644
--- a/src/android/camera_device.cpp
+++ b/src/android/camera_device.cpp
@@ -748,27 +748,40 @@ int CameraDevice::open(const hw_module_t *hardwareModule)
 
 void CameraDevice::close()
 {
-	streams_.clear();
-
-	stop();
+	std::scoped_lock<std::mutex> lock(mutex_);
 
-	camera_->release();
+	stop(/*releaseCamera=*/true);
 }
 
-void CameraDevice::stop()
+void CameraDevice::stop(bool releaseCamera)
 {
-	if (!running_)
+	streams_.clear();
+
+	if (!running_) {
+		if (releaseCamera) {
+			descriptors_.clear();
+			camera_->release();
+		}
 		return;
+	}
+
+	stopping_ = true;
 
 	worker_.stop();
 	camera_->stop();
 
 	descriptors_.clear();
+
 	running_ = false;
+	stopping_ = false;
+
+	if (releaseCamera)
+		camera_->release();
 }
 
 void CameraDevice::setCallbacks(const camera3_callback_ops_t *callbacks)
 {
+	std::scoped_lock<std::mutex> lock(mutex_);
 	callbacks_ = callbacks;
 }
 
@@ -1581,6 +1594,8 @@ std::unique_ptr<CameraMetadata> CameraDevice::requestTemplateVideo()
  */
 const camera_metadata_t *CameraDevice::constructDefaultRequestSettings(int type)
 {
+	std::scoped_lock<std::mutex> lock(mutex_);
+
 	auto it = requestTemplates_.find(type);
 	if (it != requestTemplates_.end())
 		return it->second->get();
@@ -1648,8 +1663,7 @@ PixelFormat CameraDevice::toPixelFormat(int format) const
  */
 int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
 {
-	/* Before any configuration attempt, stop the camera. */
-	stop();
+	std::scoped_lock<std::mutex> lock(mutex_);
 
 	if (stream_list->num_streams == 0) {
 		LOG(HAL, Error) << "No streams in configuration";
@@ -1661,6 +1675,9 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
 		return -EINVAL;
 #endif
 
+	/* Before any configuration attempt, stop the camera. */
+	stop();
+
 	/*
 	 * Generate an empty configuration, and construct a StreamConfiguration
 	 * for each camera3_stream to add to it.
@@ -1672,11 +1689,10 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
 	}
 
 	/*
-	 * Clear and remove any existing configuration from previous calls, and
-	 * ensure the required entries are available without further
+	 * Ensure the required entries are available without further
 	 * reallocation.
 	 */
-	streams_.clear();
+	ASSERT(streams_.empty());
 	streams_.reserve(stream_list->num_streams);
 
 	std::vector<Camera3StreamConfig> streamConfigs;
@@ -1912,6 +1928,8 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
 	if (!isValidRequest(camera3Request))
 		return -EINVAL;
 
+	std::scoped_lock<std::mutex> lock(mutex_);
+
 	/* Start the camera if that's the first request we handle. */
 	if (!running_) {
 		worker_.start();
@@ -2015,33 +2033,31 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
 
 	worker_.queueRequest(descriptor.request_.get());
 
-	{
-		std::scoped_lock<std::mutex> lock(mutex_);
-		descriptors_[descriptor.request_->cookie()] = std::move(descriptor);
-	}
+	descriptors_[descriptor.request_->cookie()] = std::move(descriptor);
 
 	return 0;
 }
 
 void CameraDevice::requestComplete(Request *request)
 {
+	if (stopping_)
+		return;
+
+	std::scoped_lock<std::mutex> lock(mutex_);
+
 	const Request::BufferMap &buffers = request->buffers();
 	camera3_buffer_status status = CAMERA3_BUFFER_STATUS_OK;
 	std::unique_ptr<CameraMetadata> resultMetadata;
 
-	decltype(descriptors_)::node_type node;
-	{
-		std::scoped_lock<std::mutex> lock(mutex_);
-		auto it = descriptors_.find(request->cookie());
-		if (it == descriptors_.end()) {
-			LOG(HAL, Fatal)
-				<< "Unknown request: " << request->cookie();
-			status = CAMERA3_BUFFER_STATUS_ERROR;
-			return;
-		}
-
-		node = descriptors_.extract(it);
+	auto it = descriptors_.find(request->cookie());
+	if (it == descriptors_.end()) {
+		LOG(HAL, Debug)
+			<< "Unknown request: " << request->cookie();
+		status = CAMERA3_BUFFER_STATUS_ERROR;
+		return;
 	}
+
+	auto node = descriptors_.extract(it);
 	Camera3RequestDescriptor &descriptor = node.mapped();
 
 	if (request->status() != Request::RequestComplete) {
diff --git a/src/android/camera_device.h b/src/android/camera_device.h
index c63e8e21..08553584 100644
--- a/src/android/camera_device.h
+++ b/src/android/camera_device.h
@@ -88,7 +88,7 @@ private:
 		int androidFormat;
 	};
 
-	void stop();
+	void stop(bool releaseCamera = false);
 
 	int initializeStreamConfigurations();
 	std::vector<libcamera::Size>
@@ -127,7 +127,12 @@ private:
 	std::map<int, libcamera::PixelFormat> formatsMap_;
 	std::vector<CameraStream> streams_;
 
-	std::mutex mutex_; /* Protect descriptors_ */
+	/*
+	 * Protect descriptors_ and camera_, and also avoid races by concurrent
+	 * Camera HAL API calls.
+	 */
+	std::mutex mutex_;
+	std::atomic_bool stopping_;
 	std::map<uint64_t, Camera3RequestDescriptor> descriptors_;
 
 	std::string maker_;
-- 
2.31.1.498.g6c1eba8ee3d-goog