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

[ios_platform_view] MaskView pool to reuse maskViews. #38989

Merged
merged 6 commits into from
Jan 21, 2023
Merged
Show file tree
Hide file tree
Changes from 3 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
46 changes: 30 additions & 16 deletions shell/platform/darwin/ios/framework/Source/FlutterPlatformViews.mm
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@
#import "flutter/shell/platform/darwin/ios/framework/Source/FlutterViewController_Internal.h"
#import "flutter/shell/platform/darwin/ios/ios_surface.h"

static const NSUInteger kFlutterClippingMaskViewPoolCapacity = 5;

@implementation UIView (FirstResponder)
- (BOOL)flt_hasFirstResponderInViewHierarchySubtree {
if (self.isFirstResponder) {
Expand Down Expand Up @@ -451,6 +453,17 @@ static bool ClipRRectContainsPlatformViewBoundingRect(const SkRRect& clip_rrect,
return clipCount;
}

void FlutterPlatformViewsController::ClipViewAddMaskView(UIView* clipView) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

supernit: ClipViewSetMaskView?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

if (clipView.maskView) {
return;
}
UIView* flutterView = flutter_view_.get();
CGRect frame =
CGRectMake(-clipView.frame.origin.x, -clipView.frame.origin.y,
CGRectGetWidth(flutterView.bounds), CGRectGetHeight(flutterView.bounds));
clipView.maskView = [mask_view_pool_.get() getMaskViewWithFrame:frame];
}

void FlutterPlatformViewsController::ApplyMutators(const MutatorsStack& mutators_stack,
UIView* embedded_view,
const SkRect& bounding_rect) {
Expand All @@ -461,18 +474,15 @@ static bool ClipRRectContainsPlatformViewBoundingRect(const SkRRect& clip_rrect,
ResetAnchor(embedded_view.layer);
ChildClippingView* clipView = (ChildClippingView*)embedded_view.superview;

CGFloat screenScale = [UIScreen mainScreen].scale;

UIView* flutter_view = flutter_view_.get();
FlutterClippingMaskView* maskView = [[[FlutterClippingMaskView alloc]
initWithFrame:CGRectMake(-clipView.frame.origin.x, -clipView.frame.origin.y,
CGRectGetWidth(flutter_view.bounds),
CGRectGetHeight(flutter_view.bounds))
screenScale:screenScale] autorelease];

SkMatrix transformMatrix;
NSMutableArray* blurFilters = [[[NSMutableArray alloc] init] autorelease];

FML_DCHECK(!clipView.maskView ||
[clipView.maskView isKindOfClass:[FlutterClippingMaskView class]]);
if (mask_view_pool_.get() == nil) {
mask_view_pool_.reset([[FlutterClippingMaskViewPool alloc]
initWithCapacity:kFlutterClippingMaskViewPoolCapacity]);
}
[mask_view_pool_.get() recycleMaskViews];
clipView.maskView = nil;
auto iter = mutators_stack.Begin();
while (iter != mutators_stack.End()) {
Expand All @@ -486,25 +496,28 @@ static bool ClipRRectContainsPlatformViewBoundingRect(const SkRRect& clip_rrect,
transformMatrix)) {
break;
}
[maskView clipRect:(*iter)->GetRect() matrix:transformMatrix];
clipView.maskView = maskView;
ClipViewAddMaskView(clipView);
[(FlutterClippingMaskView*)clipView.maskView clipRect:(*iter)->GetRect()
matrix:transformMatrix];
break;
}
case kClipRRect: {
if (ClipRRectContainsPlatformViewBoundingRect((*iter)->GetRRect(), bounding_rect,
transformMatrix)) {
break;
}
[maskView clipRRect:(*iter)->GetRRect() matrix:transformMatrix];
clipView.maskView = maskView;
ClipViewAddMaskView(clipView);
[(FlutterClippingMaskView*)clipView.maskView clipRRect:(*iter)->GetRRect()
matrix:transformMatrix];
break;
}
case kClipPath: {
// TODO(cyanglaz): Find a way to pre-determine if path contains the PlatformView boudning
// rect. See `ClipRRectContainsPlatformViewBoundingRect`.
// https://github.com/flutter/flutter/issues/118650
[maskView clipPath:(*iter)->GetPath() matrix:transformMatrix];
clipView.maskView = maskView;
ClipViewAddMaskView(clipView);
[(FlutterClippingMaskView*)clipView.maskView clipPath:(*iter)->GetPath()
matrix:transformMatrix];
break;
}
case kOpacity:
Expand Down Expand Up @@ -551,6 +564,7 @@ static bool ClipRRectContainsPlatformViewBoundingRect(const SkRRect& clip_rrect,
[clipView applyBlurBackdropFilters:blurFilters];
}

CGFloat screenScale = [UIScreen mainScreen].scale;
// The UIKit frame is set based on the logical resolution (points) instead of physical.
// (https://developer.apple.com/library/archive/documentation/DeviceInformation/Reference/iOSDeviceCompatibility/Displays/Displays.html).
// However, flow is based on the physical resolution. For example, 1000 pixels in flow equals
Expand Down
105 changes: 105 additions & 0 deletions shell/platform/darwin/ios/framework/Source/FlutterPlatformViewsTest.mm
Original file line number Diff line number Diff line change
Expand Up @@ -2445,6 +2445,111 @@ - (void)testHasFirstResponderInViewHierarchySubtree_descendantViewBecomesFirstRe
XCTAssertFalse(view.flt_hasFirstResponderInViewHierarchySubtree);
}

- (void)testFlutterClippingMaskViewPoolReuseViewsAfterRecycle {
FlutterClippingMaskViewPool* pool =
[[[FlutterClippingMaskViewPool alloc] initWithCapacity:2] autorelease];
FlutterClippingMaskView* view1 = [pool getMaskViewWithFrame:CGRectZero];
FlutterClippingMaskView* view2 = [pool getMaskViewWithFrame:CGRectZero];
[pool recycleMaskViews];
CGRect newRect = CGRectMake(0, 0, 10, 10);
FlutterClippingMaskView* view3 = [pool getMaskViewWithFrame:newRect];
FlutterClippingMaskView* view4 = [pool getMaskViewWithFrame:newRect];
XCTAssertEqual(view1, view3);
XCTAssertEqual(view2, view4);
XCTAssertTrue(CGRectEqualToRect(view3.frame, newRect));
XCTAssertTrue(CGRectEqualToRect(view4.frame, newRect));
}

- (void)testFlutterClippingMaskViewPoolAllocsNewMaskViewsAfterReachingCapacity {
FlutterClippingMaskViewPool* pool =
[[[FlutterClippingMaskViewPool alloc] initWithCapacity:2] autorelease];
FlutterClippingMaskView* view1 = [pool getMaskViewWithFrame:CGRectZero];
FlutterClippingMaskView* view2 = [pool getMaskViewWithFrame:CGRectZero];
FlutterClippingMaskView* view3 = [pool getMaskViewWithFrame:CGRectZero];
XCTAssertNotEqual(view1, view3);
XCTAssertNotEqual(view2, view3);
}

- (void)testClipMaskViewIsReused {
flutter::FlutterPlatformViewsTestMockPlatformViewDelegate mock_delegate;
auto thread_task_runner = CreateNewThread("FlutterPlatformViewsTest");
flutter::TaskRunners runners(/*label=*/self.name.UTF8String,
/*platform=*/thread_task_runner,
/*raster=*/thread_task_runner,
/*ui=*/thread_task_runner,
/*io=*/thread_task_runner);
auto flutterPlatformViewsController = std::make_shared<flutter::FlutterPlatformViewsController>();
auto platform_view = std::make_unique<flutter::PlatformViewIOS>(
/*delegate=*/mock_delegate,
/*rendering_api=*/flutter::IOSRenderingAPI::kSoftware,
/*platform_views_controller=*/flutterPlatformViewsController,
/*task_runners=*/runners);

FlutterPlatformViewsTestMockFlutterPlatformFactory* factory =
[[FlutterPlatformViewsTestMockFlutterPlatformFactory new] autorelease];
flutterPlatformViewsController->RegisterViewFactory(
factory, @"MockFlutterPlatformView",
FlutterPlatformViewGestureRecognizersBlockingPolicyEager);
FlutterResult result = ^(id result) {
};
flutterPlatformViewsController->OnMethodCall(
[FlutterMethodCall
methodCallWithMethodName:@"create"
arguments:@{@"id" : @1, @"viewType" : @"MockFlutterPlatformView"}],
result);

XCTAssertNotNil(gMockPlatformView);
UIView* mockFlutterView = [[[UIView alloc] initWithFrame:CGRectMake(0, 0, 10, 10)] autorelease];
flutterPlatformViewsController->SetFlutterView(mockFlutterView);
// Create embedded view params
flutter::MutatorsStack stack1;
// Layer tree always pushes a screen scale factor to the stack
SkMatrix screenScaleMatrix =
SkMatrix::Scale([UIScreen mainScreen].scale, [UIScreen mainScreen].scale);
stack1.PushTransform(screenScaleMatrix);
// Push a clip rect
SkRect rect = SkRect::MakeXYWH(2, 2, 3, 3);
stack1.PushClipRect(rect);

auto embeddedViewParams1 = std::make_unique<flutter::EmbeddedViewParams>(
screenScaleMatrix, SkSize::Make(10, 10), stack1);

flutter::MutatorsStack stack2;
auto embeddedViewParams2 = std::make_unique<flutter::EmbeddedViewParams>(
screenScaleMatrix, SkSize::Make(10, 10), stack2);

flutterPlatformViewsController->PrerollCompositeEmbeddedView(1, std::move(embeddedViewParams1));
flutterPlatformViewsController->CompositeEmbeddedView(1);
UIView* childClippingView1 = gMockPlatformView.superview.superview;
UIView* maskView1 = childClippingView1.maskView;
XCTAssertNotNil(maskView1);

// Composite a new frame.
auto embeddedViewParams3 = std::make_unique<flutter::EmbeddedViewParams>(
screenScaleMatrix, SkSize::Make(10, 10), stack2);
flutterPlatformViewsController->PrerollCompositeEmbeddedView(1, std::move(embeddedViewParams3));
flutterPlatformViewsController->CompositeEmbeddedView(1);
childClippingView1 = gMockPlatformView.superview.superview;

// This overrides gMockPlatformView to point to the newly created platform view.
flutterPlatformViewsController->OnMethodCall(
[FlutterMethodCall
methodCallWithMethodName:@"create"
arguments:@{@"id" : @2, @"viewType" : @"MockFlutterPlatformView"}],
result);

auto embeddedViewParams4 = std::make_unique<flutter::EmbeddedViewParams>(
screenScaleMatrix, SkSize::Make(10, 10), stack1);
flutterPlatformViewsController->PrerollCompositeEmbeddedView(2, std::move(embeddedViewParams4));
flutterPlatformViewsController->CompositeEmbeddedView(2);
UIView* childClippingView2 = gMockPlatformView.superview.superview;

UIView* maskView2 = childClippingView2.maskView;
XCTAssertEqual(maskView1, maskView2);
XCTAssertNotNil(childClippingView2.maskView);
XCTAssertNil(childClippingView1.maskView);
}

// Return true if a correct visual effect view is found. It also implies all the validation in this
// method passes.
//
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@

- (instancetype)initWithFrame:(CGRect)frame screenScale:(CGFloat)screenScale;

- (void)reset;

// Adds a clip rect operation to the queue.
//
// The `clipSkRect` is transformed with the `matrix` before adding to the queue.
Expand All @@ -47,6 +49,28 @@

@end

// A pool that provides |FlutterClippingMaskView|s.
//
// The pool has a capacity that can be set in the initializer.
// When requesting a FlutterClippingMaskView, the pool will first try to reuse an available maskView
// in the pool. If there are none available, a new FlutterClippingMaskView is constructed. If the
// capacity is reached, the newly constructed FlutterClippingMaskView is not added to the pool.
//
// Call |recycleMaskViews| to mark all the FlutterClippingMaskViews in the pool available.
@interface FlutterClippingMaskViewPool : NSObject

// Initialize the pool with `capacity`. When the `capacity` is reached, a FlutterClippingMaskView is
// constructed when requested, and it is not added to the pool.
- (instancetype)initWithCapacity:(NSInteger)capacity;

// Reuse a maskView from the pool, or allocate a new one.
- (FlutterClippingMaskView*)getMaskViewWithFrame:(CGRect)frame;

// Mark all the maskViews available.
- (void)recycleMaskViews;

@end

// An object represents a blur filter.
//
// This object produces a `backdropFilterView`.
Expand Down Expand Up @@ -268,6 +292,7 @@ class FlutterPlatformViewsController {
// Traverse the `mutators_stack` and return the number of clip operations.
int CountClips(const MutatorsStack& mutators_stack);

void ClipViewAddMaskView(UIView* clipView);
// Applies the mutators in the mutators_stack to the UIView chain that was constructed by
// `ReconstructClipViewsChain`
//
Expand Down Expand Up @@ -328,6 +353,7 @@ class FlutterPlatformViewsController {
fml::scoped_nsobject<FlutterMethodChannel> channel_;
fml::scoped_nsobject<UIView> flutter_view_;
fml::scoped_nsobject<UIViewController> flutter_view_controller_;
fml::scoped_nsobject<FlutterClippingMaskViewPool> mask_view_pool_;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason to make this a scoped_nsobject?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I should change it to properties. I was blindly following the current pattern.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, actually, it is in a c++ class. So I think scoped_nsobject is better that the lifecycle is automatically managed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which class is c++? Your new FlutterClippingMaskViewPool is a Obj-C class.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FlutterPlatformViewsController is the c++ class.

std::map<std::string, fml::scoped_nsobject<NSObject<FlutterPlatformViewFactory>>> factories_;
std::map<int64_t, fml::scoped_nsobject<NSObject<FlutterPlatformView>>> views_;
std::map<int64_t, fml::scoped_nsobject<FlutterTouchInterceptingView>> touch_interceptors_;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -271,6 +271,10 @@ - (instancetype)initWithFrame:(CGRect)frame screenScale:(CGFloat)screenScale {
return self;
}

- (void)reset {
paths_.clear();
}

// In some scenarios, when we add this view as a maskView of the ChildClippingView, iOS added
// this view as a subview of the ChildClippingView.
// This results this view blocking touch events on the ChildClippingView.
Expand Down Expand Up @@ -447,3 +451,64 @@ - (void)clipPath:(const SkPath&)path matrix:(const SkMatrix&)matrix {
}

@end

@interface FlutterClippingMaskViewPool ()

@property(assign, nonatomic) NSUInteger capacity;
@property(retain, nonatomic) NSMutableArray<FlutterClippingMaskView*>* pool;
Copy link
Member

@jmagman jmagman Jan 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this retain the FlutterClippingMaskView even after the ChildClippingView is dealloc'd?
Would NSPointerArray with NSPointerFunctionsWeakMemory (I think zeroes it out with MRC but worth testing) work better here? If you could reliably know the mask views in the pool are used (because they are retained by a ChildClippingView), then you could get rid of recycleMaskViews and during insert compact, then insert at count if there was capacity.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well never mind, retaining it after the ChildClippingView is dealloc'd is the whole point of this PR...
Can you confirm that deallocating FlutterPlatformViewsController releases the pool and the mask views?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will add a test for it!

// The index points to the first available FlutterClippingMaskView in the `pool`.
@property(assign, nonatomic) NSUInteger availableIndex;

@end

@implementation FlutterClippingMaskViewPool : NSObject

- (instancetype)initWithCapacity:(NSInteger)capacity {
if (self = [super init]) {
_pool = [[NSMutableArray alloc] initWithCapacity:capacity];
_capacity = capacity;
_availableIndex = 0;
}
return self;
}

- (FlutterClippingMaskView*)getMaskViewWithFrame:(CGRect)frame {
FML_DCHECK(self.availableIndex <= self.capacity);
FlutterClippingMaskView* maskView;
if (self.availableIndex == self.capacity) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it looks like the capacity is not updated? should it be increased when you add a new one?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can probably just compare with pool.count, and do not keep this capacity ivar?

Copy link
Contributor Author

@cyanglaz cyanglaz Jan 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The capacity is a static limit set for the pool. My intention is to limit the memory used by the pool. For example, (it is probably not possible in real life), I don't want the pool to cache 20 views for the whole application session.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gotcha. I mistakenly thought that this capacity increases like an NSMutableArray.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yeah it could be confusing, I added a comment.

// The pool is full, alloc a new one.
maskView =
[[[FlutterClippingMaskView alloc] initWithFrame:frame
screenScale:[UIScreen mainScreen].scale] autorelease];
return maskView;
}

if (self.availableIndex >= self.pool.count) {
// The pool doesn't have enough maskViews, alloc a new one and add to the pool.
maskView =
[[[FlutterClippingMaskView alloc] initWithFrame:frame
screenScale:[UIScreen mainScreen].scale] autorelease];
[self.pool addObject:maskView];
FML_DCHECK(self.pool.count <= self.capacity);
} else {
// Reuse a maskView from the pool.
maskView = [self.pool objectAtIndex:self.availableIndex];
maskView.frame = frame;
[maskView reset];
}
self.availableIndex++;
return maskView;
}

- (void)recycleMaskViews {
self.availableIndex = 0;
}

- (void)dealloc {
[_pool release];
_pool = nil;

[super dealloc];
}

@end