SF: fix frame rate for layer tree

With the current implementation we might mark layers as NoVote
incorrectly when a sibling has a frame rate.

Test: adb shell /data/nativetest64/libsurfaceflinger_unittest/libsurfaceflinger_unittest
Bug: 195205467
Change-Id: I62641f855a027e1192f9a85a4bc50692b1744764

3 files changed, 97 insertions, 68 deletions

diff --git a/services/surfaceflinger/Layer.cpp b/services/surfaceflinger/Layer.cpp
index dbd2793276..e4a777f3c7 100644
--- a/services/surfaceflinger/Layer.cpp
+++ b/services/surfaceflinger/Layer.cpp
@@ -1117,46 +1117,59 @@ StretchEffect Layer::getStretchEffect() const {
     return StretchEffect{};
 }
 
-void Layer::updateTreeHasFrameRateVote() {
-    const auto traverseTree = [&](const LayerVector::Visitor& visitor) {
-        auto parent = getParent();
-        while (parent) {
-            visitor(parent.get());
-            parent = parent->getParent();
+bool Layer::propagateFrameRateForLayerTree(FrameRate parentFrameRate, bool* transactionNeeded) {
+    // The frame rate for layer tree is this layer's frame rate if present, or the parent frame rate
+    const auto frameRate = [&] {
+        if (mDrawingState.frameRate.rate.isValid() ||
+            mDrawingState.frameRate.type == FrameRateCompatibility::NoVote) {
+            return mDrawingState.frameRate;
         }
 
-        traverse(LayerVector::StateSet::Current, visitor);
-    };
-
-    // update parents and children about the vote
-    // First traverse the tree and count how many layers has votes.
-    int layersWithVote = 0;
-    traverseTree([&layersWithVote](Layer* layer) {
-        const auto layerVotedWithDefaultCompatibility =
-                layer->mDrawingState.frameRate.rate.isValid() &&
-                layer->mDrawingState.frameRate.type == FrameRateCompatibility::Default;
-        const auto layerVotedWithNoVote =
-                layer->mDrawingState.frameRate.type == FrameRateCompatibility::NoVote;
-        const auto layerVotedWithExactCompatibility =
-                layer->mDrawingState.frameRate.type == FrameRateCompatibility::Exact;
-
-        // We do not count layers that are ExactOrMultiple for the same reason
-        // we are allowing touch boost for those layers. See
-        // RefreshRateConfigs::getBestRefreshRate for more details.
-        if (layerVotedWithDefaultCompatibility || layerVotedWithNoVote ||
-            layerVotedWithExactCompatibility) {
-            layersWithVote++;
+        return parentFrameRate;
+    }();
+
+    *transactionNeeded |= setFrameRateForLayerTree(frameRate);
+
+    // The frame rate is propagated to the children
+    bool childrenHaveFrameRate = false;
+    for (const sp<Layer>& child : mCurrentChildren) {
+        childrenHaveFrameRate |=
+                child->propagateFrameRateForLayerTree(frameRate, transactionNeeded);
+    }
+
+    // If we don't have a valid frame rate, but the children do, we set this
+    // layer as NoVote to allow the children to control the refresh rate
+    if (!frameRate.rate.isValid() && frameRate.type != FrameRateCompatibility::NoVote &&
+        childrenHaveFrameRate) {
+        *transactionNeeded |=
+                setFrameRateForLayerTree(FrameRate(Fps(0.0f), FrameRateCompatibility::NoVote));
+    }
+
+    // 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
+    // RefreshRateConfigs::getBestRefreshRate for more details.
+    const auto layerVotedWithDefaultCompatibility =
+            frameRate.rate.isValid() && frameRate.type == FrameRateCompatibility::Default;
+    const auto layerVotedWithNoVote = frameRate.type == FrameRateCompatibility::NoVote;
+    const auto layerVotedWithExactCompatibility =
+            frameRate.rate.isValid() && frameRate.type == FrameRateCompatibility::Exact;
+    return layerVotedWithDefaultCompatibility || layerVotedWithNoVote ||
+            layerVotedWithExactCompatibility || childrenHaveFrameRate;
+}
+
+void Layer::updateTreeHasFrameRateVote() {
+    const auto root = [&]() -> sp<Layer> {
+        sp<Layer> layer = this;
+        while (auto parent = layer->getParent()) {
+            layer = parent;
         }
-    });
+        return layer;
+    }();
 
-    // Now we can update the tree frame rate vote for each layer in the tree
-    const bool treeHasFrameRateVote = layersWithVote > 0;
     bool transactionNeeded = false;
+    root->propagateFrameRateForLayerTree({}, &transactionNeeded);
 
-    traverseTree([treeHasFrameRateVote, &transactionNeeded](Layer* layer) {
-        transactionNeeded = layer->updateFrameRateForLayerTree(treeHasFrameRateVote);
-    });
-
+    // TODO(b/195668952): we probably don't need eTraversalNeeded here
     if (transactionNeeded) {
         mFlinger->setTransactionFlags(eTraversalNeeded);
     }
@@ -1283,42 +1296,23 @@ std::shared_ptr<frametimeline::SurfaceFrame> Layer::createSurfaceFrameForBuffer(
     return surfaceFrame;
 }
 
-bool Layer::updateFrameRateForLayerTree(bool treeHasFrameRateVote) {
-    const auto updateDrawingState = [&](FrameRate frameRate) {
-        if (mDrawingState.frameRateForLayerTree == frameRate) {
-            return false;
-        }
-
-        mDrawingState.frameRateForLayerTree = frameRate;
-        mDrawingState.sequence++;
-        mDrawingState.modified = true;
-        setTransactionFlags(eTransactionNeeded);
-
-        mFlinger->mScheduler->recordLayerHistory(this, systemTime(),
-                                                 LayerHistory::LayerUpdateType::SetFrameRate);
-
-        return true;
-    };
-
-    const auto frameRate = mDrawingState.frameRate;
-    if (frameRate.rate.isValid() || frameRate.type == FrameRateCompatibility::NoVote) {
-        return updateDrawingState(frameRate);
+bool Layer::setFrameRateForLayerTree(FrameRate frameRate) {
+    if (mDrawingState.frameRateForLayerTree == frameRate) {
+        return false;
     }
 
-    // This layer doesn't have a frame rate. Check if its ancestors have a vote
-    for (sp<Layer> parent = getParent(); parent; parent = parent->getParent()) {
-        if (parent->mDrawingState.frameRate.rate.isValid()) {
-            return updateDrawingState(parent->mDrawingState.frameRate);
-        }
-    }
+    mDrawingState.frameRateForLayerTree = frameRate;
 
-    // This layer and its ancestors don't have a frame rate. If one of successors
-    // has a vote, return a NoVote for successors to set the vote
-    if (treeHasFrameRateVote) {
-        return updateDrawingState(FrameRate(Fps(0.0f), FrameRateCompatibility::NoVote));
-    }
+    // TODO(b/195668952): we probably don't need to dirty visible regions here
+    // or even store frameRateForLayerTree in mDrawingState
+    mDrawingState.sequence++;
+    mDrawingState.modified = true;
+    setTransactionFlags(eTransactionNeeded);
+
+    mFlinger->mScheduler->recordLayerHistory(this, systemTime(),
+                                             LayerHistory::LayerUpdateType::SetFrameRate);
 
-    return updateDrawingState(frameRate);
+    return true;
 }
 
 Layer::FrameRate Layer::getFrameRateForLayerTree() const {
diff --git a/services/surfaceflinger/Layer.h b/services/surfaceflinger/Layer.h
index c5cb17ffc7..4426c70c19 100644
--- a/services/surfaceflinger/Layer.h
+++ b/services/surfaceflinger/Layer.h
@@ -1053,6 +1053,8 @@ private:
                                           const std::vector<Layer*>& layersInTree);
 
     void updateTreeHasFrameRateVote();
+    bool propagateFrameRateForLayerTree(FrameRate parentFrameRate, bool* transactionNeeded);
+    bool setFrameRateForLayerTree(FrameRate);
     void setZOrderRelativeOf(const wp<Layer>& relativeOf);
     bool isTrustedOverlay() const;
 
@@ -1071,8 +1073,6 @@ private:
     // Fills in the frame and transform info for the InputWindowInfo
     void fillInputFrameInfo(InputWindowInfo& info, const ui::Transform& toPhysicalDisplay);
 
-    bool updateFrameRateForLayerTree(bool treeHasFrameRateVote);
-
     // Cached properties computed from drawing state
     // Effective transform taking into account parent transforms and any parent scaling, which is
     // a transform from the current layer coordinate space to display(screen) coordinate space.
diff --git a/services/surfaceflinger/tests/unittests/SetFrameRateTest.cpp b/services/surfaceflinger/tests/unittests/SetFrameRateTest.cpp
index 1ed52ea389..2761470c31 100644
--- a/services/surfaceflinger/tests/unittests/SetFrameRateTest.cpp
+++ b/services/surfaceflinger/tests/unittests/SetFrameRateTest.cpp
@@ -485,5 +485,40 @@ TEST_P(SetFrameRateTest, SetOnParentActivatesTree) {
     EXPECT_TRUE(FRAME_RATE_VOTE1.rate.equalsWithMargin(layerHistorySummary[1].desiredRefreshRate));
 }
 
+TEST_P(SetFrameRateTest, addChildForParentWithTreeVote) {
+    EXPECT_CALL(*mMessageQueue, invalidate()).Times(1);
+
+    const auto& layerFactory = GetParam();
+
+    const auto parent = mLayers.emplace_back(layerFactory->createLayer(mFlinger));
+    const auto child1 = mLayers.emplace_back(layerFactory->createLayer(mFlinger));
+    const auto child2 = mLayers.emplace_back(layerFactory->createLayer(mFlinger));
+    const auto childOfChild1 = mLayers.emplace_back(layerFactory->createLayer(mFlinger));
+
+    addChild(parent, child1);
+    addChild(child1, childOfChild1);
+
+    childOfChild1->setFrameRate(FRAME_RATE_VOTE1);
+    commitTransaction();
+    EXPECT_EQ(FRAME_RATE_TREE, parent->getFrameRateForLayerTree());
+    EXPECT_EQ(FRAME_RATE_TREE, child1->getFrameRateForLayerTree());
+    EXPECT_EQ(FRAME_RATE_VOTE1, childOfChild1->getFrameRateForLayerTree());
+    EXPECT_EQ(FRAME_RATE_NO_VOTE, child2->getFrameRateForLayerTree());
+
+    addChild(parent, child2);
+    commitTransaction();
+    EXPECT_EQ(FRAME_RATE_TREE, parent->getFrameRateForLayerTree());
+    EXPECT_EQ(FRAME_RATE_TREE, child1->getFrameRateForLayerTree());
+    EXPECT_EQ(FRAME_RATE_VOTE1, childOfChild1->getFrameRateForLayerTree());
+    EXPECT_EQ(FRAME_RATE_NO_VOTE, child2->getFrameRateForLayerTree());
+
+    childOfChild1->setFrameRate(FRAME_RATE_NO_VOTE);
+    commitTransaction();
+    EXPECT_EQ(FRAME_RATE_NO_VOTE, parent->getFrameRateForLayerTree());
+    EXPECT_EQ(FRAME_RATE_NO_VOTE, child1->getFrameRateForLayerTree());
+    EXPECT_EQ(FRAME_RATE_NO_VOTE, childOfChild1->getFrameRateForLayerTree());
+    EXPECT_EQ(FRAME_RATE_NO_VOTE, child2->getFrameRateForLayerTree());
+}
+
 } // namespace
 } // namespace android