[ios] Enable/disable presentsWithTransaction to address #14232 #14307

julianrex · 2019-04-02T22:50:05Z

Moved from: #14264

This is a potential work around for for the slowdown reported in #14232. This may not address the associated crash.

As @jmkiley notes in that issue the cause appears to be the introduction of CAEAGLLayer.presentsWithTransaction in #12895 to synchronize OpenGL and UIKit views.

…ndow. Added change log Recreate GL views if rendering takes > 1 second. (since glClear is blocked for 1 sec) Use presentsWithTransaction only if we have annotation UIViews. Re-add annotation views.

…re are 3 view annotations

platform/ios/src/MGLMapView.mm

platform/ios/CHANGELOG.md

platform/ios/src/MGLMapView.mm

chloekraw · 2019-04-02T23:11:11Z

platform/ios/src/MGLMapView.mm

+            [self.glView display];
+            CFTimeInterval after = CACurrentMediaTime();
+
+            if (after-before >= 1.0) {


My second comment on this #ifdef is that I'm concerned about unintended consequences and believe the risks could outweigh the potential benefits, especially since this issue is so difficult to reproduce, but glClear could be blocked due to a variety of circumstances. We don't want to throw this hammer at every circumstance.

I can get onboard with offering this "escape hatch" in the code if we can implement it in a way so that it's entirely opt-in by the developer. I'm also ok with testing this before merging to see what the impact is like. But I remain skeptical that this is going to be a net-better experience for most apps and their end-users, compared to the status quo where sometimes when you rapidly switch between tabs many times, the map freezes and you have to restart the app. The vast majority of apps don't have use cases where users are frequently switching tabs, and until we hear more than one report of this issue, I think this would be overkill.

The developer would need to be building from source & provide the MGL_RECREATE_GL_IN_AN_EMERGENCY define as @1ec5 mentions above.

However, we will need to test that code path regardless.

A single OpenGL call should take milliseconds. If glClear() is blocked for a second or more, that’s a clear sign of trouble, whether or not it’s the specific issue we’re trying to work around.

But I remain skeptical that this is going to be a net-better experience for most apps and their end-users, compared to the status quo where sometimes when you rapidly switch between tabs many times, the map freezes and you have to restart the app.

Totally agree. We could decide (later) to change from an #ifdef to a developer set property, but hopefully a better solution will have shown its head by then.

Yep, longer term it might be worth having something more robust anyway. But then, Metal.

Sounds good!

…anges (if visible)

1ec5 · 2019-04-03T03:24:53Z

platform/ios/src/MGLMapView.mm

@@ -444,6 +449,7 @@ - (void)commonInit
 {
    _isTargetingInterfaceBuilder = NSProcessInfo.processInfo.mgl_isInterfaceBuilderDesignablesAgent;
    _opaque = NO;
+    _atLeastiOS_12_2_0 = [NSProcessInfo.processInfo isOperatingSystemAtLeastVersion:(NSOperatingSystemVersion){12,2,0}];


Nit: The operating system version doesn’t change dynamically during the process’ lifetime, so we can set this variable in +initialize above. (The same also goes for _isTargetingInterfaceBuilder, come to think of it.)

Yep, let's address this later.

1ec5 · 2019-04-03T03:29:08Z

platform/ios/src/MGLMapView.mm

@@ -1101,6 +1110,13 @@ - (void)updateFromDisplayLink:(CADisplayLink *)displayLink
 {
    MGLAssertIsMainThread();

+    // Not "visible" - this isn't a full definition of visibility, but if
+    // the map view doesn't have a window then it *cannot* be visible.
+    if (!self.window) {


This seems like a pretty safe assumption. Do we suspect that -didMoveToWindow doesn’t get called as reliably?

More concerns about when and how the display link method may be called. It's definitely defensive and a remnant of testing, but I think it's worth keeping.

1ec5 · 2019-04-03T03:49:45Z

platform/ios/src/MGLMapView.mm

+    _glView.drawableDepthFormat = GLKViewDrawableDepthFormat16;
+    _glView.contentScaleFactor = [UIScreen instancesRespondToSelector:@selector(nativeScale)] ? [[UIScreen mainScreen] nativeScale] : [[UIScreen mainScreen] scale];
+    _glView.layer.opaque = _opaque;
+    _glView.delegate = self;


These several lines are copy-pasted from -createGLView. Can we refactor that method to avoid the duplication? I’m just concerned that we may find it useful to keep this method around longer-term, but it’s really easy to forget to keep two code paths in sync.

chloekraw

✅

…4307) (#14318)

Julian Rex added 2 commits April 2, 2019 18:44

[ios] Enable/disable presentsWithTransaction when moving to/from a wi…

d9379c1

…ndow. Added change log Recreate GL views if rendering takes > 1 second. (since glClear is blocked for 1 sec) Use presentsWithTransaction only if we have annotation UIViews. Re-add annotation views.

ifdef out the emergency code. Enable presentsWithTransaction when the…

630c10f

…re are 3 view annotations

julianrex requested a review from a team April 2, 2019 22:50

julianrex added bug iOS Mapbox Maps SDK for iOS labels Apr 2, 2019

julianrex mentioned this pull request Apr 2, 2019

[ios] Enable/disable presentsWithTransaction when moving to/from a window #14264

Closed

julianrex requested review from chloekraw and friedbunny April 2, 2019 22:51

Added missing ifdef

cfeaad3

chloekraw reviewed Apr 2, 2019

View reviewed changes

Update with PR feedback

da1c4de

julianrex added this to the release-liquid milestone Apr 3, 2019

Toggle presentsWithTransactions when enablePresentsWithTransaction ch…

bc4d3ab

…anges (if visible)

1ec5 reviewed Apr 3, 2019

View reviewed changes

Julian Rex added 2 commits April 3, 2019 01:54

Refactor GLKView generation

511edbf

Lower presentsWithTransaction threshold

bbde922

chloekraw self-requested a review April 3, 2019 15:44

chloekraw approved these changes Apr 3, 2019

View reviewed changes

julianrex merged commit c7b709c into master Apr 3, 2019

julianrex added a commit that referenced this pull request Apr 3, 2019

[ios] Enable/disable presentsWithTransaction to address #14232 (#14307)

c279059

julianrex mentioned this pull request Apr 3, 2019

[ios] Cherry picks - Enable/disable presentsWithTransaction to address #14232 (#14307) #14318

Merged

julianrex added a commit that referenced this pull request Apr 3, 2019

[ios] Enable/disable presentsWithTransaction to address #14232 (CP #1…

70c3abf

…4307) (#14318)

friedbunny deleted the jrex/14232-work-around-master branch April 5, 2019 20:41

julianrex mentioned this pull request Apr 16, 2019

[ios] presentsWithTransaction logic should include callout views. #14442

Closed

julianrex mentioned this pull request May 4, 2019

Custom callout presentation should call delegate methods. mapbox/ios-sdk-examples#300

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ios] Enable/disable presentsWithTransaction to address #14232 #14307

[ios] Enable/disable presentsWithTransaction to address #14232 #14307

julianrex commented Apr 2, 2019 •

edited

Loading

chloekraw Apr 2, 2019 •

edited

Loading

julianrex Apr 3, 2019

1ec5 Apr 3, 2019 •

edited

Loading

julianrex Apr 3, 2019

julianrex Apr 3, 2019 •

edited

Loading

chloekraw Apr 3, 2019

1ec5 Apr 3, 2019

julianrex Apr 3, 2019

1ec5 Apr 3, 2019

julianrex Apr 3, 2019

1ec5 Apr 3, 2019

chloekraw left a comment

[ios] Enable/disable presentsWithTransaction to address #14232 #14307

[ios] Enable/disable presentsWithTransaction to address #14232 #14307

Conversation

julianrex commented Apr 2, 2019 • edited Loading

chloekraw Apr 2, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1ec5 Apr 3, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julianrex Apr 3, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chloekraw left a comment

Choose a reason for hiding this comment

julianrex commented Apr 2, 2019 •

edited

Loading

chloekraw Apr 2, 2019 •

edited

Loading

1ec5 Apr 3, 2019 •

edited

Loading

julianrex Apr 3, 2019 •

edited

Loading