Camera: fix acquire fence FD double close
This patch fixes several issues:
* Change the timing the acquire fence FD is closed: we used to
close the FD when the buffer is returned from HAL. This patch
changes that to after the request is sent to HAL (or failed
to send to HAL)
* Cleanup inflight buffer map if the request fails to be sent to
HAL
* With the CL, the acquire fence FDs are now closed by
- HalInterface::processBatchCaptureRequests if the HIDL
processCaptureRequests call succeeds and HAL is running
in binderized mode (for passthrough mode the FD is owned
by HAL if the HIDL call succeeds)
- Camera3Device::cleanupFailedRequests otherwise
Test: Camera CTS tests
Bug: 132594861
Change-Id: I5f67ae9e7b8008738bd9a24246d754a6a3669b0c
diff --git a/services/camera/libcameraservice/device3/Camera3Device.cpp b/services/camera/libcameraservice/device3/Camera3Device.cpp
index e18415b..306f9b6 100644
--- a/services/camera/libcameraservice/device3/Camera3Device.cpp
+++ b/services/camera/libcameraservice/device3/Camera3Device.cpp
@@ -4381,11 +4381,12 @@
status_t Camera3Device::HalInterface::wrapAsHidlRequest(camera3_capture_request_t* request,
/*out*/device::V3_2::CaptureRequest* captureRequest,
- /*out*/std::vector<native_handle_t*>* handlesCreated) {
+ /*out*/std::vector<native_handle_t*>* handlesCreated,
+ /*out*/std::vector<std::pair<int32_t, int32_t>>* inflightBuffers) {
ATRACE_CALL();
- if (captureRequest == nullptr || handlesCreated == nullptr) {
- ALOGE("%s: captureRequest (%p) and handlesCreated (%p) must not be null",
- __FUNCTION__, captureRequest, handlesCreated);
+ if (captureRequest == nullptr || handlesCreated == nullptr || inflightBuffers == nullptr) {
+ ALOGE("%s: captureRequest (%p), handlesCreated (%p), and inflightBuffers(%p) "
+ "must not be null", __FUNCTION__, captureRequest, handlesCreated, inflightBuffers);
return BAD_VALUE;
}
@@ -4415,8 +4416,8 @@
captureRequest->inputBuffer.releaseFence = nullptr;
pushInflightBufferLocked(captureRequest->frameNumber, streamId,
- request->input_buffer->buffer,
- request->input_buffer->acquire_fence);
+ request->input_buffer->buffer);
+ inflightBuffers->push_back(std::make_pair(captureRequest->frameNumber, streamId));
} else {
captureRequest->inputBuffer.streamId = -1;
captureRequest->inputBuffer.bufferId = BUFFER_ID_NO_BUFFER;
@@ -4455,14 +4456,31 @@
// Output buffers are empty when using HAL buffer manager
if (!mUseHalBufManager) {
- pushInflightBufferLocked(captureRequest->frameNumber, streamId,
- src->buffer, src->acquire_fence);
+ pushInflightBufferLocked(captureRequest->frameNumber, streamId, src->buffer);
+ inflightBuffers->push_back(std::make_pair(captureRequest->frameNumber, streamId));
}
}
}
return OK;
}
+void Camera3Device::HalInterface::cleanupNativeHandles(
+ std::vector<native_handle_t*> *handles, bool closeFd) {
+ if (handles == nullptr) {
+ return;
+ }
+ if (closeFd) {
+ for (auto& handle : *handles) {
+ native_handle_close(handle);
+ }
+ }
+ for (auto& handle : *handles) {
+ native_handle_delete(handle);
+ }
+ handles->clear();
+ return;
+}
+
status_t Camera3Device::HalInterface::processBatchCaptureRequests(
std::vector<camera3_capture_request_t*>& requests,/*out*/uint32_t* numRequestProcessed) {
ATRACE_NAME("CameraHal::processBatchCaptureRequests");
@@ -4483,17 +4501,20 @@
captureRequests.resize(batchSize);
}
std::vector<native_handle_t*> handlesCreated;
+ std::vector<std::pair<int32_t, int32_t>> inflightBuffers;
status_t res = OK;
for (size_t i = 0; i < batchSize; i++) {
if (hidlSession_3_4 != nullptr) {
res = wrapAsHidlRequest(requests[i], /*out*/&captureRequests_3_4[i].v3_2,
- /*out*/&handlesCreated);
+ /*out*/&handlesCreated, /*out*/&inflightBuffers);
} else {
- res = wrapAsHidlRequest(requests[i],
- /*out*/&captureRequests[i], /*out*/&handlesCreated);
+ res = wrapAsHidlRequest(requests[i], /*out*/&captureRequests[i],
+ /*out*/&handlesCreated, /*out*/&inflightBuffers);
}
if (res != OK) {
+ popInflightBuffers(inflightBuffers);
+ cleanupNativeHandles(&handlesCreated);
return res;
}
}
@@ -4590,18 +4611,30 @@
}
if (!err.isOk()) {
ALOGE("%s: Transaction error: %s", __FUNCTION__, err.description().c_str());
- return DEAD_OBJECT;
+ status = common::V1_0::Status::CAMERA_DISCONNECTED;
}
+
if (status == common::V1_0::Status::OK && *numRequestProcessed != batchSize) {
ALOGE("%s: processCaptureRequest returns OK but processed %d/%zu requests",
__FUNCTION__, *numRequestProcessed, batchSize);
status = common::V1_0::Status::INTERNAL_ERROR;
}
- for (auto& handle : handlesCreated) {
- native_handle_delete(handle);
+ res = CameraProviderManager::mapToStatusT(status);
+ if (res == OK) {
+ if (mHidlSession->isRemote()) {
+ // Only close acquire fence FDs when the HIDL transaction succeeds (so the FDs have been
+ // sent to camera HAL processes)
+ cleanupNativeHandles(&handlesCreated, /*closeFd*/true);
+ } else {
+ // In passthrough mode the FDs are now owned by HAL
+ cleanupNativeHandles(&handlesCreated);
+ }
+ } else {
+ popInflightBuffers(inflightBuffers);
+ cleanupNativeHandles(&handlesCreated);
}
- return CameraProviderManager::mapToStatusT(status);
+ return res;
}
status_t Camera3Device::HalInterface::flush() {
@@ -4683,10 +4716,9 @@
}
status_t Camera3Device::HalInterface::pushInflightBufferLocked(
- int32_t frameNumber, int32_t streamId, buffer_handle_t *buffer, int acquireFence) {
+ int32_t frameNumber, int32_t streamId, buffer_handle_t *buffer) {
uint64_t key = static_cast<uint64_t>(frameNumber) << 32 | static_cast<uint64_t>(streamId);
- auto pair = std::make_pair(buffer, acquireFence);
- mInflightBufferMap[key] = pair;
+ mInflightBufferMap[key] = buffer;
return OK;
}
@@ -4698,16 +4730,22 @@
uint64_t key = static_cast<uint64_t>(frameNumber) << 32 | static_cast<uint64_t>(streamId);
auto it = mInflightBufferMap.find(key);
if (it == mInflightBufferMap.end()) return NAME_NOT_FOUND;
- auto pair = it->second;
- *buffer = pair.first;
- int acquireFence = pair.second;
- if (acquireFence > 0) {
- ::close(acquireFence);
+ if (buffer != nullptr) {
+ *buffer = it->second;
}
mInflightBufferMap.erase(it);
return OK;
}
+void Camera3Device::HalInterface::popInflightBuffers(
+ const std::vector<std::pair<int32_t, int32_t>>& buffers) {
+ for (const auto& pair : buffers) {
+ int32_t frameNumber = pair.first;
+ int32_t streamId = pair.second;
+ popInflightBuffer(frameNumber, streamId, nullptr);
+ }
+}
+
status_t Camera3Device::HalInterface::pushInflightRequestBuffer(
uint64_t bufferId, buffer_handle_t* buf, int32_t streamId) {
std::lock_guard<std::mutex> lock(mRequestedBuffersLock);
diff --git a/services/camera/libcameraservice/device3/Camera3Device.h b/services/camera/libcameraservice/device3/Camera3Device.h
index fe94f8e..1e0040c 100644
--- a/services/camera/libcameraservice/device3/Camera3Device.h
+++ b/services/camera/libcameraservice/device3/Camera3Device.h
@@ -358,13 +358,21 @@
// Do not free input camera3_capture_request_t before output HIDL request
status_t wrapAsHidlRequest(camera3_capture_request_t* in,
/*out*/hardware::camera::device::V3_2::CaptureRequest* out,
- /*out*/std::vector<native_handle_t*>* handlesCreated);
+ /*out*/std::vector<native_handle_t*>* handlesCreated,
+ /*out*/std::vector<std::pair<int32_t, int32_t>>* inflightBuffers);
status_t pushInflightBufferLocked(int32_t frameNumber, int32_t streamId,
- buffer_handle_t *buffer, int acquireFence);
+ buffer_handle_t *buffer);
+
+ // Pop inflight buffers based on pairs of (frameNumber,streamId)
+ void popInflightBuffers(const std::vector<std::pair<int32_t, int32_t>>& buffers);
+
// Cache of buffer handles keyed off (frameNumber << 32 | streamId)
- // value is a pair of (buffer_handle_t*, acquire_fence FD)
- std::unordered_map<uint64_t, std::pair<buffer_handle_t*, int>> mInflightBufferMap;
+ std::unordered_map<uint64_t, buffer_handle_t*> mInflightBufferMap;
+
+ // Delete and optionally close native handles and clear the input vector afterward
+ static void cleanupNativeHandles(
+ std::vector<native_handle_t*> *handles, bool closeFd = false);
struct BufferHasher {
size_t operator()(const buffer_handle_t& buf) const {