SF: Frontend fixes for issues surfaced by SurfaceFlinger_test
- separate color and bgcolor in layer state so we don't set a color
on a layer without a buffer
- avoid using legacy layer state when latching a buffer
- fix callback issue where invisible or offscreen layers were not invoking
the callbacks
- pass in layer snapshot for trusted presentation state
- fix a screenshot issue where the root layer was skipped
- pass in framerate changes to layer history
Test: presubmit
Test: atest SurfaceFlinger_test with new fe
Bug: 238781169
Change-Id: Id9ff8a036dc283e21a05985c1c01ebd070b1df24
diff --git a/services/surfaceflinger/FrontEnd/LayerCreationArgs.cpp b/services/surfaceflinger/FrontEnd/LayerCreationArgs.cpp
index 6d492c0..6659825 100644
--- a/services/surfaceflinger/FrontEnd/LayerCreationArgs.cpp
+++ b/services/surfaceflinger/FrontEnd/LayerCreationArgs.cpp
@@ -24,9 +24,13 @@
std::atomic<uint32_t> LayerCreationArgs::sSequence{1};
+uint32_t LayerCreationArgs::getInternalLayerId(uint32_t id) {
+ return id | INTERNAL_LAYER_PREFIX;
+}
+
LayerCreationArgs::LayerCreationArgs(SurfaceFlinger* flinger, sp<Client> client, std::string name,
uint32_t flags, gui::LayerMetadata metadataArg,
- std::optional<uint32_t> id)
+ std::optional<uint32_t> id, bool internalLayer)
: flinger(flinger),
client(std::move(client)),
name(std::move(name)),
@@ -46,10 +50,15 @@
if (id) {
sequence = *id;
- sSequence = *id + 1;
+ if (internalLayer) {
+ sequence = getInternalLayerId(*id);
+ } else {
+ sSequence = *id + 1;
+ }
} else {
sequence = sSequence++;
- if (sequence == UNASSIGNED_LAYER_ID) {
+ if (sequence >= INTERNAL_LAYER_PREFIX) {
+ sSequence = 1;
ALOGW("Layer sequence id rolled over.");
sequence = sSequence++;
}
diff --git a/services/surfaceflinger/FrontEnd/LayerCreationArgs.h b/services/surfaceflinger/FrontEnd/LayerCreationArgs.h
index 9d2aaab..2cd6b55 100644
--- a/services/surfaceflinger/FrontEnd/LayerCreationArgs.h
+++ b/services/surfaceflinger/FrontEnd/LayerCreationArgs.h
@@ -25,6 +25,7 @@
#include <optional>
constexpr uint32_t UNASSIGNED_LAYER_ID = std::numeric_limits<uint32_t>::max();
+constexpr uint32_t INTERNAL_LAYER_PREFIX = 1u << 31;
namespace android {
class SurfaceFlinger;
@@ -35,10 +36,11 @@
struct LayerCreationArgs {
static std::atomic<uint32_t> sSequence;
+ static uint32_t getInternalLayerId(uint32_t id);
LayerCreationArgs(android::SurfaceFlinger*, sp<android::Client>, std::string name,
- uint32_t flags, gui::LayerMetadata,
- std::optional<uint32_t> id = std::nullopt);
+ uint32_t flags, gui::LayerMetadata, std::optional<uint32_t> id = std::nullopt,
+ bool internalLayer = false);
LayerCreationArgs(const LayerCreationArgs&);
android::SurfaceFlinger* flinger;
diff --git a/services/surfaceflinger/FrontEnd/LayerHierarchy.h b/services/surfaceflinger/FrontEnd/LayerHierarchy.h
index 2ab897b..0f700a9 100644
--- a/services/surfaceflinger/FrontEnd/LayerHierarchy.h
+++ b/services/surfaceflinger/FrontEnd/LayerHierarchy.h
@@ -128,12 +128,18 @@
// Traverse the hierarchy and visit all child variants.
void traverse(const Visitor& visitor) const {
TraversalPath root = TraversalPath::ROOT;
+ if (mLayer) {
+ root.id = mLayer->id;
+ }
traverse(visitor, root);
}
// Traverse the hierarchy in z-order, skipping children that have relative parents.
void traverseInZOrder(const Visitor& visitor) const {
TraversalPath root = TraversalPath::ROOT;
+ if (mLayer) {
+ root.id = mLayer->id;
+ }
traverseInZOrder(visitor, root);
}
diff --git a/services/surfaceflinger/FrontEnd/LayerLifecycleManager.cpp b/services/surfaceflinger/FrontEnd/LayerLifecycleManager.cpp
index 66197be..79ffcbf 100644
--- a/services/surfaceflinger/FrontEnd/LayerLifecycleManager.cpp
+++ b/services/surfaceflinger/FrontEnd/LayerLifecycleManager.cpp
@@ -192,35 +192,38 @@
layer->merge(resolvedComposerState);
if (layer->what & layer_state_t::eBackgroundColorChanged) {
- if (layer->bgColorLayerId == UNASSIGNED_LAYER_ID && layer->bgColorAlpha != 0) {
+ if (layer->bgColorLayerId == UNASSIGNED_LAYER_ID && layer->bgColor.a != 0) {
LayerCreationArgs backgroundLayerArgs{nullptr,
nullptr,
layer->name + "BackgroundColorLayer",
ISurfaceComposerClient::eFXSurfaceEffect,
- {}};
+ {},
+ layer->id,
+ /*internalLayer=*/true};
std::vector<std::unique_ptr<RequestedLayerState>> newLayers;
newLayers.emplace_back(
std::make_unique<RequestedLayerState>(backgroundLayerArgs));
RequestedLayerState* backgroundLayer = newLayers.back().get();
+ backgroundLayer->bgColorLayer = true;
backgroundLayer->handleAlive = false;
backgroundLayer->parentId = layer->id;
backgroundLayer->z = std::numeric_limits<int32_t>::min();
- backgroundLayer->color.rgb = layer->color.rgb;
- backgroundLayer->color.a = layer->bgColorAlpha;
+ backgroundLayer->color = layer->bgColor;
backgroundLayer->dataspace = layer->bgColorDataspace;
-
layer->bgColorLayerId = backgroundLayer->id;
addLayers({std::move(newLayers)});
- } else if (layer->bgColorLayerId != UNASSIGNED_LAYER_ID &&
- layer->bgColorAlpha == 0) {
+ } else if (layer->bgColorLayerId != UNASSIGNED_LAYER_ID && layer->bgColor.a == 0) {
RequestedLayerState* bgColorLayer = getLayerFromId(layer->bgColorLayerId);
- bgColorLayer->parentId = UNASSIGNED_LAYER_ID;
- onHandlesDestroyed({layer->bgColorLayerId});
+ layer->bgColorLayerId = UNASSIGNED_LAYER_ID;
+ bgColorLayer->parentId = unlinkLayer(bgColorLayer->parentId, bgColorLayer->id);
+ onHandlesDestroyed({bgColorLayer->id});
} else if (layer->bgColorLayerId != UNASSIGNED_LAYER_ID) {
RequestedLayerState* bgColorLayer = getLayerFromId(layer->bgColorLayerId);
- bgColorLayer->color.rgb = layer->color.rgb;
- bgColorLayer->color.a = layer->bgColorAlpha;
+ bgColorLayer->color = layer->bgColor;
bgColorLayer->dataspace = layer->bgColorDataspace;
+ bgColorLayer->what |= layer_state_t::eColorChanged |
+ layer_state_t::eDataspaceChanged | layer_state_t::eAlphaChanged;
+ bgColorLayer->changes |= RequestedLayerState::Changes::Content;
mGlobalChanges |= RequestedLayerState::Changes::Content;
}
}
@@ -256,8 +259,7 @@
listener->onLayerAdded(*layer);
}
}
- layer->what = 0;
- layer->changes.clear();
+ layer->clearChanges();
}
for (auto& destroyedLayer : mDestroyedLayers) {
diff --git a/services/surfaceflinger/FrontEnd/LayerSnapshot.cpp b/services/surfaceflinger/FrontEnd/LayerSnapshot.cpp
index dbb7fbf..f866175 100644
--- a/services/surfaceflinger/FrontEnd/LayerSnapshot.cpp
+++ b/services/surfaceflinger/FrontEnd/LayerSnapshot.cpp
@@ -139,7 +139,8 @@
// visible
std::stringstream reason;
if (sidebandStream != nullptr) reason << " sidebandStream";
- if (externalTexture != nullptr) reason << " buffer";
+ if (externalTexture != nullptr)
+ reason << " buffer:" << externalTexture->getId() << " frame:" << frameNumber;
if (fillsColor() || color.a > 0.0f) reason << " color{" << color << "}";
if (drawShadows()) reason << " shadowSettings.length=" << shadowSettings.length;
if (backgroundBlurRadius > 0) reason << " backgroundBlurRadius=" << backgroundBlurRadius;
diff --git a/services/surfaceflinger/FrontEnd/LayerSnapshotBuilder.cpp b/services/surfaceflinger/FrontEnd/LayerSnapshotBuilder.cpp
index d740350..c7f9aa7 100644
--- a/services/surfaceflinger/FrontEnd/LayerSnapshotBuilder.cpp
+++ b/services/surfaceflinger/FrontEnd/LayerSnapshotBuilder.cpp
@@ -337,12 +337,12 @@
LayerSnapshotBuilder::LayerSnapshotBuilder() : mRootSnapshot(getRootSnapshot()) {}
LayerSnapshotBuilder::LayerSnapshotBuilder(Args args) : LayerSnapshotBuilder() {
- args.forceUpdate = true;
+ args.forceUpdate = ForceUpdateFlags::ALL;
updateSnapshots(args);
}
bool LayerSnapshotBuilder::tryFastUpdate(const Args& args) {
- if (args.forceUpdate || args.displayChanges) {
+ if (args.forceUpdate != ForceUpdateFlags::NONE || args.displayChanges) {
// force update requested, or we have display changes, so skip the fast path
return false;
}
@@ -393,19 +393,31 @@
ATRACE_NAME("UpdateSnapshots");
if (args.parentCrop) {
mRootSnapshot.geomLayerBounds = *args.parentCrop;
- } else if (args.forceUpdate || args.displayChanges) {
+ } else if (args.forceUpdate == ForceUpdateFlags::ALL || args.displayChanges) {
mRootSnapshot.geomLayerBounds = getMaxDisplayBounds(args.displays);
}
if (args.displayChanges) {
mRootSnapshot.changes = RequestedLayerState::Changes::AffectsChildren |
RequestedLayerState::Changes::Geometry;
}
+ if (args.forceUpdate == ForceUpdateFlags::HIERARCHY) {
+ mRootSnapshot.changes |=
+ RequestedLayerState::Changes::Hierarchy | RequestedLayerState::Changes::Visibility;
+ }
LayerHierarchy::TraversalPath root = LayerHierarchy::TraversalPath::ROOT;
- for (auto& [childHierarchy, variant] : args.root.mChildren) {
- LayerHierarchy::ScopedAddToTraversalPath addChildToPath(root,
- childHierarchy->getLayer()->id,
- variant);
- updateSnapshotsInHierarchy(args, *childHierarchy, root, mRootSnapshot);
+ if (args.root.getLayer()) {
+ // The hierarchy can have a root layer when used for screenshots otherwise, it will have
+ // multiple children.
+ LayerHierarchy::ScopedAddToTraversalPath addChildToPath(root, args.root.getLayer()->id,
+ LayerHierarchy::Variant::Attached);
+ updateSnapshotsInHierarchy(args, args.root, root, mRootSnapshot);
+ } else {
+ for (auto& [childHierarchy, variant] : args.root.mChildren) {
+ LayerHierarchy::ScopedAddToTraversalPath addChildToPath(root,
+ childHierarchy->getLayer()->id,
+ variant);
+ updateSnapshotsInHierarchy(args, *childHierarchy, root, mRootSnapshot);
+ }
}
sortSnapshotsByZ(args);
@@ -451,6 +463,7 @@
if (newSnapshot) {
snapshot = createSnapshot(traversalPath, *layer);
}
+ scheduler::LayerInfo::FrameRate oldFrameRate = snapshot->frameRate;
if (traversalPath.isRelative()) {
bool parentIsRelative = traversalPath.variant == LayerHierarchy::Variant::Relative;
updateRelativeState(*snapshot, parentSnapshot, parentIsRelative, args);
@@ -469,6 +482,10 @@
updateSnapshotsInHierarchy(args, *childHierarchy, traversalPath, *snapshot);
updateChildState(*snapshot, childSnapshot, args);
}
+
+ if (oldFrameRate == snapshot->frameRate) {
+ snapshot->changes.clear(RequestedLayerState::Changes::FrameRate);
+ }
return *snapshot;
}
@@ -495,7 +512,7 @@
}
void LayerSnapshotBuilder::sortSnapshotsByZ(const Args& args) {
- if (!mResortSnapshots && !args.forceUpdate &&
+ if (!mResortSnapshots && args.forceUpdate == ForceUpdateFlags::NONE &&
!args.layerLifecycleManager.getGlobalChanges().any(
RequestedLayerState::Changes::Hierarchy |
RequestedLayerState::Changes::Visibility)) {
@@ -569,7 +586,8 @@
if (snapshot.childState.hasValidFrameRate) {
return;
}
- if (args.forceUpdate || childSnapshot.changes.test(RequestedLayerState::Changes::FrameRate)) {
+ if (args.forceUpdate == ForceUpdateFlags::ALL ||
+ childSnapshot.changes.test(RequestedLayerState::Changes::FrameRate)) {
// We return whether this layer ot its children has a vote. We ignore ExactOrMultiple votes
// for the same reason we are allowing touch boost for those layers. See
// RefreshRateSelector::rankFrameRates for details.
@@ -618,21 +636,20 @@
ftl::Flags<RequestedLayerState::Changes> parentChanges = parentSnapshot.changes &
(RequestedLayerState::Changes::Hierarchy | RequestedLayerState::Changes::Geometry |
RequestedLayerState::Changes::Visibility | RequestedLayerState::Changes::Metadata |
- RequestedLayerState::Changes::AffectsChildren);
+ RequestedLayerState::Changes::AffectsChildren |
+ RequestedLayerState::Changes::FrameRate);
snapshot.changes = parentChanges | requested.changes;
snapshot.isHiddenByPolicyFromParent = parentSnapshot.isHiddenByPolicyFromParent ||
parentSnapshot.invalidTransform || requested.isHiddenByPolicy() ||
(args.excludeLayerIds.find(path.id) != args.excludeLayerIds.end());
snapshot.contentDirty = requested.what & layer_state_t::CONTENT_DIRTY;
// TODO(b/238781169) scope down the changes to only buffer updates.
- snapshot.hasReadyFrame =
- (snapshot.contentDirty || requested.autoRefresh) && (requested.externalTexture);
- // TODO(b/238781169) how is this used? ag/15523870
- snapshot.sidebandStreamHasFrame = false;
+ snapshot.hasReadyFrame = requested.hasReadyFrame();
+ snapshot.sidebandStreamHasFrame = requested.hasSidebandStreamFrame();
updateSurfaceDamage(requested, snapshot.hasReadyFrame, args.forceFullDamage,
snapshot.surfaceDamage);
- const bool forceUpdate = newSnapshot || args.forceUpdate ||
+ const bool forceUpdate = newSnapshot || args.forceUpdate == ForceUpdateFlags::ALL ||
snapshot.changes.any(RequestedLayerState::Changes::Visibility |
RequestedLayerState::Changes::Created);
snapshot.outputFilter.layerStack = requested.parentId != UNASSIGNED_LAYER_ID
@@ -708,11 +725,6 @@
snapshot.gameMode = requested.metadata.has(gui::METADATA_GAME_MODE)
? requested.gameMode
: parentSnapshot.gameMode;
- snapshot.frameRate = (requested.requestedFrameRate.rate.isValid() ||
- (requested.requestedFrameRate.type ==
- scheduler::LayerInfo::FrameRateCompatibility::NoVote))
- ? requested.requestedFrameRate
- : parentSnapshot.frameRate;
snapshot.fixedTransformHint = requested.fixedTransformHint != ui::Transform::ROT_INVALID
? requested.fixedTransformHint
: parentSnapshot.fixedTransformHint;
@@ -722,6 +734,16 @@
(requested.layerStackToMirror != ui::INVALID_LAYER_STACK);
}
+ if (forceUpdate ||
+ snapshot.changes.any(RequestedLayerState::Changes::FrameRate |
+ RequestedLayerState::Changes::Hierarchy)) {
+ snapshot.frameRate = (requested.requestedFrameRate.rate.isValid() ||
+ (requested.requestedFrameRate.type ==
+ scheduler::LayerInfo::FrameRateCompatibility::NoVote))
+ ? requested.requestedFrameRate
+ : parentSnapshot.frameRate;
+ }
+
if (forceUpdate || requested.changes.get() != 0) {
snapshot.compositionType = requested.getCompositionType();
snapshot.dimmingEnabled = requested.dimmingEnabled;
@@ -773,10 +795,11 @@
// snapshot.metadata;
LLOGV(snapshot.sequence,
"%supdated [%d]%s changes parent:%s global:%s local:%s requested:%s %s from parent %s",
- args.forceUpdate ? "Force " : "", requested.id, requested.name.c_str(),
- parentSnapshot.changes.string().c_str(), snapshot.changes.string().c_str(),
- requested.changes.string().c_str(), std::to_string(requested.what).c_str(),
- snapshot.getDebugString().c_str(), parentSnapshot.getDebugString().c_str());
+ args.forceUpdate == ForceUpdateFlags::ALL ? "Force " : "", requested.id,
+ requested.name.c_str(), parentSnapshot.changes.string().c_str(),
+ snapshot.changes.string().c_str(), requested.changes.string().c_str(),
+ std::to_string(requested.what).c_str(), snapshot.getDebugString().c_str(),
+ parentSnapshot.getDebugString().c_str());
}
void LayerSnapshotBuilder::updateRoundedCorner(LayerSnapshot& snapshot,
@@ -912,8 +935,12 @@
} else {
snapshot.inputInfo = {};
}
- snapshot.inputInfo.displayId = static_cast<int32_t>(snapshot.outputFilter.layerStack.id);
+ snapshot.inputInfo.name = requested.name;
+ snapshot.inputInfo.id = static_cast<int32_t>(requested.id);
+ snapshot.inputInfo.ownerUid = static_cast<int32_t>(requested.ownerUid);
+ snapshot.inputInfo.ownerPid = requested.ownerPid;
+ snapshot.inputInfo.displayId = static_cast<int32_t>(snapshot.outputFilter.layerStack.id);
if (!needsInputInfo(snapshot, requested)) {
return;
}
diff --git a/services/surfaceflinger/FrontEnd/LayerSnapshotBuilder.h b/services/surfaceflinger/FrontEnd/LayerSnapshotBuilder.h
index 0902ab8..d70bdb9 100644
--- a/services/surfaceflinger/FrontEnd/LayerSnapshotBuilder.h
+++ b/services/surfaceflinger/FrontEnd/LayerSnapshotBuilder.h
@@ -35,10 +35,15 @@
// snapshots when there are only buffer updates.
class LayerSnapshotBuilder {
public:
+ enum class ForceUpdateFlags {
+ NONE,
+ ALL,
+ HIERARCHY,
+ };
struct Args {
LayerHierarchy root;
const LayerLifecycleManager& layerLifecycleManager;
- bool forceUpdate = false;
+ ForceUpdateFlags forceUpdate = ForceUpdateFlags::NONE;
bool includeMetadata = false;
const display::DisplayMap<ui::LayerStack, frontend::DisplayInfo>& displays;
// Set to true if there were display changes since last update.
diff --git a/services/surfaceflinger/FrontEnd/RequestedLayerState.cpp b/services/surfaceflinger/FrontEnd/RequestedLayerState.cpp
index 09523d3..e2cbe28 100644
--- a/services/surfaceflinger/FrontEnd/RequestedLayerState.cpp
+++ b/services/surfaceflinger/FrontEnd/RequestedLayerState.cpp
@@ -165,8 +165,13 @@
if (hadBufferOrSideStream != hasBufferOrSideStream) {
changes |= RequestedLayerState::Changes::Geometry |
RequestedLayerState::Changes::VisibleRegion |
- RequestedLayerState::Changes::Visibility | RequestedLayerState::Changes::Input |
- RequestedLayerState::Changes::Buffer;
+ RequestedLayerState::Changes::Visibility | RequestedLayerState::Changes::Input;
+ }
+ if (clientState.what & layer_state_t::eBufferChanged) {
+ changes |= RequestedLayerState::Changes::Buffer;
+ }
+ if (clientState.what & layer_state_t::eSidebandStreamChanged) {
+ changes |= RequestedLayerState::Changes::SidebandStream;
}
}
if (what & (layer_state_t::eAlphaChanged)) {
@@ -197,7 +202,9 @@
static const mat4 identityMatrix = mat4();
hasColorTransform = colorTransform != identityMatrix;
}
- if (clientState.what & (layer_state_t::eLayerChanged | layer_state_t::eRelativeLayerChanged)) {
+ if (clientState.what &
+ (layer_state_t::eLayerChanged | layer_state_t::eRelativeLayerChanged |
+ layer_state_t::eLayerStackChanged)) {
changes |= RequestedLayerState::Changes::Z;
}
if (clientState.what & layer_state_t::eReparent) {
@@ -453,4 +460,22 @@
return backgroundBlurRadius > 0 || blurRegions.size() > 0;
}
+bool RequestedLayerState::hasFrameUpdate() const {
+ return what & layer_state_t::CONTENT_DIRTY &&
+ (externalTexture || bgColorLayerId != UNASSIGNED_LAYER_ID);
+}
+
+bool RequestedLayerState::hasReadyFrame() const {
+ return hasFrameUpdate() || changes.test(Changes::SidebandStream) || autoRefresh;
+}
+
+bool RequestedLayerState::hasSidebandStreamFrame() const {
+ return hasFrameUpdate() && sidebandStream.get();
+}
+
+void RequestedLayerState::clearChanges() {
+ what = 0;
+ changes.clear();
+}
+
} // namespace android::surfaceflinger::frontend
diff --git a/services/surfaceflinger/FrontEnd/RequestedLayerState.h b/services/surfaceflinger/FrontEnd/RequestedLayerState.h
index 6840b25..8b475a3 100644
--- a/services/surfaceflinger/FrontEnd/RequestedLayerState.h
+++ b/services/surfaceflinger/FrontEnd/RequestedLayerState.h
@@ -52,10 +52,13 @@
FrameRate = 1u << 13,
VisibleRegion = 1u << 14,
Buffer = 1u << 15,
+ SidebandStream = 1u << 16,
};
static Rect reduce(const Rect& win, const Region& exclude);
RequestedLayerState(const LayerCreationArgs&);
void merge(const ResolvedComposerState&);
+ void clearChanges();
+
// Currently we only care about the primary display
ui::Transform getTransform(uint32_t displayRotationFlags) const;
ui::Size getUnrotatedBufferSize(uint32_t displayRotationFlags) const;
@@ -72,6 +75,9 @@
bool hasValidRelativeParent() const;
bool hasInputInfo() const;
bool hasBlur() const;
+ bool hasFrameUpdate() const;
+ bool hasReadyFrame() const;
+ bool hasSidebandStreamFrame() const;
// Layer serial number. This gives layers an explicit ordering, so we
// have a stable sort order when their layer stack and Z-order are
@@ -110,6 +116,7 @@
uint32_t touchCropId = UNASSIGNED_LAYER_ID;
uint32_t bgColorLayerId = UNASSIGNED_LAYER_ID;
ftl::Flags<RequestedLayerState::Changes> changes;
+ bool bgColorLayer = false;
};
} // namespace android::surfaceflinger::frontend