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

Make sure that 'BeginFrame' and 'EndFrame' of the 'ExternalViewEmbedder' are paired#32058

Merged
fluttergithubbot merged 4 commits into
flutter-team-archive:mainfrom
ColdPaleLight:begin_frame_and_end_frame
Mar 24, 2022
Merged

Make sure that 'BeginFrame' and 'EndFrame' of the 'ExternalViewEmbedder' are paired#32058
fluttergithubbot merged 4 commits into
flutter-team-archive:mainfrom
ColdPaleLight:begin_frame_and_end_frame

Conversation

@ColdPaleLight

@ColdPaleLight ColdPaleLight commented Mar 16, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

fix flutter/flutter#100207

There is still a situation where BeginFrame and EndFrame are not paired. For example, when RasterStatus is RasterStatus::kDiscarded, BeginFrame is not called, but EndFrame is called.

And when we try to pair BeginFrame and EndFrame, the tests ShellTest.DiscardLayerTreeOnResize and ShellTest.DiscardResubmittedLayerTreeOnResize are failed. so we have two options:

  1. Disable these tests:
    We're actually going to do that Disable several tests due to flakiness #32059 , and the reason for doing it is that these tests introduce this Issue: ShellTest.DiscardLayerTreeOnResize is flaky on presubmits. flutter/flutter#95751

  2. Introduce a new mechanism, such as adding a closure called draw_layer_tree_finished_callback in Settings.
    At the end of each Rasterizer::Draw and Rasterizer::DrawLastLayerTree, no matter whether it is succeeded or failed, this closure is called back, and this mechanism is used to replace the ExternalViewEmbedder in these tests.

cc @dnfield @cyanglaz @blasten

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.

cyanglaz
cyanglaz reviewed Mar 16, 2022
View reviewed changes

@cyanglaz cyanglaz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code LGTM % nits.
As for the tests, @iskakaushik is trying to de-flake the tests, so we should try to go with a solution where we don't need to disable the tests.

Comment thread flow/embedded_views.h Outdated

// Change the flag about whether it is used in this frame, it will be set to
// true when 'BeginFrame' and false when 'EndFrame'.
void set_used_this_frame(bool used_this_frame) {

@cyanglaz cyanglaz Mar 16, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nits: void SetUsedThisFrame(bool used_this_frame)

@ColdPaleLight ColdPaleLight Mar 17, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

Comment thread flow/embedded_views.h Outdated

// Whether it is used in this frame, returns true between 'BeginFrame' and
// 'EndFrame', otherwise returns false.
bool used_this_frame() const { return used_this_frame_; }

@cyanglaz cyanglaz Mar 16, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nits: bool GetUsedThisFrame() or bool IsFrameUsed()

@ColdPaleLight ColdPaleLight Mar 17, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

Comment thread shell/common/rasterizer.cc Outdated
external_view_embedder_->BeginFrame(
layer_tree.frame_size(), surface_->GetContext(),
layer_tree.device_pixel_ratio(), raster_thread_merger_);
external_view_embedder_->set_used_this_frame(true);

@cyanglaz cyanglaz Mar 16, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe add a: FML_DCHECK(!external_view_embedder_->used_this_frame())

@ColdPaleLight ColdPaleLight Mar 17, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

@cyanglaz

cyanglaz commented Mar 16, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

It seems that we are disabling the tests. If we make this change and disabling the tests in #32059 (comment), we will have to fix the tests for this change when re-enabling them.

CC @iskakaushik

@ColdPaleLight ColdPaleLight force-pushed the begin_frame_and_end_frame branch from 3754158 to 28fc23d Compare March 17, 2022 13:30
@ColdPaleLight ColdPaleLight mentioned this pull request Mar 17, 2022
Open
@ColdPaleLight

ColdPaleLight commented Mar 17, 2022

Copy link
Copy Markdown
Contributor Author

@cyanglaz @iskakaushik
I filed a new issue flutter/flutter#100299 and add TODO with linked issue to the tests ShellTest.DiscardLayerTreeOnResize and ShellTest.DiscardResubmittedLayerTreeOnResize.

chinmaygarde
chinmaygarde approved these changes Mar 24, 2022
View reviewed changes

@chinmaygarde chinmaygarde left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I believe the pattern followed elsewhere in the engine is to use an RAII wrapper that automatically inserts the end frame call. See flutter::CompositorContext::ScopedFrame. But this is fine too.

@ColdPaleLight ColdPaleLight 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 Mar 24, 2022
@fluttergithubbot fluttergithubbot merged commit 98962ee into flutter-team-archive:main Mar 24, 2022
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Mar 25, 2022
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 25, 2022
@engine-flutter-autoroll
98962ee Make sure that 'BeginFrame' and 'EndFrame' of the 'ExternalVi…
e12a4a2 
…ewEmbedder' are paired (flutter-team-archive/engine#32058)
zanderso pushed a commit to flutter/flutter that referenced this pull request Mar 25, 2022
@engine-flutter-autoroll
Roll Engine from 816c865 to f17ee9d (5 revisions) (#100733)
9d0570a 
* 98962ee Make sure that 'BeginFrame' and 'EndFrame' of the 'ExternalViewEmbedder' are paired (flutter-team-archive/engine#32058)

* c62b0e2 Roll Skia from 3b238ceae9c3 to 8318cc9928ef (1 revision) (flutter-team-archive/engine#32249)

* f049e57 Explicitly cast fuchsia.io constants to uint32_t (flutter-team-archive/engine#32250)

* 5804870 Roll Skia from 8318cc9928ef to ae5984082b5e (3 revisions) (flutter-team-archive/engine#32251)

* f17ee9d Roll buildroot to 9e88510. (flutter-team-archive/engine#32248)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@blasten blasten Awaiting requested review from blasten
@dnfield dnfield Awaiting requested review from dnfield
2 more reviewers
@cyanglaz cyanglaz cyanglaz left review comments
@chinmaygarde chinmaygarde chinmaygarde approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Make sure that 'BeginFrame' and 'EndFrame' of the 'ExternalViewEmbedder' are paired

4 participants

@ColdPaleLight @cyanglaz @chinmaygarde @fluttergithubbot