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 8, 2021

Create a new VariableRefreshRateDisplay, which replies on a VariableRefreshRateReporter to dynamically report refresh rate.
Make VsyncWaiterIOS a VariableRefreshRateReporter that reports the refresh rate dynamically.

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
Copy link
Contributor Author

friendly ping @dnfield @iskakaushik :)

Copy link
Contributor

@dnfield dnfield left a comment

Choose a reason for hiding this comment

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

LGTM

#include <optional>

#include "flutter/fml/macros.h"
#include "variable_refresh_rate_reporter.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: full path for the include.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

return runtime_controller_.get();
}

const std::shared_ptr<VsyncWaiter> GetVsyncWaiter() const;
Copy link
Contributor

Choose a reason for hiding this comment

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

given that engine will outlive the vsync waiter, can we hand a reference rather than handing out a shared pointer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

void UpdateRefreshRate(double refresh_rate);

// |RefreshRateReporter|
double GetRefreshRate() override;
Copy link
Contributor

Choose a reason for hiding this comment

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

this can be const

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@chinmaygarde
Copy link
Member

Any updates on @iskakaushik s queries?

@cyanglaz
Copy link
Contributor Author

cyanglaz commented Jan 7, 2022

@chinmaygarde I was sick the last several days. I'm currently looking into another issue and will update this asap.

Copy link
Contributor Author

@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.

@iskakaushik Updated based on your review comments! PTAL.

#include <optional>

#include "flutter/fml/macros.h"
#include "variable_refresh_rate_reporter.h"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

return runtime_controller_.get();
}

const std::shared_ptr<VsyncWaiter> GetVsyncWaiter() const;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

void UpdateRefreshRate(double refresh_rate);

// |RefreshRateReporter|
double GetRefreshRate() override;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

namespace testing {

TEST(VariableRefreshRateDisplayTest, ReportCorrectInitialRefreshRate) {
auto refresh_rate_reporter = std::make_shared<TestRefreshRateReporter>(60);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: no need for make shared in these tests anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

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

@cyanglaz cyanglaz 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 Jan 11, 2022
@fluttergithubbot fluttergithubbot merged commit f1b4548 into flutter:main Jan 11, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 11, 2022
zanderso pushed a commit to flutter/flutter that referenced this pull request Jan 12, 2022
* f5ebc91 Roll Fuchsia Linux SDK from DS-3oCidC... to 3-zk7OPWY... (flutter/engine#30792)

* 1064abe Roll Skia from 7a14f783bda3 to 41e994f735d9 (2 revisions) (flutter/engine#30793)

* a4a505b Revert "Roll Fuchsia Linux SDK from DS-3oCidC... to 3-zk7OPWY... (#30792)" (flutter/engine#30798)

* 6389ee3 [fuchsia] Bump buildroot/ to fix Fuchsia SDK roll (flutter/engine#30794)

* f94c8e0 Implemented library uri support for FlutterFragmentActivities (flutter/engine#30790)

* c180748 Roll Fuchsia Mac SDK from 9rr72kcgt... to LxyiyZWks... (flutter/engine#30796)

* 6927c00 Roll Dart SDK from 3061b1d4c117 to 099f7f57767c (5 revisions) (flutter/engine#30797)

* d889c0c Roll Skia from 41e994f735d9 to 345587df72ae (3 revisions) (flutter/engine#30800)

* d53514e Remove unused setPersistentIsolateData function (flutter/engine#30795)

* f1b4548 Variable Refresh Rate Display (flutter/engine#30223)

* 0139f6e Roll Skia from 345587df72ae to 30fdea3d8fd5 (2 revisions) (flutter/engine#30802)

* 643b278 Add 'keyCode' field to eventData map created from html keyboard events. (flutter/engine#30801)
JsouLiang pushed a commit to JsouLiang/engine that referenced this pull request Jan 14, 2022
@eggfly
Copy link
Member

eggfly commented Jan 29, 2022

@cyanglaz maybe we can also let Android also use this VariableRefreshRateDisplay class?

@cyanglaz cyanglaz deleted the ios_display branch February 7, 2022 18:36
@cyanglaz
Copy link
Contributor Author

cyanglaz commented Feb 7, 2022

@eggfly Yes, I think it can.

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

Labels

platform-ios 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

Development

Successfully merging this pull request may close these issues.

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

6 participants