[libcamera-devel] [RFC PATCH 4/6] android: camera_stream: 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 CameraStream.
> Mutex and MutexLocker there are replaced with Mutex2 and
> MutexLocer2.
s/MutexLocer2/MutexLocker2/  here as well as in rest of patches' commit 
message
>
> Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> ---
>   src/android/camera_stream.cpp | 22 ++++++++++++----------
>   src/android/camera_stream.h   | 13 +++++++------
>   2 files changed, 19 insertions(+), 16 deletions(-)
>
> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
> index 9023c13c..c5272445 100644
> --- a/src/android/camera_stream.cpp
> +++ b/src/android/camera_stream.cpp
> @@ -119,12 +119,13 @@ int CameraStream::configure()
>   
>   	if (type_ == Type::Internal) {
>   		allocator_ = std::make_unique<FrameBufferAllocator>(cameraDevice_->camera());
> -		mutex_ = std::make_unique<std::mutex>();
> +		mutex_ = std::make_unique<Mutex2>();
>   
>   		int ret = allocator_->allocate(stream());
>   		if (ret < 0)
>   			return ret;
>   
> +		MutexLocker2 lock(*mutex_);
>   		/* Save a pointer to the reserved frame buffers */
>   		for (const auto &frameBuffer : allocator_->buffers(stream()))
>   			buffers_.push_back(frameBuffer.get());
> @@ -208,7 +209,7 @@ FrameBuffer *CameraStream::getBuffer()
>   	if (!allocator_)
>   		return nullptr;
>   
> -	std::lock_guard<std::mutex> locker(*mutex_);
> +	MutexLocker2 lock(*mutex_);
>   
>   	if (buffers_.empty()) {
>   		LOG(HAL, Error) << "Buffer underrun";
> @@ -226,20 +227,21 @@ void CameraStream::putBuffer(FrameBuffer *buffer)
>   	if (!allocator_)
>   		return;
>   
> -	std::lock_guard<std::mutex> locker(*mutex_);
> +	MutexLocker2 lock(*mutex_);
>   
>   	buffers_.push_back(buffer);
>   }
>   
>   CameraStream::PostProcessorWorker::PostProcessorWorker(PostProcessor *postProcessor)
> -	: postProcessor_(postProcessor)
> +	: postProcessor_(postProcessor),
> +	  state_(State::Stopped)


why this unrelated change? Also state_ is member initialized in 
PostProcessorWorker class definition

>   {
>   }
>   
>   CameraStream::PostProcessorWorker::~PostProcessorWorker()
>   {
>   	{
> -		libcamera::MutexLocker lock(mutex_);
> +		libcamera::MutexLocker2 lock(mutex_);
>   		state_ = State::Stopped;
>   	}
>   
> @@ -250,7 +252,7 @@ CameraStream::PostProcessorWorker::~PostProcessorWorker()
>   void CameraStream::PostProcessorWorker::start()
>   {
>   	{
> -		libcamera::MutexLocker lock(mutex_);
> +		libcamera::MutexLocker2 lock(mutex_);
>   		ASSERT(state_ != State::Running);
>   		state_ = State::Running;
>   	}
> @@ -261,7 +263,7 @@ void CameraStream::PostProcessorWorker::start()
>   void CameraStream::PostProcessorWorker::queueRequest(Camera3RequestDescriptor::StreamBuffer *dest)
>   {
>   	{
> -		MutexLocker lock(mutex_);
> +		MutexLocker2 lock(mutex_);
>   		ASSERT(state_ == State::Running);
>   		requests_.push(dest);
>   	}
> @@ -271,10 +273,10 @@ void CameraStream::PostProcessorWorker::queueRequest(Camera3RequestDescriptor::S
>   
>   void CameraStream::PostProcessorWorker::run()
>   {
> -	MutexLocker locker(mutex_);
> +	MutexLocker2 locker(mutex_);
>   
>   	while (1) {
> -		cv_.wait(locker, [&] {
> +		cv_.wait(locker.get(), [&]() REQUIRES(mutex_) {


ah nice spot!

>   			return state_ != State::Running || !requests_.empty();
>   		});
>   
> @@ -308,7 +310,7 @@ void CameraStream::PostProcessorWorker::run()
>   
>   void CameraStream::PostProcessorWorker::flush()
>   {
> -	libcamera::MutexLocker lock(mutex_);
> +	MutexLocker2 lock(mutex_);
>   	state_ = State::Flushing;
>   	lock.unlock();
>   
> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
> index 0c402deb..665bdf5c 100644
> --- a/src/android/camera_stream.h
> +++ b/src/android/camera_stream.h
> @@ -9,13 +9,14 @@
>   
>   #include <condition_variable>
>   #include <memory>
> -#include <mutex>
>   #include <queue>
>   #include <vector>
>   
>   #include <hardware/camera3.h>
>   
> +#include <libcamera/base/mutex.h>
>   #include <libcamera/base/thread.h>
> +#include <libcamera/base/thread_annotations.h>
>   
>   #include <libcamera/camera.h>
>   #include <libcamera/framebuffer.h>
> @@ -153,11 +154,11 @@ private:
>   	private:
>   		PostProcessor *postProcessor_;
>   
> -		libcamera::Mutex mutex_;
> +		libcamera::Mutex2 mutex_;
>   		std::condition_variable cv_;
>   
> -		std::queue<Camera3RequestDescriptor::StreamBuffer *> requests_;
> -		State state_ = State::Stopped;
> +		std::queue<Camera3RequestDescriptor::StreamBuffer *> requests_ GUARDED_BY(mutex_);
> +		State state_ GUARDED_BY(mutex_);


ah so that's why you have introduced setting the state_ via constructor. 
GUARDED_BY syntax can handle member - initialization?

Patch seems on the right track so,

Reviewed-by: Umang Jain <umang.jain at ideasonboard.com>

>   	};
>   
>   	int waitFence(int fence);
> @@ -169,12 +170,12 @@ private:
>   	const unsigned int index_;
>   
>   	std::unique_ptr<libcamera::FrameBufferAllocator> allocator_;
> -	std::vector<libcamera::FrameBuffer *> buffers_;
> +	std::vector<libcamera::FrameBuffer *> buffers_ GUARDED_BY(mutex_);
>   	/*
>   	 * The class has to be MoveConstructible as instances are stored in
>   	 * an std::vector in CameraDevice.
>   	 */
> -	std::unique_ptr<std::mutex> mutex_;
> +	std::unique_ptr<libcamera::Mutex2> mutex_;
>   	std::unique_ptr<PostProcessor> postProcessor_;
>   
>   	std::unique_ptr<PostProcessorWorker> worker_;