From aa6993121e607ffcb4492d0f984db3c48748e150 Mon Sep 17 00:00:00 2001 From: Yin-Chia Yeh Date: Fri, 26 May 2017 14:01:32 -0700 Subject: [PATCH] Camera: fix release fence FD leaks Test: use sw_sync to fake release fence FDs, GCA, CTS Bug: 62070085 Change-Id: Iae77a2e112df5363e55e4177656a5dd41b830cbd --- .../3.2/default/CameraDeviceSession.cpp | 70 ++++++++++++++++--- .../device/3.2/default/CameraDeviceSession.h | 8 +++ 2 files changed, 67 insertions(+), 11 deletions(-) diff --git a/camera/device/3.2/default/CameraDeviceSession.cpp b/camera/device/3.2/default/CameraDeviceSession.cpp index 06a6bd01d6..f33adf8d1a 100644 --- a/camera/device/3.2/default/CameraDeviceSession.cpp +++ b/camera/device/3.2/default/CameraDeviceSession.cpp @@ -341,7 +341,7 @@ void CameraDeviceSession::ResultBatcher::registerBatch( batch->mLastFrame = batch->mFirstFrame + batch->mBatchSize - 1; batch->mNumPartialResults = mNumPartialResults; for (int id : mStreamsToBatch) { - batch->mBatchBufs[id] = InflightBatch::BufferBatch(); + batch->mBatchBufs.emplace(id, batch->mBatchSize); } Mutex::Autolock _l(mLock); mInflightBatches.push_back(batch); @@ -418,6 +418,29 @@ void CameraDeviceSession::ResultBatcher::freeReleaseFences(hidl_vec& dst) { + // Only dealing with releaseFence here. Assume buffer/acquireFence are null + const native_handle_t* handle = src.releaseFence.getNativeHandle(); + src.releaseFence = nullptr; + dst.push_back(src); + dst.back().releaseFence = handle; + if (handle != dst.back().releaseFence.getNativeHandle()) { + ALOGE("%s: native handle cloned!", __FUNCTION__); + } +} + void CameraDeviceSession::ResultBatcher::sendBatchBuffersLocked(std::shared_ptr batch) { sendBatchBuffersLocked(batch, mStreamsToBatch); } @@ -444,7 +467,12 @@ void CameraDeviceSession::ResultBatcher::sendBatchBuffersLocked( if (batchSize == 0) { ALOGW("%s: there is no buffer to be delivered for this batch.", __FUNCTION__); for (int streamId : streams) { - InflightBatch::BufferBatch& bb = batch->mBatchBufs[streamId]; + auto it = batch->mBatchBufs.find(streamId); + if (it == batch->mBatchBufs.end()) { + ALOGE("%s: cannot find stream %d in batched buffers!", __FUNCTION__, streamId); + return; + } + InflightBatch::BufferBatch& bb = it->second; bb.mDelivered = true; } return; @@ -460,21 +488,35 @@ void CameraDeviceSession::ResultBatcher::sendBatchBuffersLocked( results[i].inputBuffer.bufferId = 0; results[i].inputBuffer.buffer = nullptr; std::vector outBufs; + outBufs.reserve(streams.size()); for (int streamId : streams) { - InflightBatch::BufferBatch& bb = batch->mBatchBufs[streamId]; + auto it = batch->mBatchBufs.find(streamId); + if (it == batch->mBatchBufs.end()) { + ALOGE("%s: cannot find stream %d in batched buffers!", __FUNCTION__, streamId); + return; + } + InflightBatch::BufferBatch& bb = it->second; if (bb.mDelivered) { continue; } if (i < bb.mBuffers.size()) { - outBufs.push_back(bb.mBuffers[i]); + pushStreamBuffer(std::move(bb.mBuffers[i]), outBufs); } } - results[i].outputBuffers = outBufs; + results[i].outputBuffers.resize(outBufs.size()); + for (size_t j = 0; j < outBufs.size(); j++) { + moveStreamBuffer(std::move(outBufs[j]), results[i].outputBuffers[j]); + } } invokeProcessCaptureResultCallback(results, /* tryWriteFmq */false); freeReleaseFences(results); for (int streamId : streams) { - InflightBatch::BufferBatch& bb = batch->mBatchBufs[streamId]; + auto it = batch->mBatchBufs.find(streamId); + if (it == batch->mBatchBufs.end()) { + ALOGE("%s: cannot find stream %d in batched buffers!", __FUNCTION__, streamId); + return; + } + InflightBatch::BufferBatch& bb = it->second; bb.mDelivered = true; bb.mBuffers.clear(); } @@ -613,7 +655,9 @@ void CameraDeviceSession::ResultBatcher::invokeProcessCaptureResultCallback( } void CameraDeviceSession::ResultBatcher::processOneCaptureResult(CaptureResult& result) { - hidl_vec results = {result}; + hidl_vec results; + results.resize(1); + results[0] = std::move(result); invokeProcessCaptureResultCallback(results, /* tryWriteFmq */true); freeReleaseFences(results); return; @@ -650,10 +694,10 @@ void CameraDeviceSession::ResultBatcher::processCaptureResult(CaptureResult& res auto it = batch->mBatchBufs.find(buffer.streamId); if (it != batch->mBatchBufs.end()) { InflightBatch::BufferBatch& bb = it->second; - bb.mBuffers.push_back(buffer); + pushStreamBuffer(std::move(buffer), bb.mBuffers); filledStreams.push_back(buffer.streamId); } else { - nonBatchedBuffers.push_back(buffer); + pushStreamBuffer(std::move(buffer), nonBatchedBuffers); } } @@ -662,8 +706,12 @@ void CameraDeviceSession::ResultBatcher::processCaptureResult(CaptureResult& res CaptureResult nonBatchedResult; nonBatchedResult.frameNumber = result.frameNumber; nonBatchedResult.fmqResultSize = 0; - nonBatchedResult.outputBuffers = nonBatchedBuffers; - nonBatchedResult.inputBuffer = result.inputBuffer; + nonBatchedResult.outputBuffers.resize(nonBatchedBuffers.size()); + for (size_t i = 0; i < nonBatchedBuffers.size(); i++) { + moveStreamBuffer( + std::move(nonBatchedBuffers[i]), nonBatchedResult.outputBuffers[i]); + } + moveStreamBuffer(std::move(result.inputBuffer), nonBatchedResult.inputBuffer); nonBatchedResult.partialResult = 0; // 0 for buffer only results processOneCaptureResult(nonBatchedResult); } diff --git a/camera/device/3.2/default/CameraDeviceSession.h b/camera/device/3.2/default/CameraDeviceSession.h index d559c48f73..fb3fc02a0f 100644 --- a/camera/device/3.2/default/CameraDeviceSession.h +++ b/camera/device/3.2/default/CameraDeviceSession.h @@ -184,6 +184,9 @@ private: std::vector mShutterMsgs; struct BufferBatch { + BufferBatch(uint32_t batchSize) { + mBuffers.reserve(batchSize); + } bool mDelivered = false; // This currently assumes every batched request will output to the batched stream // and since HAL must always send buffers in order, no frameNumber tracking is @@ -241,6 +244,11 @@ private: void processOneCaptureResult(CaptureResult& result); void invokeProcessCaptureResultCallback(hidl_vec &results, bool tryWriteFmq); + // move/push function avoids "hidl_handle& operator=(hidl_handle&)", which clones native + // handle + void moveStreamBuffer(StreamBuffer&& src, StreamBuffer& dst); + void pushStreamBuffer(StreamBuffer&& src, std::vector& dst); + // Protect access to mInflightBatches, mNumPartialResults and mStreamsToBatch // processCaptureRequest, processCaptureResult, notify will compete for this lock // Do NOT issue HIDL IPCs while holding this lock (except when HAL reports error)