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

Embedder uses too many surfaces for platform views #129710

Closed
knopp opened this issue Jun 28, 2023 · 10 comments · Fixed by flutter/engine#43301
Closed

Embedder uses too many surfaces for platform views #129710

knopp opened this issue Jun 28, 2023 · 10 comments · Fixed by flutter/engine#43301
Labels
a: desktop Running on desktop a: platform-views Embedding Android/iOS views in Flutter apps c: new feature Nothing broken; request for a new capability engine flutter/engine repository. See also e: labels. fyi-linux For the attention of the Linux platform team fyi-macos For the attention of macOS platform team fyi-windows For the attention of the Windows platform team P3 Issues that are less important to the Flutter project

Comments

@knopp
Copy link
Member

knopp commented Jun 28, 2023

Here is example application that has 4 platform views interleaved with Flutter content:

Screenshot 2023-06-28 at 14 22 26

It uses a total of 5 surfaces, each one has size of entire window (because resizing surfaces is expensive). However there is no content directly painted over platform view so this entire scene could be rendered to the background surface and the end result would be same, saving 4 surfaces.

Another example - 4 platform views with Flutter content painted above the views:

Screenshot 2023-06-28 at 14 21 51

This still uses 5 surfaces, but the same visual result can be represented by only two surfaces - content that is painted below platform views in first surface and content that is painted above platform views in second surface.

Same scene, painted with https://flutter.dev/go/optimized-platform-view-layers:

optimized-platform-view-layers.mov

Initially, there are two surface, but as they move to not paint over platform views, only one surface remains.

@knopp knopp added c: new feature Nothing broken; request for a new capability a: platform-views Embedding Android/iOS views in Flutter apps a: desktop Running on desktop engine flutter/engine repository. See also e: labels. labels Jun 28, 2023
@stuartmorgan
Copy link
Contributor

Won't this potentially create a lot of view allocation and destruction during animation?

@knopp
Copy link
Member Author

knopp commented Jun 28, 2023

I assume you mean surface allocations? Is the scenario something like a platform view being repeatedly covered and then uncovered by Flutter content during animation?

If this turns out to be a problem we can cache the surfaces more aggressively in the embedder and release only after they haven't been used for n-frames (or some time). Hover same scenario right now on iOS would currently result in repeatedly creating and destroying one or two CAMetalLayers per platform view, which is quite a bit more expensive than a single surface and I assume it's not a big issue in practice there so I don't think it's going to be here.

@lucalooz
Copy link

Hi @knopp,
do you think that flutter/engine#43301 could have a positive impact on #129632?

@knopp
Copy link
Member Author

knopp commented Jun 28, 2023

Hi @knopp,
do you think that flutter/engine#43301 could have a positive impact on #129632?

Unlikely, because iOS doesn't use the embedder API and the platform view implementation is very much different between these two. It might eventually get migrated to embedder API, which in my opinion should improve platform view performance quite a bit, but I'm not aware of any timeline for this.

@knopp knopp closed this as completed Jun 28, 2023
@knopp knopp reopened this Jun 28, 2023
@loic-sharma loic-sharma added the P3 Issues that are less important to the Flutter project label Jun 29, 2023
@jwinarske
Copy link

@knopp Is this example available somewhere? I have started work on platform views support for Wayland compositor on Linux, and this would be a great starting point.

@knopp
Copy link
Member Author

knopp commented Nov 26, 2023

@knopp Is this example available somewhere? I have started work on platform views support for Wayland compositor on Linux, and this would be a great starting point.

https://github.com/knopp/layer_playground

Might not be up-to-date with latest platform view API in Flutter though.

@jwinarske
Copy link

@knopp great thanks!

knopp added a commit to flutter/engine that referenced this issue Nov 27, 2023
…3301)

Fixes flutter/flutter#129710
Fixes flutter/flutter#138936

Implements https://flutter.dev/go/optimized-platform-view-layers

## Example

This scene would normally require 5 surfaces, but with the PR it comes
down to 2 (when drawing over platform views) and 1 when all drawing is
outside of platform views).


https://github.com/flutter/flutter/assets/96958/091da832-bfbc-44a2-8da5-d55d84024c96

*If you had to change anything in the [flutter/tests] repo, include a
link to the migration guide as per the [breaking change policy].*

## Pre-launch Checklist

- [x] I read the [Contributor Guide] and followed the process outlined
there for submitting PRs.
- [x] I read the [Tree Hygiene] wiki page, which explains my
responsibilities.
- [x] I read and followed the [Flutter Style Guide] and the [C++,
Objective-C, Java style guides].
- [x] I listed at least one issue that this PR fixes in the description
above.
- [x] 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.
- [x] I updated/added relevant documentation (doc comments with `///`).
- [x] I signed the [CLA].
- [x] All existing and new tests are passing.

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

<!-- Links -->
[Contributor Guide]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#overview
[Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene
[Flutter Style Guide]:
https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo
[C++, Objective-C, Java style guides]:
https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
[testing the engine]:
https://github.com/flutter/flutter/wiki/Testing-the-engine
[CLA]: https://cla.developers.google.com/
[flutter/tests]: https://github.com/flutter/tests
[breaking change policy]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes
[Discord]: https://github.com/flutter/flutter/wiki/Chat
harryterkelsen pushed a commit to harryterkelsen/engine that referenced this issue Nov 27, 2023
…utter#43301)

Fixes flutter/flutter#129710
Fixes flutter/flutter#138936

Implements https://flutter.dev/go/optimized-platform-view-layers

## Example

This scene would normally require 5 surfaces, but with the PR it comes
down to 2 (when drawing over platform views) and 1 when all drawing is
outside of platform views).


https://github.com/flutter/flutter/assets/96958/091da832-bfbc-44a2-8da5-d55d84024c96

*If you had to change anything in the [flutter/tests] repo, include a
link to the migration guide as per the [breaking change policy].*

## Pre-launch Checklist

- [x] I read the [Contributor Guide] and followed the process outlined
there for submitting PRs.
- [x] I read the [Tree Hygiene] wiki page, which explains my
responsibilities.
- [x] I read and followed the [Flutter Style Guide] and the [C++,
Objective-C, Java style guides].
- [x] I listed at least one issue that this PR fixes in the description
above.
- [x] 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.
- [x] I updated/added relevant documentation (doc comments with `///`).
- [x] I signed the [CLA].
- [x] All existing and new tests are passing.

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

<!-- Links -->
[Contributor Guide]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#overview
[Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene
[Flutter Style Guide]:
https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo
[C++, Objective-C, Java style guides]:
https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
[testing the engine]:
https://github.com/flutter/flutter/wiki/Testing-the-engine
[CLA]: https://cla.developers.google.com/
[flutter/tests]: https://github.com/flutter/tests
[breaking change policy]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes
[Discord]: https://github.com/flutter/flutter/wiki/Chat
@renanyoy
Copy link

renanyoy commented Dec 5, 2023

This fix removed the ability to use Opacity(...) on platform views, platform views are only displayed when opacity = 1 now and hidden otherwise.

In my usage, as many platform views are displayed a lot faster. I can avoid using opacity now.. it's just to signal it.

@knopp
Copy link
Member Author

knopp commented Dec 5, 2023

I seem to be unable to reproduce this with layer_playground example:

Screenshot 2023-12-05 at 10 55 56

(blue rectangles are platform views wrapped with 0.5 Opacity). So there must be some additional factor.

Copy link

This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of flutter doctor -v and a minimal reproduction of the issue.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 19, 2023
@cbracken cbracken added fyi-macos For the attention of macOS platform team fyi-windows For the attention of the Windows platform team fyi-linux For the attention of the Linux platform team and removed team-desktop labels Jun 7, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
a: desktop Running on desktop a: platform-views Embedding Android/iOS views in Flutter apps c: new feature Nothing broken; request for a new capability engine flutter/engine repository. See also e: labels. fyi-linux For the attention of the Linux platform team fyi-macos For the attention of macOS platform team fyi-windows For the attention of the Windows platform team P3 Issues that are less important to the Flutter project
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants