Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

@huanghongxun
Copy link
Contributor

@huanghongxun huanghongxun commented Jan 28, 2021

This pull request is part of #23985.

This pull request refactors FlRenderer to a platform-independent implementation.

  1. Switch to embedder compositor.
  2. Introduced FlGLArea to represent a Flutter content layer from compositor.

Code changes:

  1. Removes egl_utils.h/cc since I'm using libepoxy instead, which is also used by GTK. mock_egl.cc is refactored into mock_epoxy.cc for mocking libepoxy.
  2. Drop out wayland-client and x11 requirement. Now we use gtk_cairo to draw OpenGL textures.
  3. FlBackingStoreProvider for framebuffer and texture creation. Just like FlutterFrameBufferProvider in macOS implementation.
  4. FlEngine now enables compositor capability of embedder.
  5. FlGLArea for framebuffer and texture drawing. every FlGLArea has a GdkWindow, because we may embed some widgets provided by plugins, which have own GdkWindows.
  6. FlRenderer is now responsible for compositing. Creates backing store by FlBackingStoreProvider, and present layers by putting FlGLAreas with given textures into FlView.
  7. FlRendererGL for OpenGL renderer, creates GdkGLContext as OpenGL context.
  8. FlView now is GtkContainer to support multiple layers of FlGLArea, maintaining a cached gl_area_list, and children_list. Every frame FlRenderer will reset children of FlView, but if every time we unparent and parent these child widgets, GtkInspector will crash. So calculates diff of children_list is necessary.
  9. Pointer events now is captured by a GtkEventBox on top of FlView, instead of directly capturing events in FlView, because FlGLAreas have their own GdkWindows, which can capture pointer events, we must cover a GdkWindow for event capturing on the top.

This pr is part of platform view implementation on Linux, related issue: flutter/flutter#41724.

Pre-launch Checklist

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • I read the [Tree Hygiene] wiki page, which explains my responsibilities.
  • I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides].
  • I listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test exempt. See [testing the engine] for instructions on
    writing and running engine tests.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the [CLA].
  • All existing and new tests are passing.
  • The reviewer has submitted any presubmit flakes in this PR using the [engine presubmit flakes form] before re-triggering the failure.

If you need help, consider asking for advice on the #hackers-new channel on [Discord].

@huanghongxun
Copy link
Contributor Author

@robert-ancell This is the first part of pr to implement platform views on Linux. I have tried to minimize modifications.. But implementing compositor requires all of these changes.

@huanghongxun huanghongxun force-pushed the dev/glarea branch 4 times, most recently from b66a9de to 4e0c564 Compare January 28, 2021 14:29
@huanghongxun huanghongxun force-pushed the dev/glarea branch 3 times, most recently from 914d01f to e8e01b9 Compare January 29, 2021 12:36
@huanghongxun
Copy link
Contributor Author

huanghongxun commented Jan 29, 2021

Linux Web Engine
recipe infra failure: Recipe timed out

All tests are passed now.

*
* Returns %TRUE if successful.
*/
gboolean (*collect_backing_store)(FlRenderer* renderer,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is "collect" used elsewhere in Flutter to mean release? If not I think the meaning of this name is unintuitive.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function implements

FlutterBackingStoreCollectCallback collect_backing_store_callback;

Copy link
Contributor

@wmww wmww Jan 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, in that case sounds good

glGenTextures(1, &provider->texture_id);
glGenFramebuffers(1, &provider->framebuffer_id);

glBindFramebuffer(GL_FRAMEBUFFER, provider->framebuffer_id);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I notice you unbind the texture at the end of the function, but don't unbind the framebuffer. It doesn't seem to be documented that this leaves the framebuffer bound. Is this intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Binding framebuffer 0 does not mean 'unbinding' framebuffer, but binding to the system default framebuffer. Flutter/Skia will always bind a specific framebuffer (provided by FlBackingStoreProvider) before rendering. So we have no need to bind to framebuffer 0 here. Restoring framebuffer binding requires a call to glGetIntegerv(GL_FRAMEBUFFER_BINDING, &old_fbo);.

macOS implementation has the same behavior, and because framebuffer 0 is not the default framebuffer.

glBindFramebuffer(GL_FRAMEBUFFER, _frameBufferId);
glBindTexture(GL_TEXTURE_RECTANGLE_ARB, _backingTexture);
glTexParameterf(GL_TEXTURE_RECTANGLE_ARB, GL_TEXTURE_MIN_FILTER, GL_NEAREST);
glTexParameterf(GL_TEXTURE_RECTANGLE_ARB, GL_TEXTURE_MAG_FILTER, GL_NEAREST);
glTexParameteri(GL_TEXTURE_RECTANGLE_ARB, GL_TEXTURE_WRAP_S, GL_CLAMP_TO_EDGE);
glTexParameteri(GL_TEXTURE_RECTANGLE_ARB, GL_TEXTURE_WRAP_T, GL_CLAMP_TO_EDGE);
glBindTexture(GL_TEXTURE_RECTANGLE_ARB, 0);


uint32_t framebuffer_id;
uint32_t texture_id;
GdkRectangle geometry;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does geometry get set anywhere? I see it's used, but I don't see it being set.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now geometry is used in FlGLArea for resolving drawing texture repeatedly.

GdkWindow* window = gtk_widget_get_parent_window(widget);

*visible = gdk_window_create_gl_context(window, error);
*resource = gdk_window_create_gl_context(window, error);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks sus to use error twice without checking it. probably best to check error after each call.

In practice I don't think this is unsafe or a memory leak, but if the 1st call results in an error the 2nd call will fail a non-fatal assertion inside GTK (which prints an error message to console). Might leak an error if assertions are turned off.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added error check right after line 24.

@wmww
Copy link
Contributor

wmww commented Jan 29, 2021

Tested and Wayland works on Sway and Mir. One oddity is that the GL view seems to be in tiled mode, so when you're resizing the app to be bigger (which is still al little janky) instead of being black, white or whatever was there before, the top and/or right edge is a copy of the other edge side of the window.

@huanghongxun
Copy link
Contributor Author

Tested and Wayland works on Sway and Mir. One oddity is that the GL view seems to be in tiled mode, so when you're resizing the app to be bigger (which is still al little janky) instead of being black, white or whatever was there before, the top and/or right edge is a copy of the other edge side of the window.

Now resizing the app to be bigger, area not covered will be white.

Copy link
Contributor

@wmww wmww left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@robert-ancell robert-ancell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @huanghongxun!

@huanghongxun
Copy link
Contributor Author

Maybe we need your approval? @stuartmorgan @cbracken

@stuartmorgan-g
Copy link
Contributor

@robert-ancell is a committer, so his review is sufficient unless there's a reason someone else needs to review it.

@robert-ancell robert-ancell merged commit f94cf14 into flutter:master Feb 2, 2021
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 3, 2021
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 3, 2021
hjfreyer pushed a commit to hjfreyer/engine that referenced this pull request Mar 22, 2021
…4011)

Refactor FlRenderer to platform-independent implementation
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants