New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[impeller] Wire up the OpenGL ES Backend. #33405
Conversation
std::optional<bool> DetermineIfES(const std::string& version) { | ||
if (HasPrefix(version, "OpenGL ES")) { | ||
return true; | ||
} else if (HasPrefix(version, "OpenGL")) { |
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.
This case looks redundant with the else {}
case.
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.
So, I am not actually sure if the OpenGL prefix is required. I seemed to think it was. But the Mac desktop implementation does not seem to have it. The else below this was supposed to be std::nullopt
. Notice how the bool is optional but a value is always returned. I am going to assume the Mac desktop behavior is correct and return a non-optional for now.
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.
Done.
} | ||
} | ||
|
||
static std::optional<Version> DetermineVersion(std::string version) { |
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.
Can this have some unit tests? String parsing without unit tests makes me very nervous.
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.
(seconded)
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.
Adding this in a followup patch along with other fixes.
d8ab012
to
4c77bb9
Compare
"gpu_surface_gl_skia.cc", | ||
"gpu_surface_gl_skia.h", |
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.
not a problem but will require compensating changes for the roll.
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 didn't give this whole patch a close look, but from what I've spot checked it looks good enough to land for experimentation.
https://flutter-review.googlesource.com/c/recipes/+/30204 will be needed to clear up the "Linux clang-tidy" presubmit. |
887f33c
to
5a3ddf5
Compare
@jonahwilliams for advice on 'fractional' build failure. Maybe just needs a rebase? |
You might just need 9867003 |
5a3ddf5
to
42233f9
Compare
This reverts commit 01a129b.
For flutter/engine#33405 led run: https://ci.chromium.org/raw/build/logs.chromium.org/flutter/led/zra_google.com/42f90b28a0d3e5058eaf5d82962f3a876a0631d843452ba011dbe1c0cb9d4ce6/+/build.proto?server=chromium-swarm.appspot.com Change-Id: Ied1019884e4687d1c15448fb700f3d5d1a42d58c Reviewed-on: https://flutter-review.googlesource.com/c/recipes/+/30204 Reviewed-by: Dan Field <dnfield@google.com> Reviewed-by: Godofredo Contreras <godofredoc@google.com> Commit-Queue: Dan Field <dnfield@google.com>
This is still work in progress but basic rendering is functional. There are issues around context access from multiple threads that I am going to address in a subsequent patch. This also brings in the binary cost overhead of using Impeller on Android.