Round SkRects in ClipRectContainsPlatformViewBoundingRect

[jmagman]

Platform view clip mutators are being applied when the platform view scrolls off screen. This was due to floating point errors comparing the bounds of the platform view with the bounds of the clip view.

engine/shell/platform/darwin/ios/framework/Source/FlutterPlatformViews.mm

Lines 44 to 48 in 373b1ec

	static bool ClipRectContainsPlatformViewBoundingRect(const SkRect& clip_rect,
	const SkRect& platformview_boundingrect,
	const SkMatrix& transform_matrix) {
	SkRect transformed_rect = transform_matrix.mapRect(clip_rect);
	return transformed_rect.contains(platformview_boundingrect);

Example:

Clip rect:

(fLeft = 135, fTop = 17.2308426, fRight = 1305, fBottom = 2240.23096)

Platform view rect:

(fLeft = 135, fTop = 17.230835, fRight = 1035, fBottom = 767.230835)

The clip rect should be larger than the platform view rect and therefore no clipping should happen. The left and top should be equal, and the right and bottom clip is greater than the platform view. However, because the top 17.2308426 > 17.230835 the rect is not larger (for the clip rect to contain the platform view, left and top must be <= and right and bottom must be >=), then clipping happens, causing an expensive -[FlutterClippingMaskView drawRect:] call. Handle this by rounding the rect bounds during the fast-path ClipRectContainsPlatformViewBoundingRect check.

Fixes flutter/flutter#142828
See also #37434 and #20050

Pre-launch Checklist

• I read the Contributor Guide and followed the process outlined there for submitting PRs.
• I read the Tree Hygiene wiki page, which explains my responsibilities.
• I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
• I listed at least one issue that this PR fixes in the description above.
• I added new tests to check the change I am making or feature I am adding, or the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
• I updated/added relevant documentation (doc comments with ///).
• I signed the CLA.
• All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

[jmagman]

This check fails on master but passes on this PR.

[jmagman]

(@stuartmorgan I added you since you reviewed #37434 and maybe at one point were familiar with this code)

[jonahwilliams]

LGTM

I think if you plan to work on platform view performance then it would be beneficial to have a benchmark running a non-trivial platform view. Eyeballing the cumulative effect of many small perf improvements will be difficult, and I've known many an engineer get discouraged when it felt like they weren't making progress.

[stuartmorgan-g]

Are these logical pixels or physical pixels? Integers seems pretty coarse.

Should we maybe outset the clip rect by a small delta instead?

[jmagman]

Physical pixels since it's running right before this logic:

engine/shell/platform/darwin/ios/framework/Source/FlutterPlatformViews.mm

Lines 562 to 567 in 45137ea

	// 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
	// 500 points in UIKit for devices that has screenScale of 2. We need to scale the transformMatrix
	// down to the logical resoltion before applying it to the layer of PlatformView.
	transformMatrix.postScale(1 / screenScale, 1 / screenScale);

[jonahwilliams]

if its physical pixels and the error is at the scale of 0.00001, then even rounding to the nearest 0.01 should resolve the problem without having any meaningful effect on the rendering while still being way smaller than the space between two sample points (at 4x MSAA there are 4 per physical pixel).

[jmagman]

I think it should be fine to copy the logic here and round the outer rect out:

engine/shell/platform/darwin/ios/framework/Source/FlutterPlatformViews.mm

Lines 730 to 733 in 919af2c

	for (SkIRect& joined_rect : intersection_rects) {
	// Get the intersection rect between the current rect
	// and the platform view rect.
	joined_rect.intersect(platform_view_rect.roundOut());

    /** Sets SkRect by discarding the fractional portion of fLeft and fTop; and rounding
        up fRight and fBottom, using
        (sk_float_floor(fLeft), sk_float_floor(fTop),
         sk_float_ceil(fRight), sk_float_ceil(fBottom)).

        @param dst  storage for SkRect
    */
    void roundOut(SkRect* dst) const {
        dst->setLTRB(sk_float_floor(fLeft), sk_float_floor(fTop),
                     sk_float_ceil(fRight), sk_float_ceil(fBottom));
    }

[hellohuanlin]

Platform view clip mutators are being applied when the platform view scrolls off screen.

For me to better understand - this rounding trick works because the platform view is less than 1 pixel outside the screen bound, and we mistakenly considered it as still within the screen bound (due to rounding errors)?

[chinmaygarde]

@jmagman: @hellohuanlin has an open question. But otherwise this PR seems good to go. Can we land it?

[jmagman]

But otherwise this PR seems good to go. Can we land it?

There's a request to make it round to 0.01 instead of integer. I need to update it.

[hellohuanlin]

do we want to hold this till we have a project to measure the improvement?

[jmagman]

Removed in #48162

[jmagman]

FlutterBinaryMessengerRelayTest is no longer in the iOS directory and is tested in common #44395

[chinmaygarde]

Triage: The last update was that there was still some work to do to setup a benchmark for this. Is that still the case?

[jmagman]

This doesn't work except on top of #50386, which makes me think I don't have the whole picture here. Closing for now.