[macOS] Isolate openGL rendering to FlutterOpenGLRenderer #22569
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. Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
iskakaushik
force-pushed
the
opengl_rendering_isolation
branch
from
ac3988c to
41b490f
Compare
chinmaygarde
left a comment
chinmaygarde
left a comment
There was a problem hiding this comment.
I may have reviewed some code that you only moved. But LGTM otherwise.
| const FlutterRendererConfig rendererConfig = { | ||
| .type = kOpenGL, | ||
| .open_gl.struct_size = sizeof(FlutterOpenGLRendererConfig), | ||
| .open_gl.make_current = (BoolCallback)OnMakeCurrent, |
There was a problem hiding this comment.
Nit: Here and elsewhere, C-style casts in ObjC++ code.
| } | ||
|
|
||
| - (void)textureFrameAvailable:(int64_t)textureID { | ||
| _embedderAPI.MarkExternalTextureFrameAvailable(_engine, textureID); |
There was a problem hiding this comment.
Nit: Return codes are ignored. Maybe log it at the very least.
| FlutterExternalTextureGL* FlutterTexture = | ||
| [[FlutterExternalTextureGL alloc] initWithFlutterTexture:texture]; | ||
| int64_t textureID = [FlutterTexture textureID]; | ||
| _embedderAPI.RegisterExternalTexture(_engine, textureID); |
There was a problem hiding this comment.
Add the the registered textures only if you can successfully register the texture with the engine.
| NSOpenGLPFAColorSize, 24, NSOpenGLPFAAlphaSize, 8, 0, | ||
| }; | ||
| NSOpenGLPixelFormat* pixelFormat = [[NSOpenGLPixelFormat alloc] initWithAttributes:attributes]; | ||
| _resourceContext = [[NSOpenGLContext alloc] initWithFormat:pixelFormat shareContext:nil]; |
There was a problem hiding this comment.
Just for my clarification: How is the shareContext nil here? Are we using creating this context first and then using it as the prototype for the main context?
There was a problem hiding this comment.
| self = [super init]; | ||
| if (self) { | ||
| _engine = engine; | ||
| _textures = [[NSMutableDictionary alloc] init]; |
There was a problem hiding this comment.
Can you confirm that these TUs use ARC?
There was a problem hiding this comment.
This is in preparation for having an equivalent FlutterMetalRenderer
iskakaushik
force-pushed
the
opengl_rendering_isolation
branch
from
41b490f to
36b8aa2
Compare
iskakaushik
added
the
waiting for tree to go green
| .open_gl.make_resource_current = (BoolCallback)OnMakeResourceCurrent, | ||
| .open_gl.gl_external_texture_frame_callback = (TextureFrameCallback)OnAcquireExternalTexture, | ||
| }; | ||
| [_openGLRenderer attachToFlutterView:_viewController.flutterView]; |
There was a problem hiding this comment.
There is no guarantee that there will be a view here. How are you handling later changes to the view controller given that you removed setViewController:?
There was a problem hiding this comment.
Good point, will account for it.
| /** | ||
| * Called by the engine when framebuffer object ID is requested. | ||
| */ | ||
| - (uint32_t)getFBO:(const FlutterFrameInfo*)info; |
There was a problem hiding this comment.
"get" is not idiomatic ObjC naming. This should be called identifierForFBO: or similar.
| * Coordinates a single instance of execution of a Flutter engine. | ||
| */ | ||
| FLUTTER_EXPORT | ||
| @interface FlutterEngine : NSObject <FlutterTextureRegistry, FlutterPluginRegistry> |
There was a problem hiding this comment.
Did you follow the breaking change process for this change? You've changed the public API of a public class.
…utter#22569)" This reverts commit 94e217b.
4 participants
This is in preparation for having an equivalent FlutterMetalRenderer