-
Notifications
You must be signed in to change notification settings - Fork 6k
Correct the frame timing in 'Rasterizer::Draw' when pipeline is more available #32283
Correct the frame timing in 'Rasterizer::Draw' when pipeline is more available #32283
Conversation
"GPURasterizer::Draw"); | ||
RasterStatus Rasterizer::Draw(std::shared_ptr<LayerTreePipeline> pipeline, | ||
LayerTreeDiscardCallback discard_callback) { | ||
TRACE_EVENT0("flutter", "GPURasterizer::Draw"); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
From PR review triage: @chinmaygarde @iskakaushik is this ready for a review? |
I don't have the cycles to review this ATM. I'll bring this up with Kaushik. |
2a882e3
to
f36816c
Compare
friendly ping @iskakaushik |
This change looks good to me overall, I will wait for Kenzie's response on this reg. devtools. |
I see the devtools change has landed. Is this one ready to go? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@ColdPaleLight thanks for the contribution. |
…is more available (flutter/engine#32283)
…is more available (flutter/engine#32283)
The current implementation always uses
resubmit_recorder
as theframe_timings_recorder
for the nextRasterizer::Draw
whenconsume_result
isPipelineConsumeResult::MoreAvailable
. However, in fact, we should useresubmit_recorder
only whenshould_resubmit_frame
istrue
, and for other cases, we should use theframe_timing_recorder
corresponding to the next item in the pipeline.In this PR, I introduced a new struct
LayerTreeItem
to storelayer_tree
andframe_timings_recorder
, and push theLayerTreeItem
intoPipeline
. This ensures that the correctframe_timings_recorder
is used inRasterizer::Draw
;fix flutter/flutter#96699
c.f.
engine/shell/common/rasterizer.cc
Line 226 in 98962ee
Pre-launch Checklist
writing and running engine tests.
///
).