Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.
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
3 changes: 3 additions & 0 deletions shell/common/animator.cc
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,9 @@ void Animator::Render(std::unique_ptr<flutter::LayerTree> layer_tree) {
"Animator::Render");
frame_timings_recorder_->RecordBuildEnd(fml::TimePoint::Now());

delegate_.OnAnimatorUpdateLatestFrameTargetTime(
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this safe to access without a lock?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it's safe, the lock is in Shell::OnAnimatorUpdateLatestFrameTargetTime

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I meant for this comment to be in the line below this (frame_timings_recorder_->GetVsyncTargetTime()

Copy link
Contributor

Choose a reason for hiding this comment

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

I see now though that takes a lock by itself.

frame_timings_recorder_->GetVsyncTargetTime());

// Commit the pending continuation.
PipelineProduceResult result =
producer_continuation_.Complete(std::move(layer_tree));
Expand Down
3 changes: 3 additions & 0 deletions shell/common/animator.h
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,9 @@ class Animator final {

virtual void OnAnimatorNotifyIdle(fml::TimePoint deadline) = 0;

virtual void OnAnimatorUpdateLatestFrameTargetTime(
fml::TimePoint frame_target_time) = 0;

virtual void OnAnimatorDraw(
std::shared_ptr<Pipeline<flutter::LayerTree>> pipeline,
std::unique_ptr<FrameTimingsRecorder> frame_timings_recorder) = 0;
Expand Down
12 changes: 9 additions & 3 deletions shell/common/animator_unittests.cc
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,9 @@ class FakeAnimatorDelegate : public Animator::Delegate {
notify_idle_called_ = true;
}

MOCK_METHOD1(OnAnimatorUpdateLatestFrameTargetTime,
void(fml::TimePoint frame_target_time));

MOCK_METHOD2(
OnAnimatorDraw,
void(std::shared_ptr<Pipeline<flutter::LayerTree>> pipeline,
Expand Down Expand Up @@ -222,9 +225,12 @@ TEST_F(ShellTest, AnimatorDoesNotNotifyDelegateIfPipelineIsNotEmpty) {
[&](fml::TimePoint frame_target_time, uint64_t frame_number) {
begin_frame_latch.Signal();
});

// It will only be called once even though we call the method Animator::Render
// twice. because it will only be called when the pipeline is empty.
// It must always be called when the method 'Animator::Render' is called,
// regardless of whether the pipeline is empty or not.
EXPECT_CALL(delegate, OnAnimatorUpdateLatestFrameTargetTime).Times(2);
// It will only be called once even though we call the method
// 'Animator::Render' twice. because it will only be called when the pipeline
// is empty.
EXPECT_CALL(delegate, OnAnimatorDraw).Times(1);

for (int i = 0; i < 2; i++) {
Expand Down
15 changes: 9 additions & 6 deletions shell/common/shell.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1106,23 +1106,26 @@ void Shell::OnAnimatorNotifyIdle(fml::TimePoint deadline) {
}
}

// |Animator::Delegate|
void Shell::OnAnimatorDraw(
std::shared_ptr<Pipeline<flutter::LayerTree>> pipeline,
std::unique_ptr<FrameTimingsRecorder> frame_timings_recorder) {
void Shell::OnAnimatorUpdateLatestFrameTargetTime(
fml::TimePoint frame_target_time) {
FML_DCHECK(is_setup_);

// record the target time for use by rasterizer.
{
std::scoped_lock time_recorder_lock(time_recorder_mutex_);
const fml::TimePoint frame_target_time =
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we still need the lock here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we still need it, because the Rasterizer will still call Shell::GetLatestFrameTargetTime to access latest_frame_target_time_ on the raster thread.

frame_timings_recorder->GetVsyncTargetTime();
if (!latest_frame_target_time_) {
latest_frame_target_time_ = frame_target_time;
} else if (latest_frame_target_time_ < frame_target_time) {
latest_frame_target_time_ = frame_target_time;
}
}
}

// |Animator::Delegate|
void Shell::OnAnimatorDraw(
std::shared_ptr<Pipeline<flutter::LayerTree>> pipeline,
std::unique_ptr<FrameTimingsRecorder> frame_timings_recorder) {
FML_DCHECK(is_setup_);

auto discard_callback = [this](flutter::LayerTree& tree) {
std::scoped_lock<std::mutex> lock(resize_mutex_);
Expand Down
4 changes: 4 additions & 0 deletions shell/common/shell.h
Original file line number Diff line number Diff line change
Expand Up @@ -579,6 +579,10 @@ class Shell final : public PlatformView::Delegate,
// |Animator::Delegate|
void OnAnimatorNotifyIdle(fml::TimePoint deadline) override;

// |Animator::Delegate|
void OnAnimatorUpdateLatestFrameTargetTime(
fml::TimePoint frame_target_time) override;

// |Animator::Delegate|
void OnAnimatorDraw(
std::shared_ptr<Pipeline<flutter::LayerTree>> pipeline,
Expand Down