Skip to content

Commit

Permalink
[ios]ignore single edge pixel instead of rounding (flutter#51687)
Browse files Browse the repository at this point in the history
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
  • Loading branch information
hellohuanlin authored Mar 27, 2024
1 parent 00dab0d commit 01d42ad
Show file tree
Hide file tree
Showing 6 changed files with 112 additions and 29 deletions.
21 changes: 0 additions & 21 deletions flow/embedded_views.h
Original file line number Diff line number Diff line change
Expand Up @@ -339,31 +339,10 @@ class EmbedderViewSlice {
virtual DlCanvas* canvas() = 0;
virtual void end_recording() = 0;
virtual const DlRegion& getRegion() const = 0;
// TODO(hellohuanlin): We should deprecate this function if we migrate
// all platforms to use `roundedInRegion`. Then we should rename
// `roundedInRegion` to just `region`.
DlRegion region(const SkRect& query) const {
return DlRegion::MakeIntersection(getRegion(), DlRegion(query.roundOut()));
}

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

virtual void render_into(DlCanvas* canvas) = 0;
};

Expand Down
26 changes: 24 additions & 2 deletions shell/platform/darwin/ios/framework/Source/FlutterPlatformViews.mm
Original file line number Diff line number Diff line change
Expand Up @@ -695,6 +695,8 @@ static bool ClipRRectContainsPlatformViewBoundingRect(const SkRRect& clip_rrect,
auto did_submit = true;
auto num_platform_views = composition_order_.size();

// TODO(hellohuanlin) this double for-loop is expensive with wasted computations.
// See: https://github.com/flutter/flutter/issues/145802
for (size_t i = 0; i < num_platform_views; i++) {
int64_t platform_view_id = composition_order_[i];
EmbedderViewSlice* slice = slices_[platform_view_id].get();
Expand All @@ -705,8 +707,28 @@ static bool ClipRRectContainsPlatformViewBoundingRect(const SkRRect& clip_rrect,
for (size_t j = i + 1; j > 0; j--) {
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->roundedInRegion(platform_view_rect).getRects();
std::vector<SkIRect> intersection_rects = slice->region(platform_view_rect).getRects();
const SkIRect rounded_in_platform_view_rect = platform_view_rect.roundIn();
// Ignore intersections of single width/height on the edge of the platform view.
// This is to address the following performance issue when interleaving adjacent
// platform views and layers:
// Since we `roundOut` both platform view rects and the layer rects, as long as
// the coordinate is fractional, there will be an intersection of a single pixel width
// (or height) after rounding out, even if they do not intersect before rounding out.
// We have to round out both platform view rect and the layer rect.
// Rounding in platform view rect will result in missing pixel on the intersection edge.
// Rounding in layer rect will result in missing pixel on the edge of the layer on top
// of the platform view.
for (auto it = intersection_rects.begin(); it != intersection_rects.end(); /*no-op*/) {
// If intersection_rect does not intersect with the *rounded in* platform
// view rect, then the intersection must be a single pixel width (or height) on edge.
if (!SkIRect::Intersects(*it, rounded_in_platform_view_rect)) {
it = intersection_rects.erase(it);
} else {
++it;
}
}

auto allocation_size = intersection_rects.size();

// For testing purposes, the overlay id is used to find the overlay view.
Expand Down
5 changes: 4 additions & 1 deletion testing/scenario_app/ios/Scenarios/Scenarios/AppDelegate.m
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,10 @@ - (BOOL)application:(UIApplication*)application
@"platform_view_one_overlay_two_intersecting_overlays",
@"--platform-view-multiple-without-overlays" : @"platform_view_multiple_without_overlays",
@"--platform-view-max-overlays" : @"platform_view_max_overlays",
@"--platform-view-surrounding-layers" : @"platform_view_surrounding_layers",
@"--platform-view-surrounding-layers-fractional-coordinate" :
@"platform_view_surrounding_layers_fractional_coordinate",
@"--platform-view-partial-intersection-fractional-coordinate" :
@"platform_view_partial_intersection_fractional_coordinate",
@"--platform-view-multiple" : @"platform_view_multiple",
@"--platform-view-multiple-background-foreground" :
@"platform_view_multiple_background_foreground",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -313,9 +313,9 @@ - (void)testPlatformViewsMaxOverlays {
// +---+----+---+
// | D |
// +----+
- (void)testPlatformViewsWithAdjacentSurroundingLayers {
- (void)testPlatformViewsWithAdjacentSurroundingLayersAndFractionalCoordinate {
XCUIApplication* app = [[XCUIApplication alloc] init];
app.launchArguments = @[ @"--platform-view-surrounding-layers" ];
app.launchArguments = @[ @"--platform-view-surrounding-layers-fractional-coordinate" ];
[app launch];

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

// Platform view partially intersect with a layer in fractional coordinate.
// +-------+
// | |
// | PV +--+--+
// | | |
// +----+ A |
// | |
// +-----+
- (void)testPlatformViewsWithPartialIntersectionAndFractionalCoordinate {
XCUIApplication* app = [[XCUIApplication alloc] init];
app.launchArguments = @[ @"--platform-view-partial-intersection-fractional-coordinate" ];
[app launch];

XCUIElement* platform_view = app.otherElements[@"platform_view[0]"];
XCTAssertTrue([platform_view waitForExistenceWithTimeout:1.0]);

CGFloat scale = [UIScreen mainScreen].scale;
XCTAssertEqual(platform_view.frame.origin.x * scale, 0.5);
XCTAssertEqual(platform_view.frame.origin.y * scale, 0.5);
XCTAssertEqual(platform_view.frame.size.width * scale, 100);
XCTAssertEqual(platform_view.frame.size.height * scale, 100);

XCUIElement* overlay = app.otherElements[@"platform_view[0].overlay[0]"];
XCTAssert(overlay.exists);

// We want to make sure the overlay covers the edge (which is at 100.5).
XCTAssertEqual(CGRectGetMaxX(overlay.frame) * scale, 101);
XCTAssertEqual(CGRectGetMaxY(overlay.frame) * scale, 101);
}
@end
53 changes: 51 additions & 2 deletions testing/scenario_app/lib/src/platform_view.dart
Original file line number Diff line number Diff line change
Expand Up @@ -367,10 +367,10 @@ class PlatformViewMaxOverlaysScenario extends Scenario
}

/// A platform view with adjacent surrounding layers should not create overlays.
class PlatformViewSurroundingLayersScenario extends Scenario
class PlatformViewSurroundingLayersFractionalCoordinateScenario extends Scenario
with _BasePlatformViewScenarioMixin {
/// Creates the PlatformView scenario.
PlatformViewSurroundingLayersScenario(
PlatformViewSurroundingLayersFractionalCoordinateScenario(
super.view, {
required this.id,
});
Expand Down Expand Up @@ -438,6 +438,55 @@ class PlatformViewSurroundingLayersScenario extends Scenario
}
}

/// A platform view partially intersect with a layer, both with fractional coordinates.
class PlatformViewPartialIntersectionFractionalCoordinateScenario extends Scenario
with _BasePlatformViewScenarioMixin {
/// Creates the PlatformView scenario.
PlatformViewPartialIntersectionFractionalCoordinateScenario(
super.view, {
required this.id,
});

/// The platform view identifier.
final int id;

@override
void onBeginFrame(Duration duration) {
final SceneBuilder builder = SceneBuilder();

// Simulate partial pixel offsets as we would see while scrolling.
// All objects in the scene below are then on sub-pixel boundaries.
builder.pushOffset(0.5, 0.5);

// a platform view from (0, 0) to (100, 100)
addPlatformView(
id,
width: 100,
height: 100,
dispatcher: view.platformDispatcher,
sceneBuilder: builder,
);

final PictureRecorder recorder = PictureRecorder();
final Canvas canvas = Canvas(recorder);

canvas.drawRect(
const Rect.fromLTWH(50, 50, 100, 100),
Paint()..color = const Color(0x22FF0000),
);

final Picture picture = recorder.endRecording();
builder.addPicture(Offset.zero, picture);

// Pop the (0.5, 0.5) offset.
builder.pop();

final Scene scene = builder.build();
view.render(scene);
scene.dispose();
}
}

/// Builds a scene with 2 platform views.
class MultiPlatformViewScenario extends Scenario
with _BasePlatformViewScenarioMixin {
Expand Down
3 changes: 2 additions & 1 deletion testing/scenario_app/lib/src/scenarios.dart
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,8 @@ Map<String, _ScenarioFactory> _scenarios = <String, _ScenarioFactory>{
'platform_view_one_overlay_two_intersecting_overlays': (FlutterView view) => PlatformViewOneOverlayTwoIntersectingOverlaysScenario(view, id: _viewId++),
'platform_view_multiple_without_overlays': (FlutterView view) => MultiPlatformViewWithoutOverlaysScenario(view, firstId: _viewId++, secondId: _viewId++),
'platform_view_max_overlays': (FlutterView view) => PlatformViewMaxOverlaysScenario(view, id: _viewId++),
'platform_view_surrounding_layers': (FlutterView view) => PlatformViewSurroundingLayersScenario(view, id: _viewId++),
'platform_view_surrounding_layers_fractional_coordinate': (FlutterView view) => PlatformViewSurroundingLayersFractionalCoordinateScenario(view, id: _viewId++),
'platform_view_partial_intersection_fractional_coordinate': (FlutterView view) => PlatformViewPartialIntersectionFractionalCoordinateScenario(view, id: _viewId++),
'platform_view_cliprect': (FlutterView view) => PlatformViewClipRectScenario(view, id: _viewId++),
'platform_view_cliprect_with_transform': (FlutterView view) => PlatformViewClipRectWithTransformScenario(view, id: _viewId++),
'platform_view_cliprect_after_moved': (FlutterView view) => PlatformViewClipRectAfterMovedScenario(view, id: _viewId++),
Expand Down

0 comments on commit 01d42ad

Please sign in to comment.