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

iOS embedder corrupts the scene if its Paint() method is not called #81419

Open
flar opened this issue Apr 28, 2021 · 3 comments
Open

iOS embedder corrupts the scene if its Paint() method is not called #81419

flar opened this issue Apr 28, 2021 · 3 comments
Labels
a: platform-views Embedding Android/iOS views in Flutter apps a: plugins Support for writing, building, and running plugin packages engine flutter/engine repository. See also e: labels. P2 Important issues not at the top of the work list platform-ios iOS applications specifically team-ios Owned by iOS platform team triaged-ios Triaged by iOS platform team

Comments

@flar
Copy link
Contributor

flar commented Apr 28, 2021

To fix #69552 we changed the engine to always Preroll all layers even if they may be culled. (See flutter/engine#22336)

As a result, layers may now be Prerolled even if they won't eventually be painted on a given scene.

The iOS embedder was not expecting this behavior and so it can leave visible pieces of its embedded view on the screen after it is scrolled away. (See #76097) Note that the Android embedder does not have this same issue, so this is a design issue with the way we manage the iOS embedder.

A work-around was added to always Paint all layers in a sub-tree that contains a PlatformViewLayer. (See flutter/engine#25824)

If the iOS embedder is taught to not make assumptions about Paint() being called, then the work-around mentioned above can be deleted.

@flar flar added the engine flutter/engine repository. See also e: labels. label Apr 28, 2021
@cyanglaz cyanglaz added a: platform-views Embedding Android/iOS views in Flutter apps platform-ios iOS applications specifically plugin P1 High-priority issues at the top of the work list labels Apr 29, 2021
@cyanglaz
Copy link
Contributor

cyanglaz commented Apr 29, 2021

@flar Thanks for the summary. Does submitFrame always get called in this situation?

Also, what's the desired behavior in this situation? If paint is not called, should the platform view not be displayed? Or should the platform view keep the same composition behavior (position, size, clips etc)?

@flar
Copy link
Contributor Author

flar commented Apr 29, 2021

@flar Thanks for the summary. Does submitFrame always get called in this situation?

I'm not sure what goes on in the embedder, I'm just looking at it from an Engine "flow" layers perspective.

The conditions under which this happens is when a parent Clip layer realizes that it isn't visible on the screen (maybe because it is nested inside another clip with no intersection, or maybe it was placed just off-screen). In that case the ClipLayer used to make the decision on behalf of its children that it won't bother to preroll them and we'd also end up not calling their Paint().

We now preroll all children because that was faulty logic. The clip might believe it's children won't be painted, but we have other reasons to paint things that end up calling the Paint() methods anyway. So, now we Preroll all children even if they are not visible.

As a result we will always call PlatformViewLayer::Preroll() any time it is in a scene even if it isn't visible.

And in cases like the test case in the associated issue, we don't then follow that up with a call to PlatformViewLayer::Paint() and we end up with stale pieces of the embedded view on the screen.

The Android embedder is fine in this case. It will not carve out any pieces of the screen if we call its Preroll and don't call its Paint().

I think what is happening is that whatever calls we make to the embedder in the Preroll() method have the iOS embedder think "OK, so I'm going to appear on screen, I'll make note of that. For simplicity I'll leave the previous bounds/area that we last appeared in still active and assume that the Paint() call will fix that". In the Paint() method it revises the concept of where and how much of it appears and updates that data. The problem is that if we don't actually call the Paint() method (as in the case of being clipped out) then we are left with the stale data.

@chinmaygarde and I briefly looked at the code that computes what is visible and thought that perhaps if we simply invalidated it in the Preroll flow that we would end up with a potentially expensive situation where it recomputes the exact same visible portion on every frame, but that may have been a mistake assumption. In the end, since calling the Paint() method fixed the problem we decided to just work out a way to have the layers always call Paint() on a PlatformViewLayer as the simplest solution. If someone can figure out how to have Preroll not enable a stale "viewport" or whatever it computes, then that would be a more targeted solution. And possibly, if correcting the viewport data is expensive, have a way to mark it as "stale" without destroying it and then the Paint() method can mark it "current" after reevaluating the contextual data. If it's not expensive to recompute, then perhaps the Preroll() method should clear it out so that we don't leave a hole punched in the app if we don't end up calling Paint()?

Also, what's the desired behavior in this situation? If paint is not called, should the platform view not be displayed? Or should the platform view keep the same composition behavior (position, size, clips etc)?

If Paint() was not called, it shouldn't paint. The fact that its painting may happen somewhere else in the frame cycle (i.e. not directly inside the Layer::Paint() call) is incidental. Preroll just gathers data and doesn't imply that a layer will paint. Paint() is the method that implies "now go ahead and show yourself" - so if we don't call Paint() on a given frame, then don't paint anything...

@jmagman jmagman added this to Awaiting triage in iOS Platform - platform view review Jun 8, 2021
@cyanglaz cyanglaz moved this from Awaiting triage to Engineer reviewed in iOS Platform - platform view review Aug 26, 2021
@stuartmorgan stuartmorgan added a: plugins Support for writing, building, and running plugin packages and removed plugin labels Mar 6, 2023
@flutter-triage-bot flutter-triage-bot bot added multiteam-retriage-candidate team-ios Owned by iOS platform team triaged-ios Triaged by iOS platform team labels Jul 8, 2023
@flutter-triage-bot flutter-triage-bot bot removed the triaged-ios Triaged by iOS platform team label Dec 8, 2023
@flutter-triage-bot
Copy link

This issue is marked P1 but has had no recent status updates.

The P1 label indicates high-priority issues that are at the top of the work list. This is the highest priority level a bug can have if it isn't affecting a top-tier customer or breaking the build. Bugs marked P1 are generally actively being worked on unless the assignee is dealing with a P0 bug (or another P1 bug). Issues at this level should be resolved in a matter of months and should have monthly updates on GitHub.

Please consider where this bug really falls in our current priorities, and label it or assign it accordingly. This allows people to have a clearer picture of what work is actually planned. Thanks!

@vashworth vashworth added P2 Important issues not at the top of the work list triaged-ios Triaged by iOS platform team and removed P1 High-priority issues at the top of the work list labels Dec 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a: platform-views Embedding Android/iOS views in Flutter apps a: plugins Support for writing, building, and running plugin packages engine flutter/engine repository. See also e: labels. P2 Important issues not at the top of the work list platform-ios iOS applications specifically team-ios Owned by iOS platform team triaged-ios Triaged by iOS platform team
Projects
Development

No branches or pull requests

6 participants