-
Notifications
You must be signed in to change notification settings - Fork 1.3k
[ios] Add preferred FPS setting; vary maximum FPS by device capability #12501
Conversation
a5762eb
to
9432d47
Compare
@@ -79,6 +79,21 @@ typedef NS_ENUM(NSUInteger, MGLUserTrackingMode) { | |||
MGLUserTrackingModeFollowWithCourse, | |||
}; | |||
|
|||
/** Options for `MGLMapView.preferredFramesPerSecond`. */ | |||
typedef NSInteger MGLMapViewPreferredFramesPerSecond NS_TYPED_EXTENSIBLE_ENUM; |
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 is implemented with NS_TYPED_EXTENSIBLE_ENUM
(instead of using NS_ENUM
or the like) to enable Swift users to omit the full key/const name and use the favored type-aware dot syntax (e.g., mapView.preferredFramesPerSecond = .lowPower
).
The preferred frame rate at which the map view is rendered. | ||
|
||
The default value for this property is | ||
`MGLMapViewPreferredFramesPerSecondDefault`, which will adaptively set the |
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.
which will adaptively set the ...
does this means that it will be overridden by the sdk according to the device?
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.
Yes, MGLMapViewPreferredFramesPerSecondDefault
means that the SDK will decide, based on the list of legacy devices in UIDevice.mgl_isLegacyDevice
, whether or not to set the FPS limit to 30
or 60
.
|
||
@see `CADisplayLink.preferredFramesPerSecond` | ||
*/ | ||
@property (nonatomic, assign) MGLMapViewPreferredFramesPerSecond preferredFramesPerSecond; |
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.
What is the purpose for letting devs set 0...60? does it makes a difference than restricting to -1/30/60?
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.
Allowing developers to set arbitrary values means that we can allow:
- 120 FPS (via
0
) without explicitly documenting it or adding a key. - Extreme throttling, as our navigation SDK currently does (as low as 5 FPS, apparently).
The -1
of MGLMapViewPreferredFramesPerSecondDefault
is a placeholder so that we can name the key, but it has no direct meaning in terms of FPS (#12501 (comment)).
A few discussion points:
|
FOUNDATION_EXTERN MGL_EXPORT const MGLMapViewPreferredFramesPerSecond MGLMapViewPreferredFramesPerSecondDefault; | ||
|
||
/** A conservative frame rate; typically 30 FPS. */ | ||
FOUNDATION_EXTERN MGL_EXPORT const MGLMapViewPreferredFramesPerSecond MGLMapViewPreferredFramesPerSecondLowPower; |
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 wonder if lowPower
would be better for a even-lower setting. 30fps is still be pretty decent frame rate. Did you run any energy tests?
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 not entirely happy with lowPower
as the name for this setting, but I couldn’t think of anything better. Google calls this mode conservative
, which avoids the potential confusion of “does this setting use less energy or is it meant for underpowered devices?” — both are true, I suppose.
@@ -87,6 +88,10 @@ | |||
const CGFloat MGLMapViewDecelerationRateFast = UIScrollViewDecelerationRateFast; | |||
const CGFloat MGLMapViewDecelerationRateImmediate = 0.0; | |||
|
|||
const MGLMapViewPreferredFramesPerSecond MGLMapViewPreferredFramesPerSecondDefault = -1; | |||
const MGLMapViewPreferredFramesPerSecond MGLMapViewPreferredFramesPerSecondLowPower = 30; | |||
const MGLMapViewPreferredFramesPerSecond MGLMapViewPreferredFramesPerSecondMaximum = 60; |
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.
Should .lowPower
and .maximum
also be based per-device type?
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 think there’s room for that kind of functionality, yeah — testing on an iPhone 5, it does seem like 15 FPS may be more appropriate for that device.
Should we synchronize rendering to the vsync?As our property is essentially a very thin wrapper around
This tech note is also interesting:
... so in the case of Should we restrict to the set of enums?I’d argue that exposing the ability to set the preferred FPS directly is a feature — see #12501 (comment). What happens on macOS?Nothing — this PR doesn’t affect that platform (and I don’t expect there to be any need to implement this feature there, for various hardware/product/development-effort reasons). What about external displays?I don’t know, but I’d expect this setting to generally work on external displays. External displays are a rather uncommon use case on iOS and not something we’ve really ever considered. Exposing the ability to set the underlying property arbitrarily would allow a developer to fine-tune performance in this and other edge cases we might not consider/optimize for. |
I agree wholeheartedly. |
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.
👍
9432d47
to
436a39a
Compare
Should we temporarily toggle default framerate when interacting with the map view via gesture recognizers? Try panning the map using |
I don’t think the SDK should do this, but a developer is welcome to. I don’t see For weaker devices, though, lowering the number of frames rendered can be a huge improvement in usability. Panning and drifting after a pan are particularly intensive operations; if we were to boost the frame rate beyond the capabilities of a device in interactive scenarios, we’d be eliminating most of the benefit of capping the frame rate. From some rough measurement, it appears that gesture recognizers will continue to fire at up to the device’s maximum supported delivery rate (60Hz, in the case of all current iPhones) regardless of the frame rate. In the future, I think it would definitely be worth making sure that we invalidate or skip gesture-triggered work that won’t result in a frame draw. |
- Add `MGLMapView.preferredFramesPerSecond`, which can be set with the provided `MGLMapViewPreferredFramesPerSecond` enum values or directly with an integer. - Adaptively set the preferred FPS based on the capabilities of the device: the oldest and least powerful devices are now capped at 30 FPS, which results in a more consistent/smoother experience.
436a39a
to
a004e7d
Compare
There are still a variety of aspects of this PR that we may want to tweak (thresholds, devices, naming, etc.), but let’s merge this now and give it a test in the forthcoming |
If you wanted to enable 120 FPS, here’s how I’d suggest doing it: if #available(iOS 10.3, *), UIScreen.main.maximumFramesPerSecond > 60 {
mapView.preferredFramesPerSecond = MGLMapViewPreferredFramesPerSecond(rawValue: 0)
} This maintains the default throttling of older devices. |
The macOS implementation of MGLMapView doesn’t explicitly set a frame rate. In fact, MGLOpenGLLayer leaves the frame rate up to WindowServer. This could be an issue with regard to #7820, but explicitly setting a frame rate would require having Core Video drive the animation, which in my early experiments turned my MacBook Pro into a space heater and white noise generator. |
New in this PR
MGLMapView.preferredFramesPerSecond
, which can be set with the providedMGLMapViewPreferredFramesPerSecond
enum values or directly with an integer.Notes
CADisplayLink.preferredFramesPerSecond
.0
(“native”).Tailwork
/cc @julianrex @fabian-guerra @ChrisLoer @1ec5 @bsudekum @frederoni @kkaefer @jfirebaugh