<div dir="ltr"><div dir="ltr">Hi Jacopo, thank you for the patch.</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, May 13, 2021 at 6:22 PM Jacopo Mondi <<a href="mailto:jacopo@jmondi.org" target="_blank">jacopo@jmondi.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Guard access to the camera state and the start/stop sequences<br>
with a mutex.<br>
<br>
Currently only stop() and the first call to processCaptureRequest()<br>
start and stop the camera, and they're not meant to race with each<br>
other. With the introduction of flush() the camera can be stopped<br>
concurrently to a processCaptureRequest() call, hence access to the<br>
camera state will need to be protected.<br>
<br>
Prepare for that by guarding the existing paths with a mutex.<br>
<br>
Signed-off-by: Jacopo Mondi <<a href="mailto:jacopo@jmondi.org" target="_blank">jacopo@jmondi.org</a>><br>
Reviewed-by: Niklas Söderlund <<a href="mailto:niklas.soderlund@ragnatech.se" target="_blank">niklas.soderlund@ragnatech.se</a>><br>
---<br>
src/android/camera_device.cpp | 23 ++++++++++++++---------<br>
src/android/camera_device.h | 1 +<br>
2 files changed, 15 insertions(+), 9 deletions(-)<br>
<br>
diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp<br>
index 8a4543f079eb..c6cd0b6e8be7 100644<br>
--- a/src/android/camera_device.cpp<br>
+++ b/src/android/camera_device.cpp<br>
@@ -759,6 +759,7 @@ void CameraDevice::close()<br>
<br>
void CameraDevice::stop()<br>
{<br>
+ MutexLocker cameraLock(cameraMutex_);<br>
if (state_ == CameraStopped)<br>
return;<br>
<br>
@@ -1915,17 +1916,21 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques<br>
if (!isValidRequest(camera3Request))<br>
return -EINVAL;<br>
<br>
- /* Start the camera if that's the first request we handle. */<br>
- if (state_ == CameraStopped) {<br>
- worker_.start();<br>
+ {<br>
+ MutexLocker cameraLock(cameraMutex_);<br>
<br>
- int ret = camera_->start();<br>
- if (ret) {<br>
- LOG(HAL, Error) << "Failed to start camera";<br>
- return ret;<br>
- }<br>
+ /* Start the camera if that's the first request we handle. */<br>
+ if (state_ == CameraStopped) {<br>
+ worker_.start();<br>
<br>
- state_ = CameraRunning;<br>
+ int ret = camera_->start();<br>
+ if (ret) {<br>
+ LOG(HAL, Error) << "Failed to start camera";<br>
+ return ret;<br>
+ }<br>
+<br>
+ state_ = CameraRunning;<br>
+ }<br>
}<br>
<br>
/*<br>
diff --git a/src/android/camera_device.h b/src/android/camera_device.h<br>
index 30b364decaab..f263fdae472a 100644<br>
--- a/src/android/camera_device.h<br>
+++ b/src/android/camera_device.h<br>
@@ -120,6 +120,7 @@ private:<br>
<br>
CameraWorker worker_;<br>
<br>
+ libcamera::Mutex cameraMutex_; /* Protects access to the camera state. */<br></blockquote><div><br></div><div>I wonder if this should be used to protect camera_ and worker_.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
State state_;<br>
<br>
std::shared_ptr<libcamera::Camera> camera_;<br>
-- <br>
2.31.1<br>
<br>
_______________________________________________<br>
libcamera-devel mailing list<br>
<a href="mailto:libcamera-devel@lists.libcamera.org" target="_blank">libcamera-devel@lists.libcamera.org</a><br>
<a href="https://lists.libcamera.org/listinfo/libcamera-devel" rel="noreferrer" target="_blank">https://lists.libcamera.org/listinfo/libcamera-devel</a><br>
</blockquote></div></div>