-
Notifications
You must be signed in to change notification settings - Fork 6k
WIP: Windows: Use direct composition if available #24705
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. |
clarkezone
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.
Would be good to confirm this doesn't break UWP target
|
@knopp Gentle ping about @clarkezone s pending comments. |
|
Sorry, been busy on the MacOS side of things, this slipped my mind. This only needs some minor tweaks, I'll update it soon. |
|
@clarkezone Can you re-review this please? @knopp The test failures are due to a commit that has been reverted on ToT. Can you rebase this commit please? |
0704fa6 to
9461004
Compare
|
Rebased. |
|
I will take a look this weekend, likely tomorrow (Sunday). |
|
|
||
| if (!CompositionSupported()) { | ||
| // eglPostSubBufferNV during resizing causes significant artifacts on | ||
| // Windows 7, so use fixed size surface instead |
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.
Since this code is touching the resize logic, I wonder if we should also revert eglPostSubBufferNV in the non-UWP composition case as well pending flutter/flutter#76465 being addressed. There are reports of the visual artifacts in cases where WM_NCCALCSIZE is not in use (see the thread in the bug) which means the mainstream case can be subject to the artifacts. If you don't want to address in this PR I will do in a follow-on PR. If the new DComp path in some way depends on eglPostSubBufferNV then we may have to hold off on landing this PR until the above big is correctly fixed. That is currently backed up behind the UWP work.
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 problem with composition enabled (both direct composition and uwp I believe) and recreating surface during resizing is that every now and then you get brief transparent flashes (even with ClearContext in place). I wasn't able to find any workaround for this. So if you decide to revert eglPostSubBufferNV then it would be better to NOT apply this PR until there's proper solution for manual surface resizing.
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 have a possible fix for the resize present issue which I pinged you about.
|
I have one concern about resize logic expressed in a comment. |
|
For triage folks: In #25319, @clarkezone is landing a temporary workaround to the resize issue he alluded to above. That will temporarily put this patch on hold until an upstream ANGLE change and roll can be made. |
|
I added WIP to this PR, just in case. I'll keep it updated once the Angle part is resolved. |
|
The issue tracking Angle support is here: flutter/flutter#79427 |
|
@knopp any updates on this PR? Are you still planning to land this? (Desktop Triage) |
|
@gspencergoog, not until flutter/flutter#79427 gets resolved. It's probably better to close this for now. |
flutter/flutter#76789
Supersedes #24629
This PR will use direct composition when available. Tested on Windows 7.
Pre-launch Checklist
writing and running engine tests.
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.