Workaround for iOS embedder preroll issue

[flar]

These changes are a potential fix for flutter/flutter#76097

The problem appears to be that a fix integrated in November will now lead to all layers being prerolled even if they may not paint. Previously we would try to avoid prerolling them if we didn't think we would call their paint method, but that turned out to be too aggressive a heuristic and so the new code will always preroll every layer. As such, a layer (or an embedder) cannot assume that because it was prerolled, that it will also paint.

The changes here attempt to avoid calling the embedder's preroll method if the PlatformViewLayer does not think that its Paint method will be called., but the issue with that workaround is that the condition used here to predict if the Paint method will be called on the PlatformViewLayer is not precise. The actual test that determines culling in the Paint cycle uses a different algorithm that is based on information maintained by the SkCanvas. As a result we can't guarantee that this test will catch 100% of cases where paint will or will not be called.

Without this fix, the Android embedder does not exhibit the issue linked above, so that version of the embedder proves that it is possible for an embedder implementation to have its preroll methods called, but not paint residue on the screen. Only the iOS embedder is leaving a last sliver of the platform view visible after it is scrolled behind something.

A better fix would be to modify the iOS embedder so that it paints nothing if its Paint() method is not actually called.

I'm tagging some of the names that show up in the iOS FlutterPlatformViews.mm file s possible reviewers or contributors who can suggest a more complete fix: @blasten @cyanglaz @amirh @cbracken @iskakaushik

[flutter-dashboard]

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

[flar]

Note that the original issue used a Map view to demonstrate the problem, but it was discovered that the problem manifests with a WebView which is simpler to use for diagnosis and testing. Here is my WebView version of the original test case in case someone wants to investigate the potential fix further:

Simpler test case

import 'dart:async';

import 'package:flutter/material.dart';
// import 'package:google_maps_flutter/google_maps_flutter.dart';
import 'package:webview_flutter/webview_flutter.dart';

void main() {
  runApp(MyApp());
}

class MyApp extends StatelessWidget {
  @override
  Widget build(BuildContext context) {
    return MaterialApp(
      title: 'Flutter Demo',
      theme: ThemeData(
        primarySwatch: Colors.blue,
      ),
      home: MyHomePage(title: 'Flutter Demo Home Page'),
    );
  }
}

class MyHomePage extends StatefulWidget {
  MyHomePage({Key key, this.title}) : super(key: key);

  final String title;

  @override
  _MyHomePageState createState() => _MyHomePageState();
}

class _MyHomePageState extends State<MyHomePage> {
  @override
  Widget build(BuildContext context) {
    var data = MediaQuery.of(context);
    var size = data.size;
    var padding = data.padding;

    var height = size.height - padding.top - padding.bottom - 100;

    return Scaffold(
      appBar: AppBar(
        title: Text(widget.title),
      ),
      body: SingleChildScrollView(
        child: Column(
          children: [
            Container(
              color: Colors.blue,
              height: height / 2.0,
            ),
            Container(
              color: Colors.red,
              height: height / 2.0,
            ),
            Map(),
          ],
        ),
      ),
      bottomNavigationBar: Container(
        color: Colors.purple,
        height: 150,
      ),
    );
  }
}

class Map extends StatefulWidget {
  @override
  _MapState createState() => _MapState();
}

class _MapState extends State<Map> {
  // static final CameraPosition _kGooglePlex = CameraPosition(
  //   target: LatLng(37.42796133580664, -122.085749655962),
  //   zoom: 14.4746,
  // );
  String selectedUrl = 'http://www.google.com/';
  final Completer<WebViewController> _controller = Completer<WebViewController>();
  @override
  Widget build(BuildContext context) {
    return AspectRatio(
      aspectRatio: 1.75,
        child: WebView(
          initialUrl: selectedUrl,
          javascriptMode: JavascriptMode.unrestricted,
          onWebViewCreated: (WebViewController webViewController) {
            _controller.complete(webViewController);
          },
        )
      // child: GoogleMap(
      //   gestureRecognizers: {},
      //   buildingsEnabled: false,
      //   compassEnabled: false,
      //   indoorViewEnabled: false,
      //   mapToolbarEnabled: false,
      //   myLocationButtonEnabled: false,
      //   myLocationEnabled: false,
      //   padding: EdgeInsets.only(
      //     left: 0,
      //     top: 0,
      //     right: 0,
      //     bottom: 0,
      //   ),
      //   trafficEnabled: false,
      //   zoomControlsEnabled: false,
      //   zoomGesturesEnabled: false,
      //   rotateGesturesEnabled: false,
      //   scrollGesturesEnabled: false,
      //   tiltGesturesEnabled: false,
      //   initialCameraPosition: _kGooglePlex,
      // ),
    );
  }
}

[iskakaushik]

@flar I agree with your assessment that this fix wouldn't account for all cases where Paint would get called but the proposed heuristic will fail.

I'm trying to understand why iOS still sees the last sliver of the platform view when the composition shouldn't contain the platform view in the first place. My question is when the frame shouldn't contain the platform view (which can presumably be determined by the framework), why is the layer tree containing the platform view?

[flar]

@iskakaushik I agree that's the pressing question.

It must be something that happens in the Preroll methods in the embedder since this fix is basically just omitting those calls. Also, I can confirm that the PlatformViewLayer Paint method is not called when the engine thinks that the view is not visible - so whatever is showing up on screen is planned by the actions taken during preroll and there is no stopping them once that happens.

[flar]

My theory, based entirely on symptoms and when and how they present is that the logic is as follows:

iOS embedder preroll: records "I'm going to be painted", does not update anything about how much and where.

iOS embedder paint: determines exactly what parts should appear

So, on the last visible frame, the paint method updated the "how much and where". When the next preroll comes in, it leaves that information in some data structure somewhere and marks it as "likely to paint". When the paint method doesn't run to discover that the "how much" is "nothing", then the final compositing step for the frame just inserts the part of the view that had been visible the last time its visibility was computed...?

If so, then the fix would be to clear the visibility region calculations from the previous frame during preroll, or to set the "I need to be painted" flag only when called from the Paint part of the layer mechanism.

[iskakaushik]

The question I'm trying to get an answer to is, Why does the scene even contain a PlatformViewLayer? I would have expected the framework to not call SceneBuilder::addPlatformView in this case.

I see what you are saying about Preroll saving state. FlutterPlatfomView.mm does store some state about the composition order in Preroll methods, which is used in SubmitFrame in the same class to paint the residual platform view. We could totally ensure that only Painted views get submitted by storing some state in CompositeEmbedderView and fixing the compositon order in SubmitFrame. But there are a few open questions:

1. context->has_platform_view should likely be false in these cases, and it likely needs to be updated prior to Paint since it determines raster cache usage. We should account for this.
2. There seems to be something about range trees in FlutterPlatformView.mm those should potentially let us know about whether a platform views is visible in the scene or not. This would help us answer "1" at the end of preroll still.

I still want to figure out why addPlatformView is getting called and if we can avoid doing that, that should be the real-fix.

@cyanglaz do you know the answer to any of these questions?

[cyanglaz]

Why does the scene even contain a PlatformViewLayer? I would have expected the framework to not call SceneBuilder::addPlatformView in this case.

+1, we need to figure out why a PlatformViewLayer is in the layer tree if the framework thinks the platform view should not be visiable.

There seems to be something about range trees in FlutterPlatformView.mm those should potentially let us know about whether a platform views is visible in the scene or not. This would help us answer "1" at the end of preroll still.

I don't think RTree's purpose was to determine whether the platform view is in the scene. We might be potentially use it to determine that, but it doesn't seem to be very straight forward. @blasten might know.

And sorry to add more questions here but why would a layer needs to be prerolled but not painted? Was there some change regarding how the layer-tree is built because of this? previously, I think layer tree is the source of truth as what should be painted on the scene, which means everything on the layer tree should be painted.

[flar]

The view is in the scene. But it is hidden out of sight by a scrolling clip. The framework does cull offscreen pieces of scrolling views, but it does not do so tightly. I think it keeps all items that are visible plus an extra item on each side. I'm guessing this is so that the items can be warmed up as they come into view. In either case, the view here does still intersect the screen space, but other items on the page cause it to be clipped out - not something easy to determine at scene building time.

[flar]

We need to preroll all layers because they can be incidentally rendered during layer caching even when they are currently off screen. They may not "show up" in this particular frame, but they might show up in a layer cache that is being prepared during this frame.

When we were making assumptions about not prerolling children and then those children were painted via the layer cache mechanism, they were producing bad output because their preroll was not called, but their paint was.

[flar]

Layer tree is not the source of truth, it is an approximation of the truth based on first approximations available at the framework level. The operations that are done on the layer tree inside the engine determine the final truth.

[flar]

One source of inspiration could be the Android platform view embedder which does not suffer from this phenomenon. Perhaps it isn't directly comparable because the two platforms might be quite different, but it might provide some insight.

[flar]

I'm marking this PR "WIP" since this is just a(n ill-advised) workaround for now.

[chinmaygarde]

@flar and I discussed this in person and the consensus was that eliding the Paint was causing platform view mutators to not be applied. Even though there is nothing for Flutter to paint, the platform views might still need to be updated. IMO, the most simple fix for this would be to defeat the optimization when a platform view is present in the hierarchy. A slightly more involved fix could be to tell the platform view embedder (in the preroll) that it might not be receiving the paint soon. But that may run the risk of too many unnecessary updates. I think the simple fix makes the most sense.

W.r.t the contents of this patch, I don't think we should land this now that we understand the issue.

[flar]

Closing in favor of new fix (actually still mostly a workaround, but the new workaround should be more correct)

#25824