Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Commit 01d42ad

Browse files
authored
[ios]ignore single edge pixel instead of rounding (#51687)
The previous PR flutter/flutter#143420 rounds out the layers and rounds in the platform views. This results in missing pixel on the edge of the intersection when there's fractional coordinate (as shown in the screenshot below), because platform view is below the layers. It turns out that we have to round out both platform view and layers, because: - rounding in platform view rects will result in missing pixels on the edge of the intersection. - rounding in layer rects will result in missing pixels on the edge of the layer that's on top of the platform view. This PR simply skips the single (or partial) pixel on the edge, which is a special case, while still preserve the `roundOut` behavior for general non-edge cases. Before the fix, notice a very thin gray line cutting through the purple box: <img src="https://github.com/flutter/engine/assets/41930132/1482d81a-337e-4841-ac08-eff08bbc71ef" height="500"> Then after the fix, the gray line is gone: <img src="https://github.com/flutter/engine/assets/41930132/0eddae69-ab62-4de6-8932-c67cc5aced73" height="500"> *List which issues are fixed by this PR. You must list at least one issue.* 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].* [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
1 parent 00dab0d commit 01d42ad

File tree

6 files changed

+112
-29
lines changed

6 files changed

+112
-29
lines changed

flow/embedded_views.h

Lines changed: 0 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -339,31 +339,10 @@ class EmbedderViewSlice {
339339
virtual DlCanvas* canvas() = 0;
340340
virtual void end_recording() = 0;
341341
virtual const DlRegion& getRegion() const = 0;
342-
// TODO(hellohuanlin): We should deprecate this function if we migrate
343-
// all platforms to use `roundedInRegion`. Then we should rename
344-
// `roundedInRegion` to just `region`.
345342
DlRegion region(const SkRect& query) const {
346343
return DlRegion::MakeIntersection(getRegion(), DlRegion(query.roundOut()));
347344
}
348345

349-
// TODO(hellohuanlin): iOS only for now, but we should try it on other
350-
// platforms.
351-
DlRegion roundedInRegion(const SkRect& query) const {
352-
// Use `roundIn` to address a performance issue when we interleave embedded
353-
// view (the queried rect) and flutter widgets (the slice regions).
354-
// Rounding out both the queried rect and slice regions will
355-
// result in an intersection region of 1 px height, which is then used to
356-
// create an overlay layer. For each overlay, we acquire a surface frame,
357-
// paint the pixels and submit the frame. This resulted in performance
358-
// issues since the surface frame acquisition is expensive. Since slice
359-
// regions are already rounded out (see:
360-
// https://github.com/flutter/engine/blob/5f40c9f49f88729bc3e71390356209dbe29ec788/display_list/geometry/dl_rtree.cc#L209),
361-
// we can simply round in the queried rect to avoid the situation.
362-
// After rounding in, it will ignore a single (or partial) pixel overlap,
363-
// and give the ownership to the platform view.
364-
return DlRegion::MakeIntersection(getRegion(), DlRegion(query.roundIn()));
365-
}
366-
367346
virtual void render_into(DlCanvas* canvas) = 0;
368347
};
369348

shell/platform/darwin/ios/framework/Source/FlutterPlatformViews.mm

Lines changed: 24 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -695,6 +695,8 @@ static bool ClipRRectContainsPlatformViewBoundingRect(const SkRRect& clip_rrect,
695695
auto did_submit = true;
696696
auto num_platform_views = composition_order_.size();
697697

698+
// TODO(hellohuanlin) this double for-loop is expensive with wasted computations.
699+
// See: https://github.com/flutter/flutter/issues/145802
698700
for (size_t i = 0; i < num_platform_views; i++) {
699701
int64_t platform_view_id = composition_order_[i];
700702
EmbedderViewSlice* slice = slices_[platform_view_id].get();
@@ -705,8 +707,28 @@ static bool ClipRRectContainsPlatformViewBoundingRect(const SkRRect& clip_rrect,
705707
for (size_t j = i + 1; j > 0; j--) {
706708
int64_t current_platform_view_id = composition_order_[j - 1];
707709
SkRect platform_view_rect = GetPlatformViewRect(current_platform_view_id);
708-
std::vector<SkIRect> intersection_rects =
709-
slice->roundedInRegion(platform_view_rect).getRects();
710+
std::vector<SkIRect> intersection_rects = slice->region(platform_view_rect).getRects();
711+
const SkIRect rounded_in_platform_view_rect = platform_view_rect.roundIn();
712+
// Ignore intersections of single width/height on the edge of the platform view.
713+
// This is to address the following performance issue when interleaving adjacent
714+
// platform views and layers:
715+
// Since we `roundOut` both platform view rects and the layer rects, as long as
716+
// the coordinate is fractional, there will be an intersection of a single pixel width
717+
// (or height) after rounding out, even if they do not intersect before rounding out.
718+
// We have to round out both platform view rect and the layer rect.
719+
// Rounding in platform view rect will result in missing pixel on the intersection edge.
720+
// Rounding in layer rect will result in missing pixel on the edge of the layer on top
721+
// of the platform view.
722+
for (auto it = intersection_rects.begin(); it != intersection_rects.end(); /*no-op*/) {
723+
// If intersection_rect does not intersect with the *rounded in* platform
724+
// view rect, then the intersection must be a single pixel width (or height) on edge.
725+
if (!SkIRect::Intersects(*it, rounded_in_platform_view_rect)) {
726+
it = intersection_rects.erase(it);
727+
} else {
728+
++it;
729+
}
730+
}
731+
710732
auto allocation_size = intersection_rects.size();
711733

712734
// For testing purposes, the overlay id is used to find the overlay view.

testing/scenario_app/ios/Scenarios/Scenarios/AppDelegate.m

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,10 @@ - (BOOL)application:(UIApplication*)application
5252
@"platform_view_one_overlay_two_intersecting_overlays",
5353
@"--platform-view-multiple-without-overlays" : @"platform_view_multiple_without_overlays",
5454
@"--platform-view-max-overlays" : @"platform_view_max_overlays",
55-
@"--platform-view-surrounding-layers" : @"platform_view_surrounding_layers",
55+
@"--platform-view-surrounding-layers-fractional-coordinate" :
56+
@"platform_view_surrounding_layers_fractional_coordinate",
57+
@"--platform-view-partial-intersection-fractional-coordinate" :
58+
@"platform_view_partial_intersection_fractional_coordinate",
5659
@"--platform-view-multiple" : @"platform_view_multiple",
5760
@"--platform-view-multiple-background-foreground" :
5861
@"platform_view_multiple_background_foreground",

testing/scenario_app/ios/Scenarios/ScenariosUITests/UnobstructedPlatformViewTests.m

Lines changed: 31 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -313,9 +313,9 @@ - (void)testPlatformViewsMaxOverlays {
313313
// +---+----+---+
314314
// | D |
315315
// +----+
316-
- (void)testPlatformViewsWithAdjacentSurroundingLayers {
316+
- (void)testPlatformViewsWithAdjacentSurroundingLayersAndFractionalCoordinate {
317317
XCUIApplication* app = [[XCUIApplication alloc] init];
318-
app.launchArguments = @[ @"--platform-view-surrounding-layers" ];
318+
app.launchArguments = @[ @"--platform-view-surrounding-layers-fractional-coordinate" ];
319319
[app launch];
320320

321321
XCUIElement* platform_view = app.otherElements[@"platform_view[0]"];
@@ -331,4 +331,33 @@ - (void)testPlatformViewsWithAdjacentSurroundingLayers {
331331
XCTAssertFalse(overlay.exists);
332332
}
333333

334+
// Platform view partially intersect with a layer in fractional coordinate.
335+
// +-------+
336+
// | |
337+
// | PV +--+--+
338+
// | | |
339+
// +----+ A |
340+
// | |
341+
// +-----+
342+
- (void)testPlatformViewsWithPartialIntersectionAndFractionalCoordinate {
343+
XCUIApplication* app = [[XCUIApplication alloc] init];
344+
app.launchArguments = @[ @"--platform-view-partial-intersection-fractional-coordinate" ];
345+
[app launch];
346+
347+
XCUIElement* platform_view = app.otherElements[@"platform_view[0]"];
348+
XCTAssertTrue([platform_view waitForExistenceWithTimeout:1.0]);
349+
350+
CGFloat scale = [UIScreen mainScreen].scale;
351+
XCTAssertEqual(platform_view.frame.origin.x * scale, 0.5);
352+
XCTAssertEqual(platform_view.frame.origin.y * scale, 0.5);
353+
XCTAssertEqual(platform_view.frame.size.width * scale, 100);
354+
XCTAssertEqual(platform_view.frame.size.height * scale, 100);
355+
356+
XCUIElement* overlay = app.otherElements[@"platform_view[0].overlay[0]"];
357+
XCTAssert(overlay.exists);
358+
359+
// We want to make sure the overlay covers the edge (which is at 100.5).
360+
XCTAssertEqual(CGRectGetMaxX(overlay.frame) * scale, 101);
361+
XCTAssertEqual(CGRectGetMaxY(overlay.frame) * scale, 101);
362+
}
334363
@end

testing/scenario_app/lib/src/platform_view.dart

Lines changed: 51 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -367,10 +367,10 @@ class PlatformViewMaxOverlaysScenario extends Scenario
367367
}
368368

369369
/// A platform view with adjacent surrounding layers should not create overlays.
370-
class PlatformViewSurroundingLayersScenario extends Scenario
370+
class PlatformViewSurroundingLayersFractionalCoordinateScenario extends Scenario
371371
with _BasePlatformViewScenarioMixin {
372372
/// Creates the PlatformView scenario.
373-
PlatformViewSurroundingLayersScenario(
373+
PlatformViewSurroundingLayersFractionalCoordinateScenario(
374374
super.view, {
375375
required this.id,
376376
});
@@ -438,6 +438,55 @@ class PlatformViewSurroundingLayersScenario extends Scenario
438438
}
439439
}
440440

441+
/// A platform view partially intersect with a layer, both with fractional coordinates.
442+
class PlatformViewPartialIntersectionFractionalCoordinateScenario extends Scenario
443+
with _BasePlatformViewScenarioMixin {
444+
/// Creates the PlatformView scenario.
445+
PlatformViewPartialIntersectionFractionalCoordinateScenario(
446+
super.view, {
447+
required this.id,
448+
});
449+
450+
/// The platform view identifier.
451+
final int id;
452+
453+
@override
454+
void onBeginFrame(Duration duration) {
455+
final SceneBuilder builder = SceneBuilder();
456+
457+
// Simulate partial pixel offsets as we would see while scrolling.
458+
// All objects in the scene below are then on sub-pixel boundaries.
459+
builder.pushOffset(0.5, 0.5);
460+
461+
// a platform view from (0, 0) to (100, 100)
462+
addPlatformView(
463+
id,
464+
width: 100,
465+
height: 100,
466+
dispatcher: view.platformDispatcher,
467+
sceneBuilder: builder,
468+
);
469+
470+
final PictureRecorder recorder = PictureRecorder();
471+
final Canvas canvas = Canvas(recorder);
472+
473+
canvas.drawRect(
474+
const Rect.fromLTWH(50, 50, 100, 100),
475+
Paint()..color = const Color(0x22FF0000),
476+
);
477+
478+
final Picture picture = recorder.endRecording();
479+
builder.addPicture(Offset.zero, picture);
480+
481+
// Pop the (0.5, 0.5) offset.
482+
builder.pop();
483+
484+
final Scene scene = builder.build();
485+
view.render(scene);
486+
scene.dispose();
487+
}
488+
}
489+
441490
/// Builds a scene with 2 platform views.
442491
class MultiPlatformViewScenario extends Scenario
443492
with _BasePlatformViewScenarioMixin {

testing/scenario_app/lib/src/scenarios.dart

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,8 @@ Map<String, _ScenarioFactory> _scenarios = <String, _ScenarioFactory>{
3434
'platform_view_one_overlay_two_intersecting_overlays': (FlutterView view) => PlatformViewOneOverlayTwoIntersectingOverlaysScenario(view, id: _viewId++),
3535
'platform_view_multiple_without_overlays': (FlutterView view) => MultiPlatformViewWithoutOverlaysScenario(view, firstId: _viewId++, secondId: _viewId++),
3636
'platform_view_max_overlays': (FlutterView view) => PlatformViewMaxOverlaysScenario(view, id: _viewId++),
37-
'platform_view_surrounding_layers': (FlutterView view) => PlatformViewSurroundingLayersScenario(view, id: _viewId++),
37+
'platform_view_surrounding_layers_fractional_coordinate': (FlutterView view) => PlatformViewSurroundingLayersFractionalCoordinateScenario(view, id: _viewId++),
38+
'platform_view_partial_intersection_fractional_coordinate': (FlutterView view) => PlatformViewPartialIntersectionFractionalCoordinateScenario(view, id: _viewId++),
3839
'platform_view_cliprect': (FlutterView view) => PlatformViewClipRectScenario(view, id: _viewId++),
3940
'platform_view_cliprect_with_transform': (FlutterView view) => PlatformViewClipRectWithTransformScenario(view, id: _viewId++),
4041
'platform_view_cliprect_after_moved': (FlutterView view) => PlatformViewClipRectAfterMovedScenario(view, id: _viewId++),

0 commit comments

Comments
 (0)