Rework GLContextSwitch, get rid of RendererContextManager #18601
Conversation
cyanglaz
force-pushed
the
context_switch_only
branch
from
fd5ac21 to
7ec9c87
Compare
cyanglaz
force-pushed
the
context_switch_only
branch
from
7ec9c87 to
202afe7
Compare
shell/common/surface.h
Outdated
| SurfaceFrame(sk_sp<SkSurface> surface, | ||
| bool supports_readback, | ||
| const SubmitCallback& submit_callback, | ||
| std::unique_ptr<GLContextResult> context_result); |
There was a problem hiding this comment.
Added a GLContextResult in the constructor, so iOS platform can pass in a GLContectSwitch whose lifecycle is tied to the SurfaceFrame
|
|
||
| #if FLUTTER_SHELL_ENABLE_METAL | ||
| bool ShouldUseMetalRenderer() { | ||
| return false; |
There was a problem hiding this comment.
REMOVE BEFORE LANDING!
|
|
||
| class IOSSwitchableGLContext final : public SwitchableGLContext { | ||
| public: | ||
| IOSSwitchableGLContext(EAGLContext& context); |
There was a problem hiding this comment.
You should avoid passing non-const references, this will soon be a linter error:
https://google.github.io/styleguide/cppguide.html#Inputs_and_Outputs
It's especially egregious for NSObjects. This class should retain and release the eaglcontext, too.
There was a problem hiding this comment.
This should actually be const. I'll update. This class doesn't retain the EAGLContext, all the EAGLContexts are retained by IOSRenderTarget and IOSContextGL. This class should just use the context and never out live either of the 2.
| FML_DISALLOW_COPY_AND_ASSIGN(SwitchableGLContext); | ||
| }; | ||
|
|
||
| class GLContextResult { |
There was a problem hiding this comment.
Why have this class instead of using a bool?
There was a problem hiding this comment.
We need to use GLContextSwitch on iOS to prevent plugins to pollute our context. For methods like MakeFlutterContextCurrent, we need to return a switch instead of a bool. But for platforms that doesn't require the switch (iOS software for example), we still need to return a same object. Making them returning a switch doesn't make sense as they are not actually switching context. So I thought to make this base class so at least the platforms that doesn't require a switch can return a same class.
| /// In destruction, it should reset the current context back to what was | ||
| /// before the construction of this switch. | ||
| /// | ||
| class GLContextSwitch final : public GLContextResult { |
There was a problem hiding this comment.
This is a bit confusing a GLContextSwitch is a GLContextResult.
There was a problem hiding this comment.
Yeah, I know :p Need some help with refactoring and naming here :)
shell/common/rasterizer.cc
Outdated
| // happen in case of software rendering. | ||
| surface = SkSurface::MakeRaster(image_info); | ||
| } else { | ||
| if (!surface_->MakeRenderContextCurrent()) { |
There was a problem hiding this comment.
This particular change isn't as clear, surface_->MakeRenderContextCurrent is a much more clear verbiage than context_switch->GetResult
There was a problem hiding this comment.
We need to grab onto a switch object so it can live through out the scope.
The switch object clears the GLContext on destruction.
gaaclarke
left a comment
gaaclarke
left a comment
There was a problem hiding this comment.
Hey chris, I don't quite get the value of this change. At face value it just looks like an obfuscation. What am I missing?
| } | ||
|
|
||
| source_set("surface_frame") { | ||
| sources = [ |
There was a problem hiding this comment.
This target now has a different meaning. Something like standalone_common ? The purpose of this target is to avoid the dependency on //flutter/flow because //flutter/flow depends on this target.
There was a problem hiding this comment.
offline discussion, we will use common_standalone
shell/gpu/BUILD.gn
Outdated
| "//flutter/fml", | ||
| "//flutter/shell/common", | ||
| "//third_party/skia", | ||
| "//flutter/shell/common:surface_frame" |
There was a problem hiding this comment.
gn format shell/gpu/BUILD.gn, so it automatically sorts the deps.
5 participants
In my last patch #17941. I introduced a RendererContextManager which manages a stack that pushes and pops GLContext.
This turns out to be problematic, as the stack has to be thread local, because the GLContexts are thread locals. And then if we make the stack thread local, we need to handle the case where 2 flutter shells exists and makes sure they don't interfere the stack.
It gets really complicated.
I think a better an easier way to solve this is to get rid of the manager and stack all together.
Instead, the GLContextSwitch will makes sure to call the SwitchableGLContext's setCurrent and removeCurrent during construction and destruction.
Then the SwitchableGLContext is responsible to reset the GLContext in removeCurrent.
A GLContextDefaultResult is introduced for platforms that are not using context switching yet.(Android, embedder)
Ideally, we should migrate all platforms to use context switching in the future.