From ce4648ac818de8ae1fbc4299bfb1b4e4ef8b44f8 Mon Sep 17 00:00:00 2001 From: David Iglesias Teixeira Date: Wed, 8 Feb 2023 23:58:18 +0000 Subject: [PATCH 1/5] Somewhat works... Remove first overlay though. --- .../src/engine/canvaskit/embedded_views.dart | 26 ++++++++++++++----- 1 file changed, 19 insertions(+), 7 deletions(-) diff --git a/lib/web_ui/lib/src/engine/canvaskit/embedded_views.dart b/lib/web_ui/lib/src/engine/canvaskit/embedded_views.dart index 27694946d5a23..d92a179277c3e 100644 --- a/lib/web_ui/lib/src/engine/canvaskit/embedded_views.dart +++ b/lib/web_ui/lib/src/engine/canvaskit/embedded_views.dart @@ -136,8 +136,7 @@ class HtmlViewEmbedder { // We need an overlay for the first platform view no matter what. The first // visible platform view doesn't need to create a new one if we already // created one. - final bool needNewOverlay = (platformViewManager.isVisible(viewId) && - _context.seenFirstVisibleViewInPreroll) || + final bool needNewOverlay = platformViewManager.isVisible(viewId) || _context.pictureRecordersCreatedDuringPreroll.isEmpty; if (platformViewManager.isVisible(viewId)) { _context.seenFirstVisibleViewInPreroll = true; @@ -174,8 +173,7 @@ class HtmlViewEmbedder { _context.visibleViewCount++; } // We need a new overlay if this is a visible view or if we don't have one yet. - final bool needNewOverlay = (platformViewManager.isVisible(viewId) && - _context.seenFirstVisibleView) || + final bool needNewOverlay = platformViewManager.isVisible(viewId) || _context.pictureRecorders.isEmpty; if (platformViewManager.isVisible(viewId)) { _context.seenFirstVisibleView = true; @@ -669,10 +667,24 @@ class HtmlViewEmbedder { } else { currentGroup = [view]; } - } else { - // We hit this case if this is the first visible view. + } + if (!foundFirstVisibleView) { + // First visible view found... foundFirstVisibleView = true; - currentGroup.add(view); + if (currentGroup.isNotEmpty) { + // If we've seen invisible views earlier, split them off... + result.add(currentGroup); + // If we are out of overlays, then break let the rest of the views be + // added to an extra group that will be rendered on top of the scene. + if (result.length == maxOverlays) { + currentGroup = []; + break; + } else { + currentGroup = [view]; + } + } else { + currentGroup.add(view); + } } } } From f6fecbb14d996ed70e2964a38aa580cc3e40a774 Mon Sep 17 00:00:00 2001 From: David Iglesias Teixeira Date: Thu, 9 Feb 2023 01:14:46 +0000 Subject: [PATCH 2/5] needNewOverlay is equivalent to isVisible in the algorithm. --- .../src/engine/canvaskit/embedded_views.dart | 47 +++++-------------- 1 file changed, 11 insertions(+), 36 deletions(-) diff --git a/lib/web_ui/lib/src/engine/canvaskit/embedded_views.dart b/lib/web_ui/lib/src/engine/canvaskit/embedded_views.dart index d92a179277c3e..418012ae3d0ec 100644 --- a/lib/web_ui/lib/src/engine/canvaskit/embedded_views.dart +++ b/lib/web_ui/lib/src/engine/canvaskit/embedded_views.dart @@ -136,11 +136,7 @@ class HtmlViewEmbedder { // We need an overlay for the first platform view no matter what. The first // visible platform view doesn't need to create a new one if we already // created one. - final bool needNewOverlay = platformViewManager.isVisible(viewId) || - _context.pictureRecordersCreatedDuringPreroll.isEmpty; - if (platformViewManager.isVisible(viewId)) { - _context.seenFirstVisibleViewInPreroll = true; - } + final bool needNewOverlay = platformViewManager.isVisible(viewId); if (needNewOverlay && hasAvailableOverlay) { final CkPictureRecorder pictureRecorder = CkPictureRecorder(); pictureRecorder.beginRecording(ui.Offset.zero & _frameSize); @@ -168,16 +164,11 @@ class HtmlViewEmbedder { CkCanvas? compositeEmbeddedView(int viewId) { final int overlayIndex = _context.visibleViewCount; _compositionOrder.add(viewId); - if (platformViewManager.isVisible(viewId) || - _context.pictureRecorders.isEmpty) { + if (platformViewManager.isVisible(viewId)) { _context.visibleViewCount++; } // We need a new overlay if this is a visible view or if we don't have one yet. - final bool needNewOverlay = platformViewManager.isVisible(viewId) || - _context.pictureRecorders.isEmpty; - if (platformViewManager.isVisible(viewId)) { - _context.seenFirstVisibleView = true; - } + final bool needNewOverlay = platformViewManager.isVisible(viewId); CkPictureRecorder? recorderToUseForRendering; if (needNewOverlay) { if (overlayIndex < _context.pictureRecordersCreatedDuringPreroll.length) { @@ -422,7 +413,11 @@ class HtmlViewEmbedder { ? null : diffViewList(_activeCompositionOrder, _compositionOrder); _updateOverlays(diffResult); - assert(_context.pictureRecorders.length == _overlays.length); + assert( + _context.pictureRecorders.length == _overlays.length, + 'There should be the same picture recorders (${_context.pictureRecorders.length}) ' + 'than overlays (${_overlays.length}).', + ); int pictureRecorderIndex = 0; for (int i = 0; i < _compositionOrder.length; i++) { @@ -667,24 +662,10 @@ class HtmlViewEmbedder { } else { currentGroup = [view]; } - } - if (!foundFirstVisibleView) { - // First visible view found... + } else { + // We hit this case if this is the first visible view. foundFirstVisibleView = true; - if (currentGroup.isNotEmpty) { - // If we've seen invisible views earlier, split them off... - result.add(currentGroup); - // If we are out of overlays, then break let the rest of the views be - // added to an extra group that will be rendered on top of the scene. - if (result.length == maxOverlays) { - currentGroup = []; - break; - } else { - currentGroup = [view]; - } - } else { - currentGroup.add(view); - } + currentGroup.add(view); } } } @@ -917,12 +898,6 @@ class MutatorsStack extends Iterable { /// The state for the current frame. class EmbedderFrameContext { - /// Whether or not we have seen a visible platform view in this frame yet. - bool seenFirstVisibleViewInPreroll = false; - - /// Whether or not we have seen a visible platform view in this frame yet. - bool seenFirstVisibleView = false; - /// Picture recorders which were created during the preroll phase. /// /// These picture recorders will be "claimed" in the paint phase by platform From 5cb315dd3226ccda1b549965977c85220c68afdb Mon Sep 17 00:00:00 2001 From: David Iglesias Teixeira Date: Thu, 9 Feb 2023 02:58:04 +0000 Subject: [PATCH 3/5] Only consider groups with a visible view to need an overlay. --- .../lib/src/engine/canvaskit/embedded_views.dart | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/lib/web_ui/lib/src/engine/canvaskit/embedded_views.dart b/lib/web_ui/lib/src/engine/canvaskit/embedded_views.dart index 418012ae3d0ec..e327f587be8aa 100644 --- a/lib/web_ui/lib/src/engine/canvaskit/embedded_views.dart +++ b/lib/web_ui/lib/src/engine/canvaskit/embedded_views.dart @@ -594,8 +594,14 @@ class HtmlViewEmbedder { return; } final List> overlayGroups = getOverlayGroups(_compositionOrder); - final List viewsNeedingOverlays = - overlayGroups.map((List group) => group.last).toList(); + final List viewsNeedingOverlays = overlayGroups + .where( + (List group) => group.any( + (int viewId) => platformViewManager.isVisible(viewId) + ) + ) + .map((List group) => group.last) + .toList(); // If there were more visible views than overlays, then the last group // doesn't have an overlay. if (viewsNeedingOverlays.length > SurfaceFactory.instance.maximumOverlays) { From 030ac473c54a599587ea3d5efefdf4367a9fb2c1 Mon Sep 17 00:00:00 2001 From: David Iglesias Teixeira Date: Thu, 9 Feb 2023 02:59:40 +0000 Subject: [PATCH 4/5] Update tests with new logic. Make test output easier to understand and find in the test file. --- .../test/canvaskit/embedded_views_test.dart | 38 +++++++++---------- 1 file changed, 18 insertions(+), 20 deletions(-) diff --git a/lib/web_ui/test/canvaskit/embedded_views_test.dart b/lib/web_ui/test/canvaskit/embedded_views_test.dart index 112fc0b34ae77..fe32856ba75d4 100644 --- a/lib/web_ui/test/canvaskit/embedded_views_test.dart +++ b/lib/web_ui/test/canvaskit/embedded_views_test.dart @@ -926,8 +926,7 @@ void testMain() { _expectSceneMatches(<_EmbeddedViewMarker>[ _overlay, _platformView, - _overlay, - ]); + ], reason: 'Invisible view alone renders on top of base overlay.'); sb = LayerSceneBuilder(); sb.pushOffset(0, 0); @@ -940,7 +939,7 @@ void testMain() { _platformView, _platformView, _overlay, - ]); + ], reason: 'Overlay created after a group containing a visible view.'); sb = LayerSceneBuilder(); sb.pushOffset(0, 0); @@ -956,7 +955,7 @@ void testMain() { _overlay, _platformView, _overlay, - ]); + ], reason: 'Overlays created after each group containing a visible view.'); sb = LayerSceneBuilder(); sb.pushOffset(0, 0); @@ -974,7 +973,7 @@ void testMain() { _platformView, _platformView, _overlay, - ]); + ], reason: 'Invisible views grouped in with visible views.'); sb = LayerSceneBuilder(); sb.pushOffset(0, 0); @@ -1058,8 +1057,7 @@ void testMain() { _platformView, _platformView, _platformView, - _overlay, - ]); + ], reason: 'Many invisible views can be rendered on top of the base overlay.'); sb = LayerSceneBuilder(); sb.pushOffset(0, 0); @@ -1108,20 +1106,20 @@ enum _EmbeddedViewMarker { _EmbeddedViewMarker get _overlay => _EmbeddedViewMarker.overlay; _EmbeddedViewMarker get _platformView => _EmbeddedViewMarker.platformView; -void _expectSceneMatches(List<_EmbeddedViewMarker> markers) { - final List sceneElements = flutterViewEmbedder +const Map _tagToViewMarker = { + 'flt-canvas-container': _EmbeddedViewMarker.overlay, + 'flt-platform-view-slot': _EmbeddedViewMarker.platformView, +}; + +void _expectSceneMatches(List<_EmbeddedViewMarker> expectedMarkers, { + String? reason, +}) { + // Convert the scene elements to its corresponding array of _EmbeddedViewMarker + final List<_EmbeddedViewMarker> sceneElements = flutterViewEmbedder .sceneElement!.children .where((DomElement element) => element.tagName != 'svg') + .map((DomElement element) => _tagToViewMarker[element.tagName.toLowerCase()]!) .toList(); - expect(markers, hasLength(sceneElements.length)); - for (int i = 0; i < markers.length; i++) { - switch (markers[i]) { - case _EmbeddedViewMarker.overlay: - expect(sceneElements[i].tagName, 'FLT-CANVAS-CONTAINER'); - break; - case _EmbeddedViewMarker.platformView: - expect(sceneElements[i].tagName, 'FLT-PLATFORM-VIEW-SLOT'); - break; - } - } + + expect(sceneElements, expectedMarkers, reason: reason); } From ac3fe4562d823255384ba1f2423d72a5792d1dd8 Mon Sep 17 00:00:00 2001 From: David Iglesias Teixeira Date: Fri, 10 Feb 2023 01:48:01 +0000 Subject: [PATCH 5/5] Introduce OverlayGroup helper class. --- .../src/engine/canvaskit/embedded_views.dart | 117 ++++++++++++------ 1 file changed, 76 insertions(+), 41 deletions(-) diff --git a/lib/web_ui/lib/src/engine/canvaskit/embedded_views.dart b/lib/web_ui/lib/src/engine/canvaskit/embedded_views.dart index e327f587be8aa..f2ae09c9b236e 100644 --- a/lib/web_ui/lib/src/engine/canvaskit/embedded_views.dart +++ b/lib/web_ui/lib/src/engine/canvaskit/embedded_views.dart @@ -133,9 +133,8 @@ class HtmlViewEmbedder { 'displayed at once. ' 'You may experience incorrect rendering.'); } - // We need an overlay for the first platform view no matter what. The first - // visible platform view doesn't need to create a new one if we already - // created one. + // We need an overlay for each visible platform view. Invisible platform + // views will be grouped with (at most) one visible platform view later. final bool needNewOverlay = platformViewManager.isVisible(viewId); if (needNewOverlay && hasAvailableOverlay) { final CkPictureRecorder pictureRecorder = CkPictureRecorder(); @@ -164,10 +163,11 @@ class HtmlViewEmbedder { CkCanvas? compositeEmbeddedView(int viewId) { final int overlayIndex = _context.visibleViewCount; _compositionOrder.add(viewId); + // Keep track of the number of visible platform views. if (platformViewManager.isVisible(viewId)) { _context.visibleViewCount++; } - // We need a new overlay if this is a visible view or if we don't have one yet. + // We need a new overlay if this is a visible view. final bool needNewOverlay = platformViewManager.isVisible(viewId); CkPictureRecorder? recorderToUseForRendering; if (needNewOverlay) { @@ -415,8 +415,8 @@ class HtmlViewEmbedder { _updateOverlays(diffResult); assert( _context.pictureRecorders.length == _overlays.length, - 'There should be the same picture recorders (${_context.pictureRecorders.length}) ' - 'than overlays (${_overlays.length}).', + 'There should be the same number of picture recorders ' + '(${_context.pictureRecorders.length}) as overlays (${_overlays.length}).', ); int pictureRecorderIndex = 0; @@ -593,15 +593,13 @@ class HtmlViewEmbedder { // overlays. return; } - final List> overlayGroups = getOverlayGroups(_compositionOrder); - final List viewsNeedingOverlays = overlayGroups - .where( - (List group) => group.any( - (int viewId) => platformViewManager.isVisible(viewId) - ) - ) - .map((List group) => group.last) - .toList(); + // Group platform views from their composition order. + // Each group contains one visible view, and any number of invisible views + // before or after that visible view. + final List overlayGroups = + getOverlayGroups(_compositionOrder); + final List viewsNeedingOverlays = + overlayGroups.map((OverlayGroup group) => group.last).toList(); // If there were more visible views than overlays, then the last group // doesn't have an overlay. if (viewsNeedingOverlays.length > SurfaceFactory.instance.maximumOverlays) { @@ -641,44 +639,48 @@ class HtmlViewEmbedder { // If there are more visible views than overlays, then the views which cannot // be assigned an overlay are grouped together and will be rendered on top of // the rest of the scene. - List> getOverlayGroups(List views) { - // Visibility groups are typically a visible view followed by zero or more - // invisible views. However, if the view list begins with one or more - // invisible views, we can group them with the first visible view. + List getOverlayGroups(List views) { final int maxOverlays = SurfaceFactory.instance.maximumOverlays; if (maxOverlays == 0) { - return const >[]; + return const []; } - bool foundFirstVisibleView = false; - final List> result = >[]; - List currentGroup = []; - int i = 0; - for (; i < views.length; i++) { + final List result = []; + OverlayGroup currentGroup = OverlayGroup([]); + + for (int i = 0; i < views.length; i++) { final int view = views[i]; if (platformViewManager.isInvisible(view)) { + // We add as many invisible views as we find to the current group. currentGroup.add(view); } else { - if (foundFirstVisibleView) { - result.add(currentGroup); - // If we are out of overlays, then break let the rest of the views be - // added to an extra group that will be rendered on top of the scene. - if (result.length == maxOverlays) { - currentGroup = []; - break; + // `view` is visible. + if (!currentGroup.hasVisibleView) { + // If `view` is the first visible one of the group, add it. + currentGroup.add(view, visible: true); + } else { + // There's already a visible `view` in `currentGroup`, so a new + // OverlayGroup will be needed. + // Let's decide what to do with the `currentGroup` first: + if (currentGroup.hasVisibleView) { + // We only care about groups that have one visible view. + result.add(currentGroup); + } + // If there are overlays still available. + if (result.length < maxOverlays) { + // Create a new group, starting with `view`. + currentGroup = OverlayGroup([view], visible: true); } else { - currentGroup = [view]; + // Add the rest of the views to a final group that will be rendered + // on top of the scene. + currentGroup = OverlayGroup(views.sublist(i), visible: true); + // And break out of the loop! + break; } - } else { - // We hit this case if this is the first visible view. - foundFirstVisibleView = true; - currentGroup.add(view); } } } - if (i < views.length) { - currentGroup.addAll(views.sublist(i)); - } - if (currentGroup.isNotEmpty) { + // Handle the last group to be (maybe) returned. + if (currentGroup.hasVisibleView) { result.add(currentGroup); } return result; @@ -726,6 +728,39 @@ class HtmlViewEmbedder { } } +/// A group of views that will be composited together within the same overlay. +/// +/// Each OverlayGroup is a sublist of the composition order which can share the +/// same overlay. +/// +/// Every overlay group is a list containing a visible view preceded or followed +/// by zero or more invisible views. +class OverlayGroup { + /// Constructor + OverlayGroup( + List viewGroup, { + bool visible = false, + }) : _group = viewGroup, + _containsVisibleView = visible; + + // The internal list of ints. + final List _group; + // A boolean flag to mark if any visible view has been added to the list. + bool _containsVisibleView; + + /// Add a [view] (maybe [visible]) to this group. + void add(int view, {bool visible = false}) { + _group.add(view); + _containsVisibleView |= visible; + } + + /// Get the "last" view added to this group. + int get last => _group.last; + + /// Returns true if this group contains any visible view. + bool get hasVisibleView => _group.isNotEmpty && _containsVisibleView; +} + /// Represents a Clip Chain (for a view). /// /// Objects of this class contain: