[libcamera-devel] [RFC PATCH 5/6] android: camera_device: Add thread safety annotation

[Umang Jain]

Hi Hiro,

Thank you for the patch

On 10/29/21 9:44 AM, Hirokazu Honda wrote:
> This applies clang thread safety annotation to CameraDevice.
> Mutex and MutexLocker there are replaced with Mutex2 and
> MutexLocer2.
>
> Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> ---
>   src/android/camera_device.cpp | 26 ++++++++++++++------------
>   src/android/camera_device.h   | 18 +++++++++---------
>   2 files changed, 23 insertions(+), 21 deletions(-)
>
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index f2e0bdbd..e05b5767 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -14,7 +14,6 @@
>   #include <vector>
>   
>   #include <libcamera/base/log.h>
> -#include <libcamera/base/thread.h>


Ok, so this removal because we are no longer need to use Mutex and 
MutexLocker from thread.h, makes sense

>   #include <libcamera/base/utils.h>
>   
>   #include <libcamera/control_ids.h>
> @@ -399,7 +398,7 @@ void CameraDevice::close()
>   void CameraDevice::flush()
>   {
>   	{
> -		MutexLocker stateLock(stateMutex_);
> +		MutexLocker2 stateLock(stateMutex_);
>   		if (state_ != State::Running)
>   			return;
>   
> @@ -409,20 +408,23 @@ void CameraDevice::flush()
>   	worker_.stop();
>   	camera_->stop();
>   
> -	MutexLocker stateLock(stateMutex_);
> +	MutexLocker2 stateLock(stateMutex_);
>   	state_ = State::Stopped;
>   }
>   
>   void CameraDevice::stop()
>   {
> -	MutexLocker stateLock(stateMutex_);
> +	MutexLocker2 stateLock(stateMutex_);
>   	if (state_ == State::Stopped)
>   		return;
>   
>   	worker_.stop();
>   	camera_->stop();
>   
> -	descriptors_ = {};
> +	{
> +		MutexLocker2 descriptorsLock(descriptorsMutex_);
> +		descriptors_ = {};
> +	}


oh, we were resetting descriptors_ without taking the lock here.

I am curious, did you notice this as WARNING from annotation and then 
introduced this change? If yes, then annotation is already proving 
useful to us.

>   	streams_.clear();
>   
>   	state_ = State::Stopped;
> @@ -919,6 +921,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>   		 */
>   		FrameBuffer *frameBuffer = nullptr;
>   		int acquireFence = -1;
> +		MutexLocker2 lock(descriptor->streamsProcessMutex_);


aha, one more change as result warning I suppose?

>   		switch (cameraStream->type()) {
>   		case CameraStream::Type::Mapped:
>   			/*
> @@ -926,7 +929,6 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>   			 * Request.
>   			 */
>   			LOG(HAL, Debug) << ss.str() << " (mapped)";
> -
?
>   			descriptor->pendingStreamsToProcess_.insert(
>   				{ cameraStream, &buffer });
>   			continue;
> @@ -986,12 +988,12 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>   	 * on the queue to be later completed. If the camera has been stopped we
>   	 * have to re-start it to be able to process the request.
>   	 */
> -	MutexLocker stateLock(stateMutex_);
> +	MutexLocker2 stateLock(stateMutex_);
>   
>   	if (state_ == State::Flushing) {
>   		Camera3RequestDescriptor *rawDescriptor = descriptor.get();
>   		{
> -			MutexLocker descriptorsLock(descriptorsMutex_);
> +			MutexLocker2 descriptorsLock(descriptorsMutex_);
>   			descriptors_.push(std::move(descriptor));
>   		}
>   		abortRequest(rawDescriptor);
> @@ -1016,7 +1018,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>   	CaptureRequest *request = descriptor->request_.get();
>   
>   	{
> -		MutexLocker descriptorsLock(descriptorsMutex_);
> +		MutexLocker2 descriptorsLock(descriptorsMutex_);
>   		descriptors_.push(std::move(descriptor));
>   	}
>   
> @@ -1103,7 +1105,7 @@ void CameraDevice::requestComplete(Request *request)
>   	}
>   
>   	/* Handle post-processing. */
> -	MutexLocker locker(descriptor->streamsProcessMutex_);
> +	MutexLocker2 locker(descriptor->streamsProcessMutex_);
>   
>   	/*
>   	 * Queue all the post-processing streams request at once. The completion
> @@ -1149,7 +1151,7 @@ void CameraDevice::requestComplete(Request *request)
>   
>   void CameraDevice::completeDescriptor(Camera3RequestDescriptor *descriptor)
>   {
> -	MutexLocker lock(descriptorsMutex_);
> +	MutexLocker2 lock(descriptorsMutex_);
>   	descriptor->complete_ = true;
>   
>   	sendCaptureResults();
> @@ -1229,7 +1231,7 @@ void CameraDevice::streamProcessingComplete(Camera3RequestDescriptor::StreamBuff
>   	Camera3RequestDescriptor *request = streamBuffer->request;
>   
>   	{
> -		MutexLocker locker(request->streamsProcessMutex_);
> +		MutexLocker2 locker(request->streamsProcessMutex_);
>   
>   		request->pendingStreamsToProcess_.erase(streamBuffer->stream);
>   		if (!request->pendingStreamsToProcess_.empty())
> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> index 2a414020..9feb287e 100644
> --- a/src/android/camera_device.h
> +++ b/src/android/camera_device.h
> @@ -9,7 +9,6 @@
>   
>   #include <map>
>   #include <memory>
> -#include <mutex>
>   #include <queue>
>   #include <vector>
>   
> @@ -18,7 +17,8 @@
>   #include <libcamera/base/class.h>
>   #include <libcamera/base/log.h>
>   #include <libcamera/base/message.h>
> -#include <libcamera/base/thread.h>
> +#include <libcamera/base/mutex.h>
> +#include <libcamera/base/thread_annotations.h>
>   
>   #include <libcamera/camera.h>
>   #include <libcamera/framebuffer.h>
> @@ -83,7 +83,7 @@ private:
>   		Running,
>   	};
>   
> -	void stop();
> +	void stop() EXCLUDES(stateMutex_);
>   
>   	std::unique_ptr<libcamera::FrameBuffer>
>   	createFrameBuffer(const buffer_handle_t camera3buffer,
> @@ -95,8 +95,8 @@ private:
>   	void notifyError(uint32_t frameNumber, camera3_stream_t *stream,
>   			 camera3_error_msg_code code) const;
>   	int processControls(Camera3RequestDescriptor *descriptor);
> -	void completeDescriptor(Camera3RequestDescriptor *descriptor);
> -	void sendCaptureResults();
> +	void completeDescriptor(Camera3RequestDescriptor *descriptor) EXCLUDES(descriptorsMutex_);


Am I right to infer that the EXCLUDES here means, descriptorsMutex_ is 
not held (i.e. is not locked) beforehand, calling to completeDescriptor? 
Since the completeDescriptor will actually lock descriptorMutex_

> +	void sendCaptureResults() REQUIRES(descriptorsMutex_);


And this requires descriptorsMutex_ to be locked, which makes sense.

>   	void setBufferStatus(Camera3RequestDescriptor::StreamBuffer &buffer,
>   			     Camera3RequestDescriptor::Status status);
>   	std::unique_ptr<CameraMetadata> getResultMetadata(
> @@ -107,8 +107,8 @@ private:
>   
>   	CameraWorker worker_;
>   
> -	libcamera::Mutex stateMutex_; /* Protects access to the camera state. */
> -	State state_;
> +	libcamera::Mutex2 stateMutex_; /* Protects access to the camera state. */
> +	State state_ GUARDED_BY(stateMutex_);
>   
>   	std::shared_ptr<libcamera::Camera> camera_;
>   	std::unique_ptr<libcamera::CameraConfiguration> config_;
> @@ -119,8 +119,8 @@ private:
>   
>   	std::vector<CameraStream> streams_;
>   
> -	libcamera::Mutex descriptorsMutex_; /* Protects descriptors_. */
> -	std::queue<std::unique_ptr<Camera3RequestDescriptor>> descriptors_;
> +	libcamera::Mutex2 descriptorsMutex_ ACQUIRED_AFTER(stateMutex_);


Well, the document currently states that ACQUIRED_AFTER is currently 
un-implemented.

Secondly, I don't think we enforce a design interaction between the two 
mutexes currently, that requires this macro. For e.g. 
completeDescriptor() on a requestcomplete() path, will acquire 
descriptorsMutex_ irrespective of acquiring stateMutex_. Is there any 
strong reasoning I am missing which led to use of ACQUIRED_AFTER for 
descriptorsMutex_?

> +	std::queue<std::unique_ptr<Camera3RequestDescriptor>> descriptors_ GUARDED_BY(descriptorsMutex_);


This is becoming a bit harder to read (not your fault, probably mine). I 
should spend some time tinkering on naming these members/classes.

>   
>   	std::string maker_;
>   	std::string model_;