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

Conversation

dnfield
Copy link
Contributor

@dnfield dnfield commented Nov 18, 2021

See also #29765

Fixes flutter/flutter#93688
Fixes flutter/flutter#93698

This makes sure the frame timings recorder and vsync waiter implementations get the correct refresh rate if or when it adjusts at runtime. This should be OK becuase we'll only query the refresh rate when the display metrics actually change, which is much less frequent than every frame. I experimented with an NDK implementation in the previous patch, but that vastly restricts the API levels we can support, and currently on API 30 and 31 it just calls Java methods through JNI anyway.

I've refactored the way Display updates are reported so that AndroidDisplay can just dynamically get the refresh rate correctly. These values are used by a service protocol extension and by some Stopwatches on the CompositorContext.

Copy link
Contributor

@ds84182 ds84182 left a comment

Choose a reason for hiding this comment

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

LGTM

@iskakaushik
Copy link
Contributor

If I am understanding this correctly, this change makes it so Android’s VsyncWaiter.java has a DisplayListener which updates the refresh rate held by the vsync waiter, so the next time we call VsyncWaiter.getInstance we get a vsync waiter with the right refresh rate set.

Assuming that is correct, I think an alternative way of achieving the same thing would be to have the DisplayListener notify shell OnDisplayUpdates with a new DisplayUpdateType::kRefreshRateChange, and shell can update the vsync waiter’s refresh rate, and refresh rate of any other component as well. This way the shell is doing most of the heavy lifting and we can adopt the same model to other platforms as well.

What do you think?

@dnfield
Copy link
Contributor Author

dnfield commented Nov 18, 2021

We talked offline - the direction here is to have specific platform implemenations of Display rather than needing to push updates to DisplayManager whenever a single property changes, which should be better as we add more properties to Displays and make things less confusing about which piece of data we last fed to the display manager vs. what the system currently says about refresh rate.

It might be worth moving DisplayListener out of VsyncWaiter ont he java side at some point, but for now it's convenient to keep it there.

@dnfield dnfield 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 Nov 18, 2021
@dnfield
Copy link
Contributor Author

dnfield commented Nov 18, 2021

/cc @cyanglaz FYI since I hear you might be looking at similar changes on iOS

@cyanglaz
Copy link
Contributor

cyanglaz commented Nov 18, 2021

@dnfield Thanks, I was actually looking into this yesterday :) This is going to help a lot when I implement the iOS part.

@dnfield
Copy link
Contributor Author

dnfield commented Nov 18, 2021

@jason-simmons was checking this on a different device and noticed the display manager doesn't call us back upon registration - I've updated this PR to make sure we set the refresh rate on initialization with a DisplayManager, so that we don't have to wait for it to update us.

@VisibleForTesting
public static void reset() {
instance = null;
listener = null;
Copy link
Member

Choose a reason for hiding this comment

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

Should refreshPeriodNanos also be reset to -1 here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not static, so when the instance is re-created it'll be reset.

@chinmaygarde
Copy link
Member

Per the sheriff, the CQ tag may not be doing the right thing. Landing this manually and pinging sheriff chat about it.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes embedder Related to the embedder API platform-android 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
6 participants