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 intoflutter:mainfrom
ColdPaleLight:fix_wrong_frame_timing
Jun 3, 2022
Merged

Correct the frame timing in 'Rasterizer::Draw' when pipeline is more available#32283
fluttergithubbot merged 5 commits intoflutter:mainfrom
ColdPaleLight:fix_wrong_frame_timing

Conversation

@ColdPaleLight
Copy link
Member

@ColdPaleLight ColdPaleLight commented Mar 27, 2022 •
edited
Loading

The current implementation always uses resubmit_recorder as the frame_timings_recorder for the next Rasterizer::Draw when consume_result is PipelineConsumeResult::MoreAvailable. However, in fact, we should use resubmit_recorder only when should_resubmit_frame is true, and for other cases, we should use the frame_timing_recorder corresponding to the next item in the pipeline.

In this PR, I introduced a new struct LayerTreeItem to store layer_tree and frame_timings_recorder, and push the LayerTreeItem into Pipeline. This ensures that the correct frame_timings_recorder is used in Rasterizer::Draw;

fix flutter/flutter#96699

c.f.

engine/shell/common/rasterizer.cc

Line 226 in 98962ee

resubmit_recorder = std::move(resubmit_recorder),

Pre-launch Checklist

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • I read the [Tree Hygiene] wiki page, which explains my responsibilities.
  • I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides].
  • I listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See [testing the engine] for instructions on
    writing and running engine tests.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the [CLA].
  • All existing and new tests are passing.

@ColdPaleLight ColdPaleLight mentioned this pull request Mar 27, 2022
Closed
@ColdPaleLight ColdPaleLight requested review from chinmaygarde, iskakaushik and jason-simmons and removed request for chinmaygarde and iskakaushik March 27, 2022 13:49
ColdPaleLight
ColdPaleLight commented Mar 27, 2022
View reviewed changes
shell/common/rasterizer.cc
"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

@chinmaygarde chinmaygarde mentioned this pull request Mar 28, 2022
Closed
@zanderso
Copy link
Member

zanderso commented Mar 31, 2022

From PR review triage: @chinmaygarde @iskakaushik is this ready for a review?

@chinmaygarde
Copy link
Contributor

chinmaygarde commented Apr 14, 2022

I don't have the cycles to review this ATM. I'll bring this up with Kaushik.

@ColdPaleLight ColdPaleLight force-pushed the fix_wrong_frame_timing branch from 2a882e3 to f36816c Compare April 19, 2022 06:19
@iskakaushik iskakaushik self-assigned this Apr 28, 2022
@chinmaygarde chinmaygarde removed their request for review April 28, 2022 20:27
@chinmaygarde chinmaygarde removed the request for review from jason-simmons May 19, 2022 20:12
@ColdPaleLight
Copy link
Member Author

ColdPaleLight commented May 19, 2022

friendly ping @iskakaushik

@iskakaushik
Copy link
Contributor

iskakaushik commented May 31, 2022

This change looks good to me overall, I will wait for Kenzie's response on this reg. devtools.

@iskakaushik iskakaushik mentioned this pull request Jun 1, 2022
Closed
@zanderso
Copy link
Member

zanderso commented Jun 2, 2022

I see the devtools change has landed. Is this one ready to go?

iskakaushik
iskakaushik approved these changes Jun 3, 2022
View reviewed changes
Copy link
Contributor

@iskakaushik iskakaushik left a comment

Choose a reason for hiding this comment

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

LGTM

@iskakaushik iskakaushik added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Jun 3, 2022
@iskakaushik
Copy link
Contributor

iskakaushik commented Jun 3, 2022

@ColdPaleLight thanks for the contribution.

@fluttergithubbot fluttergithubbot merged commit 14194cb into flutter:main Jun 3, 2022
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jun 3, 2022
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 3, 2022
@engine-flutter-autoroll
14194cb Correct the frame timing in 'Rasterizer::Draw' when pipeline …
ff69d33 
…is more available (flutter/engine#32283)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jun 3, 2022
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 3, 2022
@engine-flutter-autoroll
14194cb Correct the frame timing in 'Rasterizer::Draw' when pipeline …
6025318 
…is more available (flutter/engine#32283)
houhuayong pushed a commit to houhuayong/engine that referenced this pull request Jun 21, 2022
@ColdPaleLight @houhuayong
Correct the frame timing in 'Rasterizer::Draw' when pipeline is more …
e2a05da 
…available (flutter#32283)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@kenzieschmoll kenzieschmoll kenzieschmoll left review comments

@iskakaushik iskakaushik iskakaushik approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@iskakaushik iskakaushik

Labels

waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

addTimingsCallback return wrong timestamp.

6 participants

@ColdPaleLight @zanderso @chinmaygarde @iskakaushik @kenzieschmoll @fluttergithubbot