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

Conversation

@jason-simmons
Copy link
Member

The Android Choreographer.doFrame callback provides a timestamp in
the clock used by System.nanoTime. The VsyncWaiter needs to translate
this to a frame start time based on the fml::TimePoint clock
(similar to how VsyncWaiterIOS calculates the frame start time)

@flutter-dashboard
Copy link

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@ColdPaleLight
Copy link
Member

ColdPaleLight commented Oct 9, 2021

Hi, @jason-simmons

Thank you for taking the time out of your busy schedule to solve this problem.

I'm not sure if this change is needed for Android. The following are some of my personal views:

  1. The reason why iOS needs to do this is that CADisplayLink.link and CADisplayLink.duration are both in seconds. So it has to do the conversion. But Android does not have this problem.

  2. System.nanoTime and DartTimelineTicksSinceEpoch are both based on clock_gettime

Thanks

@jason-simmons
Copy link
Member Author

Yes - this is not strictly necessary given the similarity between the clocks used by Choreographer.doFrame and fml::TimePoint.

However, I do think this will help ensure correctness because that similarlity is an implementation detail. If either clock implementation ever changes, then it will be valuable to do an explicit conversion of the timestamp between the two clocks.

@ColdPaleLight
Copy link
Member

However, I do think this will help ensure correctness because that similarlity is an implementation detail. If either clock implementation ever changes, then it will be valuable to do an explicit conversion of the timestamp between the two clocks.

That sounds good.

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

…::TimePoint

The Android Choreographer.doFrame callback provides a timestamp in
the clock used by System.nanoTime.  The VsyncWaiter needs to translate
this to a frame start time based on the fml::TimePoint clock
(similar to how VsyncWaiterIOS calculates the frame start time)
@jason-simmons jason-simmons 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 Oct 11, 2021
@fluttergithubbot fluttergithubbot merged commit e365502 into flutter:master Oct 11, 2021
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cla: yes needs tests platform-android 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.

4 participants