| Message ID | 20211029041424.1430886-5-hiroh@chromium.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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@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@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_;
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@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_;
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) { } 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_); }; 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_;
This applies clang thread safety annotation to CameraStream. Mutex and MutexLocker there are replaced with Mutex2 and MutexLocer2. Signed-off-by: Hirokazu Honda <hiroh@chromium.org> --- src/android/camera_stream.cpp | 22 ++++++++++++---------- src/android/camera_stream.h | 13 +++++++------ 2 files changed, 19 insertions(+), 16 deletions(-)