-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Prevent map view on external display from sleeping in background #13701
Conversation
19f96d3
to
29683e2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@1ec5 How do the helperMapView
s mirror the non-camera state of the map view from the main screen? Or is this just supposed to be a dummy window that keeps the app/screen alive on the other display?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me - thanks so much!
BTW I did run into #13703 when I tested on an Apple TV.
Incidentally, there are a number of places where, like in #7819 on macOS, we assume the map view’s screen is the main screen for the purpose of choosing a pixel density:
The CarPlay documentation specifically advises applications to account for pixel density differences, because CarPlay-enabled devices can have 1×, 2×, or 3× screens depending on the make and model. A mismatch could result in pixel alignment issues or less legible text rendering. But I think we should move forward with this background rendering fix independently of solving that pixel density mismatch issue. |
For the purpose of demonstration, I made it a secondary window that contains a separate map view. AirPlay mirroring is something an end user would configure rather than the application; instead, the view controller is synchronizing the camera between both map views, but otherwise the map views operate independently. |
// gpus_ReturnNotPermittedKillClient in libGPUSupportMercury, because the | ||
// external connection keeps the application from truly receding to the | ||
// background. | ||
if (self.window.screen != [UIScreen mainScreen]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you consider adding a flag along the lines of enableExternalDisplay
or enableSecondaryExternalDisplay
? Although I don't have an specific use case in mind, it may be good let the developers control this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m leaning towards not making this behavior configurable. Note that this PR only affects whether external displays can continue to render after the main display falls asleep; I’m pretty certain map views can already render on external displays.
I think an enableExternalDisplay
option would lead to surprising behavior. Suppose an application has non-mirroring support for external displays, such as what this PR adds to iosapp, and the external display’s window contains not only a map view but other views. Perhaps these other views are programmatically synchronized to the map view, with dynamic text based on the current camera. If enableExternalDisplay
is disabled, then the UI will appear to work normally, but as soon as the main display falls asleep, the map view portion of the UI will halt and eventually go black. All the while, the rest of the UI will continue to update, because the map view’s model continues to function even while rendering is disabled.
That said, I’m open to adding an explicit option if we find that this PR regresses the crash fix and we can’t figure out a way around it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’d agree with @1ec5 here — the developer should probably be responsible for handling how mirroring/other-screens are managed (and we should try to transparently support those without configuration).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@1ec5 @friedbunny that makes sense, thank you for explaining your thoughts. I got a final question: If a map is being displayed on a second screen; would it be useful have delegates that accounts for that case?
e.g.
mapViewWillRenderSecondScreen
/mapViewWillStopRenderingSecondScreen
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
☝🏼discard my comment above.
Based on tests on the CarPlay head unit, I confirmed that items 1 and 2 in the ticket description have been met.
It works! Tested in navigation mode, switched over to the Phone app and back (to the Navigation Example App), while home screen was locked and fell asleep. We need to be aware of these console logs, about Insufficient Permission (to submit GPU work from background) observed during the test.
With the home screen locked, I tested pan gestures and interaction with the map buttons for zoom in/out and center location. Saw the Insufficient Permission (to submit GPU work from background)
FYI, user experience was smooth and didn't crash either. LGTM 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job! @1ec5 🏅
Some additional test information: Switching applications when the world tour is running works well, with or without mirroring to an AppleTV.
|
I also gave this a try with an iPad 10.5” and an Apple TV 4K — it behaved as expected. Mirroring/second-screen’ing are perhaps a bit edge-case’y, but it’d be fun to eventually expand iosapp’s capabilities a bit around that. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for tackling this!
// gpus_ReturnNotPermittedKillClient in libGPUSupportMercury, because the | ||
// external connection keeps the application from truly receding to the | ||
// background. | ||
if (self.window.screen != [UIScreen mainScreen]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’d agree with @1ec5 here — the developer should probably be responsible for handling how mirroring/other-screens are managed (and we should try to transparently support those without configuration).
When the application enters background mode, only put the map view to sleep if it’s installed on the main screen or hasn’t been added to any window. Otherwise, if it’s targeting an external display, such as via AirPlay or CarPlay, allow the map view to continue rendering.
Whenever an external display is connected, show a map synchronized with the main map on the main display.
29683e2
to
0fbd878
Compare
Fixed an issue where an MGLMapView targeting an external display via CarPlay would freeze, ignoring both programmatic changes and user gestures, whenever the main device’s screen fell asleep automatically or the user manually locked the screen.
MGLMapView has to tiptoe around a hard prohibition against OpenGL rendering when the application recedes into the background or enters background mode, exhibited in crashes such as #1198 and #1460. However, if an application integrates the navigation SDK and is connected to CarPlay, the application will stay active even when the screen is locked.
This PR also includes a change to iosapp to show a map view on any connected external display and keep it synchronized with the map on the main display. This iosapp feature only works with AirPlay, not CarPlay, which requires a special delegate and entitlement.
There are a few edge cases to test before we can feel confident about this fix:
audio
(“Audio, AirPlay, and Picture in Picture”) background mode. That case is quite unlikely in conjunction with the navigation SDK and CarPlay, but it’s plausible for ordinary AirPlay-enabled applications.Fixes mapbox/mapbox-navigation-ios#1774.
/cc @mapbox/maps-ios @mapbox/navigation-ios