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

Conversation

blasten
Copy link

@blasten blasten commented Jun 24, 2020

Description

EndFrame should be called by the rasterizer as it is used to restore up state or recycle the overlay surfaces.

Related Issues

Fixes flutter/flutter#60124

Tests

I added the following tests: updated unit tests

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

  • I read the contributor guide and followed the process outlined there for submitting PRs.
  • I signed the CLA.
  • I read and followed the C++, Objective-C, Java style guides for the engine.
  • I read the tree hygiene wiki page, which explains my responsibilities.
  • I updated/added relevant documentation.
  • All existing and new tests are passing.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Did any tests fail when you ran them? Please read handling breaking changes.

@blasten
Copy link
Author

blasten commented Jun 24, 2020

@cyanglaz I verified this fixes the perf issue you were seeing.

Copy link
Contributor

@cyanglaz cyanglaz left a comment

Choose a reason for hiding this comment

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

LGTM modulo nits. For testing, we need to make sure the existing tests related to platform views don't fail.

// This method provides the embedder a way to do additional tasks after
// |SubmitFrame|. After invoking this method, the current task on the
// TaskRunner should end immediately.
// |SubmitFrame|. For example, merge tasks if `should_resubmit_frame` is true.
Copy link
Contributor

Choose a reason for hiding this comment

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

nits: merge task runners?

Copy link
Author

Choose a reason for hiding this comment

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

Done.

TRACE_EVENT0("flutter", "IOSSurface::EndFrame");
FML_CHECK(platform_views_controller_ != nullptr);
return platform_views_controller_->EndFrame(raster_thread_merger);
return platform_views_controller_->EndFrame(raster_status, raster_thread_merger);
Copy link
Contributor

Choose a reason for hiding this comment

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

platform_views_controller_->EndFrame(should_resubmit_frame, raster_thread_merger); ?

Copy link
Author

Choose a reason for hiding this comment

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

Done.

// causes|RasterStatus::kResubmit|. Check to make sure.
FML_DCHECK(external_view_embedder != nullptr);
external_view_embedder->EndFrame(raster_thread_merger_);
if (surface_ != nullptr && surface_->GetExternalViewEmbedder() != nullptr) {
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe also modify the https://github.com/flutter/engine/blob/master/shell/common/shell_unittests.cc#L472 test to make sure the should_resubmit_frame flag is passed correctly?

Copy link
Author

Choose a reason for hiding this comment

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

ah, didn't know there was that test

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Rasterizer should always call EndFrame

4 participants