-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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][platform_view][performance] overlay intersection #50637
[ios][platform_view][performance] overlay intersection #50637
Conversation
We should benchmark it again when we have a better benchmark project and better toolings, but I am already getting some good result so far on my iPhone 13 mini: Before the fix:
Run 2:
Run 3:
After the fix:
Run 2:
Run 3:
Observation About 22% time reduction on "average frames", and 40%+ time reduction on "worst frames". Also pretty significant reduction (800%, from 5.33 to 0.67 frames) on "missing frames". Though keep in mind that "missing frames" can vary dramatically based on how we write the sample project. The dummy benchmark project I used is pretty much the same as https://github.com/lucalooz/flutter_ads_list_perf from flutter/flutter#129632, except that I don't use a real AdMob banner. I also put like 7-8 banners on screen. The numbers will be smaller if we have less platform views on screen. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great work so far! I need to study this code a bit to make sure I understand it, but I'd encourage you to get the benchmark you are using locally on CI.
If you want to pair on that, I'd be happy to sit down tomorrow or friday morning, but otherwise I'll be out a lot the rest of this week.
@@ -706,6 +706,14 @@ static bool ClipRRectContainsPlatformViewBoundingRect(const SkRRect& clip_rrect, | |||
int64_t current_platform_view_id = composition_order_[j - 1]; | |||
SkRect platform_view_rect = GetPlatformViewRect(current_platform_view_id); | |||
std::vector<SkIRect> intersection_rects = slice->region(platform_view_rect).getRects(); | |||
for (auto it = intersection_rects.begin(); it != intersection_rects.end(); /*no-op*/) { | |||
if (it->width() <= 1 || it->height() <= 1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So if the intersection is less than a pixel in either direction, we ignore it? I suppose this makes sense - its why we were seeing those 1 pixel boundaries.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the intersection values you are seeing are even smaller than 1px, you could tighten the condition further (i.e. 0.5 or 0.25 or 0.1) but that might be unecessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are getting 1 pixel because of a roundOut operation. Note that this is already in physical pixels in integers and won't have 0.5 here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh, where does the round out happen?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The roundOut
happens to both flutter layer and platform view layer. For flutter layer, it's https://github.com/flutter/engine/blob/main/display_list/geometry/dl_rtree.cc#L209, for platform view layer, it's https://github.com/flutter/engine/blob/main/flow/embedded_views.h#L343
For example, if we interleave flutter widget and platform view in a list, and let's say we have a flutter widget (top = 0, bottom = 100.1), and a platform view below that widget (top = 100.1, bottom = 200). They are NOT supposed to be overlapping. However, after rounding out, we will get flutter widget (top = 0, bottom = 101), and platform view (top = 100, bottom 200). This will result in 1 pixel overlap as long as there's a floating point in the coord.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This particular problem is scrolling though, which is a very general problem. We are treating platform views and flutter Ui as always overlapping, even if that overlap is a near infinitesimal
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is scrolling any different? It's just a layout. Scroll to pixel boundaries and the problem disappears...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding a 1 unit pad between the widget and the platform view would also prevent them from overlapping. Regardless of DPI, 1 unit in the framework layout coordinates will always be at least 1 pixel. That doesn't require any engine changes, just add a Padding around the widget or the PV.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This issue is a scrolling issue. Have you looked at the benchmark application?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To clarify, the overlap between (0-100.1) and (100.1-200) is 0, not 0.1. The reason we get 1 pixel overlay is because we computed the intersection AFTER we roundOut
both sizes.
An alternative is to compute the intersection in the original floating point values (without roundOut).
Jenn (OOO this week) probably has more ideas on the benchmark project (e.g. use For now this dummy project is just my personal playground. What I did was pretty much the same as https://github.com/lucalooz/flutter_ads_list_perf, except that I don't use real AdMob (since it's a non-flutter-org repo). Also I used 320x50 standard banner size, so I can fit 7-8 banners on screen, which should be enough (i guess?). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not a robust solution and may fix a specific test case, but it will cause unintended problems with other situations.
@@ -706,6 +706,14 @@ static bool ClipRRectContainsPlatformViewBoundingRect(const SkRRect& clip_rrect, | |||
int64_t current_platform_view_id = composition_order_[j - 1]; | |||
SkRect platform_view_rect = GetPlatformViewRect(current_platform_view_id); | |||
std::vector<SkIRect> intersection_rects = slice->region(platform_view_rect).getRects(); | |||
for (auto it = intersection_rects.begin(); it != intersection_rects.end(); /*no-op*/) { | |||
if (it->width() <= 1 || it->height() <= 1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should round out because we are looking for actual pixel interference, not intended geometry interference.
The object that ends at 100.1 could contribute up to .1 of its color to that pixel and so it contributes to that pixel and so it SHOULD be included in the query. This assumes AA rendering, which is the default. If there was non-AA rendering going on then .1 of a pixel would not render anything - but there is no way of knowing what kind of rendering is happening at this level. This could potentially be noted during DisplayList construction and the bounding rects adjusted at some cost. Also, the switch to Impeller will mean that all rendering is MSAA and we have no good way to predict how much of a pixel will be covered for a given sub-pixel overlap.
We shouldn't ignore single pixel overlaps because both contributors will attempt to modify the pixel. If you feel that the two Flutter objects should not be considered overlapping, then Flutter should not lay them out with overlapping pixels.
This goes back to my suggestion that Flutter implement pixel-aware layouts. Until it does then we end up with AA overlaps between allegedly non-overlapping widgets and we have this problem.
I'd also be interested in hearing from @knopp on this. |
I agree with @flar here. This seems like a result of not being aware of physical pixels on framework level. Personally I'm pixel snapping all my applications, even wrote a package for it, but it's one I wish I didn't have to write. @hellohuanlin, is there a sample project for this? I'd like to run it with pixel_snap to see if it fixes the issue. The solution here likely helps in this scenario, but what if there is actual one pixel overlap? Unfortunately at the point where there overlap is tested the rect has already part of region and region works with integers only. It doesn't help that unobstructed platform view overlays on iOS are currently ridiculously expensive. Up to 2 |
@knopp I think it's the opposite - we are supposed to do geometric intersection between 2 sizes (in floating points), but instead we did pixel intersection (in integers), resulting in a false positive detection of overlap.
If we do geometric intersection rather than pixel intersection, then this should be solved (if we care about this 1 pixel overlap) |
Another thing to consider would be a |
Well, yes, but if content was snapped to physical pixes then the roundOut would not do anything. So you would not get the overlap. Region represents the area of surface where something paints. If you have fractional position that means whole pixel is affected. |
Not sure how "snapping" works. Does it mean you always align widgets in pixel grid? Can you explain more? Is there a proposal doc that I can refer to? |
@hellohuanlin if you have RectA (0, 0, 1.1, 1.1) and RectB (1.1, 1.1, 2, 2), they don't overlap, geometric intersection will tell you that they don't overlap. But we don't have infinite resolution surfaces, so they both affect physical pixel 1,1, so they infact do overlap. |
They do overlap from that perspective, unless the platform view is opaque and ontop . However the overlap is small and I think of the cost is severe enough we should make a fidelity trade off here |
Ya, but this is doing pixel intersection of 2 rects, where colors from both rects contributes to the same pixel (which is not the case for platform view overlays). My point is, for the purpose of platform view overlay logic, we should be doing geometric intersection, and not to put an overlay layer of size 1x1 on top of the platform view. |
Let's bring the discussion to real time chat on discord if it's easier. @knopp I couldn't find your username there, so here's the link: https://discord.com/channels/608014603317936148/608021010377080866/1209974674696437781 |
It's not just fidelity. This one pixel overlay basically defeats the whole purpose of unobstructed platform views, as it is a (unnecessary) layer directly on top of ad. Not sure how to fix this though. The proposed solution basically means if there is an intentional 1px hairline overlay somewhere it will not be rendered. Is that common? Is that going to be a problem? I'd not think so but I don't have anything to back this with. If we want more fidelity, we'd need changes to DlRegion that'd preserve the fractional positions. I'm not to keen on that. |
For the record, here is the flutter_ads_list_perf example snapped to physical pixels. As expected this solves the problem, the only overlay seems to be one for the debug banner. Unfortunately this requires a manual intervention and a third party package so not really a transparent solution. |
Put aside the performance impact for now, I think it is a tradeoff between false positive and false negative: (1) The existing behavior has a false positive of the "hairline" overlay, when we interleave ads and flutter widgets in a list. Also note:
(2) On the other hand, the new behavior has a false negative of the "hairline" overlay. It fails to display the "hairline" of 1 pixel when we actually want it. Again, even if we add this hairline, it is not rendered correctly most of the time, as it does not interpolate the colors from both sides. Then if we weight in the performance impact, I am leaning towards the new behavior. |
Just to be certain as I'm not sure we are talking about the same thing. With a platform view at (100, 100) -> (200, 200) (it's 100x100 in size)... an overlap rectangle of (100, 100) -> (200, 101) is a potentially unintended overlap and it probably doesn't matter to the flutter app if we ignore it (potentially a point for discussion, but I think it is unlikely) and the proposed solution would ignore it (shove it behind the platform view) an overlap rectangle of (100, 150) -> (200, 151) is definitely absolutely an intended overlap, but it happens to only be 1 pixel in height which means the solution as proposed will delete this rendering. I really think that is bad behavior of the fix and likely just a mistake. Now both situations could be called "hairlines", but the way that they intersect the platform view can make them seem intentional or accidental. The first case is what I believe this fix is trying to eliminate. The later case is something that this fix also eliminates that I don't think it intends to eliminate...? |
An alternate solution that I think doesn't have the unintended side effect of eliminating a 1-pixel line drawn across the middle of the platform view would be to simply use
|
Good call about the second case! I was only referring to the first case (edge hairline) and was trying to explain why it's better not to show the edge hairline. But yes non-edge hairline should be shown for sure.
I think it's a good idea. |
So the basic rule here is that if you want to draw over a platform view, you need to overlap it by more than a single pixel. I can't imagine that anyone would find value in drawing over just a single pixel (wide/tall) sliver on the edge of a platform view. Possibly if they were trying to give it a border, but that would be better accomplished by drawing a more than hairline box around it. One issue that I think still remains to be seen is whether an adjacent contrasting widget will flicker as you scroll it next to a platform view. I don't think that will be visible on platforms which use a whole number for the DPR (such as Apple devices?) but that should be tested to be sure. It might come into play on platforms that have non-integer DPRs, but this change is only being proposed for iOS. Do we potentially also want to make the same change to the other platform embedders? |
Strongly agreed!
Will do. I haven't seen flicker as of this change for now, but will test more.
Yes I think so |
f68ea30
to
0fa06bf
Compare
// TODO(hellohuanlin): iOS only for now, but we should try it on other | ||
// platforms. We should deprecate the above `region` function if we migrate | ||
// all platforms to use this function. | ||
DlRegion roundedInRegion(const SkRect& query) const { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps have the region() method take an SkIRect and let the caller decide how to turn their SkRect into the appropriate pixel query?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be a common functionality across platforms. I'd prefer to keep the logic here so we can try it on other platforms, rather than duplicate the logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess my thinking was that this class should not be imposing rounding logic assumptions on the callers. It has "pixel based" data to share and should expect "pixel based" queries from the callers.
But this is just a nit suggestion...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha. I do see pros and cons. Worth to think about.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The eventual name should just be "region" if we consolidate.
I think you need to rebase to ToT due to the infra misconfiguration causing the ci.yaml step to fail. |
Oh this is just a draft PR that I open for early discussion purpose. I haven't added test etc and I still need to play around when new benchmark projects and toolings are ready. |
0fa06bf
to
99943fc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I added a nit about the eventual name of the consolidated method, not sure if it really needs a direct mention in the comments. I'll leave that up to you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I think this should also fix flutter/flutter#138656, where we end up creating a fullscreen blur due to the 1px overlay layer. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving, but I'd like to see the test case nesting made more straight-forward so it is more readable for future maintenance.
Let me take a look. |
5b26a0c
to
5a81c3f
Compare
…145578) flutter/engine@e2f324b...5a12de1 2024-03-21 41930132+hellohuanlin@users.noreply.github.com [ios][platform_view][performance] overlay intersection (flutter/engine#50637) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC jonahwilliams@google.com,rmistry@google.com,zra@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Address the performance of platform view due to an extra overlay. This overlay was added due to the following rounding problem:
(Quote myself from the discussion below).
List which issues are fixed by this PR. You must list at least one issue.
Fixes flutter/flutter#143420
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
///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.