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

Revert "Listen for Vsync callback on the UI thread directly" #29923

Merged
fluttergithubbot merged 1 commit into from
Nov 24, 2021
Merged

Revert "Listen for Vsync callback on the UI thread directly" #29923

fluttergithubbot merged 1 commit into from
Nov 24, 2021

Conversation

@GaryQian
Copy link
Contributor

@GaryQian GaryQian commented Nov 24, 2021

Reverts #29889

https://ci.chromium.org/ui/p/flutter/builders/prod/Mac_android%20hybrid_android_views_integration_test/3199/overview

[hybrid_android_views_integration_test] [STDOUT] stdout: [ +348 ms] 00:17 �[32m+3�[0m�[31m -1�[0m: Flutter surface switch Uses FlutterImageView when Android view is on the screen �[1m�[31m[E]�[0m�[0m
[hybrid_android_views_integration_test] [STDOUT] stdout: [        ]   Expected: '|-FlutterView\n'
[hybrid_android_views_integration_test] [STDOUT] stdout: [        ]               '  |-FlutterSurfaceView\n'
[hybrid_android_views_integration_test] [STDOUT] stdout: [        ]               '  |-FlutterImageView\n'
[hybrid_android_views_integration_test] [STDOUT] stdout: [        ]               '  |-ViewGroup\n'
[hybrid_android_views_integration_test] [STDOUT] stdout: [        ]               '    |-ViewGroup\n'
[hybrid_android_views_integration_test] [STDOUT] stdout: [        ]               '  |-FlutterImageView\n'
[hybrid_android_views_integration_test] [STDOUT] stdout: [        ]               ''
[hybrid_android_views_integration_test] [STDOUT] stdout: [        ]     Actual: '|-FlutterView\n'
[hybrid_android_views_integration_test] [STDOUT] stdout: [        ]               '  |-FlutterSurfaceView\n'
[hybrid_android_views_integration_test] [STDOUT] stdout: [        ]               '  |-FlutterImageView\n'
[hybrid_android_views_integration_test] [STDOUT] stdout: [        ]               '  |-FlutterImageView\n'
[hybrid_android_views_integration_test] [STDOUT] stdout: [        ]               ''
[hybrid_android_views_integration_test] [STDOUT] stdout: [        ]      Which: is different.
[hybrid_android_views_integration_test] [STDOUT] stdout: [        ]             Expected: ... View\n  |-ViewGroup\ ...
[hybrid_android_views_integration_test] [STDOUT] stdout: [        ]               Actual: ... View\n  |-FlutterIma ...
[hybrid_android_views_integration_test] [STDOUT] stdout: [        ]                                     ^
[hybrid_android_views_integration_test] [STDOUT] stdout: [        ]              Differ at offset 65
[hybrid_android_views_integration_test] [STDOUT] stdout: [  +10 ms]   package:test_api/src/expect/expect.dart 137:31    fail
[hybrid_android_views_integration_test] [STDOUT] stdout: [        ]   package:test_api/src/expect/expect.dart 132:3     _expect
[hybrid_android_views_integration_test] [STDOUT] stdout: [        ]   package:test_api/src/expect/expect.dart 46:3      expect
[hybrid_android_views_integration_test] [STDOUT] stdout: [        ]   test_driver/main_test.dart 76:7                   main.<fn>.<fn>

@flutter-dashboard
Copy link

flutter-dashboard bot commented Nov 24, 2021

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.

@google-cla google-cla bot added the cla: yes label Nov 24, 2021
mdebbar
mdebbar approved these changes Nov 24, 2021
View reviewed changes
Copy link
Contributor

@mdebbar mdebbar left a comment

Choose a reason for hiding this comment

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

LGTM

@GaryQian GaryQian 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 24, 2021
@fluttergithubbot fluttergithubbot merged commit fda9166 into main Nov 24, 2021
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Nov 24, 2021
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 24, 2021
@engine-flutter-autoroll
fda9166 Revert "Listen for Vsync callback on the UI thread directly" (f…
012f6bb 
…lutter/engine#29923)
@FelixZhang00
Copy link
Contributor

FelixZhang00 commented Nov 26, 2021 •
edited
Loading

Whitout reverting this commit, I run hybrid_android_views_integration_test on my mac laptop, it's success.
I doubt if this is caused by a specific device.
What device did you use that run the test ? @GaryQian

Here is the output log:
hybrid_android_views_integration_test.log

@zanderso zanderso deleted the revert-29889-ui_vsync_directly branch November 29, 2021 03:14
@zanderso
Copy link
Member

zanderso commented Nov 29, 2021

Can @blasten or @GaryQian answer the question above from @FelixZhang00 ?

@GaryQian
Copy link
Contributor Author

GaryQian commented Nov 29, 2021

If it is passing, we can try to land it again, the failure may have been a flake then. I opted for revert since it was the only risky change in the roll and it was blocking the tree.

@mdebbar
Copy link
Contributor

mdebbar commented Nov 29, 2021

Based on the failures in the Flutter Dashboard, this doesn't seem to be flaky. It's consistently passing in all commits without retries, and kept failing consistently on this commit despite retrying multiple times.

image

@blasten
Copy link

blasten commented Nov 29, 2021

@FelixZhang00 It seems like your change exposed a race in the current implementation of platform views.

The race is more noticeable due to the changes to shell/platform/android/vsync_waiter_android.cc in #29889.

The issue is that to show a platform view in the first frame, the platform thread needs to wait for a message from framework. Currently, the engine post a task to the platform thread to generate a frame, which may cause the ordering of events to be "just" right. However, your change posts a frame without going through the queue.

A potential solution to this problem would require to detect if the current frame has a platform view, and wait for the message (that allows to create the Android view) before producing the actual frame on the screen.

cc @chinmaygarde

@blasten blasten mentioned this pull request Nov 29, 2021
Closed
@ColdPaleLight ColdPaleLight mentioned this pull request Feb 20, 2022
Merged
8 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@mdebbar mdebbar mdebbar approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@GaryQian @FelixZhang00 @zanderso @mdebbar @blasten @fluttergithubbot