Fix out-of-order transactions (2/2)

The following sequence of order may happen which cause wrong
surface positions:
- WA.animate updates surfaces properties to S
- WA.animate closes the surface transaction
- Since the previous animation transaction wasn't commited yet,
closeSurfaceTransaction blocks and updating the surface properties
on SF side is deferred.
- In the meantime, since we are not holding WM lock, we have
another thread updating surfaces properties to S'
- Closing the transaction in this thread completes immediately
because it's not a synchronous transaction or animation
transaction.
- After a frame has been processed S gets applied on SF side as
the other transaction is done waiting for the frame to complete.

The issue here is that properties are now set to S instead of S'.
Sad!

We originally started calling closeTransaction without the WM
lock being held because it lead to thread starvation (b/38192114).
However, that fix has this big flaw as described above.

To fix this, we create an empty animation transaction before
updating the animation properties to simulate the back-pressuring
behavior of animation transactions without the WM lock being held.
If that transaction arrives out of order, it doesn't matter at all
because it is empty.

After that, we perform the animation udpate in a transaction that
is not marked as an animation transaction, and thus will not
block, which avoids the starvation issue.

Part of this change is also a change in SF to allow executing
empty animation transactions.

Test: go/wm-smoke
Test: Open VideoPlayer from VRCore, close it, observe no wrong
positiioning of surfaces.
Test: Inspect traces while animating. Ensure back pressuring still
works.

Change-Id: Ie545463e71e0d1bc73439d14381077a290d2f959
Fixes: 63905190
Bug: 38192114

1 files changed, 6 insertions, 4 deletions

diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp
index 0f6c581e14..bc3a3ef626 100644
--- a/services/surfaceflinger/SurfaceFlinger.cpp
+++ b/services/surfaceflinger/SurfaceFlinger.cpp
@@ -2861,10 +2861,12 @@ void SurfaceFlinger::setTransactionState(
         }
     }
 
-    // If a synchronous transaction is explicitly requested without any changes,
-    // force a transaction anyway. This can be used as a flush mechanism for
-    // previous async transactions.
-    if (transactionFlags == 0 && (flags & eSynchronous)) {
+    // If a synchronous transaction is explicitly requested without any changes, force a transaction
+    // anyway. This can be used as a flush mechanism for previous async transactions.
+    // Empty animation transaction can be used to simulate back-pressure, so also force a
+    // transaction for empty animation transactions.
+    if (transactionFlags == 0 &&
+            ((flags & eSynchronous) || (flags & eAnimation))) {
         transactionFlags = eTransactionNeeded;
     }