Camera: use finer lock in external camera OutputThread
To avoid OutputThread waiting for mLock.
Test: CTS RecordingTest + systrace
Bug: 72261744
Change-Id: If387a1d4a2b0081c4bc43cb648a98e3706864f19
diff --git a/camera/device/3.4/default/ExternalCameraDeviceSession.cpp b/camera/device/3.4/default/ExternalCameraDeviceSession.cpp
index 7015bcb..0ede889 100644
--- a/camera/device/3.4/default/ExternalCameraDeviceSession.cpp
+++ b/camera/device/3.4/default/ExternalCameraDeviceSession.cpp
@@ -51,10 +51,12 @@
// webcam showing temporarily ioctl failures.
constexpr int IOCTL_RETRY_SLEEP_US = 33000; // 33ms * MAX_RETRY = 5 seconds
+// Constants for tryLock during dumpstate
+static constexpr int kDumpLockRetries = 50;
+static constexpr int kDumpLockSleep = 60000;
+
bool tryLock(Mutex& mutex)
{
- static const int kDumpLockRetries = 50;
- static const int kDumpLockSleep = 60000;
bool locked = false;
for (int i = 0; i < kDumpLockRetries; ++i) {
if (mutex.tryLock() == NO_ERROR) {
@@ -66,6 +68,19 @@
return locked;
}
+bool tryLock(std::mutex& mutex)
+{
+ bool locked = false;
+ for (int i = 0; i < kDumpLockRetries; ++i) {
+ if (mutex.try_lock()) {
+ locked = true;
+ break;
+ }
+ usleep(kDumpLockSleep);
+ }
+ return locked;
+}
+
} // Anonymous namespace
// Static instances
@@ -163,7 +178,6 @@
bool streaming = false;
size_t v4L2BufferCount = 0;
SupportedV4L2Format streamingFmt;
- std::unordered_set<uint32_t> inflightFrames;
{
bool sessionLocked = tryLock(mLock);
if (!sessionLocked) {
@@ -172,12 +186,25 @@
streaming = mV4l2Streaming;
streamingFmt = mV4l2StreamingFmt;
v4L2BufferCount = mV4L2BufferCount;
- inflightFrames = mInflightFrames;
+
if (sessionLocked) {
mLock.unlock();
}
}
+ std::unordered_set<uint32_t> inflightFrames;
+ {
+ bool iffLocked = tryLock(mInflightFramesLock);
+ if (!iffLocked) {
+ dprintf(fd,
+ "!! ExternalCameraDeviceSession mInflightFramesLock may be deadlocked !!\n");
+ }
+ inflightFrames = mInflightFrames;
+ if (iffLocked) {
+ mInflightFramesLock.unlock();
+ }
+ }
+
dprintf(fd, "External camera %s V4L2 FD %d, cropping type %s, %s\n",
mCameraId.c_str(), mV4l2Fd.get(),
(mCroppingType == VERTICAL) ? "vertical" : "horizontal",
@@ -457,6 +484,7 @@
}
int ExternalCameraDeviceSession::waitForV4L2BufferReturnLocked(std::unique_lock<std::mutex>& lk) {
+ ATRACE_CALL();
std::chrono::seconds timeout = std::chrono::seconds(kBufferWaitTimeoutSec);
mLock.unlock();
auto st = mV4L2BufferReturned.wait_for(lk, timeout);
@@ -612,7 +640,10 @@
halBuf.acquireFence = allFences[i];
halBuf.fenceTimeout = false;
}
- mInflightFrames.insert(halReq->frameNumber);
+ {
+ std::lock_guard<std::mutex> lk(mInflightFramesLock);
+ mInflightFrames.insert(halReq->frameNumber);
+ }
// Send request to OutputThread for the rest of processing
mOutputThread->submitRequest(halReq);
mFirstRequest = false;
@@ -670,7 +701,7 @@
// update inflight records
{
- Mutex::Autolock _l(mLock);
+ std::lock_guard<std::mutex> lk(mInflightFramesLock);
mInflightFrames.erase(req->frameNumber);
}
@@ -724,7 +755,7 @@
// update inflight records
{
- Mutex::Autolock _l(mLock);
+ std::lock_guard<std::mutex> lk(mInflightFramesLock);
mInflightFrames.erase(req->frameNumber);
}
@@ -2278,6 +2309,7 @@
}
}
+ ATRACE_BEGIN("VIDIOC_DQBUF");
v4l2_buffer buffer{};
buffer.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
buffer.memory = V4L2_MEMORY_MMAP;
@@ -2285,6 +2317,7 @@
ALOGE("%s: DQBUF fails: %s", __FUNCTION__, strerror(errno));
return ret;
}
+ ATRACE_END();
if (buffer.index >= mV4L2BufferCount) {
ALOGE("%s: Invalid buffer id: %d", __FUNCTION__, buffer.index);
@@ -2316,21 +2349,18 @@
void ExternalCameraDeviceSession::enqueueV4l2Frame(const sp<V4L2Frame>& frame) {
ATRACE_CALL();
- {
- // Release mLock before acquiring mV4l2BufferLock to avoid potential
- // deadlock
- Mutex::Autolock _l(mLock);
- frame->unmap();
- v4l2_buffer buffer{};
- buffer.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
- buffer.memory = V4L2_MEMORY_MMAP;
- buffer.index = frame->mBufferIndex;
- if (TEMP_FAILURE_RETRY(ioctl(mV4l2Fd.get(), VIDIOC_QBUF, &buffer)) < 0) {
- ALOGE("%s: QBUF index %d fails: %s", __FUNCTION__,
- frame->mBufferIndex, strerror(errno));
- return;
- }
+ frame->unmap();
+ ATRACE_BEGIN("VIDIOC_QBUF");
+ v4l2_buffer buffer{};
+ buffer.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
+ buffer.memory = V4L2_MEMORY_MMAP;
+ buffer.index = frame->mBufferIndex;
+ if (TEMP_FAILURE_RETRY(ioctl(mV4l2Fd.get(), VIDIOC_QBUF, &buffer)) < 0) {
+ ALOGE("%s: QBUF index %d fails: %s", __FUNCTION__,
+ frame->mBufferIndex, strerror(errno));
+ return;
}
+ ATRACE_END();
{
std::lock_guard<std::mutex> lk(mV4l2BufferLock);
@@ -2383,13 +2413,17 @@
return status;
}
- Mutex::Autolock _l(mLock);
- if (!mInflightFrames.empty()) {
- ALOGE("%s: trying to configureStreams while there are still %zu inflight frames!",
- __FUNCTION__, mInflightFrames.size());
- return Status::INTERNAL_ERROR;
+
+ {
+ std::lock_guard<std::mutex> lk(mInflightFramesLock);
+ if (!mInflightFrames.empty()) {
+ ALOGE("%s: trying to configureStreams while there are still %zu inflight frames!",
+ __FUNCTION__, mInflightFrames.size());
+ return Status::INTERNAL_ERROR;
+ }
}
+ Mutex::Autolock _l(mLock);
// Add new streams
for (const auto& stream : config.streams) {
if (mStreamMap.count(stream.id) == 0) {
@@ -2689,14 +2723,17 @@
const uint8_t ae_lock = ANDROID_CONTROL_AE_LOCK_OFF;
UPDATE(md, ANDROID_CONTROL_AE_LOCK, &ae_lock, 1);
- bool afTrigger = mAfTrigger;
- if (md.exists(ANDROID_CONTROL_AF_TRIGGER)) {
- Mutex::Autolock _l(mLock);
- camera_metadata_entry entry = md.find(ANDROID_CONTROL_AF_TRIGGER);
- if (entry.data.u8[0] == ANDROID_CONTROL_AF_TRIGGER_START) {
- mAfTrigger = afTrigger = true;
- } else if (entry.data.u8[0] == ANDROID_CONTROL_AF_TRIGGER_CANCEL) {
- mAfTrigger = afTrigger = false;
+ bool afTrigger = false;
+ {
+ std::lock_guard<std::mutex> lk(mAfTriggerLock);
+ afTrigger = mAfTrigger;
+ if (md.exists(ANDROID_CONTROL_AF_TRIGGER)) {
+ camera_metadata_entry entry = md.find(ANDROID_CONTROL_AF_TRIGGER);
+ if (entry.data.u8[0] == ANDROID_CONTROL_AF_TRIGGER_START) {
+ mAfTrigger = afTrigger = true;
+ } else if (entry.data.u8[0] == ANDROID_CONTROL_AF_TRIGGER_CANCEL) {
+ mAfTrigger = afTrigger = false;
+ }
}
}
diff --git a/camera/device/3.4/default/include/ext_device_v3_4_impl/ExternalCameraDeviceSession.h b/camera/device/3.4/default/include/ext_device_v3_4_impl/ExternalCameraDeviceSession.h
index 5315097..9843a50 100644
--- a/camera/device/3.4/default/include/ext_device_v3_4_impl/ExternalCameraDeviceSession.h
+++ b/camera/device/3.4/default/include/ext_device_v3_4_impl/ExternalCameraDeviceSession.h
@@ -299,6 +299,9 @@
const std::vector<SupportedV4L2Format> mSupportedFormats;
const CroppingType mCroppingType;
const std::string& mCameraId;
+
+ // Not protected by mLock, this is almost a const.
+ // Setup in constructor, reset in close() after OutputThread is joined
unique_fd mV4l2Fd;
// device is closed either
@@ -325,6 +328,8 @@
// Stream ID -> Camera3Stream cache
std::unordered_map<int, Stream> mStreamMap;
+
+ std::mutex mInflightFramesLock; // protect mInflightFrames
std::unordered_set<uint32_t> mInflightFrames;
// buffers currently circulating between HAL and camera service
@@ -336,6 +341,7 @@
// Stream ID -> circulating buffers map
std::map<int, CirculatingBuffers> mCirculatingBuffers;
+ std::mutex mAfTriggerLock; // protect mAfTrigger
bool mAfTrigger = false;
static HandleImporter sHandleImporter;