[libcamera-devel] [PATCH v3 2/3] 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 | 59 ++++++++++++++++++++---------------
 src/android/camera_device.h   |  7 +++--
 2 files changed, 39 insertions(+), 27 deletions(-)

diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
index b3def04b..96c8d4a9 100644
--- a/src/android/camera_device.cpp
+++ b/src/android/camera_device.cpp
@@ -743,7 +743,7 @@ int CameraDevice::open(const hw_module_t *hardwareModule)
 
 void CameraDevice::close()
 {
-	streams_.clear();
+	std::scoped_lock<std::mutex> halLock(halMutex_);
 
 	stop();
 
@@ -758,12 +758,17 @@ void CameraDevice::stop()
 	worker_.stop();
 	camera_->stop();
 
-	descriptors_.clear();
+	{
+		std::scoped_lock<std::mutex> lock(mutex_);
+		descriptors_.clear();
+	}
+
 	running_ = false;
 }
 
 void CameraDevice::setCallbacks(const camera3_callback_ops_t *callbacks)
 {
+	std::scoped_lock<std::mutex> halLock(halMutex_);
 	callbacks_ = callbacks;
 }
 
@@ -1643,8 +1648,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> halLock(halMutex_);
 
 	if (stream_list->num_streams == 0) {
 		LOG(HAL, Error) << "No streams in configuration";
@@ -1656,6 +1660,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.
@@ -1671,6 +1678,8 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
 	 * ensure the required entries are available without further
 	 * reallocation.
 	 */
+
+	std::scoped_lock<std::mutex> lock(mutex_);
 	streams_.clear();
 	streams_.reserve(stream_list->num_streams);
 
@@ -1907,6 +1916,8 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
 	if (!isValidRequest(camera3Request))
 		return -EINVAL;
 
+	std::scoped_lock<std::mutex> halLock(halMutex_);
+
 	/* Start the camera if that's the first request we handle. */
 	if (!running_) {
 		worker_.start();
@@ -2022,29 +2033,26 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
 
 void CameraDevice::requestComplete(Request *request)
 {
+	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::unique_ptr<CaptureRequest> captureRequest;
-	{
-		std::scoped_lock<std::mutex> lock(mutex_);
-		auto requestIt = requests_.find(request->cookie());
-		if (requestIt == requests_.end()) {
-			LOG(HAL, Fatal)
-				<< "Unknown request: " << request->cookie();
-			return;
-		}
-		captureRequest = std::move(requestIt->second);
-		requests_.erase(requestIt);
+	auto requestIt = requests_.find(request->cookie());
+	if (requestIt == requests_.end()) {
+		LOG(HAL, Fatal)
+			<< "Unknown request: " << request->cookie();
+		return;
+	}
+	auto captureRequest = std::move(requestIt->second);
+	requests_.erase(requestIt);
 
-		auto it = descriptors_.find(request->cookie());
-		if (it == descriptors_.end())
-			return;
+	auto it = descriptors_.find(request->cookie());
+	if (it == descriptors_.end())
+		return;
 
-		node = descriptors_.extract(it);
-	}
+	auto node = descriptors_.extract(it);
 	Camera3RequestDescriptor &descriptor = node.mapped();
 
 	if (request->status() != Request::RequestComplete) {
@@ -2149,11 +2157,12 @@ void CameraDevice::notifyError(uint32_t frameNumber, camera3_stream_t *stream)
 	camera3_notify_msg_t notify = {};
 
 	/*
-	 * \todo Report and identify the stream number or configuration to
-	 * clarify the stream that failed.
+	 * \todo Report and identify the stream number or configuration
+	 * to clarify the stream that failed.
 	 */
-	LOG(HAL, Error) << "Error occurred on frame " << frameNumber << " ("
-			<< toPixelFormat(stream->format).toString() << ")";
+	LOG(HAL, Error)
+		<< "Error occurred on frame " << descriptor.frameNumber_ << " ("
+		<< toPixelFormat(descriptor.buffers_[0].stream->format).toString() << ")";
 
 	notify.type = CAMERA3_MSG_ERROR;
 	notify.message.error.error_stream = stream;
diff --git a/src/android/camera_device.h b/src/android/camera_device.h
index 95d77890..325fb967 100644
--- a/src/android/camera_device.h
+++ b/src/android/camera_device.h
@@ -125,9 +125,12 @@ private:
 
 	std::vector<Camera3StreamConfiguration> streamConfigurations_;
 	std::map<int, libcamera::PixelFormat> formatsMap_;
-	std::vector<CameraStream> streams_;
 
-	std::mutex mutex_; /* Protect descriptors_ and requests_. */
+	/* Avoid races by concurrent Camera HAL API calls. */
+	std::mutex halMutex_;
+
+	std::mutex mutex_; /* Protect streams_, descriptors_ and requests_. */
+	std::vector<CameraStream> streams_;
 	std::map<uint64_t, Camera3RequestDescriptor> descriptors_;
 	std::map<uint64_t, std::unique_ptr<CaptureRequest>> requests_;
 
-- 
2.31.1.498.g6c1eba8ee3d-goog