camera: add a lock for the request tracker

The request tracker manages "in flights" buffers and frames. These
two containers are modified by calling the tracker's member functions.
Such functions are called from at least three threads: (see
`v4l2_camera.cpp` for more details):
 - "main" thread;
 - separate buffer enqueue thread ("Enqueue buffers");
 - separate buffer dequeue thread ("Dequeue buffers").
It is proposed to introduce additional mutex for the request tracker
to prevent a race condition.

An alternative approach is introducing an additional mutex for the
shared containers inside the `RequresTracker` class. But such approach
requires more changes and may lead to problems with design in the
future.

This commit contains a few additional changes which are not directly
related to the topic:
 - as far as more than one local lock is used all of them are renamed;
 - the `mFlushLock` member is removed because it is completely covered
   by the new mutex.

Test: try to use a camera with more than one buffer and get SIGSEGV
      crash of the `cameraserver` in the `RequestTracker` class.
Change-Id: I2c1901722289cee9f0cb05a11a4dabe70d18f2ba
Signed-off-by: Sergii Piatakov <sergii.piatakov@globallogic.com>

2 files changed, 11 insertions, 8 deletions

diff --git a/modules/camera/3_4/camera.cpp b/modules/camera/3_4/camera.cpp
index 387134f9..3d2e2257 100644
--- a/modules/camera/3_4/camera.cpp
+++ b/modules/camera/3_4/camera.cpp
@@ -76,7 +76,7 @@ int Camera::openDevice(const hw_module_t *module, hw_device_t **device)
 {
     ALOGI("%s:%d: Opening camera device", __func__, mId);
     ATRACE_CALL();
-    android::Mutex::Autolock al(mDeviceLock);
+    android::Mutex::Autolock dl(mDeviceLock);
 
     if (mBusy) {
         ALOGE("%s:%d: Error! Camera device already opened", __func__, mId);
@@ -113,7 +113,7 @@ int Camera::getInfo(struct camera_info *info)
 int Camera::loadStaticInfo() {
   // Using a lock here ensures |mStaticInfo| will only ever be set once,
   // even in concurrent situations.
-  android::Mutex::Autolock al(mStaticInfoLock);
+  android::Mutex::Autolock sl(mStaticInfoLock);
 
   if (mStaticInfo) {
     return 0;
@@ -143,7 +143,7 @@ int Camera::close()
 {
     ALOGI("%s:%d: Closing camera device", __func__, mId);
     ATRACE_CALL();
-    android::Mutex::Autolock al(mDeviceLock);
+    android::Mutex::Autolock dl(mDeviceLock);
 
     if (!mBusy) {
         ALOGE("%s:%d: Error! Camera device not open", __func__, mId);
@@ -173,7 +173,8 @@ int Camera::initialize(const camera3_callback_ops_t *callback_ops)
 
 int Camera::configureStreams(camera3_stream_configuration_t *stream_config)
 {
-    android::Mutex::Autolock al(mDeviceLock);
+    android::Mutex::Autolock dl(mDeviceLock);
+    android::Mutex::Autolock tl(mInFlightTrackerLock);
 
     ALOGV("%s:%d: stream_config=%p", __func__, mId, stream_config);
     ATRACE_CALL();
@@ -302,7 +303,7 @@ int Camera::processCaptureRequest(camera3_capture_request_t *temp_request)
     int res;
     // TODO(b/32917568): A capture request submitted or ongoing during a flush
     // should be returned with an error; for now they are mutually exclusive.
-    android::Mutex::Autolock al(mFlushLock);
+    android::Mutex::Autolock tl(mInFlightTrackerLock);
 
     ATRACE_CALL();
 
@@ -376,6 +377,8 @@ int Camera::processCaptureRequest(camera3_capture_request_t *temp_request)
 
 void Camera::completeRequest(std::shared_ptr<CaptureRequest> request, int err)
 {
+    android::Mutex::Autolock tl(mInFlightTrackerLock);
+
     if (!mInFlightTracker->Remove(request)) {
         ALOGE("%s:%d: Completed request %p is not being tracked. "
               "It may have been cleared out during a flush.",
@@ -424,7 +427,7 @@ int Camera::flush()
     // is called concurrently with this (in either order).
     // Since the callback to completeRequest also may happen on a separate
     // thread, this function should behave nicely concurrently with that too.
-    android::Mutex::Autolock al(mFlushLock);
+    android::Mutex::Autolock tl(mInFlightTrackerLock);
 
     std::set<std::shared_ptr<CaptureRequest>> requests;
     mInFlightTracker->Clear(&requests);
@@ -517,7 +520,7 @@ void Camera::dump(int fd)
 {
     ALOGV("%s:%d: Dumping to fd %d", __func__, mId, fd);
     ATRACE_CALL();
-    android::Mutex::Autolock al(mDeviceLock);
+    android::Mutex::Autolock dl(mDeviceLock);
 
     dprintf(fd, "Camera ID: %d (Busy: %d)\n", mId, mBusy);
 
diff --git a/modules/camera/3_4/camera.h b/modules/camera/3_4/camera.h
index 687c733f..8c49c8dd 100644
--- a/modules/camera/3_4/camera.h
+++ b/modules/camera/3_4/camera.h
@@ -134,11 +134,11 @@ class Camera {
         // Lock protecting only static camera characteristics, which may
         // be accessed without the camera device open
         android::Mutex mStaticInfoLock;
-        android::Mutex mFlushLock;
         // Standard camera settings templates
         std::unique_ptr<const android::CameraMetadata> mTemplates[CAMERA3_TEMPLATE_COUNT];
         // Track in flight requests.
         std::unique_ptr<RequestTracker> mInFlightTracker;
+        android::Mutex mInFlightTrackerLock;
 };
 }  // namespace default_camera_hal