engine
engine copied to clipboard
flutter
[DRAFT][ios][platform_view][performance] overlay intersection
Replace this paragraph with a description of what this PR is changing or adding, and why. Consider including before/after screenshots.
List which issues are fixed by this PR. You must list at least one issue.
https://github.com/flutter/flutter/issues/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
- [ ] I read the Contributor Guide and followed the process outlined there for submitting PRs.
- [ ] I read the Tree Hygiene wiki page, which explains my responsibilities.
- [ ] I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
- [ ] I listed at least one issue that this PR fixes in the description above.
- [ ] I added new tests to check the change I am making or feature I am adding, or the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
- [ ] I updated/added relevant documentation (doc comments with
///). - [ ] I signed the CLA.
- [ ] All existing and new tests are passing.
If you need help, consider asking for advice on the #hackers-new channel on Discord.
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 1:
"average_frame_rasterizer_time_millis": 7.543352459016395,
"stddev_frame_rasterizer_time_millis": 2.3777649825315796,
"90th_percentile_frame_rasterizer_time_millis": 11.35,
"99th_percentile_frame_rasterizer_time_millis": 25.963,
"worst_frame_rasterizer_time_millis": 32.74,
"missed_frame_rasterizer_budget_count": 6,
Run 2:
"average_frame_rasterizer_time_millis": 7.445008264462812,
"stddev_frame_rasterizer_time_millis": 2.144756232497781,
"90th_percentile_frame_rasterizer_time_millis": 11.018,
"99th_percentile_frame_rasterizer_time_millis": 24.253,
"worst_frame_rasterizer_time_millis": 26.484,
"missed_frame_rasterizer_budget_count": 4,
Run 3:
"average_frame_rasterizer_time_millis": 7.6296694214876055,
"stddev_frame_rasterizer_time_millis": 2.3363895908749415,
"90th_percentile_frame_rasterizer_time_millis": 11.072,
"99th_percentile_frame_rasterizer_time_millis": 24.35,
"worst_frame_rasterizer_time_millis": 27.078,
"missed_frame_rasterizer_budget_count": 6,
After the fix: Run 1:
"average_frame_rasterizer_time_millis": 5.863311827956986,
"stddev_frame_rasterizer_time_millis": 1.990753728754767,
"90th_percentile_frame_rasterizer_time_millis": 9.91,
"99th_percentile_frame_rasterizer_time_millis": 15.554,
"worst_frame_rasterizer_time_millis": 16.889,
"missed_frame_rasterizer_budget_count": 1,
Run 2:
"average_frame_rasterizer_time_millis": 5.756603260869563,
"stddev_frame_rasterizer_time_millis": 2.0359048913043467,
"90th_percentile_frame_rasterizer_time_millis": 10.113,
"99th_percentile_frame_rasterizer_time_millis": 15.44,
"worst_frame_rasterizer_time_millis": 15.971,
"missed_frame_rasterizer_budget_count": 0,
Run 3:
"average_frame_rasterizer_time_millis": 5.811156756756758,
"stddev_frame_rasterizer_time_millis": 2.0427237399561724,
"90th_percentile_frame_rasterizer_time_millis": 9.815,
"99th_percentile_frame_rasterizer_time_millis": 15.915,
"worst_frame_rasterizer_time_millis": 16.57,
"missed_frame_rasterizer_budget_count": 1,
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 https://github.com/flutter/flutter/issues/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.
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.
Jenn (OOO this week) probably has more ideas on the benchmark project (e.g. use UIWebView?) so let's wait for her.
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?).
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. EDIT: I see the project now.
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 CAMetalLayers per platform view with surface of size of entire screen. I'm still hoping one they this can be implemented same way macOS embedder does it - single IOSurface shared between multiple overlay CALayers. That itself would likely solve the performance problem here.
This seems like a result of not being aware of physical pixels on framework level.
@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.
The solution here likely helps in this scenario, but what if there is actual one pixel 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 DlRegion that uses floating point or fixed precision numbers. This would probably have some performance implications though. Edit: On second thought this is probably a bad idea, as this would probably get incorrect result because we care about physical pixels when deciding whether to display the overlay.
This seems like a result of not being aware of physical pixels on framework level.
@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.
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.
Well, yes, but if content was snapped to physical pixes then the roundOut would not do anything. So you would not get the overlap.
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
@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.
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
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
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.
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.
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:
- this "hairline" overlay is actually not rendered correctly, because it only receives colors from the edge of flutter side. It does not receive colors from the edge of platform view, and does not interpolate colors from both sides.
- some Ad SDKs (not sure about admob tho) actually check whether ad banners are obstructed to prevent fraud.
(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.
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.
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:
- this "hairline" overlay is actually not rendered correctly, because it only receives colors from the edge of flutter side. It does not receive colors from the edge of platform view, and does not interpolate colors from both sides.
- some Ad SDKs (not sure about admob tho) actually check whether ad banners are obstructed to prevent fraud.
(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 platform_view_bounds.RoundIn() to do the rectangle query. If a rendering intersects that rectangle then the overlap is more likely deliberate. Also, the simple case of adjacent widgets that fall on the same edge pixel as the platform view will not trigger this new query rectangle.
platform_view_bounds.RoundOut() should still be the rectangle donated to the platform to draw its widget, though.
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...?
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.
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 platform_view_bounds.RoundIn() to do the rectangle query.
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?
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.
Strongly agreed!
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.
Will do. I haven't seen flicker as of this change for now, but will test more.
Do we potentially also want to make the same change to the other platform embedders?
Yes I think so
I think you need to rebase to ToT due to the infra misconfiguration causing the ci.yaml step to fail.
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.
I think this should also fix https://github.com/flutter/flutter/issues/138656, where we end up creating a fullscreen blur due to the 1px overlay layer.
I think this should also fix https://github.com/flutter/flutter/issues/138656, where we end up creating a fullscreen blur due to the 1px overlay layer.
Let me take a look.
SkiaPerf noted some improvements on this PR: https://flutter-flutter-perf.skia.org/e/?begin=1710569785&end=1711132808&keys=Xfcc3b437ed855c530c4746be51cac5da&requestType=0&selected=commit%3D39955%26name%3D%252Carch%253Dintel%252Cbranch%253Dmaster%252Cconfig%253Ddefault%252Cdevice_type%253DiPhone_11%252Cdevice_version%253Dnone%252Chost_type%253Dmac%252Csub_result%253D90th_percentile_frame_rasterizer_time_millis%252Ctest%253Dplatform_views_scroll_perf_ad_banners__timeline_summary%252C&xbaroffset=39955
