-
Notifications
You must be signed in to change notification settings - Fork 6k
Fix eglPresentationTimeANDROID is no effective #30182
Conversation
|
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. |
30e04fe to
f3b5eb6
Compare
f3b5eb6 to
de18d90
Compare
|
This may need a rebase to get the windows tests passing. Thanks for catching this! I guess I forgot to rerun a local benchmark before refactoring this and then hadn't checked the benchmarks again after landing it. |
|
@dnfield Thanks for review. I have rebase main and all test passed ~ |
dnfield
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lgtm
| if (strstr(extensions, "EGL_ANDROID_presentation_time")) { | ||
| presentation_time_proc_ = | ||
| reinterpret_cast<PFNEGLPRESENTATIONTIMEANDROIDPROC>( | ||
| eglGetProcAddress("sEGL_ANDROID_presentation_time")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this have been EGL_ANDROID_presentation_time ? What is the relationship between this value and L24? @dnfield
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think what happened here is that I meant to copy and paste eglPresentationTimeANDROID, and instead I copy and pasted EGL_ANDROID_presentation_time .. and hit the s key by accident .. and then didn't notice it.
When I initially did some tests with this extension, I was just directly calling it and hacking it in as if it wouldn't have to be looked up. I then refactored to do it the right way (get the proc address this way), but never re-tested things to make sure it was still doing the good thing.. and then this got missed in review :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@iskakaushik FYI since I think you reviewed the original PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The bottom line is: this lookup is correct and we should accept it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe can see this L92 and L193
https://cs.android.com/android/platform/superproject/+/android-10.0.0_r1:frameworks/native/opengl/libs/EGL/egl_platform_entries.cpp;l=92
In my opinion:
EGL_ANDROID_presentation_time is gBuiltinExtensionString, and It is used to determine whether an extension point exists.
eglPresentationTimeANDROID is key when get address.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could we add a comment about what EGL_ANDROID_presentation_time and eglPresentationTimeANDROID are used for?
It will definitely help next time there's a refactor, and the blame doesn't point to this PR. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I am not native speaker. I'm not sure my description is accurate. Maybe @dnfield could help to add this comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@blasten I added some comments.
* 37223ca [fuchsia] Use network-legacy-deprecated pkg in embedder test (flutter/engine#30244) * 86769ee Roll Dart SDK from 2ace65b1b408 to 2091ff49c513 (8 revisions) (flutter/engine#30251) * 5312e55 Win32: Handle OnAccessibilityEvent (flutter/engine#30176) * 8fd8912 Revert the latest rolls of the Fuchsia SDK (flutter/engine#30254) * df21689 Call IAccessible::accFocus to move a11y focus (flutter/engine#30256) * 6aa8d5c Simplify win32 platform node delegate GetParent (flutter/engine#30258) * ff328f0 Fix TestAccessibilityBridgeDelegate event caching (flutter/engine#30260) * 842281e Test shell/platform/common a11y code on Windows (flutter/engine#30262) * 806d8bc Roll Dart SDK from 2091ff49c513 to 7b6056f9db7f (1 revision) (flutter/engine#30259) * 1ed10bb Android accessibility bridge also fire selection change event when it predict selection change. (flutter/engine#30199) * 71fdb89 Override FlutterPlatformNodeDelegate::GetUniqueId (flutter/engine#30261) * 5aa7b30 Cleanup old ndk CIPD upload script (flutter/engine#30253) * 3653881 Roll Fuchsia Mac SDK from EcjcLVqar... to 1DgBWWOWV... (flutter/engine#30265) * f764864 Roll Skia from 44c81d149273 to a964a72174e8 (9 revisions) (flutter/engine#30255) * 2eca05e Migrate to Mockito 4.1.0 (flutter/engine#30257) * 1a6005f Add iOS version to scenario golden images (flutter/engine#30263) * 59c6f28 Roll Fuchsia Linux SDK from 3rLypXNTd... to UPWdoQziF... (flutter/engine#30266) * 5502a0a Fix eglPresentationTimeANDROID is no effective (flutter/engine#30182) * bb20dd0 Roll Skia from a964a72174e8 to 4d35c0d31d79 (16 revisions) (flutter/engine#30269) * 33f4c32 Revert "Accessibility number formatting improvements for Windows (#29773)" (flutter/engine#30267) * 5ece42f Roll Dart SDK from 7b6056f9db7f to f568598a262e (4 revisions) (flutter/engine#30270) * 8d1272b Roll Dart SDK from f568598a262e to 49d5a2f58c4a (3 revisions) (flutter/engine#30277) * 8b074c8 Roll Fuchsia Mac SDK from 1DgBWWOWV... to GoQ7KEnqp... (flutter/engine#30278) * e1a71cf Roll Fuchsia Linux SDK from UPWdoQziF... to hhFGKobVD... (flutter/engine#30279) * 6dc4046 Roll Fuchsia Mac SDK from GoQ7KEnqp... to Y_9lCzY64... (flutter/engine#30280) * ff7d4d6 Roll Fuchsia Linux SDK from hhFGKobVD... to NAkkk-Vn5... (flutter/engine#30281) * 2f97ac4 Roll Dart SDK from 49d5a2f58c4a to 01a9d4e9ede7 (1 revision) (flutter/engine#30283) * 85f4d68 Roll Fuchsia Linux SDK from NAkkk-Vn5... to V2JLZw4H1... (flutter/engine#30285) * 5130b83 Roll Fuchsia Mac SDK from Y_9lCzY64... to xBCcWw6gj... (flutter/engine#30284) * e28882a Roll Dart SDK from 01a9d4e9ede7 to a78eae43582d (1 revision) (flutter/engine#30288) * 368e3cc Revert Dart SDK to 2ace65b1b408 (flutter/engine#30290)
Fix flutter/flutter#94786
After Fix
Kindly See Log
eglPresentationTimeANDROID-fixed.log
[download it and delete .log . And use https://ui.perfetto.dev/ to open it]
Can see trace also. Between the Red Flag to Blue Flag is one frame. In this frame raster commit 2 buffer to bufferQueue. These 2 buffer is used/discard in the next Vsync-sf (Blue Flag).
Pre-launch Checklist
writing and running engine tests.
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.