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

9 files changed, 53 insertions, 22 deletions

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<Fence
             std::optional<SurfaceControlStats> 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 <cstdint>
+#include <optional>
+
 #include <gui/ISurfaceComposer.h>
 #include <gui/ITransactionCompletedListener.h>
 #include <gui/LayerState.h>
@@ -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<int32_t>(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, &currentMaxAcquiredBufferCount);
     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<IBinder>& sc, std::variant<nsecs_t, sp<Fence>> acquireTimeOrFence,
-                 const sp<Fence>& prevReleaseFence, uint32_t hint,
+                 const sp<Fence>& prevReleaseFence, std::optional<uint32_t> hint,
                  uint32_t currentMaxAcquiredBuffersCount, FrameEventHistoryStats frameEventStats,
                  std::vector<JankData> jankData, ReleaseCallbackId previousReleaseCallbackId)
           : surfaceControl(sc),
@@ -147,7 +147,7 @@ public:
     sp<IBinder> surfaceControl;
     std::variant<nsecs_t, sp<Fence>> acquireTimeOrFence = -1;
     sp<Fence> previousReleaseFence;
-    uint32_t transformHint = 0;
+    std::optional<uint32_t> transformHint = 0;
     uint32_t currentMaxAcquiredBufferCount = 0;
     FrameEventHistoryStats eventStats;
     std::vector<JankData> 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<SurfaceControl>& sc, nsecs_t latchTime,
                         std::variant<nsecs_t, sp<Fence>> acquireTimeOrFence,
                         const sp<Fence>& presentFence, const sp<Fence>& prevReleaseFence,
-                        uint32_t hint, FrameEventHistoryStats eventStats,
+                        std::optional<uint32_t> hint, FrameEventHistoryStats eventStats,
                         uint32_t currentMaxAcquiredBufferCount)
           : surfaceControl(sc),
             latchTime(latchTime),
@@ -85,7 +85,7 @@ struct SurfaceControlStats {
     std::variant<nsecs_t, sp<Fence>> acquireTimeOrFence = -1;
     sp<Fence> presentFence;
     sp<Fence> previousReleaseFence;
-    uint32_t transformHint = 0;
+    std::optional<uint32_t> 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 <algorithm>
 #include <mutex>
+#include <optional>
 #include <sstream>
 
 #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<uint32_t>(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<renderengine::ExternalTexture>& 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 <condition_variable>
 #include <deque>
 #include <mutex>
+#include <optional>
 #include <queue>
 #include <thread>
 #include <unordered_map>
@@ -48,7 +49,7 @@ public:
     std::vector<ftl::SharedFuture<FenceResult>> previousReleaseFences;
     std::variant<nsecs_t, sp<Fence>> acquireTimeOrFence = -1;
     nsecs_t latchTime = -1;
-    uint32_t transformHint = 0;
+    std::optional<uint32_t> transformHint = std::nullopt;
     uint32_t currentMaxAcquiredBufferCount = 0;
     std::shared_ptr<FenceTime> gpuCompositionDoneFence{FenceTime::NO_FENCE};
     CompositorTiming compositorTiming;