From 43549d934ea08427d21401dabc0f3067fe2bf5bb Mon Sep 17 00:00:00 2001 From: Avichal Rakesh Date: Thu, 25 Jan 2024 16:08:52 -0800 Subject: [PATCH 1/3] ExternalCameraHAL: Skip importing buffer from capture request ExternalCameraHAL supports HAL buffer management which means cameraservice will not send it an output buffer along with the capture request, and the HAL has the freedom to request an output buffer when an output buffer is needed. As a remnant of migration from HIDL to AIDL, the ExternalCameraHAL still attempted to import buffers that the cameraservice has sent with the CaptureRequest. However, with HAL buffer manager enabled, this buffer is always null and results in the HAL failing to process the capture request. This CL removes the logic for importing output buffers when processing capture requests from the cameraservice, and lets the HAL call requestStreamBuffers when it needs an output buffer. Bug: 299182874 Test: VTS Tests now pass Change-Id: I00654836b7ae91a91a2afa4b149712977e07dcc5 --- .../default/ExternalCameraDeviceSession.cpp | 76 ++++--------------- .../default/ExternalCameraDeviceSession.h | 9 --- 2 files changed, 15 insertions(+), 70 deletions(-) diff --git a/camera/device/default/ExternalCameraDeviceSession.cpp b/camera/device/default/ExternalCameraDeviceSession.cpp index a16dd7fd83..479167cf62 100644 --- a/camera/device/default/ExternalCameraDeviceSession.cpp +++ b/camera/device/default/ExternalCameraDeviceSession.cpp @@ -538,6 +538,19 @@ Status ExternalCameraDeviceSession::processOneCaptureRequest(const CaptureReques return Status::INTERNAL_ERROR; } + if (request.outputBuffers.empty()) { + ALOGE("%s: No output buffers provided.", __FUNCTION__); + return Status::ILLEGAL_ARGUMENT; + } + + for (auto& outputBuf : request.outputBuffers) { + if (outputBuf.streamId == -1 || mStreamMap.find(outputBuf.streamId) == mStreamMap.end()) { + ALOGE("%s: Invalid streamId in CaptureRequest.outputBuffers: %d", __FUNCTION__, + outputBuf.streamId); + return Status::ILLEGAL_ARGUMENT; + } + } + const camera_metadata_t* rawSettings = nullptr; bool converted; CameraMetadata settingsFmq; // settings from FMQ @@ -572,8 +585,6 @@ Status ExternalCameraDeviceSession::processOneCaptureRequest(const CaptureReques return Status::ILLEGAL_ARGUMENT; } - std::vector allBufPtrs; - std::vector allFences; size_t numOutputBufs = request.outputBuffers.size(); if (numOutputBufs == 0) { @@ -629,11 +640,6 @@ Status ExternalCameraDeviceSession::processOneCaptureRequest(const CaptureReques } } - status = importRequestLocked(request, allBufPtrs, allFences); - if (status != Status::OK) { - return status; - } - nsecs_t shutterTs = 0; std::unique_ptr frameIn = dequeueV4l2FrameLocked(&shutterTs); if (frameIn == nullptr) { @@ -656,8 +662,8 @@ Status ExternalCameraDeviceSession::processOneCaptureRequest(const CaptureReques halBuf.height = stream.height; halBuf.format = stream.format; halBuf.usage = stream.usage; - halBuf.bufPtr = allBufPtrs[i]; - halBuf.acquireFence = allFences[i]; + halBuf.bufPtr = nullptr; // threadloop will request buffer from cameraservice + halBuf.acquireFence = 0; // threadloop will request fence from cameraservice halBuf.fenceTimeout = false; } { @@ -1351,58 +1357,6 @@ bool ExternalCameraDeviceSession::isSupported( return false; } -Status ExternalCameraDeviceSession::importRequestLocked(const CaptureRequest& request, - std::vector& allBufPtrs, - std::vector& allFences) { - return importRequestLockedImpl(request, allBufPtrs, allFences); -} - -Status ExternalCameraDeviceSession::importRequestLockedImpl( - const CaptureRequest& request, std::vector& allBufPtrs, - std::vector& allFences) { - size_t numOutputBufs = request.outputBuffers.size(); - size_t numBufs = numOutputBufs; - // Validate all I/O buffers - std::vector allBufs; - std::vector allBufIds; - allBufs.resize(numBufs); - allBufIds.resize(numBufs); - allBufPtrs.resize(numBufs); - allFences.resize(numBufs); - std::vector streamIds(numBufs); - - for (size_t i = 0; i < numOutputBufs; i++) { - allBufs[i] = ::android::makeFromAidl(request.outputBuffers[i].buffer); - allBufIds[i] = request.outputBuffers[i].bufferId; - allBufPtrs[i] = &allBufs[i]; - streamIds[i] = request.outputBuffers[i].streamId; - } - - { - Mutex::Autolock _l(mCbsLock); - for (size_t i = 0; i < numBufs; i++) { - Status st = importBufferLocked(streamIds[i], allBufIds[i], allBufs[i], &allBufPtrs[i]); - if (st != Status::OK) { - // Detailed error logs printed in importBuffer - return st; - } - } - } - - // All buffers are imported. Now validate output buffer acquire fences - for (size_t i = 0; i < numOutputBufs; i++) { - native_handle_t* h = ::android::makeFromAidl(request.outputBuffers[i].acquireFence); - if (!sHandleImporter.importFence(h, allFences[i])) { - ALOGE("%s: output buffer %zu acquire fence is invalid", __FUNCTION__, i); - cleanupInflightFences(allFences, i); - native_handle_delete(h); - return Status::INTERNAL_ERROR; - } - native_handle_delete(h); - } - return Status::OK; -} - Status ExternalCameraDeviceSession::importBuffer(int32_t streamId, uint64_t bufId, buffer_handle_t buf, /*out*/ buffer_handle_t** outBufPtr) { diff --git a/camera/device/default/ExternalCameraDeviceSession.h b/camera/device/default/ExternalCameraDeviceSession.h index 736bfd1528..795b5899b6 100644 --- a/camera/device/default/ExternalCameraDeviceSession.h +++ b/camera/device/default/ExternalCameraDeviceSession.h @@ -266,15 +266,6 @@ class ExternalCameraDeviceSession : public BnCameraDeviceSession, public OutputT const std::vector& supportedFormats, const ExternalCameraConfig& cfg); - // Validate and import request's output buffers and acquire fence - Status importRequestLocked(const CaptureRequest& request, - std::vector& allBufPtrs, - std::vector& allFences); - - Status importRequestLockedImpl(const CaptureRequest& request, - std::vector& allBufPtrs, - std::vector& allFences); - Status importBufferLocked(int32_t streamId, uint64_t bufId, buffer_handle_t buf, /*out*/ buffer_handle_t** outBufPtr); static void cleanupInflightFences(std::vector& allFences, size_t numFences); From ef97150dc97716bb4d53f1dfe004769e11a07194 Mon Sep 17 00:00:00 2001 From: Tang Lee Date: Sat, 27 Jan 2024 12:19:52 +0800 Subject: [PATCH 2/3] ExternalCameraHAL: improve buffer and error handling After enabling HALL Buffer Management, it requires more careful error handling and syncing. Process the buffer request error correctly. Handle the lock and states correctly. Bug: 299182874 Test: CTS passed, expecially ./cts-tradefed run cts -m CtsCameraTestCases -t android.hardware.camera2.cts.NativeCameraDeviceTest ./cts-tradefed run cts -m CtsCameraTestCases -t android.hardware.camera2.cts.RobustnessTest Change-Id: I04d8e19a2ee78580e54340378122c724a3de8edb --- .../device/default/ExternalCameraDeviceSession.cpp | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/camera/device/default/ExternalCameraDeviceSession.cpp b/camera/device/default/ExternalCameraDeviceSession.cpp index 479167cf62..075a9f6d49 100644 --- a/camera/device/default/ExternalCameraDeviceSession.cpp +++ b/camera/device/default/ExternalCameraDeviceSession.cpp @@ -1726,8 +1726,8 @@ Status ExternalCameraDeviceSession::processCaptureRequestError( result.outputBuffers[i].bufferId = req->buffers[i].bufferId; result.outputBuffers[i].status = BufferStatus::ERROR; if (req->buffers[i].acquireFence >= 0) { - native_handle_t* handle = native_handle_create(/*numFds*/ 1, /*numInts*/ 0); - handle->data[0] = req->buffers[i].acquireFence; + // numFds = 0 for error + native_handle_t* handle = native_handle_create(/*numFds*/ 0, /*numInts*/ 0); result.outputBuffers[i].releaseFence = android::dupToAidl(handle); native_handle_delete(handle); } @@ -1961,6 +1961,7 @@ int ExternalCameraDeviceSession::BufferRequestThread::waitForBufferRequestDone( std::chrono::milliseconds timeout = std::chrono::milliseconds(kReqProcTimeoutMs); auto st = mRequestDoneCond.wait_for(lk, timeout); if (st == std::cv_status::timeout) { + mRequestingBuffer = false; ALOGE("%s: wait for buffer request finish timeout!", __FUNCTION__); return -1; } @@ -2078,6 +2079,7 @@ bool ExternalCameraDeviceSession::BufferRequestThread::threadLoop() { } } else { ALOGE("%s: requestStreamBuffers call failed!", __FUNCTION__); + return false; } mPendingReturnBufferReqs = std::move(mBufferReqs); @@ -2797,9 +2799,15 @@ bool ExternalCameraDeviceSession::OutputThread::threadLoop() { ATRACE_END(); if (res != 0) { + // HAL buffer management buffer request can fail ALOGE("%s: wait for BufferRequest done failed! res %d", __FUNCTION__, res); lk.unlock(); - return onDeviceError("%s: failed to process buffer request error!", __FUNCTION__); + Status st = parent->processCaptureRequestError(req); + if (st != Status::OK) { + return onDeviceError("%s: failed to process capture request error!", __FUNCTION__); + } + signalRequestDone(); + return true; } ALOGV("%s processing new request", __FUNCTION__); From ba1854fb0edb8e0e1fd29332199159c9b9b69e0e Mon Sep 17 00:00:00 2001 From: Tang Lee Date: Sun, 28 Jan 2024 00:34:33 +0800 Subject: [PATCH 3/3] ExternalCameraHAL: fix CTS failures with callback for errors For every request, either requestStreamBuffers fails or handling of the requested buffer fails, always trigger the processCaptureResult callback by notifying the request is ready. This avoids the errors like the service side receives fewer results than the requests and waits until timeout. Bug: 299182874 Test: cts cts-tradefed run cts \ --include-filter "CtsCameraTestCases android.hardware.camera2.cts.RobustnessTest" \ --include-filter "CtsCameraTestCases android.hardware.camera2.cts.PerformanceTest" \ --include-filter "CtsCameraTestCases android.hardware.camera2.cts.StillCaptureTest" \ --include-filter "CtsCameraTestCases android.hardware.camera2.cts.SurfaceViewPreviewTest" \ --include-filter "CtsCameraTestCases android.hardware.cts.CameraGLTest" \ --include-filter "CtsCameraTestCases android.hardware.cts.LegacyCameraPerformanceTest" \ Change-Id: I86ba422524e79af6b318b50bd6eebe2cb27fa50a --- .../default/ExternalCameraDeviceSession.cpp | 43 ++++++++++++++++--- 1 file changed, 38 insertions(+), 5 deletions(-) diff --git a/camera/device/default/ExternalCameraDeviceSession.cpp b/camera/device/default/ExternalCameraDeviceSession.cpp index 075a9f6d49..91196d4228 100644 --- a/camera/device/default/ExternalCameraDeviceSession.cpp +++ b/camera/device/default/ExternalCameraDeviceSession.cpp @@ -1965,6 +1965,12 @@ int ExternalCameraDeviceSession::BufferRequestThread::waitForBufferRequestDone( ALOGE("%s: wait for buffer request finish timeout!", __FUNCTION__); return -1; } + + if (mPendingReturnBufferReqs.empty()) { + mRequestingBuffer = false; + ALOGE("%s: cameraservice did not return any buffers!", __FUNCTION__); + return -1; + } } mRequestingBuffer = false; *outBufReqs = std::move(mPendingReturnBufferReqs); @@ -2014,6 +2020,8 @@ bool ExternalCameraDeviceSession::BufferRequestThread::threadLoop() { if (!ret.isOk()) { ALOGE("%s: Transaction error: %d:%d", __FUNCTION__, ret.getExceptionCode(), ret.getServiceSpecificError()); + mBufferReqs.clear(); + mRequestDoneCond.notify_one(); return false; } @@ -2022,17 +2030,24 @@ bool ExternalCameraDeviceSession::BufferRequestThread::threadLoop() { if (bufRets.size() != mHalBufferReqs.size()) { ALOGE("%s: expect %zu buffer requests returned, only got %zu", __FUNCTION__, mHalBufferReqs.size(), bufRets.size()); + mBufferReqs.clear(); + lk.unlock(); + mRequestDoneCond.notify_one(); return false; } auto parent = mParent.lock(); if (parent == nullptr) { ALOGE("%s: session has been disconnected!", __FUNCTION__); + mBufferReqs.clear(); + lk.unlock(); + mRequestDoneCond.notify_one(); return false; } std::vector importedFences; importedFences.resize(bufRets.size()); + bool hasError = false; for (size_t i = 0; i < bufRets.size(); i++) { int streamId = bufRets[i].streamId; switch (bufRets[i].val.getTag()) { @@ -2043,7 +2058,8 @@ bool ExternalCameraDeviceSession::BufferRequestThread::threadLoop() { bufRets[i].val.get(); if (hBufs.size() != 1) { ALOGE("%s: expect 1 buffer returned, got %zu!", __FUNCTION__, hBufs.size()); - return false; + hasError = true; + break; } const StreamBuffer& hBuf = hBufs[0]; @@ -2060,26 +2076,38 @@ bool ExternalCameraDeviceSession::BufferRequestThread::threadLoop() { if (s != Status::OK) { ALOGE("%s: stream %d import buffer failed!", __FUNCTION__, streamId); cleanupInflightFences(importedFences, i - 1); - return false; + hasError = true; + break; } h = makeFromAidl(hBuf.acquireFence); if (!sHandleImporter.importFence(h, mBufferReqs[i].acquireFence)) { ALOGE("%s: stream %d import fence failed!", __FUNCTION__, streamId); cleanupInflightFences(importedFences, i - 1); native_handle_delete(h); - return false; + hasError = true; + break; } native_handle_delete(h); importedFences[i] = mBufferReqs[i].acquireFence; } break; default: ALOGE("%s: Unknown StreamBuffersVal!", __FUNCTION__); - return false; + hasError = true; + break; + } + if (hasError) { + mBufferReqs.clear(); + lk.unlock(); + mRequestDoneCond.notify_one(); + return true; } } } else { ALOGE("%s: requestStreamBuffers call failed!", __FUNCTION__); - return false; + mBufferReqs.clear(); + lk.unlock(); + mRequestDoneCond.notify_one(); + return true; } mPendingReturnBufferReqs = std::move(mBufferReqs); @@ -2784,6 +2812,11 @@ bool ExternalCameraDeviceSession::OutputThread::threadLoop() { if (res != 0) { // For some webcam, the first few V4L2 frames might be malformed... ALOGE("%s: Convert V4L2 frame to YU12 failed! res %d", __FUNCTION__, res); + + ATRACE_BEGIN("Wait for BufferRequest done"); + res = waitForBufferRequestDone(&req->buffers); + ATRACE_END(); + lk.unlock(); Status st = parent->processCaptureRequestError(req); if (st != Status::OK) {