Skip to content

Commit 9892aac

Browse files
AMouriAMouri
committed
Correctly pass screenshot fences to transaction callbacks
In some instances, a screenshot may be captured before a layer has a release callback registered. This can happen when a new buffer has not yet been transacted before a screenshot is captured. This causes the screenshot fence to be dropped and the possibility of tearing when capturing a screenshot while continuously rendering. To resolve this, buffer screenshot fences into a list of future fences when there is no callback registered, and merge those fences when dispatching the release callback. Bug: 302703346 Test: SurfaceViewTests#testMovingWhiteSurfaceView 100 times Change-Id: I91aec3cdb0973092d48cd77e59dd3999e9d9e847
1 parent 05e3113 commit 9892aac

11 files changed

Lines changed: 197 additions & 36 deletions

File tree

‎include/ftl/details/future.h‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -73,8 +73,18 @@ class BaseFuture<Self, T, std::future> {
7373
return std::get<Impl>(self()).get();
7474
}
7575

76+
template <class Rep, class Period>
77+
std::future_status wait_for(const std::chrono::duration<Rep, Period>& timeout_duration) const {
78+
if (std::holds_alternative<T>(self())) {
79+
return std::future_status::ready;
80+
}
81+
82+
return std::get<Impl>(self()).wait_for(timeout_duration);
83+
}
84+
7685
private:
7786
auto& self() { return static_cast<Self&>(*this).future_; }
87+
const auto& self() const { return static_cast<const Self&>(*this).future_; }
7888
};
7989

8090
template <typename Self, typename T>
@@ -90,6 +100,15 @@ class BaseFuture<Self, T, std::shared_future> {
90100
return std::get<Impl>(self()).get();
91101
}
92102

103+
template <class Rep, class Period>
104+
std::future_status wait_for(const std::chrono::duration<Rep, Period>& timeout_duration) const {
105+
if (std::holds_alternative<T>(self())) {
106+
return std::future_status::ready;
107+
}
108+
109+
return std::get<Impl>(self()).wait_for(timeout_duration);
110+
}
111+
93112
private:
94113
const auto& self() const { return static_cast<const Self&>(*this).future_; }
95114
};

‎include/ftl/future.h‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,7 @@ class Future final : public details::BaseFuture<Future<T, FutureImpl>, T, Future
5151
// Forwarding functions. Base::share is only defined when FutureImpl is std::future, whereas the
5252
// following are defined for either FutureImpl:
5353
using Base::get;
54+
using Base::wait_for;
5455

5556
// Attaches a continuation to the future. The continuation is a function that maps T to either R
5657
// or ftl::Future<R>. In the former case, the chain wraps the result in a future as if by

‎libs/ftl/future_test.cpp‎

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -102,4 +102,42 @@ TEST(Future, Chain) {
102102
decrement_thread.join();
103103
}
104104

105+
TEST(Future, WaitFor) {
106+
using namespace std::chrono_literals;
107+
{
108+
auto future = ftl::yield(42);
109+
// Check that we can wait_for multiple times without invalidating the future
110+
EXPECT_EQ(future.wait_for(1s), std::future_status::ready);
111+
EXPECT_EQ(future.wait_for(1s), std::future_status::ready);
112+
EXPECT_EQ(future.get(), 42);
113+
}
114+
115+
{
116+
std::condition_variable cv;
117+
std::mutex m;
118+
bool ready = false;
119+
120+
std::packaged_task<int32_t()> get_int([&] {
121+
std::unique_lock lk(m);
122+
cv.wait(lk, [&] { return ready; });
123+
return 24;
124+
});
125+
126+
auto get_future = ftl::Future(get_int.get_future());
127+
std::thread get_thread(std::move(get_int));
128+
129+
EXPECT_EQ(get_future.wait_for(0s), std::future_status::timeout);
130+
{
131+
std::unique_lock lk(m);
132+
ready = true;
133+
}
134+
cv.notify_one();
135+
136+
EXPECT_EQ(get_future.wait_for(1s), std::future_status::ready);
137+
EXPECT_EQ(get_future.get(), 24);
138+
139+
get_thread.join();
140+
}
141+
}
142+
105143
} // namespace android::test

‎services/surfaceflinger/Layer.cpp‎

Lines changed: 43 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -78,11 +78,13 @@
7878
#include "SurfaceFlinger.h"
7979
#include "TimeStats/TimeStats.h"
8080
#include "TunnelModeEnabledReporter.h"
81+
#include "Utils/FenceUtils.h"
8182

8283
#define DEBUG_RESIZE 0
8384
#define EARLY_RELEASE_ENABLED false
8485

8586
namespace android {
87+
using namespace std::chrono_literals;
8688
namespace {
8789
constexpr int kDumpTableRowLength = 159;
8890

@@ -2911,7 +2913,8 @@ void Layer::callReleaseBufferCallback(const sp<ITransactionCompletedListener>& l
29112913
}
29122914

29132915
void Layer::onLayerDisplayed(ftl::SharedFuture<FenceResult> futureFenceResult,
2914-
ui::LayerStack layerStack) {
2916+
ui::LayerStack layerStack,
2917+
std::function<FenceResult(FenceResult)>&& continuation) {
29152918
// If we are displayed on multiple displays in a single composition cycle then we would
29162919
// need to do careful tracking to enable the use of the mLastClientCompositionFence.
29172920
// For example we can only use it if all the displays are client comp, and we need
@@ -2946,11 +2949,41 @@ void Layer::onLayerDisplayed(ftl::SharedFuture<FenceResult> futureFenceResult,
29462949
break;
29472950
}
29482951
}
2952+
2953+
if (!FlagManager::getInstance().screenshot_fence_preservation() && continuation) {
2954+
futureFenceResult = ftl::Future(futureFenceResult).then(std::move(continuation)).share();
2955+
}
2956+
29492957
if (ch != nullptr) {
29502958
ch->previousReleaseCallbackId = mPreviousReleaseCallbackId;
29512959
ch->previousReleaseFences.emplace_back(std::move(futureFenceResult));
29522960
ch->name = mName;
2961+
} else if (FlagManager::getInstance().screenshot_fence_preservation()) {
2962+
// If we didn't get a release callback yet, e.g. some scenarios when capturing screenshots
2963+
// asynchronously, then make sure we don't drop the fence.
2964+
mAdditionalPreviousReleaseFences.emplace_back(std::move(futureFenceResult),
2965+
std::move(continuation));
2966+
std::vector<FenceAndContinuation> mergedFences;
2967+
sp<Fence> prevFence = nullptr;
2968+
// For a layer that's frequently screenshotted, try to merge fences to make sure we don't
2969+
// grow unbounded.
2970+
for (const auto& futureAndContinution : mAdditionalPreviousReleaseFences) {
2971+
auto result = futureAndContinution.future.wait_for(0s);
2972+
if (result != std::future_status::ready) {
2973+
mergedFences.emplace_back(futureAndContinution);
2974+
continue;
2975+
}
2976+
2977+
mergeFence(getDebugName(), futureAndContinution.chain().get().value_or(Fence::NO_FENCE),
2978+
prevFence);
2979+
}
2980+
if (prevFence != nullptr) {
2981+
mergedFences.emplace_back(ftl::yield(FenceResult(std::move(prevFence))).share());
2982+
}
2983+
2984+
mAdditionalPreviousReleaseFences.swap(mergedFences);
29532985
}
2986+
29542987
if (mBufferInfo.mBuffer) {
29552988
mPreviouslyPresentedLayerStacks.push_back(layerStack);
29562989
}
@@ -3440,6 +3473,15 @@ bool Layer::setTransactionCompletedListeners(const std::vector<sp<CallbackHandle
34403473
handle->acquireTimeOrFence = mCallbackHandleAcquireTimeOrFence;
34413474
handle->frameNumber = mDrawingState.frameNumber;
34423475
handle->previousFrameNumber = mDrawingState.previousFrameNumber;
3476+
if (FlagManager::getInstance().screenshot_fence_preservation() &&
3477+
mPreviousReleaseBufferEndpoint == handle->listener) {
3478+
// Add fences from previous screenshots now so that they can be dispatched to the
3479+
// client.
3480+
for (const auto& futureAndContinution : mAdditionalPreviousReleaseFences) {
3481+
handle->previousReleaseFences.emplace_back(futureAndContinution.chain());
3482+
}
3483+
mAdditionalPreviousReleaseFences.clear();
3484+
}
34433485

34443486
// Store so latched time and release fence can be set
34453487
mDrawingState.callbackHandles.push_back(handle);

‎services/surfaceflinger/Layer.h‎

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -555,7 +555,8 @@ class Layer : public virtual RefBase {
555555
const compositionengine::LayerFECompositionState* getCompositionState() const;
556556
bool fenceHasSignaled() const;
557557
void onPreComposition(nsecs_t refreshStartTime);
558-
void onLayerDisplayed(ftl::SharedFuture<FenceResult>, ui::LayerStack layerStack);
558+
void onLayerDisplayed(ftl::SharedFuture<FenceResult>, ui::LayerStack layerStack,
559+
std::function<FenceResult(FenceResult)>&& continuation = nullptr);
559560

560561
void setWasClientComposed(const sp<Fence>& fence) {
561562
mLastClientCompositionFence = fence;
@@ -932,6 +933,19 @@ class Layer : public virtual RefBase {
932933
// the release fences from the correct displays when we release the last buffer
933934
// from the layer.
934935
std::vector<ui::LayerStack> mPreviouslyPresentedLayerStacks;
936+
struct FenceAndContinuation {
937+
ftl::SharedFuture<FenceResult> future;
938+
std::function<FenceResult(FenceResult)> continuation;
939+
940+
ftl::SharedFuture<FenceResult> chain() const {
941+
if (continuation) {
942+
return ftl::Future(future).then(continuation).share();
943+
} else {
944+
return future;
945+
}
946+
}
947+
};
948+
std::vector<FenceAndContinuation> mAdditionalPreviousReleaseFences;
935949
// Exposed so SurfaceFlinger can assert that it's held
936950
const sp<SurfaceFlinger> mFlinger;
937951

‎services/surfaceflinger/SurfaceFlinger.cpp‎

Lines changed: 17 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -8152,14 +8152,23 @@ ftl::SharedFuture<FenceResult> SurfaceFlinger::renderScreenImpl(
81528152
: ftl::yield(present()).share();
81538153

81548154
for (auto& [layer, layerFE] : layers) {
8155-
layer->onLayerDisplayed(ftl::Future(presentFuture)
8156-
.then([layerFE = std::move(layerFE)](FenceResult) {
8157-
return layerFE->stealCompositionResult()
8158-
.releaseFences.back()
8159-
.first.get();
8160-
})
8161-
.share(),
8162-
ui::INVALID_LAYER_STACK);
8155+
layer->onLayerDisplayed(presentFuture, ui::INVALID_LAYER_STACK,
8156+
[layerFE = std::move(layerFE)](FenceResult) {
8157+
if (FlagManager::getInstance()
8158+
.screenshot_fence_preservation()) {
8159+
const auto compositionResult =
8160+
layerFE->stealCompositionResult();
8161+
const auto& fences = compositionResult.releaseFences;
8162+
// CompositionEngine may choose to cull layers that
8163+
// aren't visible, so pass a non-fence.
8164+
return fences.empty() ? Fence::NO_FENCE
8165+
: fences.back().first.get();
8166+
} else {
8167+
return layerFE->stealCompositionResult()
8168+
.releaseFences.back()
8169+
.first.get();
8170+
}
8171+
});
81638172
}
81648173

81658174
return presentFuture;

‎services/surfaceflinger/TransactionCallbackInvoker.cpp‎

Lines changed: 2 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@
2525

2626
#include "TransactionCallbackInvoker.h"
2727
#include "BackgroundExecutor.h"
28+
#include "Utils/FenceUtils.h"
2829

2930
#include <cinttypes>
3031

@@ -127,33 +128,8 @@ status_t TransactionCallbackInvoker::addCallbackHandle(const sp<CallbackHandle>&
127128
sp<IBinder> surfaceControl = handle->surfaceControl.promote();
128129
if (surfaceControl) {
129130
sp<Fence> prevFence = nullptr;
130-
131131
for (const auto& future : handle->previousReleaseFences) {
132-
sp<Fence> currentFence = future.get().value_or(Fence::NO_FENCE);
133-
if (prevFence == nullptr && currentFence->getStatus() != Fence::Status::Invalid) {
134-
prevFence = std::move(currentFence);
135-
} else if (prevFence != nullptr) {
136-
// If both fences are signaled or both are unsignaled, we need to merge
137-
// them to get an accurate timestamp.
138-
if (prevFence->getStatus() != Fence::Status::Invalid &&
139-
prevFence->getStatus() == currentFence->getStatus()) {
140-
char fenceName[32] = {};
141-
snprintf(fenceName, 32, "%.28s", handle->name.c_str());
142-
sp<Fence> mergedFence = Fence::merge(fenceName, prevFence, currentFence);
143-
if (mergedFence->isValid()) {
144-
prevFence = std::move(mergedFence);
145-
}
146-
} else if (currentFence->getStatus() == Fence::Status::Unsignaled) {
147-
// If one fence has signaled and the other hasn't, the unsignaled
148-
// fence will approximately correspond with the correct timestamp.
149-
// There's a small race if both fences signal at about the same time
150-
// and their statuses are retrieved with unfortunate timing. However,
151-
// by this point, they will have both signaled and only the timestamp
152-
// will be slightly off; any dependencies after this point will
153-
// already have been met.
154-
prevFence = std::move(currentFence);
155-
}
156-
}
132+
mergeFence(handle->name.c_str(), future.get().value_or(Fence::NO_FENCE), prevFence);
157133
}
158134
handle->previousReleaseFence = prevFence;
159135
handle->previousReleaseFences.clear();

‎services/surfaceflinger/Utils/FenceUtils.h‎

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,51 @@
1+
/**
2+
* Copyright (C) 2024 The Android Open Source Project
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* http://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
17+
#pragma once
18+
19+
#include <ui/Fence.h>
20+
21+
namespace android {
22+
23+
// TODO: measure if Fence::merge is cheaper
24+
inline void mergeFence(const char* debugName, sp<Fence>&& incomingFence, sp<Fence>& prevFence) {
25+
if (prevFence == nullptr && incomingFence->getStatus() != Fence::Status::Invalid) {
26+
prevFence = std::move(incomingFence);
27+
} else if (prevFence != nullptr) {
28+
// If both fences are signaled or both are unsignaled, we need to merge
29+
// them to get an accurate timestamp.
30+
if (prevFence->getStatus() != Fence::Status::Invalid &&
31+
prevFence->getStatus() == incomingFence->getStatus()) {
32+
char fenceName[32] = {};
33+
snprintf(fenceName, 32, "%.28s", debugName);
34+
sp<Fence> mergedFence = Fence::merge(fenceName, prevFence, incomingFence);
35+
if (mergedFence->isValid()) {
36+
prevFence = std::move(mergedFence);
37+
}
38+
} else if (incomingFence->getStatus() == Fence::Status::Unsignaled) {
39+
// If one fence has signaled and the other hasn't, the unsignaled
40+
// fence will approximately correspond with the correct timestamp.
41+
// There's a small race if both fences signal at about the same time
42+
// and their statuses are retrieved with unfortunate timing. However,
43+
// by this point, they will have both signaled and only the timestamp
44+
// will be slightly off; any dependencies after this point will
45+
// already have been met.
46+
prevFence = std::move(incomingFence);
47+
}
48+
}
49+
}
50+
51+
} // namespace android

‎services/surfaceflinger/common/FlagManager.cpp‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -128,6 +128,7 @@ void FlagManager::dump(std::string& result) const {
128128
DUMP_READ_ONLY_FLAG(fp16_client_target);
129129
DUMP_READ_ONLY_FLAG(game_default_frame_rate);
130130
DUMP_READ_ONLY_FLAG(enable_layer_command_batching);
131+
DUMP_READ_ONLY_FLAG(screenshot_fence_preservation);
131132
DUMP_READ_ONLY_FLAG(vulkan_renderengine);
132133
#undef DUMP_READ_ONLY_FLAG
133134
#undef DUMP_SERVER_FLAG
@@ -203,6 +204,7 @@ FLAG_MANAGER_READ_ONLY_FLAG(display_protected, "")
203204
FLAG_MANAGER_READ_ONLY_FLAG(fp16_client_target, "debug.sf.fp16_client_target")
204205
FLAG_MANAGER_READ_ONLY_FLAG(game_default_frame_rate, "")
205206
FLAG_MANAGER_READ_ONLY_FLAG(enable_layer_command_batching, "")
207+
FLAG_MANAGER_READ_ONLY_FLAG(screenshot_fence_preservation, "debug.sf.screenshot_fence_preservation")
206208
FLAG_MANAGER_READ_ONLY_FLAG(vulkan_renderengine, "debug.renderengine.vulkan")
207209

208210
/// Trunk stable server flags ///

‎services/surfaceflinger/common/include/common/FlagManager.h‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,7 @@ class FlagManager {
6868
bool fp16_client_target() const;
6969
bool game_default_frame_rate() const;
7070
bool enable_layer_command_batching() const;
71+
bool screenshot_fence_preservation() const;
7172
bool vulkan_renderengine() const;
7273

7374
protected:

0 commit comments

Comments
 (0)