From f0baa24d4333642c55e38200b80474134d2df748 Mon Sep 17 00:00:00 2001 From: John Reck Date: Thu, 13 May 2021 18:14:45 -0400 Subject: [PATCH] Fix some FD lifecycle issues Use unique_fd, it's cool Bug: 186919605 Test: make, guessing from a pretty clear error message & documentation Change-Id: Ied546565261708b7e4c2d220dce6d6a89725cc06 --- libs/hwui/renderthread/VulkanSurface.cpp | 40 +++++++++++------------- libs/hwui/renderthread/VulkanSurface.h | 3 +- 2 files changed, 21 insertions(+), 22 deletions(-) diff --git a/libs/hwui/renderthread/VulkanSurface.cpp b/libs/hwui/renderthread/VulkanSurface.cpp index 474d2ccf8600a..c55942596e604 100644 --- a/libs/hwui/renderthread/VulkanSurface.cpp +++ b/libs/hwui/renderthread/VulkanSurface.cpp @@ -329,20 +329,16 @@ void VulkanSurface::releaseBuffers() { if (bufferInfo.buffer.get() != nullptr && bufferInfo.dequeued) { int err = mNativeWindow->cancelBuffer(mNativeWindow.get(), bufferInfo.buffer.get(), - bufferInfo.dequeue_fence); + bufferInfo.dequeue_fence.release()); if (err != 0) { ALOGE("cancelBuffer[%u] failed during destroy: %s (%d)", i, strerror(-err), err); } bufferInfo.dequeued = false; - - if (bufferInfo.dequeue_fence >= 0) { - close(bufferInfo.dequeue_fence); - bufferInfo.dequeue_fence = -1; - } + bufferInfo.dequeue_fence.reset(); } LOG_ALWAYS_FATAL_IF(bufferInfo.dequeued); - LOG_ALWAYS_FATAL_IF(bufferInfo.dequeue_fence != -1); + LOG_ALWAYS_FATAL_IF(bufferInfo.dequeue_fence.ok()); bufferInfo.skSurface.reset(); bufferInfo.buffer.clear(); @@ -365,8 +361,12 @@ VulkanSurface::NativeBufferInfo* VulkanSurface::dequeueNativeBuffer() { // Since auto pre-rotation is enabled, dequeueBuffer to get the consumer driven buffer size // from ANativeWindowBuffer. ANativeWindowBuffer* buffer; - int fence_fd; - err = mNativeWindow->dequeueBuffer(mNativeWindow.get(), &buffer, &fence_fd); + base::unique_fd fence_fd; + { + int rawFd = -1; + err = mNativeWindow->dequeueBuffer(mNativeWindow.get(), &buffer, &rawFd); + fence_fd.reset(rawFd); + } if (err != 0) { ALOGE("dequeueBuffer failed: %s (%d)", strerror(-err), err); return nullptr; @@ -387,7 +387,7 @@ VulkanSurface::NativeBufferInfo* VulkanSurface::dequeueNativeBuffer() { if (err != 0) { ALOGE("native_window_set_buffers_transform(%d) failed: %s (%d)", transformHint, strerror(-err), err); - mNativeWindow->cancelBuffer(mNativeWindow.get(), buffer, fence_fd); + mNativeWindow->cancelBuffer(mNativeWindow.get(), buffer, fence_fd.release()); return nullptr; } mWindowInfo.transform = transformHint; @@ -405,19 +405,19 @@ VulkanSurface::NativeBufferInfo* VulkanSurface::dequeueNativeBuffer() { for (idx = 0; idx < mWindowInfo.bufferCount; idx++) { if (mNativeBuffers[idx].buffer.get() == buffer) { mNativeBuffers[idx].dequeued = true; - mNativeBuffers[idx].dequeue_fence = fence_fd; + mNativeBuffers[idx].dequeue_fence = std::move(fence_fd); break; } else if (mNativeBuffers[idx].buffer.get() == nullptr) { // increasing the number of buffers we have allocated mNativeBuffers[idx].buffer = buffer; mNativeBuffers[idx].dequeued = true; - mNativeBuffers[idx].dequeue_fence = fence_fd; + mNativeBuffers[idx].dequeue_fence = std::move(fence_fd); break; } } if (idx == mWindowInfo.bufferCount) { ALOGE("dequeueBuffer returned unrecognized buffer"); - mNativeWindow->cancelBuffer(mNativeWindow.get(), buffer, fence_fd); + mNativeWindow->cancelBuffer(mNativeWindow.get(), buffer, fence_fd.release()); return nullptr; } @@ -429,7 +429,7 @@ VulkanSurface::NativeBufferInfo* VulkanSurface::dequeueNativeBuffer() { kTopLeft_GrSurfaceOrigin, mWindowInfo.colorspace, nullptr); if (bufferInfo->skSurface.get() == nullptr) { ALOGE("SkSurface::MakeFromAHardwareBuffer failed"); - mNativeWindow->cancelBuffer(mNativeWindow.get(), buffer, fence_fd); + mNativeWindow->cancelBuffer(mNativeWindow.get(), buffer, fence_fd.release()); return nullptr; } } @@ -460,25 +460,23 @@ bool VulkanSurface::presentCurrentBuffer(const SkRect& dirtyRect, int semaphoreF LOG_ALWAYS_FATAL_IF(!mCurrentBufferInfo); VulkanSurface::NativeBufferInfo& currentBuffer = *mCurrentBufferInfo; - int queuedFd = (semaphoreFd != -1) ? semaphoreFd : currentBuffer.dequeue_fence; + // queueBuffer always closes fence, even on error + int queuedFd = (semaphoreFd != -1) ? semaphoreFd : currentBuffer.dequeue_fence.release(); int err = mNativeWindow->queueBuffer(mNativeWindow.get(), currentBuffer.buffer.get(), queuedFd); currentBuffer.dequeued = false; - // queueBuffer always closes fence, even on error if (err != 0) { ALOGE("queueBuffer failed: %s (%d)", strerror(-err), err); + // cancelBuffer takes ownership of the fence mNativeWindow->cancelBuffer(mNativeWindow.get(), currentBuffer.buffer.get(), - currentBuffer.dequeue_fence); + currentBuffer.dequeue_fence.release()); } else { currentBuffer.hasValidContents = true; currentBuffer.lastPresentedCount = mPresentCount; mPresentCount++; } - if (currentBuffer.dequeue_fence >= 0) { - close(currentBuffer.dequeue_fence); - currentBuffer.dequeue_fence = -1; - } + currentBuffer.dequeue_fence.reset(); return err == 0; } diff --git a/libs/hwui/renderthread/VulkanSurface.h b/libs/hwui/renderthread/VulkanSurface.h index 7c25545839619..beb71b727f516 100644 --- a/libs/hwui/renderthread/VulkanSurface.h +++ b/libs/hwui/renderthread/VulkanSurface.h @@ -15,6 +15,7 @@ */ #pragma once +#include #include #include #include @@ -57,7 +58,7 @@ private: // -1 any other time. When valid, we own the fd, and must ensure it is // closed: either by closing it explicitly when queueing the buffer, // or by passing ownership e.g. to ANativeWindow::cancelBuffer(). - int dequeue_fence = -1; + base::unique_fd dequeue_fence; bool dequeued = false; uint32_t lastPresentedCount = 0; bool hasValidContents = false;