Started asserting that metal gpu contexts are shared #23539
Conversation
| @@ -43,6 +43,16 @@ | |||
| GrBackend::kOpenGL_GrBackend, GPUSurfaceGLDelegate::GetDefaultPlatformGLInterface()); | |||
| } | |||
|
|
|||
| // |IOSContext| | |||
| sk_sp<GrDirectContext> IOSContextGL::GetMainContext() const { | |||
| /// TODO(b/tbd): Currently the GPUSurfaceGL creates the main context for | |||
There was a problem hiding this comment.
I'll write up the issue once this PR has been approved. This means the savings won't happen on OpenGL until that PR is resolved. It was risky enough and lower priority since less people are on OpenGL so I punted for now.
|
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. Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
| @@ -106,6 +106,14 @@ class IOSContext { | |||
| int64_t texture_id, | |||
| fml::scoped_nsobject<NSObject<FlutterTexture>> texture) = 0; | |||
|
|
|||
| //---------------------------------------------------------------------------- | |||
| /// @brief Accessor for the Skia context associated with IOSSurfaces. | |||
| /// | |||
There was a problem hiding this comment.
add a bit more description and tie the wording back to existing wording in the class doc like Manages the lifetime of the on-screen and off-screen rendering. Where does "main" fit into the existing picture. Also describe why this is exposed and other contexts are not a bit. Also when is this available. Is there sequencing expected with the other methods in this class?
There was a problem hiding this comment.
I updated this docstring. I think it addresses your points if you want to give a look.
I didn't name this method, I just promoted it to its superclass to make opengl and metal behave similarly and make my work easier (the opengl refactor will be done at a later date). There is 1 skia context for the raster thread for rendering to the screen. There can be any number of resource contexts. In practice only one is ever created for use with the io thread.
| @@ -24,7 +24,7 @@ class IOSContextMetal final : public IOSContext { | |||
|
|
|||
| fml::scoped_nsobject<FlutterDarwinContextMetal> GetDarwinContext() const; | |||
|
|
|||
| sk_sp<GrDirectContext> GetMainContext() const; | |||
There was a problem hiding this comment.
So the idea is that there's 2, a main rendering context on the raster thread and a resource secondary context on the io thread? If so, be fully descriptive in the doc so someone new can understand.
There was a problem hiding this comment.
updated docstring in ios_context, added link to it.
for IOSContexts to store their skia contexts so we can make the OpenGL code work like the Metal code.
gaaclarke
force-pushed
the
lfe-share-gpu-context-2
branch
from
42ff328 to
9857163
Compare
|
Landing on read, the last CI task has a known issue with timeouts (flutter/flutter#72391) This task has completed successfully previously without any code changes afterwards, just to documentation. |
Started asserting that metal gpu contexts are shared, added ability for IOSContexts to store their skia contexts so we can make the OpenGL code work like the Metal code.
Description
Also, added the ability for IOSContexts to store their skia contexts so we can make the OpenGL code work like the Metal code.
Related Issues
flutter/flutter#72021
Tests
I added the following tests:
FlutterEngineTest_mrc test asserts changed.
Checklist
Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes (
[x]). This will ensure a smooth and quick review process.Reviewer Checklist
Breaking Change
Did any tests fail when you ran them? Please read handling breaking changes.