Linux texture support #20714

anirudhb · 2020-08-22T22:15:15Z

Description

Add support for external textures to the Linux platform shell.

Related Issues

Fixes flutter/flutter#64188.

Tests

I added the following tests:

Tests for FlTextureRegistrar
Tests for FlExternalTextureGl

Checklist

I read the contributor guide and followed the process outlined there for submitting PRs.
I signed the CLA.
I read and followed the C++, Objective-C, Java style guides for the engine.
I read the tree hygiene wiki page, which explains my responsibilities.
I updated/added relevant documentation.
All existing and new tests are passing.
I am willing to follow-up on review comments in a timely manner.

Breaking Change

Did any tests fail when you ran them? Please read handling breaking changes.

No, no existing tests failed, so this is not a breaking change.

flutter-dashboard · 2020-08-22T22:15:17Z

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.

pingbird · 2020-08-22T23:08:56Z

You're failing lint checks! https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket.appspot.com/8871227673395193760/+/steps/lint_test/0/stdout

anirudhb · 2020-08-23T00:09:13Z

Fixed!

stuartmorgan · 2020-08-25T18:41:05Z

Filed flutter/flutter#64576 for the lint issue; we'll figure out the plan to resolve the unrelated issues there. I'll review the rest for functionality now.

stuartmorgan

At a high level, the structure matches the other desktop platforms, so that part looks good. I'll defer to @robert-ancell on most of the detailed review since I'm still not fluent enough in reading GObject code to feel like I would catch bugs there.

I left some some nits and comments though.

As for tests, at the very least the new classes should all have tests; while the current situation is uneven, the goal is to improve the level of coverage, not regress it. There are examples of tests already; are there specific questions you have about how to test the new classes?

shell/platform/linux/fl_engine.cc

shell/platform/linux/fl_external_texture_gl.cc

shell/platform/linux/fl_external_texture_gl.h

shell/platform/linux/fl_texture_registrar.cc

anirudhb · 2020-08-27T00:45:23Z

@stuartmorgan Resolved nits and added partial tests. I've rolled back the changes that don't touch my code (linting fixes), as you've filed an issue for that. Re-review?

Also addresses nits from code review. Testing will be complete once I figure out how to stub `eglGetProcAddress` in the tests.

stuartmorgan · 2020-08-31T19:09:06Z

Filed flutter/flutter#64576 for the lint issue; we'll figure out the plan to resolve the unrelated issues there.

Based on flutter/flutter#64576, the solution for now should be to add // FLUTTER_NOLINT just after the license header. It's unclear how the file ended up not having it already, but you can just add it to this PR.

stuartmorgan · 2020-08-31T19:11:37Z

In the future, please don't force-push to in-review PRs. It removes the ability to use very helpful review features like seeing everything that changed since the last review.

shell/platform/linux/BUILD.gn

shell/platform/linux/testing/fl_test.cc

shell/platform/linux/testing/fl_test.h

shell/platform/linux/fl_external_texture_gl.cc

shell/platform/linux/fl_texture_registrar_test.cc

robert-ancell

Thanks for working on this!

shell/platform/linux/fl_engine_private.h

shell/platform/linux/fl_external_texture_gl.cc

shell/platform/linux/public/flutter_linux/fl_texture_registrar.h

anirudhb · 2020-09-05T23:46:27Z

I've addressed nits. Finished tests as well by changing the mock implementation in mock_egl.cc.

I ended up taking the approach that @robert-ancell suggested and removed the FlPixelBuffer type in favor of inout parameters to the callback directly.

stuartmorgan · 2020-09-08T20:58:56Z

The conversation starting here is relevant to this PR as well; we should address them the same way, ideally.

anirudhb · 2020-09-08T21:39:56Z

I've read that conversation, and my personal thinking is that this initial functionality is worth merging, then we can handle more efficient texture handoff later. Like Windows, Linux doesn't have any graphics API-independent way to hand off textures so I think this is the best way for now, since it abstracts that functionality from the user.

stuartmorgan · 2020-09-08T22:19:23Z

this initial functionality is worth merging, then we can handle more efficient texture handoff later

As I explained there, it's important that enough thought is given to things like API names in this version that the more efficient version(s) can be added later without it being a breaking change (or being stuck with confusing APIs where there are 2+ variants, but one of them has a confusingly generic name).

anirudhb

@stuartmorgan I've left some notes on what I think could be a way to make this API more agnostic of the underlying texture backend. Please comment any thoughts or nits you may have on this design.

anirudhb · 2020-09-12T17:51:34Z

shell/platform/linux/fl_external_texture_gl.h

+gboolean fl_external_texture_gl_copy_pixel_buffer(
+    FlExternalTextureGl* external_texture,
+    size_t* width,
+    size_t* height);


I think renaming this to fl_external_texture_gl_prepare would be more fitting if we wanted to make it agnostic. This should also be a part of the FlExternalTexture interface.

anirudhb · 2020-09-12T17:52:19Z

shell/platform/linux/fl_external_texture_gl.h

+/**
+ * FlExternalTextureGl:
+ *
+ * #FlExternalTextureGl is an abstraction over OpenGL textures.
+ */


On making this agnostic-- It could be better if this inherited from a base type FlExternalTexture, and the texture registrar would handle those instead of FlExternalTextureGl.

anirudhb · 2020-09-12T17:54:26Z

shell/platform/linux/fl_external_texture_gl.h

+int64_t fl_external_texture_gl_texture_id(
+    FlExternalTextureGl* external_texture);


This should just be a part of the interface as well. Since each subclass of FlExternalTexture should represent one graphics API's textures, ids shouldn't be overlapping.

anirudhb · 2020-09-12T17:55:38Z

shell/platform/linux/fl_external_texture_gl.h

+gboolean fl_external_texture_gl_populate_texture(
+    FlExternalTextureGl* external_texture,
+    size_t width,
+    size_t height,
+    FlutterOpenGLTexture* opengl_texture);


This can also be part of the interface, but maybe swapping FlutterOpenGLTexture* for a void* since each subclass will want a different texture type. Keeping this out of the interface doesn't make sense since every subclass will have to do this at some point. This could also be renamed to fl_external_texture_gl_prepare_for_embedder, but that's a bit more verbose.

anirudhb · 2020-09-12T18:03:18Z

shell/platform/linux/fl_external_texture_gl.h

+FlExternalTextureGl* fl_external_texture_gl_new(
+    FlTextureCallback texture_callback,
+    void* user_data);


This should definitely be part of the interface but not sure how to make it generic based on FlTextureCallback returning different values. One way would be to change the output buffer of FlTextureCallback to a void*, then the subclass defines what is supposed to be output.

anirudhb · 2020-09-12T18:04:09Z

shell/platform/linux/fl_texture_registrar_private.h

+bool fl_texture_registrar_populate_texture(
+    FlTextureRegistrar* registrar,
+    int64_t texture_id,
+    size_t width,
+    size_t height,
+    FlutterOpenGLTexture* opengl_texture);


Again, the FlutterOpenGLTexture should be changed to a void*.

anirudhb · 2020-09-12T18:04:53Z

shell/platform/linux/public/flutter_linux/fl_texture_registrar.h

+ */
+typedef gboolean (*FlTextureCallback)(size_t* width,
+                                      size_t* height,
+                                      const uint8_t** buffer,


This might be better redefined as void* out, so it is agnostic of the underlying output and can therefore be safely passed to all FlExternalTexture subclasses.

anirudhb · 2020-09-12T18:05:25Z

shell/platform/linux/public/flutter_linux/fl_texture_registrar.h

+/**
+ * FlTextureRegistrar:
+ *
+ * #FlTextureRegistrar is used when registering textures.
+ */


Internally, the FlTextureRegistrar hashmap should be managing FlExternalTextures not their subclasses.

anirudhb · 2020-09-12T18:09:19Z

shell/platform/linux/public/flutter_linux/fl_texture_registrar.h

+int64_t fl_texture_registrar_register_texture(
+    FlTextureRegistrar* registrar,
+    FlTextureCallback texture_callback,
+    void* user_data);


On making this agnostic-- it might make sense to make an enum here to differentiate which type of texture is being registered (i.e. OpenGL, Vulkan or Pixel Buffer which should be agnostic). Then the texture registrar would instantiate the respective underlying FlExternalTexture subclass. Managing Pixel Buffer callbacks could be done by the texture registrar itself, maybe by simply wrapping it with another callback.

robert-ancell · 2020-09-23T02:36:22Z

shell/platform/linux/testing/fl_test.cc

@@ -39,3 +48,14 @@ gchar* bytes_to_hex_string(GBytes* bytes) {
    g_string_append_printf(hex_string, "%02x", data[i]);
  return g_string_free(hex_string, FALSE);
 }
+
+FlEngine* make_mock_engine() {


Can you split this change out into a separate PR?

There's a number of cases where this could be common.

Done, #21585.

shell/platform/linux/fl_engine.cc

stuartmorgan · 2020-09-28T22:47:16Z

@stuartmorgan I've left some notes on what I think could be a way to make this API more agnostic of the underlying texture backend. Please comment any thoughts or nits you may have on this design.

Some of #19405 (review) (the C API layer part) is relevant here.

You've suggested void* in several places; that's a solution of absolute last resort because it means we're no longer providing any type safety for that portion of the API. I don't see any fundamental reason that a discriminated-union-style approach, just as is used for the embedder.h texture API, couldn't be used here to allow for a set of possible textures (this would stand in for the std::variant at the C++ wrapper layer in my comments on the Windows version).

Or maybe there's something that's a higher-level std::variant-like tool in GObject that would be useful instead of a discriminated union? @robert-ancell ?

chinmaygarde · 2020-10-22T22:13:45Z

From PR triage, is there any progress being made on this PR? If not, can we close this as it looks stale (last update 21 days ago).

anirudhb · 2020-10-23T17:03:53Z

I do plan to make progress on this PR, but I'm waiting on #21585 being merged first to avoid conflicts. Additionally, this and #19405 both seem to be stalled on working out a proper API design, so it would be nice to keep it consistent across both platforms.

stuartmorgan · 2020-10-24T18:38:50Z

Additionally, this and #19405 both seem to be stalled on working out a proper API design

I outlined a path forward in 19405 for the API.

anirudhb · 2020-11-12T16:47:23Z

Looks like this PR will need quite a bit of refactoring to pass review again. I don't think I'll have time for the next month or so, so I've left this as a draft for now. I am definitely still interested in adding support, and will undraft the PR when I feel it's ready :)

chinmaygarde · 2021-02-04T22:56:32Z

Looks like progress on this has stalled. Closing it for now. Please re-open when needed.

dnsoumik · 2021-03-09T13:02:29Z

This needs to re-Open lots of things depend on this one,
Can anyone tell me when this request will be merged?

stuartmorgan · 2021-03-09T15:07:21Z

Please read the recent comments; it's not ready for final review yet, so can't be merged as-is.

huanghongxun · 2021-03-10T13:25:03Z

I have redesigned API and resubmit this PR #24916.
Please have a look.

googlebot added the cla: yes label Aug 22, 2020

auto-assign bot requested a review from flar August 22, 2020 22:18

anirudhb force-pushed the linux_textures branch from 37ba1bd to 3db32a6 Compare August 22, 2020 22:23

anirudhb force-pushed the linux_textures branch from d7ff522 to 63e980a Compare August 23, 2020 00:24

stuartmorgan requested review from robert-ancell, stuartmorgan and chinmaygarde August 23, 2020 04:13

stuartmorgan mentioned this pull request Aug 25, 2020

Engine linting fails on existing files flutter/flutter#64576

Closed

stuartmorgan requested changes Aug 25, 2020

View reviewed changes

anirudhb force-pushed the linux_textures branch from 63e980a to 333457a Compare August 27, 2020 00:41

anirudhb force-pushed the linux_textures branch from 333457a to 9b271a6 Compare August 27, 2020 01:12

anirudhb added 3 commits August 31, 2020 09:37

Linux texture support

05b183d

Fix clang-tidy errors

1b77228

Tests (partial)

013cd2a

Also addresses nits from code review. Testing will be complete once I figure out how to stub `eglGetProcAddress` in the tests.

anirudhb force-pushed the linux_textures branch from 9b271a6 to 013cd2a Compare August 31, 2020 16:39

stuartmorgan self-requested a review August 31, 2020 19:10

stuartmorgan requested changes Aug 31, 2020

View reviewed changes

robert-ancell requested changes Aug 31, 2020

View reviewed changes

Fix nits, finish tests

612995a

anirudhb requested review from robert-ancell and stuartmorgan September 5, 2020 23:47

Change texture_id back to before

8a007ba

flar removed their request for review September 7, 2020 01:03

anirudhb added 2 commits September 8, 2020 13:33

Update golden licenses with new files

051333b

Oops

e2e365f

anirudhb commented Sep 12, 2020

View reviewed changes

robert-ancell reviewed Sep 23, 2020

View reviewed changes

shell/platform/linux/fl_engine.cc Show resolved Hide resolved

robert-ancell reviewed Sep 23, 2020

View reviewed changes

shell/platform/linux/fl_engine.cc Outdated Show resolved Hide resolved

Fix nits

db16569

anirudhb mentioned this pull request Oct 3, 2020

Refactor make_mock_engine into fl_test #21585

Merged

8 tasks

chinmaygarde added the Work in progress (WIP) Not ready (yet) for review! label Oct 23, 2020

anirudhb marked this pull request as draft November 12, 2020 16:46

Leadrive mentioned this pull request Feb 2, 2021

Plugin for flutter.io/videoPlayer go-flutter-desktop/go-flutter#134

Open

chinmaygarde closed this Feb 4, 2021

cloudwebrtc mentioned this pull request Mar 9, 2021

Support for desktop linux version flutter-webrtc/flutter-webrtc#492

Closed

stuartmorgan mentioned this pull request Mar 10, 2021

Linux texture support #24916

Merged

9 tasks

		int64_t fl_external_texture_gl_texture_id(
		FlExternalTextureGl* external_texture);

Linux texture support #20714

Linux texture support #20714

Conversation

anirudhb commented Aug 22, 2020 • edited Loading

Description

Related Issues

Tests

Checklist

Breaking Change

flutter-dashboard bot commented Aug 22, 2020

pingbird commented Aug 22, 2020

anirudhb commented Aug 23, 2020

stuartmorgan commented Aug 25, 2020

stuartmorgan left a comment

Choose a reason for hiding this comment

anirudhb commented Aug 27, 2020 • edited Loading

stuartmorgan commented Aug 31, 2020

stuartmorgan commented Aug 31, 2020

robert-ancell left a comment

Choose a reason for hiding this comment

anirudhb commented Sep 5, 2020 • edited Loading

stuartmorgan commented Sep 8, 2020

anirudhb commented Sep 8, 2020

stuartmorgan commented Sep 8, 2020

anirudhb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuartmorgan commented Sep 28, 2020

chinmaygarde commented Oct 22, 2020

anirudhb commented Oct 23, 2020

stuartmorgan commented Oct 24, 2020

anirudhb commented Nov 12, 2020

chinmaygarde commented Feb 4, 2021

dnsoumik commented Mar 9, 2021

stuartmorgan commented Mar 9, 2021

huanghongxun commented Mar 10, 2021

anirudhb commented Aug 22, 2020 •

edited

Loading

anirudhb commented Aug 27, 2020 •

edited

Loading

anirudhb commented Sep 5, 2020 •

edited

Loading