From 79468ab6ac9fb866a21408253c7c9c6f9229e2b1 Mon Sep 17 00:00:00 2001 From: chaviw Date: Wed, 27 Oct 2021 11:11:24 -0500 Subject: Allow offscreen mirrored layers to be captured. If an offscreen layer is valid and has content, we should be able to take a screen capture of it. If we're mirroring content, but it's not on screen, we need to ensure updateMirrorInfo is called to get the updated hierarchy. This will allow the mirror content to get properly screenshot Test: screencapture offscreen Test: MirrorLayerTest Test: ScreenCaptureTest Change-Id: I31f806eb616e2d6f800da6328c9878a3e47d6a14 Bug: 188222480 --- services/surfaceflinger/LayerRenderArea.cpp | 14 ++++++++++++++ 1 file changed, 14 insertions(+) (limited to 'services/surfaceflinger/LayerRenderArea.cpp') diff --git a/services/surfaceflinger/LayerRenderArea.cpp b/services/surfaceflinger/LayerRenderArea.cpp index e84508febc..11fe6d0755 100644 --- a/services/surfaceflinger/LayerRenderArea.cpp +++ b/services/surfaceflinger/LayerRenderArea.cpp @@ -94,8 +94,22 @@ void LayerRenderArea::render(std::function drawLayers) { // no need to check rotation because there is none mNeedsFiltering = sourceCrop.width() != getReqWidth() || sourceCrop.height() != getReqHeight(); + // If layer is offscreen, update mirroring info if it exists + if (mLayer->isRemovedFromCurrentState()) { + mLayer->traverse(LayerVector::StateSet::Drawing, + [&](Layer* layer) { layer->updateMirrorInfo(); }); + mLayer->traverse(LayerVector::StateSet::Drawing, + [&](Layer* layer) { layer->updateCloneBufferInfo(); }); + } + if (!mChildrenOnly) { mTransform = mLayer->getTransform().inverse(); + // If the layer is offscreen, compute bounds since we don't compute bounds for offscreen + // layers in a regular cycles. + if (mLayer->isRemovedFromCurrentState()) { + FloatRect maxBounds = mFlinger.getMaxDisplayBounds(); + mLayer->computeBounds(maxBounds, ui::Transform(), 0.f /* shadowRadius */); + } drawLayers(); } else { uint32_t w = static_cast(getWidth()); -- cgit v1.2.3-59-g8ed1b From 7fb9e5a047292114552ba6523bd790269b6cf937 Mon Sep 17 00:00:00 2001 From: Vishnu Nair Date: Mon, 8 Nov 2021 12:44:05 -0800 Subject: SF: Create layers with layerid When recreating layer states from transaction traces we need to create layers with a specific layer id. Add the layer id as part of LayerCreationArgs and pass the struct around instead of individual args. Test: presubmit Bug: 200284593 Change-Id: I029cdb5362d1926deaf2ce64f70a1882a418705b --- services/surfaceflinger/BufferQueueLayer.cpp | 2 +- services/surfaceflinger/BufferQueueLayer.h | 5 + services/surfaceflinger/BufferStateLayer.cpp | 2 +- services/surfaceflinger/Client.cpp | 45 +++---- services/surfaceflinger/ContainerLayer.cpp | 3 +- services/surfaceflinger/EffectLayer.cpp | 3 +- services/surfaceflinger/Layer.cpp | 15 +-- services/surfaceflinger/Layer.h | 10 +- services/surfaceflinger/LayerRenderArea.cpp | 4 +- services/surfaceflinger/MonitoredProducer.cpp | 8 +- services/surfaceflinger/SurfaceFlinger.cpp | 132 +++++---------------- services/surfaceflinger/SurfaceFlinger.h | 44 ++----- .../tests/SurfaceInterceptor_test.cpp | 4 +- .../tests/unittests/CompositionTest.cpp | 5 +- .../tests/unittests/FpsReporterTest.cpp | 3 +- .../tests/unittests/GameModeTest.cpp | 2 +- .../tests/unittests/RefreshRateSelectionTest.cpp | 7 +- .../tests/unittests/SetFrameRateTest.cpp | 6 +- .../tests/unittests/TransactionFrameTracerTest.cpp | 2 +- .../unittests/TransactionSurfaceFrameTest.cpp | 2 +- .../unittests/TunnelModeEnabledReporterTest.cpp | 3 +- .../tests/unittests/mock/MockLayer.h | 2 +- 22 files changed, 99 insertions(+), 210 deletions(-) (limited to 'services/surfaceflinger/LayerRenderArea.cpp') diff --git a/services/surfaceflinger/BufferQueueLayer.cpp b/services/surfaceflinger/BufferQueueLayer.cpp index 28c387e5bd..dec7cc0806 100644 --- a/services/surfaceflinger/BufferQueueLayer.cpp +++ b/services/surfaceflinger/BufferQueueLayer.cpp @@ -523,7 +523,7 @@ void BufferQueueLayer::gatherBufferInfo() { } sp BufferQueueLayer::createClone() { - LayerCreationArgs args(mFlinger.get(), nullptr, mName + " (Mirror)", 0, 0, 0, LayerMetadata()); + LayerCreationArgs args(mFlinger.get(), nullptr, mName + " (Mirror)", 0, LayerMetadata()); args.textureName = mTextureName; sp layer = mFlinger->getFactory().createBufferQueueLayer(args); layer->setInitialValuesForClone(this); diff --git a/services/surfaceflinger/BufferQueueLayer.h b/services/surfaceflinger/BufferQueueLayer.h index dfdb5c055d..c6e0727806 100644 --- a/services/surfaceflinger/BufferQueueLayer.h +++ b/services/surfaceflinger/BufferQueueLayer.h @@ -62,6 +62,11 @@ public: status_t setDefaultBufferProperties(uint32_t w, uint32_t h, PixelFormat format); sp getProducer() const; + void setSizeForTest(uint32_t w, uint32_t h) { + mDrawingState.active_legacy.w = w; + mDrawingState.active_legacy.h = h; + } + protected: void gatherBufferInfo() override; diff --git a/services/surfaceflinger/BufferStateLayer.cpp b/services/surfaceflinger/BufferStateLayer.cpp index b6cbbb658f..88e3fb6744 100644 --- a/services/surfaceflinger/BufferStateLayer.cpp +++ b/services/surfaceflinger/BufferStateLayer.cpp @@ -890,7 +890,7 @@ Rect BufferStateLayer::computeBufferCrop(const State& s) { } sp BufferStateLayer::createClone() { - LayerCreationArgs args(mFlinger.get(), nullptr, mName + " (Mirror)", 0, 0, 0, LayerMetadata()); + LayerCreationArgs args(mFlinger.get(), nullptr, mName + " (Mirror)", 0, LayerMetadata()); args.textureName = mTextureName; sp layer = mFlinger->getFactory().createBufferStateLayer(args); layer->mHwcSlotGenerator = mHwcSlotGenerator; diff --git a/services/surfaceflinger/Client.cpp b/services/surfaceflinger/Client.cpp index 8da2e24aa4..0a8ebec9f3 100644 --- a/services/surfaceflinger/Client.cpp +++ b/services/surfaceflinger/Client.cpp @@ -72,35 +72,28 @@ 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* handle, - sp* gbp, int32_t* outLayerId, - uint32_t* outTransformHint) { +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) { // We rely on createLayer to check permissions. - return mFlinger->createLayer(name, this, w, h, format, flags, std::move(metadata), handle, gbp, - parentHandle, outLayerId, nullptr, outTransformHint); + 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) { - if (mFlinger->authenticateSurfaceTexture(parent) == false) { - ALOGE("failed to authenticate surface texture"); - return BAD_VALUE; - } - - const auto& layer = (static_cast(parent.get()))->getLayer(); - if (layer == nullptr) { - ALOGE("failed to find parent layer"); - return BAD_VALUE; - } - - return mFlinger->createLayer(name, this, w, h, format, flags, std::move(metadata), handle, gbp, - nullptr, outLayerId, layer, 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, diff --git a/services/surfaceflinger/ContainerLayer.cpp b/services/surfaceflinger/ContainerLayer.cpp index 841e79f8af..3ccc229261 100644 --- a/services/surfaceflinger/ContainerLayer.cpp +++ b/services/surfaceflinger/ContainerLayer.cpp @@ -36,8 +36,7 @@ bool ContainerLayer::isVisible() const { sp ContainerLayer::createClone() { sp layer = mFlinger->getFactory().createContainerLayer( - LayerCreationArgs(mFlinger.get(), nullptr, mName + " (Mirror)", 0, 0, 0, - LayerMetadata())); + LayerCreationArgs(mFlinger.get(), nullptr, mName + " (Mirror)", 0, LayerMetadata())); layer->setInitialValuesForClone(this); return layer; } diff --git a/services/surfaceflinger/EffectLayer.cpp b/services/surfaceflinger/EffectLayer.cpp index 86c6b2161c..845176c112 100644 --- a/services/surfaceflinger/EffectLayer.cpp +++ b/services/surfaceflinger/EffectLayer.cpp @@ -136,8 +136,7 @@ ui::Dataspace EffectLayer::getDataSpace() const { sp EffectLayer::createClone() { sp layer = mFlinger->getFactory().createEffectLayer( - LayerCreationArgs(mFlinger.get(), nullptr, mName + " (Mirror)", 0, 0, 0, - LayerMetadata())); + LayerCreationArgs(mFlinger.get(), nullptr, mName + " (Mirror)", 0, LayerMetadata())); layer->setInitialValuesForClone(this); return layer; } diff --git a/services/surfaceflinger/Layer.cpp b/services/surfaceflinger/Layer.cpp index d85e843d83..3d189d681b 100644 --- a/services/surfaceflinger/Layer.cpp +++ b/services/surfaceflinger/Layer.cpp @@ -87,11 +87,12 @@ using gui::WindowInfo; std::atomic Layer::sSequence{1}; Layer::Layer(const LayerCreationArgs& args) - : mFlinger(args.flinger), + : sequence(args.sequence.value_or(sSequence++)), + 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(METADATA_WINDOW_TYPE, 0))), + mLayerCreationFlags(args.flags) { uint32_t layerFlags = 0; if (args.flags & ISurfaceComposerClient::eHidden) layerFlags |= layer_state_t::eLayerHidden; if (args.flags & ISurfaceComposerClient::eOpaque) layerFlags |= layer_state_t::eLayerOpaque; @@ -99,8 +100,6 @@ Layer::Layer(const LayerCreationArgs& args) if (args.flags & ISurfaceComposerClient::eSkipScreenshot) layerFlags |= layer_state_t::eLayerSkipScreenshot; - mDrawingState.active_legacy.w = args.w; - mDrawingState.active_legacy.h = args.h; mDrawingState.flags = layerFlags; mDrawingState.active_legacy.transform.set(0, 0); mDrawingState.crop.makeInvalid(); @@ -185,12 +184,10 @@ Layer::~Layer() { } LayerCreationArgs::LayerCreationArgs(SurfaceFlinger* flinger, sp client, std::string name, - uint32_t w, uint32_t h, uint32_t flags, LayerMetadata metadata) + uint32_t flags, LayerMetadata metadata) : flinger(flinger), client(std::move(client)), name(std::move(name)), - w(w), - h(h), flags(flags), metadata(std::move(metadata)) { IPCThreadState* ipc = IPCThreadState::self(); @@ -887,7 +884,7 @@ bool Layer::setBackgroundColor(const half3& color, float alpha, ui::Dataspace da uint32_t flags = ISurfaceComposerClient::eFXSurfaceEffect; std::string name = mName + "BackgroundColorLayer"; mDrawingState.bgColorLayer = mFlinger->getFactory().createEffectLayer( - LayerCreationArgs(mFlinger.get(), nullptr, std::move(name), 0, 0, flags, + LayerCreationArgs(mFlinger.get(), nullptr, std::move(name), flags, LayerMetadata())); // add to child list diff --git a/services/surfaceflinger/Layer.h b/services/surfaceflinger/Layer.h index b79903d11b..3da07e824d 100644 --- a/services/surfaceflinger/Layer.h +++ b/services/surfaceflinger/Layer.h @@ -85,20 +85,18 @@ class SurfaceFrame; } // namespace frametimeline struct LayerCreationArgs { - LayerCreationArgs(SurfaceFlinger*, sp, std::string name, uint32_t w, uint32_t h, - uint32_t flags, LayerMetadata); + LayerCreationArgs(SurfaceFlinger*, sp, std::string name, uint32_t flags, LayerMetadata); SurfaceFlinger* flinger; const sp client; std::string name; - uint32_t w; - uint32_t h; uint32_t flags; LayerMetadata metadata; pid_t callingPid; uid_t callingUid; uint32_t textureName; + std::optional sequence = std::nullopt; }; class Layer : public virtual RefBase, compositionengine::LayerFE { @@ -879,7 +877,7 @@ public: // Layer serial number. This gives layers an explicit ordering, so we // have a stable sort order when their layer stack and Z-order are // the same. - int32_t sequence{sSequence++}; + const int32_t sequence; bool mPendingHWCDestroy{false}; @@ -1117,6 +1115,8 @@ private: const std::vector getBlurRegions() const; bool mIsAtRoot = false; + + uint32_t mLayerCreationFlags; }; std::ostream& operator<<(std::ostream& stream, const Layer::FrameRate& rate); diff --git a/services/surfaceflinger/LayerRenderArea.cpp b/services/surfaceflinger/LayerRenderArea.cpp index 11fe6d0755..a1e14559e9 100644 --- a/services/surfaceflinger/LayerRenderArea.cpp +++ b/services/surfaceflinger/LayerRenderArea.cpp @@ -112,12 +112,10 @@ void LayerRenderArea::render(std::function drawLayers) { } drawLayers(); } else { - uint32_t w = static_cast(getWidth()); - uint32_t h = static_cast(getHeight()); // In the "childrenOnly" case we reparent the children to a screenshot // layer which has no properties set and which does not draw. sp screenshotParentLayer = mFlinger.getFactory().createContainerLayer( - {&mFlinger, nullptr, "Screenshot Parent"s, w, h, 0, LayerMetadata()}); + {&mFlinger, nullptr, "Screenshot Parent"s, 0, LayerMetadata()}); ReparentForDrawing reparent(mLayer, screenshotParentLayer, sourceCrop); drawLayers(); diff --git a/services/surfaceflinger/MonitoredProducer.cpp b/services/surfaceflinger/MonitoredProducer.cpp index 6b2d745998..df76f50112 100644 --- a/services/surfaceflinger/MonitoredProducer.cpp +++ b/services/surfaceflinger/MonitoredProducer.cpp @@ -33,13 +33,7 @@ MonitoredProducer::MonitoredProducer(const sp& producer, mFlinger(flinger), mLayer(layer) {} -MonitoredProducer::~MonitoredProducer() { - // Remove ourselves from SurfaceFlinger's list. We do this asynchronously - // because we don't know where this destructor is called from. It could be - // called with the mStateLock held, leading to a dead-lock (it actually - // happens). - mFlinger->removeGraphicBufferProducerAsync(onAsBinder()); -} +MonitoredProducer::~MonitoredProducer() {} status_t MonitoredProducer::requestBuffer(int slot, sp* buf) { return mProducer->requestBuffer(slot, buf); diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp index c2dcd70166..5a6a8ce05a 100644 --- a/services/surfaceflinger/SurfaceFlinger.cpp +++ b/services/surfaceflinger/SurfaceFlinger.cpp @@ -904,9 +904,8 @@ bool SurfaceFlinger::authenticateSurfaceTexture( } bool SurfaceFlinger::authenticateSurfaceTextureLocked( - const sp& bufferProducer) const { - sp surfaceTextureBinder(IInterface::asBinder(bufferProducer)); - return mGraphicBufferProducerList.count(surfaceTextureBinder.get()) > 0; + const sp& /* bufferProducer */) const { + return false; } status_t SurfaceFlinger::getSupportedFrameTimestamps( @@ -3355,20 +3354,15 @@ bool SurfaceFlinger::latchBuffers() { } status_t SurfaceFlinger::addClientLayer(const sp& client, const sp& handle, - const sp& gbc, const sp& lbc, - const wp& parent, bool addToRoot, - uint32_t* outTransformHint) { + const sp& lbc, const wp& parent, + bool addToRoot, uint32_t* outTransformHint) { if (mNumLayers >= ISurfaceComposer::MAX_LAYERS) { ALOGE("AddClientLayer failed, mNumLayers (%zu) >= MAX_LAYERS (%zu)", mNumLayers.load(), ISurfaceComposer::MAX_LAYERS); return NO_MEMORY; } - wp initialProducer; - if (gbc != nullptr) { - initialProducer = IInterface::asBinder(gbc); - } - setLayerCreatedState(handle, lbc, parent, initialProducer, addToRoot); + setLayerCreatedState(handle, lbc, parent, addToRoot); // Create a transaction includes the initial parent and producer. Vector states; @@ -3384,7 +3378,9 @@ status_t SurfaceFlinger::addClientLayer(const sp& client, const spattachLayer(handle, lbc); + if (client != nullptr) { + client->attachLayer(handle, lbc); + } return setTransactionState(FrameTimelineInfo{}, states, displays, 0 /* flags */, nullptr, InputWindowCommands{}, -1 /* desiredPresentTime */, @@ -3392,13 +3388,6 @@ status_t SurfaceFlinger::addClientLayer(const sp& client, const sp& binder) { - static_cast(schedule([=] { - Mutex::Autolock lock(mStateLock); - mGraphicBufferProducerList.erase(binder); - })); -} - uint32_t SurfaceFlinger::getTransactionFlags() const { return mTransactionFlags; } @@ -4281,17 +4270,14 @@ status_t SurfaceFlinger::mirrorLayer(const sp& client, const sp sp mirrorLayer; sp mirrorFrom; - std::string layerName = "MirrorRoot"; - { Mutex::Autolock _l(mStateLock); mirrorFrom = fromHandle(mirrorFromHandle).promote(); if (!mirrorFrom) { return NAME_NOT_FOUND; } - - status_t result = createContainerLayer(client, std::move(layerName), -1, -1, 0, - LayerMetadata(), outHandle, &mirrorLayer); + LayerCreationArgs args(this, client, "MirrorRoot", 0, LayerMetadata()); + status_t result = createContainerLayer(args, outHandle, &mirrorLayer); if (result != NO_ERROR) { return result; } @@ -4300,22 +4286,13 @@ status_t SurfaceFlinger::mirrorLayer(const sp& client, const sp } *outLayerId = mirrorLayer->sequence; - return addClientLayer(client, *outHandle, nullptr, mirrorLayer, nullptr, false, - nullptr /* outTransformHint */); + return addClientLayer(client, *outHandle, mirrorLayer /* layer */, nullptr /* parent */, + false /* addAsRoot */, nullptr /* outTransformHint */); } -status_t SurfaceFlinger::createLayer(const String8& name, const sp& client, uint32_t w, - uint32_t h, PixelFormat format, uint32_t flags, - LayerMetadata metadata, sp* handle, - sp* gbp, +status_t SurfaceFlinger::createLayer(LayerCreationArgs& args, sp* outHandle, const sp& parentHandle, int32_t* outLayerId, const sp& parentLayer, uint32_t* outTransformHint) { - if (int32_t(w|h) < 0) { - ALOGE("createLayer() failed, w or h is negative (w=%d, h=%d)", - int(w), int(h)); - return BAD_VALUE; - } - ALOG_ASSERT(parentLayer == nullptr || parentHandle == nullptr, "Expected only one of parentLayer or parentHandle to be non-null. " "Programmer error?"); @@ -4324,40 +4301,22 @@ status_t SurfaceFlinger::createLayer(const String8& name, const sp& clie sp layer; - std::string layerName{name.string()}; - - switch (flags & ISurfaceComposerClient::eFXSurfaceMask) { + switch (args.flags & ISurfaceComposerClient::eFXSurfaceMask) { case ISurfaceComposerClient::eFXSurfaceBufferQueue: case ISurfaceComposerClient::eFXSurfaceBufferState: { - result = createBufferStateLayer(client, std::move(layerName), w, h, flags, - std::move(metadata), handle, &layer); + result = createBufferStateLayer(args, outHandle, &layer); std::atomic* pendingBufferCounter = layer->getPendingBufferCounter(); if (pendingBufferCounter) { std::string counterName = layer->getPendingBufferCounterName(); - mBufferCountTracker.add((*handle)->localBinder(), counterName, + mBufferCountTracker.add((*outHandle)->localBinder(), counterName, pendingBufferCounter); } } break; case ISurfaceComposerClient::eFXSurfaceEffect: - // check if buffer size is set for color layer. - if (w > 0 || h > 0) { - ALOGE("createLayer() failed, w or h cannot be set for color layer (w=%d, h=%d)", - int(w), int(h)); - return BAD_VALUE; - } - - result = createEffectLayer(client, std::move(layerName), w, h, flags, - std::move(metadata), handle, &layer); + result = createEffectLayer(args, outHandle, &layer); break; case ISurfaceComposerClient::eFXSurfaceContainer: - // check if buffer size is set for container layer. - if (w > 0 || h > 0) { - ALOGE("createLayer() failed, w or h cannot be set for container layer (w=%d, h=%d)", - int(w), int(h)); - return BAD_VALUE; - } - result = createContainerLayer(client, std::move(layerName), w, h, flags, - std::move(metadata), handle, &layer); + result = createContainerLayer(args, outHandle, &layer); break; default: result = BAD_VALUE; @@ -4377,7 +4336,7 @@ status_t SurfaceFlinger::createLayer(const String8& name, const sp& clie if (parentLayer != nullptr) { addToRoot = false; } - result = addClientLayer(client, *handle, *gbp, layer, parent, addToRoot, outTransformHint); + result = addClientLayer(args.client, *outHandle, layer, parent, addToRoot, outTransformHint); if (result != NO_ERROR) { return result; } @@ -4387,9 +4346,7 @@ status_t SurfaceFlinger::createLayer(const String8& name, const sp& clie return result; } -status_t SurfaceFlinger::createBufferQueueLayer(const sp& client, std::string name, - uint32_t w, uint32_t h, uint32_t flags, - LayerMetadata metadata, PixelFormat& format, +status_t SurfaceFlinger::createBufferQueueLayer(LayerCreationArgs& args, PixelFormat& format, sp* handle, sp* gbp, sp* outLayer) { @@ -4405,7 +4362,6 @@ status_t SurfaceFlinger::createBufferQueueLayer(const sp& client, std::s } sp layer; - LayerCreationArgs args(this, client, std::move(name), w, h, flags, std::move(metadata)); args.textureName = getNewTexture(); { // Grab the SF state lock during this since it's the only safe way to access @@ -4415,7 +4371,7 @@ status_t SurfaceFlinger::createBufferQueueLayer(const sp& client, std::s layer = getFactory().createBufferQueueLayer(args); } - status_t err = layer->setDefaultBufferProperties(w, h, format); + status_t err = layer->setDefaultBufferProperties(0, 0, format); if (err == NO_ERROR) { *handle = layer->getHandle(); *gbp = layer->getProducer(); @@ -4426,34 +4382,24 @@ status_t SurfaceFlinger::createBufferQueueLayer(const sp& client, std::s return err; } -status_t SurfaceFlinger::createBufferStateLayer(const sp& client, std::string name, - uint32_t w, uint32_t h, uint32_t flags, - LayerMetadata metadata, sp* handle, +status_t SurfaceFlinger::createBufferStateLayer(LayerCreationArgs& args, sp* handle, sp* outLayer) { - LayerCreationArgs args(this, client, std::move(name), w, h, flags, std::move(metadata)); args.textureName = getNewTexture(); - sp layer = getFactory().createBufferStateLayer(args); - *handle = layer->getHandle(); - *outLayer = layer; - + *outLayer = getFactory().createBufferStateLayer(args); + *handle = (*outLayer)->getHandle(); return NO_ERROR; } -status_t SurfaceFlinger::createEffectLayer(const sp& client, std::string name, uint32_t w, - uint32_t h, uint32_t flags, LayerMetadata metadata, - sp* handle, sp* outLayer) { - *outLayer = getFactory().createEffectLayer( - {this, client, std::move(name), w, h, flags, std::move(metadata)}); +status_t SurfaceFlinger::createEffectLayer(LayerCreationArgs& args, sp* handle, + sp* outLayer) { + *outLayer = getFactory().createEffectLayer(args); *handle = (*outLayer)->getHandle(); return NO_ERROR; } -status_t SurfaceFlinger::createContainerLayer(const sp& client, std::string name, - uint32_t w, uint32_t h, uint32_t flags, - LayerMetadata metadata, sp* handle, +status_t SurfaceFlinger::createContainerLayer(LayerCreationArgs& args, sp* handle, sp* outLayer) { - *outLayer = getFactory().createContainerLayer( - {this, client, std::move(name), w, h, flags, std::move(metadata)}); + *outLayer = getFactory().createContainerLayer(args); *handle = (*outLayer)->getHandle(); return NO_ERROR; } @@ -5039,8 +4985,6 @@ void SurfaceFlinger::dumpAllLocked(const DumpArgs& args, std::string& result) co */ colorizer.bold(result); StringAppendF(&result, "Visible layers (count = %zu)\n", mNumLayers.load()); - StringAppendF(&result, "GraphicBufferProducers: %zu, max %zu\n", - mGraphicBufferProducerList.size(), mMaxGraphicBufferProducerListSize); colorizer.reset(result); { @@ -6802,11 +6746,10 @@ void TransactionState::traverseStatesWithBuffers( } void SurfaceFlinger::setLayerCreatedState(const sp& handle, const wp& layer, - const wp parent, const wp& producer, - bool addToRoot) { + const wp parent, bool addToRoot) { Mutex::Autolock lock(mCreatedLayersLock); mCreatedLayers[handle->localBinder()] = - std::make_unique(layer, parent, producer, addToRoot); + std::make_unique(layer, parent, addToRoot); } auto SurfaceFlinger::getLayerCreatedState(const sp& handle) { @@ -6867,19 +6810,6 @@ sp SurfaceFlinger::handleLayerCreatedLocked(const sp& handle) { layer->updateTransformHint(mActiveDisplayTransformHint); - if (state->initialProducer != nullptr) { - mGraphicBufferProducerList.insert(state->initialProducer); - LOG_ALWAYS_FATAL_IF(mGraphicBufferProducerList.size() > mMaxGraphicBufferProducerListSize, - "Suspected IGBP leak: %zu IGBPs (%zu max), %zu Layers", - mGraphicBufferProducerList.size(), mMaxGraphicBufferProducerListSize, - mNumLayers.load()); - if (mGraphicBufferProducerList.size() > mGraphicBufferProducerListSizeLogThreshold) { - ALOGW("Suspected IGBP leak: %zu IGBPs (%zu max), %zu Layers", - mGraphicBufferProducerList.size(), mMaxGraphicBufferProducerListSize, - mNumLayers.load()); - } - } - mInterceptor->saveSurfaceCreation(layer); return layer; } diff --git a/services/surfaceflinger/SurfaceFlinger.h b/services/surfaceflinger/SurfaceFlinger.h index bf628dc309..b432f246c0 100644 --- a/services/surfaceflinger/SurfaceFlinger.h +++ b/services/surfaceflinger/SurfaceFlinger.h @@ -763,29 +763,23 @@ private: /* * Layer management */ - status_t createLayer(const String8& name, const sp& client, uint32_t w, uint32_t h, - PixelFormat format, uint32_t flags, LayerMetadata metadata, - sp* handle, sp* gbp, + status_t createLayer(LayerCreationArgs& args, sp* outHandle, const sp& parentHandle, int32_t* outLayerId, const sp& parentLayer = nullptr, uint32_t* outTransformHint = nullptr); - status_t createBufferQueueLayer(const sp& client, std::string name, uint32_t w, - uint32_t h, uint32_t flags, LayerMetadata metadata, - PixelFormat& format, sp* outHandle, - sp* outGbp, sp* outLayer); + status_t createBufferQueueLayer(LayerCreationArgs& args, PixelFormat& format, + sp* outHandle, sp* outGbp, + sp* outLayer); - status_t createBufferStateLayer(const sp& client, std::string name, uint32_t w, - uint32_t h, uint32_t flags, LayerMetadata metadata, - sp* outHandle, sp* outLayer); + status_t createBufferStateLayer(LayerCreationArgs& args, sp* outHandle, + sp* outLayer); - status_t createEffectLayer(const sp& client, std::string name, uint32_t w, uint32_t h, - uint32_t flags, LayerMetadata metadata, sp* outHandle, + status_t createEffectLayer(LayerCreationArgs& args, sp* outHandle, sp* outLayer); - status_t createContainerLayer(const sp& client, std::string name, uint32_t w, - uint32_t h, uint32_t flags, LayerMetadata metadata, - sp* outHandle, sp* outLayer); + status_t createContainerLayer(LayerCreationArgs& args, sp* outHandle, + sp* outLayer); status_t mirrorLayer(const sp& client, const sp& mirrorFromHandle, sp* outHandle, int32_t* outLayerId); @@ -798,8 +792,7 @@ private: // add a layer to SurfaceFlinger status_t addClientLayer(const sp& client, const sp& handle, - const sp& gbc, const sp& lbc, - const wp& parentLayer, bool addToRoot, + const sp& lbc, const wp& parentLayer, bool addToRoot, uint32_t* outTransformHint); // Traverse through all the layers and compute and cache its bounds. @@ -1115,16 +1108,12 @@ private: float mGlobalSaturationFactor = 1.0f; mat4 mClientColorMatrix; - // Can't be unordered_set because wp<> isn't hashable - std::set> mGraphicBufferProducerList; size_t mMaxGraphicBufferProducerListSize = ISurfaceComposer::MAX_LAYERS; // If there are more GraphicBufferProducers tracked by SurfaceFlinger than // this threshold, then begin logging. size_t mGraphicBufferProducerListSizeLogThreshold = static_cast(0.95 * static_cast(MAX_LAYERS)); - void removeGraphicBufferProducerAsync(const wp&); - // protected by mStateLock (but we could use another lock) bool mLayersRemoved = false; bool mLayersAdded = false; @@ -1339,19 +1328,12 @@ private: GUARDED_BY(mStateLock); mutable Mutex mCreatedLayersLock; struct LayerCreatedState { - LayerCreatedState(const wp& layer, const wp parent, - const wp& producer, bool addToRoot) - : layer(layer), - initialParent(parent), - initialProducer(producer), - addToRoot(addToRoot) {} + LayerCreatedState(const wp& layer, const wp parent, bool addToRoot) + : layer(layer), initialParent(parent), addToRoot(addToRoot) {} wp layer; // Indicates the initial parent of the created layer, only used for creating layer in // SurfaceFlinger. If nullptr, it may add the created layer into the current root layers. wp initialParent; - // Indicates the initial graphic buffer producer of the created layer, only used for - // creating layer in SurfaceFlinger. - wp initialProducer; // Indicates whether the layer getting created should be added at root if there's no parent // and has permission ACCESS_SURFACE_FLINGER. If set to false and no parent, the layer will // be added offscreen. @@ -1362,7 +1344,7 @@ private: // thread. std::unordered_map> mCreatedLayers; void setLayerCreatedState(const sp& handle, const wp& layer, - const wp parent, const wp& producer, bool addToRoot); + const wp parent, bool addToRoot); auto getLayerCreatedState(const sp& handle); sp handleLayerCreatedLocked(const sp& handle) REQUIRES(mStateLock); diff --git a/services/surfaceflinger/tests/SurfaceInterceptor_test.cpp b/services/surfaceflinger/tests/SurfaceInterceptor_test.cpp index 2082c42001..28e8b8c78b 100644 --- a/services/surfaceflinger/tests/SurfaceInterceptor_test.cpp +++ b/services/surfaceflinger/tests/SurfaceInterceptor_test.cpp @@ -754,9 +754,7 @@ void SurfaceInterceptorTest::assertAllUpdatesFound(const Trace& trace) { } bool SurfaceInterceptorTest::surfaceCreationFound(const Increment& increment, bool foundSurface) { - bool isMatch(increment.surface_creation().name() == getUniqueName(LAYER_NAME, increment) && - increment.surface_creation().w() == SIZE_UPDATE && - increment.surface_creation().h() == SIZE_UPDATE); + bool isMatch(increment.surface_creation().name() == getUniqueName(LAYER_NAME, increment)); if (isMatch && !foundSurface) { foundSurface = true; } else if (isMatch && foundSurface) { diff --git a/services/surfaceflinger/tests/unittests/CompositionTest.cpp b/services/surfaceflinger/tests/unittests/CompositionTest.cpp index 40ef6e702d..52d8c35ede 100644 --- a/services/surfaceflinger/tests/unittests/CompositionTest.cpp +++ b/services/surfaceflinger/tests/unittests/CompositionTest.cpp @@ -535,6 +535,7 @@ struct BaseLayerProperties { static void setupLatchedBuffer(CompositionTest* test, sp layer) { // TODO: Eliminate the complexity of actually creating a buffer + layer->setSizeForTest(LayerProperties::WIDTH, LayerProperties::HEIGHT); status_t err = layer->setDefaultBufferProperties(LayerProperties::WIDTH, LayerProperties::HEIGHT, LayerProperties::FORMAT); @@ -901,7 +902,6 @@ struct EffectLayerVariant : public BaseLayerVariant { FlingerLayerType layer = Base::template createLayerWithFactory(test, [test]() { return new EffectLayer( LayerCreationArgs(test->mFlinger.flinger(), sp(), "test-layer", - LayerProperties::WIDTH, LayerProperties::HEIGHT, LayerProperties::LAYER_FLAGS, LayerMetadata())); }); @@ -940,7 +940,6 @@ struct BufferLayerVariant : public BaseLayerVariant { FlingerLayerType layer = Base::template createLayerWithFactory(test, [test]() { LayerCreationArgs args(test->mFlinger.flinger(), sp(), "test-layer", - LayerProperties::WIDTH, LayerProperties::HEIGHT, LayerProperties::LAYER_FLAGS, LayerMetadata()); args.textureName = test->mFlinger.mutableTexturePool().back(); return new BufferQueueLayer(args); @@ -952,7 +951,6 @@ struct BufferLayerVariant : public BaseLayerVariant { } static void cleanupInjectedLayers(CompositionTest* test) { - EXPECT_CALL(*test->mMessageQueue, postMessage(_)).Times(1); Base::cleanupInjectedLayers(test); } @@ -990,7 +988,6 @@ struct ContainerLayerVariant : public BaseLayerVariant { static FlingerLayerType createLayer(CompositionTest* test) { LayerCreationArgs args(test->mFlinger.flinger(), sp(), "test-container-layer", - LayerProperties::WIDTH, LayerProperties::HEIGHT, LayerProperties::LAYER_FLAGS, LayerMetadata()); FlingerLayerType layer = new ContainerLayer(args); Base::template initLayerDrawingStateAndComputeBounds(test, layer); diff --git a/services/surfaceflinger/tests/unittests/FpsReporterTest.cpp b/services/surfaceflinger/tests/unittests/FpsReporterTest.cpp index 010c675574..cd2fc7426e 100644 --- a/services/surfaceflinger/tests/unittests/FpsReporterTest.cpp +++ b/services/surfaceflinger/tests/unittests/FpsReporterTest.cpp @@ -114,8 +114,7 @@ FpsReporterTest::~FpsReporterTest() { sp FpsReporterTest::createBufferStateLayer(LayerMetadata metadata = {}) { sp client; - LayerCreationArgs args(mFlinger.flinger(), client, "buffer-state-layer", WIDTH, HEIGHT, - LAYER_FLAGS, metadata); + LayerCreationArgs args(mFlinger.flinger(), client, "buffer-state-layer", LAYER_FLAGS, metadata); return new BufferStateLayer(args); } diff --git a/services/surfaceflinger/tests/unittests/GameModeTest.cpp b/services/surfaceflinger/tests/unittests/GameModeTest.cpp index 3fa1a2c2f5..d6459429fb 100644 --- a/services/surfaceflinger/tests/unittests/GameModeTest.cpp +++ b/services/surfaceflinger/tests/unittests/GameModeTest.cpp @@ -53,7 +53,7 @@ public: sp createBufferStateLayer() { sp client; - LayerCreationArgs args(mFlinger.flinger(), client, "buffer-state-layer", 100, 100, 0, + LayerCreationArgs args(mFlinger.flinger(), client, "buffer-state-layer", 0, LayerMetadata()); return new BufferStateLayer(args); } diff --git a/services/surfaceflinger/tests/unittests/RefreshRateSelectionTest.cpp b/services/surfaceflinger/tests/unittests/RefreshRateSelectionTest.cpp index e388a6f40f..1e6e3361b2 100644 --- a/services/surfaceflinger/tests/unittests/RefreshRateSelectionTest.cpp +++ b/services/surfaceflinger/tests/unittests/RefreshRateSelectionTest.cpp @@ -91,15 +91,14 @@ RefreshRateSelectionTest::~RefreshRateSelectionTest() { sp RefreshRateSelectionTest::createBufferStateLayer() { sp client; - LayerCreationArgs args(mFlinger.flinger(), client, "buffer-queue-layer", WIDTH, HEIGHT, - LAYER_FLAGS, LayerMetadata()); + LayerCreationArgs args(mFlinger.flinger(), client, "buffer-queue-layer", LAYER_FLAGS, + LayerMetadata()); return new BufferStateLayer(args); } sp RefreshRateSelectionTest::createEffectLayer() { sp client; - LayerCreationArgs args(mFlinger.flinger(), client, "color-layer", WIDTH, HEIGHT, LAYER_FLAGS, - LayerMetadata()); + LayerCreationArgs args(mFlinger.flinger(), client, "color-layer", LAYER_FLAGS, LayerMetadata()); return new EffectLayer(args); } diff --git a/services/surfaceflinger/tests/unittests/SetFrameRateTest.cpp b/services/surfaceflinger/tests/unittests/SetFrameRateTest.cpp index d0211780ab..360f9c684f 100644 --- a/services/surfaceflinger/tests/unittests/SetFrameRateTest.cpp +++ b/services/surfaceflinger/tests/unittests/SetFrameRateTest.cpp @@ -70,8 +70,8 @@ public: std::string name() override { return "BufferStateLayer"; } sp createLayer(TestableSurfaceFlinger& flinger) override { sp client; - LayerCreationArgs args(flinger.flinger(), client, "buffer-state-layer", WIDTH, HEIGHT, - LAYER_FLAGS, LayerMetadata()); + LayerCreationArgs args(flinger.flinger(), client, "buffer-state-layer", LAYER_FLAGS, + LayerMetadata()); return new BufferStateLayer(args); } }; @@ -81,7 +81,7 @@ public: std::string name() override { return "EffectLayer"; } sp createLayer(TestableSurfaceFlinger& flinger) override { sp client; - LayerCreationArgs args(flinger.flinger(), client, "color-layer", WIDTH, HEIGHT, LAYER_FLAGS, + LayerCreationArgs args(flinger.flinger(), client, "color-layer", LAYER_FLAGS, LayerMetadata()); return new EffectLayer(args); } diff --git a/services/surfaceflinger/tests/unittests/TransactionFrameTracerTest.cpp b/services/surfaceflinger/tests/unittests/TransactionFrameTracerTest.cpp index bd6a7805ec..deeb785bb9 100644 --- a/services/surfaceflinger/tests/unittests/TransactionFrameTracerTest.cpp +++ b/services/surfaceflinger/tests/unittests/TransactionFrameTracerTest.cpp @@ -57,7 +57,7 @@ public: sp createBufferStateLayer() { sp client; - LayerCreationArgs args(mFlinger.flinger(), client, "buffer-state-layer", 100, 100, 0, + LayerCreationArgs args(mFlinger.flinger(), client, "buffer-state-layer", 0, LayerMetadata()); return new BufferStateLayer(args); } diff --git a/services/surfaceflinger/tests/unittests/TransactionSurfaceFrameTest.cpp b/services/surfaceflinger/tests/unittests/TransactionSurfaceFrameTest.cpp index bf69704210..704340deac 100644 --- a/services/surfaceflinger/tests/unittests/TransactionSurfaceFrameTest.cpp +++ b/services/surfaceflinger/tests/unittests/TransactionSurfaceFrameTest.cpp @@ -57,7 +57,7 @@ public: sp createBufferStateLayer() { sp client; - LayerCreationArgs args(mFlinger.flinger(), client, "buffer-state-layer", 100, 100, 0, + LayerCreationArgs args(mFlinger.flinger(), client, "buffer-state-layer", 0, LayerMetadata()); return new BufferStateLayer(args); } diff --git a/services/surfaceflinger/tests/unittests/TunnelModeEnabledReporterTest.cpp b/services/surfaceflinger/tests/unittests/TunnelModeEnabledReporterTest.cpp index e4f74694f7..ade4fbb850 100644 --- a/services/surfaceflinger/tests/unittests/TunnelModeEnabledReporterTest.cpp +++ b/services/surfaceflinger/tests/unittests/TunnelModeEnabledReporterTest.cpp @@ -100,8 +100,7 @@ TunnelModeEnabledReporterTest::~TunnelModeEnabledReporterTest() { sp TunnelModeEnabledReporterTest::createBufferStateLayer( LayerMetadata metadata = {}) { sp client; - LayerCreationArgs args(mFlinger.flinger(), client, "buffer-state-layer", WIDTH, HEIGHT, - LAYER_FLAGS, metadata); + LayerCreationArgs args(mFlinger.flinger(), client, "buffer-state-layer", LAYER_FLAGS, metadata); return new BufferStateLayer(args); } diff --git a/services/surfaceflinger/tests/unittests/mock/MockLayer.h b/services/surfaceflinger/tests/unittests/mock/MockLayer.h index ba2e4db0fa..8b48e1c16d 100644 --- a/services/surfaceflinger/tests/unittests/mock/MockLayer.h +++ b/services/surfaceflinger/tests/unittests/mock/MockLayer.h @@ -25,7 +25,7 @@ namespace android::mock { class MockLayer : public Layer { public: MockLayer(SurfaceFlinger* flinger, std::string name) - : Layer(LayerCreationArgs(flinger, nullptr, std::move(name), 800, 600, 0, {})) {} + : Layer(LayerCreationArgs(flinger, nullptr, std::move(name), 0, {})) {} explicit MockLayer(SurfaceFlinger* flinger) : MockLayer(flinger, "TestLayer") {} MOCK_CONST_METHOD0(getType, const char*()); -- cgit v1.2.3-59-g8ed1b From 22ceeaaff9be3e71c5473aeabccdb02705565190 Mon Sep 17 00:00:00 2001 From: Robert Carr Date: Tue, 8 Mar 2022 13:13:22 -0800 Subject: SurfaceFlinger/LayerRenderArea: Hold lock while modifying hierarchy Multiple stability reports from OEMs are reporting a crash in LayerRenderArea. In particular this occurs when destroying the screenshotParentLayer container layer used by LayerRenderArea. This code executes on the main thread without a lock. Our rules for locking in SurfaceFlinger are like this: 1. The main thread will hold the lock when writing to the state, and be the only writer 2. Anyone can read the state if they hold the lock 3. Main thread can read from the state without the lock since it is the only writer. LayerRenderArea's reparentForDrawing operation violates these rules by updating mDrawingParent without holding the lock. If there were some other binder thread which was accessing the hierarchy (say calling getAlpha on a layer in the hierarchy descending from our screenshot layer), we could have a sequence like this: 1. Other thread calls mDrawingParent.promote(), on the wp we provided constructing ReparentForDrawing 2. At the same time we destroy ReparentForDrawing and overwrite the wp 3. This causes wp::~wp and wp::promote to interleave 4. While the wp counters themselves are atomic, remember the object itself is not locked, and so after ~wp the counters that we think are stored in our ~wp could be anything and so we could promote without properly incremeting the reference ccount 5. When the sp returned from promote is destroyed, it could then destroy the layer, and when our original sp from LayerRenderArea is destroyed, we then crash (which is what we see in the traces). It's hard to say what this other thread is. Over time we have tried to move usage of the Layer hierarchy off the Binder threads to achieve a totally lock-free model. At the moment, I can't find spots in tip-of-tree which would race with mDrawingParent in this fashion. The reports are from S, and from OEMs only so it's possible it relates to an OEM modification, or an issue we have already fixed. Despite this we should have a consistent locking model, and until we are ready to get rid of mStateLock, apply it's usage consistently with our three rules of locking. Bug: 220176775 Bug: 223069308 Bug: 223081111 Change-Id: I89c5add585f96b7de1f1b21f76b6ea0c6b0de127 --- services/surfaceflinger/LayerRenderArea.cpp | 26 ++++++++++++++------------ services/surfaceflinger/SurfaceFlinger.h | 1 + 2 files changed, 15 insertions(+), 12 deletions(-) (limited to 'services/surfaceflinger/LayerRenderArea.cpp') diff --git a/services/surfaceflinger/LayerRenderArea.cpp b/services/surfaceflinger/LayerRenderArea.cpp index a1e14559e9..896f25404d 100644 --- a/services/surfaceflinger/LayerRenderArea.cpp +++ b/services/surfaceflinger/LayerRenderArea.cpp @@ -26,18 +26,12 @@ namespace android { namespace { -struct ReparentForDrawing { - const sp& oldParent; - - ReparentForDrawing(const sp& oldParent, const sp& newParent, - const Rect& drawingBounds) - : oldParent(oldParent) { +void reparentForDrawing(const sp& oldParent, const sp& newParent, + const Rect& drawingBounds) { // Compute and cache the bounds for the new parent layer. newParent->computeBounds(drawingBounds.toFloatRect(), ui::Transform(), - 0.f /* shadowRadius */); + 0.f /* shadowRadius */); oldParent->setChildrenDrawingParent(newParent); - } - ~ReparentForDrawing() { oldParent->setChildrenDrawingParent(oldParent); } }; } // namespace @@ -114,11 +108,19 @@ void LayerRenderArea::render(std::function drawLayers) { } else { // In the "childrenOnly" case we reparent the children to a screenshot // layer which has no properties set and which does not draw. + // We hold the statelock as the reparent-for-drawing operation modifies the + // hierarchy and there could be readers on Binder threads, like dump. sp screenshotParentLayer = mFlinger.getFactory().createContainerLayer( - {&mFlinger, nullptr, "Screenshot Parent"s, 0, LayerMetadata()}); - - ReparentForDrawing reparent(mLayer, screenshotParentLayer, sourceCrop); + {&mFlinger, nullptr, "Screenshot Parent"s, 0, LayerMetadata()}); + { + Mutex::Autolock _l(mFlinger.mStateLock); + reparentForDrawing(mLayer, screenshotParentLayer, sourceCrop); + } drawLayers(); + { + Mutex::Autolock _l(mFlinger.mStateLock); + mLayer->setChildrenDrawingParent(mLayer); + } } } diff --git a/services/surfaceflinger/SurfaceFlinger.h b/services/surfaceflinger/SurfaceFlinger.h index 58298387c1..c852c07638 100644 --- a/services/surfaceflinger/SurfaceFlinger.h +++ b/services/surfaceflinger/SurfaceFlinger.h @@ -380,6 +380,7 @@ private: friend class MonitoredProducer; friend class RefreshRateOverlay; friend class RegionSamplingThread; + friend class LayerRenderArea; friend class LayerTracing; // For unit tests -- cgit v1.2.3-59-g8ed1b