Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add windows plugin texture support #19405

Merged
merged 18 commits into from
Jan 19, 2021

Conversation

jnschulze
Copy link
Member

@jnschulze jnschulze commented Jun 30, 2020

Description

This PR adds OpenGL texture support for plugins on the Windows platform.

Once this has landed flutter/flutter#61098 needs to be merged as well.

Related Issues

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.

  • 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.
  • Yes, this is a breaking change.

@auto-assign auto-assign bot requested a review from gw280 June 30, 2020 06:53
@jnschulze jnschulze force-pushed the feature/windows-textures branch 3 times, most recently from 7e00d76 to 3d88880 Compare June 30, 2020 07:26
@jnschulze jnschulze marked this pull request as draft June 30, 2020 07:37
@jnschulze jnschulze force-pushed the feature/windows-textures branch from 3d88880 to e16603c Compare June 30, 2020 07:46
@jnschulze jnschulze marked this pull request as ready for review June 30, 2020 08:18
@auto-assign auto-assign bot requested a review from iskakaushik June 30, 2020 08:18
@jnschulze jnschulze force-pushed the feature/windows-textures branch 2 times, most recently from fc7b5ff to d5b4626 Compare June 30, 2020 08:58
@jnschulze
Copy link
Member Author

@stuartmorgan
@chinmaygarde

Could you review this PR please?

@cloudwebrtc
Copy link
Contributor

cloudwebrtc commented Jul 5, 2020

@jnschulze I tested on the flutter-webrtc plugin for windows and it works. You did cool things.

@jnschulze
Copy link
Member Author

@cloudwebrtc
I‘m glad it‘s working. But since this is based on your GLFW texture support PR (#9822) the credit goes to you.

Copy link
Contributor

@clarkezone clarkezone left a comment

Choose a reason for hiding this comment

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

Generally looks good. Will need to rebase on top of #18878 which will be landing first.

@jnschulze jnschulze requested a review from clarkezone July 7, 2020 06:20
@clarkezone
Copy link
Contributor

#18878 has landed now. Please update to reflect that, thx.

@jnschulze jnschulze force-pushed the feature/windows-textures branch 2 times, most recently from 72797f6 to cd70eed Compare July 8, 2020 10:46
@jnschulze
Copy link
Member Author

@clarkezone I've just updated the PR. Please have a look.

Copy link
Contributor

@clarkezone clarkezone left a comment

Choose a reason for hiding this comment

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

LGTM. Is there a demo / hello world example of how to work with external textures?

@jnschulze
Copy link
Member Author

@stuartmorgan done, see #23623

@stuartmorgan
Copy link
Contributor

Recipe change out for review: https://flutter-review.googlesource.com/c/recipes/+/9680

@escamoteur
Copy link

We are currently working on an FFI wrapper for the WebGPU API as a more flexible approach to OpenGL and till I stumbled upon this issue I wasn't aware that its currently not possbile to use a Widget with external Textturebuffer on Windows.
Is this already avalable on MacOs and Linux?

@stuartmorgan
Copy link
Contributor

Is this already avalable on MacOs and Linux?

flutter/flutter#64188

@vjeson
Copy link

vjeson commented Jan 15, 2021

@vjeson

I can't get it to work, could you update your example demo? And it's better to how to use it in multithreading with a video player

I've just updated the demo and added a simple timer that periodically calls MarkTextureFrameAvailable to demonstrate that it can be called from arbitrary threads. See https://github.com/jnschulze/flutter-playground/blob/52005d38e6935c4bbafe07b7f6f5346ca7cc30de/windows_texture_test/windows/windows_texture_test_plugin.cpp#L166

Thanks for your demo,I still have another issue, I'm migrating flutter-webrtc for windows, it uses event_sink onFrame event.
void FlutterVideoRenderer::OnFrame(scoped_refptr frame) {
if (!first_frame_rendered) {
if (event_sink_) {
EncodableMap params;
params[EncodableValue("event")] = "didFirstFrameRendered";
params[EncodableValue("id")] = texture_id_;
//event_sink_->Success(EncodableValue(params));
}
Here I got a thread exception, we need to switch to UI thread to run event_sink_->Success .
Just as the doc official demo, I don't known how to call it on desktop embedding,It seems we didn't export such method for desktop embedding.
https://flutter.dev/docs/development/platform-integration/platform-channels#channels-and-platform-threading
Maybe we need to have a method like this ? task_runners_.GetPlatformTaskRunner()->RunsTasksOnCurrentThread()

@jnschulze
Copy link
Member Author

https://flutter.dev/docs/development/platform-integration/platform-channels#channels-and-platform-threading
Maybe we need to have a method like this ? task_runners_.GetPlatformTaskRunner()->RunsTasksOnCurrentThread()

You're right. Unfortunately, the client wrapper doesn't expose a method like PostTask.

While it should be fairly simple to extend the client wrapper accordingly, we shouldn't discuss it here as it's a general Windows platform issue.

@escamoteur
Copy link

Is this already avalable on MacOs and Linux?

flutter/flutter#64188

So there isn't even an issue for MacOs?

@jnschulze
Copy link
Member Author

jnschulze commented Jan 15, 2021

Is this already avalable on MacOs and Linux?

flutter/flutter#64188

So there isn't even an issue for MacOs?

Only Linux is missing. macOS support was introduced by #8507

@stuartmorgan
Copy link
Contributor

The handling of the new header has landed end-to-end, so this is good to land as soon as the tree re-opens 🎉

@stuartmorgan stuartmorgan added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Jan 19, 2021
@dahabit
Copy link

dahabit commented Jan 19, 2021

@stuartmorgan Thank you for your support, I really appreciate it, this issue was blocking using for making cool Windows desktop apps with Flutter.

can't wait to see it land in the stable channel

@oddko
Copy link

oddko commented May 18, 2021

What would it require for this to gain some zero-copy interop with an OpenGL texture/FBO (and/or other APIs) ? Adapted the provided example to have a texture backed by an OpenGL renderer and calls to glReadPixels are prohibitively expensive for any real time rendering (around 10ms for a 1920x1080 texture, if not more on lower end systems). Pixel buffers aren't suited for such use.

@stuartmorgan
Copy link
Contributor

stuartmorgan commented May 18, 2021

What would it require for this to gain some zero-copy interop with an OpenGL texture/FBO (and/or other APIs) ?

The first step would be a design doc with one or more concrete proposals for possible texture sources, including exploring what that API would look like, how it would work with Angle, and what the impact on that API of an eventual migration to a DirectX Skia backend would be.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. Work in progress (WIP) Not ready (yet) for review!
Projects
None yet
Development

Successfully merging this pull request may close these issues.