Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Do not call Animator::Delegate::OnAnimatorNotifyIdle until at least one frame has been rendered. #29015

Merged
merged 5 commits into from
Oct 7, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions shell/common/animator.cc
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,7 @@ void Animator::BeginFrame(
delegate_.OnAnimatorBeginFrame(frame_target_time, frame_number);
}

if (!frame_scheduled_) {
if (!frame_scheduled_ && has_rendered_) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you know what the magic number 100000 down on line 172 is for? Sorry it isn't part of your PR, but if you're in the neighborhood and you know what it is and why it has that value, could you make a constant for it and document it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK this was done by some Fuchsia folks to try to avoid kicking off a GC when we're about to animate. I'm not sure where it came from though, and it's not well tested.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose it's to make up for the fact that we just delayed 51ms but I'm really not sure. I'm going to land this as-is, and if Nathan knows what's up maybe we can add more docs there.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh ok thanks Jason

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The magic number "100ms" was meant to be enough time to run the compactor but not so much time that things wouldn't be awful if we've incorrectly detected idle time. 100ms probably isn't enough time for slower devices or larger heaps. This logic should probably be moved to the engine's task runners / scheduler to properly detect idle time, and invoke Dart_NotifyIdle with a much further deadline.

// Under certain workloads (such as our parent view resizing us, which is
// communicated to us by repeat viewport metrics events), we won't
// actually have a frame scheduled yet, despite the fact that we *will* be
Expand Down Expand Up @@ -177,6 +177,7 @@ void Animator::BeginFrame(
}

void Animator::Render(std::unique_ptr<flutter::LayerTree> layer_tree) {
has_rendered_ = true;
if (dimension_change_pending_ &&
layer_tree->frame_size() != last_layer_tree_size_) {
dimension_change_pending_ = false;
Expand Down Expand Up @@ -268,9 +269,10 @@ void Animator::AwaitVSync() {
}
}
});

delegate_.OnAnimatorNotifyIdle(
dart_frame_deadline_.ToEpochDelta().ToMicroseconds());
if (has_rendered_) {
delegate_.OnAnimatorNotifyIdle(
dart_frame_deadline_.ToEpochDelta().ToMicroseconds());
}
}

void Animator::ScheduleSecondaryVsyncCallback(uintptr_t id,
Expand Down
1 change: 1 addition & 0 deletions shell/common/animator.h
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,7 @@ class Animator final {
bool dimension_change_pending_ = false;
SkISize last_layer_tree_size_ = {0, 0};
std::deque<uint64_t> trace_flow_ids_;
bool has_rendered_ = false;

fml::WeakPtrFactory<Animator> weak_factory_;

Expand Down
105 changes: 104 additions & 1 deletion shell/common/animator_unittests.cc
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,25 @@
namespace flutter {
namespace testing {

class FakeAnimatorDelegate : public Animator::Delegate {
public:
void OnAnimatorBeginFrame(fml::TimePoint frame_target_time,
uint64_t frame_number) override {}

void OnAnimatorNotifyIdle(int64_t deadline) override {
notify_idle_called_ = true;
}

void OnAnimatorDraw(
std::shared_ptr<Pipeline<flutter::LayerTree>> pipeline,
std::unique_ptr<FrameTimingsRecorder> frame_timings_recorder) override {}

void OnAnimatorDrawLastLayerTree(
std::unique_ptr<FrameTimingsRecorder> frame_timings_recorder) override {}

bool notify_idle_called_ = false;
};

TEST_F(ShellTest, VSyncTargetTime) {
// Add native callbacks to listen for window.onBeginFrame
int64_t target_time;
Expand Down Expand Up @@ -103,7 +122,8 @@ TEST_F(ShellTest, AnimatorStartsPaused) {
auto settings = CreateSettingsForFixture();
TaskRunners task_runners = GetTaskRunnersForFixture();

auto shell = CreateShell(std::move(settings), task_runners);
auto shell = CreateShell(std::move(settings), task_runners,
/* simulate_vsync=*/true);
ASSERT_TRUE(DartVMRef::IsInstanceRunning());

auto configuration = RunConfiguration::InferFromSettings(settings);
Expand All @@ -120,5 +140,88 @@ TEST_F(ShellTest, AnimatorStartsPaused) {
ASSERT_FALSE(DartVMRef::IsInstanceRunning());
}

TEST_F(ShellTest, AnimatorDoesNotNotifyIdleBeforeRender) {
FakeAnimatorDelegate delegate;
TaskRunners task_runners = {
"test",
CreateNewThread(), // platform
CreateNewThread(), // raster
CreateNewThread(), // ui
CreateNewThread() // io
};

auto clock = std::make_shared<ShellTestVsyncClock>();
fml::AutoResetWaitableEvent latch;
std::shared_ptr<Animator> animator;

auto flush_vsync_task = [&] {
fml::AutoResetWaitableEvent ui_latch;
task_runners.GetUITaskRunner()->PostTask([&] { ui_latch.Signal(); });
do {
clock->SimulateVSync();
} while (ui_latch.WaitWithTimeout(fml::TimeDelta::FromMilliseconds(1)));
latch.Signal();
};

// Create the animator on the UI task runner.
task_runners.GetUITaskRunner()->PostTask([&] {
auto vsync_waiter = static_cast<std::unique_ptr<VsyncWaiter>>(
std::make_unique<ShellTestVsyncWaiter>(task_runners, clock));
animator = std::make_unique<Animator>(delegate, task_runners,
std::move(vsync_waiter));
latch.Signal();
});
latch.Wait();

// Validate it has not notified idle and start it. This will request a frame.
task_runners.GetUITaskRunner()->PostTask([&] {
ASSERT_FALSE(delegate.notify_idle_called_);
animator->Start();
// Immediately request a frame saying it can reuse the last layer tree to
// avoid more calls to BeginFrame by the animator.
animator->RequestFrame(false);
task_runners.GetPlatformTaskRunner()->PostTask(flush_vsync_task);
});
latch.Wait();
ASSERT_FALSE(delegate.notify_idle_called_);

// Validate it has not notified idle and try to render.
task_runners.GetUITaskRunner()->PostDelayedTask(
[&] {
ASSERT_FALSE(delegate.notify_idle_called_);
auto layer_tree =
std::make_unique<LayerTree>(SkISize::Make(600, 800), 1.0);
animator->Render(std::move(layer_tree));
task_runners.GetPlatformTaskRunner()->PostTask(flush_vsync_task);
},
// See kNotifyIdleTaskWaitTime in animator.cc.
fml::TimeDelta::FromMilliseconds(60));
latch.Wait();

// Still hasn't notified idle because there has been no frame request.
task_runners.GetUITaskRunner()->PostTask([&] {
ASSERT_FALSE(delegate.notify_idle_called_);
// False to avoid getting cals to BeginFrame that will request more frames
// before we are ready.
animator->RequestFrame(false);
task_runners.GetPlatformTaskRunner()->PostTask(flush_vsync_task);
});
latch.Wait();

// Now it should notify idle. Make sure it is destroyed on the UI thread.
ASSERT_TRUE(delegate.notify_idle_called_);

// Stop and do one more flush so we can safely clean up on the UI thread.
animator->Stop();
task_runners.GetPlatformTaskRunner()->PostTask(flush_vsync_task);
latch.Wait();

task_runners.GetUITaskRunner()->PostTask([&] {
animator.reset();
latch.Signal();
});
latch.Wait();
}

} // namespace testing
} // namespace flutter