Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions flow/embedded_views.h
Original file line number Diff line number Diff line change
Expand Up @@ -339,10 +339,31 @@ 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 {
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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...

Copy link
Contributor Author

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.

Copy link
Contributor

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.

// 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
Original file line number Diff line number Diff line change
Expand Up @@ -705,7 +705,8 @@ 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->region(platform_view_rect).getRects();
std::vector<SkIRect> intersection_rects =
slice->roundedInRegion(platform_view_rect).getRects();
auto allocation_size = intersection_rects.size();

// For testing purposes, the overlay id is used to find the overlay view.
Expand Down
1 change: 1 addition & 0 deletions testing/scenario_app/ios/Scenarios/Scenarios/AppDelegate.m
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ - (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-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 @@ -305,4 +305,30 @@ - (void)testPlatformViewsMaxOverlays {
XCTAssertFalse(overlayView1.exists);
}

// Platform view surrounded by adjacent layers on each side should not create any overlays.
// +----+
// | B |
// +---+----+---+
// | A | PV | C |
// +---+----+---+
// | D |
// +----+
- (void)testPlatformViewsWithAdjacentSurroundingLayers {
XCUIApplication* app = [[XCUIApplication alloc] init];
app.launchArguments = @[ @"--platform-view-surrounding-layers" ];
[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, 100.5);
XCTAssertEqual(platform_view.frame.origin.y * scale, 100.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]"];
XCTAssertFalse(overlay.exists);
}

@end
72 changes: 72 additions & 0 deletions testing/scenario_app/lib/src/platform_view.dart
Original file line number Diff line number Diff line change
Expand Up @@ -366,6 +366,78 @@ class PlatformViewMaxOverlaysScenario extends Scenario
}
}

/// A platform view with adjacent surrounding layers should not create overlays.
class PlatformViewSurroundingLayersScenario extends Scenario
with _BasePlatformViewScenarioMixin {
/// Creates the PlatformView scenario.
PlatformViewSurroundingLayersScenario(
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 (100, 100) to (200, 200)
builder.pushOffset(100, 100);
addPlatformView(
id,
width: 100,
height: 100,
dispatcher: view.platformDispatcher,
sceneBuilder: builder,
);
builder.pop();

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

const Rect rect = Rect.fromLTWH(100, 100, 100, 100);

// Rect at the left of platform view
canvas.drawRect(
rect.shift(const Offset(-100, 0)),
Paint()..color = const Color(0x22FF0000),
);

// Rect at the right of platform view
canvas.drawRect(
rect.shift(const Offset(100, 0)),
Paint()..color = const Color(0x22FF0000),
);

// Rect at the top of platform view
canvas.drawRect(
rect.shift(const Offset(0, -100)),
Paint()..color = const Color(0x22FF0000),
);

// Rect at the bottom of platform view
canvas.drawRect(
rect.shift(const Offset(0, 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
1 change: 1 addition & 0 deletions testing/scenario_app/lib/src/scenarios.dart
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ 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_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