IOS Platform view transform/clipping#9075
Conversation
amirh
left a comment
There was a problem hiding this comment.
First pass, still need to review the ObjC++ code more thoroughly.
flow/embedded_views.h
Outdated
|
|
||
| FML_DISALLOW_COPY_AND_ASSIGN(ExternalViewEmbedder); | ||
|
|
||
| #pragma mark - transforms |
There was a problem hiding this comment.
I believe we don't do these in our C++ code
flow/embedded_views.h
Outdated
|
|
||
| }; // ExternalViewEmbedder | ||
|
|
||
| class FlutterEmbededViewTransformStack { |
There was a problem hiding this comment.
We can probably omit the "Flutter" prefix from these classes.
| #include "flutter/fml/platform/darwin/scoped_nsobject.h" | ||
| #include "flutter/shell/platform/darwin/ios/framework/Headers/FlutterChannels.h" | ||
|
|
||
| #pragma mark - Transforms Utils |
There was a problem hiding this comment.
nit: lets split these to a separate file, this one is getting too long
There was a problem hiding this comment.
Should I just add these functions into the flutter namespace?
| case SkRRect::kNinePatch_Type: | ||
| case SkRRect::kComplex_Type: { | ||
| CGMutablePathRef mutablePathRef = CGPathCreateMutable(); | ||
| // Complex types, we manually add each cornor. |
There was a problem hiding this comment.
s/cornor/corner/ (here and below)
flow/layers/clip_rect_layer.cc
Outdated
| SkAutoCanvasRestore save(context.internal_nodes_canvas, true); | ||
| context.internal_nodes_canvas->clipRect(clip_rect_, | ||
| clip_behavior_ != Clip::hardEdge); | ||
| context.view_embedder->transformStack->pushClipRect(clip_rect_); |
There was a problem hiding this comment.
view_embedder can be null, protect against it in all call sites.
flow/embedded_views.h
Outdated
| public: | ||
| SkPoint offsetPixels; | ||
| SkSize sizePoints; | ||
| std::shared_ptr<FlutterEmbededViewTransformStack> transformStack; |
There was a problem hiding this comment.
Both EmbededViewParams and ExternalViewEmbedder holds a reference to this object. I might be wrong but I thought I had to use shared for it?
flow/embedded_views.h
Outdated
| FML_DISALLOW_COPY_AND_ASSIGN(ExternalViewEmbedder); | ||
|
|
||
| #pragma mark - transforms | ||
| std::shared_ptr<FlutterEmbededViewTransformStack> transformStack; |
There was a problem hiding this comment.
This is the state of an ongoing paint traversal, I do not think it belongs in ExternalViewEmbedder(e.g think of an hypothetical world where we'll be painting multiple parts of the tree in parallel). I'd keep this in PaintContext.
There was a problem hiding this comment.
Sure. Do you think all the stack related classes belong to the same file of paintContext?
flow/embedded_views.h
Outdated
|
|
||
| }; // ExternalViewEmbedder | ||
|
|
||
| class FlutterEmbededViewTransformStack { |
There was a problem hiding this comment.
Our language is a little ambiguous around "transforms", e.g usually we mean the transform matrix when we say transforms. I don't have a good idea though.
Update: I asked @chinmaygarde and he suggested to call it mutators, I think it's a much more reasonable name.
flow/embedded_views.h
Outdated
| // and destroys it. | ||
| void pop(); | ||
|
|
||
| // Returns the iterator points to the bottom of the stack. |
There was a problem hiding this comment.
Just from reading this comment I'm not sure what's the "bottom of the stack" the mutator that should apply to all other mutators in the stack, or the one that all other mutators applies to? can you make this more clear in the comment ?(for both iterators)
There was a problem hiding this comment.
Done. Updated to
// Returns the iterator points to the bottom of the stack.
// When we composite an embedded view, this is the first mutator we should apply to the view.
// And we should iterate through all the mutators until we reach `rend()` and apply all the mutations to the view along the way.
Let me know what you think.
There was a problem hiding this comment.
Suggestion:
// A stack of mutators that can be applied to an embedded platform view.
//
// The stack may include mutators like transforms and clips, each mutator applies to all the mutators that are below it in the stack
// and to the embedded view.
//
// For example consider the following stack: [T1, T2, T3], where T1 is the top of the stack and T3 is the bottom of the stack.
// Applying this mutators stack to a platform view P1 will result in T1(T2(T2(P1))).
class MutatorsStack {
...
// Returns an iterator pointing to the top of the stack.
std::vector<EmbeddedViewMutator>::reverse_iterator top();
// Returns an iterator pointing to the bottom of the stack.
std::vector<EmbeddedViewMutator>::reverse_iterator bottom();
}
flow/embedded_views.h
Outdated
|
|
||
| }; // ExternalViewEmbedder | ||
|
|
||
| class FlutterEmbededViewTransformStack { |
There was a problem hiding this comment.
Add unit tests for this class.
flow/embedded_views.h
Outdated
| // and destroys it. | ||
| void pop(); | ||
|
|
||
| // Returns the iterator points to the bottom of the stack. |
There was a problem hiding this comment.
Suggestion:
// A stack of mutators that can be applied to an embedded platform view.
//
// The stack may include mutators like transforms and clips, each mutator applies to all the mutators that are below it in the stack
// and to the embedded view.
//
// For example consider the following stack: [T1, T2, T3], where T1 is the top of the stack and T3 is the bottom of the stack.
// Applying this mutators stack to a platform view P1 will result in T1(T2(T2(P1))).
class MutatorsStack {
...
// Returns an iterator pointing to the top of the stack.
std::vector<EmbeddedViewMutator>::reverse_iterator top();
// Returns an iterator pointing to the bottom of the stack.
std::vector<EmbeddedViewMutator>::reverse_iterator bottom();
}
flow/embedded_views.cc
Outdated
|
|
||
| namespace flutter { | ||
|
|
||
| ExternalViewEmbedder::ExternalViewEmbedder() |
flow/embedded_views.h
Outdated
| class ExternalViewEmbedder { | ||
| public: | ||
| ExternalViewEmbedder() = default; | ||
| ExternalViewEmbedder(); |
flow/embedded_views.h
Outdated
| FML_DISALLOW_COPY_AND_ASSIGN(ExternalViewEmbedder); | ||
| }; | ||
|
|
||
| std::shared_ptr<EmbeddedViewMutatorStack> transformStack; |
| EmbeddedViewMutator element = EmbeddedViewMutator(); | ||
| element.setType(clip_rect); | ||
| element.setRect(rect); | ||
| vector_.push_back(element); |
There was a problem hiding this comment.
nit: we can save some copies by making it a vector of unique pointers
There was a problem hiding this comment.
I tried it naively and caused some issue when trying to copy the stack to the params, reverting the change and adding a TODO and will work on it in future.
| 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_; | ||
| std::map<int64_t, fml::scoped_nsobject<UIView>> root_views_; |
There was a problem hiding this comment.
Can you add a comment explaining this and clip_count_?
|
|
||
| CGRect frame = CGRectMake(0, 0, params.sizePoints.width(), params.sizePoints.height()); | ||
| touch_interceptor.frame = frame; | ||
| UIView* lastView = touch_interceptor; |
There was a problem hiding this comment.
It wasn't immediately clear to me what's lastView, the logic here is not completely trivial, maybe we can tell the story better by being a little more verbose on comment and names, maybe something like(pseudo code):
// Build a chain of UIViews that applies the mutations described by mutatorsStack.
//
// Clips are applied by adding a super view with the CALayer mask. Transforms are applied to the view that's at the
// head of the chain. For example the following mutators stack [T_1, C_2, T_3, T_4, C_5, T_6] where T denotes a
// transform and C denotes a clip, will result in the following UIView tree:
//
// C_2 -> C_5 -> PLATFORM_VIEW
// (PLATFORM_VIEW is a subview of C_5 which is a subview of C_2)
//
// T_1 is applied to C_2, T_3 and T_4 are applied to C_5, and T_6 is applied to PLATFORM_VIEW.
//
// UIView instances used for clip mutations are re-used when possible. [ADD AN EXAMPLE HERE]
//
// Returns the UIView that's at the top of the generated chain(C_2 for the example above).
UIView* FlutterPlatformViewsController::ApplyMutators(ViewMutatorsStack& mutators_stack, UIView* embedded_view) {
UIView* head = embedded_view; // [instead of the current variable named lastView]
...
}|
|
||
| // If we have less cilp operations this time, remove unnecessary views. | ||
| // We skip this process if we have more clip operations this time. | ||
| int64_t extraClipsFromLastTime = clip_count_[view_id] - clipCount; |
There was a problem hiding this comment.
I'd keep this part in the proposed ApplyMutators
| // If we have less cilp operations this time, remove unnecessary views. | ||
| // We skip this process if we have more clip operations this time. | ||
| int64_t extraClipsFromLastTime = clip_count_[view_id] - clipCount; | ||
| if (extraClipsFromLastTime > 0 && lastView.superview) { |
There was a problem hiding this comment.
Maybe something like the following will look simpler:
if (!hasExtraClipViews) {
return head;
}
[head removeFromSuperView];
[root_views_[view_id] removeFromSuperView];
[root_views_[view_id] release];
[root_views_[view_id] = [head retain];
// here add head to flutter_view_
| [superview release]; | ||
| superview = superSuperView; | ||
| } | ||
| [flutter_view_.get() addSubview:lastView]; |
There was a problem hiding this comment.
I think this might change the composition order in an unwanted way when we have multiple platform views, e.g [P_1, P_2] (in this order), and now the clip chain for P_1 changes, we remove the chain for P_1 and add it back to flutter_view_ which results in flipping the order([P_2, P]).
| } | ||
|
|
||
| int FlutterPlatformViewsController::GetNumberOfClips(const MutatorsStack& mutators_stack) { | ||
| // Apply transforms/clips. |
| UIView* FlutterPlatformViewsController::ReconstructClipViewsChain(int number_of_clips, | ||
| UIView* child, | ||
| UIView* parent) { | ||
| NSInteger index = -1; |
There was a problem hiding this comment.
nit: maybe call this indexInFlutterView?
| if (!needAddNewView && head != parent) { | ||
| [head removeFromSuperview]; | ||
| } | ||
| if (index > -1) { |
There was a problem hiding this comment.
nit: add a comment saying a value of -1 means that the chain was not attached to the FlutterView
| UIView* parent) { | ||
| NSInteger index = -1; | ||
| if (parent.superview) { | ||
| // TODO(cyanglaz): protentially cache the index of oldPlatformViewRoot to make this a O(1). |
There was a problem hiding this comment.
File an issue so we don't lose track of this.
| return clipCount; | ||
| } | ||
|
|
||
| UIView* FlutterPlatformViewsController::ReconstructClipViewsChain(int number_of_clips, |
There was a problem hiding this comment.
I'm not sure what child and parent mean in this context... would it make sense to call it platform_view and head_clip_view?
| case clip_rect: | ||
| case clip_rrect: | ||
| case clip_path: { | ||
| UIView* clipView = head.superview; |
There was a problem hiding this comment.
nit: maybe cast this to a ChildClippingView already here?
We can add a comment saying that this is called after ReconstructClipViewsChain which assures we have a ChildClippingView parent for each clip in the stack.
| std::shared_ptr<IOSGLContext> gl_context, | ||
| GrContext* gr_context); | ||
| // Traverse the `mutators_stack` and return the number of clip operations. | ||
| int GetNumberOfClips(const MutatorsStack& mutators_stack); |
| // numbers of UIViews between the `child` and the `parent` including the `parent`. Returns the new | ||
| // parent after reconsturcting. If the returning parent is not the same as child, it will be an | ||
| // instance of `ChildClippingView`. | ||
| UIView* ReconstructClipViewsChain(int number_of_clips, UIView* child, UIView* parent); |
There was a problem hiding this comment.
Suggestion:
// Make sure that platform_view has exactly clip_count ChildClippingView ancestors.
//
// Existing ChildClippingViews are re-used. If there are currently more ChildClippingView ancestors than needed
// the extra views are detached. If there are less ChildClippingView ancestors than needed new ChildClippingViews will be added.
//
// If head_clip_view was attached as a subview to FlutterView, the head of the newly constructed ChildClippingViews chain is attached to FlutterView in the same position.
//
// Returns the new head of the clip views chain.
| // instance of `ChildClippingView`. | ||
| UIView* ReconstructClipViewsChain(int number_of_clips, UIView* child, UIView* parent); | ||
|
|
||
| // Apply mutators in the mutators_stack to the UIView chain that was constructed by |
There was a problem hiding this comment.
nit: grammar - Applies the mutators
| // After each clip operation, we update the head to the super view of the current head. | ||
| void ApplyMutators(const MutatorsStack& mutators_stack, UIView* embedded_view); | ||
|
|
||
| // Composite the platform view with the passed in `params`. |
There was a problem hiding this comment.
this comment doesn't add much information on top of the method and argument names.
This reverts commit ebb5b90. Seeing the following breakage on host build: ``` ../../flutter/flow/scene_update_context.cc:205:36: error: non-const lvalue reference to type 'flutter::MutatorsStack' cannot bind to a value of unrelated type 'const flutter::Stopwatch' frame.context().raster_time(), ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ../../flutter/flow/scene_update_context.cc:207:36: error: no viable conversion from 'flutter::TextureRegistry' to 'const flutter::Stopwatch' frame.context().texture_registry(), ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ../../flutter/flow/instrumentation.h:55:32: note: candidate constructor not viable: no known conversion from 'flutter::TextureRegistry' to 'const flutter::Stopwatch &' for 1st argument FML_DISALLOW_COPY_AND_ASSIGN(Stopwatch); ^ ../../flutter/fml/macros.h:28:3: note: expanded from macro 'FML_DISALLOW_COPY_AND_ASSIGN' TypeName(const TypeName&) = delete; \ ^ ../../flutter/flow/scene_update_context.cc:208:36: error: non-const lvalue reference to type 'flutter::TextureRegistry' cannot bind to a temporary of type 'flutter::RasterCache *' &frame.context().raster_cache(), ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ../../flutter/flow/scene_update_context.cc:209:36: error: cannot initialize a member subobject of type 'const flutter::RasterCache *' with an rvalue of type 'bool' false}; ^~~~~ ```
…lutter#9480)" This reverts commit 00d929f.
flutter/engine@ae8e6d9...107fe82 git log ae8e6d9..107fe82 --no-merges --oneline 107fe82 Add --observatory-host switch (flutter/engine#9485) 20a76de Roll src/third_party/skia ebbc82c02471..69881fb0b5fb (11 commits) (flutter/engine#9479) 00d929f Revert "IOS Platform view transform/clipping (#9075)" (flutter/engine#9480) 3390019 fix NPE when a touch event is sent to an unknown Android platform view (flutter/engine#9476) ebb5b90 IOS Platform view transform/clipping (flutter/engine#9075) 13145e9 ios-unit-tests: Started using rsync instead of cp -R to copy frameworks. (flutter/engine#9471) The AutoRoll server is located here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md If the roll is causing failures, please contact the current sheriff ([email protected]), and stop the roller if necessary.
flutter/engine@ae8e6d9...107fe82 git log ae8e6d9..107fe82 --no-merges --oneline 107fe82 Add --observatory-host switch (flutter/engine#9485) 20a76de Roll src/third_party/skia ebbc82c02471..69881fb0b5fb (11 commits) (flutter/engine#9479) 00d929f Revert &flutter#34;IOS Platform view transform/clipping (flutter#9075)&flutter#34; (flutter/engine#9480) 3390019 fix NPE when a touch event is sent to an unknown Android platform view (flutter/engine#9476) ebb5b90 IOS Platform view transform/clipping (flutter/engine#9075) 13145e9 ios-unit-tests: Started using rsync instead of cp -R to copy frameworks. (flutter/engine#9471) The AutoRoll server is located here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md If the roll is causing failures, please contact the current sheriff ([email protected]), and stop the roller if necessary.
WIP:
Introduce
FlutterEmbededViewTransformStackto store all the transforms/clips layer before a platform view.When we composite platform view, we traverse the stack and apply each transform/clip. If it is a transform we apply to the current view; if it is a clip, we create a super view and apply clip to the super view, then set the current view to this super view.
We also check whether the clip operation required for the current composition is the same as the old composition. If they are the same, we reuse the old UIView stacks; if not same, we add/remove views accordingly.
===============================
Another solution to this would be creating one UIView (say
ClipView) for all the clips. So when we see a transform node, we apply the transform to the platform view directly. When we see a clip, we apply thetotalMatrixuntil the clip to the clip and append to the current clip path. When we finish loop through all the nodes, we apply the total clip to theClipView.I am happy to try to implement this with the alternative solution to compare the performance.
@amirh