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

Conversation

@cyanglaz
Copy link
Contributor

@cyanglaz cyanglaz commented Dec 1, 2021

This PR makes the DisplayManager determines the frame rate at the beginning of each frame and update the reporting accordingly.

Fixes flutter/flutter#94507

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.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@cyanglaz cyanglaz marked this pull request as draft December 1, 2021 22:51
@cyanglaz cyanglaz marked this pull request as ready for review December 3, 2021 21:55
@godofredoc
Copy link
Contributor

Please rebase this change to fix the windows UWP build

Comment on lines +1077 to +1079
display_manager_->UpdateRefreshRateIfNecessary(
flutter::DisplayUpdateType::kUpdateRefreshRate, vsync_start_time,
frame_target_time);
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the cost of this on iOS? We know for sure that on some Android platforms it's pretty expensive (e.g. 2-5ms) and this is during frameworkload.

Can we instead listen to refresh rate updates from the OS somehow?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you elaborate how this is expensive on Android. This method is essentially a no-op if framerate doesn't change. See: https://github.com/flutter/engine/pull/30054/files#diff-c9141b5122b7cb9dcc29a05b1660dbb92c1c6ea8e0df831c96b42708fa1e20b5R66
(I should probably need to update the code to return early so it is clearer)


double new_frame_rate =
round((1 / (frame_target_time - vsync_start_time).ToSecondsF()));
if (!is_frame_rate_same(GetMainDisplayRefreshRate(), new_frame_rate)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

On Android now this should always return false, but I'm curious if iOS can do something closer to what Android does and just supply a display object that always returns the right value for GetMainDisplayRefreshRate rather than polling/checking every frame.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On iOS, we can only get the frame time from target_time - start_time. We can have a platform(iOS) layer object to manage that calculation so it is abstracted from other platforms.

I think it is true for all the platforms that target_time - start_time represents the frame duration, and doing such calculation per frame is not expensive?

Copy link
Contributor

Choose a reason for hiding this comment

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

How do you calculate the target_time on iOS?

On Android, it's calculated using the frame rate - requesting the frame rate every frame is expensive on Android in some vendor implementations though.

@cyanglaz
Copy link
Contributor Author

cyanglaz commented Dec 8, 2021

Close in favor of #30223

@cyanglaz cyanglaz closed this Dec 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[engine] Reporting frame rate should always based on the current frame rate

3 participants