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

Correct the frame timing in 'Rasterizer::Draw' when pipeline is more available #32283

Merged
fluttergithubbot merged 5 commits into from
Jun 3, 2022
Merged

Correct the frame timing in 'Rasterizer::Draw' when pipeline is more available #32283

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ed35fdf
Correct the frame timing in 'Rasterizer::Draw' when pipeline is more …
ColdPaleLight Mar 27, 2022
1d18039
Fix compile error
ColdPaleLight Mar 28, 2022
f36816c
format
ColdPaleLight Apr 19, 2022
7664c4f
merge main into fix_wrong_frame_timing
ColdPaleLight May 19, 2022
a5f4830
Merge branch 'flutter:main' into fix_wrong_frame_timing
ColdPaleLight Jun 2, 2022
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
9 changes: 4 additions & 5 deletions shell/common/animator.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,26 +163,25 @@ void Animator::Render(std::unique_ptr<flutter::LayerTree> layer_tree) {
delegate_.OnAnimatorUpdateLatestFrameTargetTime(
frame_timings_recorder_->GetVsyncTargetTime());

auto layer_tree_item = std::make_unique<LayerTreeItem>(
std::move(layer_tree), std::move(frame_timings_recorder_));
// Commit the pending continuation.
PipelineProduceResult result =
producer_continuation_.Complete(std::move(layer_tree));
producer_continuation_.Complete(std::move(layer_tree_item));

if (!result.success) {
frame_timings_recorder_.reset();
FML_DLOG(INFO) << "No pending continuation to commit";
return;
}

if (!result.is_first_item) {
frame_timings_recorder_.reset();
// It has been successfully pushed to the pipeline but not as the first
// item. Eventually the 'Rasterizer' will consume it, so we don't need to
// notify the delegate.
return;
}

delegate_.OnAnimatorDraw(layer_tree_pipeline_,
std::move(frame_timings_recorder_));
delegate_.OnAnimatorDraw(layer_tree_pipeline_);
}

const std::weak_ptr<VsyncWaiter> Animator::GetVsyncWaiter() const {
Expand Down
5 changes: 1 addition & 4 deletions shell/common/animator.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,7 @@ class Animator final {
fml::TimePoint frame_target_time) = 0;

virtual void OnAnimatorDraw(
std::shared_ptr<Pipeline<flutter::LayerTree>> pipeline,
std::unique_ptr<FrameTimingsRecorder> frame_timings_recorder) = 0;
std::shared_ptr<LayerTreePipeline> pipeline) = 0;

virtual void OnAnimatorDrawLastLayerTree(
std::unique_ptr<FrameTimingsRecorder> frame_timings_recorder) = 0;
Expand Down Expand Up @@ -83,8 +82,6 @@ class Animator final {
void EnqueueTraceFlowId(uint64_t trace_flow_id);

private:
using LayerTreePipeline = Pipeline<flutter::LayerTree>;

void BeginFrame(std::unique_ptr<FrameTimingsRecorder> frame_timings_recorder);

bool CanReuseLastLayerTree();
Expand Down
6 changes: 2 additions & 4 deletions shell/common/animator_unittests.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,10 +35,8 @@ class FakeAnimatorDelegate : public Animator::Delegate {
MOCK_METHOD1(OnAnimatorUpdateLatestFrameTargetTime,
void(fml::TimePoint frame_target_time));

MOCK_METHOD2(
OnAnimatorDraw,
void(std::shared_ptr<Pipeline<flutter::LayerTree>> pipeline,
std::unique_ptr<FrameTimingsRecorder> frame_timings_recorder));
MOCK_METHOD1(OnAnimatorDraw,
void(std::shared_ptr<LayerTreePipeline> pipeline));

void OnAnimatorDrawLastLayerTree(
std::unique_ptr<FrameTimingsRecorder> frame_timings_recorder) override {}
Expand Down
13 changes: 13 additions & 0 deletions shell/common/pipeline.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@
#include <memory>
#include <mutex>

#include "flutter/flow/frame_timings.h"
#include "flutter/flow/layers/layer_tree.h"
#include "flutter/fml/macros.h"
#include "flutter/fml/memory/ref_counted.h"
#include "flutter/fml/synchronization/semaphore.h"
Expand Down Expand Up @@ -219,6 +221,17 @@ class Pipeline {
FML_DISALLOW_COPY_AND_ASSIGN(Pipeline);
};

struct LayerTreeItem {
LayerTreeItem(std::unique_ptr<LayerTree> layer_tree,
std::unique_ptr<FrameTimingsRecorder> frame_timings_recorder)
: layer_tree(std::move(layer_tree)),
frame_timings_recorder(std::move(frame_timings_recorder)) {}
std::unique_ptr<LayerTree> layer_tree;
std::unique_ptr<FrameTimingsRecorder> frame_timings_recorder;
};

using LayerTreePipeline = Pipeline<LayerTreeItem>;

} // namespace flutter

#endif // FLUTTER_SHELL_COMMON_PIPELINE_H_
32 changes: 16 additions & 16 deletions shell/common/rasterizer.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,12 +160,9 @@ void Rasterizer::DrawLastLayerTree(
}
}

RasterStatus Rasterizer::Draw(
std::unique_ptr<FrameTimingsRecorder> frame_timings_recorder,
std::shared_ptr<Pipeline<flutter::LayerTree>> pipeline,
LayerTreeDiscardCallback discard_callback) {
TRACE_EVENT_WITH_FRAME_NUMBER(frame_timings_recorder, "flutter",
"GPURasterizer::Draw");
RasterStatus Rasterizer::Draw(std::shared_ptr<LayerTreePipeline> pipeline,
LayerTreeDiscardCallback discard_callback) {
TRACE_EVENT0("flutter", "GPURasterizer::Draw");
Copy link
Member Author

@ColdPaleLight ColdPaleLight Mar 27, 2022

Choose a reason for hiding this comment

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

Since frame_timings_recorder is in the pipeline, it can't be accessed here. So I changed the code here back to before this PR #26205 and added a trace in DoDraw. It looks like this will have an impact on the dev tools. Please let me know if you guys have any thoughts.

Copy link
Contributor

@iskakaushik iskakaushik May 31, 2022

Choose a reason for hiding this comment

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

This might impact the devtools, as it looks for specific frame number events corresponding to ::Draw. @kenzieschmoll do we still rely on the names of trace events or do we only care about the frame timings API?

Copy link
Member

@kenzieschmoll kenzieschmoll May 31, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

We look for the 'frame_number' arg in the 'GPURasterizer::Draw' trace event and the 'Animator::BeginFrame' trace event to link timeline events to their matching frame number from the FrameTimings API. If this change removes the 'frame_number' argument, this will break DevTools. However, if it just moves the argument to a different event, we can update the event name we look for to include both the legacy name and the new name.

Copy link
Contributor

@iskakaushik iskakaushik Jun 1, 2022

Choose a reason for hiding this comment

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

I filed https://github.com/flutter/flutter/issues/105140, once that is addressed, we can land this PR.

Copy link
Member

@kenzieschmoll kenzieschmoll Jun 1, 2022

Choose a reason for hiding this comment

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

Addressed here: flutter/devtools#4156

if (raster_thread_merger_ &&
!raster_thread_merger_->IsOnRasterizingThread()) {
// we yield and let this frame be serviced on the right thread.
Expand All @@ -175,13 +172,12 @@ RasterStatus Rasterizer::Draw(
.GetRasterTaskRunner()
->RunsTasksOnCurrentThread());

std::unique_ptr<FrameTimingsRecorder> resubmit_recorder =
frame_timings_recorder->CloneUntil(
FrameTimingsRecorder::State::kBuildEnd);

RasterStatus raster_status = RasterStatus::kFailed;
Pipeline<flutter::LayerTree>::Consumer consumer =
[&](std::unique_ptr<LayerTree> layer_tree) {
LayerTreePipeline::Consumer consumer =
[&](std::unique_ptr<LayerTreeItem> item) {
std::unique_ptr<LayerTree> layer_tree = std::move(item->layer_tree);
std::unique_ptr<FrameTimingsRecorder> frame_timings_recorder =
std::move(item->frame_timings_recorder);
if (discard_callback(*layer_tree.get())) {
raster_status = RasterStatus::kDiscarded;
} else {
Expand All @@ -199,9 +195,11 @@ RasterStatus Rasterizer::Draw(

bool should_resubmit_frame = ShouldResubmitFrame(raster_status);
if (should_resubmit_frame) {
auto resubmitted_layer_tree_item = std::make_unique<LayerTreeItem>(
std::move(resubmitted_layer_tree_), std::move(resubmitted_recorder_));
auto front_continuation = pipeline->ProduceIfEmpty();
PipelineProduceResult result =
front_continuation.Complete(std::move(resubmitted_layer_tree_));
front_continuation.Complete(std::move(resubmitted_layer_tree_item));
if (result.success) {
consume_result = PipelineConsumeResult::MoreAvailable;
}
Expand All @@ -224,11 +222,9 @@ RasterStatus Rasterizer::Draw(
delegate_.GetTaskRunners().GetRasterTaskRunner()->PostTask(
fml::MakeCopyable(
[weak_this = weak_factory_.GetWeakPtr(), pipeline,
resubmit_recorder = std::move(resubmit_recorder),
discard_callback = std::move(discard_callback)]() mutable {
if (weak_this) {
weak_this->Draw(std::move(resubmit_recorder), pipeline,
std::move(discard_callback));
weak_this->Draw(pipeline, std::move(discard_callback));
}
}));
break;
Expand Down Expand Up @@ -395,6 +391,8 @@ fml::Milliseconds Rasterizer::GetFrameBudget() const {
RasterStatus Rasterizer::DoDraw(
std::unique_ptr<FrameTimingsRecorder> frame_timings_recorder,
std::unique_ptr<flutter::LayerTree> layer_tree) {
TRACE_EVENT_WITH_FRAME_NUMBER(frame_timings_recorder, "flutter",
"Rasterizer::DoDraw");
FML_DCHECK(delegate_.GetTaskRunners()
.GetRasterTaskRunner()
->RunsTasksOnCurrentThread());
Expand All @@ -412,6 +410,8 @@ RasterStatus Rasterizer::DoDraw(
last_layer_tree_ = std::move(layer_tree);
} else if (ShouldResubmitFrame(raster_status)) {
resubmitted_layer_tree_ = std::move(layer_tree);
resubmitted_recorder_ = frame_timings_recorder->CloneUntil(
FrameTimingsRecorder::State::kBuildEnd);
return raster_status;
} else if (raster_status == RasterStatus::kDiscarded) {
return raster_status;
Expand Down
7 changes: 3 additions & 4 deletions shell/common/rasterizer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,10 +241,8 @@ class Rasterizer final : public SnapshotDelegate,
/// @param[in] discard_callback if specified and returns true, the layer tree
/// is discarded instead of being rendered
///
RasterStatus Draw(
std::unique_ptr<FrameTimingsRecorder> frame_timings_recorder,
std::shared_ptr<Pipeline<flutter::LayerTree>> pipeline,
LayerTreeDiscardCallback discard_callback = NoDiscard);
RasterStatus Draw(std::shared_ptr<LayerTreePipeline> pipeline,
LayerTreeDiscardCallback discard_callback = NoDiscard);

//----------------------------------------------------------------------------
/// @brief The type of the screenshot to obtain of the previously
Expand Down Expand Up @@ -509,6 +507,7 @@ class Rasterizer final : public SnapshotDelegate,
// has not successfully rasterized. This can happen due to the change in the
// thread configuration. This will be inserted to the front of the pipeline.
std::unique_ptr<flutter::LayerTree> resubmitted_layer_tree_;
std::unique_ptr<FrameTimingsRecorder> resubmitted_recorder_;
fml::closure next_frame_callback_;
bool user_override_resource_cache_bytes_;
std::optional<size_t> max_cache_bytes_;
Expand Down
Loading