[libcamera-devel] [RFC PATCH 4/6] android: camera_stream: Add thread safety annotation

[Laurent Pinchart]

Hi Hiro,

Thank you for the patch.

On Fri, Oct 29, 2021 at 01:14:22PM +0900, Hirokazu Honda wrote:
> This applies clang thread safety annotation to CameraStream.
> Mutex and MutexLocker there are replaced with Mutex2 and
> MutexLocer2.

Same typo as in 3/6. Same for the next patches.

> 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_);

Is this a bug fix that thread safety annotation reported ? I'd split it
to a separate patch.

>  		/* 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)
>  {
>  }
>  
>  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_) {
>  			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_);

Is there a problem with

		State state_ GUARDED_BY(mutex_) = State::Stopped;

?

>  	};
>  
>  	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_;

-- 
Regards,

Laurent Pinchart