From 48a619f8332e06ea1cd96d82719cdf5e05c69630 Mon Sep 17 00:00:00 2001 From: Yi Kong Date: Tue, 5 Jun 2018 16:34:59 -0700 Subject: Replace NULL/0 with nullptr Fixes -Wzero-as-null-pointer-constant warning. clang-tidy -checks=modernize-use-nullptr -p compile_commands.json -fix ... Test: m Bug: 68236239 Change-Id: I3a8e982ba40f9b029bafef78437b146a878f56a9 --- libs/gui/BufferQueueProducer.cpp | 52 ++++++++++++++++++++-------------------- 1 file changed, 26 insertions(+), 26 deletions(-) (limited to 'libs/gui/BufferQueueProducer.cpp') diff --git a/libs/gui/BufferQueueProducer.cpp b/libs/gui/BufferQueueProducer.cpp index c8021e4d54..ce3a90a483 100644 --- a/libs/gui/BufferQueueProducer.cpp +++ b/libs/gui/BufferQueueProducer.cpp @@ -166,7 +166,7 @@ status_t BufferQueueProducer::setMaxDequeuedBufferCount( } // Autolock scope // Call back without lock held - if (listener != NULL) { + if (listener != nullptr) { listener->onBuffersReleased(); } @@ -221,7 +221,7 @@ status_t BufferQueueProducer::setAsyncMode(bool async) { } // Autolock scope // Call back without lock held - if (listener != NULL) { + if (listener != nullptr) { listener->onBuffersReleased(); } return NO_ERROR; @@ -450,11 +450,11 @@ status_t BufferQueueProducer::dequeueBuffer(int* outSlot, sp* ou mSlots[found].mBufferState.dequeue(); - if ((buffer == NULL) || + if ((buffer == nullptr) || buffer->needsReallocation(width, height, format, BQ_LAYER_COUNT, usage)) { mSlots[found].mAcquireCalled = false; - mSlots[found].mGraphicBuffer = NULL; + mSlots[found].mGraphicBuffer = nullptr; mSlots[found].mRequestBufferCalled = false; mSlots[found].mEglDisplay = EGL_NO_DISPLAY; mSlots[found].mEglFence = EGL_NO_SYNC_KHR; @@ -472,7 +472,7 @@ status_t BufferQueueProducer::dequeueBuffer(int* outSlot, sp* ou BQ_LOGV("dequeueBuffer: setting buffer age to %" PRIu64, mCore->mBufferAge); - if (CC_UNLIKELY(mSlots[found].mFence == NULL)) { + if (CC_UNLIKELY(mSlots[found].mFence == nullptr)) { BQ_LOGE("dequeueBuffer: about to return a NULL fence - " "slot=%d w=%d h=%d format=%u", found, buffer->width, buffer->height, buffer->format); @@ -613,7 +613,7 @@ status_t BufferQueueProducer::detachBuffer(int slot) { listener = mCore->mConsumerListener; } - if (listener != NULL) { + if (listener != nullptr) { listener->onBuffersReleased(); } @@ -624,10 +624,10 @@ status_t BufferQueueProducer::detachNextBuffer(sp* outBuffer, sp* outFence) { ATRACE_CALL(); - if (outBuffer == NULL) { + if (outBuffer == nullptr) { BQ_LOGE("detachNextBuffer: outBuffer must not be NULL"); return BAD_VALUE; - } else if (outFence == NULL) { + } else if (outFence == nullptr) { BQ_LOGE("detachNextBuffer: outFence must not be NULL"); return BAD_VALUE; } @@ -671,7 +671,7 @@ status_t BufferQueueProducer::detachNextBuffer(sp* outBuffer, listener = mCore->mConsumerListener; } - if (listener != NULL) { + if (listener != nullptr) { listener->onBuffersReleased(); } @@ -682,10 +682,10 @@ status_t BufferQueueProducer::attachBuffer(int* outSlot, const sp& buffer) { ATRACE_CALL(); - if (outSlot == NULL) { + if (outSlot == nullptr) { BQ_LOGE("attachBuffer: outSlot must not be NULL"); return BAD_VALUE; - } else if (buffer == NULL) { + } else if (buffer == nullptr) { BQ_LOGE("attachBuffer: cannot attach NULL buffer"); return BAD_VALUE; } @@ -767,7 +767,7 @@ status_t BufferQueueProducer::queueBuffer(int slot, const Region& surfaceDamage = input.getSurfaceDamage(); const HdrMetadata& hdrMetadata = input.getHdrMetadata(); - if (acquireFence == NULL) { + if (acquireFence == nullptr) { BQ_LOGE("queueBuffer: fence is NULL"); return BAD_VALUE; } @@ -973,9 +973,9 @@ status_t BufferQueueProducer::queueBuffer(int slot, mCallbackCondition.wait(mCallbackMutex); } - if (frameAvailableListener != NULL) { + if (frameAvailableListener != nullptr) { frameAvailableListener->onFrameAvailable(item); - } else if (frameReplacedListener != NULL) { + } else if (frameReplacedListener != nullptr) { frameReplacedListener->onFrameReplaced(item); } @@ -1040,7 +1040,7 @@ status_t BufferQueueProducer::cancelBuffer(int slot, const sp& fence) { BQ_LOGE("cancelBuffer: slot %d is not owned by the producer " "(state = %s)", slot, mSlots[slot].mBufferState.string()); return BAD_VALUE; - } else if (fence == NULL) { + } else if (fence == nullptr) { BQ_LOGE("cancelBuffer: fence is NULL"); return BAD_VALUE; } @@ -1070,7 +1070,7 @@ int BufferQueueProducer::query(int what, int *outValue) { ATRACE_CALL(); Mutex::Autolock lock(mCore->mMutex); - if (outValue == NULL) { + if (outValue == nullptr) { BQ_LOGE("query: outValue was NULL"); return BAD_VALUE; } @@ -1146,12 +1146,12 @@ status_t BufferQueueProducer::connect(const sp& listener, return NO_INIT; } - if (mCore->mConsumerListener == NULL) { + if (mCore->mConsumerListener == nullptr) { BQ_LOGE("connect: BufferQueue has no consumer"); return NO_INIT; } - if (output == NULL) { + if (output == nullptr) { BQ_LOGE("connect: output was NULL"); return BAD_VALUE; } @@ -1189,10 +1189,10 @@ status_t BufferQueueProducer::connect(const sp& listener, output->nextFrameNumber = mCore->mFrameCounter + 1; output->bufferReplaced = false; - if (listener != NULL) { + if (listener != nullptr) { // Set up a death notification so that we can disconnect // automatically if the remote producer dies - if (IInterface::asBinder(listener)->remoteBinder() != NULL) { + if (IInterface::asBinder(listener)->remoteBinder() != nullptr) { status = IInterface::asBinder(listener)->linkToDeath( static_cast(this)); if (status != NO_ERROR) { @@ -1269,7 +1269,7 @@ status_t BufferQueueProducer::disconnect(int api, DisconnectMode mode) { mCore->freeAllBuffersLocked(); // Remove our death notification callback if we have one - if (mCore->mLinkedToDeath != NULL) { + if (mCore->mLinkedToDeath != nullptr) { sp token = IInterface::asBinder(mCore->mLinkedToDeath); // This can fail if we're here because of the death @@ -1279,8 +1279,8 @@ status_t BufferQueueProducer::disconnect(int api, DisconnectMode mode) { } mCore->mSharedBufferSlot = BufferQueueCore::INVALID_BUFFER_SLOT; - mCore->mLinkedToDeath = NULL; - mCore->mConnectedProducerListener = NULL; + mCore->mLinkedToDeath = nullptr; + mCore->mConnectedProducerListener = nullptr; mCore->mConnectedApi = BufferQueueCore::NO_CONNECTED_API; mCore->mConnectedPid = -1; mCore->mSidebandStream.clear(); @@ -1303,7 +1303,7 @@ status_t BufferQueueProducer::disconnect(int api, DisconnectMode mode) { } // Autolock scope // Call back without lock held - if (listener != NULL) { + if (listener != nullptr) { listener->onBuffersReleased(); listener->onDisconnect(); } @@ -1319,7 +1319,7 @@ status_t BufferQueueProducer::setSidebandStream(const sp& stream) listener = mCore->mConsumerListener; } // Autolock scope - if (listener != NULL) { + if (listener != nullptr) { listener->onSidebandStreamChanged(); } return NO_ERROR; @@ -1535,7 +1535,7 @@ void BufferQueueProducer::addAndGetFrameTimestamps( Mutex::Autolock lock(mCore->mMutex); listener = mCore->mConsumerListener; } - if (listener != NULL) { + if (listener != nullptr) { listener->addAndGetFrameTimestamps(newTimestamps, outDelta); } } -- cgit v1.2.3-59-g8ed1b From ad9fe277ea967b27ce7b748bc51ff15ff011f488 Mon Sep 17 00:00:00 2001 From: Jayant Chowdhary Date: Thu, 7 Mar 2019 22:36:06 -0800 Subject: BufferQueueProducer: use the correct IPCThreadState. The system variant of libgui maybe be double loaded. Also IGraphicBufferProducer functions may be called from hwbinder threads due to the presence of TWGraphicBufferProducer wrappers (hybrid interfaces). Therefore, we should use the correct IPCThreadState/ hardware::IPCThreadState to query callingPids. This also avoids access to /dev/binder in vendor processes, in case the system variant of the library is loaded, for eg: in libmediandk. Bug: 124128212 Test: Selinux denials realted to/dev/binder acccess are not present when AImageReader from libmediandk is used in a vendor process. Test: Play Youtube movies on Chrome, use camera to take pictures/ record videos (sanity). Change-Id: I27d78e30e16b7df5e3dfbb130121f3d7078671a3 Signed-off-by: Jayant Chowdhary --- libs/gui/Android.bp | 10 ++++-- libs/gui/BufferQueueConsumer.cpp | 29 ++++++++++++----- libs/gui/BufferQueueProducer.cpp | 5 +-- libs/gui/BufferQueueThreadState.cpp | 38 ++++++++++++++++++++++ .../include/private/gui/BufferQueueThreadState.h | 30 +++++++++++++++++ 5 files changed, 99 insertions(+), 13 deletions(-) create mode 100644 libs/gui/BufferQueueThreadState.cpp create mode 100644 libs/gui/include/private/gui/BufferQueueThreadState.h (limited to 'libs/gui/BufferQueueProducer.cpp') diff --git a/libs/gui/Android.bp b/libs/gui/Android.bp index 0510492803..8b663bc8ec 100644 --- a/libs/gui/Android.bp +++ b/libs/gui/Android.bp @@ -91,6 +91,7 @@ cc_library_shared { "BufferQueueConsumer.cpp", "BufferQueueCore.cpp", "BufferQueueProducer.cpp", + "BufferQueueThreadState.cpp", "BufferSlot.cpp", "ConsumerBase.cpp", "CpuConsumer.cpp", @@ -128,6 +129,7 @@ cc_library_shared { "libbase", "libsync", "libbinder", + "libhwbinder", "libbufferhub", "libbufferhubqueue", // TODO(b/70046255): Remove this once BufferHub is integrated into libgui. "libpdx_default_transport", @@ -143,12 +145,16 @@ cc_library_shared { "libhidltransport", "android.hidl.token@1.0-utils", "android.hardware.graphics.bufferqueue@1.0", + "libvndksupport", ], // bufferhub is not used when building libgui for vendors target: { vendor: { - cflags: ["-DNO_BUFFERHUB", "-DNO_INPUT"], + cflags: [ + "-DNO_BUFFERHUB", + "-DNO_INPUT", + ], exclude_srcs: [ "BufferHubConsumer.cpp", "BufferHubProducer.cpp", @@ -158,7 +164,7 @@ cc_library_shared { "libbufferhub", "libbufferhubqueue", "libpdx_default_transport", - "libinput" + "libinput", ], }, }, diff --git a/libs/gui/BufferQueueConsumer.cpp b/libs/gui/BufferQueueConsumer.cpp index 3837c3e11a..f2d5c8edd8 100644 --- a/libs/gui/BufferQueueConsumer.cpp +++ b/libs/gui/BufferQueueConsumer.cpp @@ -34,9 +34,10 @@ #include #include -#include +#include #ifndef __ANDROID_VNDK__ #include +#include #endif #include @@ -758,19 +759,29 @@ status_t BufferQueueConsumer::dumpState(const String8& prefix, String8* outResul return savedErrno ? -savedErrno : UNKNOWN_ERROR; } - const IPCThreadState* ipc = IPCThreadState::self(); - const uid_t uid = ipc->getCallingUid(); + bool denied = false; + const uid_t uid = BufferQueueThreadState::getCallingUid(); #ifndef __ANDROID_VNDK__ // permission check can't be done for vendors as vendors have no access to - // the PermissionController - const pid_t pid = ipc->getCallingPid(); - if ((uid != shellUid) && - !PermissionCache::checkPermission(String16("android.permission.DUMP"), pid, uid)) { - outResult->appendFormat("Permission Denial: can't dump BufferQueueConsumer " - "from pid=%d, uid=%d\n", pid, uid); + // the PermissionController. We need to do a runtime check as well, since + // the system variant of libgui can be loaded in a vendor process. For eg: + // if a HAL uses an llndk library that depends on libgui (libmediandk etc). + if (!android_is_in_vendor_process()) { + const pid_t pid = BufferQueueThreadState::getCallingPid(); + if ((uid != shellUid) && + !PermissionCache::checkPermission(String16("android.permission.DUMP"), pid, uid)) { + outResult->appendFormat("Permission Denial: can't dump BufferQueueConsumer " + "from pid=%d, uid=%d\n", + pid, uid); + denied = true; + } + } #else if (uid != shellUid) { + denied = true; + } #endif + if (denied) { android_errorWriteWithInfoLog(0x534e4554, "27046057", static_cast(uid), nullptr, 0); return PERMISSION_DENIED; diff --git a/libs/gui/BufferQueueProducer.cpp b/libs/gui/BufferQueueProducer.cpp index 5e250a4185..a462b0362f 100644 --- a/libs/gui/BufferQueueProducer.cpp +++ b/libs/gui/BufferQueueProducer.cpp @@ -35,6 +35,7 @@ #include #include #include +#include #include #include @@ -1210,7 +1211,7 @@ status_t BufferQueueProducer::connect(const sp& listener, status = BAD_VALUE; break; } - mCore->mConnectedPid = IPCThreadState::self()->getCallingPid(); + mCore->mConnectedPid = BufferQueueThreadState::getCallingPid(); mCore->mBufferHasBeenQueued = false; mCore->mDequeueBufferCannotBlock = false; if (mDequeueTimeout < 0) { @@ -1233,7 +1234,7 @@ status_t BufferQueueProducer::disconnect(int api, DisconnectMode mode) { Mutex::Autolock lock(mCore->mMutex); if (mode == DisconnectMode::AllLocal) { - if (IPCThreadState::self()->getCallingPid() != mCore->mConnectedPid) { + if (BufferQueueThreadState::getCallingPid() != mCore->mConnectedPid) { return NO_ERROR; } api = BufferQueueCore::CURRENTLY_CONNECTED_API; diff --git a/libs/gui/BufferQueueThreadState.cpp b/libs/gui/BufferQueueThreadState.cpp new file mode 100644 index 0000000000..3b531ec752 --- /dev/null +++ b/libs/gui/BufferQueueThreadState.cpp @@ -0,0 +1,38 @@ +/* + * Copyright 2019 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include +#include +#include +#include + +namespace android { + +uid_t BufferQueueThreadState::getCallingUid() { + if (hardware::IPCThreadState::self()->isServingCall()) { + return hardware::IPCThreadState::self()->getCallingUid(); + } + return IPCThreadState::self()->getCallingUid(); +} + +pid_t BufferQueueThreadState::getCallingPid() { + if (hardware::IPCThreadState::self()->isServingCall()) { + return hardware::IPCThreadState::self()->getCallingPid(); + } + return IPCThreadState::self()->getCallingPid(); +} + +} // namespace android diff --git a/libs/gui/include/private/gui/BufferQueueThreadState.h b/libs/gui/include/private/gui/BufferQueueThreadState.h new file mode 100644 index 0000000000..67dcf621ce --- /dev/null +++ b/libs/gui/include/private/gui/BufferQueueThreadState.h @@ -0,0 +1,30 @@ +/* + * Copyright 2019 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include +#include +#include + +namespace android { + +// TODO: Replace this with b/127962003 +class BufferQueueThreadState { +public: + static pid_t getCallingPid(); + static uid_t getCallingUid(); +}; + +} // namespace android -- cgit v1.2.3-59-g8ed1b From d7b3a8bcf9946a32213812a46f9c88a910151686 Mon Sep 17 00:00:00 2001 From: Alec Mouri Date: Thu, 21 Mar 2019 11:44:18 -0700 Subject: Create EGLImages during buffer allocation EGLImage creation is now performed on an async binder thread, so now GPU composition should rarely be stalled by expensive image creation. Bug: 129008989 Test: systrace Change-Id: I9732f866933a8950a4c69ff51d5ac1622bbb3470 --- libs/gui/BufferQueue.cpp | 7 ++++ libs/gui/BufferQueueProducer.cpp | 14 +++++++ libs/gui/ConsumerBase.cpp | 2 + libs/gui/IConsumerListener.cpp | 10 ++++- libs/gui/include/gui/BufferQueue.h | 1 + libs/gui/include/gui/ConsumerBase.h | 1 + libs/gui/include/gui/IConsumerListener.h | 7 ++++ libs/renderengine/gl/GLESRenderEngine.cpp | 43 +++++++++++++++++----- libs/renderengine/gl/GLESRenderEngine.h | 13 +++++-- .../include/renderengine/RenderEngine.h | 10 ++++- .../include/renderengine/mock/RenderEngine.h | 4 +- libs/renderengine/tests/RenderEngineTest.cpp | 14 +++++++ services/surfaceflinger/BufferLayerConsumer.cpp | 32 +++++++++++++++- services/surfaceflinger/BufferLayerConsumer.h | 19 +++++++--- 14 files changed, 152 insertions(+), 25 deletions(-) (limited to 'libs/gui/BufferQueueProducer.cpp') diff --git a/libs/gui/BufferQueue.cpp b/libs/gui/BufferQueue.cpp index 5fb3f0b80f..d87228fbbd 100644 --- a/libs/gui/BufferQueue.cpp +++ b/libs/gui/BufferQueue.cpp @@ -59,6 +59,13 @@ void BufferQueue::ProxyConsumerListener::onFrameReplaced( } } +void BufferQueue::ProxyConsumerListener::onBufferAllocated(const BufferItem& item) { + sp listener(mConsumerListener.promote()); + if (listener != nullptr) { + listener->onBufferAllocated(item); + } +} + void BufferQueue::ProxyConsumerListener::onBuffersReleased() { sp listener(mConsumerListener.promote()); if (listener != nullptr) { diff --git a/libs/gui/BufferQueueProducer.cpp b/libs/gui/BufferQueueProducer.cpp index a462b0362f..e657488969 100644 --- a/libs/gui/BufferQueueProducer.cpp +++ b/libs/gui/BufferQueueProducer.cpp @@ -530,6 +530,13 @@ status_t BufferQueueProducer::dequeueBuffer(int* outSlot, sp* ou return NO_INIT; } + if (mCore->mConsumerListener != nullptr) { + BufferItem item; + item.mGraphicBuffer = graphicBuffer; + item.mSlot = *outSlot; + mCore->mConsumerListener->onBufferAllocated(item); + } + VALIDATE_CONSISTENCY(); } // Autolock scope } @@ -1414,6 +1421,13 @@ void BufferQueueProducer::allocateBuffers(uint32_t width, uint32_t height, BQ_LOGV("allocateBuffers: allocated a new buffer in slot %d", *slot); + if (mCore->mConsumerListener != nullptr) { + BufferItem item; + item.mGraphicBuffer = buffers[i]; + item.mSlot = *slot; + mCore->mConsumerListener->onBufferAllocated(item); + } + // Make sure the erase is done after all uses of the slot // iterator since it will be invalid after this point. mCore->mFreeSlots.erase(slot); diff --git a/libs/gui/ConsumerBase.cpp b/libs/gui/ConsumerBase.cpp index abd9921fa9..1e94cc13cd 100644 --- a/libs/gui/ConsumerBase.cpp +++ b/libs/gui/ConsumerBase.cpp @@ -131,6 +131,8 @@ void ConsumerBase::onFrameReplaced(const BufferItem &item) { } } +void ConsumerBase::onBufferAllocated(const BufferItem& /*item*/) {} + void ConsumerBase::onBuffersReleased() { Mutex::Autolock lock(mMutex); diff --git a/libs/gui/IConsumerListener.cpp b/libs/gui/IConsumerListener.cpp index 85ac304ab8..ea9045cad9 100644 --- a/libs/gui/IConsumerListener.cpp +++ b/libs/gui/IConsumerListener.cpp @@ -28,7 +28,8 @@ enum class Tag : uint32_t { ON_FRAME_REPLACED, ON_BUFFERS_RELEASED, ON_SIDEBAND_STREAM_CHANGED, - LAST = ON_SIDEBAND_STREAM_CHANGED, + ON_BUFFER_ALLOCATED, + LAST = ON_BUFFER_ALLOCATED, }; } // Anonymous namespace @@ -54,6 +55,11 @@ public: item); } + void onBufferAllocated(const BufferItem& item) override { + callRemoteAsync(Tag::ON_BUFFER_ALLOCATED, + item); + } + void onBuffersReleased() override { callRemoteAsync(Tag::ON_BUFFERS_RELEASED); } @@ -89,6 +95,8 @@ status_t BnConsumerListener::onTransact(uint32_t code, const Parcel& data, Parce return callLocalAsync(data, reply, &IConsumerListener::onFrameAvailable); case Tag::ON_FRAME_REPLACED: return callLocalAsync(data, reply, &IConsumerListener::onFrameReplaced); + case Tag::ON_BUFFER_ALLOCATED: + return callLocalAsync(data, reply, &IConsumerListener::onBufferAllocated); case Tag::ON_BUFFERS_RELEASED: return callLocalAsync(data, reply, &IConsumerListener::onBuffersReleased); case Tag::ON_SIDEBAND_STREAM_CHANGED: diff --git a/libs/gui/include/gui/BufferQueue.h b/libs/gui/include/gui/BufferQueue.h index da952744f3..721427be7b 100644 --- a/libs/gui/include/gui/BufferQueue.h +++ b/libs/gui/include/gui/BufferQueue.h @@ -61,6 +61,7 @@ public: void onDisconnect() override; void onFrameAvailable(const BufferItem& item) override; void onFrameReplaced(const BufferItem& item) override; + void onBufferAllocated(const BufferItem& item) override; void onBuffersReleased() override; void onSidebandStreamChanged() override; void addAndGetFrameTimestamps( diff --git a/libs/gui/include/gui/ConsumerBase.h b/libs/gui/include/gui/ConsumerBase.h index 366ced380b..7c26482509 100644 --- a/libs/gui/include/gui/ConsumerBase.h +++ b/libs/gui/include/gui/ConsumerBase.h @@ -141,6 +141,7 @@ protected: // classes if they want the notification. virtual void onFrameAvailable(const BufferItem& item) override; virtual void onFrameReplaced(const BufferItem& item) override; + virtual void onBufferAllocated(const BufferItem& item) override; virtual void onBuffersReleased() override; virtual void onSidebandStreamChanged() override; diff --git a/libs/gui/include/gui/IConsumerListener.h b/libs/gui/include/gui/IConsumerListener.h index c0828820e3..03fefbe90c 100644 --- a/libs/gui/include/gui/IConsumerListener.h +++ b/libs/gui/include/gui/IConsumerListener.h @@ -61,6 +61,13 @@ public: // This is called without any lock held and can be called concurrently by multiple threads. virtual void onFrameReplaced(const BufferItem& /* item */) {} /* Asynchronous */ + // onBufferAllocated is called to notify the buffer consumer that the BufferQueue has allocated + // a GraphicBuffer for a particular slot. Only the GraphicBuffer pointer and the slot ID will + // be populated. + // + // This is called without any lock held and can be called concurrently by multiple threads. + virtual void onBufferAllocated(const BufferItem& /* item */) {} /* Asynchronous */ + // onBuffersReleased is called to notify the buffer consumer that the BufferQueue has released // its references to one or more GraphicBuffers contained in its slots. The buffer consumer // should then call BufferQueue::getReleasedBuffers to retrieve the list of buffers. diff --git a/libs/renderengine/gl/GLESRenderEngine.cpp b/libs/renderengine/gl/GLESRenderEngine.cpp index 1980f50bac..fb71ce5332 100644 --- a/libs/renderengine/gl/GLESRenderEngine.cpp +++ b/libs/renderengine/gl/GLESRenderEngine.cpp @@ -626,23 +626,26 @@ void GLESRenderEngine::bindExternalTextureImage(uint32_t texName, const Image& i } } -status_t GLESRenderEngine::bindExternalTextureBuffer(uint32_t texName, sp buffer, - sp bufferFence) { +status_t GLESRenderEngine::cacheExternalTextureBuffer(const sp& buffer) { + std::lock_guard lock(mRenderingMutex); + return cacheExternalTextureBufferLocked(buffer); +} + +status_t GLESRenderEngine::bindExternalTextureBuffer(uint32_t texName, + const sp& buffer, + const sp& bufferFence) { std::lock_guard lock(mRenderingMutex); return bindExternalTextureBufferLocked(texName, buffer, bufferFence); } -status_t GLESRenderEngine::bindExternalTextureBufferLocked(uint32_t texName, - sp buffer, - sp bufferFence) { +status_t GLESRenderEngine::cacheExternalTextureBufferLocked(const sp& buffer) { if (buffer == nullptr) { return BAD_VALUE; } + ATRACE_CALL(); - auto cachedImage = mImageCache.find(buffer->getId()); - if (cachedImage != mImageCache.end()) { - bindExternalTextureImage(texName, *cachedImage->second); + if (mImageCache.count(buffer->getId()) > 0) { return NO_ERROR; } @@ -654,11 +657,32 @@ status_t GLESRenderEngine::bindExternalTextureBufferLocked(uint32_t texName, ALOGE("Failed to create image. size=%ux%u st=%u usage=%#" PRIx64 " fmt=%d", buffer->getWidth(), buffer->getHeight(), buffer->getStride(), buffer->getUsage(), buffer->getPixelFormat()); + return NO_INIT; + } + mImageCache.insert(std::make_pair(buffer->getId(), std::move(newImage))); + + return NO_ERROR; +} + +status_t GLESRenderEngine::bindExternalTextureBufferLocked(uint32_t texName, + const sp& buffer, + const sp& bufferFence) { + ATRACE_CALL(); + status_t cacheResult = cacheExternalTextureBufferLocked(buffer); + + if (cacheResult != NO_ERROR) { + return cacheResult; + } + + auto cachedImage = mImageCache.find(buffer->getId()); + + if (cachedImage == mImageCache.end()) { + // We failed creating the image if we got here, so bail out. bindExternalTextureImage(texName, *createImage()); return NO_INIT; } - bindExternalTextureImage(texName, *newImage); + bindExternalTextureImage(texName, *cachedImage->second); // Wait for the new buffer to be ready. if (bufferFence != nullptr && bufferFence->isValid()) { @@ -680,7 +704,6 @@ status_t GLESRenderEngine::bindExternalTextureBufferLocked(uint32_t texName, } } } - mImageCache.insert(std::make_pair(buffer->getId(), std::move(newImage))); return NO_ERROR; } diff --git a/libs/renderengine/gl/GLESRenderEngine.h b/libs/renderengine/gl/GLESRenderEngine.h index 8c8f308d3b..613629e4a1 100644 --- a/libs/renderengine/gl/GLESRenderEngine.h +++ b/libs/renderengine/gl/GLESRenderEngine.h @@ -72,8 +72,9 @@ public: void genTextures(size_t count, uint32_t* names) override; void deleteTextures(size_t count, uint32_t const* names) override; void bindExternalTextureImage(uint32_t texName, const Image& image) override; - status_t bindExternalTextureBuffer(uint32_t texName, sp buffer, sp fence) - EXCLUDES(mRenderingMutex); + status_t bindExternalTextureBuffer(uint32_t texName, const sp& buffer, + const sp& fence) EXCLUDES(mRenderingMutex); + status_t cacheExternalTextureBuffer(const sp& buffer) EXCLUDES(mRenderingMutex); void unbindExternalTextureBuffer(uint64_t bufferId) EXCLUDES(mRenderingMutex); status_t bindFrameBuffer(Framebuffer* framebuffer) override; void unbindFrameBuffer(Framebuffer* framebuffer) override; @@ -219,8 +220,12 @@ private: // See bindExternalTextureBuffer above, but requiring that mRenderingMutex // is held. - status_t bindExternalTextureBufferLocked(uint32_t texName, sp buffer, - sp fence) REQUIRES(mRenderingMutex); + status_t bindExternalTextureBufferLocked(uint32_t texName, const sp& buffer, + const sp& fence) REQUIRES(mRenderingMutex); + // See cacheExternalTextureBuffer above, but requiring that mRenderingMutex + // is held. + status_t cacheExternalTextureBufferLocked(const sp& buffer) + REQUIRES(mRenderingMutex); std::unique_ptr mDrawingBuffer; diff --git a/libs/renderengine/include/renderengine/RenderEngine.h b/libs/renderengine/include/renderengine/RenderEngine.h index b211551348..6773859eb7 100644 --- a/libs/renderengine/include/renderengine/RenderEngine.h +++ b/libs/renderengine/include/renderengine/RenderEngine.h @@ -106,8 +106,14 @@ public: virtual void genTextures(size_t count, uint32_t* names) = 0; virtual void deleteTextures(size_t count, uint32_t const* names) = 0; virtual void bindExternalTextureImage(uint32_t texName, const Image& image) = 0; - virtual status_t bindExternalTextureBuffer(uint32_t texName, sp buffer, - sp fence) = 0; + // Legacy public method used by devices that don't support native fence + // synchronization in their GPU driver, as this method provides implicit + // synchronization for latching buffers. + virtual status_t bindExternalTextureBuffer(uint32_t texName, const sp& buffer, + const sp& fence) = 0; + // Caches Image resources for this buffer, but does not bind the buffer to + // a particular texture. + virtual status_t cacheExternalTextureBuffer(const sp& buffer) = 0; // Removes internal resources referenced by the bufferId. This method should be // invoked when the caller will no longer hold a reference to a GraphicBuffer // and needs to clean up its resources. diff --git a/libs/renderengine/include/renderengine/mock/RenderEngine.h b/libs/renderengine/include/renderengine/mock/RenderEngine.h index 479c7ac035..4f7d9f4352 100644 --- a/libs/renderengine/include/renderengine/mock/RenderEngine.h +++ b/libs/renderengine/include/renderengine/mock/RenderEngine.h @@ -51,7 +51,9 @@ public: MOCK_METHOD2(genTextures, void(size_t, uint32_t*)); MOCK_METHOD2(deleteTextures, void(size_t, uint32_t const*)); MOCK_METHOD2(bindExternalTextureImage, void(uint32_t, const renderengine::Image&)); - MOCK_METHOD3(bindExternalTextureBuffer, status_t(uint32_t, sp, sp)); + MOCK_METHOD1(cacheExternalTextureBuffer, status_t(const sp&)); + MOCK_METHOD3(bindExternalTextureBuffer, + status_t(uint32_t, const sp&, const sp&)); MOCK_METHOD1(unbindExternalTextureBuffer, void(uint64_t)); MOCK_CONST_METHOD0(checkErrors, void()); MOCK_METHOD4(setViewportAndProjection, diff --git a/libs/renderengine/tests/RenderEngineTest.cpp b/libs/renderengine/tests/RenderEngineTest.cpp index 8c93cf432c..24904cfaa7 100644 --- a/libs/renderengine/tests/RenderEngineTest.cpp +++ b/libs/renderengine/tests/RenderEngineTest.cpp @@ -1003,4 +1003,18 @@ TEST_F(RenderEngineTest, drawLayers_bindExternalBufferCachesImages) { EXPECT_FALSE(sRE->isImageCachedForTesting(bufferId)); } +TEST_F(RenderEngineTest, drawLayers_cacheExternalBufferWithNullBuffer) { + status_t result = sRE->cacheExternalTextureBuffer(nullptr); + ASSERT_EQ(BAD_VALUE, result); +} + +TEST_F(RenderEngineTest, drawLayers_cacheExternalBufferCachesImages) { + sp buf = allocateSourceBuffer(1, 1); + uint64_t bufferId = buf->getId(); + sRE->cacheExternalTextureBuffer(buf); + EXPECT_TRUE(sRE->isImageCachedForTesting(bufferId)); + sRE->unbindExternalTextureBuffer(bufferId); + EXPECT_FALSE(sRE->isImageCachedForTesting(bufferId)); +} + } // namespace android diff --git a/services/surfaceflinger/BufferLayerConsumer.cpp b/services/surfaceflinger/BufferLayerConsumer.cpp index f2d4c5113f..fc98dc836a 100644 --- a/services/surfaceflinger/BufferLayerConsumer.cpp +++ b/services/surfaceflinger/BufferLayerConsumer.cpp @@ -217,7 +217,11 @@ status_t BufferLayerConsumer::acquireBufferLocked(BufferItem* item, nsecs_t pres // If item->mGraphicBuffer is not null, this buffer has not been acquired // before, so we need to clean up old references. if (item->mGraphicBuffer != nullptr) { - mImages[item->mSlot] = std::make_shared(item->mGraphicBuffer, mRE); + std::lock_guard lock(mImagesMutex); + if (mImages[item->mSlot] == nullptr || mImages[item->mSlot]->graphicBuffer() == nullptr || + mImages[item->mSlot]->graphicBuffer()->getId() != item->mGraphicBuffer->getId()) { + mImages[item->mSlot] = std::make_shared(item->mGraphicBuffer, mRE); + } } return NO_ERROR; @@ -238,7 +242,12 @@ status_t BufferLayerConsumer::updateAndReleaseLocked(const BufferItem& item, // Hang onto the pointer so that it isn't freed in the call to // releaseBufferLocked() if we're in shared buffer mode and both buffers are // the same. - std::shared_ptr nextTextureBuffer = mImages[slot]; + + std::shared_ptr nextTextureBuffer; + { + std::lock_guard lock(mImagesMutex); + nextTextureBuffer = mImages[slot]; + } // release old buffer if (mCurrentTexture != BufferQueue::INVALID_BUFFER_SLOT) { @@ -436,6 +445,7 @@ status_t BufferLayerConsumer::doFenceWaitLocked() const { void BufferLayerConsumer::freeBufferLocked(int slotIndex) { BLC_LOGV("freeBufferLocked: slotIndex=%d", slotIndex); + std::lock_guard lock(mImagesMutex); if (slotIndex == mCurrentTexture) { mCurrentTexture = BufferQueue::INVALID_BUFFER_SLOT; } @@ -468,6 +478,23 @@ void BufferLayerConsumer::onSidebandStreamChanged() { } } +void BufferLayerConsumer::onBufferAllocated(const BufferItem& item) { + if (item.mGraphicBuffer != nullptr) { + std::shared_ptr image = std::make_shared(item.mGraphicBuffer, mRE); + std::shared_ptr oldImage; + { + std::lock_guard lock(mImagesMutex); + oldImage = mImages[item.mSlot]; + if (oldImage == nullptr || oldImage->graphicBuffer() == nullptr || + oldImage->graphicBuffer()->getId() != item.mGraphicBuffer->getId()) { + mImages[item.mSlot] = std::make_shared(item.mGraphicBuffer, mRE); + } + image = mImages[item.mSlot]; + } + mRE.cacheExternalTextureBuffer(image->graphicBuffer()); + } +} + void BufferLayerConsumer::addAndGetFrameTimestamps(const NewFrameEventsEntry* newTimestamps, FrameEventHistoryDelta* outDelta) { sp l = mLayer.promote(); @@ -480,6 +507,7 @@ void BufferLayerConsumer::abandonLocked() { BLC_LOGV("abandonLocked"); mCurrentTextureBuffer = nullptr; for (int i = 0; i < BufferQueue::NUM_BUFFER_SLOTS; i++) { + std::lock_guard lock(mImagesMutex); mImages[i] = nullptr; } ConsumerBase::abandonLocked(); diff --git a/services/surfaceflinger/BufferLayerConsumer.h b/services/surfaceflinger/BufferLayerConsumer.h index f4ca846ea3..0f0655d705 100644 --- a/services/surfaceflinger/BufferLayerConsumer.h +++ b/services/surfaceflinger/BufferLayerConsumer.h @@ -17,6 +17,7 @@ #ifndef ANDROID_BUFFERLAYERCONSUMER_H #define ANDROID_BUFFERLAYERCONSUMER_H +#include #include #include #include @@ -179,7 +180,7 @@ public: protected: // abandonLocked overrides the ConsumerBase method to clear // mCurrentTextureImage in addition to the ConsumerBase behavior. - virtual void abandonLocked(); + virtual void abandonLocked() EXCLUDES(mImagesMutex); // dumpLocked overrides the ConsumerBase method to dump BufferLayerConsumer- // specific info in addition to the ConsumerBase behavior. @@ -187,7 +188,8 @@ protected: // See ConsumerBase::acquireBufferLocked virtual status_t acquireBufferLocked(BufferItem* item, nsecs_t presentWhen, - uint64_t maxFrameNumber = 0) override; + uint64_t maxFrameNumber = 0) override + EXCLUDES(mImagesMutex); bool canUseImageCrop(const Rect& crop) const; @@ -206,7 +208,8 @@ protected: // completion of the method will instead be returned to the caller, so that // it may call releaseBufferLocked itself later. status_t updateAndReleaseLocked(const BufferItem& item, - PendingRelease* pendingRelease = nullptr); + PendingRelease* pendingRelease = nullptr) + EXCLUDES(mImagesMutex); // Binds mTexName and the current buffer to TEXTURE_EXTERNAL target. // If the bind succeeds, this calls doFenceWait. @@ -234,10 +237,11 @@ private: // that slot. Otherwise it has no effect. // // This method must be called with mMutex locked. - virtual void freeBufferLocked(int slotIndex); + virtual void freeBufferLocked(int slotIndex) EXCLUDES(mImagesMutex); // IConsumerListener interface void onDisconnect() override; + void onBufferAllocated(const BufferItem& item) override EXCLUDES(mImagesMutex); void onSidebandStreamChanged() override; void addAndGetFrameTimestamps(const NewFrameEventsEntry* newTimestamps, FrameEventHistoryDelta* outDelta) override; @@ -344,7 +348,12 @@ private: int mCurrentTexture; // Shadow buffer cache for cleaning up renderengine references. - std::shared_ptr mImages[BufferQueueDefs::NUM_BUFFER_SLOTS]; + std::shared_ptr mImages[BufferQueueDefs::NUM_BUFFER_SLOTS] GUARDED_BY(mImagesMutex); + + // Separate mutex guarding the shadow buffer cache. + // mImagesMutex can be manipulated with binder threads (e.g. onBuffersAllocated) + // which is contentious enough that we can't just use mMutex. + mutable std::mutex mImagesMutex; // A release that is pending on the receipt of a new release fence from // presentDisplay -- cgit v1.2.3-59-g8ed1b From 6ae550323264bacc45da2d8fca90534d02815308 Mon Sep 17 00:00:00 2001 From: Jorim Jaggi Date: Tue, 2 Apr 2019 02:27:44 +0200 Subject: Convert Mutex to std::mutex in BufferQueueCore/Producer/Consumer Bug: 111517695 Change-Id: I1226ed7396d0d768a26f4145af5fe48e2c70e8d2 --- libs/gui/BufferQueueConsumer.cpp | 50 ++++++------- libs/gui/BufferQueueCore.cpp | 6 +- libs/gui/BufferQueueProducer.cpp | 109 ++++++++++++++--------------- libs/gui/include/gui/BufferQueueCore.h | 12 ++-- libs/gui/include/gui/BufferQueueProducer.h | 7 +- 5 files changed, 92 insertions(+), 92 deletions(-) (limited to 'libs/gui/BufferQueueProducer.cpp') diff --git a/libs/gui/BufferQueueConsumer.cpp b/libs/gui/BufferQueueConsumer.cpp index f2d5c8edd8..776c6e6c11 100644 --- a/libs/gui/BufferQueueConsumer.cpp +++ b/libs/gui/BufferQueueConsumer.cpp @@ -58,7 +58,7 @@ status_t BufferQueueConsumer::acquireBuffer(BufferItem* outBuffer, int numDroppedBuffers = 0; sp listener; { - Mutex::Autolock lock(mCore->mMutex); + std::unique_lock lock(mCore->mMutex); // Check that the consumer doesn't currently have the maximum number of // buffers acquired. We allow the max buffer count to be exceeded by one @@ -203,7 +203,7 @@ status_t BufferQueueConsumer::acquireBuffer(BufferItem* outBuffer, if (sharedBufferAvailable && mCore->mQueue.empty()) { // make sure the buffer has finished allocating before acquiring it - mCore->waitWhileAllocatingLocked(); + mCore->waitWhileAllocatingLocked(lock); slot = mCore->mSharedBufferSlot; @@ -264,7 +264,7 @@ status_t BufferQueueConsumer::acquireBuffer(BufferItem* outBuffer, // We might have freed a slot while dropping old buffers, or the producer // may be blocked waiting for the number of buffers in the queue to // decrease. - mCore->mDequeueCondition.broadcast(); + mCore->mDequeueCondition.notify_all(); ATRACE_INT(mCore->mConsumerName.string(), static_cast(mCore->mQueue.size())); @@ -286,7 +286,7 @@ status_t BufferQueueConsumer::detachBuffer(int slot) { ATRACE_CALL(); ATRACE_BUFFER_INDEX(slot); BQ_LOGV("detachBuffer: slot %d", slot); - Mutex::Autolock lock(mCore->mMutex); + std::lock_guard lock(mCore->mMutex); if (mCore->mIsAbandoned) { BQ_LOGE("detachBuffer: BufferQueue has been abandoned"); @@ -312,7 +312,7 @@ status_t BufferQueueConsumer::detachBuffer(int slot) { mCore->mActiveBuffers.erase(slot); mCore->mFreeSlots.insert(slot); mCore->clearBufferSlotLocked(slot); - mCore->mDequeueCondition.broadcast(); + mCore->mDequeueCondition.notify_all(); VALIDATE_CONSISTENCY(); return NO_ERROR; @@ -330,7 +330,7 @@ status_t BufferQueueConsumer::attachBuffer(int* outSlot, return BAD_VALUE; } - Mutex::Autolock lock(mCore->mMutex); + std::lock_guard lock(mCore->mMutex); if (mCore->mSharedBufferMode) { BQ_LOGE("attachBuffer: cannot attach a buffer in shared buffer mode"); @@ -422,7 +422,7 @@ status_t BufferQueueConsumer::releaseBuffer(int slot, uint64_t frameNumber, sp listener; { // Autolock scope - Mutex::Autolock lock(mCore->mMutex); + std::lock_guard lock(mCore->mMutex); // If the frame number has changed because the buffer has been reallocated, // we can ignore this releaseBuffer for the old buffer. @@ -461,7 +461,7 @@ status_t BufferQueueConsumer::releaseBuffer(int slot, uint64_t frameNumber, listener = mCore->mConnectedProducerListener; BQ_LOGV("releaseBuffer: releasing slot %d", slot); - mCore->mDequeueCondition.broadcast(); + mCore->mDequeueCondition.notify_all(); VALIDATE_CONSISTENCY(); } // Autolock scope @@ -485,7 +485,7 @@ status_t BufferQueueConsumer::connect( BQ_LOGV("connect: controlledByApp=%s", controlledByApp ? "true" : "false"); - Mutex::Autolock lock(mCore->mMutex); + std::lock_guard lock(mCore->mMutex); if (mCore->mIsAbandoned) { BQ_LOGE("connect: BufferQueue has been abandoned"); @@ -503,7 +503,7 @@ status_t BufferQueueConsumer::disconnect() { BQ_LOGV("disconnect"); - Mutex::Autolock lock(mCore->mMutex); + std::lock_guard lock(mCore->mMutex); if (mCore->mConsumerListener == nullptr) { BQ_LOGE("disconnect: no consumer is connected"); @@ -515,7 +515,7 @@ status_t BufferQueueConsumer::disconnect() { mCore->mQueue.clear(); mCore->freeAllBuffersLocked(); mCore->mSharedBufferSlot = BufferQueueCore::INVALID_BUFFER_SLOT; - mCore->mDequeueCondition.broadcast(); + mCore->mDequeueCondition.notify_all(); return NO_ERROR; } @@ -527,7 +527,7 @@ status_t BufferQueueConsumer::getReleasedBuffers(uint64_t *outSlotMask) { return BAD_VALUE; } - Mutex::Autolock lock(mCore->mMutex); + std::lock_guard lock(mCore->mMutex); if (mCore->mIsAbandoned) { BQ_LOGE("getReleasedBuffers: BufferQueue has been abandoned"); @@ -569,7 +569,7 @@ status_t BufferQueueConsumer::setDefaultBufferSize(uint32_t width, BQ_LOGV("setDefaultBufferSize: width=%u height=%u", width, height); - Mutex::Autolock lock(mCore->mMutex); + std::lock_guard lock(mCore->mMutex); mCore->mDefaultWidth = width; mCore->mDefaultHeight = height; return NO_ERROR; @@ -583,7 +583,7 @@ status_t BufferQueueConsumer::setMaxBufferCount(int bufferCount) { return BAD_VALUE; } - Mutex::Autolock lock(mCore->mMutex); + std::lock_guard lock(mCore->mMutex); if (mCore->mConnectedApi != BufferQueueCore::NO_CONNECTED_API) { BQ_LOGE("setMaxBufferCount: producer is already connected"); @@ -623,8 +623,8 @@ status_t BufferQueueConsumer::setMaxAcquiredBufferCount( sp listener; { // Autolock scope - Mutex::Autolock lock(mCore->mMutex); - mCore->waitWhileAllocatingLocked(); + std::unique_lock lock(mCore->mMutex); + mCore->waitWhileAllocatingLocked(lock); if (mCore->mIsAbandoned) { BQ_LOGE("setMaxAcquiredBufferCount: consumer is abandoned"); @@ -684,7 +684,7 @@ status_t BufferQueueConsumer::setMaxAcquiredBufferCount( status_t BufferQueueConsumer::setConsumerName(const String8& name) { ATRACE_CALL(); BQ_LOGV("setConsumerName: '%s'", name.string()); - Mutex::Autolock lock(mCore->mMutex); + std::lock_guard lock(mCore->mMutex); mCore->mConsumerName = name; mConsumerName = name; return NO_ERROR; @@ -693,7 +693,7 @@ status_t BufferQueueConsumer::setConsumerName(const String8& name) { status_t BufferQueueConsumer::setDefaultBufferFormat(PixelFormat defaultFormat) { ATRACE_CALL(); BQ_LOGV("setDefaultBufferFormat: %u", defaultFormat); - Mutex::Autolock lock(mCore->mMutex); + std::lock_guard lock(mCore->mMutex); mCore->mDefaultBufferFormat = defaultFormat; return NO_ERROR; } @@ -702,7 +702,7 @@ status_t BufferQueueConsumer::setDefaultBufferDataSpace( android_dataspace defaultDataSpace) { ATRACE_CALL(); BQ_LOGV("setDefaultBufferDataSpace: %u", defaultDataSpace); - Mutex::Autolock lock(mCore->mMutex); + std::lock_guard lock(mCore->mMutex); mCore->mDefaultBufferDataSpace = defaultDataSpace; return NO_ERROR; } @@ -710,7 +710,7 @@ status_t BufferQueueConsumer::setDefaultBufferDataSpace( status_t BufferQueueConsumer::setConsumerUsageBits(uint64_t usage) { ATRACE_CALL(); BQ_LOGV("setConsumerUsageBits: %#" PRIx64, usage); - Mutex::Autolock lock(mCore->mMutex); + std::lock_guard lock(mCore->mMutex); mCore->mConsumerUsageBits = usage; return NO_ERROR; } @@ -718,7 +718,7 @@ status_t BufferQueueConsumer::setConsumerUsageBits(uint64_t usage) { status_t BufferQueueConsumer::setConsumerIsProtected(bool isProtected) { ATRACE_CALL(); BQ_LOGV("setConsumerIsProtected: %s", isProtected ? "true" : "false"); - Mutex::Autolock lock(mCore->mMutex); + std::lock_guard lock(mCore->mMutex); mCore->mConsumerIsProtected = isProtected; return NO_ERROR; } @@ -726,26 +726,26 @@ status_t BufferQueueConsumer::setConsumerIsProtected(bool isProtected) { status_t BufferQueueConsumer::setTransformHint(uint32_t hint) { ATRACE_CALL(); BQ_LOGV("setTransformHint: %#x", hint); - Mutex::Autolock lock(mCore->mMutex); + std::lock_guard lock(mCore->mMutex); mCore->mTransformHint = hint; return NO_ERROR; } status_t BufferQueueConsumer::getSidebandStream(sp* outStream) const { - Mutex::Autolock lock(mCore->mMutex); + std::lock_guard lock(mCore->mMutex); *outStream = mCore->mSidebandStream; return NO_ERROR; } status_t BufferQueueConsumer::getOccupancyHistory(bool forceFlush, std::vector* outHistory) { - Mutex::Autolock lock(mCore->mMutex); + std::lock_guard lock(mCore->mMutex); *outHistory = mCore->mOccupancyTracker.getSegmentHistory(forceFlush); return NO_ERROR; } status_t BufferQueueConsumer::discardFreeBuffers() { - Mutex::Autolock lock(mCore->mMutex); + std::lock_guard lock(mCore->mMutex); mCore->discardFreeBuffersLocked(); return NO_ERROR; } diff --git a/libs/gui/BufferQueueCore.cpp b/libs/gui/BufferQueueCore.cpp index 960b1948c2..96c55acdb0 100644 --- a/libs/gui/BufferQueueCore.cpp +++ b/libs/gui/BufferQueueCore.cpp @@ -110,7 +110,7 @@ BufferQueueCore::BufferQueueCore() : BufferQueueCore::~BufferQueueCore() {} void BufferQueueCore::dumpState(const String8& prefix, String8* outResult) const { - Mutex::Autolock lock(mMutex); + std::lock_guard lock(mMutex); outResult->appendFormat("%s- BufferQueue ", prefix.string()); outResult->appendFormat("mMaxAcquiredBufferCount=%d mMaxDequeuedBufferCount=%d\n", @@ -306,10 +306,10 @@ bool BufferQueueCore::adjustAvailableSlotsLocked(int delta) { return true; } -void BufferQueueCore::waitWhileAllocatingLocked() const { +void BufferQueueCore::waitWhileAllocatingLocked(std::unique_lock& lock) const { ATRACE_CALL(); while (mIsAllocating) { - mIsAllocatingCondition.wait(mMutex); + mIsAllocatingCondition.wait(lock); } } diff --git a/libs/gui/BufferQueueProducer.cpp b/libs/gui/BufferQueueProducer.cpp index a462b0362f..c17a525cb2 100644 --- a/libs/gui/BufferQueueProducer.cpp +++ b/libs/gui/BufferQueueProducer.cpp @@ -66,7 +66,7 @@ BufferQueueProducer::~BufferQueueProducer() {} status_t BufferQueueProducer::requestBuffer(int slot, sp* buf) { ATRACE_CALL(); BQ_LOGV("requestBuffer: slot %d", slot); - Mutex::Autolock lock(mCore->mMutex); + std::lock_guard lock(mCore->mMutex); if (mCore->mIsAbandoned) { BQ_LOGE("requestBuffer: BufferQueue has been abandoned"); @@ -101,8 +101,8 @@ status_t BufferQueueProducer::setMaxDequeuedBufferCount( sp listener; { // Autolock scope - Mutex::Autolock lock(mCore->mMutex); - mCore->waitWhileAllocatingLocked(); + std::unique_lock lock(mCore->mMutex); + mCore->waitWhileAllocatingLocked(lock); if (mCore->mIsAbandoned) { BQ_LOGE("setMaxDequeuedBufferCount: BufferQueue has been " @@ -163,7 +163,7 @@ status_t BufferQueueProducer::setMaxDequeuedBufferCount( if (delta < 0) { listener = mCore->mConsumerListener; } - mCore->mDequeueCondition.broadcast(); + mCore->mDequeueCondition.notify_all(); } // Autolock scope // Call back without lock held @@ -180,8 +180,8 @@ status_t BufferQueueProducer::setAsyncMode(bool async) { sp listener; { // Autolock scope - Mutex::Autolock lock(mCore->mMutex); - mCore->waitWhileAllocatingLocked(); + std::unique_lock lock(mCore->mMutex); + mCore->waitWhileAllocatingLocked(lock); if (mCore->mIsAbandoned) { BQ_LOGE("setAsyncMode: BufferQueue has been abandoned"); @@ -215,7 +215,7 @@ status_t BufferQueueProducer::setAsyncMode(bool async) { } mCore->mAsyncMode = async; VALIDATE_CONSISTENCY(); - mCore->mDequeueCondition.broadcast(); + mCore->mDequeueCondition.notify_all(); if (delta < 0) { listener = mCore->mConsumerListener; } @@ -247,7 +247,7 @@ int BufferQueueProducer::getFreeSlotLocked() const { } status_t BufferQueueProducer::waitForFreeSlotThenRelock(FreeSlotCaller caller, - int* found) const { + std::unique_lock& lock, int* found) const { auto callerString = (caller == FreeSlotCaller::Dequeue) ? "dequeueBuffer" : "attachBuffer"; bool tryAgain = true; @@ -334,13 +334,13 @@ status_t BufferQueueProducer::waitForFreeSlotThenRelock(FreeSlotCaller caller, return WOULD_BLOCK; } if (mDequeueTimeout >= 0) { - status_t result = mCore->mDequeueCondition.waitRelative( - mCore->mMutex, mDequeueTimeout); - if (result == TIMED_OUT) { - return result; + std::cv_status result = mCore->mDequeueCondition.wait_for(lock, + std::chrono::nanoseconds(mDequeueTimeout)); + if (result == std::cv_status::timeout) { + return TIMED_OUT; } } else { - mCore->mDequeueCondition.wait(mCore->mMutex); + mCore->mDequeueCondition.wait(lock); } } } // while (tryAgain) @@ -354,7 +354,7 @@ status_t BufferQueueProducer::dequeueBuffer(int* outSlot, sp* ou FrameEventHistoryDelta* outTimestamps) { ATRACE_CALL(); { // Autolock scope - Mutex::Autolock lock(mCore->mMutex); + std::lock_guard lock(mCore->mMutex); mConsumerName = mCore->mConsumerName; if (mCore->mIsAbandoned) { @@ -381,7 +381,7 @@ status_t BufferQueueProducer::dequeueBuffer(int* outSlot, sp* ou bool attachedByConsumer = false; { // Autolock scope - Mutex::Autolock lock(mCore->mMutex); + std::unique_lock lock(mCore->mMutex); if (format == 0) { format = mCore->mDefaultBufferFormat; @@ -398,8 +398,7 @@ status_t BufferQueueProducer::dequeueBuffer(int* outSlot, sp* ou int found = BufferItem::INVALID_BUFFER_SLOT; while (found == BufferItem::INVALID_BUFFER_SLOT) { - status_t status = waitForFreeSlotThenRelock(FreeSlotCaller::Dequeue, - &found); + status_t status = waitForFreeSlotThenRelock(FreeSlotCaller::Dequeue, lock, &found); if (status != NO_ERROR) { return status; } @@ -506,7 +505,7 @@ status_t BufferQueueProducer::dequeueBuffer(int* outSlot, sp* ou status_t error = graphicBuffer->initCheck(); { // Autolock scope - Mutex::Autolock lock(mCore->mMutex); + std::lock_guard lock(mCore->mMutex); if (error == NO_ERROR && !mCore->mIsAbandoned) { graphicBuffer->setGenerationNumber(mCore->mGenerationNumber); @@ -514,7 +513,7 @@ status_t BufferQueueProducer::dequeueBuffer(int* outSlot, sp* ou } mCore->mIsAllocating = false; - mCore->mIsAllocatingCondition.broadcast(); + mCore->mIsAllocatingCondition.notify_all(); if (error != NO_ERROR) { mCore->mFreeSlots.insert(*outSlot); @@ -573,7 +572,7 @@ status_t BufferQueueProducer::detachBuffer(int slot) { sp listener; { - Mutex::Autolock lock(mCore->mMutex); + std::lock_guard lock(mCore->mMutex); if (mCore->mIsAbandoned) { BQ_LOGE("detachBuffer: BufferQueue has been abandoned"); @@ -608,7 +607,7 @@ status_t BufferQueueProducer::detachBuffer(int slot) { mCore->mActiveBuffers.erase(slot); mCore->mFreeSlots.insert(slot); mCore->clearBufferSlotLocked(slot); - mCore->mDequeueCondition.broadcast(); + mCore->mDequeueCondition.notify_all(); VALIDATE_CONSISTENCY(); listener = mCore->mConsumerListener; } @@ -634,7 +633,7 @@ status_t BufferQueueProducer::detachNextBuffer(sp* outBuffer, sp listener; { - Mutex::Autolock lock(mCore->mMutex); + std::unique_lock lock(mCore->mMutex); if (mCore->mIsAbandoned) { BQ_LOGE("detachNextBuffer: BufferQueue has been abandoned"); @@ -652,7 +651,7 @@ status_t BufferQueueProducer::detachNextBuffer(sp* outBuffer, return BAD_VALUE; } - mCore->waitWhileAllocatingLocked(); + mCore->waitWhileAllocatingLocked(lock); if (mCore->mFreeBuffers.empty()) { return NO_MEMORY; @@ -690,7 +689,7 @@ status_t BufferQueueProducer::attachBuffer(int* outSlot, return BAD_VALUE; } - Mutex::Autolock lock(mCore->mMutex); + std::unique_lock lock(mCore->mMutex); if (mCore->mIsAbandoned) { BQ_LOGE("attachBuffer: BufferQueue has been abandoned"); @@ -714,11 +713,11 @@ status_t BufferQueueProducer::attachBuffer(int* outSlot, return BAD_VALUE; } - mCore->waitWhileAllocatingLocked(); + mCore->waitWhileAllocatingLocked(lock); status_t returnFlags = NO_ERROR; int found; - status_t status = waitForFreeSlotThenRelock(FreeSlotCaller::Attach, &found); + status_t status = waitForFreeSlotThenRelock(FreeSlotCaller::Attach, lock, &found); if (status != NO_ERROR) { return status; } @@ -791,7 +790,7 @@ status_t BufferQueueProducer::queueBuffer(int slot, uint64_t currentFrameNumber = 0; BufferItem item; { // Autolock scope - Mutex::Autolock lock(mCore->mMutex); + std::lock_guard lock(mCore->mMutex); if (mCore->mIsAbandoned) { BQ_LOGE("queueBuffer: BufferQueue has been abandoned"); @@ -932,7 +931,7 @@ status_t BufferQueueProducer::queueBuffer(int slot, } mCore->mBufferHasBeenQueued = true; - mCore->mDequeueCondition.broadcast(); + mCore->mDequeueCondition.notify_all(); mCore->mLastQueuedSlot = slot; output->width = mCore->mDefaultWidth; @@ -968,9 +967,9 @@ status_t BufferQueueProducer::queueBuffer(int slot, sp lastQueuedFence; { // scope for the lock - Mutex::Autolock lock(mCallbackMutex); + std::unique_lock lock(mCallbackMutex); while (callbackTicket != mCurrentCallbackTicket) { - mCallbackCondition.wait(mCallbackMutex); + mCallbackCondition.wait(lock); } if (frameAvailableListener != nullptr) { @@ -987,7 +986,7 @@ status_t BufferQueueProducer::queueBuffer(int slot, mLastQueuedTransform = item.mTransform; ++mCurrentCallbackTicket; - mCallbackCondition.broadcast(); + mCallbackCondition.notify_all(); } // Wait without lock held @@ -1015,7 +1014,7 @@ status_t BufferQueueProducer::queueBuffer(int slot, status_t BufferQueueProducer::cancelBuffer(int slot, const sp& fence) { ATRACE_CALL(); BQ_LOGV("cancelBuffer: slot %d", slot); - Mutex::Autolock lock(mCore->mMutex); + std::lock_guard lock(mCore->mMutex); if (mCore->mIsAbandoned) { BQ_LOGE("cancelBuffer: BufferQueue has been abandoned"); @@ -1060,7 +1059,7 @@ status_t BufferQueueProducer::cancelBuffer(int slot, const sp& fence) { } mSlots[slot].mFence = fence; - mCore->mDequeueCondition.broadcast(); + mCore->mDequeueCondition.notify_all(); VALIDATE_CONSISTENCY(); return NO_ERROR; @@ -1068,7 +1067,7 @@ status_t BufferQueueProducer::cancelBuffer(int slot, const sp& fence) { int BufferQueueProducer::query(int what, int *outValue) { ATRACE_CALL(); - Mutex::Autolock lock(mCore->mMutex); + std::lock_guard lock(mCore->mMutex); if (outValue == nullptr) { BQ_LOGE("query: outValue was NULL"); @@ -1136,7 +1135,7 @@ int BufferQueueProducer::query(int what, int *outValue) { status_t BufferQueueProducer::connect(const sp& listener, int api, bool producerControlledByApp, QueueBufferOutput *output) { ATRACE_CALL(); - Mutex::Autolock lock(mCore->mMutex); + std::lock_guard lock(mCore->mMutex); mConsumerName = mCore->mConsumerName; BQ_LOGV("connect: api=%d producerControlledByApp=%s", api, producerControlledByApp ? "true" : "false"); @@ -1231,7 +1230,7 @@ status_t BufferQueueProducer::disconnect(int api, DisconnectMode mode) { int status = NO_ERROR; sp listener; { // Autolock scope - Mutex::Autolock lock(mCore->mMutex); + std::unique_lock lock(mCore->mMutex); if (mode == DisconnectMode::AllLocal) { if (BufferQueueThreadState::getCallingPid() != mCore->mConnectedPid) { @@ -1240,7 +1239,7 @@ status_t BufferQueueProducer::disconnect(int api, DisconnectMode mode) { api = BufferQueueCore::CURRENTLY_CONNECTED_API; } - mCore->waitWhileAllocatingLocked(); + mCore->waitWhileAllocatingLocked(lock); if (mCore->mIsAbandoned) { // It's not really an error to disconnect after the surface has @@ -1284,7 +1283,7 @@ status_t BufferQueueProducer::disconnect(int api, DisconnectMode mode) { mCore->mConnectedApi = BufferQueueCore::NO_CONNECTED_API; mCore->mConnectedPid = -1; mCore->mSidebandStream.clear(); - mCore->mDequeueCondition.broadcast(); + mCore->mDequeueCondition.notify_all(); listener = mCore->mConsumerListener; } else if (mCore->mConnectedApi == BufferQueueCore::NO_CONNECTED_API) { BQ_LOGE("disconnect: not connected (req=%d)", api); @@ -1314,7 +1313,7 @@ status_t BufferQueueProducer::disconnect(int api, DisconnectMode mode) { status_t BufferQueueProducer::setSidebandStream(const sp& stream) { sp listener; { // Autolock scope - Mutex::Autolock _l(mCore->mMutex); + std::lock_guard _l(mCore->mMutex); mCore->mSidebandStream = stream; listener = mCore->mConsumerListener; } // Autolock scope @@ -1336,8 +1335,8 @@ void BufferQueueProducer::allocateBuffers(uint32_t width, uint32_t height, uint64_t allocUsage = 0; std::string allocName; { // Autolock scope - Mutex::Autolock lock(mCore->mMutex); - mCore->waitWhileAllocatingLocked(); + std::unique_lock lock(mCore->mMutex); + mCore->waitWhileAllocatingLocked(lock); if (!mCore->mAllowAllocation) { BQ_LOGE("allocateBuffers: allocation is not allowed for this " @@ -1372,16 +1371,16 @@ void BufferQueueProducer::allocateBuffers(uint32_t width, uint32_t height, if (result != NO_ERROR) { BQ_LOGE("allocateBuffers: failed to allocate buffer (%u x %u, format" " %u, usage %#" PRIx64 ")", width, height, format, usage); - Mutex::Autolock lock(mCore->mMutex); + std::lock_guard lock(mCore->mMutex); mCore->mIsAllocating = false; - mCore->mIsAllocatingCondition.broadcast(); + mCore->mIsAllocatingCondition.notify_all(); return; } buffers.push_back(graphicBuffer); } { // Autolock scope - Mutex::Autolock lock(mCore->mMutex); + std::unique_lock lock(mCore->mMutex); uint32_t checkWidth = width > 0 ? width : mCore->mDefaultWidth; uint32_t checkHeight = height > 0 ? height : mCore->mDefaultHeight; PixelFormat checkFormat = format != 0 ? @@ -1392,7 +1391,7 @@ void BufferQueueProducer::allocateBuffers(uint32_t width, uint32_t height, // Something changed while we released the lock. Retry. BQ_LOGV("allocateBuffers: size/format/usage changed while allocating. Retrying."); mCore->mIsAllocating = false; - mCore->mIsAllocatingCondition.broadcast(); + mCore->mIsAllocatingCondition.notify_all(); continue; } @@ -1420,7 +1419,7 @@ void BufferQueueProducer::allocateBuffers(uint32_t width, uint32_t height, } mCore->mIsAllocating = false; - mCore->mIsAllocatingCondition.broadcast(); + mCore->mIsAllocatingCondition.notify_all(); VALIDATE_CONSISTENCY(); } // Autolock scope } @@ -1430,7 +1429,7 @@ status_t BufferQueueProducer::allowAllocation(bool allow) { ATRACE_CALL(); BQ_LOGV("allowAllocation: %s", allow ? "true" : "false"); - Mutex::Autolock lock(mCore->mMutex); + std::lock_guard lock(mCore->mMutex); mCore->mAllowAllocation = allow; return NO_ERROR; } @@ -1439,14 +1438,14 @@ status_t BufferQueueProducer::setGenerationNumber(uint32_t generationNumber) { ATRACE_CALL(); BQ_LOGV("setGenerationNumber: %u", generationNumber); - Mutex::Autolock lock(mCore->mMutex); + std::lock_guard lock(mCore->mMutex); mCore->mGenerationNumber = generationNumber; return NO_ERROR; } String8 BufferQueueProducer::getConsumerName() const { ATRACE_CALL(); - Mutex::Autolock lock(mCore->mMutex); + std::lock_guard lock(mCore->mMutex); BQ_LOGV("getConsumerName: %s", mConsumerName.string()); return mConsumerName; } @@ -1455,7 +1454,7 @@ status_t BufferQueueProducer::setSharedBufferMode(bool sharedBufferMode) { ATRACE_CALL(); BQ_LOGV("setSharedBufferMode: %d", sharedBufferMode); - Mutex::Autolock lock(mCore->mMutex); + std::lock_guard lock(mCore->mMutex); if (!sharedBufferMode) { mCore->mSharedBufferSlot = BufferQueueCore::INVALID_BUFFER_SLOT; } @@ -1467,7 +1466,7 @@ status_t BufferQueueProducer::setAutoRefresh(bool autoRefresh) { ATRACE_CALL(); BQ_LOGV("setAutoRefresh: %d", autoRefresh); - Mutex::Autolock lock(mCore->mMutex); + std::lock_guard lock(mCore->mMutex); mCore->mAutoRefresh = autoRefresh; return NO_ERROR; @@ -1477,7 +1476,7 @@ status_t BufferQueueProducer::setDequeueTimeout(nsecs_t timeout) { ATRACE_CALL(); BQ_LOGV("setDequeueTimeout: %" PRId64, timeout); - Mutex::Autolock lock(mCore->mMutex); + std::lock_guard lock(mCore->mMutex); int delta = mCore->getMaxBufferCountLocked(mCore->mAsyncMode, false, mCore->mMaxBufferCount) - mCore->getMaxBufferCountLocked(); if (!mCore->adjustAvailableSlotsLocked(delta)) { @@ -1498,7 +1497,7 @@ status_t BufferQueueProducer::getLastQueuedBuffer(sp* outBuffer, ATRACE_CALL(); BQ_LOGV("getLastQueuedBuffer"); - Mutex::Autolock lock(mCore->mMutex); + std::lock_guard lock(mCore->mMutex); if (mCore->mLastQueuedSlot == BufferItem::INVALID_BUFFER_SLOT) { *outBuffer = nullptr; *outFence = Fence::NO_FENCE; @@ -1534,7 +1533,7 @@ void BufferQueueProducer::addAndGetFrameTimestamps( BQ_LOGV("addAndGetFrameTimestamps"); sp listener; { - Mutex::Autolock lock(mCore->mMutex); + std::lock_guard lock(mCore->mMutex); listener = mCore->mConsumerListener; } if (listener != nullptr) { @@ -1561,7 +1560,7 @@ status_t BufferQueueProducer::getUniqueId(uint64_t* outId) const { status_t BufferQueueProducer::getConsumerUsage(uint64_t* outUsage) const { BQ_LOGV("getConsumerUsage"); - Mutex::Autolock lock(mCore->mMutex); + std::lock_guard lock(mCore->mMutex); *outUsage = mCore->mConsumerUsageBits; return NO_ERROR; } diff --git a/libs/gui/include/gui/BufferQueueCore.h b/libs/gui/include/gui/BufferQueueCore.h index b377a412bc..0e80283804 100644 --- a/libs/gui/include/gui/BufferQueueCore.h +++ b/libs/gui/include/gui/BufferQueueCore.h @@ -22,8 +22,6 @@ #include #include -#include -#include #include #include #include @@ -33,6 +31,8 @@ #include #include +#include +#include #define BQ_LOGV(x, ...) ALOGV("[%s] " x, mConsumerName.string(), ##__VA_ARGS__) #define BQ_LOGD(x, ...) ALOGD("[%s] " x, mConsumerName.string(), ##__VA_ARGS__) @@ -134,7 +134,7 @@ private: bool adjustAvailableSlotsLocked(int delta); // waitWhileAllocatingLocked blocks until mIsAllocating is false. - void waitWhileAllocatingLocked() const; + void waitWhileAllocatingLocked(std::unique_lock& lock) const; #if DEBUG_ONLY_CODE // validateConsistencyLocked ensures that the free lists are in sync with @@ -145,7 +145,7 @@ private: // mMutex is the mutex used to prevent concurrent access to the member // variables of BufferQueueCore objects. It must be locked whenever any // member variable is accessed. - mutable Mutex mMutex; + mutable std::mutex mMutex; // mIsAbandoned indicates that the BufferQueue will no longer be used to // consume image buffers pushed to it using the IGraphicBufferProducer @@ -219,7 +219,7 @@ private: // mDequeueCondition is a condition variable used for dequeueBuffer in // synchronous mode. - mutable Condition mDequeueCondition; + mutable std::condition_variable mDequeueCondition; // mDequeueBufferCannotBlock indicates whether dequeueBuffer is allowed to // block. This flag is set during connect when both the producer and @@ -282,7 +282,7 @@ private: // mIsAllocatingCondition is a condition variable used by producers to wait until mIsAllocating // becomes false. - mutable Condition mIsAllocatingCondition; + mutable std::condition_variable mIsAllocatingCondition; // mAllowAllocation determines whether dequeueBuffer is allowed to allocate // new buffers diff --git a/libs/gui/include/gui/BufferQueueProducer.h b/libs/gui/include/gui/BufferQueueProducer.h index 73bc5dd318..b6e98862f7 100644 --- a/libs/gui/include/gui/BufferQueueProducer.h +++ b/libs/gui/include/gui/BufferQueueProducer.h @@ -211,7 +211,8 @@ private: Dequeue, Attach, }; - status_t waitForFreeSlotThenRelock(FreeSlotCaller caller, int* found) const; + status_t waitForFreeSlotThenRelock(FreeSlotCaller caller, std::unique_lock& lock, + int* found) const; sp mCore; @@ -243,10 +244,10 @@ private: // (mCore->mMutex) is held, a ticket is retained by the producer. After // dropping the BufferQueue lock, the producer must wait on the condition // variable until the current callback ticket matches its retained ticket. - Mutex mCallbackMutex; + std::mutex mCallbackMutex; int mNextCallbackTicket; // Protected by mCore->mMutex int mCurrentCallbackTicket; // Protected by mCallbackMutex - Condition mCallbackCondition; + std::condition_variable mCallbackCondition; // Sets how long dequeueBuffer or attachBuffer will block if a buffer or // slot is not yet available. -- cgit v1.2.3-59-g8ed1b From 35b4e3839a2eb405bd172351042f81d842183715 Mon Sep 17 00:00:00 2001 From: Jorim Jaggi Date: Thu, 28 Mar 2019 00:44:03 +0100 Subject: Wait for buffer allocation In dequeueBuffer, if we don't have a free slot but are in the process of allocating, we wait for that to complete instead of kicking off our own allocation, as it's likely that the allocation from allocateBuffers is able to finish earlier than if we'd kick off our own allocation. Test: Swipe up, see less initial buffer dequeue delay Fixes: 111517695 Change-Id: I735d68e87fc7e2ff5b7ec3595f0ced5a94ebbf9c --- libs/gui/BufferQueueProducer.cpp | 18 +++++++++++++++++- libs/gui/include/gui/BufferQueueProducer.h | 7 +++++++ 2 files changed, 24 insertions(+), 1 deletion(-) (limited to 'libs/gui/BufferQueueProducer.cpp') diff --git a/libs/gui/BufferQueueProducer.cpp b/libs/gui/BufferQueueProducer.cpp index c17a525cb2..b353ffef92 100644 --- a/libs/gui/BufferQueueProducer.cpp +++ b/libs/gui/BufferQueueProducer.cpp @@ -59,7 +59,8 @@ BufferQueueProducer::BufferQueueProducer(const sp& core, mNextCallbackTicket(0), mCurrentCallbackTicket(0), mCallbackCondition(), - mDequeueTimeout(-1) {} + mDequeueTimeout(-1), + mDequeueWaitingForAllocation(false) {} BufferQueueProducer::~BufferQueueProducer() {} @@ -383,6 +384,15 @@ status_t BufferQueueProducer::dequeueBuffer(int* outSlot, sp* ou { // Autolock scope std::unique_lock lock(mCore->mMutex); + // If we don't have a free buffer, but we are currently allocating, we wait until allocation + // is finished such that we don't allocate in parallel. + if (mCore->mFreeBuffers.empty() && mCore->mIsAllocating) { + mDequeueWaitingForAllocation = true; + mCore->waitWhileAllocatingLocked(lock); + mDequeueWaitingForAllocation = false; + mDequeueWaitingForAllocationCondition.notify_all(); + } + if (format == 0) { format = mCore->mDefaultBufferFormat; } @@ -1421,6 +1431,12 @@ void BufferQueueProducer::allocateBuffers(uint32_t width, uint32_t height, mCore->mIsAllocating = false; mCore->mIsAllocatingCondition.notify_all(); VALIDATE_CONSISTENCY(); + + // If dequeue is waiting for to allocate a buffer, release the lock until it's not + // waiting anymore so it can use the buffer we just allocated. + while (mDequeueWaitingForAllocation) { + mDequeueWaitingForAllocationCondition.wait(lock); + } } // Autolock scope } } diff --git a/libs/gui/include/gui/BufferQueueProducer.h b/libs/gui/include/gui/BufferQueueProducer.h index b6e98862f7..415e2a616e 100644 --- a/libs/gui/include/gui/BufferQueueProducer.h +++ b/libs/gui/include/gui/BufferQueueProducer.h @@ -253,6 +253,13 @@ private: // slot is not yet available. nsecs_t mDequeueTimeout; + // If set to true, dequeueBuffer() is currently waiting for buffer allocation to complete. + bool mDequeueWaitingForAllocation; + + // Condition variable to signal allocateBuffers() that dequeueBuffer() is no longer waiting for + // allocation to complete. + std::condition_variable mDequeueWaitingForAllocationCondition; + }; // class BufferQueueProducer } // namespace android -- cgit v1.2.3-59-g8ed1b From 3249fb6a0d8dd1e2020884711ce7d078b913ee44 Mon Sep 17 00:00:00 2001 From: Sungtak Lee Date: Sat, 2 Mar 2019 16:40:47 -0800 Subject: BufferQueueProducer::queueBuffer may not drop buffers Enable BufferQueueProducer::setDequeueTimeout retain existing dropping behavior. Setting negative and zero to setDequeueTimeout will not change existing dropping behavior. BufferQueueProducer::setLegacyBufferDrop can disable buffer dropping behavior of BufferQueueProducer::queueBuffer. If it's disabled, buffers will not be dropped unless consumer is SurfaceFlinger. Bug: 130039639 Change-Id: I8432a7ad386836498e632c67953ad49c6be008bb --- libs/gui/BufferQueueCore.cpp | 4 ++++ libs/gui/BufferQueueProducer.cpp | 27 +++++++++++++++++----- libs/gui/IGraphicBufferProducer.cpp | 32 +++++++++++++++++++++++++++ libs/gui/include/gui/BufferQueueCore.h | 10 +++++++++ libs/gui/include/gui/BufferQueueProducer.h | 3 +++ libs/gui/include/gui/IGraphicBufferProducer.h | 8 +++++++ services/surfaceflinger/MonitoredProducer.cpp | 4 ++++ services/surfaceflinger/MonitoredProducer.h | 1 + 8 files changed, 84 insertions(+), 5 deletions(-) (limited to 'libs/gui/BufferQueueProducer.cpp') diff --git a/libs/gui/BufferQueueCore.cpp b/libs/gui/BufferQueueCore.cpp index 96c55acdb0..e0e3431ca5 100644 --- a/libs/gui/BufferQueueCore.cpp +++ b/libs/gui/BufferQueueCore.cpp @@ -73,6 +73,8 @@ BufferQueueCore::BufferQueueCore() : mActiveBuffers(), mDequeueCondition(), mDequeueBufferCannotBlock(false), + mQueueBufferCanDrop(false), + mLegacyBufferDrop(true), mDefaultBufferFormat(PIXEL_FORMAT_RGBA_8888), mDefaultWidth(1), mDefaultHeight(1), @@ -117,6 +119,8 @@ void BufferQueueCore::dumpState(const String8& prefix, String8* outResult) const mMaxAcquiredBufferCount, mMaxDequeuedBufferCount); outResult->appendFormat("%s mDequeueBufferCannotBlock=%d mAsyncMode=%d\n", prefix.string(), mDequeueBufferCannotBlock, mAsyncMode); + outResult->appendFormat("%s mQueueBufferCanDrop=%d mLegacyBufferDrop=%d\n", prefix.string(), + mQueueBufferCanDrop, mLegacyBufferDrop); outResult->appendFormat("%s default-size=[%dx%d] default-format=%d ", prefix.string(), mDefaultWidth, mDefaultHeight, mDefaultBufferFormat); outResult->appendFormat("transform-hint=%02x frame-counter=%" PRIu64, mTransformHint, diff --git a/libs/gui/BufferQueueProducer.cpp b/libs/gui/BufferQueueProducer.cpp index 72ae3758c8..4ff69c57ce 100644 --- a/libs/gui/BufferQueueProducer.cpp +++ b/libs/gui/BufferQueueProducer.cpp @@ -889,7 +889,8 @@ status_t BufferQueueProducer::queueBuffer(int slot, item.mFence = acquireFence; item.mFenceTime = acquireFenceTime; item.mIsDroppable = mCore->mAsyncMode || - mCore->mDequeueBufferCannotBlock || + (!mCore->mLegacyBufferDrop && mConsumerIsSurfaceFlinger) || + (mCore->mLegacyBufferDrop && mCore->mQueueBufferCanDrop) || (mCore->mSharedBufferMode && mCore->mSharedBufferSlot == slot); item.mSurfaceDamage = surfaceDamage; item.mQueuedBuffer = true; @@ -1230,9 +1231,11 @@ status_t BufferQueueProducer::connect(const sp& listener, mCore->mConnectedPid = BufferQueueThreadState::getCallingPid(); mCore->mBufferHasBeenQueued = false; mCore->mDequeueBufferCannotBlock = false; - if (mDequeueTimeout < 0) { - mCore->mDequeueBufferCannotBlock = - mCore->mConsumerControlledByApp && producerControlledByApp; + mCore->mQueueBufferCanDrop = false; + mCore->mLegacyBufferDrop = true; + if (mCore->mConsumerControlledByApp && producerControlledByApp) { + mCore->mDequeueBufferCannotBlock = mDequeueTimeout < 0; + mCore->mQueueBufferCanDrop = mDequeueTimeout <= 0; } mCore->mAllowAllocation = true; @@ -1516,12 +1519,26 @@ status_t BufferQueueProducer::setDequeueTimeout(nsecs_t timeout) { } mDequeueTimeout = timeout; - mCore->mDequeueBufferCannotBlock = false; + if (timeout >= 0) { + mCore->mDequeueBufferCannotBlock = false; + if (timeout != 0) { + mCore->mQueueBufferCanDrop = false; + } + } VALIDATE_CONSISTENCY(); return NO_ERROR; } +status_t BufferQueueProducer::setLegacyBufferDrop(bool drop) { + ATRACE_CALL(); + BQ_LOGV("setLegacyBufferDrop: drop = %d", drop); + + std::lock_guard lock(mCore->mMutex); + mCore->mLegacyBufferDrop = drop; + return NO_ERROR; +} + status_t BufferQueueProducer::getLastQueuedBuffer(sp* outBuffer, sp* outFence, float outTransformMatrix[16]) { ATRACE_CALL(); diff --git a/libs/gui/IGraphicBufferProducer.cpp b/libs/gui/IGraphicBufferProducer.cpp index bf44121677..0e03b7d393 100644 --- a/libs/gui/IGraphicBufferProducer.cpp +++ b/libs/gui/IGraphicBufferProducer.cpp @@ -72,6 +72,7 @@ enum { GET_FRAME_TIMESTAMPS, GET_UNIQUE_ID, GET_CONSUMER_USAGE, + SET_LEGACY_BUFFER_DROP, }; class BpGraphicBufferProducer : public BpInterface @@ -437,6 +438,20 @@ public: return reply.readInt32(); } + virtual status_t setLegacyBufferDrop(bool drop) { + Parcel data, reply; + data.writeInterfaceToken( + IGraphicBufferProducer::getInterfaceDescriptor()); + data.writeInt32(drop); + status_t result = remote()->transact(SET_LEGACY_BUFFER_DROP, + data, &reply); + if (result != NO_ERROR) { + return result; + } + result = reply.readInt32(); + return result; + } + virtual status_t getLastQueuedBuffer(sp* outBuffer, sp* outFence, float outTransformMatrix[16]) override { Parcel data, reply; @@ -637,6 +652,10 @@ public: return mBase->setDequeueTimeout(timeout); } + status_t setLegacyBufferDrop(bool drop) override { + return mBase->setLegacyBufferDrop(drop); + } + status_t getLastQueuedBuffer( sp* outBuffer, sp* outFence, @@ -663,6 +682,12 @@ IMPLEMENT_HYBRID_META_INTERFACE(GraphicBufferProducer, // ---------------------------------------------------------------------- +status_t IGraphicBufferProducer::setLegacyBufferDrop(bool drop) { + // No-op for IGBP other than BufferQueue. + (void) drop; + return INVALID_OPERATION; +} + status_t IGraphicBufferProducer::exportToParcel(Parcel* parcel) { status_t res = OK; res = parcel->writeUint32(USE_BUFFER_QUEUE); @@ -1018,6 +1043,13 @@ status_t BnGraphicBufferProducer::onTransact( } return NO_ERROR; } + case SET_LEGACY_BUFFER_DROP: { + CHECK_INTERFACE(IGraphicBufferProducer, data, reply); + bool drop = data.readInt32(); + int result = setLegacyBufferDrop(drop); + reply->writeInt32(result); + return NO_ERROR; + } } return BBinder::onTransact(code, data, reply, flags); } diff --git a/libs/gui/include/gui/BufferQueueCore.h b/libs/gui/include/gui/BufferQueueCore.h index 0e80283804..9c0ee99b59 100644 --- a/libs/gui/include/gui/BufferQueueCore.h +++ b/libs/gui/include/gui/BufferQueueCore.h @@ -226,6 +226,16 @@ private: // consumer are controlled by the application. bool mDequeueBufferCannotBlock; + // mQueueBufferCanDrop indicates whether queueBuffer is allowed to drop + // buffers in non-async mode. This flag is set during connect when both the + // producer and consumer are controlled by application. + bool mQueueBufferCanDrop; + + // mLegacyBufferDrop indicates whether mQueueBufferCanDrop is in effect. + // If this flag is set mQueueBufferCanDrop is working as explained. If not + // queueBuffer will not drop buffers unless consumer is SurfaceFlinger. + bool mLegacyBufferDrop; + // mDefaultBufferFormat can be set so it will override the buffer format // when it isn't specified in dequeueBuffer. PixelFormat mDefaultBufferFormat; diff --git a/libs/gui/include/gui/BufferQueueProducer.h b/libs/gui/include/gui/BufferQueueProducer.h index 415e2a616e..d2a47a6aa8 100644 --- a/libs/gui/include/gui/BufferQueueProducer.h +++ b/libs/gui/include/gui/BufferQueueProducer.h @@ -174,6 +174,9 @@ public: // See IGraphicBufferProducer::setDequeueTimeout virtual status_t setDequeueTimeout(nsecs_t timeout) override; + // see IGraphicBufferProducer::setLegacyBufferDrop + virtual status_t setLegacyBufferDrop(bool drop); + // See IGraphicBufferProducer::getLastQueuedBuffer virtual status_t getLastQueuedBuffer(sp* outBuffer, sp* outFence, float outTransformMatrix[16]) override; diff --git a/libs/gui/include/gui/IGraphicBufferProducer.h b/libs/gui/include/gui/IGraphicBufferProducer.h index 9f7e22bac5..3dde8c8c80 100644 --- a/libs/gui/include/gui/IGraphicBufferProducer.h +++ b/libs/gui/include/gui/IGraphicBufferProducer.h @@ -592,12 +592,20 @@ public: // non-blocking mode and its corresponding spare buffer (which is used to // ensure a buffer is always available). // + // Note well: queueBuffer will stop buffer dropping behavior if timeout is + // strictly positive. If timeout is zero or negative, previous buffer + // dropping behavior will not be changed. + // // Return of a value other than NO_ERROR means an error has occurred: // * BAD_VALUE - Failure to adjust the number of available slots. This can // happen because of trying to allocate/deallocate the async // buffer. virtual status_t setDequeueTimeout(nsecs_t timeout) = 0; + // Used to enable/disable buffer drop behavior of queueBuffer. + // If it's not used, legacy drop behavior will be retained. + virtual status_t setLegacyBufferDrop(bool drop); + // Returns the last queued buffer along with a fence which must signal // before the contents of the buffer are read. If there are no buffers in // the queue, outBuffer will be populated with nullptr and outFence will be diff --git a/services/surfaceflinger/MonitoredProducer.cpp b/services/surfaceflinger/MonitoredProducer.cpp index 06e3d9c154..c60421b538 100644 --- a/services/surfaceflinger/MonitoredProducer.cpp +++ b/services/surfaceflinger/MonitoredProducer.cpp @@ -132,6 +132,10 @@ status_t MonitoredProducer::setDequeueTimeout(nsecs_t timeout) { return mProducer->setDequeueTimeout(timeout); } +status_t MonitoredProducer::setLegacyBufferDrop(bool drop) { + return mProducer->setLegacyBufferDrop(drop); +} + status_t MonitoredProducer::getLastQueuedBuffer(sp* outBuffer, sp* outFence, float outTransformMatrix[16]) { return mProducer->getLastQueuedBuffer(outBuffer, outFence, diff --git a/services/surfaceflinger/MonitoredProducer.h b/services/surfaceflinger/MonitoredProducer.h index 1246d142f3..d346f821d3 100644 --- a/services/surfaceflinger/MonitoredProducer.h +++ b/services/surfaceflinger/MonitoredProducer.h @@ -61,6 +61,7 @@ public: virtual status_t setGenerationNumber(uint32_t generationNumber); virtual String8 getConsumerName() const override; virtual status_t setDequeueTimeout(nsecs_t timeout) override; + virtual status_t setLegacyBufferDrop(bool drop) override; virtual status_t getLastQueuedBuffer(sp* outBuffer, sp* outFence, float outTransformMatrix[16]) override; virtual IBinder* onAsBinder(); -- cgit v1.2.3-59-g8ed1b From cb7dd008877ec881dc25d7c941bda802a0b467ff Mon Sep 17 00:00:00 2001 From: Yiwei Zhang Date: Tue, 16 Apr 2019 11:03:01 -0700 Subject: TimeStats: fix a racing case setPostTime is plumbed without BufferQueue lock, and it's called at the end of queueBuffer. If the addAndGetFrameTimestamps is delayed by queueBuffer back pressure (this could happen on special SF/BQ configs), setAcquireFence will be called with a out-of-bound index internally. So this change will tweak the order of the back Pressure logic to make the post time correct when queueBuffer back pressure happens as well as guarding the TimeStats more strictly. Bug: 130515827 Test: all SurfaceFlinger tests Change-Id: I2eace6d8693cc647284de0f33e7b58a6bf79eaaf --- libs/gui/BufferQueueProducer.cpp | 16 ++++++++-------- services/surfaceflinger/TimeStats/TimeStats.cpp | 18 ++++++++++++++++++ 2 files changed, 26 insertions(+), 8 deletions(-) (limited to 'libs/gui/BufferQueueProducer.cpp') diff --git a/libs/gui/BufferQueueProducer.cpp b/libs/gui/BufferQueueProducer.cpp index 4ff69c57ce..c94c6b31c6 100644 --- a/libs/gui/BufferQueueProducer.cpp +++ b/libs/gui/BufferQueueProducer.cpp @@ -1007,14 +1007,6 @@ status_t BufferQueueProducer::queueBuffer(int slot, mCallbackCondition.notify_all(); } - // Wait without lock held - if (connectedApi == NATIVE_WINDOW_API_EGL) { - // Waiting here allows for two full buffers to be queued but not a - // third. In the event that frames take varying time, this makes a - // small trade-off in favor of latency rather than throughput. - lastQueuedFence->waitForever("Throttling EGL Production"); - } - // Update and get FrameEventHistory. nsecs_t postedTime = systemTime(SYSTEM_TIME_MONOTONIC); NewFrameEventsEntry newFrameEventsEntry = { @@ -1026,6 +1018,14 @@ status_t BufferQueueProducer::queueBuffer(int slot, addAndGetFrameTimestamps(&newFrameEventsEntry, getFrameTimestamps ? &output->frameTimestamps : nullptr); + // Wait without lock held + if (connectedApi == NATIVE_WINDOW_API_EGL) { + // Waiting here allows for two full buffers to be queued but not a + // third. In the event that frames take varying time, this makes a + // small trade-off in favor of latency rather than throughput. + lastQueuedFence->waitForever("Throttling EGL Production"); + } + return NO_ERROR; } diff --git a/services/surfaceflinger/TimeStats/TimeStats.cpp b/services/surfaceflinger/TimeStats/TimeStats.cpp index 78c6e74150..740099e350 100644 --- a/services/surfaceflinger/TimeStats/TimeStats.cpp +++ b/services/surfaceflinger/TimeStats/TimeStats.cpp @@ -291,6 +291,9 @@ void TimeStats::setLatchTime(int32_t layerID, uint64_t frameNumber, nsecs_t latc std::lock_guard lock(mMutex); if (!mTimeStatsTracker.count(layerID)) return; LayerRecord& layerRecord = mTimeStatsTracker[layerID]; + if (layerRecord.waitData < 0 || + layerRecord.waitData >= static_cast(layerRecord.timeRecords.size())) + return; TimeRecord& timeRecord = layerRecord.timeRecords[layerRecord.waitData]; if (timeRecord.frameTime.frameNumber == frameNumber) { timeRecord.frameTime.latchTime = latchTime; @@ -306,6 +309,9 @@ void TimeStats::setDesiredTime(int32_t layerID, uint64_t frameNumber, nsecs_t de std::lock_guard lock(mMutex); if (!mTimeStatsTracker.count(layerID)) return; LayerRecord& layerRecord = mTimeStatsTracker[layerID]; + if (layerRecord.waitData < 0 || + layerRecord.waitData >= static_cast(layerRecord.timeRecords.size())) + return; TimeRecord& timeRecord = layerRecord.timeRecords[layerRecord.waitData]; if (timeRecord.frameTime.frameNumber == frameNumber) { timeRecord.frameTime.desiredTime = desiredTime; @@ -321,6 +327,9 @@ void TimeStats::setAcquireTime(int32_t layerID, uint64_t frameNumber, nsecs_t ac std::lock_guard lock(mMutex); if (!mTimeStatsTracker.count(layerID)) return; LayerRecord& layerRecord = mTimeStatsTracker[layerID]; + if (layerRecord.waitData < 0 || + layerRecord.waitData >= static_cast(layerRecord.timeRecords.size())) + return; TimeRecord& timeRecord = layerRecord.timeRecords[layerRecord.waitData]; if (timeRecord.frameTime.frameNumber == frameNumber) { timeRecord.frameTime.acquireTime = acquireTime; @@ -338,6 +347,9 @@ void TimeStats::setAcquireFence(int32_t layerID, uint64_t frameNumber, std::lock_guard lock(mMutex); if (!mTimeStatsTracker.count(layerID)) return; LayerRecord& layerRecord = mTimeStatsTracker[layerID]; + if (layerRecord.waitData < 0 || + layerRecord.waitData >= static_cast(layerRecord.timeRecords.size())) + return; TimeRecord& timeRecord = layerRecord.timeRecords[layerRecord.waitData]; if (timeRecord.frameTime.frameNumber == frameNumber) { timeRecord.acquireFence = acquireFence; @@ -353,6 +365,9 @@ void TimeStats::setPresentTime(int32_t layerID, uint64_t frameNumber, nsecs_t pr std::lock_guard lock(mMutex); if (!mTimeStatsTracker.count(layerID)) return; LayerRecord& layerRecord = mTimeStatsTracker[layerID]; + if (layerRecord.waitData < 0 || + layerRecord.waitData >= static_cast(layerRecord.timeRecords.size())) + return; TimeRecord& timeRecord = layerRecord.timeRecords[layerRecord.waitData]; if (timeRecord.frameTime.frameNumber == frameNumber) { timeRecord.frameTime.presentTime = presentTime; @@ -374,6 +389,9 @@ void TimeStats::setPresentFence(int32_t layerID, uint64_t frameNumber, std::lock_guard lock(mMutex); if (!mTimeStatsTracker.count(layerID)) return; LayerRecord& layerRecord = mTimeStatsTracker[layerID]; + if (layerRecord.waitData < 0 || + layerRecord.waitData >= static_cast(layerRecord.timeRecords.size())) + return; TimeRecord& timeRecord = layerRecord.timeRecords[layerRecord.waitData]; if (timeRecord.frameTime.frameNumber == frameNumber) { timeRecord.presentFence = presentFence; -- cgit v1.2.3-59-g8ed1b From 485e4c358b26f09ec0634e63c311131c9507779a Mon Sep 17 00:00:00 2001 From: Alec Mouri Date: Tue, 30 Apr 2019 18:24:05 -0700 Subject: Clean-up egl image preallocation Allocate EGL images in onFrameAvailable, instead of a custom onBuffersAllocated callback. This way we reduce traffic over binder while still performing GL work ahead of time in queueBuffer(). Bug: 130567928 Test: systrace Change-Id: I4070e9ddbd379dac3d809d0e7edb2855fc8b7a80 --- libs/gui/BufferQueue.cpp | 7 ------- libs/gui/BufferQueueProducer.cpp | 17 ----------------- libs/gui/ConsumerBase.cpp | 2 -- libs/gui/IConsumerListener.cpp | 10 +--------- libs/gui/include/gui/BufferQueue.h | 1 - libs/gui/include/gui/ConsumerBase.h | 1 - libs/gui/include/gui/IConsumerListener.h | 7 ------- services/surfaceflinger/BufferLayerConsumer.cpp | 22 ++++++++-------------- services/surfaceflinger/BufferLayerConsumer.h | 2 +- services/surfaceflinger/BufferQueueLayer.cpp | 7 +++++++ 10 files changed, 17 insertions(+), 59 deletions(-) (limited to 'libs/gui/BufferQueueProducer.cpp') diff --git a/libs/gui/BufferQueue.cpp b/libs/gui/BufferQueue.cpp index d87228fbbd..5fb3f0b80f 100644 --- a/libs/gui/BufferQueue.cpp +++ b/libs/gui/BufferQueue.cpp @@ -59,13 +59,6 @@ void BufferQueue::ProxyConsumerListener::onFrameReplaced( } } -void BufferQueue::ProxyConsumerListener::onBufferAllocated(const BufferItem& item) { - sp listener(mConsumerListener.promote()); - if (listener != nullptr) { - listener->onBufferAllocated(item); - } -} - void BufferQueue::ProxyConsumerListener::onBuffersReleased() { sp listener(mConsumerListener.promote()); if (listener != nullptr) { diff --git a/libs/gui/BufferQueueProducer.cpp b/libs/gui/BufferQueueProducer.cpp index c94c6b31c6..3928bb9e40 100644 --- a/libs/gui/BufferQueueProducer.cpp +++ b/libs/gui/BufferQueueProducer.cpp @@ -539,13 +539,6 @@ status_t BufferQueueProducer::dequeueBuffer(int* outSlot, sp* ou return NO_INIT; } - if (mCore->mConsumerListener != nullptr) { - BufferItem item; - item.mGraphicBuffer = graphicBuffer; - item.mSlot = *outSlot; - mCore->mConsumerListener->onBufferAllocated(item); - } - VALIDATE_CONSISTENCY(); } // Autolock scope } @@ -975,9 +968,6 @@ status_t BufferQueueProducer::queueBuffer(int slot, item.mGraphicBuffer.clear(); } - // Don't send the slot number through the callback since the consumer shouldn't need it - item.mSlot = BufferItem::INVALID_BUFFER_SLOT; - // Call back without the main BufferQueue lock held, but with the callback // lock held so we can ensure that callbacks occur in order @@ -1433,13 +1423,6 @@ void BufferQueueProducer::allocateBuffers(uint32_t width, uint32_t height, BQ_LOGV("allocateBuffers: allocated a new buffer in slot %d", *slot); - if (mCore->mConsumerListener != nullptr) { - BufferItem item; - item.mGraphicBuffer = buffers[i]; - item.mSlot = *slot; - mCore->mConsumerListener->onBufferAllocated(item); - } - // Make sure the erase is done after all uses of the slot // iterator since it will be invalid after this point. mCore->mFreeSlots.erase(slot); diff --git a/libs/gui/ConsumerBase.cpp b/libs/gui/ConsumerBase.cpp index 1e94cc13cd..abd9921fa9 100644 --- a/libs/gui/ConsumerBase.cpp +++ b/libs/gui/ConsumerBase.cpp @@ -131,8 +131,6 @@ void ConsumerBase::onFrameReplaced(const BufferItem &item) { } } -void ConsumerBase::onBufferAllocated(const BufferItem& /*item*/) {} - void ConsumerBase::onBuffersReleased() { Mutex::Autolock lock(mMutex); diff --git a/libs/gui/IConsumerListener.cpp b/libs/gui/IConsumerListener.cpp index ea9045cad9..85ac304ab8 100644 --- a/libs/gui/IConsumerListener.cpp +++ b/libs/gui/IConsumerListener.cpp @@ -28,8 +28,7 @@ enum class Tag : uint32_t { ON_FRAME_REPLACED, ON_BUFFERS_RELEASED, ON_SIDEBAND_STREAM_CHANGED, - ON_BUFFER_ALLOCATED, - LAST = ON_BUFFER_ALLOCATED, + LAST = ON_SIDEBAND_STREAM_CHANGED, }; } // Anonymous namespace @@ -55,11 +54,6 @@ public: item); } - void onBufferAllocated(const BufferItem& item) override { - callRemoteAsync(Tag::ON_BUFFER_ALLOCATED, - item); - } - void onBuffersReleased() override { callRemoteAsync(Tag::ON_BUFFERS_RELEASED); } @@ -95,8 +89,6 @@ status_t BnConsumerListener::onTransact(uint32_t code, const Parcel& data, Parce return callLocalAsync(data, reply, &IConsumerListener::onFrameAvailable); case Tag::ON_FRAME_REPLACED: return callLocalAsync(data, reply, &IConsumerListener::onFrameReplaced); - case Tag::ON_BUFFER_ALLOCATED: - return callLocalAsync(data, reply, &IConsumerListener::onBufferAllocated); case Tag::ON_BUFFERS_RELEASED: return callLocalAsync(data, reply, &IConsumerListener::onBuffersReleased); case Tag::ON_SIDEBAND_STREAM_CHANGED: diff --git a/libs/gui/include/gui/BufferQueue.h b/libs/gui/include/gui/BufferQueue.h index 721427be7b..da952744f3 100644 --- a/libs/gui/include/gui/BufferQueue.h +++ b/libs/gui/include/gui/BufferQueue.h @@ -61,7 +61,6 @@ public: void onDisconnect() override; void onFrameAvailable(const BufferItem& item) override; void onFrameReplaced(const BufferItem& item) override; - void onBufferAllocated(const BufferItem& item) override; void onBuffersReleased() override; void onSidebandStreamChanged() override; void addAndGetFrameTimestamps( diff --git a/libs/gui/include/gui/ConsumerBase.h b/libs/gui/include/gui/ConsumerBase.h index 7c26482509..366ced380b 100644 --- a/libs/gui/include/gui/ConsumerBase.h +++ b/libs/gui/include/gui/ConsumerBase.h @@ -141,7 +141,6 @@ protected: // classes if they want the notification. virtual void onFrameAvailable(const BufferItem& item) override; virtual void onFrameReplaced(const BufferItem& item) override; - virtual void onBufferAllocated(const BufferItem& item) override; virtual void onBuffersReleased() override; virtual void onSidebandStreamChanged() override; diff --git a/libs/gui/include/gui/IConsumerListener.h b/libs/gui/include/gui/IConsumerListener.h index 03fefbe90c..c0828820e3 100644 --- a/libs/gui/include/gui/IConsumerListener.h +++ b/libs/gui/include/gui/IConsumerListener.h @@ -61,13 +61,6 @@ public: // This is called without any lock held and can be called concurrently by multiple threads. virtual void onFrameReplaced(const BufferItem& /* item */) {} /* Asynchronous */ - // onBufferAllocated is called to notify the buffer consumer that the BufferQueue has allocated - // a GraphicBuffer for a particular slot. Only the GraphicBuffer pointer and the slot ID will - // be populated. - // - // This is called without any lock held and can be called concurrently by multiple threads. - virtual void onBufferAllocated(const BufferItem& /* item */) {} /* Asynchronous */ - // onBuffersReleased is called to notify the buffer consumer that the BufferQueue has released // its references to one or more GraphicBuffers contained in its slots. The buffer consumer // should then call BufferQueue::getReleasedBuffers to retrieve the list of buffers. diff --git a/services/surfaceflinger/BufferLayerConsumer.cpp b/services/surfaceflinger/BufferLayerConsumer.cpp index fc98dc836a..6709fb4b48 100644 --- a/services/surfaceflinger/BufferLayerConsumer.cpp +++ b/services/surfaceflinger/BufferLayerConsumer.cpp @@ -478,20 +478,15 @@ void BufferLayerConsumer::onSidebandStreamChanged() { } } -void BufferLayerConsumer::onBufferAllocated(const BufferItem& item) { - if (item.mGraphicBuffer != nullptr) { - std::shared_ptr image = std::make_shared(item.mGraphicBuffer, mRE); - std::shared_ptr oldImage; - { - std::lock_guard lock(mImagesMutex); - oldImage = mImages[item.mSlot]; - if (oldImage == nullptr || oldImage->graphicBuffer() == nullptr || - oldImage->graphicBuffer()->getId() != item.mGraphicBuffer->getId()) { - mImages[item.mSlot] = std::make_shared(item.mGraphicBuffer, mRE); - } - image = mImages[item.mSlot]; +void BufferLayerConsumer::onBufferAvailable(const BufferItem& item) { + if (item.mGraphicBuffer != nullptr && item.mSlot != BufferQueue::INVALID_BUFFER_SLOT) { + std::lock_guard lock(mImagesMutex); + const std::shared_ptr& oldImage = mImages[item.mSlot]; + if (oldImage == nullptr || oldImage->graphicBuffer() == nullptr || + oldImage->graphicBuffer()->getId() != item.mGraphicBuffer->getId()) { + mImages[item.mSlot] = std::make_shared(item.mGraphicBuffer, mRE); + mRE.cacheExternalTextureBuffer(item.mGraphicBuffer); } - mRE.cacheExternalTextureBuffer(image->graphicBuffer()); } } @@ -533,5 +528,4 @@ BufferLayerConsumer::Image::~Image() { mRE.unbindExternalTextureBuffer(mGraphicBuffer->getId()); } } - }; // namespace android diff --git a/services/surfaceflinger/BufferLayerConsumer.h b/services/surfaceflinger/BufferLayerConsumer.h index 0f0655d705..e3f6100c35 100644 --- a/services/surfaceflinger/BufferLayerConsumer.h +++ b/services/surfaceflinger/BufferLayerConsumer.h @@ -176,6 +176,7 @@ public: // setConsumerUsageBits overrides the ConsumerBase method to OR // DEFAULT_USAGE_FLAGS to usage. status_t setConsumerUsageBits(uint64_t usage); + void onBufferAvailable(const BufferItem& item) EXCLUDES(mImagesMutex); protected: // abandonLocked overrides the ConsumerBase method to clear @@ -241,7 +242,6 @@ private: // IConsumerListener interface void onDisconnect() override; - void onBufferAllocated(const BufferItem& item) override EXCLUDES(mImagesMutex); void onSidebandStreamChanged() override; void addAndGetFrameTimestamps(const NewFrameEventsEntry* newTimestamps, FrameEventHistoryDelta* outDelta) override; diff --git a/services/surfaceflinger/BufferQueueLayer.cpp b/services/surfaceflinger/BufferQueueLayer.cpp index 5d729f5b4f..3d51ec33b2 100644 --- a/services/surfaceflinger/BufferQueueLayer.cpp +++ b/services/surfaceflinger/BufferQueueLayer.cpp @@ -14,6 +14,9 @@ * limitations under the License. */ +#undef LOG_TAG +#define LOG_TAG "BufferQueueLayer" +#define ATRACE_TAG ATRACE_TAG_GRAPHICS #include #include #include @@ -435,6 +438,7 @@ void BufferQueueLayer::fakeVsync() { } void BufferQueueLayer::onFrameAvailable(const BufferItem& item) { + ATRACE_CALL(); // Add this buffer from our internal queue tracker { // Autolock scope if (mFlinger->mUseSmart90ForVideo) { @@ -475,9 +479,11 @@ void BufferQueueLayer::onFrameAvailable(const BufferItem& item) { } else { mFlinger->signalLayerUpdate(); } + mConsumer->onBufferAvailable(item); } void BufferQueueLayer::onFrameReplaced(const BufferItem& item) { + ATRACE_CALL(); { // Autolock scope Mutex::Autolock lock(mQueueItemLock); @@ -499,6 +505,7 @@ void BufferQueueLayer::onFrameReplaced(const BufferItem& item) { mLastFrameNumberReceived = item.mFrameNumber; mQueueItemCondition.broadcast(); } + mConsumer->onBufferAvailable(item); } void BufferQueueLayer::onSidebandStreamChanged() { -- cgit v1.2.3-59-g8ed1b From af14124aaca79f6afa314b19b2a6bf931846fcf9 Mon Sep 17 00:00:00 2001 From: Sungtak Lee Date: Wed, 24 Apr 2019 16:36:44 -0700 Subject: Suppress BufferQueueProducer::dequeueBuffer error logs BufferQueue acts as an allocator for Codec2. Since BufferQueue does not have notifications on availability of buffers for dequeueBuffer now, suppress error messages from BufferQueueProducer::dequeueBuffer for looping if dequeue timeout is not infinite. Bug: 129899822 Bug: 131773342 Change-Id: Id82ad9207847e71739840969da0375494aaa317f --- libs/gui/BufferQueueProducer.cpp | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) (limited to 'libs/gui/BufferQueueProducer.cpp') diff --git a/libs/gui/BufferQueueProducer.cpp b/libs/gui/BufferQueueProducer.cpp index 3928bb9e40..96d75689eb 100644 --- a/libs/gui/BufferQueueProducer.cpp +++ b/libs/gui/BufferQueueProducer.cpp @@ -274,8 +274,12 @@ status_t BufferQueueProducer::waitForFreeSlotThenRelock(FreeSlotCaller caller, // This check is only done if a buffer has already been queued if (mCore->mBufferHasBeenQueued && dequeuedCount >= mCore->mMaxDequeuedBufferCount) { - BQ_LOGE("%s: attempting to exceed the max dequeued buffer count " - "(%d)", callerString, mCore->mMaxDequeuedBufferCount); + // Supress error logs when timeout is non-negative. + if (mDequeueTimeout < 0) { + BQ_LOGE("%s: attempting to exceed the max dequeued buffer " + "count (%d)", callerString, + mCore->mMaxDequeuedBufferCount); + } return INVALID_OPERATION; } -- cgit v1.2.3-59-g8ed1b From 42a94c6b3592f2aeddcbb39fa23c87136e5f1178 Mon Sep 17 00:00:00 2001 From: Sungtak Lee Date: Fri, 24 May 2019 14:27:14 -0700 Subject: BufferQueueProducer: fix buffer adjustment during setDequeueTimeout Adjust buffer during setDequeueTimeout() according to the expected future value of mDequeueBufferCannotBlock. Bug: 133208327 Change-Id: Ibc6b1dd6c3870c6c80e76279f1f104200a3d0241 --- libs/gui/BufferQueueProducer.cpp | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) (limited to 'libs/gui/BufferQueueProducer.cpp') diff --git a/libs/gui/BufferQueueProducer.cpp b/libs/gui/BufferQueueProducer.cpp index 96d75689eb..c20c2f3dca 100644 --- a/libs/gui/BufferQueueProducer.cpp +++ b/libs/gui/BufferQueueProducer.cpp @@ -1497,7 +1497,9 @@ status_t BufferQueueProducer::setDequeueTimeout(nsecs_t timeout) { BQ_LOGV("setDequeueTimeout: %" PRId64, timeout); std::lock_guard lock(mCore->mMutex); - int delta = mCore->getMaxBufferCountLocked(mCore->mAsyncMode, false, + bool dequeueBufferCannotBlock = + timeout >= 0 ? false : mCore->mDequeueBufferCannotBlock; + int delta = mCore->getMaxBufferCountLocked(mCore->mAsyncMode, dequeueBufferCannotBlock, mCore->mMaxBufferCount) - mCore->getMaxBufferCountLocked(); if (!mCore->adjustAvailableSlotsLocked(delta)) { BQ_LOGE("setDequeueTimeout: BufferQueue failed to adjust the number of " @@ -1506,11 +1508,9 @@ status_t BufferQueueProducer::setDequeueTimeout(nsecs_t timeout) { } mDequeueTimeout = timeout; - if (timeout >= 0) { - mCore->mDequeueBufferCannotBlock = false; - if (timeout != 0) { - mCore->mQueueBufferCanDrop = false; - } + mCore->mDequeueBufferCannotBlock = dequeueBufferCannotBlock; + if (timeout > 0) { + mCore->mQueueBufferCanDrop = false; } VALIDATE_CONSISTENCY(); -- cgit v1.2.3-59-g8ed1b From 45e9e0b133576dbfaf354ada2ab4179e9e4118b8 Mon Sep 17 00:00:00 2001 From: Sungtak Lee Date: Tue, 28 May 2019 13:38:23 -0700 Subject: Drop buffers for SurfaceFlinger properly Drop buffers restrictively when consumer is SurfaceFlinger. When both producer and consumer are controlled by app and timeout is not positive, drop buffers for SurfaceFlinger. Bug: 133214906 Change-Id: Ied102857673cbf36e51aac6abeea9abffbdcce67 --- libs/gui/BufferQueueProducer.cpp | 2 +- libs/gui/include/gui/BufferQueueCore.h | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) (limited to 'libs/gui/BufferQueueProducer.cpp') diff --git a/libs/gui/BufferQueueProducer.cpp b/libs/gui/BufferQueueProducer.cpp index c20c2f3dca..9c311a314f 100644 --- a/libs/gui/BufferQueueProducer.cpp +++ b/libs/gui/BufferQueueProducer.cpp @@ -886,7 +886,7 @@ status_t BufferQueueProducer::queueBuffer(int slot, item.mFence = acquireFence; item.mFenceTime = acquireFenceTime; item.mIsDroppable = mCore->mAsyncMode || - (!mCore->mLegacyBufferDrop && mConsumerIsSurfaceFlinger) || + (mConsumerIsSurfaceFlinger && mCore->mQueueBufferCanDrop) || (mCore->mLegacyBufferDrop && mCore->mQueueBufferCanDrop) || (mCore->mSharedBufferMode && mCore->mSharedBufferSlot == slot); item.mSurfaceDamage = surfaceDamage; diff --git a/libs/gui/include/gui/BufferQueueCore.h b/libs/gui/include/gui/BufferQueueCore.h index 9c0ee99b59..690a85f395 100644 --- a/libs/gui/include/gui/BufferQueueCore.h +++ b/libs/gui/include/gui/BufferQueueCore.h @@ -233,7 +233,8 @@ private: // mLegacyBufferDrop indicates whether mQueueBufferCanDrop is in effect. // If this flag is set mQueueBufferCanDrop is working as explained. If not - // queueBuffer will not drop buffers unless consumer is SurfaceFlinger. + // queueBuffer will not drop buffers unless consumer is SurfaceFlinger and + // mQueueBufferCanDrop is set. bool mLegacyBufferDrop; // mDefaultBufferFormat can be set so it will override the buffer format -- cgit v1.2.3-59-g8ed1b