From 02186fbaa27f4a7e78a0207da49ef38baacd1571 Mon Sep 17 00:00:00 2001 From: Huihong Luo Date: Wed, 23 Feb 2022 14:21:54 -0800 Subject: Migrate 13 methods of ISurfaceComposer to AIDL More misc methods are migrated to AIDL. ARect parcelable is added to serialize Rect data structure, defined in libui Rect.h. Bug: 211009610 Test: atest libgui_test libsurfaceflinger_unittest SurfaceFlinger_test Change-Id: I549e06c6f550760974d965d08783338635a5a5fe --- libs/gui/BLASTBufferQueue.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) (limited to 'libs/gui/BLASTBufferQueue.cpp') diff --git a/libs/gui/BLASTBufferQueue.cpp b/libs/gui/BLASTBufferQueue.cpp index c2793ac5de..bba7387c3a 100644 --- a/libs/gui/BLASTBufferQueue.cpp +++ b/libs/gui/BLASTBufferQueue.cpp @@ -33,6 +33,7 @@ #include #include +#include #include @@ -160,7 +161,7 @@ BLASTBufferQueue::BLASTBufferQueue(const std::string& name, bool updateDestinati mBufferItemConsumer->setFrameAvailableListener(this); mBufferItemConsumer->setBufferFreedListener(this); - ComposerService::getComposerService()->getMaxAcquiredBufferCount(&mMaxAcquiredBuffers); + ComposerServiceAIDL::getComposerService()->getMaxAcquiredBufferCount(&mMaxAcquiredBuffers); mBufferItemConsumer->setMaxAcquiredBufferCount(mMaxAcquiredBuffers); mCurrentMaxAcquiredBufferCount = mMaxAcquiredBuffers; mNumAcquired = 0; -- cgit v1.2.3-59-g8ed1b From d3d8f8e3501a07d55090c287ac45943d687b7006 Mon Sep 17 00:00:00 2001 From: Huihong Luo Date: Tue, 8 Mar 2022 14:48:46 -0800 Subject: Migrate ISurfaceComposerClient to AIDL Migrate and clean up ISurfaceComposerClient to aidl, removed the deprecated createWithParent method, removed non used parameters. Bug: 172002646 Test: atest libgui_test Change-Id: I8ceb7cd90104f2ad9ca72c8025f6298de1fb1ba0 --- libs/binder/include/binder/IInterface.h | 147 ++++++++++----------- libs/gui/Android.bp | 1 - libs/gui/BLASTBufferQueue.cpp | 2 +- libs/gui/ISurfaceComposer.cpp | 17 +-- libs/gui/ISurfaceComposerClient.cpp | 124 ----------------- libs/gui/LayerMetadata.cpp | 4 +- libs/gui/LayerState.cpp | 2 +- libs/gui/SurfaceComposerClient.cpp | 89 ++++++------- libs/gui/SurfaceControl.cpp | 15 +-- libs/gui/aidl/android/gui/CreateSurfaceResult.aidl | 24 ++++ libs/gui/aidl/android/gui/ISurfaceComposer.aidl | 7 + .../aidl/android/gui/ISurfaceComposerClient.aidl | 62 +++++++++ libs/gui/aidl/android/gui/LayerMetadata.aidl | 19 +++ libs/gui/aidl/android/gui/MirrorSurfaceResult.aidl | 23 ++++ libs/gui/include/gui/ISurfaceComposer.h | 9 +- libs/gui/include/gui/ISurfaceComposerClient.h | 97 -------------- libs/gui/include/gui/LayerMetadata.h | 13 +- libs/gui/include/gui/LayerState.h | 4 +- libs/gui/include/gui/SurfaceComposerClient.h | 19 +-- libs/gui/include/gui/SurfaceControl.h | 9 +- libs/gui/tests/Surface_test.cpp | 6 +- services/surfaceflinger/BufferLayer.h | 2 +- services/surfaceflinger/Client.cpp | 96 +++++++++----- services/surfaceflinger/Client.h | 28 ++-- services/surfaceflinger/FpsReporter.cpp | 4 +- services/surfaceflinger/Layer.cpp | 14 +- services/surfaceflinger/Layer.h | 2 +- services/surfaceflinger/SurfaceFlinger.cpp | 31 +++-- services/surfaceflinger/SurfaceFlinger.h | 4 +- services/surfaceflinger/TimeStats/TimeStats.h | 2 + .../include/timestatsproto/TimeStatsHelper.h | 3 + .../Tracing/TransactionProtoParser.cpp | 4 +- .../fuzzer/surfaceflinger_fuzzers_utils.h | 2 +- .../include/layerproto/LayerProtoParser.h | 2 + .../surfaceflinger/tests/InvalidHandles_test.cpp | 4 +- .../tests/unittests/FpsReporterTest.cpp | 5 +- .../tests/unittests/GameModeTest.cpp | 4 +- .../tests/unittests/LayerMetadataTest.cpp | 4 +- .../tests/unittests/TransactionProtoParserTest.cpp | 3 +- 39 files changed, 413 insertions(+), 494 deletions(-) delete mode 100644 libs/gui/ISurfaceComposerClient.cpp create mode 100644 libs/gui/aidl/android/gui/CreateSurfaceResult.aidl create mode 100644 libs/gui/aidl/android/gui/ISurfaceComposerClient.aidl create mode 100644 libs/gui/aidl/android/gui/LayerMetadata.aidl create mode 100644 libs/gui/aidl/android/gui/MirrorSurfaceResult.aidl delete mode 100644 libs/gui/include/gui/ISurfaceComposerClient.h (limited to 'libs/gui/BLASTBufferQueue.cpp') diff --git a/libs/binder/include/binder/IInterface.h b/libs/binder/include/binder/IInterface.h index 706783093c..1576c9427d 100644 --- a/libs/binder/include/binder/IInterface.h +++ b/libs/binder/include/binder/IInterface.h @@ -219,80 +219,79 @@ inline IBinder* BpInterface::onAsBinder() namespace internal { constexpr const char* const kManualInterfaces[] = { - "android.app.IActivityManager", - "android.app.IUidObserver", - "android.drm.IDrm", - "android.dvr.IVsyncCallback", - "android.dvr.IVsyncService", - "android.gfx.tests.ICallback", - "android.gfx.tests.IIPCTest", - "android.gfx.tests.ISafeInterfaceTest", - "android.graphicsenv.IGpuService", - "android.gui.IConsumerListener", - "android.gui.IGraphicBufferConsumer", - "android.gui.ITransactionComposerListener", - "android.gui.SensorEventConnection", - "android.gui.SensorServer", - "android.hardware.ICamera", - "android.hardware.ICameraClient", - "android.hardware.ICameraRecordingProxy", - "android.hardware.ICameraRecordingProxyListener", - "android.hardware.ICrypto", - "android.hardware.IOMXObserver", - "android.hardware.IStreamListener", - "android.hardware.IStreamSource", - "android.media.IAudioService", - "android.media.IDataSource", - "android.media.IDrmClient", - "android.media.IMediaCodecList", - "android.media.IMediaDrmService", - "android.media.IMediaExtractor", - "android.media.IMediaExtractorService", - "android.media.IMediaHTTPConnection", - "android.media.IMediaHTTPService", - "android.media.IMediaLogService", - "android.media.IMediaMetadataRetriever", - "android.media.IMediaMetricsService", - "android.media.IMediaPlayer", - "android.media.IMediaPlayerClient", - "android.media.IMediaPlayerService", - "android.media.IMediaRecorder", - "android.media.IMediaRecorderClient", - "android.media.IMediaResourceMonitor", - "android.media.IMediaSource", - "android.media.IRemoteDisplay", - "android.media.IRemoteDisplayClient", - "android.media.IResourceManagerClient", - "android.media.IResourceManagerService", - "android.os.IComplexTypeInterface", - "android.os.IPermissionController", - "android.os.IPingResponder", - "android.os.IProcessInfoService", - "android.os.ISchedulingPolicyService", - "android.os.IStringConstants", - "android.os.storage.IObbActionListener", - "android.os.storage.IStorageEventListener", - "android.os.storage.IStorageManager", - "android.os.storage.IStorageShutdownObserver", - "android.service.vr.IPersistentVrStateCallbacks", - "android.service.vr.IVrManager", - "android.service.vr.IVrStateCallbacks", - "android.ui.ISurfaceComposer", - "android.ui.ISurfaceComposerClient", - "android.utils.IMemory", - "android.utils.IMemoryHeap", - "com.android.car.procfsinspector.IProcfsInspector", - "com.android.internal.app.IAppOpsCallback", - "com.android.internal.app.IAppOpsService", - "com.android.internal.app.IBatteryStats", - "com.android.internal.os.IResultReceiver", - "com.android.internal.os.IShellCallback", - "drm.IDrmManagerService", - "drm.IDrmServiceListener", - "IAAudioClient", - "IAAudioService", - "VtsFuzzer", - nullptr, + "android.app.IActivityManager", + "android.app.IUidObserver", + "android.drm.IDrm", + "android.dvr.IVsyncCallback", + "android.dvr.IVsyncService", + "android.gfx.tests.ICallback", + "android.gfx.tests.IIPCTest", + "android.gfx.tests.ISafeInterfaceTest", + "android.graphicsenv.IGpuService", + "android.gui.IConsumerListener", + "android.gui.IGraphicBufferConsumer", + "android.gui.ITransactionComposerListener", + "android.gui.SensorEventConnection", + "android.gui.SensorServer", + "android.hardware.ICamera", + "android.hardware.ICameraClient", + "android.hardware.ICameraRecordingProxy", + "android.hardware.ICameraRecordingProxyListener", + "android.hardware.ICrypto", + "android.hardware.IOMXObserver", + "android.hardware.IStreamListener", + "android.hardware.IStreamSource", + "android.media.IAudioService", + "android.media.IDataSource", + "android.media.IDrmClient", + "android.media.IMediaCodecList", + "android.media.IMediaDrmService", + "android.media.IMediaExtractor", + "android.media.IMediaExtractorService", + "android.media.IMediaHTTPConnection", + "android.media.IMediaHTTPService", + "android.media.IMediaLogService", + "android.media.IMediaMetadataRetriever", + "android.media.IMediaMetricsService", + "android.media.IMediaPlayer", + "android.media.IMediaPlayerClient", + "android.media.IMediaPlayerService", + "android.media.IMediaRecorder", + "android.media.IMediaRecorderClient", + "android.media.IMediaResourceMonitor", + "android.media.IMediaSource", + "android.media.IRemoteDisplay", + "android.media.IRemoteDisplayClient", + "android.media.IResourceManagerClient", + "android.media.IResourceManagerService", + "android.os.IComplexTypeInterface", + "android.os.IPermissionController", + "android.os.IPingResponder", + "android.os.IProcessInfoService", + "android.os.ISchedulingPolicyService", + "android.os.IStringConstants", + "android.os.storage.IObbActionListener", + "android.os.storage.IStorageEventListener", + "android.os.storage.IStorageManager", + "android.os.storage.IStorageShutdownObserver", + "android.service.vr.IPersistentVrStateCallbacks", + "android.service.vr.IVrManager", + "android.service.vr.IVrStateCallbacks", + "android.ui.ISurfaceComposer", + "android.utils.IMemory", + "android.utils.IMemoryHeap", + "com.android.car.procfsinspector.IProcfsInspector", + "com.android.internal.app.IAppOpsCallback", + "com.android.internal.app.IAppOpsService", + "com.android.internal.app.IBatteryStats", + "com.android.internal.os.IResultReceiver", + "com.android.internal.os.IShellCallback", + "drm.IDrmManagerService", + "drm.IDrmServiceListener", + "IAAudioClient", + "IAAudioService", + "VtsFuzzer", + nullptr, }; constexpr const char* const kDownstreamManualInterfaces[] = { diff --git a/libs/gui/Android.bp b/libs/gui/Android.bp index a3f5a06d3b..648bcc33cb 100644 --- a/libs/gui/Android.bp +++ b/libs/gui/Android.bp @@ -204,7 +204,6 @@ cc_library_shared { "IGraphicBufferProducer.cpp", "IProducerListener.cpp", "ISurfaceComposer.cpp", - "ISurfaceComposerClient.cpp", "ITransactionCompletedListener.cpp", "LayerDebugInfo.cpp", "LayerMetadata.cpp", diff --git a/libs/gui/BLASTBufferQueue.cpp b/libs/gui/BLASTBufferQueue.cpp index bba7387c3a..ccb901c6a7 100644 --- a/libs/gui/BLASTBufferQueue.cpp +++ b/libs/gui/BLASTBufferQueue.cpp @@ -551,7 +551,7 @@ void BLASTBufferQueue::acquireNextBufferLocked( if (dequeueTime != mDequeueTimestamps.end()) { Parcel p; p.writeInt64(dequeueTime->second); - t->setMetadata(mSurfaceControl, METADATA_DEQUEUE_TIME, p); + t->setMetadata(mSurfaceControl, gui::METADATA_DEQUEUE_TIME, p); mDequeueTimestamps.erase(dequeueTime); } } diff --git a/libs/gui/ISurfaceComposer.cpp b/libs/gui/ISurfaceComposer.cpp index 80e512379f..54e50b50ac 100644 --- a/libs/gui/ISurfaceComposer.cpp +++ b/libs/gui/ISurfaceComposer.cpp @@ -25,7 +25,6 @@ #include #include #include -#include #include #include #include @@ -61,14 +60,6 @@ public: virtual ~BpSurfaceComposer(); - virtual sp createConnection() - { - Parcel data, reply; - data.writeInterfaceToken(ISurfaceComposer::getInterfaceDescriptor()); - remote()->transact(BnSurfaceComposer::CREATE_CONNECTION, data, &reply); - return interface_cast(reply.readStrongBinder()); - } - status_t setTransactionState(const FrameTimelineInfo& frameTimelineInfo, const Vector& state, const Vector& displays, uint32_t flags, @@ -159,13 +150,7 @@ IMPLEMENT_META_INTERFACE(SurfaceComposer, "android.ui.ISurfaceComposer"); status_t BnSurfaceComposer::onTransact( uint32_t code, const Parcel& data, Parcel* reply, uint32_t flags) { - switch(code) { - case CREATE_CONNECTION: { - CHECK_INTERFACE(ISurfaceComposer, data, reply); - sp b = IInterface::asBinder(createConnection()); - reply->writeStrongBinder(b); - return NO_ERROR; - } + switch (code) { case SET_TRANSACTION_STATE: { CHECK_INTERFACE(ISurfaceComposer, data, reply); diff --git a/libs/gui/ISurfaceComposerClient.cpp b/libs/gui/ISurfaceComposerClient.cpp deleted file mode 100644 index 5e7a7ec67b..0000000000 --- a/libs/gui/ISurfaceComposerClient.cpp +++ /dev/null @@ -1,124 +0,0 @@ -/* - * Copyright (C) 2007 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. - */ - -// tag as surfaceflinger -#define LOG_TAG "SurfaceFlinger" - -#include - -#include - -#include - -#include - -namespace android { - -namespace { // Anonymous - -enum class Tag : uint32_t { - CREATE_SURFACE = IBinder::FIRST_CALL_TRANSACTION, - CREATE_WITH_SURFACE_PARENT, - CLEAR_LAYER_FRAME_STATS, - GET_LAYER_FRAME_STATS, - MIRROR_SURFACE, - LAST = MIRROR_SURFACE, -}; - -} // Anonymous namespace - -class BpSurfaceComposerClient : public SafeBpInterface { -public: - explicit BpSurfaceComposerClient(const sp& impl) - : SafeBpInterface(impl, "BpSurfaceComposerClient") {} - - ~BpSurfaceComposerClient() override; - - status_t createSurface(const String8& name, uint32_t width, uint32_t height, PixelFormat format, - uint32_t flags, const sp& parent, LayerMetadata metadata, - sp* handle, sp* gbp, - int32_t* outLayerId, uint32_t* outTransformHint) override { - return callRemote(Tag::CREATE_SURFACE, - name, width, height, - format, flags, parent, - std::move(metadata), - handle, gbp, outLayerId, - outTransformHint); - } - - status_t createWithSurfaceParent(const String8& name, uint32_t width, uint32_t height, - PixelFormat format, uint32_t flags, - const sp& parent, - LayerMetadata metadata, sp* handle, - sp* gbp, int32_t* outLayerId, - uint32_t* outTransformHint) override { - return callRemote(Tag::CREATE_WITH_SURFACE_PARENT, - name, width, height, format, - flags, parent, - std::move(metadata), handle, gbp, - outLayerId, outTransformHint); - } - - status_t clearLayerFrameStats(const sp& handle) const override { - return callRemote(Tag::CLEAR_LAYER_FRAME_STATS, - handle); - } - - status_t getLayerFrameStats(const sp& handle, FrameStats* outStats) const override { - return callRemote(Tag::GET_LAYER_FRAME_STATS, handle, - outStats); - } - - status_t mirrorSurface(const sp& mirrorFromHandle, sp* outHandle, - int32_t* outLayerId) override { - return callRemote(Tag::MIRROR_SURFACE, - mirrorFromHandle, - outHandle, outLayerId); - } -}; - -// Out-of-line virtual method definition to trigger vtable emission in this -// translation unit (see clang warning -Wweak-vtables) -BpSurfaceComposerClient::~BpSurfaceComposerClient() {} - -IMPLEMENT_META_INTERFACE(SurfaceComposerClient, "android.ui.ISurfaceComposerClient"); - -// ---------------------------------------------------------------------- - -status_t BnSurfaceComposerClient::onTransact(uint32_t code, const Parcel& data, Parcel* reply, - uint32_t flags) { - if (code < IBinder::FIRST_CALL_TRANSACTION || code > static_cast(Tag::LAST)) { - return BBinder::onTransact(code, data, reply, flags); - } - auto tag = static_cast(code); - switch (tag) { - case Tag::CREATE_SURFACE: - return callLocal(data, reply, &ISurfaceComposerClient::createSurface); - case Tag::CREATE_WITH_SURFACE_PARENT: - return callLocal(data, reply, &ISurfaceComposerClient::createWithSurfaceParent); - case Tag::CLEAR_LAYER_FRAME_STATS: - return callLocal(data, reply, &ISurfaceComposerClient::clearLayerFrameStats); - case Tag::GET_LAYER_FRAME_STATS: - return callLocal(data, reply, &ISurfaceComposerClient::getLayerFrameStats); - case Tag::MIRROR_SURFACE: - return callLocal(data, reply, &ISurfaceComposerClient::mirrorSurface); - } -} - -} // namespace android diff --git a/libs/gui/LayerMetadata.cpp b/libs/gui/LayerMetadata.cpp index 189d51a4c1..4e12fd330c 100644 --- a/libs/gui/LayerMetadata.cpp +++ b/libs/gui/LayerMetadata.cpp @@ -23,7 +23,7 @@ using android::base::StringPrintf; -namespace android { +namespace android::gui { LayerMetadata::LayerMetadata() = default; @@ -144,4 +144,4 @@ std::string LayerMetadata::itemToString(uint32_t key, const char* separator) con } } -} // namespace android +} // namespace android::gui diff --git a/libs/gui/LayerState.cpp b/libs/gui/LayerState.cpp index 502031c8d8..3ab9e974bc 100644 --- a/libs/gui/LayerState.cpp +++ b/libs/gui/LayerState.cpp @@ -19,10 +19,10 @@ #include #include +#include #include #include #include -#include #include #include #include diff --git a/libs/gui/SurfaceComposerClient.cpp b/libs/gui/SurfaceComposerClient.cpp index 065deb6143..035aac9530 100644 --- a/libs/gui/SurfaceComposerClient.cpp +++ b/libs/gui/SurfaceComposerClient.cpp @@ -20,6 +20,7 @@ #include #include +#include #include #include #include @@ -39,7 +40,6 @@ #include #include #include -#include #include #include #include @@ -2004,11 +2004,11 @@ SurfaceComposerClient::SurfaceComposerClient(const sp& c : mStatus(NO_ERROR), mClient(client) {} void SurfaceComposerClient::onFirstRef() { - sp sf(ComposerService::getComposerService()); + sp sf(ComposerServiceAIDL::getComposerService()); if (sf != nullptr && mStatus == NO_INIT) { sp conn; - conn = sf->createConnection(); - if (conn != nullptr) { + binder::Status status = sf->createConnection(&conn); + if (status.isOk() && conn != nullptr) { mClient = conn; mStatus = NO_ERROR; } @@ -2046,7 +2046,7 @@ void SurfaceComposerClient::dispose() { } sp SurfaceComposerClient::createSurface(const String8& name, uint32_t w, uint32_t h, - PixelFormat format, uint32_t flags, + PixelFormat format, int32_t flags, const sp& parentHandle, LayerMetadata metadata, uint32_t* outTransformHint) { @@ -2056,38 +2056,9 @@ sp SurfaceComposerClient::createSurface(const String8& name, uin return s; } -sp SurfaceComposerClient::createWithSurfaceParent(const String8& name, uint32_t w, - uint32_t h, PixelFormat format, - uint32_t flags, Surface* parent, - LayerMetadata metadata, - uint32_t* outTransformHint) { - sp sur; - status_t err = mStatus; - - if (mStatus == NO_ERROR) { - sp handle; - sp parentGbp = parent->getIGraphicBufferProducer(); - sp gbp; - - uint32_t transformHint = 0; - int32_t id = -1; - err = mClient->createWithSurfaceParent(name, w, h, format, flags, parentGbp, - std::move(metadata), &handle, &gbp, &id, - &transformHint); - if (outTransformHint) { - *outTransformHint = transformHint; - } - ALOGE_IF(err, "SurfaceComposerClient::createWithSurfaceParent error %s", strerror(-err)); - if (err == NO_ERROR) { - return new SurfaceControl(this, handle, gbp, id, transformHint); - } - } - return nullptr; -} - status_t SurfaceComposerClient::createSurfaceChecked(const String8& name, uint32_t w, uint32_t h, PixelFormat format, - sp* outSurface, uint32_t flags, + sp* outSurface, int32_t flags, const sp& parentHandle, LayerMetadata metadata, uint32_t* outTransformHint) { @@ -2095,21 +2066,17 @@ status_t SurfaceComposerClient::createSurfaceChecked(const String8& name, uint32 status_t err = mStatus; if (mStatus == NO_ERROR) { - sp handle; - sp gbp; - - uint32_t transformHint = 0; - int32_t id = -1; - err = mClient->createSurface(name, w, h, format, flags, parentHandle, std::move(metadata), - &handle, &gbp, &id, &transformHint); - + gui::CreateSurfaceResult result; + binder::Status status = mClient->createSurface(std::string(name.string()), flags, + parentHandle, std::move(metadata), &result); + err = statusTFromBinderStatus(status); if (outTransformHint) { - *outTransformHint = transformHint; + *outTransformHint = result.transformHint; } ALOGE_IF(err, "SurfaceComposerClient::createSurface error %s", strerror(-err)); if (err == NO_ERROR) { - *outSurface = - new SurfaceControl(this, handle, gbp, id, w, h, format, transformHint, flags); + *outSurface = new SurfaceControl(this, result.handle, result.layerId, w, h, format, + result.transformHint, flags); } } return err; @@ -2120,12 +2087,12 @@ sp SurfaceComposerClient::mirrorSurface(SurfaceControl* mirrorFr return nullptr; } - sp handle; sp mirrorFromHandle = mirrorFromSurface->getHandle(); - int32_t layer_id = -1; - status_t err = mClient->mirrorSurface(mirrorFromHandle, &handle, &layer_id); + gui::MirrorSurfaceResult result; + const binder::Status status = mClient->mirrorSurface(mirrorFromHandle, &result); + const status_t err = statusTFromBinderStatus(status); if (err == NO_ERROR) { - return new SurfaceControl(this, handle, nullptr, layer_id, true /* owned */); + return new SurfaceControl(this, result.handle, result.layerId); } return nullptr; } @@ -2134,7 +2101,8 @@ status_t SurfaceComposerClient::clearLayerFrameStats(const sp& token) c if (mStatus != NO_ERROR) { return mStatus; } - return mClient->clearLayerFrameStats(token); + const binder::Status status = mClient->clearLayerFrameStats(token); + return statusTFromBinderStatus(status); } status_t SurfaceComposerClient::getLayerFrameStats(const sp& token, @@ -2142,7 +2110,24 @@ status_t SurfaceComposerClient::getLayerFrameStats(const sp& token, if (mStatus != NO_ERROR) { return mStatus; } - return mClient->getLayerFrameStats(token, outStats); + gui::FrameStats stats; + const binder::Status status = mClient->getLayerFrameStats(token, &stats); + if (status.isOk()) { + outStats->refreshPeriodNano = stats.refreshPeriodNano; + outStats->desiredPresentTimesNano.setCapacity(stats.desiredPresentTimesNano.size()); + for (const auto& t : stats.desiredPresentTimesNano) { + outStats->desiredPresentTimesNano.add(t); + } + outStats->actualPresentTimesNano.setCapacity(stats.actualPresentTimesNano.size()); + for (const auto& t : stats.actualPresentTimesNano) { + outStats->actualPresentTimesNano.add(t); + } + outStats->frameReadyTimesNano.setCapacity(stats.frameReadyTimesNano.size()); + for (const auto& t : stats.frameReadyTimesNano) { + outStats->frameReadyTimesNano.add(t); + } + } + return statusTFromBinderStatus(status); } // ---------------------------------------------------------------------------- diff --git a/libs/gui/SurfaceControl.cpp b/libs/gui/SurfaceControl.cpp index 654fb336fe..84257dee9b 100644 --- a/libs/gui/SurfaceControl.cpp +++ b/libs/gui/SurfaceControl.cpp @@ -49,12 +49,10 @@ namespace android { // ============================================================================ SurfaceControl::SurfaceControl(const sp& client, const sp& handle, - const sp& gbp, int32_t layerId, - uint32_t w, uint32_t h, PixelFormat format, uint32_t transform, - uint32_t flags) + int32_t layerId, uint32_t w, uint32_t h, PixelFormat format, + uint32_t transform, uint32_t flags) : mClient(client), mHandle(handle), - mGraphicBufferProducer(gbp), mLayerId(layerId), mTransformHint(transform), mWidth(w), @@ -65,7 +63,6 @@ SurfaceControl::SurfaceControl(const sp& client, const sp SurfaceControl::SurfaceControl(const sp& other) { mClient = other->mClient; mHandle = other->mHandle; - mGraphicBufferProducer = other->mGraphicBufferProducer; mTransformHint = other->mTransformHint; mLayerId = other->mLayerId; mWidth = other->mWidth; @@ -165,11 +162,11 @@ sp SurfaceControl::createSurface() void SurfaceControl::updateDefaultBufferSize(uint32_t width, uint32_t height) { Mutex::Autolock _l(mLock); - mWidth = width; mHeight = height; + mWidth = width; + mHeight = height; if (mBbq) { mBbq->update(mBbqChild, width, height, mFormat); } - } sp SurfaceControl::getLayerStateHandle() const @@ -245,9 +242,7 @@ status_t SurfaceControl::readFromParcel(const Parcel& parcel, *outSurfaceControl = new SurfaceControl(new SurfaceComposerClient( interface_cast(client)), - handle.get(), nullptr, layerId, - width, height, format, - transformHint); + handle.get(), layerId, width, height, format, transformHint); return NO_ERROR; } diff --git a/libs/gui/aidl/android/gui/CreateSurfaceResult.aidl b/libs/gui/aidl/android/gui/CreateSurfaceResult.aidl new file mode 100644 index 0000000000..39e49167aa --- /dev/null +++ b/libs/gui/aidl/android/gui/CreateSurfaceResult.aidl @@ -0,0 +1,24 @@ +/* + * Copyright 2022 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. + */ + +package android.gui; + +/** @hide */ +parcelable CreateSurfaceResult { + IBinder handle; + int layerId; + int transformHint; +} diff --git a/libs/gui/aidl/android/gui/ISurfaceComposer.aidl b/libs/gui/aidl/android/gui/ISurfaceComposer.aidl index 6ec6f760ae..26b4e819ee 100644 --- a/libs/gui/aidl/android/gui/ISurfaceComposer.aidl +++ b/libs/gui/aidl/android/gui/ISurfaceComposer.aidl @@ -34,6 +34,7 @@ import android.gui.IFpsListener; import android.gui.IHdrLayerInfoListener; import android.gui.IRegionSamplingListener; import android.gui.IScreenCaptureListener; +import android.gui.ISurfaceComposerClient; import android.gui.ITransactionTraceListener; import android.gui.ITunnelModeEnabledListener; import android.gui.IWindowInfosListener; @@ -45,6 +46,12 @@ import android.gui.StaticDisplayInfo; /** @hide */ interface ISurfaceComposer { + + /** + * Create a connection with SurfaceFlinger. + */ + @nullable ISurfaceComposerClient createConnection(); + /** * Create a virtual display * requires ACCESS_SURFACE_FLINGER permission. diff --git a/libs/gui/aidl/android/gui/ISurfaceComposerClient.aidl b/libs/gui/aidl/android/gui/ISurfaceComposerClient.aidl new file mode 100644 index 0000000000..71933aa6ff --- /dev/null +++ b/libs/gui/aidl/android/gui/ISurfaceComposerClient.aidl @@ -0,0 +1,62 @@ +/* + * Copyright 2022 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. + */ + +package android.gui; + +import android.gui.CreateSurfaceResult; +import android.gui.FrameStats; +import android.gui.LayerMetadata; +import android.gui.MirrorSurfaceResult; + +/** @hide */ +interface ISurfaceComposerClient { + + // flags for createSurface() + // (keep in sync with SurfaceControl.java) + const int eHidden = 0x00000004; + const int eDestroyBackbuffer = 0x00000020; + const int eSkipScreenshot = 0x00000040; + const int eSecure = 0x00000080; + const int eNonPremultiplied = 0x00000100; + const int eOpaque = 0x00000400; + const int eProtectedByApp = 0x00000800; + const int eProtectedByDRM = 0x00001000; + const int eCursorWindow = 0x00002000; + const int eNoColorFill = 0x00004000; + + const int eFXSurfaceBufferQueue = 0x00000000; + const int eFXSurfaceEffect = 0x00020000; + const int eFXSurfaceBufferState = 0x00040000; + const int eFXSurfaceContainer = 0x00080000; + const int eFXSurfaceMask = 0x000F0000; + + /** + * Requires ACCESS_SURFACE_FLINGER permission + */ + CreateSurfaceResult createSurface(@utf8InCpp String name, int flags, @nullable IBinder parent, in LayerMetadata metadata); + + /** + * Requires ACCESS_SURFACE_FLINGER permission + */ + void clearLayerFrameStats(IBinder handle); + + /** + * Requires ACCESS_SURFACE_FLINGER permission + */ + FrameStats getLayerFrameStats(IBinder handle); + + MirrorSurfaceResult mirrorSurface(IBinder mirrorFromHandle); +} diff --git a/libs/gui/aidl/android/gui/LayerMetadata.aidl b/libs/gui/aidl/android/gui/LayerMetadata.aidl new file mode 100644 index 0000000000..1368ac512f --- /dev/null +++ b/libs/gui/aidl/android/gui/LayerMetadata.aidl @@ -0,0 +1,19 @@ +/* + * Copyright 2022 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. + */ + +package android.gui; + +parcelable LayerMetadata cpp_header "gui/LayerMetadata.h"; diff --git a/libs/gui/aidl/android/gui/MirrorSurfaceResult.aidl b/libs/gui/aidl/android/gui/MirrorSurfaceResult.aidl new file mode 100644 index 0000000000..9fac3e8644 --- /dev/null +++ b/libs/gui/aidl/android/gui/MirrorSurfaceResult.aidl @@ -0,0 +1,23 @@ +/* + * Copyright 2022 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. + */ + +package android.gui; + +/** @hide */ +parcelable MirrorSurfaceResult { + IBinder handle; + int layerId; +} diff --git a/libs/gui/include/gui/ISurfaceComposer.h b/libs/gui/include/gui/ISurfaceComposer.h index 8f75d296c4..7139e1bb93 100644 --- a/libs/gui/include/gui/ISurfaceComposer.h +++ b/libs/gui/include/gui/ISurfaceComposer.h @@ -62,8 +62,6 @@ struct DisplayStatInfo; struct DisplayState; struct InputWindowCommands; class HdrCapabilities; -class IGraphicBufferProducer; -class ISurfaceComposerClient; class Rect; using gui::FrameTimelineInfo; @@ -127,11 +125,6 @@ public: using EventRegistrationFlags = ftl::Flags; - /* - * Create a connection with SurfaceFlinger. - */ - virtual sp createConnection() = 0; - /* return an IDisplayEventConnection */ virtual sp createDisplayEventConnection( VsyncSource vsyncSource = eVsyncSourceApp, @@ -159,7 +152,7 @@ public: // Note: BOOT_FINISHED must remain this value, it is called from // Java by ActivityManagerService. BOOT_FINISHED = IBinder::FIRST_CALL_TRANSACTION, - CREATE_CONNECTION, + CREATE_CONNECTION, // Deprecated. Autogenerated by .aidl now. GET_STATIC_DISPLAY_INFO, // Deprecated. Autogenerated by .aidl now. CREATE_DISPLAY_EVENT_CONNECTION, CREATE_DISPLAY, // Deprecated. Autogenerated by .aidl now. diff --git a/libs/gui/include/gui/ISurfaceComposerClient.h b/libs/gui/include/gui/ISurfaceComposerClient.h deleted file mode 100644 index 9e9e191480..0000000000 --- a/libs/gui/include/gui/ISurfaceComposerClient.h +++ /dev/null @@ -1,97 +0,0 @@ -/* - * Copyright (C) 2007 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. - */ - -#pragma once - -#include -#include -#include -#include - -#include - -namespace android { - -class FrameStats; -class IGraphicBufferProducer; - -class ISurfaceComposerClient : public IInterface { -public: - DECLARE_META_INTERFACE(SurfaceComposerClient) - - // flags for createSurface() - enum { // (keep in sync with SurfaceControl.java) - eHidden = 0x00000004, - eDestroyBackbuffer = 0x00000020, - eSkipScreenshot = 0x00000040, - eSecure = 0x00000080, - eNonPremultiplied = 0x00000100, - eOpaque = 0x00000400, - eProtectedByApp = 0x00000800, - eProtectedByDRM = 0x00001000, - eCursorWindow = 0x00002000, - eNoColorFill = 0x00004000, - - eFXSurfaceBufferQueue = 0x00000000, - eFXSurfaceEffect = 0x00020000, - eFXSurfaceBufferState = 0x00040000, - eFXSurfaceContainer = 0x00080000, - eFXSurfaceMask = 0x000F0000, - }; - - // TODO(b/172002646): Clean up the Surface Creation Arguments - /* - * Requires ACCESS_SURFACE_FLINGER permission - */ - virtual status_t createSurface(const String8& name, uint32_t w, uint32_t h, PixelFormat format, - uint32_t flags, const sp& parent, - LayerMetadata metadata, sp* handle, - sp* gbp, int32_t* outLayerId, - uint32_t* outTransformHint) = 0; - - /* - * Requires ACCESS_SURFACE_FLINGER permission - */ - virtual status_t createWithSurfaceParent(const String8& name, uint32_t w, uint32_t h, - PixelFormat format, uint32_t flags, - const sp& parent, - LayerMetadata metadata, sp* handle, - sp* gbp, int32_t* outLayerId, - uint32_t* outTransformHint) = 0; - - /* - * Requires ACCESS_SURFACE_FLINGER permission - */ - virtual status_t clearLayerFrameStats(const sp& handle) const = 0; - - /* - * Requires ACCESS_SURFACE_FLINGER permission - */ - virtual status_t getLayerFrameStats(const sp& handle, FrameStats* outStats) const = 0; - - virtual status_t mirrorSurface(const sp& mirrorFromHandle, sp* outHandle, - int32_t* outLayerId) = 0; -}; - -class BnSurfaceComposerClient : public SafeBnInterface { -public: - BnSurfaceComposerClient() - : SafeBnInterface("BnSurfaceComposerClient") {} - - status_t onTransact(uint32_t code, const Parcel& data, Parcel* reply, uint32_t flags) override; -}; - -} // namespace android diff --git a/libs/gui/include/gui/LayerMetadata.h b/libs/gui/include/gui/LayerMetadata.h index 27f4d379e9..5af598956b 100644 --- a/libs/gui/include/gui/LayerMetadata.h +++ b/libs/gui/include/gui/LayerMetadata.h @@ -20,7 +20,7 @@ #include -namespace android { +namespace android::gui { enum { METADATA_OWNER_UID = 1, @@ -69,4 +69,13 @@ enum class GameMode : int32_t { ftl_last = Battery }; -} // namespace android +} // namespace android::gui + +using android::gui::METADATA_ACCESSIBILITY_ID; +using android::gui::METADATA_DEQUEUE_TIME; +using android::gui::METADATA_GAME_MODE; +using android::gui::METADATA_MOUSE_CURSOR; +using android::gui::METADATA_OWNER_PID; +using android::gui::METADATA_OWNER_UID; +using android::gui::METADATA_TASK_ID; +using android::gui::METADATA_WINDOW_TYPE; diff --git a/libs/gui/include/gui/LayerState.h b/libs/gui/include/gui/LayerState.h index 0a9b75a7f1..4cd9a56fcd 100644 --- a/libs/gui/include/gui/LayerState.h +++ b/libs/gui/include/gui/LayerState.h @@ -51,7 +51,9 @@ namespace android { class Parcel; -class ISurfaceComposerClient; + +using gui::ISurfaceComposerClient; +using gui::LayerMetadata; struct client_cache_t { wp token = nullptr; diff --git a/libs/gui/include/gui/SurfaceComposerClient.h b/libs/gui/include/gui/SurfaceComposerClient.h index 8569a27808..dc9624269b 100644 --- a/libs/gui/include/gui/SurfaceComposerClient.h +++ b/libs/gui/include/gui/SurfaceComposerClient.h @@ -40,6 +40,8 @@ #include #include +#include + #include #include #include @@ -53,14 +55,15 @@ namespace android { class HdrCapabilities; -class ISurfaceComposerClient; class IGraphicBufferProducer; class ITunnelModeEnabledListener; class Region; using gui::DisplayCaptureArgs; using gui::IRegionSamplingListener; +using gui::ISurfaceComposerClient; using gui::LayerCaptureArgs; +using gui::LayerMetadata; struct SurfaceControlStats { SurfaceControlStats(const sp& sc, nsecs_t latchTime, @@ -312,7 +315,7 @@ public: uint32_t w, // width in pixel uint32_t h, // height in pixel PixelFormat format, // pixel-format desired - uint32_t flags = 0, // usage flags + int32_t flags = 0, // usage flags const sp& parentHandle = nullptr, // parentHandle LayerMetadata metadata = LayerMetadata(), // metadata uint32_t* outTransformHint = nullptr); @@ -322,21 +325,11 @@ public: uint32_t h, // height in pixel PixelFormat format, // pixel-format desired sp* outSurface, - uint32_t flags = 0, // usage flags + int32_t flags = 0, // usage flags const sp& parentHandle = nullptr, // parentHandle LayerMetadata metadata = LayerMetadata(), // metadata uint32_t* outTransformHint = nullptr); - //! Create a surface - sp createWithSurfaceParent(const String8& name, // name of the surface - uint32_t w, // width in pixel - uint32_t h, // height in pixel - PixelFormat format, // pixel-format desired - uint32_t flags = 0, // usage flags - Surface* parent = nullptr, // parent - LayerMetadata metadata = LayerMetadata(), // metadata - uint32_t* outTransformHint = nullptr); - // Creates a mirrored hierarchy for the mirrorFromSurface. This returns a SurfaceControl // which is a parent of the root of the mirrored hierarchy. // diff --git a/libs/gui/include/gui/SurfaceControl.h b/libs/gui/include/gui/SurfaceControl.h index b72cf8390e..e4a1350643 100644 --- a/libs/gui/include/gui/SurfaceControl.h +++ b/libs/gui/include/gui/SurfaceControl.h @@ -24,11 +24,12 @@ #include #include +#include + #include #include #include -#include #include namespace android { @@ -93,8 +94,7 @@ public: explicit SurfaceControl(const sp& other); SurfaceControl(const sp& client, const sp& handle, - const sp& gbp, int32_t layerId, - uint32_t width = 0, uint32_t height = 0, PixelFormat format = 0, + int32_t layerId, uint32_t width = 0, uint32_t height = 0, PixelFormat format = 0, uint32_t transformHint = 0, uint32_t flags = 0); sp getParentingLayer(); @@ -115,8 +115,7 @@ private: status_t validate() const; sp mClient; - sp mHandle; - sp mGraphicBufferProducer; + sp mHandle; mutable Mutex mLock; mutable sp mSurfaceData; mutable sp mBbq; diff --git a/libs/gui/tests/Surface_test.cpp b/libs/gui/tests/Surface_test.cpp index a9d8436e41..4ab380c8b8 100644 --- a/libs/gui/tests/Surface_test.cpp +++ b/libs/gui/tests/Surface_test.cpp @@ -692,7 +692,6 @@ public: mSupportsPresent = supportsPresent; } - sp createConnection() override { return nullptr; } sp createDisplayEventConnection( ISurfaceComposer::VsyncSource, ISurfaceComposer::EventRegistrationFlags) override { return nullptr; @@ -725,6 +724,11 @@ public: void setSupportsPresent(bool supportsPresent) { mSupportsPresent = supportsPresent; } + binder::Status createConnection(sp* outClient) override { + *outClient = nullptr; + return binder::Status::ok(); + } + binder::Status createDisplay(const std::string& /*displayName*/, bool /*secure*/, sp* /*outDisplay*/) override { return binder::Status::ok(); diff --git a/services/surfaceflinger/BufferLayer.h b/services/surfaceflinger/BufferLayer.h index 3e70493101..2f3db7428b 100644 --- a/services/surfaceflinger/BufferLayer.h +++ b/services/surfaceflinger/BufferLayer.h @@ -20,7 +20,7 @@ #include #include -#include +#include #include #include #include diff --git a/services/surfaceflinger/Client.cpp b/services/surfaceflinger/Client.cpp index 6d7b732b36..b27055d57a 100644 --- a/services/surfaceflinger/Client.cpp +++ b/services/surfaceflinger/Client.cpp @@ -21,12 +21,16 @@ #include +#include + #include "Client.h" #include "Layer.h" #include "SurfaceFlinger.h" namespace android { +using gui::aidl_utils::binderStatusFromStatusT; + // --------------------------------------------------------------------------- const String16 sAccessSurfaceFlinger("android.permission.ACCESS_SURFACE_FLINGER"); @@ -72,52 +76,74 @@ sp Client::getLayerUser(const sp& handle) const return lbc; } -status_t Client::createSurface(const String8& name, uint32_t /* w */, uint32_t /* h */, - PixelFormat /* format */, uint32_t flags, - const sp& parentHandle, LayerMetadata metadata, - sp* outHandle, sp* /* gbp */, - int32_t* outLayerId, uint32_t* outTransformHint) { +binder::Status Client::createSurface(const std::string& name, int32_t flags, + const sp& parent, const gui::LayerMetadata& metadata, + gui::CreateSurfaceResult* outResult) { // We rely on createLayer to check permissions. - LayerCreationArgs args(mFlinger.get(), this, name.c_str(), flags, std::move(metadata)); - return mFlinger->createLayer(args, outHandle, parentHandle, outLayerId, nullptr, - outTransformHint); -} - -status_t Client::createWithSurfaceParent(const String8& /* name */, uint32_t /* w */, - uint32_t /* h */, PixelFormat /* format */, - uint32_t /* flags */, - const sp& /* parent */, - LayerMetadata /* metadata */, sp* /* handle */, - sp* /* gbp */, - int32_t* /* outLayerId */, - uint32_t* /* outTransformHint */) { - // This api does not make sense with blast since SF no longer tracks IGBP. This api should be - // removed. - return BAD_VALUE; -} - -status_t Client::mirrorSurface(const sp& mirrorFromHandle, sp* outHandle, - int32_t* outLayerId) { - LayerCreationArgs args(mFlinger.get(), this, "MirrorRoot", 0 /* flags */, LayerMetadata()); - return mFlinger->mirrorLayer(args, mirrorFromHandle, outHandle, outLayerId); + sp handle; + int32_t layerId; + uint32_t transformHint; + LayerCreationArgs args(mFlinger.get(), this, name.c_str(), static_cast(flags), + std::move(metadata)); + const status_t status = + mFlinger->createLayer(args, &handle, parent, &layerId, nullptr, &transformHint); + if (status == NO_ERROR) { + outResult->handle = handle; + outResult->layerId = layerId; + outResult->transformHint = static_cast(transformHint); + } + return binderStatusFromStatusT(status); } -status_t Client::clearLayerFrameStats(const sp& handle) const { +binder::Status Client::clearLayerFrameStats(const sp& handle) { + status_t status; sp layer = getLayerUser(handle); if (layer == nullptr) { - return NAME_NOT_FOUND; + status = NAME_NOT_FOUND; + } else { + layer->clearFrameStats(); + status = NO_ERROR; } - layer->clearFrameStats(); - return NO_ERROR; + return binderStatusFromStatusT(status); } -status_t Client::getLayerFrameStats(const sp& handle, FrameStats* outStats) const { +binder::Status Client::getLayerFrameStats(const sp& handle, gui::FrameStats* outStats) { + status_t status; sp layer = getLayerUser(handle); if (layer == nullptr) { - return NAME_NOT_FOUND; + status = NAME_NOT_FOUND; + } else { + FrameStats stats; + layer->getFrameStats(&stats); + outStats->refreshPeriodNano = stats.refreshPeriodNano; + outStats->desiredPresentTimesNano.reserve(stats.desiredPresentTimesNano.size()); + for (const auto& t : stats.desiredPresentTimesNano) { + outStats->desiredPresentTimesNano.push_back(t); + } + outStats->actualPresentTimesNano.reserve(stats.actualPresentTimesNano.size()); + for (const auto& t : stats.actualPresentTimesNano) { + outStats->actualPresentTimesNano.push_back(t); + } + outStats->frameReadyTimesNano.reserve(stats.frameReadyTimesNano.size()); + for (const auto& t : stats.frameReadyTimesNano) { + outStats->frameReadyTimesNano.push_back(t); + } + status = NO_ERROR; } - layer->getFrameStats(outStats); - return NO_ERROR; + return binderStatusFromStatusT(status); +} + +binder::Status Client::mirrorSurface(const sp& mirrorFromHandle, + gui::MirrorSurfaceResult* outResult) { + sp handle; + int32_t layerId; + LayerCreationArgs args(mFlinger.get(), this, "MirrorRoot", 0 /* flags */, gui::LayerMetadata()); + status_t status = mFlinger->mirrorLayer(args, mirrorFromHandle, &handle, &layerId); + if (status == NO_ERROR) { + outResult->handle = handle; + outResult->layerId = layerId; + } + return binderStatusFromStatusT(status); } // --------------------------------------------------------------------------- diff --git a/services/surfaceflinger/Client.h b/services/surfaceflinger/Client.h index 15cd763822..4720d5c43d 100644 --- a/services/surfaceflinger/Client.h +++ b/services/surfaceflinger/Client.h @@ -24,15 +24,14 @@ #include #include -#include +#include namespace android { class Layer; class SurfaceFlinger; -class Client : public BnSurfaceComposerClient -{ +class Client : public gui::BnSurfaceComposerClient { public: explicit Client(const sp& flinger); ~Client() = default; @@ -47,25 +46,18 @@ public: private: // ISurfaceComposerClient interface - virtual status_t createSurface(const String8& name, uint32_t w, uint32_t h, PixelFormat format, - uint32_t flags, const sp& parent, - LayerMetadata metadata, sp* handle, - sp* gbp, int32_t* outLayerId, - uint32_t* outTransformHint = nullptr); - virtual status_t createWithSurfaceParent(const String8& name, uint32_t w, uint32_t h, - PixelFormat format, uint32_t flags, - const sp& parent, - LayerMetadata metadata, sp* handle, - sp* gbp, int32_t* outLayerId, - uint32_t* outTransformHint = nullptr); + binder::Status createSurface(const std::string& name, int32_t flags, const sp& parent, + const gui::LayerMetadata& metadata, + gui::CreateSurfaceResult* outResult) override; - status_t mirrorSurface(const sp& mirrorFromHandle, sp* handle, - int32_t* outLayerId); + binder::Status clearLayerFrameStats(const sp& handle) override; - virtual status_t clearLayerFrameStats(const sp& handle) const; + binder::Status getLayerFrameStats(const sp& handle, + gui::FrameStats* outStats) override; - virtual status_t getLayerFrameStats(const sp& handle, FrameStats* outStats) const; + binder::Status mirrorSurface(const sp& mirrorFromHandle, + gui::MirrorSurfaceResult* outResult) override; // constant sp mFlinger; diff --git a/services/surfaceflinger/FpsReporter.cpp b/services/surfaceflinger/FpsReporter.cpp index e12835f0f6..c89c9ffcd4 100644 --- a/services/surfaceflinger/FpsReporter.cpp +++ b/services/surfaceflinger/FpsReporter.cpp @@ -56,8 +56,8 @@ void FpsReporter::dispatchLayerFps() { mFlinger.mCurrentState.traverse([&](Layer* layer) { auto& currentState = layer->getDrawingState(); - if (currentState.metadata.has(METADATA_TASK_ID)) { - int32_t taskId = currentState.metadata.getInt32(METADATA_TASK_ID, 0); + if (currentState.metadata.has(gui::METADATA_TASK_ID)) { + int32_t taskId = currentState.metadata.getInt32(gui::METADATA_TASK_ID, 0); if (seenTasks.count(taskId) == 0) { // localListeners is expected to be tiny for (TrackedListener& listener : localListeners) { diff --git a/services/surfaceflinger/Layer.cpp b/services/surfaceflinger/Layer.cpp index b4f9c4e720..791ab06258 100644 --- a/services/surfaceflinger/Layer.cpp +++ b/services/surfaceflinger/Layer.cpp @@ -85,6 +85,8 @@ constexpr int kDumpTableRowLength = 159; using namespace ftl::flag_operators; using base::StringAppendF; +using gui::GameMode; +using gui::LayerMetadata; using gui::WindowInfo; using PresentState = frametimeline::SurfaceFrame::PresentState; @@ -96,7 +98,8 @@ Layer::Layer(const LayerCreationArgs& args) mFlinger(args.flinger), mName(base::StringPrintf("%s#%d", args.name.c_str(), sequence)), mClientRef(args.client), - mWindowType(static_cast(args.metadata.getInt32(METADATA_WINDOW_TYPE, 0))), + mWindowType(static_cast( + args.metadata.getInt32(gui::METADATA_WINDOW_TYPE, 0))), mLayerCreationFlags(args.flags) { uint32_t layerFlags = 0; if (args.flags & ISurfaceComposerClient::eHidden) layerFlags |= layer_state_t::eLayerHidden; @@ -161,8 +164,8 @@ Layer::Layer(const LayerCreationArgs& args) if (mCallingUid == AID_GRAPHICS || mCallingUid == AID_SYSTEM) { // If the system didn't send an ownerUid, use the callingUid for the ownerUid. - mOwnerUid = args.metadata.getInt32(METADATA_OWNER_UID, mCallingUid); - mOwnerPid = args.metadata.getInt32(METADATA_OWNER_PID, mCallingPid); + mOwnerUid = args.metadata.getInt32(gui::METADATA_OWNER_UID, mCallingUid); + mOwnerPid = args.metadata.getInt32(gui::METADATA_OWNER_PID, mCallingPid); } else { // A create layer request from a non system request cannot specify the owner uid mOwnerUid = mCallingUid; @@ -1576,8 +1579,9 @@ size_t Layer::getChildrenCount() const { void Layer::setGameModeForTree(GameMode gameMode) { const auto& currentState = getDrawingState(); - if (currentState.metadata.has(METADATA_GAME_MODE)) { - gameMode = static_cast(currentState.metadata.getInt32(METADATA_GAME_MODE, 0)); + if (currentState.metadata.has(gui::METADATA_GAME_MODE)) { + gameMode = + static_cast(currentState.metadata.getInt32(gui::METADATA_GAME_MODE, 0)); } setGameMode(gameMode); for (const sp& child : mCurrentChildren) { diff --git a/services/surfaceflinger/Layer.h b/services/surfaceflinger/Layer.h index 455920be62..c89b254e99 100644 --- a/services/surfaceflinger/Layer.h +++ b/services/surfaceflinger/Layer.h @@ -17,8 +17,8 @@ #pragma once #include +#include #include -#include #include #include #include diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp index 3762033942..46b1a60a77 100644 --- a/services/surfaceflinger/SurfaceFlinger.cpp +++ b/services/surfaceflinger/SurfaceFlinger.cpp @@ -171,8 +171,10 @@ using CompositionStrategyPredictionState = android::compositionengine::impl:: using base::StringAppendF; using gui::DisplayInfo; +using gui::GameMode; using gui::IDisplayEventConnection; using gui::IWindowInfosListener; +using gui::LayerMetadata; using gui::WindowInfo; using gui::aidl_utils::binderStatusFromStatusT; using ui::ColorMode; @@ -477,11 +479,6 @@ void SurfaceFlinger::run() { mScheduler->run(); } -sp SurfaceFlinger::createConnection() { - const sp client = new Client(this); - return client->initCheck() == NO_ERROR ? client : nullptr; -} - sp SurfaceFlinger::createDisplay(const String8& displayName, bool secure) { // onTransact already checks for some permissions, but adding an additional check here. // This is to ensure that only system and graphics can request to create a secure @@ -4458,9 +4455,10 @@ uint32_t SurfaceFlinger::setClientStateLocked(const FrameTimelineInfo& frameTime } std::optional dequeueBufferTimestamp; if (what & layer_state_t::eMetadataChanged) { - dequeueBufferTimestamp = s.metadata.getInt64(METADATA_DEQUEUE_TIME); + dequeueBufferTimestamp = s.metadata.getInt64(gui::METADATA_DEQUEUE_TIME); - if (const int32_t gameMode = s.metadata.getInt32(METADATA_GAME_MODE, -1); gameMode != -1) { + if (const int32_t gameMode = s.metadata.getInt32(gui::METADATA_GAME_MODE, -1); + gameMode != -1) { // The transaction will be received on the Task layer and needs to be applied to all // child layers. Child layers that are added at a later point will obtain the game mode // info through addChild(). @@ -5503,11 +5501,11 @@ status_t SurfaceFlinger::CheckTransactCodeCredentials(uint32_t code) { case GET_DISPLAY_MODES: // Calling setTransactionState is safe, because you need to have been // granted a reference to Client* and Handle* to do anything with it. - case SET_TRANSACTION_STATE: - case CREATE_CONNECTION: { + case SET_TRANSACTION_STATE: { // This is not sensitive information, so should not require permission control. return OK; } + case CREATE_CONNECTION: case CREATE_DISPLAY: case DESTROY_DISPLAY: case GET_PRIMARY_PHYSICAL_DISPLAY_ID: @@ -6995,8 +6993,8 @@ const std::unordered_map& SurfaceFlinger::getGenericLayer // on the work to remove the table in that bug rather than adding more to // it. static const std::unordered_map genericLayerMetadataKeyMap{ - {"org.chromium.arc.V1_0.TaskId", METADATA_TASK_ID}, - {"org.chromium.arc.V1_0.CursorInfo", METADATA_MOUSE_CURSOR}, + {"org.chromium.arc.V1_0.TaskId", gui::METADATA_TASK_ID}, + {"org.chromium.arc.V1_0.CursorInfo", gui::METADATA_MOUSE_CURSOR}, }; return genericLayerMetadataKeyMap; } @@ -7213,6 +7211,17 @@ bool SurfaceFlinger::commitCreatedLayers() { // gui::ISurfaceComposer +binder::Status SurfaceComposerAIDL::createConnection(sp* outClient) { + const sp client = new Client(mFlinger); + if (client->initCheck() == NO_ERROR) { + *outClient = client; + return binder::Status::ok(); + } else { + *outClient = nullptr; + return binderStatusFromStatusT(BAD_VALUE); + } +} + binder::Status SurfaceComposerAIDL::createDisplay(const std::string& displayName, bool secure, sp* outDisplay) { status_t status = checkAccessPermission(); diff --git a/services/surfaceflinger/SurfaceFlinger.h b/services/surfaceflinger/SurfaceFlinger.h index df00eeecdf..353df24f7f 100644 --- a/services/surfaceflinger/SurfaceFlinger.h +++ b/services/surfaceflinger/SurfaceFlinger.h @@ -25,12 +25,12 @@ #include #include #include +#include #include #include #include #include #include -#include #include #include #include @@ -536,7 +536,6 @@ private: } // Implements ISurfaceComposer - sp createConnection() override; sp createDisplay(const String8& displayName, bool secure); void destroyDisplay(const sp& displayToken); std::vector getPhysicalDisplayIds() const EXCLUDES(mStateLock) { @@ -1446,6 +1445,7 @@ class SurfaceComposerAIDL : public gui::BnSurfaceComposer { public: SurfaceComposerAIDL(sp sf) : mFlinger(std::move(sf)) {} + binder::Status createConnection(sp* outClient) override; binder::Status createDisplay(const std::string& displayName, bool secure, sp* outDisplay) override; binder::Status destroyDisplay(const sp& display) override; diff --git a/services/surfaceflinger/TimeStats/TimeStats.h b/services/surfaceflinger/TimeStats/TimeStats.h index 7a159b8eb7..61d7c22a2a 100644 --- a/services/surfaceflinger/TimeStats/TimeStats.h +++ b/services/surfaceflinger/TimeStats/TimeStats.h @@ -34,6 +34,8 @@ #include +using android::gui::GameMode; +using android::gui::LayerMetadata; using namespace android::surfaceflinger; namespace android { diff --git a/services/surfaceflinger/TimeStats/timestatsproto/include/timestatsproto/TimeStatsHelper.h b/services/surfaceflinger/TimeStats/timestatsproto/include/timestatsproto/TimeStatsHelper.h index 237ae8d761..60aa810e8b 100644 --- a/services/surfaceflinger/TimeStats/timestatsproto/include/timestatsproto/TimeStatsHelper.h +++ b/services/surfaceflinger/TimeStats/timestatsproto/include/timestatsproto/TimeStatsHelper.h @@ -24,6 +24,9 @@ #include #include +using android::gui::GameMode; +using android::gui::LayerMetadata; + namespace android { namespace surfaceflinger { diff --git a/services/surfaceflinger/Tracing/TransactionProtoParser.cpp b/services/surfaceflinger/Tracing/TransactionProtoParser.cpp index e8ecf2f33b..8ec6c99eb9 100644 --- a/services/surfaceflinger/Tracing/TransactionProtoParser.cpp +++ b/services/surfaceflinger/Tracing/TransactionProtoParser.cpp @@ -459,7 +459,7 @@ void TransactionProtoParser::fromProto(const proto::LayerState& proto, layer_sta layer.parentSurfaceControlForChild = new SurfaceControl(SurfaceComposerClient::getDefault(), mMapper->getLayerHandle(static_cast(layerId)), - nullptr, static_cast(layerId)); + static_cast(layerId)); } } if (proto.what() & layer_state_t::eRelativeLayerChanged) { @@ -470,7 +470,7 @@ void TransactionProtoParser::fromProto(const proto::LayerState& proto, layer_sta layer.relativeLayerSurfaceControl = new SurfaceControl(SurfaceComposerClient::getDefault(), mMapper->getLayerHandle(static_cast(layerId)), - nullptr, static_cast(layerId)); + static_cast(layerId)); } layer.z = proto.z(); } diff --git a/services/surfaceflinger/fuzzer/surfaceflinger_fuzzers_utils.h b/services/surfaceflinger/fuzzer/surfaceflinger_fuzzers_utils.h index 2a4d6ed86f..c4cd089a5e 100644 --- a/services/surfaceflinger/fuzzer/surfaceflinger_fuzzers_utils.h +++ b/services/surfaceflinger/fuzzer/surfaceflinger_fuzzers_utils.h @@ -555,7 +555,7 @@ public: sp fuzzBoot(FuzzedDataProvider *fdp) { mFlinger->callingThreadHasUnscopedSurfaceFlingerAccess(fdp->ConsumeBool()); - mFlinger->createConnection(); + const sp client = new Client(mFlinger); DisplayIdGenerator kGenerator; HalVirtualDisplayId halVirtualDisplayId = kGenerator.generateId().value(); diff --git a/services/surfaceflinger/layerproto/include/layerproto/LayerProtoParser.h b/services/surfaceflinger/layerproto/include/layerproto/LayerProtoParser.h index 52503bad3a..cdc2706ee2 100644 --- a/services/surfaceflinger/layerproto/include/layerproto/LayerProtoParser.h +++ b/services/surfaceflinger/layerproto/include/layerproto/LayerProtoParser.h @@ -24,6 +24,8 @@ #include #include +using android::gui::LayerMetadata; + namespace android { namespace surfaceflinger { diff --git a/services/surfaceflinger/tests/InvalidHandles_test.cpp b/services/surfaceflinger/tests/InvalidHandles_test.cpp index d192a2d83d..023133f2c3 100644 --- a/services/surfaceflinger/tests/InvalidHandles_test.cpp +++ b/services/surfaceflinger/tests/InvalidHandles_test.cpp @@ -48,7 +48,7 @@ protected: } sp makeNotSurfaceControl() { - return new SurfaceControl(mScc, new NotALayer(), nullptr, true); + return new SurfaceControl(mScc, new NotALayer(), 1); } }; @@ -64,4 +64,4 @@ TEST_F(InvalidHandleTest, captureLayersInvalidHandle) { } // namespace android // TODO(b/129481165): remove the #pragma below and fix conversion issues -#pragma clang diagnostic pop // ignored "-Wconversion" \ No newline at end of file +#pragma clang diagnostic pop // ignored "-Wconversion" diff --git a/services/surfaceflinger/tests/unittests/FpsReporterTest.cpp b/services/surfaceflinger/tests/unittests/FpsReporterTest.cpp index bb1f4328b5..9cac7c1723 100644 --- a/services/surfaceflinger/tests/unittests/FpsReporterTest.cpp +++ b/services/surfaceflinger/tests/unittests/FpsReporterTest.cpp @@ -51,6 +51,7 @@ using android::Hwc2::IComposer; using android::Hwc2::IComposerClient; using FakeHwcDisplayInjector = TestableSurfaceFlinger::FakeHwcDisplayInjector; +using gui::LayerMetadata; struct TestableFpsListener : public gui::BnFpsListener { TestableFpsListener() {} @@ -153,7 +154,7 @@ TEST_F(FpsReporterTest, callsListeners) { mParent = createBufferStateLayer(); constexpr int32_t kTaskId = 12; LayerMetadata targetMetadata; - targetMetadata.setInt32(METADATA_TASK_ID, kTaskId); + targetMetadata.setInt32(gui::METADATA_TASK_ID, kTaskId); mTarget = createBufferStateLayer(targetMetadata); mChild = createBufferStateLayer(); mGrandChild = createBufferStateLayer(); @@ -188,7 +189,7 @@ TEST_F(FpsReporterTest, callsListeners) { TEST_F(FpsReporterTest, rateLimits) { const constexpr int32_t kTaskId = 12; LayerMetadata targetMetadata; - targetMetadata.setInt32(METADATA_TASK_ID, kTaskId); + targetMetadata.setInt32(gui::METADATA_TASK_ID, kTaskId); mTarget = createBufferStateLayer(targetMetadata); mFlinger.mutableCurrentState().layersSortedByZ.add(mTarget); diff --git a/services/surfaceflinger/tests/unittests/GameModeTest.cpp b/services/surfaceflinger/tests/unittests/GameModeTest.cpp index 981ca1d227..b79909ad9d 100644 --- a/services/surfaceflinger/tests/unittests/GameModeTest.cpp +++ b/services/surfaceflinger/tests/unittests/GameModeTest.cpp @@ -34,6 +34,8 @@ using testing::_; using testing::Mock; using testing::Return; using FakeHwcDisplayInjector = TestableSurfaceFlinger::FakeHwcDisplayInjector; +using gui::GameMode; +using gui::LayerMetadata; class GameModeTest : public testing::Test { public: @@ -92,7 +94,7 @@ public: // Mocks the behavior of applying a transaction from WMShell void setGameModeMetadata(sp layer, GameMode gameMode) { - mLayerMetadata.setInt32(METADATA_GAME_MODE, static_cast(gameMode)); + mLayerMetadata.setInt32(gui::METADATA_GAME_MODE, static_cast(gameMode)); layer->setMetadata(mLayerMetadata); layer->setGameModeForTree(gameMode); } diff --git a/services/surfaceflinger/tests/unittests/LayerMetadataTest.cpp b/services/surfaceflinger/tests/unittests/LayerMetadataTest.cpp index 373fd74020..e6e02c1806 100644 --- a/services/surfaceflinger/tests/unittests/LayerMetadataTest.cpp +++ b/services/surfaceflinger/tests/unittests/LayerMetadataTest.cpp @@ -27,6 +27,8 @@ #include #include +using android::gui::LayerMetadata; + namespace android { namespace { @@ -113,4 +115,4 @@ TEST_F(LayerMetadataTest, merge) { } // namespace android // TODO(b/129481165): remove the #pragma below and fix conversion issues -#pragma clang diagnostic pop // ignored "-Wextra" \ No newline at end of file +#pragma clang diagnostic pop // ignored "-Wextra" diff --git a/services/surfaceflinger/tests/unittests/TransactionProtoParserTest.cpp b/services/surfaceflinger/tests/unittests/TransactionProtoParserTest.cpp index f5e3b77ca6..669fa3a7f0 100644 --- a/services/surfaceflinger/tests/unittests/TransactionProtoParserTest.cpp +++ b/services/surfaceflinger/tests/unittests/TransactionProtoParserTest.cpp @@ -49,8 +49,7 @@ TEST(TransactionProtoParserTest, parse) { ComposerState s; if (i == 1) { layer.parentSurfaceControlForChild = - new SurfaceControl(SurfaceComposerClient::getDefault(), layerHandle, nullptr, - 42); + new SurfaceControl(SurfaceComposerClient::getDefault(), layerHandle, 42); } s.state = layer; t1.states.add(s); -- cgit v1.2.3-59-g8ed1b From 728dba1f95927d8f7acb52d09c65c486f81ea50d Mon Sep 17 00:00:00 2001 From: Vishnu Nair Date: Wed, 29 Jun 2022 22:13:49 +0000 Subject: BBQ: Remove BufferFreedListener If BBQ is destroyed without calling abandon, its BufferItemConsumer will call onBufferFreed. Since BBQ registers itself as the BufferFreedListener we end up trying to promote and destroy the BBQ again. The BufferFreedListener is not used so as a simple fix, we remove the listener. Test: presubmit Bug: 200246498 Change-Id: I7b2be265b3c09d691cc0c41b80c73067f9a8b84d --- libs/gui/BLASTBufferQueue.cpp | 2 -- libs/gui/include/gui/BLASTBufferQueue.h | 5 +---- 2 files changed, 1 insertion(+), 6 deletions(-) (limited to 'libs/gui/BLASTBufferQueue.cpp') diff --git a/libs/gui/BLASTBufferQueue.cpp b/libs/gui/BLASTBufferQueue.cpp index f34061492a..77022dc9b8 100644 --- a/libs/gui/BLASTBufferQueue.cpp +++ b/libs/gui/BLASTBufferQueue.cpp @@ -159,7 +159,6 @@ BLASTBufferQueue::BLASTBufferQueue(const std::string& name, bool updateDestinati id++; mBufferItemConsumer->setName(String8(consumerName.c_str())); mBufferItemConsumer->setFrameAvailableListener(this); - mBufferItemConsumer->setBufferFreedListener(this); ComposerServiceAIDL::getComposerService()->getMaxAcquiredBufferCount(&mMaxAcquiredBuffers); mBufferItemConsumer->setMaxAcquiredBufferCount(mMaxAcquiredBuffers); @@ -1114,7 +1113,6 @@ void BLASTBufferQueue::abandon() { if (mBufferItemConsumer != nullptr) { mBufferItemConsumer->abandon(); mBufferItemConsumer->setFrameAvailableListener(nullptr); - mBufferItemConsumer->setBufferFreedListener(nullptr); } mBufferItemConsumer = nullptr; mConsumer = nullptr; diff --git a/libs/gui/include/gui/BLASTBufferQueue.h b/libs/gui/include/gui/BLASTBufferQueue.h index 9328a54184..f6bf861b06 100644 --- a/libs/gui/include/gui/BLASTBufferQueue.h +++ b/libs/gui/include/gui/BLASTBufferQueue.h @@ -69,9 +69,7 @@ private: bool mPreviouslyConnected GUARDED_BY(mMutex); }; -class BLASTBufferQueue - : public ConsumerBase::FrameAvailableListener, public BufferItemConsumer::BufferFreedListener -{ +class BLASTBufferQueue : public ConsumerBase::FrameAvailableListener { public: BLASTBufferQueue(const std::string& name, bool updateDestinationFrame = true); BLASTBufferQueue(const std::string& name, const sp& surface, int width, @@ -83,7 +81,6 @@ public: sp getSurface(bool includeSurfaceControlHandle); bool isSameSurfaceControl(const sp& surfaceControl) const; - void onBufferFreed(const wp&/* graphicBuffer*/) override { /* TODO */ } void onFrameReplaced(const BufferItem& item) override; void onFrameAvailable(const BufferItem& item) override; void onFrameDequeued(const uint64_t) override; -- cgit v1.2.3-59-g8ed1b From d00e0f76caeee54b436355fe2d1412e30a1f3c83 Mon Sep 17 00:00:00 2001 From: Chavi Weingarten Date: Thu, 14 Jul 2022 15:59:20 +0000 Subject: Added trace data for latch and release buffers It's helpful to see the specific layer and frame number that is being latched and released in SF to ensure we can trace where buffers go. Additionally, added new traces to BBQ where it could be flushing buffers or waiting until there's a free buffer Test: perfetto trace Bug: 238328090 Change-Id: I88fa6db62092438ed7ada12c3f0740dc3ac282d5 --- libs/gui/BLASTBufferQueue.cpp | 4 +++- libs/gui/include/gui/TraceUtils.h | 16 ++++++++++++++++ services/surfaceflinger/BufferLayer.cpp | 4 +++- services/surfaceflinger/BufferStateLayer.cpp | 16 ++++++++++------ services/surfaceflinger/BufferStateLayer.h | 5 +++++ 5 files changed, 37 insertions(+), 8 deletions(-) (limited to 'libs/gui/BLASTBufferQueue.cpp') diff --git a/libs/gui/BLASTBufferQueue.cpp b/libs/gui/BLASTBufferQueue.cpp index 2212e58e4d..3bf2e195c5 100644 --- a/libs/gui/BLASTBufferQueue.cpp +++ b/libs/gui/BLASTBufferQueue.cpp @@ -599,6 +599,7 @@ Rect BLASTBufferQueue::computeCrop(const BufferItem& item) { } void BLASTBufferQueue::acquireAndReleaseBuffer() { + BBQ_TRACE(); BufferItem bufferItem; status_t status = mBufferItemConsumer->acquireBuffer(&bufferItem, 0 /* expectedPresent */, false); @@ -612,6 +613,7 @@ void BLASTBufferQueue::acquireAndReleaseBuffer() { } void BLASTBufferQueue::flushAndWaitForFreeBuffer(std::unique_lock& lock) { + BBQ_TRACE(); if (!mSyncedFrameNumbers.empty() && mNumFrameAvailable > 0) { // We are waiting on a previous sync's transaction callback so allow another sync // transaction to proceed. @@ -642,8 +644,8 @@ void BLASTBufferQueue::onFrameAvailable(const BufferItem& item) { bool waitForTransactionCallback = !mSyncedFrameNumbers.empty(); { - BBQ_TRACE(); std::unique_lock _lock{mMutex}; + BBQ_TRACE(); const bool syncTransactionSet = mTransactionReadyCallback != nullptr; BQA_LOGV("onFrameAvailable-start syncTransactionSet=%s", boolToString(syncTransactionSet)); diff --git a/libs/gui/include/gui/TraceUtils.h b/libs/gui/include/gui/TraceUtils.h index e5d268445c..00096158e7 100644 --- a/libs/gui/include/gui/TraceUtils.h +++ b/libs/gui/include/gui/TraceUtils.h @@ -27,6 +27,8 @@ #define ATRACE_FORMAT_BEGIN(fmt, ...) TraceUtils::atraceFormatBegin(fmt, ##__VA_ARGS__) +#define ATRACE_FORMAT_INSTANT(fmt, ...) TraceUtils::intantFormat(fmt, ##__VA_ARGS__) + namespace android { class TraceUtils { @@ -50,6 +52,20 @@ public: ATRACE_BEGIN(buf); } + static void intantFormat(const char* fmt, ...) { + if (CC_LIKELY(!ATRACE_ENABLED())) return; + + const int BUFFER_SIZE = 256; + va_list ap; + char buf[BUFFER_SIZE]; + + va_start(ap, fmt); + vsnprintf(buf, BUFFER_SIZE, fmt, ap); + va_end(ap); + + ATRACE_INSTANT(buf); + } + }; // class TraceUtils } /* namespace android */ diff --git a/services/surfaceflinger/BufferLayer.cpp b/services/surfaceflinger/BufferLayer.cpp index fb15f1d06c..4ba5e907a7 100644 --- a/services/surfaceflinger/BufferLayer.cpp +++ b/services/surfaceflinger/BufferLayer.cpp @@ -37,6 +37,7 @@ #include #include #include +#include #include #include #include @@ -457,7 +458,8 @@ bool BufferLayer::shouldPresentNow(nsecs_t expectedPresentTime) const { bool BufferLayer::latchBuffer(bool& recomputeVisibleRegions, nsecs_t latchTime, nsecs_t expectedPresentTime) { - ATRACE_CALL(); + ATRACE_FORMAT_INSTANT("latchBuffer %s - %" PRIu64, getDebugName(), + getDrawingState().frameNumber); bool refreshRequired = latchSidebandStream(recomputeVisibleRegions); diff --git a/services/surfaceflinger/BufferStateLayer.cpp b/services/surfaceflinger/BufferStateLayer.cpp index d88d7c99e9..574e2f5bed 100644 --- a/services/surfaceflinger/BufferStateLayer.cpp +++ b/services/surfaceflinger/BufferStateLayer.cpp @@ -30,6 +30,7 @@ #include #include "TunnelModeEnabledReporter.h" +#include #include "EffectLayer.h" #include "FrameTracer/FrameTracer.h" #include "TimeStats/TimeStats.h" @@ -39,19 +40,20 @@ namespace android { using PresentState = frametimeline::SurfaceFrame::PresentState; -namespace { -void callReleaseBufferCallback(const sp& listener, - const sp& buffer, uint64_t framenumber, - const sp& releaseFence, - uint32_t currentMaxAcquiredBufferCount) { + +void BufferStateLayer::callReleaseBufferCallback(const sp& listener, + const sp& buffer, + uint64_t framenumber, + const sp& releaseFence, + uint32_t currentMaxAcquiredBufferCount) { if (!listener) { return; } + ATRACE_FORMAT_INSTANT("callReleaseBufferCallback %s - %" PRIu64, getDebugName(), framenumber); listener->onReleaseBuffer({buffer->getId(), framenumber}, releaseFence ? releaseFence : Fence::NO_FENCE, currentMaxAcquiredBufferCount); } -} // namespace BufferStateLayer::BufferStateLayer(const LayerCreationArgs& args) : BufferLayer(args), mHwcSlotGenerator(new HwcSlotGenerator()) { @@ -145,6 +147,8 @@ void BufferStateLayer::releasePendingBuffer(nsecs_t dequeueReadyTime) { handle->dequeueReadyTime = dequeueReadyTime; handle->currentMaxAcquiredBufferCount = mFlinger->getMaxAcquiredBufferCountForCurrentRefreshRate(mOwnerUid); + ATRACE_FORMAT_INSTANT("releasePendingBuffer %s - %" PRIu64, getDebugName(), + handle->previousReleaseCallbackId.framenumber); } for (auto& handle : mDrawingState.callbackHandles) { diff --git a/services/surfaceflinger/BufferStateLayer.h b/services/surfaceflinger/BufferStateLayer.h index 3f0dbe4039..bf98a279a3 100644 --- a/services/surfaceflinger/BufferStateLayer.h +++ b/services/surfaceflinger/BufferStateLayer.h @@ -135,6 +135,11 @@ private: bool simpleBufferUpdate(const layer_state_t& s) const override; + void callReleaseBufferCallback(const sp& listener, + const sp& buffer, uint64_t framenumber, + const sp& releaseFence, + uint32_t currentMaxAcquiredBufferCount); + ReleaseCallbackId mPreviousReleaseCallbackId = ReleaseCallbackId::INVALID_ID; uint64_t mPreviousReleasedFrameNumber = 0; -- cgit v1.2.3-59-g8ed1b From a4343e649c61e690634d7b56221b7323ab86561f Mon Sep 17 00:00:00 2001 From: Vishnu Nair Date: Thu, 8 Sep 2022 00:22:44 +0000 Subject: Clean up BBQ#abandon dead code Change-Id: I92f4d6c73a26885e1bb21c9683e25d8eb0ce8aa0 Test: presubmit Fixes: 245626507 --- libs/gui/BLASTBufferQueue.cpp | 44 --------------------------------- libs/gui/include/gui/BLASTBufferQueue.h | 1 - 2 files changed, 45 deletions(-) (limited to 'libs/gui/BLASTBufferQueue.cpp') diff --git a/libs/gui/BLASTBufferQueue.cpp b/libs/gui/BLASTBufferQueue.cpp index 3bf2e195c5..3afa339849 100644 --- a/libs/gui/BLASTBufferQueue.cpp +++ b/libs/gui/BLASTBufferQueue.cpp @@ -1083,50 +1083,6 @@ uint64_t BLASTBufferQueue::getLastAcquiredFrameNum() { return mLastAcquiredFrameNumber; } -void BLASTBufferQueue::abandon() { - std::unique_lock _lock{mMutex}; - // flush out the shadow queue - while (mNumFrameAvailable > 0) { - acquireAndReleaseBuffer(); - } - - // Clear submitted buffer states - mNumAcquired = 0; - mSubmitted.clear(); - mPendingRelease.clear(); - - if (!mPendingTransactions.empty()) { - BQA_LOGD("Applying pending transactions on abandon %d", - static_cast(mPendingTransactions.size())); - SurfaceComposerClient::Transaction t; - mergePendingTransactions(&t, std::numeric_limits::max() /* frameNumber */); - // All transactions on our apply token are one-way. See comment on mAppliedLastTransaction - t.setApplyToken(mApplyToken).apply(false, true); - } - - // Clear sync states - if (!mSyncedFrameNumbers.empty()) { - BQA_LOGD("mSyncedFrameNumbers cleared"); - mSyncedFrameNumbers.clear(); - } - - if (mSyncTransaction != nullptr) { - BQA_LOGD("mSyncTransaction cleared mAcquireSingleBuffer=%s", - mAcquireSingleBuffer ? "true" : "false"); - mSyncTransaction = nullptr; - mAcquireSingleBuffer = false; - } - - // abandon buffer queue - if (mBufferItemConsumer != nullptr) { - mBufferItemConsumer->abandon(); - mBufferItemConsumer->setFrameAvailableListener(nullptr); - } - mBufferItemConsumer = nullptr; - mConsumer = nullptr; - mProducer = nullptr; -} - bool BLASTBufferQueue::isSameSurfaceControl(const sp& surfaceControl) const { std::unique_lock _lock{mMutex}; return SurfaceControl::isSameSurface(mSurfaceControl, surfaceControl); diff --git a/libs/gui/include/gui/BLASTBufferQueue.h b/libs/gui/include/gui/BLASTBufferQueue.h index 95df811927..4535c98b97 100644 --- a/libs/gui/include/gui/BLASTBufferQueue.h +++ b/libs/gui/include/gui/BLASTBufferQueue.h @@ -108,7 +108,6 @@ public: uint32_t getLastTransformHint() const; uint64_t getLastAcquiredFrameNum(); - void abandon(); /** * Set a callback to be invoked when we are hung. The boolean parameter -- cgit v1.2.3-59-g8ed1b From 71fcf918ac5b8b3f870451e547aca25982d7cfe8 Mon Sep 17 00:00:00 2001 From: Vishnu Nair Date: Tue, 18 Oct 2022 09:14:20 -0700 Subject: SF: Avoid updating clients with stale or incorrect transform hints When the layer is removed from a display or the display the layer is on is turned off, the client will continue to receive transform hint updates via the transaction complete callback using the default/active displays install orientation. Once the layer is back on the display and it does not submit a new frame, a buffer with a suboptimal transform may remain on display. Fix this by not reporting stale/incorrect values via the callback. Once the layer is reparent back to the display and the display state is not OFF, it will continue to get hints via the callback. For special cases where we want the app to draw its first frame before the display is available, we rely on WMS and DMS to provide the right information so the client can calculate the hint. Bug: 251360251 Test: move app between displays, rotate, check final buffer transforms Change-Id: I0a9abac7e9cf4ade1c49ec400e73b634c8269b4b --- libs/gui/BLASTBufferQueue.cpp | 8 +++++--- libs/gui/ITransactionCompletedListener.cpp | 21 +++++++++++++++++++-- libs/gui/SurfaceComposerClient.cpp | 5 +++-- .../gui/include/gui/ITransactionCompletedListener.h | 4 ++-- libs/gui/include/gui/SurfaceComposerClient.h | 4 ++-- services/surfaceflinger/Layer.cpp | 10 +++++++++- services/surfaceflinger/Layer.h | 3 ++- services/surfaceflinger/SurfaceFlinger.cpp | 17 +++++++++-------- .../surfaceflinger/TransactionCallbackInvoker.h | 3 ++- 9 files changed, 53 insertions(+), 22 deletions(-) (limited to 'libs/gui/BLASTBufferQueue.cpp') diff --git a/libs/gui/BLASTBufferQueue.cpp b/libs/gui/BLASTBufferQueue.cpp index 8b9a878598..7fcb8e8b50 100644 --- a/libs/gui/BLASTBufferQueue.cpp +++ b/libs/gui/BLASTBufferQueue.cpp @@ -334,9 +334,11 @@ void BLASTBufferQueue::transactionCallback(nsecs_t /*latchTime*/, const sp statsOptional = findMatchingStat(stats, pendingSC); if (statsOptional) { SurfaceControlStats stat = *statsOptional; - mTransformHint = stat.transformHint; - mBufferItemConsumer->setTransformHint(mTransformHint); - BQA_LOGV("updated mTransformHint=%d", mTransformHint); + if (stat.transformHint) { + mTransformHint = *stat.transformHint; + mBufferItemConsumer->setTransformHint(mTransformHint); + BQA_LOGV("updated mTransformHint=%d", mTransformHint); + } // Update frametime stamps if the frame was latched and presented, indicated by a // valid latch time. if (stat.latchTime > 0) { diff --git a/libs/gui/ITransactionCompletedListener.cpp b/libs/gui/ITransactionCompletedListener.cpp index e4b8bad8f8..712aefde35 100644 --- a/libs/gui/ITransactionCompletedListener.cpp +++ b/libs/gui/ITransactionCompletedListener.cpp @@ -17,6 +17,9 @@ #define LOG_TAG "ITransactionCompletedListener" //#define LOG_NDEBUG 0 +#include +#include + #include #include #include @@ -126,7 +129,12 @@ status_t SurfaceStats::writeToParcel(Parcel* output) const { } else { SAFE_PARCEL(output->writeBool, false); } - SAFE_PARCEL(output->writeUint32, transformHint); + + SAFE_PARCEL(output->writeBool, transformHint.has_value()); + if (transformHint.has_value()) { + output->writeUint32(transformHint.value()); + } + SAFE_PARCEL(output->writeUint32, currentMaxAcquiredBufferCount); SAFE_PARCEL(output->writeParcelable, eventStats); SAFE_PARCEL(output->writeInt32, static_cast(jankData.size())); @@ -156,7 +164,16 @@ status_t SurfaceStats::readFromParcel(const Parcel* input) { previousReleaseFence = new Fence(); SAFE_PARCEL(input->read, *previousReleaseFence); } - SAFE_PARCEL(input->readUint32, &transformHint); + bool hasTransformHint = false; + SAFE_PARCEL(input->readBool, &hasTransformHint); + if (hasTransformHint) { + uint32_t tempTransformHint; + SAFE_PARCEL(input->readUint32, &tempTransformHint); + transformHint = std::make_optional(tempTransformHint); + } else { + transformHint = std::nullopt; + } + SAFE_PARCEL(input->readUint32, ¤tMaxAcquiredBufferCount); SAFE_PARCEL(input->readParcelable, &eventStats); diff --git a/libs/gui/SurfaceComposerClient.cpp b/libs/gui/SurfaceComposerClient.cpp index 81b4d3d85f..a957059220 100644 --- a/libs/gui/SurfaceComposerClient.cpp +++ b/libs/gui/SurfaceComposerClient.cpp @@ -385,10 +385,11 @@ void TransactionCompletedListener::onTransactionCompleted(ListenerStats listener surfaceStats.previousReleaseFence, surfaceStats.transformHint, surfaceStats.eventStats, surfaceStats.currentMaxAcquiredBufferCount); - if (callbacksMap[callbackId].surfaceControls[surfaceStats.surfaceControl]) { + if (callbacksMap[callbackId].surfaceControls[surfaceStats.surfaceControl] && + surfaceStats.transformHint.has_value()) { callbacksMap[callbackId] .surfaceControls[surfaceStats.surfaceControl] - ->setTransformHint(surfaceStats.transformHint); + ->setTransformHint(*surfaceStats.transformHint); } // If there is buffer id set, we look up any pending client release buffer callbacks // and call them. This is a performance optimization when we have a transaction diff --git a/libs/gui/include/gui/ITransactionCompletedListener.h b/libs/gui/include/gui/ITransactionCompletedListener.h index cc136bb40a..162d3d36ab 100644 --- a/libs/gui/include/gui/ITransactionCompletedListener.h +++ b/libs/gui/include/gui/ITransactionCompletedListener.h @@ -132,7 +132,7 @@ public: SurfaceStats() = default; SurfaceStats(const sp& sc, std::variant> acquireTimeOrFence, - const sp& prevReleaseFence, uint32_t hint, + const sp& prevReleaseFence, std::optional hint, uint32_t currentMaxAcquiredBuffersCount, FrameEventHistoryStats frameEventStats, std::vector jankData, ReleaseCallbackId previousReleaseCallbackId) : surfaceControl(sc), @@ -147,7 +147,7 @@ public: sp surfaceControl; std::variant> acquireTimeOrFence = -1; sp previousReleaseFence; - uint32_t transformHint = 0; + std::optional transformHint = 0; uint32_t currentMaxAcquiredBufferCount = 0; FrameEventHistoryStats eventStats; std::vector jankData; diff --git a/libs/gui/include/gui/SurfaceComposerClient.h b/libs/gui/include/gui/SurfaceComposerClient.h index d138d6832b..36dc3b773b 100644 --- a/libs/gui/include/gui/SurfaceComposerClient.h +++ b/libs/gui/include/gui/SurfaceComposerClient.h @@ -69,7 +69,7 @@ struct SurfaceControlStats { SurfaceControlStats(const sp& sc, nsecs_t latchTime, std::variant> acquireTimeOrFence, const sp& presentFence, const sp& prevReleaseFence, - uint32_t hint, FrameEventHistoryStats eventStats, + std::optional hint, FrameEventHistoryStats eventStats, uint32_t currentMaxAcquiredBufferCount) : surfaceControl(sc), latchTime(latchTime), @@ -85,7 +85,7 @@ struct SurfaceControlStats { std::variant> acquireTimeOrFence = -1; sp presentFence; sp previousReleaseFence; - uint32_t transformHint = 0; + std::optional transformHint = 0; FrameEventHistoryStats frameEventStats; uint32_t currentMaxAcquiredBufferCount = 0; }; diff --git a/services/surfaceflinger/Layer.cpp b/services/surfaceflinger/Layer.cpp index 410e43846d..ca330e46c7 100644 --- a/services/surfaceflinger/Layer.cpp +++ b/services/surfaceflinger/Layer.cpp @@ -63,6 +63,7 @@ #include #include +#include #include #include "DisplayDevice.h" @@ -1613,6 +1614,10 @@ uint32_t Layer::getEffectiveUsage(uint32_t usage) const { return usage; } +void Layer::skipReportingTransformHint() { + mSkipReportingTransformHint = true; +} + void Layer::updateTransformHint(ui::Transform::RotationFlags transformHint) { if (mFlinger->mDebugDisableTransformHint || transformHint & ui::Transform::ROT_INVALID) { transformHint = ui::Transform::ROT_0; @@ -2988,7 +2993,9 @@ void Layer::onSurfaceFrameCreated( void Layer::releasePendingBuffer(nsecs_t dequeueReadyTime) { for (const auto& handle : mDrawingState.callbackHandles) { - handle->transformHint = mTransformHint; + handle->transformHint = mSkipReportingTransformHint + ? std::nullopt + : std::make_optional(mTransformHint); handle->dequeueReadyTime = dequeueReadyTime; handle->currentMaxAcquiredBufferCount = mFlinger->getMaxAcquiredBufferCountForCurrentRefreshRate(mOwnerUid); @@ -4170,6 +4177,7 @@ void Layer::setTransformHint(ui::Transform::RotationFlags displayTransformHint) if (mTransformHint == ui::Transform::ROT_INVALID) { mTransformHint = displayTransformHint; } + mSkipReportingTransformHint = false; } const std::shared_ptr& Layer::getExternalTexture() const { diff --git a/services/surfaceflinger/Layer.h b/services/surfaceflinger/Layer.h index 8ace8123f9..e5fa83156f 100644 --- a/services/surfaceflinger/Layer.h +++ b/services/surfaceflinger/Layer.h @@ -723,7 +723,7 @@ public: * Sets display transform hint on BufferLayerConsumer. */ void updateTransformHint(ui::Transform::RotationFlags); - + void skipReportingTransformHint(); inline const State& getDrawingState() const { return mDrawingState; } inline State& getDrawingState() { return mDrawingState; } @@ -1205,6 +1205,7 @@ private: // Transform hint provided to the producer. This must be accessed holding // the mStateLock. ui::Transform::RotationFlags mTransformHint = ui::Transform::ROT_0; + bool mSkipReportingTransformHint = true; ReleaseCallbackId mPreviousReleaseCallbackId = ReleaseCallbackId::INVALID_ID; uint64_t mPreviousReleasedFrameNumber = 0; diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp index 6167378614..62eca56a64 100644 --- a/services/surfaceflinger/SurfaceFlinger.cpp +++ b/services/surfaceflinger/SurfaceFlinger.cpp @@ -3117,20 +3117,21 @@ void SurfaceFlinger::commitTransactionsLocked(uint32_t transactionFlags) { } } - if (!hintDisplay && mDisplays.size() > 0) { + if (!hintDisplay) { // NOTE: TEMPORARY FIX ONLY. Real fix should cause layers to // redraw after transform hint changes. See bug 8508397. - // could be null when this layer is using a layerStack // that is not visible on any display. Also can occur at // screen off/on times. - hintDisplay = getDefaultDisplayDeviceLocked(); - } - - if (hintDisplay) { - layer->updateTransformHint(hintDisplay->getTransformHint()); + // U Update: Don't provide stale hints to the clients. For + // special cases where we want the app to draw its + // first frame before the display is available, we rely + // on WMS and DMS to provide the right information + // so the client can calculate the hint. + ALOGV("Skipping reporting transform hint update for %s", layer->getDebugName()); + layer->skipReportingTransformHint(); } else { - ALOGW("Ignoring transform hint update for %s", layer->getDebugName()); + layer->updateTransformHint(hintDisplay->getTransformHint()); } }); } diff --git a/services/surfaceflinger/TransactionCallbackInvoker.h b/services/surfaceflinger/TransactionCallbackInvoker.h index 23ea7a551a..61ff9bce98 100644 --- a/services/surfaceflinger/TransactionCallbackInvoker.h +++ b/services/surfaceflinger/TransactionCallbackInvoker.h @@ -19,6 +19,7 @@ #include #include #include +#include #include #include #include @@ -48,7 +49,7 @@ public: std::vector> previousReleaseFences; std::variant> acquireTimeOrFence = -1; nsecs_t latchTime = -1; - uint32_t transformHint = 0; + std::optional transformHint = std::nullopt; uint32_t currentMaxAcquiredBufferCount = 0; std::shared_ptr gpuCompositionDoneFence{FenceTime::NO_FENCE}; CompositorTiming compositorTiming; -- cgit v1.2.3-59-g8ed1b From f1e5df1d266f70a508c7b520fd52feced8fbcf61 Mon Sep 17 00:00:00 2001 From: Patrick Williams Date: Mon, 17 Oct 2022 21:37:42 +0000 Subject: SF: Trigger ANR when buffer cache is full * Updates the transaction queue stall listener to take a string that contains the reason for hanging. * Updates ClientCache::add to indicate whether or not a failure is due to the cache being full * Calls the transaction queue stall listener when the ClientCache is full Bug: 244218818 Test: presubmits Change-Id: I5fdc9aef0f0a1601ace1c42cfac5024c3de8d299 --- libs/gui/BLASTBufferQueue.cpp | 20 +++--- libs/gui/ITransactionCompletedListener.cpp | 16 +++-- libs/gui/SurfaceComposerClient.cpp | 29 ++++---- libs/gui/include/gui/BLASTBufferQueue.h | 10 ++- .../include/gui/ITransactionCompletedListener.h | 3 +- libs/gui/include/gui/SurfaceComposerClient.h | 6 +- services/surfaceflinger/ClientCache.cpp | 23 +++--- services/surfaceflinger/ClientCache.h | 5 +- .../surfaceflinger/FrontEnd/TransactionHandler.cpp | 13 ++-- .../surfaceflinger/FrontEnd/TransactionHandler.h | 3 +- services/surfaceflinger/SurfaceFlinger.cpp | 82 +++++++++++++--------- services/surfaceflinger/SurfaceFlinger.h | 5 +- .../Tracing/tools/LayerTraceGenerator.cpp | 3 +- 13 files changed, 123 insertions(+), 95 deletions(-) (limited to 'libs/gui/BLASTBufferQueue.cpp') diff --git a/libs/gui/BLASTBufferQueue.cpp b/libs/gui/BLASTBufferQueue.cpp index 8b9a878598..fc8aff87af 100644 --- a/libs/gui/BLASTBufferQueue.cpp +++ b/libs/gui/BLASTBufferQueue.cpp @@ -167,14 +167,15 @@ BLASTBufferQueue::BLASTBufferQueue(const std::string& name, bool updateDestinati mNumFrameAvailable = 0; TransactionCompletedListener::getInstance()->addQueueStallListener( - [&]() { - std::function callbackCopy; - { - std::unique_lock _lock{mMutex}; - callbackCopy = mTransactionHangCallback; - } - if (callbackCopy) callbackCopy(true); - }, this); + [&](const std::string& reason) { + std::function callbackCopy; + { + std::unique_lock _lock{mMutex}; + callbackCopy = mTransactionHangCallback; + } + if (callbackCopy) callbackCopy(reason); + }, + this); BQA_LOGV("BLASTBufferQueue created"); } @@ -1112,7 +1113,8 @@ bool BLASTBufferQueue::isSameSurfaceControl(const sp& surfaceCon return SurfaceControl::isSameSurface(mSurfaceControl, surfaceControl); } -void BLASTBufferQueue::setTransactionHangCallback(std::function callback) { +void BLASTBufferQueue::setTransactionHangCallback( + std::function callback) { std::unique_lock _lock{mMutex}; mTransactionHangCallback = callback; } diff --git a/libs/gui/ITransactionCompletedListener.cpp b/libs/gui/ITransactionCompletedListener.cpp index ca91afa35d..2a114c82ee 100644 --- a/libs/gui/ITransactionCompletedListener.cpp +++ b/libs/gui/ITransactionCompletedListener.cpp @@ -273,15 +273,17 @@ public: void onReleaseBuffer(ReleaseCallbackId callbackId, sp releaseFence, uint32_t currentMaxAcquiredBufferCount) override { - callRemoteAsync(Tag::ON_RELEASE_BUFFER, - callbackId, releaseFence, - currentMaxAcquiredBufferCount); + callRemoteAsync(Tag::ON_RELEASE_BUFFER, callbackId, + releaseFence, + currentMaxAcquiredBufferCount); } - void onTransactionQueueStalled() override { - callRemoteAsync( - Tag::ON_TRANSACTION_QUEUE_STALLED); + void onTransactionQueueStalled(const String8& reason) override { + callRemoteAsync< + decltype(&ITransactionCompletedListener:: + onTransactionQueueStalled)>(Tag::ON_TRANSACTION_QUEUE_STALLED, + reason); } }; diff --git a/libs/gui/SurfaceComposerClient.cpp b/libs/gui/SurfaceComposerClient.cpp index a5879a77e1..9efdd35a52 100644 --- a/libs/gui/SurfaceComposerClient.cpp +++ b/libs/gui/SurfaceComposerClient.cpp @@ -455,23 +455,24 @@ void TransactionCompletedListener::onTransactionCompleted(ListenerStats listener } } -void TransactionCompletedListener::onTransactionQueueStalled() { - std::unordered_map> callbackCopy; - { - std::scoped_lock lock(mMutex); - callbackCopy = mQueueStallListeners; - } - for (auto const& it : callbackCopy) { - it.second(); - } -} - -void TransactionCompletedListener::addQueueStallListener(std::function stallListener, - void* id) { +void TransactionCompletedListener::onTransactionQueueStalled(const String8& reason) { + std::unordered_map> callbackCopy; + { + std::scoped_lock lock(mMutex); + callbackCopy = mQueueStallListeners; + } + for (auto const& it : callbackCopy) { + it.second(reason.c_str()); + } +} + +void TransactionCompletedListener::addQueueStallListener( + std::function stallListener, void* id) { std::scoped_lock lock(mMutex); mQueueStallListeners[id] = stallListener; } -void TransactionCompletedListener::removeQueueStallListener(void *id) { + +void TransactionCompletedListener::removeQueueStallListener(void* id) { std::scoped_lock lock(mMutex); mQueueStallListeners.erase(id); } diff --git a/libs/gui/include/gui/BLASTBufferQueue.h b/libs/gui/include/gui/BLASTBufferQueue.h index 827a6cc5a9..957652e1f1 100644 --- a/libs/gui/include/gui/BLASTBufferQueue.h +++ b/libs/gui/include/gui/BLASTBufferQueue.h @@ -113,12 +113,10 @@ public: uint64_t getLastAcquiredFrameNum(); /** - * Set a callback to be invoked when we are hung. The boolean parameter - * indicates whether the hang is due to an unfired fence. - * TODO: The boolean is always true atm, unfired fence is - * the only case we detect. + * Set a callback to be invoked when we are hung. The string parameter + * indicates the reason for the hang. */ - void setTransactionHangCallback(std::function callback); + void setTransactionHangCallback(std::function callback); virtual ~BLASTBufferQueue(); @@ -282,7 +280,7 @@ private: bool mAppliedLastTransaction = false; uint64_t mLastAppliedFrameNumber = 0; - std::function mTransactionHangCallback; + std::function mTransactionHangCallback; std::unordered_set mSyncedFrameNumbers GUARDED_BY(mMutex); }; diff --git a/libs/gui/include/gui/ITransactionCompletedListener.h b/libs/gui/include/gui/ITransactionCompletedListener.h index cc136bb40a..d2a6f40cea 100644 --- a/libs/gui/include/gui/ITransactionCompletedListener.h +++ b/libs/gui/include/gui/ITransactionCompletedListener.h @@ -194,7 +194,8 @@ public: virtual void onReleaseBuffer(ReleaseCallbackId callbackId, sp releaseFence, uint32_t currentMaxAcquiredBufferCount) = 0; - virtual void onTransactionQueueStalled() = 0; + + virtual void onTransactionQueueStalled(const String8& name) = 0; }; class BnTransactionCompletedListener : public SafeBnInterface { diff --git a/libs/gui/include/gui/SurfaceComposerClient.h b/libs/gui/include/gui/SurfaceComposerClient.h index 36969db576..9efde6cb35 100644 --- a/libs/gui/include/gui/SurfaceComposerClient.h +++ b/libs/gui/include/gui/SurfaceComposerClient.h @@ -786,7 +786,7 @@ protected: // This is protected by mSurfaceStatsListenerMutex, but GUARDED_BY isn't supported for // std::recursive_mutex std::multimap mSurfaceStatsListeners; - std::unordered_map> mQueueStallListeners; + std::unordered_map> mQueueStallListeners; public: static sp getInstance(); @@ -804,7 +804,7 @@ public: const sp& surfaceControl, const std::unordered_set& callbackIds); - void addQueueStallListener(std::function stallListener, void* id); + void addQueueStallListener(std::function stallListener, void* id); void removeQueueStallListener(void *id); /* @@ -835,7 +835,7 @@ public: // For Testing Only static void setInstance(const sp&); - void onTransactionQueueStalled() override; + void onTransactionQueueStalled(const String8& reason) override; private: ReleaseBufferCallback popReleaseBufferCallbackLocked(const ReleaseCallbackId&); diff --git a/services/surfaceflinger/ClientCache.cpp b/services/surfaceflinger/ClientCache.cpp index b01932e413..2bd8f324e1 100644 --- a/services/surfaceflinger/ClientCache.cpp +++ b/services/surfaceflinger/ClientCache.cpp @@ -59,16 +59,17 @@ bool ClientCache::getBuffer(const client_cache_t& cacheId, return true; } -bool ClientCache::add(const client_cache_t& cacheId, const sp& buffer) { +base::expected, ClientCache::AddError> +ClientCache::add(const client_cache_t& cacheId, const sp& buffer) { auto& [processToken, id] = cacheId; if (processToken == nullptr) { ALOGE_AND_TRACE("ClientCache::add - invalid (nullptr) process token"); - return false; + return base::unexpected(AddError::Unspecified); } if (!buffer) { ALOGE_AND_TRACE("ClientCache::add - invalid (nullptr) buffer"); - return false; + return base::unexpected(AddError::Unspecified); } std::lock_guard lock(mMutex); @@ -81,7 +82,7 @@ bool ClientCache::add(const client_cache_t& cacheId, const sp& bu token = processToken.promote(); if (!token) { ALOGE_AND_TRACE("ClientCache::add - invalid token"); - return false; + return base::unexpected(AddError::Unspecified); } // Only call linkToDeath if not a local binder @@ -89,7 +90,7 @@ bool ClientCache::add(const client_cache_t& cacheId, const sp& bu status_t err = token->linkToDeath(mDeathRecipient); if (err != NO_ERROR) { ALOGE_AND_TRACE("ClientCache::add - could not link to death"); - return false; + return base::unexpected(AddError::Unspecified); } } auto [itr, success] = @@ -104,17 +105,17 @@ bool ClientCache::add(const client_cache_t& cacheId, const sp& bu if (processBuffers.size() > BUFFER_CACHE_MAX_SIZE) { ALOGE_AND_TRACE("ClientCache::add - cache is full"); - return false; + return base::unexpected(AddError::CacheFull); } LOG_ALWAYS_FATAL_IF(mRenderEngine == nullptr, "Attempted to build the ClientCache before a RenderEngine instance was " "ready!"); - processBuffers[id].buffer = std::make_shared< - renderengine::impl::ExternalTexture>(buffer, *mRenderEngine, - renderengine::impl::ExternalTexture::Usage:: - READABLE); - return true; + + return (processBuffers[id].buffer = std::make_shared< + renderengine::impl::ExternalTexture>(buffer, *mRenderEngine, + renderengine::impl::ExternalTexture:: + Usage::READABLE)); } void ClientCache::erase(const client_cache_t& cacheId) { diff --git a/services/surfaceflinger/ClientCache.h b/services/surfaceflinger/ClientCache.h index a9b8177d70..cdeac2bbc1 100644 --- a/services/surfaceflinger/ClientCache.h +++ b/services/surfaceflinger/ClientCache.h @@ -37,7 +37,10 @@ class ClientCache : public Singleton { public: ClientCache(); - bool add(const client_cache_t& cacheId, const sp& buffer); + enum class AddError { CacheFull, Unspecified }; + + base::expected, AddError> add( + const client_cache_t& cacheId, const sp& buffer); void erase(const client_cache_t& cacheId); std::shared_ptr get(const client_cache_t& cacheId); diff --git a/services/surfaceflinger/FrontEnd/TransactionHandler.cpp b/services/surfaceflinger/FrontEnd/TransactionHandler.cpp index 6c6a487b67..95961fe6a9 100644 --- a/services/surfaceflinger/FrontEnd/TransactionHandler.cpp +++ b/services/surfaceflinger/FrontEnd/TransactionHandler.cpp @@ -168,15 +168,16 @@ bool TransactionHandler::hasPendingTransactions() { return !mPendingTransactionQueues.empty() || !mLocklessTransactionQueue.isEmpty(); } -void TransactionHandler::onTransactionQueueStalled(const TransactionState& transaction, - sp& listener) { - if (std::find(mStalledTransactions.begin(), mStalledTransactions.end(), transaction.id) != +void TransactionHandler::onTransactionQueueStalled(uint64_t transactionId, + sp& listener, + const std::string& reason) { + if (std::find(mStalledTransactions.begin(), mStalledTransactions.end(), transactionId) != mStalledTransactions.end()) { return; } - mStalledTransactions.push_back(transaction.id); - listener->onTransactionQueueStalled(); + mStalledTransactions.push_back(transactionId); + listener->onTransactionQueueStalled(String8(reason.c_str())); } void TransactionHandler::removeFromStalledTransactions(uint64_t id) { @@ -185,4 +186,4 @@ void TransactionHandler::removeFromStalledTransactions(uint64_t id) { mStalledTransactions.erase(it); } } -} // namespace android \ No newline at end of file +} // namespace android diff --git a/services/surfaceflinger/FrontEnd/TransactionHandler.h b/services/surfaceflinger/FrontEnd/TransactionHandler.h index 237b48d55f..2b6f07d49a 100644 --- a/services/surfaceflinger/FrontEnd/TransactionHandler.h +++ b/services/surfaceflinger/FrontEnd/TransactionHandler.h @@ -54,7 +54,8 @@ public: std::vector flushTransactions(); void addTransactionReadyFilter(TransactionFilter&&); void queueTransaction(TransactionState&&); - void onTransactionQueueStalled(const TransactionState&, sp&); + void onTransactionQueueStalled(uint64_t transactionId, sp&, + const std::string& reason); void removeFromStalledTransactions(uint64_t transactionId); private: diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp index 100ad43f32..d8491592d0 100644 --- a/services/surfaceflinger/SurfaceFlinger.cpp +++ b/services/surfaceflinger/SurfaceFlinger.cpp @@ -3744,7 +3744,10 @@ TransactionHandler::TransactionReadiness SurfaceFlinger::transactionReadyBufferC if (listener && (flushState.queueProcessTime - transaction.postTime) > std::chrono::nanoseconds(4s).count()) { - mTransactionHandler.onTransactionQueueStalled(transaction, listener); + mTransactionHandler + .onTransactionQueueStalled(transaction.id, listener, + "Buffer processing hung up due to stuck " + "fence. Indicates GPU hang"); } return false; } @@ -3960,8 +3963,9 @@ bool SurfaceFlinger::applyTransactionState(const FrameTimelineInfo& frameTimelin uint32_t clientStateFlags = 0; for (int i = 0; i < states.size(); i++) { ComposerState& state = states.editItemAt(i); - clientStateFlags |= setClientStateLocked(frameTimelineInfo, state, desiredPresentTime, - isAutoTimestamp, postTime, permissions); + clientStateFlags |= + setClientStateLocked(frameTimelineInfo, state, desiredPresentTime, isAutoTimestamp, + postTime, permissions, transactionId); if ((flags & eAnimation) && state.state.surface) { if (const auto layer = fromHandle(state.state.surface).promote()) { using LayerUpdateType = scheduler::LayerHistory::LayerUpdateType; @@ -4077,7 +4081,8 @@ bool SurfaceFlinger::callingThreadHasUnscopedSurfaceFlingerAccess(bool usePermis uint32_t SurfaceFlinger::setClientStateLocked(const FrameTimelineInfo& frameTimelineInfo, ComposerState& composerState, int64_t desiredPresentTime, bool isAutoTimestamp, - int64_t postTime, uint32_t permissions) { + int64_t postTime, uint32_t permissions, + uint64_t transactionId) { layer_state_t& s = composerState.state; s.sanitize(permissions); @@ -4370,7 +4375,8 @@ uint32_t SurfaceFlinger::setClientStateLocked(const FrameTimelineInfo& frameTime if (what & layer_state_t::eBufferChanged) { std::shared_ptr buffer = - getExternalTextureFromBufferData(*s.bufferData, layer->getDebugName()); + getExternalTextureFromBufferData(*s.bufferData, layer->getDebugName(), + transactionId); if (layer->setBuffer(buffer, *s.bufferData, postTime, desiredPresentTime, isAutoTimestamp, dequeueBufferTimestamp, frameTimelineInfo)) { flags |= eTraversalNeeded; @@ -6955,34 +6961,44 @@ status_t SurfaceFlinger::removeWindowInfosListener( } std::shared_ptr SurfaceFlinger::getExternalTextureFromBufferData( - const BufferData& bufferData, const char* layerName) const { - bool cacheIdChanged = bufferData.flags.test(BufferData::BufferDataChange::cachedBufferChanged); - bool bufferSizeExceedsLimit = false; - std::shared_ptr buffer = nullptr; - if (cacheIdChanged && bufferData.buffer != nullptr) { - bufferSizeExceedsLimit = exceedsMaxRenderTargetSize(bufferData.buffer->getWidth(), - bufferData.buffer->getHeight()); - if (!bufferSizeExceedsLimit) { - ClientCache::getInstance().add(bufferData.cachedBuffer, bufferData.buffer); - buffer = ClientCache::getInstance().get(bufferData.cachedBuffer); - } - } else if (cacheIdChanged) { - buffer = ClientCache::getInstance().get(bufferData.cachedBuffer); - } else if (bufferData.buffer != nullptr) { - bufferSizeExceedsLimit = exceedsMaxRenderTargetSize(bufferData.buffer->getWidth(), - bufferData.buffer->getHeight()); - if (!bufferSizeExceedsLimit) { - buffer = std::make_shared< - renderengine::impl::ExternalTexture>(bufferData.buffer, getRenderEngine(), - renderengine::impl::ExternalTexture:: - Usage::READABLE); - } - } - ALOGE_IF(bufferSizeExceedsLimit, - "Attempted to create an ExternalTexture for layer %s that exceeds render target size " - "limit.", - layerName); - return buffer; + BufferData& bufferData, const char* layerName, uint64_t transactionId) { + if (bufferData.buffer && + exceedsMaxRenderTargetSize(bufferData.buffer->getWidth(), bufferData.buffer->getHeight())) { + ALOGE("Attempted to create an ExternalTexture for layer %s that exceeds render target " + "size limit.", + layerName); + return nullptr; + } + + bool cachedBufferChanged = + bufferData.flags.test(BufferData::BufferDataChange::cachedBufferChanged); + if (cachedBufferChanged && bufferData.buffer) { + auto result = ClientCache::getInstance().add(bufferData.cachedBuffer, bufferData.buffer); + if (result.ok()) { + return result.value(); + } + + if (result.error() == ClientCache::AddError::CacheFull) { + mTransactionHandler + .onTransactionQueueStalled(transactionId, bufferData.releaseBufferListener, + "Buffer processing hung due to full buffer cache"); + } + + return nullptr; + } + + if (cachedBufferChanged) { + return ClientCache::getInstance().get(bufferData.cachedBuffer); + } + + if (bufferData.buffer) { + return std::make_shared< + renderengine::impl::ExternalTexture>(bufferData.buffer, getRenderEngine(), + renderengine::impl::ExternalTexture::Usage:: + READABLE); + } + + return nullptr; } bool SurfaceFlinger::commitMirrorDisplays(VsyncId vsyncId) { diff --git a/services/surfaceflinger/SurfaceFlinger.h b/services/surfaceflinger/SurfaceFlinger.h index 674f21f77e..3a14ba0493 100644 --- a/services/surfaceflinger/SurfaceFlinger.h +++ b/services/surfaceflinger/SurfaceFlinger.h @@ -312,7 +312,7 @@ protected: REQUIRES(mStateLock); virtual std::shared_ptr getExternalTextureFromBufferData( - const BufferData& bufferData, const char* layerName) const; + BufferData& bufferData, const char* layerName, uint64_t transactionId); // Returns true if any display matches a `bool(const DisplayDevice&)` predicate. template @@ -728,7 +728,8 @@ private: uint32_t setClientStateLocked(const FrameTimelineInfo&, ComposerState&, int64_t desiredPresentTime, bool isAutoTimestamp, - int64_t postTime, uint32_t permissions) REQUIRES(mStateLock); + int64_t postTime, uint32_t permissions, uint64_t transactionId) + REQUIRES(mStateLock); uint32_t getTransactionFlags() const; diff --git a/services/surfaceflinger/Tracing/tools/LayerTraceGenerator.cpp b/services/surfaceflinger/Tracing/tools/LayerTraceGenerator.cpp index 276b3dad18..3b11f2383e 100644 --- a/services/surfaceflinger/Tracing/tools/LayerTraceGenerator.cpp +++ b/services/surfaceflinger/Tracing/tools/LayerTraceGenerator.cpp @@ -100,7 +100,8 @@ public: MockSurfaceFlinger(Factory& factory) : SurfaceFlinger(factory, SurfaceFlinger::SkipInitialization) {} std::shared_ptr getExternalTextureFromBufferData( - const BufferData& bufferData, const char* /* layerName */) const override { + BufferData& bufferData, const char* /* layerName */, + uint64_t /* transactionId */) override { return std::make_shared(bufferData.getWidth(), bufferData.getHeight(), bufferData.getId(), -- cgit v1.2.3-59-g8ed1b From 28fe2e694f5cafaf0eaad1417e15fdee2943b15b Mon Sep 17 00:00:00 2001 From: Vishnu Nair Date: Tue, 1 Nov 2022 14:29:10 -0700 Subject: BBQ: Check if the buffer is already in the pending release queue before logging As a workaround for lost release callbacks, we try to release buffers in the submitted queue. If the buffer was released previously but held in the pending release queue, we would log incorrectly. This fixes the misleading logs. Test: presubmit Test: logcat Fixes: 255679881 Change-Id: I7e46f21f4c4fa1ee8c70e3ee8cd3f3665fe7442a --- libs/gui/BLASTBufferQueue.cpp | 22 +++++++++++++++------- libs/gui/include/gui/BLASTBufferQueue.h | 3 ++- 2 files changed, 17 insertions(+), 8 deletions(-) (limited to 'libs/gui/BLASTBufferQueue.cpp') diff --git a/libs/gui/BLASTBufferQueue.cpp b/libs/gui/BLASTBufferQueue.cpp index 0021bd6cc3..97e45c6d47 100644 --- a/libs/gui/BLASTBufferQueue.cpp +++ b/libs/gui/BLASTBufferQueue.cpp @@ -360,11 +360,12 @@ void BLASTBufferQueue::transactionCallback(nsecs_t /*latchTime*/, const sp& releaseFence, std::optional currentMaxAcquiredBufferCount) { +void BLASTBufferQueue::releaseBufferCallbackLocked( + const ReleaseCallbackId& id, const sp& releaseFence, + std::optional currentMaxAcquiredBufferCount, bool fakeRelease) { ATRACE_CALL(); BQA_LOGV("releaseBufferCallback %s", id.to_string().c_str()); @@ -435,6 +438,11 @@ void BLASTBufferQueue::releaseBufferCallbackLocked(const ReleaseCallbackId& id, auto rb = ReleasedBuffer{id, releaseFence}; if (std::find(mPendingRelease.begin(), mPendingRelease.end(), rb) == mPendingRelease.end()) { mPendingRelease.emplace_back(rb); + if (fakeRelease) { + BQA_LOGE("Faking releaseBufferCallback from transactionCompleteCallback %" PRIu64, + id.framenumber); + BBQ_TRACE("FakeReleaseCallback"); + } } // Release all buffers that are beyond the ones that we need to hold diff --git a/libs/gui/include/gui/BLASTBufferQueue.h b/libs/gui/include/gui/BLASTBufferQueue.h index 957652e1f1..47dcc42e16 100644 --- a/libs/gui/include/gui/BLASTBufferQueue.h +++ b/libs/gui/include/gui/BLASTBufferQueue.h @@ -93,7 +93,8 @@ public: void releaseBufferCallback(const ReleaseCallbackId& id, const sp& releaseFence, std::optional currentMaxAcquiredBufferCount); void releaseBufferCallbackLocked(const ReleaseCallbackId& id, const sp& releaseFence, - std::optional currentMaxAcquiredBufferCount); + std::optional currentMaxAcquiredBufferCount, + bool fakeRelease); void syncNextTransaction(std::function callback, bool acquireSingleBuffer = true); void stopContinuousSyncTransaction(); -- cgit v1.2.3-59-g8ed1b From 3a8f19bda6d380dea2a83b292e4f8efd8fb588f6 Mon Sep 17 00:00:00 2001 From: Chavi Weingarten Date: Tue, 27 Dec 2022 22:00:24 +0000 Subject: [BBQ] Only wait when acquiring a buffer if in a sync. There's no need to do the explicit maxBuffersAcquired check since the acquireBuffer call will return a status of NO_BUFFER_AVAILABLE if it can't acquire any more buffers. Therefore, the code can be simplified so it will return early in most cases if NO_BUFFER_AVAILABLE is returned. If there was a request to sync the buffer, then we need to wait on a free buffer and not return early. Test: BBQ syncs Bug: 263340543 Change-Id: I11a81a440bc0d22ba85bcf4d4a068b853ca5cf9c --- libs/gui/BLASTBufferQueue.cpp | 81 +++++++++++---------------------- libs/gui/include/gui/BLASTBufferQueue.h | 4 +- 2 files changed, 28 insertions(+), 57 deletions(-) (limited to 'libs/gui/BLASTBufferQueue.cpp') diff --git a/libs/gui/BLASTBufferQueue.cpp b/libs/gui/BLASTBufferQueue.cpp index 97e45c6d47..797d6aedcc 100644 --- a/libs/gui/BLASTBufferQueue.cpp +++ b/libs/gui/BLASTBufferQueue.cpp @@ -483,20 +483,18 @@ void BLASTBufferQueue::releaseBuffer(const ReleaseCallbackId& callbackId, mSyncedFrameNumbers.erase(callbackId.framenumber); } -void BLASTBufferQueue::acquireNextBufferLocked( +status_t BLASTBufferQueue::acquireNextBufferLocked( const std::optional transaction) { // If the next transaction is set, we want to guarantee the our acquire will not fail, so don't // include the extra buffer when checking if we can acquire the next buffer. - const bool includeExtraAcquire = !transaction; - const bool maxAcquired = maxBuffersAcquired(includeExtraAcquire); - if (mNumFrameAvailable == 0 || maxAcquired) { - BQA_LOGV("Can't process next buffer maxBuffersAcquired=%s", boolToString(maxAcquired)); - return; + if (mNumFrameAvailable == 0) { + BQA_LOGV("Can't process next buffer. No available frames"); + return NOT_ENOUGH_DATA; } if (mSurfaceControl == nullptr) { BQA_LOGE("ERROR : surface control is null"); - return; + return NAME_NOT_FOUND; } SurfaceComposerClient::Transaction localTransaction; @@ -513,10 +511,10 @@ void BLASTBufferQueue::acquireNextBufferLocked( mBufferItemConsumer->acquireBuffer(&bufferItem, 0 /* expectedPresent */, false); if (status == BufferQueue::NO_BUFFER_AVAILABLE) { BQA_LOGV("Failed to acquire a buffer, err=NO_BUFFER_AVAILABLE"); - return; + return status; } else if (status != OK) { BQA_LOGE("Failed to acquire a buffer, err=%s", statusToString(status).c_str()); - return; + return status; } auto buffer = bufferItem.mGraphicBuffer; @@ -526,7 +524,7 @@ void BLASTBufferQueue::acquireNextBufferLocked( if (buffer == nullptr) { mBufferItemConsumer->releaseBuffer(bufferItem, Fence::NO_FENCE); BQA_LOGE("Buffer was empty"); - return; + return BAD_VALUE; } if (rejectBuffer(bufferItem)) { @@ -535,8 +533,7 @@ void BLASTBufferQueue::acquireNextBufferLocked( mSize.width, mSize.height, mRequestedSize.width, mRequestedSize.height, buffer->getWidth(), buffer->getHeight(), bufferItem.mTransform); mBufferItemConsumer->releaseBuffer(bufferItem, Fence::NO_FENCE); - acquireNextBufferLocked(transaction); - return; + return acquireNextBufferLocked(transaction); } mNumAcquired++; @@ -624,6 +621,7 @@ void BLASTBufferQueue::acquireNextBufferLocked( bufferItem.mTimestamp, bufferItem.mIsAutoTimestamp ? "(auto)" : "", static_cast(mPendingTransactions.size()), bufferItem.mGraphicBuffer->getId(), bufferItem.mAutoRefresh ? " mAutoRefresh" : "", bufferItem.mTransform); + return OK; } Rect BLASTBufferQueue::computeCrop(const BufferItem& item) { @@ -647,32 +645,6 @@ void BLASTBufferQueue::acquireAndReleaseBuffer() { mBufferItemConsumer->releaseBuffer(bufferItem, bufferItem.mFence); } -void BLASTBufferQueue::flushAndWaitForFreeBuffer(std::unique_lock& lock) { - BBQ_TRACE(); - if (!mSyncedFrameNumbers.empty() && mNumFrameAvailable > 0) { - // We are waiting on a previous sync's transaction callback so allow another sync - // transaction to proceed. - // - // We need to first flush out the transactions that were in between the two syncs. - // We do this by merging them into mSyncTransaction so any buffer merging will get - // a release callback invoked. The release callback will be async so we need to wait - // on max acquired to make sure we have the capacity to acquire another buffer. - if (maxBuffersAcquired(false /* includeExtraAcquire */)) { - BQA_LOGD("waiting to flush shadow queue..."); - mCallbackCV.wait(lock); - } - while (mNumFrameAvailable > 0) { - // flush out the shadow queue - acquireAndReleaseBuffer(); - } - } - - while (maxBuffersAcquired(false /* includeExtraAcquire */)) { - BQA_LOGD("waiting for free buffer."); - mCallbackCV.wait(lock); - } -} - void BLASTBufferQueue::onFrameAvailable(const BufferItem& item) { std::function prevCallback = nullptr; SurfaceComposerClient::Transaction* prevTransaction = nullptr; @@ -685,7 +657,6 @@ void BLASTBufferQueue::onFrameAvailable(const BufferItem& item) { BQA_LOGV("onFrameAvailable-start syncTransactionSet=%s", boolToString(syncTransactionSet)); if (syncTransactionSet) { - bool mayNeedToWaitForBuffer = true; // If we are going to re-use the same mSyncTransaction, release the buffer that may // already be set in the Transaction. This is to allow us a free slot early to continue // processing a new buffer. @@ -696,14 +667,20 @@ void BLASTBufferQueue::onFrameAvailable(const BufferItem& item) { bufferData->frameNumber); releaseBuffer(bufferData->generateReleaseCallbackId(), bufferData->acquireFence); - // Because we just released a buffer, we know there's no need to wait for a free - // buffer. - mayNeedToWaitForBuffer = false; } } - if (mayNeedToWaitForBuffer) { - flushAndWaitForFreeBuffer(_lock); + if (waitForTransactionCallback) { + // We are waiting on a previous sync's transaction callback so allow another sync + // transaction to proceed. + // + // We need to first flush out the transactions that were in between the two syncs. + // We do this by merging them into mSyncTransaction so any buffer merging will get + // a release callback invoked. + while (mNumFrameAvailable > 0) { + // flush out the shadow queue + acquireAndReleaseBuffer(); + } } } @@ -719,7 +696,12 @@ void BLASTBufferQueue::onFrameAvailable(const BufferItem& item) { item.mFrameNumber, boolToString(syncTransactionSet)); if (syncTransactionSet) { - acquireNextBufferLocked(mSyncTransaction); + // If there's no available buffer and we're in a sync transaction, we need to wait + // instead of returning since we guarantee a buffer will be acquired for the sync. + while (acquireNextBufferLocked(mSyncTransaction) == BufferQueue::NO_BUFFER_AVAILABLE) { + BQA_LOGD("waiting for available buffer"); + mCallbackCV.wait(_lock); + } // Only need a commit callback when syncing to ensure the buffer that's synced has been // sent to SF @@ -829,15 +811,6 @@ bool BLASTBufferQueue::rejectBuffer(const BufferItem& item) { return mSize != bufferSize; } -// Check if we have acquired the maximum number of buffers. -// Consumer can acquire an additional buffer if that buffer is not droppable. Set -// includeExtraAcquire is true to include this buffer to the count. Since this depends on the state -// of the buffer, the next acquire may return with NO_BUFFER_AVAILABLE. -bool BLASTBufferQueue::maxBuffersAcquired(bool includeExtraAcquire) const { - int maxAcquiredBuffers = mMaxAcquiredBuffers + (includeExtraAcquire ? 2 : 1); - return mNumAcquired >= maxAcquiredBuffers; -} - class BBQSurface : public Surface { private: std::mutex mMutex; diff --git a/libs/gui/include/gui/BLASTBufferQueue.h b/libs/gui/include/gui/BLASTBufferQueue.h index 47dcc42e16..001d8e5233 100644 --- a/libs/gui/include/gui/BLASTBufferQueue.h +++ b/libs/gui/include/gui/BLASTBufferQueue.h @@ -130,12 +130,11 @@ private: void createBufferQueue(sp* outProducer, sp* outConsumer); - void acquireNextBufferLocked( + status_t acquireNextBufferLocked( const std::optional transaction) REQUIRES(mMutex); Rect computeCrop(const BufferItem& item) REQUIRES(mMutex); // Return true if we need to reject the buffer based on the scaling mode and the buffer size. bool rejectBuffer(const BufferItem& item) REQUIRES(mMutex); - bool maxBuffersAcquired(bool includeExtraAcquire) const REQUIRES(mMutex); static PixelFormat convertBufferFormat(PixelFormat& format); void mergePendingTransactions(SurfaceComposerClient::Transaction* t, uint64_t frameNumber) REQUIRES(mMutex); @@ -144,7 +143,6 @@ private: void acquireAndReleaseBuffer() REQUIRES(mMutex); void releaseBuffer(const ReleaseCallbackId& callbackId, const sp& releaseFence) REQUIRES(mMutex); - void flushAndWaitForFreeBuffer(std::unique_lock& lock); std::string mName; // Represents the queued buffer count from buffer queue, -- cgit v1.2.3-59-g8ed1b From d6e409e4614b68901e6d81f036d49b162fbe9278 Mon Sep 17 00:00:00 2001 From: Ady Abraham Date: Thu, 19 Jan 2023 16:07:31 -0800 Subject: SF: fix FrameTimelineInfo association to buffers Store a mapping between a frame number and the FrameTimelineInfo and apply the correct FrameTimelineInfo on the transaction. Test: TBD Bug: 255875655 Change-Id: I4206984be8e5a91c5dc15b74688575d97fbb5357 --- libs/gui/BLASTBufferQueue.cpp | 33 +++++++++++++++++++++------ libs/gui/Surface.cpp | 6 +++-- libs/gui/fuzzer/libgui_bufferQueue_fuzzer.cpp | 2 +- libs/gui/include/gui/BLASTBufferQueue.h | 4 ++-- libs/gui/include/gui/Surface.h | 2 +- libs/nativewindow/include/system/window.h | 5 ++-- 6 files changed, 37 insertions(+), 15 deletions(-) (limited to 'libs/gui/BLASTBufferQueue.cpp') diff --git a/libs/gui/BLASTBufferQueue.cpp b/libs/gui/BLASTBufferQueue.cpp index 797d6aedcc..60603ba50a 100644 --- a/libs/gui/BLASTBufferQueue.cpp +++ b/libs/gui/BLASTBufferQueue.cpp @@ -587,9 +587,23 @@ status_t BLASTBufferQueue::acquireNextBufferLocked( t->setDesiredPresentTime(bufferItem.mTimestamp); } - if (!mNextFrameTimelineInfoQueue.empty()) { - t->setFrameTimelineInfo(mNextFrameTimelineInfoQueue.front()); - mNextFrameTimelineInfoQueue.pop(); + // Drop stale frame timeline infos + while (!mPendingFrameTimelines.empty() && + mPendingFrameTimelines.front().first < bufferItem.mFrameNumber) { + ATRACE_FORMAT_INSTANT("dropping stale frameNumber: %" PRIu64 " vsyncId: %" PRId64, + mPendingFrameTimelines.front().first, + mPendingFrameTimelines.front().second.vsyncId); + mPendingFrameTimelines.pop(); + } + + if (!mPendingFrameTimelines.empty() && + mPendingFrameTimelines.front().first == bufferItem.mFrameNumber) { + ATRACE_FORMAT_INSTANT("Transaction::setFrameTimelineInfo frameNumber: %" PRIu64 + " vsyncId: %" PRId64, + bufferItem.mFrameNumber, + mPendingFrameTimelines.front().second.vsyncId); + t->setFrameTimelineInfo(mPendingFrameTimelines.front().second); + mPendingFrameTimelines.pop(); } { @@ -653,6 +667,7 @@ void BLASTBufferQueue::onFrameAvailable(const BufferItem& item) { { std::unique_lock _lock{mMutex}; BBQ_TRACE(); + const bool syncTransactionSet = mTransactionReadyCallback != nullptr; BQA_LOGV("onFrameAvailable-start syncTransactionSet=%s", boolToString(syncTransactionSet)); @@ -847,12 +862,13 @@ public: return mBbq->setFrameRate(frameRate, compatibility, changeFrameRateStrategy); } - status_t setFrameTimelineInfo(const FrameTimelineInfo& frameTimelineInfo) override { + status_t setFrameTimelineInfo(uint64_t frameNumber, + const FrameTimelineInfo& frameTimelineInfo) override { std::unique_lock _lock{mMutex}; if (mDestroyed) { return DEAD_OBJECT; } - return mBbq->setFrameTimelineInfo(frameTimelineInfo); + return mBbq->setFrameTimelineInfo(frameNumber, frameTimelineInfo); } void destroy() override { @@ -874,9 +890,12 @@ status_t BLASTBufferQueue::setFrameRate(float frameRate, int8_t compatibility, return t.setFrameRate(mSurfaceControl, frameRate, compatibility, shouldBeSeamless).apply(); } -status_t BLASTBufferQueue::setFrameTimelineInfo(const FrameTimelineInfo& frameTimelineInfo) { +status_t BLASTBufferQueue::setFrameTimelineInfo(uint64_t frameNumber, + const FrameTimelineInfo& frameTimelineInfo) { + ATRACE_FORMAT("%s(%s) frameNumber: %" PRIu64 " vsyncId: %" PRId64, __func__, mName.c_str(), + frameNumber, frameTimelineInfo.vsyncId); std::unique_lock _lock{mMutex}; - mNextFrameTimelineInfoQueue.push(frameTimelineInfo); + mPendingFrameTimelines.push({frameNumber, frameTimelineInfo}); return OK; } diff --git a/libs/gui/Surface.cpp b/libs/gui/Surface.cpp index edb18a86ee..b18bf5bdee 100644 --- a/libs/gui/Surface.cpp +++ b/libs/gui/Surface.cpp @@ -1863,6 +1863,7 @@ int Surface::dispatchGetLastQueuedBuffer2(va_list args) { int Surface::dispatchSetFrameTimelineInfo(va_list args) { ATRACE_CALL(); + auto frameNumber = static_cast(va_arg(args, uint64_t)); auto frameTimelineVsyncId = static_cast(va_arg(args, int64_t)); auto inputEventId = static_cast(va_arg(args, int32_t)); auto startTimeNanos = static_cast(va_arg(args, int64_t)); @@ -1872,7 +1873,7 @@ int Surface::dispatchSetFrameTimelineInfo(va_list args) { ftlInfo.vsyncId = frameTimelineVsyncId; ftlInfo.inputEventId = inputEventId; ftlInfo.startTimeNanos = startTimeNanos; - return setFrameTimelineInfo(ftlInfo); + return setFrameTimelineInfo(frameNumber, ftlInfo); } bool Surface::transformToDisplayInverse() const { @@ -2641,7 +2642,8 @@ void Surface::ProducerListenerProxy::onBuffersDiscarded(const std::vectorsetFrameRate(mFdp.ConsumeFloatingPoint(), mFdp.ConsumeIntegral(), mFdp.ConsumeBool() /*shouldBeSeamless*/); FrameTimelineInfo info; - queue->setFrameTimelineInfo(info); + queue->setFrameTimelineInfo(mFdp.ConsumeIntegral(), info); ManageResourceHandle handle(&mFdp); queue->setSidebandStream(handle.getStream()); diff --git a/libs/gui/include/gui/BLASTBufferQueue.h b/libs/gui/include/gui/BLASTBufferQueue.h index 001d8e5233..c93ab86770 100644 --- a/libs/gui/include/gui/BLASTBufferQueue.h +++ b/libs/gui/include/gui/BLASTBufferQueue.h @@ -106,7 +106,7 @@ public: void update(const sp& surface, uint32_t width, uint32_t height, int32_t format); status_t setFrameRate(float frameRate, int8_t compatibility, bool shouldBeSeamless); - status_t setFrameTimelineInfo(const FrameTimelineInfo& info); + status_t setFrameTimelineInfo(uint64_t frameNumber, const FrameTimelineInfo& info); void setSidebandStream(const sp& stream); @@ -231,7 +231,7 @@ private: std::vector> mPendingTransactions GUARDED_BY(mMutex); - std::queue mNextFrameTimelineInfoQueue GUARDED_BY(mMutex); + std::queue> mPendingFrameTimelines GUARDED_BY(mMutex); // Tracks the last acquired frame number uint64_t mLastAcquiredFrameNumber GUARDED_BY(mMutex) = 0; diff --git a/libs/gui/include/gui/Surface.h b/libs/gui/include/gui/Surface.h index b9ccdc9124..39a59e42aa 100644 --- a/libs/gui/include/gui/Surface.h +++ b/libs/gui/include/gui/Surface.h @@ -213,7 +213,7 @@ public: virtual status_t setFrameRate(float frameRate, int8_t compatibility, int8_t changeFrameRateStrategy); - virtual status_t setFrameTimelineInfo(const FrameTimelineInfo& info); + virtual status_t setFrameTimelineInfo(uint64_t frameNumber, const FrameTimelineInfo& info); protected: virtual ~Surface(); diff --git a/libs/nativewindow/include/system/window.h b/libs/nativewindow/include/system/window.h index c7745e6672..6c54635824 100644 --- a/libs/nativewindow/include/system/window.h +++ b/libs/nativewindow/include/system/window.h @@ -1067,11 +1067,12 @@ static inline int native_window_set_frame_rate(struct ANativeWindow* window, flo } static inline int native_window_set_frame_timeline_info(struct ANativeWindow* window, + uint64_t frameNumber, int64_t frameTimelineVsyncId, int32_t inputEventId, int64_t startTimeNanos) { - return window->perform(window, NATIVE_WINDOW_SET_FRAME_TIMELINE_INFO, frameTimelineVsyncId, - inputEventId, startTimeNanos); + return window->perform(window, NATIVE_WINDOW_SET_FRAME_TIMELINE_INFO, frameNumber, + frameTimelineVsyncId, inputEventId, startTimeNanos); } // ------------------------------------------------------------------------------------------------ -- cgit v1.2.3-59-g8ed1b From b4b484aca143cf86d91ed86824d9e5081e0ddbf0 Mon Sep 17 00:00:00 2001 From: Vishnu Nair Date: Fri, 20 Jan 2023 10:00:18 -0800 Subject: BBQ: Check if we have already acquired max num of buffers When the IGBP is disconnected, the BQ state is reset but the BBQ state is not. Without the max acquire check in BBQ, we will continue to acquire buffers. This may cause issues in some stress tests which continuously disconnect and reconnect while queuing buffers. The buffers will only be released once they are released by SF. This can cause some systems to run out of memory. Ideal fix would be to track and free the buffers once the BQ is disconnected but that will be more risky for QPR. Test: atest GLSurfaceViewTest#testPauseResumeWithoutDelay Bug: 263340543 Change-Id: I77b34f6151a9d1b461649c575be98df1f00f2464 --- libs/gui/BLASTBufferQueue.cpp | 33 +++++++++++++++++++++++++++------ 1 file changed, 27 insertions(+), 6 deletions(-) (limited to 'libs/gui/BLASTBufferQueue.cpp') diff --git a/libs/gui/BLASTBufferQueue.cpp b/libs/gui/BLASTBufferQueue.cpp index 5d12463f88..422002dfdc 100644 --- a/libs/gui/BLASTBufferQueue.cpp +++ b/libs/gui/BLASTBufferQueue.cpp @@ -485,11 +485,19 @@ void BLASTBufferQueue::releaseBuffer(const ReleaseCallbackId& callbackId, status_t BLASTBufferQueue::acquireNextBufferLocked( const std::optional transaction) { - // If the next transaction is set, we want to guarantee the our acquire will not fail, so don't - // include the extra buffer when checking if we can acquire the next buffer. + // Check if we have frames available and we have not acquired the maximum number of buffers. + // Even with this check, the consumer can fail to acquire an additional buffer if the consumer + // has already acquired (mMaxAcquiredBuffers + 1) and the new buffer is not droppable. In this + // case mBufferItemConsumer->acquireBuffer will return with NO_BUFFER_AVAILABLE. if (mNumFrameAvailable == 0) { - BQA_LOGV("Can't process next buffer. No available frames"); - return NOT_ENOUGH_DATA; + BQA_LOGV("Can't acquire next buffer. No available frames"); + return BufferQueue::NO_BUFFER_AVAILABLE; + } + + if (mNumAcquired >= (mMaxAcquiredBuffers + 2)) { + BQA_LOGV("Can't acquire next buffer. Already acquired max frames %d max:%d + 2", + mNumAcquired, mMaxAcquiredBuffers); + return BufferQueue::NO_BUFFER_AVAILABLE; } if (mSurfaceControl == nullptr) { @@ -662,12 +670,12 @@ void BLASTBufferQueue::acquireAndReleaseBuffer() { void BLASTBufferQueue::onFrameAvailable(const BufferItem& item) { std::function prevCallback = nullptr; SurfaceComposerClient::Transaction* prevTransaction = nullptr; - bool waitForTransactionCallback = !mSyncedFrameNumbers.empty(); { std::unique_lock _lock{mMutex}; BBQ_TRACE(); + bool waitForTransactionCallback = !mSyncedFrameNumbers.empty(); const bool syncTransactionSet = mTransactionReadyCallback != nullptr; BQA_LOGV("onFrameAvailable-start syncTransactionSet=%s", boolToString(syncTransactionSet)); @@ -696,6 +704,15 @@ void BLASTBufferQueue::onFrameAvailable(const BufferItem& item) { // flush out the shadow queue acquireAndReleaseBuffer(); } + } else { + // Make sure the frame available count is 0 before proceeding with a sync to ensure + // the correct frame is used for the sync. The only way mNumFrameAvailable would be + // greater than 0 is if we already ran out of buffers previously. This means we + // need to flush the buffers before proceeding with the sync. + while (mNumFrameAvailable > 0) { + BQA_LOGD("waiting until no queued buffers"); + mCallbackCV.wait(_lock); + } } } @@ -711,6 +728,11 @@ void BLASTBufferQueue::onFrameAvailable(const BufferItem& item) { item.mFrameNumber, boolToString(syncTransactionSet)); if (syncTransactionSet) { + // Add to mSyncedFrameNumbers before waiting in case any buffers are released + // while waiting for a free buffer. The release and commit callback will try to + // acquire buffers if there are any available, but we don't want it to acquire + // in the case where a sync transaction wants the buffer. + mSyncedFrameNumbers.emplace(item.mFrameNumber); // If there's no available buffer and we're in a sync transaction, we need to wait // instead of returning since we guarantee a buffer will be acquired for the sync. while (acquireNextBufferLocked(mSyncTransaction) == BufferQueue::NO_BUFFER_AVAILABLE) { @@ -723,7 +745,6 @@ void BLASTBufferQueue::onFrameAvailable(const BufferItem& item) { incStrong((void*)transactionCommittedCallbackThunk); mSyncTransaction->addTransactionCommittedCallback(transactionCommittedCallbackThunk, static_cast(this)); - mSyncedFrameNumbers.emplace(item.mFrameNumber); if (mAcquireSingleBuffer) { prevCallback = mTransactionReadyCallback; prevTransaction = mSyncTransaction; -- cgit v1.2.3-59-g8ed1b From e0237bbe066bb27eb88b4c5d6a5485cf9c2dd45d Mon Sep 17 00:00:00 2001 From: Chavi Weingarten Date: Mon, 6 Feb 2023 21:48:32 +0000 Subject: Add thread safety check to libgui build Fix places that had errors to ensure locks are correctly held in libgui Test: Builds Bug: 268091723 Change-Id: I8a94edeb649608ff66d25a482026a81074466305 --- libs/gui/Android.bp | 4 ++ libs/gui/BLASTBufferQueue.cpp | 58 ++++++++++++++-------------- libs/gui/SurfaceComposerClient.cpp | 3 ++ libs/gui/include/gui/BLASTBufferQueue.h | 22 +++++------ libs/gui/include/gui/SurfaceComposerClient.h | 2 +- 5 files changed, 49 insertions(+), 40 deletions(-) (limited to 'libs/gui/BLASTBufferQueue.cpp') diff --git a/libs/gui/Android.bp b/libs/gui/Android.bp index 6c9c28a48f..21900a073a 100644 --- a/libs/gui/Android.bp +++ b/libs/gui/Android.bp @@ -254,6 +254,10 @@ cc_library_shared { lto: { thin: true, }, + + cflags: [ + "-Wthread-safety", + ], } // Used by media codec services exclusively as a static lib for diff --git a/libs/gui/BLASTBufferQueue.cpp b/libs/gui/BLASTBufferQueue.cpp index 60603ba50a..de64ea8476 100644 --- a/libs/gui/BLASTBufferQueue.cpp +++ b/libs/gui/BLASTBufferQueue.cpp @@ -35,6 +35,7 @@ #include #include +#include #include using namespace std::chrono_literals; @@ -63,6 +64,10 @@ namespace android { ATRACE_FORMAT("%s - %s(f:%u,a:%u)" x, __FUNCTION__, mName.c_str(), mNumFrameAvailable, \ mNumAcquired, ##__VA_ARGS__) +#define UNIQUE_LOCK_WITH_ASSERTION(mutex) \ + std::unique_lock _lock{mutex}; \ + base::ScopedLockAssertion assumeLocked(mutex); + void BLASTBufferItemConsumer::onDisconnect() { Mutex::Autolock lock(mMutex); mPreviouslyConnected = mCurrentlyConnected; @@ -207,7 +212,7 @@ void BLASTBufferQueue::update(const sp& surface, uint32_t width, int32_t format) { LOG_ALWAYS_FATAL_IF(surface == nullptr, "BLASTBufferQueue: mSurfaceControl must not be NULL"); - std::unique_lock _lock{mMutex}; + std::lock_guard _lock{mMutex}; if (mFormat != format) { mFormat = format; mBufferItemConsumer->setDefaultBufferFormat(convertBufferFormat(format)); @@ -277,7 +282,7 @@ void BLASTBufferQueue::transactionCommittedCallback(nsecs_t /*latchTime*/, const sp& /*presentFence*/, const std::vector& stats) { { - std::unique_lock _lock{mMutex}; + std::lock_guard _lock{mMutex}; BBQ_TRACE(); BQA_LOGV("transactionCommittedCallback"); if (!mSurfaceControlsWithPendingCallback.empty()) { @@ -325,7 +330,7 @@ static void transactionCallbackThunk(void* context, nsecs_t latchTime, void BLASTBufferQueue::transactionCallback(nsecs_t /*latchTime*/, const sp& /*presentFence*/, const std::vector& stats) { { - std::unique_lock _lock{mMutex}; + std::lock_guard _lock{mMutex}; BBQ_TRACE(); BQA_LOGV("transactionCallback"); @@ -406,9 +411,8 @@ void BLASTBufferQueue::flushShadowQueue() { void BLASTBufferQueue::releaseBufferCallback( const ReleaseCallbackId& id, const sp& releaseFence, std::optional currentMaxAcquiredBufferCount) { + std::lock_guard _lock{mMutex}; BBQ_TRACE(); - - std::unique_lock _lock{mMutex}; releaseBufferCallbackLocked(id, releaseFence, currentMaxAcquiredBufferCount, false /* fakeRelease */); } @@ -423,10 +427,8 @@ void BLASTBufferQueue::releaseBufferCallbackLocked( // to the buffer queue. This will prevent higher latency when we are running // on a lower refresh rate than the max supported. We only do that for EGL // clients as others don't care about latency - const bool isEGL = [&] { - const auto it = mSubmitted.find(id); - return it != mSubmitted.end() && it->second.mApi == NATIVE_WINDOW_API_EGL; - }(); + const auto it = mSubmitted.find(id); + const bool isEGL = it != mSubmitted.end() && it->second.mApi == NATIVE_WINDOW_API_EGL; if (currentMaxAcquiredBufferCount) { mCurrentMaxAcquiredBufferCount = *currentMaxAcquiredBufferCount; @@ -607,7 +609,7 @@ status_t BLASTBufferQueue::acquireNextBufferLocked( } { - std::unique_lock _lock{mTimestampMutex}; + std::lock_guard _lock{mTimestampMutex}; auto dequeueTime = mDequeueTimestamps.find(buffer->getId()); if (dequeueTime != mDequeueTimestamps.end()) { Parcel p; @@ -662,11 +664,11 @@ void BLASTBufferQueue::acquireAndReleaseBuffer() { void BLASTBufferQueue::onFrameAvailable(const BufferItem& item) { std::function prevCallback = nullptr; SurfaceComposerClient::Transaction* prevTransaction = nullptr; - bool waitForTransactionCallback = !mSyncedFrameNumbers.empty(); { - std::unique_lock _lock{mMutex}; + UNIQUE_LOCK_WITH_ASSERTION(mMutex); BBQ_TRACE(); + bool waitForTransactionCallback = !mSyncedFrameNumbers.empty(); const bool syncTransactionSet = mTransactionReadyCallback != nullptr; BQA_LOGV("onFrameAvailable-start syncTransactionSet=%s", boolToString(syncTransactionSet)); @@ -745,25 +747,24 @@ void BLASTBufferQueue::onFrameReplaced(const BufferItem& item) { } void BLASTBufferQueue::onFrameDequeued(const uint64_t bufferId) { - std::unique_lock _lock{mTimestampMutex}; + std::lock_guard _lock{mTimestampMutex}; mDequeueTimestamps[bufferId] = systemTime(); }; void BLASTBufferQueue::onFrameCancelled(const uint64_t bufferId) { - std::unique_lock _lock{mTimestampMutex}; + std::lock_guard _lock{mTimestampMutex}; mDequeueTimestamps.erase(bufferId); }; void BLASTBufferQueue::syncNextTransaction( std::function callback, bool acquireSingleBuffer) { - BBQ_TRACE(); - std::function prevCallback = nullptr; SurfaceComposerClient::Transaction* prevTransaction = nullptr; { std::lock_guard _lock{mMutex}; + BBQ_TRACE(); // We're about to overwrite the previous call so we should invoke that callback // immediately. if (mTransactionReadyCallback) { @@ -829,8 +830,8 @@ bool BLASTBufferQueue::rejectBuffer(const BufferItem& item) { class BBQSurface : public Surface { private: std::mutex mMutex; - sp mBbq; - bool mDestroyed = false; + sp mBbq GUARDED_BY(mMutex); + bool mDestroyed GUARDED_BY(mMutex) = false; public: BBQSurface(const sp& igbp, bool controlledByApp, @@ -851,7 +852,7 @@ public: status_t setFrameRate(float frameRate, int8_t compatibility, int8_t changeFrameRateStrategy) override { - std::unique_lock _lock{mMutex}; + std::lock_guard _lock{mMutex}; if (mDestroyed) { return DEAD_OBJECT; } @@ -864,7 +865,7 @@ public: status_t setFrameTimelineInfo(uint64_t frameNumber, const FrameTimelineInfo& frameTimelineInfo) override { - std::unique_lock _lock{mMutex}; + std::lock_guard _lock{mMutex}; if (mDestroyed) { return DEAD_OBJECT; } @@ -874,7 +875,7 @@ public: void destroy() override { Surface::destroy(); - std::unique_lock _lock{mMutex}; + std::lock_guard _lock{mMutex}; mDestroyed = true; mBbq = nullptr; } @@ -884,7 +885,7 @@ public: // no timing issues. status_t BLASTBufferQueue::setFrameRate(float frameRate, int8_t compatibility, bool shouldBeSeamless) { - std::unique_lock _lock{mMutex}; + std::lock_guard _lock{mMutex}; SurfaceComposerClient::Transaction t; return t.setFrameRate(mSurfaceControl, frameRate, compatibility, shouldBeSeamless).apply(); @@ -894,20 +895,20 @@ status_t BLASTBufferQueue::setFrameTimelineInfo(uint64_t frameNumber, const FrameTimelineInfo& frameTimelineInfo) { ATRACE_FORMAT("%s(%s) frameNumber: %" PRIu64 " vsyncId: %" PRId64, __func__, mName.c_str(), frameNumber, frameTimelineInfo.vsyncId); - std::unique_lock _lock{mMutex}; + std::lock_guard _lock{mMutex}; mPendingFrameTimelines.push({frameNumber, frameTimelineInfo}); return OK; } void BLASTBufferQueue::setSidebandStream(const sp& stream) { - std::unique_lock _lock{mMutex}; + std::lock_guard _lock{mMutex}; SurfaceComposerClient::Transaction t; t.setSidebandStream(mSurfaceControl, stream).apply(); } sp BLASTBufferQueue::getSurface(bool includeSurfaceControlHandle) { - std::unique_lock _lock{mMutex}; + std::lock_guard _lock{mMutex}; sp scHandle = nullptr; if (includeSurfaceControlHandle && mSurfaceControl) { scHandle = mSurfaceControl->getHandle(); @@ -1098,6 +1099,7 @@ PixelFormat BLASTBufferQueue::convertBufferFormat(PixelFormat& format) { } uint32_t BLASTBufferQueue::getLastTransformHint() const { + std::lock_guard _lock{mMutex}; if (mSurfaceControl != nullptr) { return mSurfaceControl->getTransformHint(); } else { @@ -1106,18 +1108,18 @@ uint32_t BLASTBufferQueue::getLastTransformHint() const { } uint64_t BLASTBufferQueue::getLastAcquiredFrameNum() { - std::unique_lock _lock{mMutex}; + std::lock_guard _lock{mMutex}; return mLastAcquiredFrameNumber; } bool BLASTBufferQueue::isSameSurfaceControl(const sp& surfaceControl) const { - std::unique_lock _lock{mMutex}; + std::lock_guard _lock{mMutex}; return SurfaceControl::isSameSurface(mSurfaceControl, surfaceControl); } void BLASTBufferQueue::setTransactionHangCallback( std::function callback) { - std::unique_lock _lock{mMutex}; + std::lock_guard _lock{mMutex}; mTransactionHangCallback = callback; } diff --git a/libs/gui/SurfaceComposerClient.cpp b/libs/gui/SurfaceComposerClient.cpp index 9092f5fe67..ccd225cd62 100644 --- a/libs/gui/SurfaceComposerClient.cpp +++ b/libs/gui/SurfaceComposerClient.cpp @@ -53,6 +53,7 @@ #include #include +#include #include #include @@ -2988,6 +2989,7 @@ void ReleaseCallbackThread::threadMain() { while (true) { { std::unique_lock lock(mMutex); + base::ScopedLockAssertion assumeLocked(mMutex); callbackInfos = std::move(mCallbackInfos); mCallbackInfos = {}; } @@ -3000,6 +3002,7 @@ void ReleaseCallbackThread::threadMain() { { std::unique_lock lock(mMutex); + base::ScopedLockAssertion assumeLocked(mMutex); if (mCallbackInfos.size() == 0) { mReleaseCallbackPending.wait(lock); } diff --git a/libs/gui/include/gui/BLASTBufferQueue.h b/libs/gui/include/gui/BLASTBufferQueue.h index c93ab86770..8d07162f1b 100644 --- a/libs/gui/include/gui/BLASTBufferQueue.h +++ b/libs/gui/include/gui/BLASTBufferQueue.h @@ -44,23 +44,23 @@ public: mCurrentlyConnected(false), mPreviouslyConnected(false) {} - void onDisconnect() override; + void onDisconnect() override EXCLUDES(mMutex); void addAndGetFrameTimestamps(const NewFrameEventsEntry* newTimestamps, - FrameEventHistoryDelta* outDelta) override REQUIRES(mMutex); + FrameEventHistoryDelta* outDelta) override EXCLUDES(mMutex); void updateFrameTimestamps(uint64_t frameNumber, nsecs_t refreshStartTime, const sp& gpuCompositionDoneFence, const sp& presentFence, const sp& prevReleaseFence, CompositorTiming compositorTiming, nsecs_t latchTime, - nsecs_t dequeueReadyTime) REQUIRES(mMutex); - void getConnectionEvents(uint64_t frameNumber, bool* needsDisconnect); + nsecs_t dequeueReadyTime) EXCLUDES(mMutex); + void getConnectionEvents(uint64_t frameNumber, bool* needsDisconnect) EXCLUDES(mMutex); protected: - void onSidebandStreamChanged() override REQUIRES(mMutex); + void onSidebandStreamChanged() override EXCLUDES(mMutex); private: const wp mBLASTBufferQueue; - uint64_t mCurrentFrameNumber = 0; + uint64_t mCurrentFrameNumber GUARDED_BY(mMutex) = 0; Mutex mMutex; ConsumerFrameEventHistory mFrameEventHistory GUARDED_BY(mMutex); @@ -94,7 +94,7 @@ public: std::optional currentMaxAcquiredBufferCount); void releaseBufferCallbackLocked(const ReleaseCallbackId& id, const sp& releaseFence, std::optional currentMaxAcquiredBufferCount, - bool fakeRelease); + bool fakeRelease) REQUIRES(mMutex); void syncNextTransaction(std::function callback, bool acquireSingleBuffer = true); void stopContinuousSyncTransaction(); @@ -150,7 +150,7 @@ private: // mNumAcquired (buffers that queued to SF) mPendingRelease.size() (buffers that are held by // blast). This counter is read by android studio profiler. std::string mQueuedBufferTrace; - sp mSurfaceControl; + sp mSurfaceControl GUARDED_BY(mMutex); mutable std::mutex mMutex; std::condition_variable mCallbackCV; @@ -252,7 +252,7 @@ private: // callback for them. std::queue> mSurfaceControlsWithPendingCallback GUARDED_BY(mMutex); - uint32_t mCurrentMaxAcquiredBufferCount; + uint32_t mCurrentMaxAcquiredBufferCount GUARDED_BY(mMutex); // Flag to determine if syncTransaction should only acquire a single buffer and then clear or // continue to acquire buffers until explicitly cleared @@ -276,8 +276,8 @@ private: // need to set this flag, notably only in the case where we are transitioning from a previous // transaction applied by us (one way, may not yet have reached server) and an upcoming // transaction that will be applied by some sync consumer. - bool mAppliedLastTransaction = false; - uint64_t mLastAppliedFrameNumber = 0; + bool mAppliedLastTransaction GUARDED_BY(mMutex) = false; + uint64_t mLastAppliedFrameNumber GUARDED_BY(mMutex) = 0; std::function mTransactionHangCallback; diff --git a/libs/gui/include/gui/SurfaceComposerClient.h b/libs/gui/include/gui/SurfaceComposerClient.h index 45f4dbe2be..e3470c59b2 100644 --- a/libs/gui/include/gui/SurfaceComposerClient.h +++ b/libs/gui/include/gui/SurfaceComposerClient.h @@ -921,7 +921,7 @@ public: void onTrustedPresentationChanged(int id, bool presentedWithinThresholds) override; private: - ReleaseBufferCallback popReleaseBufferCallbackLocked(const ReleaseCallbackId&); + ReleaseBufferCallback popReleaseBufferCallbackLocked(const ReleaseCallbackId&) REQUIRES(mMutex); static sp sInstance; }; -- cgit v1.2.3-59-g8ed1b From ce14101c144b66bf8ec63b8669b51e7717f72897 Mon Sep 17 00:00:00 2001 From: Chavi Weingarten Date: Tue, 7 Feb 2023 20:11:24 +0000 Subject: Merge conflict didn't pick up changes. Change-Id: I195ebf30a87b7135a6cf3f9012947cfb794c14df --- libs/gui/BLASTBufferQueue.cpp | 1 - 1 file changed, 1 deletion(-) (limited to 'libs/gui/BLASTBufferQueue.cpp') diff --git a/libs/gui/BLASTBufferQueue.cpp b/libs/gui/BLASTBufferQueue.cpp index d5431916cc..66c0041965 100644 --- a/libs/gui/BLASTBufferQueue.cpp +++ b/libs/gui/BLASTBufferQueue.cpp @@ -678,7 +678,6 @@ void BLASTBufferQueue::onFrameAvailable(const BufferItem& item) { BBQ_TRACE(); bool waitForTransactionCallback = !mSyncedFrameNumbers.empty(); - bool waitForTransactionCallback = !mSyncedFrameNumbers.empty(); const bool syncTransactionSet = mTransactionReadyCallback != nullptr; BQA_LOGV("onFrameAvailable-start syncTransactionSet=%s", boolToString(syncTransactionSet)); -- cgit v1.2.3-59-g8ed1b From eb489f69db265f6cb1aab63130750d0c12c99d94 Mon Sep 17 00:00:00 2001 From: liulijun Date: Mon, 17 Oct 2022 22:02:14 +0800 Subject: Add producerId so we know when the BBQ producer has been changed. If BBQ producer changes but the SC remains the same, the frame numbers for the SC will get reset. This causes issues if there's a barrier layer set because the barrier is waiting for a particular frame number before applying the transaction. Since the frame numbers have been reset, the barrier will be greater than the incoming frame numbers. The change adds a producerId to the buffer being sent so it can check if the producerId is older than what's currently set on the Layer. If there's a barriers set from the old producer, the buffer can be released and not applied and will stop SF from waiting indefinitely. Bug: 251971691 Test: Builds, hard to repro Signed-off-by: Liu Lijun Change-Id: If37171de4693a73f36f8de43e29c129b352eb55f --- libs/gui/BLASTBufferQueue.cpp | 14 ++++---- libs/gui/LayerState.cpp | 2 ++ libs/gui/SurfaceComposerClient.cpp | 3 +- libs/gui/include/gui/BLASTBufferQueue.h | 5 +++ libs/gui/include/gui/LayerState.h | 1 + libs/gui/include/gui/SurfaceComposerClient.h | 2 +- .../surfaceflinger/FrontEnd/TransactionHandler.h | 2 +- services/surfaceflinger/Layer.cpp | 18 ++++------ services/surfaceflinger/Layer.h | 10 ++++++ services/surfaceflinger/SurfaceFlinger.cpp | 40 +++++++++++++++------- services/surfaceflinger/TransactionState.h | 22 +++++++++--- .../tests/ReleaseBufferCallback_test.cpp | 26 +++++++------- 12 files changed, 95 insertions(+), 50 deletions(-) (limited to 'libs/gui/BLASTBufferQueue.cpp') diff --git a/libs/gui/BLASTBufferQueue.cpp b/libs/gui/BLASTBufferQueue.cpp index 66c0041965..9d82c143f5 100644 --- a/libs/gui/BLASTBufferQueue.cpp +++ b/libs/gui/BLASTBufferQueue.cpp @@ -20,6 +20,7 @@ #define ATRACE_TAG ATRACE_TAG_GRAPHICS //#define LOG_NDEBUG 0 +#include #include #include #include @@ -157,11 +158,11 @@ BLASTBufferQueue::BLASTBufferQueue(const std::string& name, bool updateDestinati GraphicBuffer::USAGE_HW_COMPOSER | GraphicBuffer::USAGE_HW_TEXTURE, 1, false, this); - static int32_t id = 0; - mName = name + "#" + std::to_string(id); - auto consumerName = mName + "(BLAST Consumer)" + std::to_string(id); - mQueuedBufferTrace = "QueuedBuffer - " + mName + "BLAST#" + std::to_string(id); - id++; + static std::atomic nextId = 0; + mProducerId = nextId++; + mName = name + "#" + std::to_string(mProducerId); + auto consumerName = mName + "(BLAST Consumer)" + std::to_string(mProducerId); + mQueuedBufferTrace = "QueuedBuffer - " + mName + "BLAST#" + std::to_string(mProducerId); mBufferItemConsumer->setName(String8(consumerName.c_str())); mBufferItemConsumer->setFrameAvailableListener(this); @@ -572,7 +573,8 @@ status_t BLASTBufferQueue::acquireNextBufferLocked( std::bind(releaseBufferCallbackThunk, wp(this) /* callbackContext */, std::placeholders::_1, std::placeholders::_2, std::placeholders::_3); sp fence = bufferItem.mFence ? new Fence(bufferItem.mFence->dup()) : Fence::NO_FENCE; - t->setBuffer(mSurfaceControl, buffer, fence, bufferItem.mFrameNumber, releaseBufferCallback); + t->setBuffer(mSurfaceControl, buffer, fence, bufferItem.mFrameNumber, mProducerId, + releaseBufferCallback); t->setDataspace(mSurfaceControl, static_cast(bufferItem.mDataSpace)); t->setHdrMetadata(mSurfaceControl, bufferItem.mHdrMetadata); t->setSurfaceDamageRegion(mSurfaceControl, bufferItem.mSurfaceDamage); diff --git a/libs/gui/LayerState.cpp b/libs/gui/LayerState.cpp index 7772a65dae..a6276e500c 100644 --- a/libs/gui/LayerState.cpp +++ b/libs/gui/LayerState.cpp @@ -984,6 +984,7 @@ status_t BufferData::writeToParcel(Parcel* output) const { SAFE_PARCEL(output->writeUint64, cachedBuffer.id); SAFE_PARCEL(output->writeBool, hasBarrier); SAFE_PARCEL(output->writeUint64, barrierFrameNumber); + SAFE_PARCEL(output->writeUint32, producerId); return NO_ERROR; } @@ -1022,6 +1023,7 @@ status_t BufferData::readFromParcel(const Parcel* input) { SAFE_PARCEL(input->readBool, &hasBarrier); SAFE_PARCEL(input->readUint64, &barrierFrameNumber); + SAFE_PARCEL(input->readUint32, &producerId); return NO_ERROR; } diff --git a/libs/gui/SurfaceComposerClient.cpp b/libs/gui/SurfaceComposerClient.cpp index a34593825a..5088604396 100644 --- a/libs/gui/SurfaceComposerClient.cpp +++ b/libs/gui/SurfaceComposerClient.cpp @@ -1632,7 +1632,7 @@ SurfaceComposerClient::Transaction& SurfaceComposerClient::Transaction::setBuffe SurfaceComposerClient::Transaction& SurfaceComposerClient::Transaction::setBuffer( const sp& sc, const sp& buffer, const std::optional>& fence, const std::optional& optFrameNumber, - ReleaseBufferCallback callback) { + uint32_t producerId, ReleaseBufferCallback callback) { layer_state_t* s = getLayerState(sc); if (!s) { mStatus = BAD_INDEX; @@ -1651,6 +1651,7 @@ SurfaceComposerClient::Transaction& SurfaceComposerClient::Transaction::setBuffe bufferData->buffer = buffer; uint64_t frameNumber = sc->resolveFrameNumber(optFrameNumber); bufferData->frameNumber = frameNumber; + bufferData->producerId = producerId; bufferData->flags |= BufferData::BufferDataChange::frameNumberChanged; if (fence) { bufferData->acquireFence = *fence; diff --git a/libs/gui/include/gui/BLASTBufferQueue.h b/libs/gui/include/gui/BLASTBufferQueue.h index 8d07162f1b..b9e06473bf 100644 --- a/libs/gui/include/gui/BLASTBufferQueue.h +++ b/libs/gui/include/gui/BLASTBufferQueue.h @@ -162,6 +162,11 @@ private: int32_t mNumFrameAvailable GUARDED_BY(mMutex) = 0; int32_t mNumAcquired GUARDED_BY(mMutex) = 0; + // A value used to identify if a producer has been changed for the same SurfaceControl. + // This is needed to know when the frame number has been reset to make sure we don't + // latch stale buffers and that we don't wait on barriers from an old producer. + uint32_t mProducerId = 0; + // Keep a reference to the submitted buffers so we can release when surfaceflinger drops the // buffer or the buffer has been presented and a new buffer is ready to be presented. std::unordered_map mSubmitted diff --git a/libs/gui/include/gui/LayerState.h b/libs/gui/include/gui/LayerState.h index 03a2582589..ddaf473855 100644 --- a/libs/gui/include/gui/LayerState.h +++ b/libs/gui/include/gui/LayerState.h @@ -111,6 +111,7 @@ public: uint64_t frameNumber = 0; bool hasBarrier = false; uint64_t barrierFrameNumber = 0; + uint32_t producerId = 0; // Listens to when the buffer is safe to be released. This is used for blast // layers only. The callback includes a release fence as well as the graphic diff --git a/libs/gui/include/gui/SurfaceComposerClient.h b/libs/gui/include/gui/SurfaceComposerClient.h index 809ea5a30f..ffd5af100d 100644 --- a/libs/gui/include/gui/SurfaceComposerClient.h +++ b/libs/gui/include/gui/SurfaceComposerClient.h @@ -536,7 +536,7 @@ public: Transaction& setBuffer(const sp& sc, const sp& buffer, const std::optional>& fence = std::nullopt, const std::optional& frameNumber = std::nullopt, - ReleaseBufferCallback callback = nullptr); + uint32_t producerId = 0, ReleaseBufferCallback callback = nullptr); std::shared_ptr getAndClearBuffer(const sp& sc); /** diff --git a/services/surfaceflinger/FrontEnd/TransactionHandler.h b/services/surfaceflinger/FrontEnd/TransactionHandler.h index a06b870549..7fc825eba3 100644 --- a/services/surfaceflinger/FrontEnd/TransactionHandler.h +++ b/services/surfaceflinger/FrontEnd/TransactionHandler.h @@ -34,7 +34,7 @@ namespace surfaceflinger::frontend { class TransactionHandler { public: struct TransactionFlushState { - const TransactionState* transaction; + TransactionState* transaction; bool firstTransaction = true; nsecs_t queueProcessTime = 0; // Layer handles that have transactions with buffers that are ready to be applied. diff --git a/services/surfaceflinger/Layer.cpp b/services/surfaceflinger/Layer.cpp index 66c2fb658f..8c484f0039 100644 --- a/services/surfaceflinger/Layer.cpp +++ b/services/surfaceflinger/Layer.cpp @@ -228,9 +228,7 @@ Layer::~Layer() { if (mBufferInfo.mBuffer != nullptr) { callReleaseBufferCallback(mDrawingState.releaseBufferListener, mBufferInfo.mBuffer->getBuffer(), mBufferInfo.mFrameNumber, - mBufferInfo.mFence, - mFlinger->getMaxAcquiredBufferCountForCurrentRefreshRate( - mOwnerUid)); + mBufferInfo.mFence); } if (!isClone()) { // The original layer and the clone layer share the same texture. Therefore, only one of @@ -2732,12 +2730,13 @@ void Layer::cloneDrawingState(const Layer* from) { void Layer::callReleaseBufferCallback(const sp& listener, const sp& buffer, uint64_t framenumber, - const sp& releaseFence, - uint32_t currentMaxAcquiredBufferCount) { + const sp& releaseFence) { if (!listener) { return; } ATRACE_FORMAT_INSTANT("callReleaseBufferCallback %s - %" PRIu64, getDebugName(), framenumber); + uint32_t currentMaxAcquiredBufferCount = + mFlinger->getMaxAcquiredBufferCountForCurrentRefreshRate(mOwnerUid); listener->onReleaseBuffer({buffer->getId(), framenumber}, releaseFence ? releaseFence : Fence::NO_FENCE, currentMaxAcquiredBufferCount); @@ -2988,9 +2987,7 @@ bool Layer::setBuffer(std::shared_ptr& buffer, // call any release buffer callbacks if set. callReleaseBufferCallback(mDrawingState.releaseBufferListener, mDrawingState.buffer->getBuffer(), mDrawingState.frameNumber, - mDrawingState.acquireFence, - mFlinger->getMaxAcquiredBufferCountForCurrentRefreshRate( - mOwnerUid)); + mDrawingState.acquireFence); decrementPendingBufferCount(); if (mDrawingState.bufferSurfaceFrameTX != nullptr && mDrawingState.bufferSurfaceFrameTX->getPresentState() != PresentState::Presented) { @@ -3000,13 +2997,12 @@ bool Layer::setBuffer(std::shared_ptr& buffer, } else if (EARLY_RELEASE_ENABLED && mLastClientCompositionFence != nullptr) { callReleaseBufferCallback(mDrawingState.releaseBufferListener, mDrawingState.buffer->getBuffer(), mDrawingState.frameNumber, - mLastClientCompositionFence, - mFlinger->getMaxAcquiredBufferCountForCurrentRefreshRate( - mOwnerUid)); + mLastClientCompositionFence); mLastClientCompositionFence = nullptr; } } + mDrawingState.producerId = bufferData.producerId; mDrawingState.frameNumber = frameNumber; mDrawingState.releaseBufferListener = bufferData.releaseBufferListener; mDrawingState.buffer = std::move(buffer); diff --git a/services/surfaceflinger/Layer.h b/services/surfaceflinger/Layer.h index 07c01d8e17..bf8cc7e36b 100644 --- a/services/surfaceflinger/Layer.h +++ b/services/surfaceflinger/Layer.h @@ -141,6 +141,8 @@ public: uint64_t frameNumber; ui::Transform transform; + + uint32_t producerId = 0; uint32_t bufferTransform; bool transformToDisplayInverse; Region transparentRegionHint; @@ -838,6 +840,10 @@ public: std::unordered_set& visited); bool willPresentCurrentTransaction() const; + void callReleaseBufferCallback(const sp& listener, + const sp& buffer, uint64_t framenumber, + const sp& releaseFence); + protected: // For unit tests friend class TestableSurfaceFlinger; @@ -1047,6 +1053,10 @@ private: const sp& releaseFence, uint32_t currentMaxAcquiredBufferCount); + // Returns true if the transformed buffer size does not match the layer size and we need + // to apply filtering. + bool bufferNeedsFiltering() const; + // Returns true if there is a valid color to fill. bool fillsColor() const; // Returns true if this layer has a blur value. diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp index 68ab776400..e74656e55c 100644 --- a/services/surfaceflinger/SurfaceFlinger.cpp +++ b/services/surfaceflinger/SurfaceFlinger.cpp @@ -4058,16 +4058,30 @@ TransactionHandler::TransactionReadiness SurfaceFlinger::transactionReadyBufferC sp layer = LayerHandle::getLayer(s.surface); const auto& transaction = *flushState.transaction; // check for barrier frames - if (s.bufferData->hasBarrier && - ((layer->getDrawingState().frameNumber) < s.bufferData->barrierFrameNumber)) { - const bool willApplyBarrierFrame = - flushState.bufferLayersReadyToPresent.contains(s.surface.get()) && - (flushState.bufferLayersReadyToPresent.get(s.surface.get()) >= - s.bufferData->barrierFrameNumber); - if (!willApplyBarrierFrame) { - ATRACE_NAME("NotReadyBarrier"); - ready = TransactionReadiness::NotReadyBarrier; - return false; + if (s.bufferData->hasBarrier) { + // The current producerId is already a newer producer than the buffer that has a + // barrier. This means the incoming buffer is older and we can release it here. We + // don't wait on the barrier since we know that's stale information. + if (layer->getDrawingState().producerId > s.bufferData->producerId) { + layer->callReleaseBufferCallback(s.bufferData->releaseBufferListener, + s.bufferData->buffer, s.bufferData->frameNumber, + s.bufferData->acquireFence); + // Delete the entire state at this point and not just release the buffer because + // everything associated with the Layer in this Transaction is now out of date. + ATRACE_NAME("DeleteStaleBuffer"); + return TraverseBuffersReturnValues::DELETE_AND_CONTINUE_TRAVERSAL; + } + + if (layer->getDrawingState().frameNumber < s.bufferData->barrierFrameNumber) { + const bool willApplyBarrierFrame = + flushState.bufferLayersReadyToPresent.contains(s.surface.get()) && + ((flushState.bufferLayersReadyToPresent.get(s.surface.get()) >= + s.bufferData->barrierFrameNumber)); + if (!willApplyBarrierFrame) { + ATRACE_NAME("NotReadyBarrier"); + ready = TransactionReadiness::NotReadyBarrier; + return TraverseBuffersReturnValues::STOP_TRAVERSAL; + } } } @@ -4078,7 +4092,7 @@ TransactionHandler::TransactionReadiness SurfaceFlinger::transactionReadyBufferC if (layer->backpressureEnabled() && hasPendingBuffer && transaction.isAutoTimestamp) { ATRACE_NAME("hasPendingBuffer"); ready = TransactionReadiness::NotReady; - return false; + return TraverseBuffersReturnValues::STOP_TRAVERSAL; } // check fence status @@ -4105,14 +4119,14 @@ TransactionHandler::TransactionReadiness SurfaceFlinger::transactionReadyBufferC "Buffer processing hung up due to stuck " "fence. Indicates GPU hang"); } - return false; + return TraverseBuffersReturnValues::STOP_TRAVERSAL; } ready = enableLatchUnsignaledConfig == LatchUnsignaledConfig::AutoSingleLayer ? TransactionReadiness::ReadyUnsignaledSingle : TransactionReadiness::ReadyUnsignaled; } - return true; + return TraverseBuffersReturnValues::CONTINUE_TRAVERSAL; }); ATRACE_INT("TransactionReadiness", static_cast(ready)); return ready; diff --git a/services/surfaceflinger/TransactionState.h b/services/surfaceflinger/TransactionState.h index 5025c4935c..6c5a8b213d 100644 --- a/services/surfaceflinger/TransactionState.h +++ b/services/surfaceflinger/TransactionState.h @@ -27,6 +27,12 @@ namespace android { +enum TraverseBuffersReturnValues { + CONTINUE_TRAVERSAL, + STOP_TRAVERSAL, + DELETE_AND_CONTINUE_TRAVERSAL, +}; + // Extends the client side composer state by resolving buffer. class ResolvedComposerState : public ComposerState { public: @@ -75,12 +81,18 @@ struct TransactionState { } template - void traverseStatesWithBuffersWhileTrue(Visitor&& visitor) const { - for (const auto& state : states) { - if (state.state.hasBufferChanges() && state.state.hasValidBuffer() && - state.state.surface) { - if (!visitor(state.state)) return; + void traverseStatesWithBuffersWhileTrue(Visitor&& visitor) { + for (auto state = states.begin(); state != states.end();) { + if (state->state.hasBufferChanges() && state->state.hasValidBuffer() && + state->state.surface) { + int result = visitor(state->state); + if (result == STOP_TRAVERSAL) return; + if (result == DELETE_AND_CONTINUE_TRAVERSAL) { + state = states.erase(state); + continue; + } } + state++; } } diff --git a/services/surfaceflinger/tests/ReleaseBufferCallback_test.cpp b/services/surfaceflinger/tests/ReleaseBufferCallback_test.cpp index 16076eaac9..c23fb9bd23 100644 --- a/services/surfaceflinger/tests/ReleaseBufferCallback_test.cpp +++ b/services/surfaceflinger/tests/ReleaseBufferCallback_test.cpp @@ -85,7 +85,8 @@ public: sp fence, CallbackHelper& callback, const ReleaseCallbackId& id, ReleaseBufferCallbackHelper& releaseCallback) { Transaction t; - t.setBuffer(layer, buffer, fence, id.framenumber, releaseCallback.getCallback()); + t.setBuffer(layer, buffer, fence, id.framenumber, 0 /* producerId */, + releaseCallback.getCallback()); t.addTransactionCompletedCallback(callback.function, callback.getContext()); t.apply(); } @@ -301,7 +302,7 @@ TEST_F(ReleaseBufferCallbackTest, DISABLED_FrameDropping) { Transaction t; t.setBuffer(layer, firstBuffer, std::nullopt, firstBufferCallbackId.framenumber, - releaseCallback->getCallback()); + 0 /* producerId */, releaseCallback->getCallback()); t.addTransactionCompletedCallback(transactionCallback.function, transactionCallback.getContext()); t.setDesiredPresentTime(time); @@ -317,7 +318,7 @@ TEST_F(ReleaseBufferCallbackTest, DISABLED_FrameDropping) { sp secondBuffer = getBuffer(); ReleaseCallbackId secondBufferCallbackId(secondBuffer->getId(), generateFrameNumber()); t.setBuffer(layer, secondBuffer, std::nullopt, secondBufferCallbackId.framenumber, - releaseCallback->getCallback()); + 0 /* producerId */, releaseCallback->getCallback()); t.addTransactionCompletedCallback(transactionCallback.function, transactionCallback.getContext()); t.setDesiredPresentTime(time); @@ -362,7 +363,7 @@ TEST_F(ReleaseBufferCallbackTest, DISABLED_Merge_Different_Processes) { Transaction transaction1; transaction1.setBuffer(layer, secondBuffer, std::nullopt, secondBufferCallbackId.framenumber, - releaseCallback->getCallback()); + 0 /* producerId */, releaseCallback->getCallback()); transaction1.addTransactionCompletedCallback(callback1.function, callback1.getContext()); // Set a different TransactionCompletedListener to mimic a second process @@ -397,14 +398,14 @@ TEST_F(ReleaseBufferCallbackTest, DISABLED_SetBuffer_OverwriteBuffers) { // Create transaction with a buffer. Transaction transaction; transaction.setBuffer(layer, firstBuffer, std::nullopt, firstBufferCallbackId.framenumber, - releaseCallback->getCallback()); + 0 /* producerId */, releaseCallback->getCallback()); sp secondBuffer = getBuffer(); ReleaseCallbackId secondBufferCallbackId(secondBuffer->getId(), generateFrameNumber()); // Call setBuffer on the same transaction with a different buffer. transaction.setBuffer(layer, secondBuffer, std::nullopt, secondBufferCallbackId.framenumber, - releaseCallback->getCallback()); + 0 /* producerId */, releaseCallback->getCallback()); ASSERT_NO_FATAL_FAILURE(waitForReleaseBufferCallback(*releaseCallback, firstBufferCallbackId)); } @@ -419,7 +420,7 @@ TEST_F(ReleaseBufferCallbackTest, DISABLED_Merge_Transactions_OverwriteBuffers) // Create transaction with a buffer. Transaction transaction1; transaction1.setBuffer(layer, firstBuffer, std::nullopt, firstBufferCallbackId.framenumber, - releaseCallback->getCallback()); + 0 /* producerId */, releaseCallback->getCallback()); sp secondBuffer = getBuffer(); ReleaseCallbackId secondBufferCallbackId(secondBuffer->getId(), generateFrameNumber()); @@ -427,7 +428,7 @@ TEST_F(ReleaseBufferCallbackTest, DISABLED_Merge_Transactions_OverwriteBuffers) // Create a second transaction with a new buffer for the same layer. Transaction transaction2; transaction2.setBuffer(layer, secondBuffer, std::nullopt, secondBufferCallbackId.framenumber, - releaseCallback->getCallback()); + 0 /* producerId */, releaseCallback->getCallback()); // merge transaction1 into transaction2 so ensure we get a proper buffer release callback. transaction1.merge(std::move(transaction2)); @@ -450,7 +451,7 @@ TEST_F(ReleaseBufferCallbackTest, DISABLED_MergeBuffers_Different_Processes) { Transaction transaction1; transaction1.setBuffer(layer, firstBuffer, std::nullopt, firstBufferCallbackId.framenumber, - releaseCallback->getCallback()); + 0 /* producerId */, releaseCallback->getCallback()); // Sent a second buffer to allow the first buffer to get released. sp secondBuffer = getBuffer(); @@ -458,7 +459,7 @@ TEST_F(ReleaseBufferCallbackTest, DISABLED_MergeBuffers_Different_Processes) { Transaction transaction2; transaction2.setBuffer(layer, secondBuffer, std::nullopt, secondBufferCallbackId.framenumber, - releaseCallback->getCallback()); + 0 /* producerId */, releaseCallback->getCallback()); // Set a different TransactionCompletedListener to mimic a second process TransactionCompletedListener::setInstance(secondCompletedListener); @@ -479,10 +480,11 @@ TEST_F(ReleaseBufferCallbackTest, SetBuffer_OverwriteBuffersWithNull) { // Create transaction with a buffer. Transaction transaction; transaction.setBuffer(layer, firstBuffer, std::nullopt, firstBufferCallbackId.framenumber, - releaseCallback->getCallback()); + 0 /* producerId */, releaseCallback->getCallback()); // Call setBuffer on the same transaction with a null buffer. - transaction.setBuffer(layer, nullptr, std::nullopt, 0, releaseCallback->getCallback()); + transaction.setBuffer(layer, nullptr, std::nullopt, 0, 0 /* producerId */, + releaseCallback->getCallback()); ASSERT_NO_FATAL_FAILURE(waitForReleaseBufferCallback(*releaseCallback, firstBufferCallbackId)); } -- cgit v1.2.3-59-g8ed1b From 70670e63cec845ae61fe34059475c1919a2f078a Mon Sep 17 00:00:00 2001 From: Chavi Weingarten Date: Wed, 22 Feb 2023 17:36:40 +0000 Subject: [BBQ] Only update destination frame when buffer matches requested The current code in BBQ won't reject the buffer if the buffer size and last size match, even if requested size doesn't match. This is fine as long as we don't also update the destination frame to the new requested size. We can apply the old buffer, so we don't drop additional buffers, but keep the last destination frame as the last requested size. When we get a new buffer that matches the new requested size, we can update the destination frame. Without this fix, the buffer could be stretched in FREEZE mode since the buffer size will end up not matching the destination frame set in SF. Test: SurfaceSyncGroupContinuousTest Fixes: 267284591 Change-Id: Id8bc8b6e04fa1cc56f7b9eb609cadb208b27b3ea --- libs/gui/BLASTBufferQueue.cpp | 27 ++++++++++++++++++--------- 1 file changed, 18 insertions(+), 9 deletions(-) (limited to 'libs/gui/BLASTBufferQueue.cpp') diff --git a/libs/gui/BLASTBufferQueue.cpp b/libs/gui/BLASTBufferQueue.cpp index 9d82c143f5..cf8b13ffa4 100644 --- a/libs/gui/BLASTBufferQueue.cpp +++ b/libs/gui/BLASTBufferQueue.cpp @@ -486,6 +486,17 @@ void BLASTBufferQueue::releaseBuffer(const ReleaseCallbackId& callbackId, mSyncedFrameNumbers.erase(callbackId.framenumber); } +static ui::Size getBufferSize(const BufferItem& item) { + uint32_t bufWidth = item.mGraphicBuffer->getWidth(); + uint32_t bufHeight = item.mGraphicBuffer->getHeight(); + + // Take the buffer's orientation into account + if (item.mTransform & ui::Transform::ROT_90) { + std::swap(bufWidth, bufHeight); + } + return ui::Size(bufWidth, bufHeight); +} + status_t BLASTBufferQueue::acquireNextBufferLocked( const std::optional transaction) { // Check if we have frames available and we have not acquired the maximum number of buffers. @@ -563,7 +574,12 @@ status_t BLASTBufferQueue::acquireNextBufferLocked( // Ensure BLASTBufferQueue stays alive until we receive the transaction complete callback. incStrong((void*)transactionCallbackThunk); - mSize = mRequestedSize; + // Only update mSize for destination bounds if the incoming buffer matches the requested size. + // Otherwise, it could cause stretching since the destination bounds will update before the + // buffer with the new size is acquired. + if (mRequestedSize == getBufferSize(bufferItem)) { + mSize = mRequestedSize; + } Rect crop = computeCrop(bufferItem); mLastBufferInfo.update(true /* hasBuffer */, bufferItem.mGraphicBuffer->getWidth(), bufferItem.mGraphicBuffer->getHeight(), bufferItem.mTransform, @@ -834,14 +850,7 @@ bool BLASTBufferQueue::rejectBuffer(const BufferItem& item) { return false; } - uint32_t bufWidth = item.mGraphicBuffer->getWidth(); - uint32_t bufHeight = item.mGraphicBuffer->getHeight(); - - // Take the buffer's orientation into account - if (item.mTransform & ui::Transform::ROT_90) { - std::swap(bufWidth, bufHeight); - } - ui::Size bufferSize(bufWidth, bufHeight); + ui::Size bufferSize = getBufferSize(item); if (mRequestedSize != mSize && mRequestedSize == bufferSize) { return false; } -- cgit v1.2.3-59-g8ed1b From c794b698dc3eb5ee27a6d6395ca7fce5e3167bdd Mon Sep 17 00:00:00 2001 From: Brian Lindahl Date: Tue, 31 Jan 2023 15:42:47 -0700 Subject: Increase frame history size when SF buffer queue size changes While the existing frame history size is sufficient for graphics, the buffer queue sizes for video playback are much larger (more pipelined) so in order for accurate frame tracking of video frames, increase the frame event history size based on the buffer queue size (pipeline size), so that buffer queue producers can always track all frames that have been queued. Bug: 234833109 Test: atest DecoderRenderTest Change-Id: Ida587a239a03f74ebb099d8634ff722a500fcdda (cherry picked from commit 957985be86bb17f227ff0c8aefbed7d1d1fe54fe) --- libs/gui/BLASTBufferQueue.cpp | 45 +++++++- libs/gui/BufferQueueProducer.cpp | 9 ++ libs/gui/FrameTimestamps.cpp | 115 +++++++++++++++++++-- libs/gui/bufferqueue/1.0/Conversion.cpp | 15 +-- .../bufferqueue/1.0/H2BGraphicBufferProducer.cpp | 5 +- libs/gui/include/gui/BLASTBufferQueue.h | 5 + libs/gui/include/gui/BufferQueueProducer.h | 5 + libs/gui/include/gui/FrameTimestamps.h | 15 ++- 8 files changed, 187 insertions(+), 27 deletions(-) (limited to 'libs/gui/BLASTBufferQueue.cpp') diff --git a/libs/gui/BLASTBufferQueue.cpp b/libs/gui/BLASTBufferQueue.cpp index cf8b13ffa4..821dd37a85 100644 --- a/libs/gui/BLASTBufferQueue.cpp +++ b/libs/gui/BLASTBufferQueue.cpp @@ -139,6 +139,11 @@ void BLASTBufferItemConsumer::onSidebandStreamChanged() { } } +void BLASTBufferItemConsumer::resizeFrameEventHistory(size_t newSize) { + Mutex::Autolock lock(mMutex); + mFrameEventHistory.resize(newSize); +} + BLASTBufferQueue::BLASTBufferQueue(const std::string& name, bool updateDestinationFrame) : mSurfaceControl(nullptr), mSize(1, 1), @@ -1069,8 +1074,9 @@ public: // can be non-blocking when the producer is in the client process. class BBQBufferQueueProducer : public BufferQueueProducer { public: - BBQBufferQueueProducer(const sp& core) - : BufferQueueProducer(core, false /* consumerIsSurfaceFlinger*/) {} + BBQBufferQueueProducer(const sp& core, wp bbq) + : BufferQueueProducer(core, false /* consumerIsSurfaceFlinger*/), + mBLASTBufferQueue(std::move(bbq)) {} status_t connect(const sp& listener, int api, bool producerControlledByApp, QueueBufferOutput* output) override { @@ -1082,6 +1088,26 @@ public: producerControlledByApp, output); } + // We want to resize the frame history when changing the size of the buffer queue + status_t setMaxDequeuedBufferCount(int maxDequeuedBufferCount) override { + int maxBufferCount; + status_t status = BufferQueueProducer::setMaxDequeuedBufferCount(maxDequeuedBufferCount, + &maxBufferCount); + // if we can't determine the max buffer count, then just skip growing the history size + if (status == OK) { + size_t newFrameHistorySize = maxBufferCount + 2; // +2 because triple buffer rendering + // optimize away resizing the frame history unless it will grow + if (newFrameHistorySize > FrameEventHistory::INITIAL_MAX_FRAME_HISTORY) { + sp bbq = mBLASTBufferQueue.promote(); + if (bbq != nullptr) { + ALOGV("increasing frame history size to %zu", newFrameHistorySize); + bbq->resizeFrameEventHistory(newFrameHistorySize); + } + } + } + return status; + } + int query(int what, int* value) override { if (what == NATIVE_WINDOW_QUEUES_TO_WINDOW_COMPOSER) { *value = 1; @@ -1089,6 +1115,9 @@ public: } return BufferQueueProducer::query(what, value); } + +private: + const wp mBLASTBufferQueue; }; // Similar to BufferQueue::createBufferQueue but creates an adapter specific bufferqueue producer. @@ -1103,7 +1132,7 @@ void BLASTBufferQueue::createBufferQueue(sp* outProducer sp core(new BufferQueueCore()); LOG_ALWAYS_FATAL_IF(core == nullptr, "BLASTBufferQueue: failed to create BufferQueueCore"); - sp producer(new BBQBufferQueueProducer(core)); + sp producer(new BBQBufferQueueProducer(core, this)); LOG_ALWAYS_FATAL_IF(producer == nullptr, "BLASTBufferQueue: failed to create BBQBufferQueueProducer"); @@ -1116,6 +1145,16 @@ void BLASTBufferQueue::createBufferQueue(sp* outProducer *outConsumer = consumer; } +void BLASTBufferQueue::resizeFrameEventHistory(size_t newSize) { + // This can be null during creation of the buffer queue, but resizing won't do anything at that + // point in time, so just ignore. This can go away once the class relationships and lifetimes of + // objects are cleaned up with a major refactor of BufferQueue as a whole. + if (mBufferItemConsumer != nullptr) { + std::unique_lock _lock{mMutex}; + mBufferItemConsumer->resizeFrameEventHistory(newSize); + } +} + PixelFormat BLASTBufferQueue::convertBufferFormat(PixelFormat& format) { PixelFormat convertedFormat = format; switch (format) { diff --git a/libs/gui/BufferQueueProducer.cpp b/libs/gui/BufferQueueProducer.cpp index 5fe5e71db9..9eb1a9f526 100644 --- a/libs/gui/BufferQueueProducer.cpp +++ b/libs/gui/BufferQueueProducer.cpp @@ -119,6 +119,12 @@ status_t BufferQueueProducer::requestBuffer(int slot, sp* buf) { status_t BufferQueueProducer::setMaxDequeuedBufferCount( int maxDequeuedBuffers) { + int maxBufferCount; + return setMaxDequeuedBufferCount(maxDequeuedBuffers, &maxBufferCount); +} + +status_t BufferQueueProducer::setMaxDequeuedBufferCount(int maxDequeuedBuffers, + int* maxBufferCount) { ATRACE_CALL(); BQ_LOGV("setMaxDequeuedBufferCount: maxDequeuedBuffers = %d", maxDequeuedBuffers); @@ -134,6 +140,8 @@ status_t BufferQueueProducer::setMaxDequeuedBufferCount( return NO_INIT; } + *maxBufferCount = mCore->getMaxBufferCountLocked(); + if (maxDequeuedBuffers == mCore->mMaxDequeuedBufferCount) { return NO_ERROR; } @@ -183,6 +191,7 @@ status_t BufferQueueProducer::setMaxDequeuedBufferCount( return BAD_VALUE; } mCore->mMaxDequeuedBufferCount = maxDequeuedBuffers; + *maxBufferCount = mCore->getMaxBufferCountLocked(); VALIDATE_CONSISTENCY(); if (delta < 0) { listener = mCore->mConsumerListener; diff --git a/libs/gui/FrameTimestamps.cpp b/libs/gui/FrameTimestamps.cpp index e2ea3f9ab1..f3eb4e83aa 100644 --- a/libs/gui/FrameTimestamps.cpp +++ b/libs/gui/FrameTimestamps.cpp @@ -168,10 +168,11 @@ struct FrameNumberEqual { } // namespace -const size_t FrameEventHistory::MAX_FRAME_HISTORY = +const size_t FrameEventHistory::INITIAL_MAX_FRAME_HISTORY = sysprop::LibGuiProperties::frame_event_history_size().value_or(8); -FrameEventHistory::FrameEventHistory() : mFrames(std::vector(MAX_FRAME_HISTORY)) {} +FrameEventHistory::FrameEventHistory() + : mFrames(std::vector(INITIAL_MAX_FRAME_HISTORY)) {} FrameEventHistory::~FrameEventHistory() = default; @@ -227,7 +228,6 @@ void FrameEventHistory::dump(std::string& outString) const { } } - // ============================================================================ // ProducerFrameEventHistory // ============================================================================ @@ -273,6 +273,13 @@ void ProducerFrameEventHistory::applyDelta( const FrameEventHistoryDelta& delta) { mCompositorTiming = delta.mCompositorTiming; + // Deltas should have enough reserved capacity for the consumer-side, therefore if there's a + // different capacity, we re-sized on the consumer side and now need to resize on the producer + // side. + if (delta.mDeltas.capacity() > mFrames.capacity()) { + resize(delta.mDeltas.capacity()); + } + for (auto& d : delta.mDeltas) { // Avoid out-of-bounds access. if (CC_UNLIKELY(d.mIndex >= mFrames.size())) { @@ -349,13 +356,48 @@ std::shared_ptr ProducerFrameEventHistory::createFenceTime( return std::make_shared(fence); } +void ProducerFrameEventHistory::resize(size_t newSize) { + // we don't want to drop events by resizing too small, so don't resize in the negative direction + if (newSize <= mFrames.size()) { + return; + } + + // This algorithm for resizing needs to be the same as ConsumerFrameEventHistory::resize, + // because the indexes need to match when communicating the FrameEventDeltas. + + // We need to find the oldest frame, because that frame needs to move to index 0 in the new + // frame history. + size_t oldestFrameIndex = 0; + size_t oldestFrameNumber = INT32_MAX; + for (size_t i = 0; i < mFrames.size(); ++i) { + if (mFrames[i].frameNumber < oldestFrameNumber && mFrames[i].valid) { + oldestFrameNumber = mFrames[i].frameNumber; + oldestFrameIndex = i; + } + } + + // move the existing frame information into a new vector, so that the oldest frames are at + // index 0, and the latest frames are at the end of the vector + std::vector newFrames(newSize); + size_t oldI = oldestFrameIndex; + size_t newI = 0; + do { + if (mFrames[oldI].valid) { + newFrames[newI++] = std::move(mFrames[oldI]); + } + oldI = (oldI + 1) % mFrames.size(); + } while (oldI != oldestFrameIndex); + + mFrames = std::move(newFrames); + mAcquireOffset = 0; // this is just a hint, so setting this to anything is fine +} // ============================================================================ // ConsumerFrameEventHistory // ============================================================================ ConsumerFrameEventHistory::ConsumerFrameEventHistory() - : mFramesDirty(std::vector(MAX_FRAME_HISTORY)) {} + : mFramesDirty(std::vector(INITIAL_MAX_FRAME_HISTORY)) {} ConsumerFrameEventHistory::~ConsumerFrameEventHistory() = default; @@ -489,6 +531,36 @@ void ConsumerFrameEventHistory::getAndResetDelta( } } +void ConsumerFrameEventHistory::resize(size_t newSize) { + // we don't want to drop events by resizing too small, so don't resize in the negative direction + if (newSize <= mFrames.size()) { + return; + } + + // This algorithm for resizing needs to be the same as ProducerFrameEventHistory::resize, + // because the indexes need to match when communicating the FrameEventDeltas. + + // move the existing frame information into a new vector, so that the oldest frames are at + // index 0, and the latest frames are towards the end of the vector + std::vector newFrames(newSize); + std::vector newFramesDirty(newSize); + size_t oldestFrameIndex = mQueueOffset; + size_t oldI = oldestFrameIndex; + size_t newI = 0; + do { + if (mFrames[oldI].valid) { + newFrames[newI] = std::move(mFrames[oldI]); + newFramesDirty[newI] = mFramesDirty[oldI]; + newI += 1; + } + oldI = (oldI + 1) % mFrames.size(); + } while (oldI != oldestFrameIndex); + + mFrames = std::move(newFrames); + mFramesDirty = std::move(newFramesDirty); + mQueueOffset = newI; + mCompositionOffset = 0; // this is just a hint, so setting this to anything is fine +} // ============================================================================ // FrameEventsDelta @@ -558,8 +630,7 @@ status_t FrameEventsDelta::flatten(void*& buffer, size_t& size, int*& fds, return NO_MEMORY; } - if (mIndex >= FrameEventHistory::MAX_FRAME_HISTORY || - mIndex > std::numeric_limits::max()) { + if (mIndex >= UINT8_MAX || mIndex < 0) { return BAD_VALUE; } @@ -601,7 +672,7 @@ status_t FrameEventsDelta::unflatten(void const*& buffer, size_t& size, uint16_t temp16 = 0; FlattenableUtils::read(buffer, size, temp16); mIndex = temp16; - if (mIndex >= FrameEventHistory::MAX_FRAME_HISTORY) { + if (mIndex >= UINT8_MAX) { return BAD_VALUE; } uint8_t temp8 = 0; @@ -627,6 +698,25 @@ status_t FrameEventsDelta::unflatten(void const*& buffer, size_t& size, return NO_ERROR; } +uint64_t FrameEventsDelta::getFrameNumber() const { + return mFrameNumber; +} + +bool FrameEventsDelta::getLatchTime(nsecs_t* latchTime) const { + if (mLatchTime == FrameEvents::TIMESTAMP_PENDING) { + return false; + } + *latchTime = mLatchTime; + return true; +} + +bool FrameEventsDelta::getDisplayPresentFence(sp* fence) const { + if (mDisplayPresentFence.fence == Fence::NO_FENCE) { + return false; + } + *fence = mDisplayPresentFence.fence; + return true; +} // ============================================================================ // FrameEventHistoryDelta @@ -665,7 +755,7 @@ size_t FrameEventHistoryDelta::getFdCount() const { status_t FrameEventHistoryDelta::flatten( void*& buffer, size_t& size, int*& fds, size_t& count) const { - if (mDeltas.size() > FrameEventHistory::MAX_FRAME_HISTORY) { + if (mDeltas.size() > UINT8_MAX) { return BAD_VALUE; } if (size < getFlattenedSize()) { @@ -695,7 +785,7 @@ status_t FrameEventHistoryDelta::unflatten( uint32_t deltaCount = 0; FlattenableUtils::read(buffer, size, deltaCount); - if (deltaCount > FrameEventHistory::MAX_FRAME_HISTORY) { + if (deltaCount > UINT8_MAX) { return BAD_VALUE; } mDeltas.resize(deltaCount); @@ -708,5 +798,12 @@ status_t FrameEventHistoryDelta::unflatten( return NO_ERROR; } +std::vector::const_iterator FrameEventHistoryDelta::begin() const { + return mDeltas.begin(); +} + +std::vector::const_iterator FrameEventHistoryDelta::end() const { + return mDeltas.end(); +} } // namespace android diff --git a/libs/gui/bufferqueue/1.0/Conversion.cpp b/libs/gui/bufferqueue/1.0/Conversion.cpp index 55462c3b1f..96679549b2 100644 --- a/libs/gui/bufferqueue/1.0/Conversion.cpp +++ b/libs/gui/bufferqueue/1.0/Conversion.cpp @@ -725,12 +725,7 @@ status_t unflatten(HGraphicBufferProducer::FrameEventsDelta* t, // These were written as uint8_t for alignment. uint8_t temp = 0; FlattenableUtils::read(buffer, size, temp); - size_t index = static_cast(temp); - if (index >= ::android::FrameEventHistory::MAX_FRAME_HISTORY) { - return BAD_VALUE; - } - t->index = static_cast(index); - + t->index = static_cast(temp); FlattenableUtils::read(buffer, size, temp); t->addPostCompositeCalled = static_cast(temp); FlattenableUtils::read(buffer, size, temp); @@ -786,8 +781,7 @@ status_t unflatten(HGraphicBufferProducer::FrameEventsDelta* t, status_t flatten(HGraphicBufferProducer::FrameEventsDelta const& t, void*& buffer, size_t& size, int*& fds, size_t numFds) { // Check that t.index is within a valid range. - if (t.index >= static_cast(FrameEventHistory::MAX_FRAME_HISTORY) - || t.index > std::numeric_limits::max()) { + if (t.index > UINT8_MAX || t.index < 0) { return BAD_VALUE; } @@ -887,8 +881,7 @@ status_t unflatten( uint32_t deltaCount = 0; FlattenableUtils::read(buffer, size, deltaCount); - if (static_cast(deltaCount) > - ::android::FrameEventHistory::MAX_FRAME_HISTORY) { + if (deltaCount > UINT8_MAX) { return BAD_VALUE; } t->deltas.resize(deltaCount); @@ -919,7 +912,7 @@ status_t unflatten( status_t flatten( HGraphicBufferProducer::FrameEventHistoryDelta const& t, void*& buffer, size_t& size, int*& fds, size_t& numFds) { - if (t.deltas.size() > ::android::FrameEventHistory::MAX_FRAME_HISTORY) { + if (t.deltas.size() > UINT8_MAX) { return BAD_VALUE; } if (size < getFlattenedSize(t)) { diff --git a/libs/gui/bufferqueue/1.0/H2BGraphicBufferProducer.cpp b/libs/gui/bufferqueue/1.0/H2BGraphicBufferProducer.cpp index cee1b811a4..f684874aec 100644 --- a/libs/gui/bufferqueue/1.0/H2BGraphicBufferProducer.cpp +++ b/libs/gui/bufferqueue/1.0/H2BGraphicBufferProducer.cpp @@ -725,8 +725,7 @@ inline status_t flatten(HGraphicBufferProducer::FrameEventsDelta const& t, std::vector* nh, void*& buffer, size_t& size, int*& fds, size_t numFds) { // Check that t.index is within a valid range. - if (t.index >= static_cast(FrameEventHistory::MAX_FRAME_HISTORY) - || t.index > std::numeric_limits::max()) { + if (t.index > UINT8_MAX || t.index < 0) { return BAD_VALUE; } @@ -829,7 +828,7 @@ inline status_t flatten( HGraphicBufferProducer::FrameEventHistoryDelta const& t, std::vector >* nh, void*& buffer, size_t& size, int*& fds, size_t& numFds) { - if (t.deltas.size() > ::android::FrameEventHistory::MAX_FRAME_HISTORY) { + if (t.deltas.size() > UINT8_MAX) { return BAD_VALUE; } if (size < getFlattenedSize(t)) { diff --git a/libs/gui/include/gui/BLASTBufferQueue.h b/libs/gui/include/gui/BLASTBufferQueue.h index b9e06473bf..69e9f8a4fb 100644 --- a/libs/gui/include/gui/BLASTBufferQueue.h +++ b/libs/gui/include/gui/BLASTBufferQueue.h @@ -54,6 +54,8 @@ public: nsecs_t dequeueReadyTime) EXCLUDES(mMutex); void getConnectionEvents(uint64_t frameNumber, bool* needsDisconnect) EXCLUDES(mMutex); + void resizeFrameEventHistory(size_t newSize); + protected: void onSidebandStreamChanged() override EXCLUDES(mMutex); @@ -123,6 +125,7 @@ public: private: friend class BLASTBufferQueueHelper; + friend class BBQBufferQueueProducer; // can't be copied BLASTBufferQueue& operator = (const BLASTBufferQueue& rhs); @@ -130,6 +133,8 @@ private: void createBufferQueue(sp* outProducer, sp* outConsumer); + void resizeFrameEventHistory(size_t newSize); + status_t acquireNextBufferLocked( const std::optional transaction) REQUIRES(mMutex); Rect computeCrop(const BufferItem& item) REQUIRES(mMutex); diff --git a/libs/gui/include/gui/BufferQueueProducer.h b/libs/gui/include/gui/BufferQueueProducer.h index 0ad3075a4d..1d13dab623 100644 --- a/libs/gui/include/gui/BufferQueueProducer.h +++ b/libs/gui/include/gui/BufferQueueProducer.h @@ -202,6 +202,11 @@ public: // See IGraphicBufferProducer::setAutoPrerotation virtual status_t setAutoPrerotation(bool autoPrerotation); +protected: + // see IGraphicsBufferProducer::setMaxDequeuedBufferCount, but with the ability to retrieve the + // total maximum buffer count for the buffer queue (dequeued AND acquired) + status_t setMaxDequeuedBufferCount(int maxDequeuedBuffers, int* maxBufferCount); + private: // This is required by the IBinder::DeathRecipient interface virtual void binderDied(const wp& who); diff --git a/libs/gui/include/gui/FrameTimestamps.h b/libs/gui/include/gui/FrameTimestamps.h index c08a9b1b6c..3d1be4d2eb 100644 --- a/libs/gui/include/gui/FrameTimestamps.h +++ b/libs/gui/include/gui/FrameTimestamps.h @@ -97,9 +97,11 @@ public: void checkFencesForCompletion(); void dump(std::string& outString) const; - static const size_t MAX_FRAME_HISTORY; + static const size_t INITIAL_MAX_FRAME_HISTORY; protected: + void resize(size_t newSize); + std::vector mFrames; CompositorTiming mCompositorTiming; @@ -138,6 +140,8 @@ protected: virtual std::shared_ptr createFenceTime( const sp& fence) const; + void resize(size_t newSize); + size_t mAcquireOffset{0}; // The consumer updates it's timelines in Layer and SurfaceFlinger since @@ -208,6 +212,8 @@ public: void getAndResetDelta(FrameEventHistoryDelta* delta); + void resize(size_t newSize); + private: void getFrameDelta(FrameEventHistoryDelta* delta, const std::vector::iterator& frame); @@ -250,6 +256,10 @@ public: status_t unflatten(void const*& buffer, size_t& size, int const*& fds, size_t& count); + uint64_t getFrameNumber() const; + bool getLatchTime(nsecs_t* latchTime) const; + bool getDisplayPresentFence(sp* fence) const; + private: static constexpr size_t minFlattenedSize(); @@ -310,6 +320,9 @@ public: status_t unflatten(void const*& buffer, size_t& size, int const*& fds, size_t& count); + std::vector::const_iterator begin() const; + std::vector::const_iterator end() const; + private: static constexpr size_t minFlattenedSize(); -- cgit v1.2.3-59-g8ed1b From 5b5f6936cf3105f5fc4f0e517f66d4d4420303da Mon Sep 17 00:00:00 2001 From: Vishnu Nair Date: Wed, 12 Apr 2023 16:28:19 -0700 Subject: [bbq] always update size if scaling mode changes to non freeze Test: presubmit (coming soon :) ) Fixes: 277346192 Change-Id: I7d86cbd794c2bfc5f4a6847c844c6bc81f80bbcf --- libs/gui/BLASTBufferQueue.cpp | 3 ++- libs/gui/tests/BLASTBufferQueue_test.cpp | 9 ++++++--- 2 files changed, 8 insertions(+), 4 deletions(-) (limited to 'libs/gui/BLASTBufferQueue.cpp') diff --git a/libs/gui/BLASTBufferQueue.cpp b/libs/gui/BLASTBufferQueue.cpp index 821dd37a85..fd42b33e49 100644 --- a/libs/gui/BLASTBufferQueue.cpp +++ b/libs/gui/BLASTBufferQueue.cpp @@ -582,7 +582,8 @@ status_t BLASTBufferQueue::acquireNextBufferLocked( // Only update mSize for destination bounds if the incoming buffer matches the requested size. // Otherwise, it could cause stretching since the destination bounds will update before the // buffer with the new size is acquired. - if (mRequestedSize == getBufferSize(bufferItem)) { + if (mRequestedSize == getBufferSize(bufferItem) || + bufferItem.mScalingMode != NATIVE_WINDOW_SCALING_MODE_FREEZE) { mSize = mRequestedSize; } Rect crop = computeCrop(bufferItem); diff --git a/libs/gui/tests/BLASTBufferQueue_test.cpp b/libs/gui/tests/BLASTBufferQueue_test.cpp index cf2593dc81..b9e75be57d 100644 --- a/libs/gui/tests/BLASTBufferQueue_test.cpp +++ b/libs/gui/tests/BLASTBufferQueue_test.cpp @@ -32,6 +32,7 @@ #include #include #include +#include #include #include #include @@ -198,11 +199,13 @@ protected: t.apply(); t.clear(); - ui::DisplayMode mode; - ASSERT_EQ(NO_ERROR, SurfaceComposerClient::getActiveDisplayMode(mDisplayToken, &mode)); - const ui::Size& resolution = mode.resolution; + ui::DisplayState displayState; + ASSERT_EQ(NO_ERROR, SurfaceComposerClient::getDisplayState(mDisplayToken, &displayState)); + const ui::Size& resolution = displayState.layerStackSpaceRect; mDisplayWidth = resolution.getWidth(); mDisplayHeight = resolution.getHeight(); + ALOGV("Display: %dx%d orientation:%d", mDisplayWidth, mDisplayHeight, + displayState.orientation); mSurfaceControl = mClient->createSurface(String8("TestSurface"), mDisplayWidth, mDisplayHeight, PIXEL_FORMAT_RGBA_8888, -- cgit v1.2.3-59-g8ed1b From c398c0100accdcb45907f15579e9068d0bbe0a67 Mon Sep 17 00:00:00 2001 From: Chavi Weingarten Date: Wed, 12 Apr 2023 17:26:02 +0000 Subject: Add explicit clearSyncTransaction instead of passing in null When attempting to clear the last syncNextTransaction, the code was ambiguous and it was unclear what the expectation should be. Instead, don't allow null when calling syncNextTransaction and don't allow the syncTransaction to be overwritten. If the caller wants to clear the syncTransaction before a frame has arrived, they can do so by calling clearSyncTransaction. Test: No warning log in sync with no buffer. Test: BLASTBufferQueueTest Bug: 272189296 Change-Id: I3a945f5503220225f2147b0331d1fb2f9ea8dc63 --- libs/gui/BLASTBufferQueue.cpp | 59 +++++++++++++++++--------------- libs/gui/include/gui/BLASTBufferQueue.h | 3 +- libs/gui/tests/BLASTBufferQueue_test.cpp | 43 +++++++++++++++++++++-- 3 files changed, 74 insertions(+), 31 deletions(-) (limited to 'libs/gui/BLASTBufferQueue.cpp') diff --git a/libs/gui/BLASTBufferQueue.cpp b/libs/gui/BLASTBufferQueue.cpp index 821dd37a85..aeb5406bbd 100644 --- a/libs/gui/BLASTBufferQueue.cpp +++ b/libs/gui/BLASTBufferQueue.cpp @@ -800,34 +800,24 @@ void BLASTBufferQueue::onFrameCancelled(const uint64_t bufferId) { mDequeueTimestamps.erase(bufferId); }; -void BLASTBufferQueue::syncNextTransaction( +bool BLASTBufferQueue::syncNextTransaction( std::function callback, bool acquireSingleBuffer) { - std::function prevCallback = nullptr; - SurfaceComposerClient::Transaction* prevTransaction = nullptr; - - { - std::lock_guard _lock{mMutex}; - BBQ_TRACE(); - // We're about to overwrite the previous call so we should invoke that callback - // immediately. - if (mTransactionReadyCallback) { - prevCallback = mTransactionReadyCallback; - prevTransaction = mSyncTransaction; - } + LOG_ALWAYS_FATAL_IF(!callback, + "BLASTBufferQueue: callback passed in to syncNextTransaction must not be " + "NULL"); - mTransactionReadyCallback = callback; - if (callback) { - mSyncTransaction = new SurfaceComposerClient::Transaction(); - } else { - mSyncTransaction = nullptr; - } - mAcquireSingleBuffer = mTransactionReadyCallback ? acquireSingleBuffer : true; + std::lock_guard _lock{mMutex}; + BBQ_TRACE(); + if (mTransactionReadyCallback) { + ALOGW("Attempting to overwrite transaction callback in syncNextTransaction"); + return false; } - if (prevCallback) { - prevCallback(prevTransaction); - } + mTransactionReadyCallback = callback; + mSyncTransaction = new SurfaceComposerClient::Transaction(); + mAcquireSingleBuffer = acquireSingleBuffer; + return true; } void BLASTBufferQueue::stopContinuousSyncTransaction() { @@ -835,20 +825,35 @@ void BLASTBufferQueue::stopContinuousSyncTransaction() { SurfaceComposerClient::Transaction* prevTransaction = nullptr; { std::lock_guard _lock{mMutex}; - bool invokeCallback = mTransactionReadyCallback && !mAcquireSingleBuffer; - if (invokeCallback) { - prevCallback = mTransactionReadyCallback; - prevTransaction = mSyncTransaction; + if (mAcquireSingleBuffer || !mTransactionReadyCallback) { + ALOGW("Attempting to stop continuous sync when none are active"); + return; } + + prevCallback = mTransactionReadyCallback; + prevTransaction = mSyncTransaction; + mTransactionReadyCallback = nullptr; mSyncTransaction = nullptr; mAcquireSingleBuffer = true; } + if (prevCallback) { prevCallback(prevTransaction); } } +void BLASTBufferQueue::clearSyncTransaction() { + std::lock_guard _lock{mMutex}; + if (!mAcquireSingleBuffer) { + ALOGW("Attempting to clear sync transaction when none are active"); + return; + } + + mTransactionReadyCallback = nullptr; + mSyncTransaction = nullptr; +} + bool BLASTBufferQueue::rejectBuffer(const BufferItem& item) { if (item.mScalingMode != NATIVE_WINDOW_SCALING_MODE_FREEZE) { // Only reject buffers if scaling mode is freeze. diff --git a/libs/gui/include/gui/BLASTBufferQueue.h b/libs/gui/include/gui/BLASTBufferQueue.h index 69e9f8a4fb..a49a85984f 100644 --- a/libs/gui/include/gui/BLASTBufferQueue.h +++ b/libs/gui/include/gui/BLASTBufferQueue.h @@ -97,9 +97,10 @@ public: void releaseBufferCallbackLocked(const ReleaseCallbackId& id, const sp& releaseFence, std::optional currentMaxAcquiredBufferCount, bool fakeRelease) REQUIRES(mMutex); - void syncNextTransaction(std::function callback, + bool syncNextTransaction(std::function callback, bool acquireSingleBuffer = true); void stopContinuousSyncTransaction(); + void clearSyncTransaction(); void mergeWithNextTransaction(SurfaceComposerClient::Transaction* t, uint64_t frameNumber); void applyPendingTransactions(uint64_t frameNumber); diff --git a/libs/gui/tests/BLASTBufferQueue_test.cpp b/libs/gui/tests/BLASTBufferQueue_test.cpp index cf2593dc81..fd69702843 100644 --- a/libs/gui/tests/BLASTBufferQueue_test.cpp +++ b/libs/gui/tests/BLASTBufferQueue_test.cpp @@ -116,15 +116,17 @@ public: mBlastBufferQueueAdapter->syncNextTransaction(callback, acquireSingleBuffer); } - void syncNextTransaction(std::function callback, + bool syncNextTransaction(std::function callback, bool acquireSingleBuffer = true) { - mBlastBufferQueueAdapter->syncNextTransaction(callback, acquireSingleBuffer); + return mBlastBufferQueueAdapter->syncNextTransaction(callback, acquireSingleBuffer); } void stopContinuousSyncTransaction() { mBlastBufferQueueAdapter->stopContinuousSyncTransaction(); } + void clearSyncTransaction() { mBlastBufferQueueAdapter->clearSyncTransaction(); } + int getWidth() { return mBlastBufferQueueAdapter->mSize.width; } int getHeight() { return mBlastBufferQueueAdapter->mSize.height; } @@ -1108,7 +1110,11 @@ TEST_F(BLASTBufferQueueTest, SyncNextTransactionOverwrite) { ASSERT_NE(nullptr, adapter.getTransactionReadyCallback()); auto callback2 = [](Transaction*) {}; - adapter.syncNextTransaction(callback2); + ASSERT_FALSE(adapter.syncNextTransaction(callback2)); + + sp igbProducer; + setUpProducer(adapter, igbProducer); + queueBuffer(igbProducer, 0, 255, 0, 0); std::unique_lock lock(mutex); if (!receivedCallback) { @@ -1120,6 +1126,37 @@ TEST_F(BLASTBufferQueueTest, SyncNextTransactionOverwrite) { ASSERT_TRUE(receivedCallback); } +TEST_F(BLASTBufferQueueTest, ClearSyncTransaction) { + std::mutex mutex; + std::condition_variable callbackReceivedCv; + bool receivedCallback = false; + + BLASTBufferQueueHelper adapter(mSurfaceControl, mDisplayWidth, mDisplayHeight); + ASSERT_EQ(nullptr, adapter.getTransactionReadyCallback()); + auto callback = [&](Transaction*) { + std::unique_lock lock(mutex); + receivedCallback = true; + callbackReceivedCv.notify_one(); + }; + adapter.syncNextTransaction(callback); + ASSERT_NE(nullptr, adapter.getTransactionReadyCallback()); + + adapter.clearSyncTransaction(); + + sp igbProducer; + setUpProducer(adapter, igbProducer); + queueBuffer(igbProducer, 0, 255, 0, 0); + + std::unique_lock lock(mutex); + if (!receivedCallback) { + ASSERT_EQ(callbackReceivedCv.wait_for(lock, std::chrono::seconds(3)), + std::cv_status::timeout) + << "did not receive callback"; + } + + ASSERT_FALSE(receivedCallback); +} + TEST_F(BLASTBufferQueueTest, SyncNextTransactionDropBuffer) { uint8_t r = 255; uint8_t g = 0; -- cgit v1.2.3-59-g8ed1b