Add dynamic map FPS adjustment for NavigationMapboxMap #1669

danesfeder · 2019-01-14T19:36:14Z

This adds dynamic FPS throttling based on RouteProgress along the route. If we are going to execute a maneuver that is "lower risk" (continue straight, slight right, slight left) or we have determined we are far enough along the step (duration thresholds), we can set a lower ceiling for maximum FPS. iOS logic

TODO:

Unit tests
Field testing
Ensure lifecycle is handled correctly
Wait for Added RouteRetrievalEvent #1661 to land

cc @1ec5

codecov-io · 2019-01-14T19:54:24Z

Codecov Report

Merging #1669 into master will increase coverage by 0.18%.
The diff coverage is 36.02%.

@@             Coverage Diff              @@
##             master    #1669      +/-   ##
============================================
+ Coverage      26.3%   26.48%   +0.18%     
- Complexity      790      815      +25     
============================================
  Files           197      202       +5     
  Lines          8345     8513     +168     
  Branches        598      623      +25     
============================================
+ Hits           2195     2255      +60     
- Misses         5952     6046      +94     
- Partials        198      212      +14

Guardiola31337

Left some minor comments. Other than that this looks good to me 👍

Guardiola31337 · 2019-01-14T19:46:13Z

...ion-ui/src/main/java/com/mapbox/services/android/navigation/ui/v5/map/MapBatteryMonitor.java

+    }
+
+    int chargePlug = batteryStatus.getIntExtra(BatteryManager.EXTRA_PLUGGED, DEFAULT_BATTERY_LEVEL);
+    final boolean pluggedUsb = chargePlug == BatteryManager.BATTERY_PLUGGED_USB;


Why are these variables final? Those are local to isPluggedIn.

Guardiola31337 · 2019-01-14T19:47:27Z

...gation-ui/src/main/java/com/mapbox/services/android/navigation/ui/v5/map/MapFpsDelegate.java

+  private static final int PLUGGED_IN_MAX_FPS = 120;
+  static final int DEFAULT_MAX_FPS = 20;
+
+  private final WeakReference<MapView> mapViewWeakReference;


Does this actually need to be a WeakReference?

Guardiola31337 · 2019-01-14T19:48:41Z

...gation-ui/src/main/java/com/mapbox/services/android/navigation/ui/v5/map/MapFpsDelegate.java

+  private static final int VALID_DURATION_IN_SECONDS_UNTIL_NEXT_MANEUVER = 7;
+  private static final int VALID_DURATION_IN_SECONDS_SINCE_PREVIOUS_MANEUVER = 3;
+  private static final int PLUGGED_IN_MAX_FPS = 120;
+  static final int DEFAULT_MAX_FPS = 20;


Does this actually need to be package-private?

Guardiola31337 · 2019-01-14T19:51:05Z

...gation-ui/src/main/java/com/mapbox/services/android/navigation/ui/v5/map/MapFpsDelegate.java

+    } else if (validLowFpsManeuver(routeLegProgress) || validLowFpsDuration(routeLegProgress)) {
+      return maxFps;
+    } else {
+      return PLUGGED_IN_MAX_FPS;


For clarity, what about creating a separate constant called DEVICE_MAX_FPS?

1ec5 · 2019-01-15T07:10:17Z

...gation-ui/src/main/java/com/mapbox/services/android/navigation/ui/v5/map/MapFpsDelegate.java

+class MapFpsDelegate {
+
+  private static final int VALID_DURATION_IN_SECONDS_UNTIL_NEXT_MANEUVER = 7;
+  private static final int VALID_DURATION_IN_SECONDS_SINCE_PREVIOUS_MANEUVER = 5;


As measurements, durations are usually floating-point numbers. For example, in the iOS version, durations are typed as TimeInterval, which is a type alias for Double.

1ec5 · 2019-01-15T07:18:57Z

...gation-ui/src/main/java/com/mapbox/services/android/navigation/ui/v5/map/MapFpsDelegate.java

+
+  private static final int VALID_DURATION_IN_SECONDS_UNTIL_NEXT_MANEUVER = 7;
+  private static final int VALID_DURATION_IN_SECONDS_SINCE_PREVIOUS_MANEUVER = 5;
+  private static final int DEVICE_MAX_FPS = 120;


This should be set to the maximum frame rate that the device can achieve reliably.

On iOS, 120 is the refresh rate, and therefore the maximum recommended frame rate, of specific iPad Pro models. The iOS map SDK interprets the MGLMapViewPreferredFramesPerSecond.maximum enumeration case to mean the device’s refresh rate, either 60 fps or 120 fps depending on the model, except on older devices that can’t handle 60 fps reliably, in which case it throttles back to 30 fps.

This should be set to the maximum frame rate that the device can achieve reliably.

I don't feel like it's achievable to establish that based on a vast Android devices pool. I'd rather make this constants Integer.MAX_VALUE and let the hardware output frames as fast as it can. I'm sure there are, or will be in the future, devices that will be able to push more than 120 frames per second.

Yeah I couldn't find a simple way to get the device FPS capability - setting to Integer.MAX_VALUE makes sense, as it will scale as Android screens get faster 😄

Thank you both for this feedback 💯

1ec5 · 2019-01-15T07:23:14Z

...gation-ui/src/main/java/com/mapbox/services/android/navigation/ui/v5/map/MapFpsDelegate.java

+    RouteLegProgress routeLegProgress = routeProgress.currentLegProgress();
+
+    if (isPluggedIn) {
+      return DEVICE_MAX_FPS;


On iOS, when the device is plugged in, we use MGLMapViewPreferredFramesPerSecond.lowPower, which is 30 fps. That’s equivalent to the frame rate on legacy devices – higher than when unplugged, but lower than when shouldPositionCourseViewFrameByFrame is true (i.e., right before or after a maneuver).

@1ec5 Yeah sorry, I was going to double check what this meant in the iOS code.

shouldPositionCourseViewFrameByFrame is a boolean to say "I don't want to throttle anything - use device capability (FrameIntervalOptions.defaultFramesPerSecond)"?

1ec5 · 2019-01-15T07:23:42Z

...gation-ui/src/main/java/com/mapbox/services/android/navigation/ui/v5/map/MapFpsDelegate.java

+    } else if (validLowFpsManeuver(routeLegProgress) || validLowFpsDuration(routeLegProgress)) {
+      return maxFpsThreshold;
+    } else {
+      return DEVICE_MAX_FPS;


We also use MGLMapViewPreferredFramesPerSecond.lowPower in this case on iOS.

LukasPaczos · 2019-01-15T11:22:57Z

...gation-ui/src/main/java/com/mapbox/services/android/navigation/ui/v5/map/MapFpsDelegate.java

+
+  private static final int VALID_DURATION_IN_SECONDS_UNTIL_NEXT_MANEUVER = 7;
+  private static final int VALID_DURATION_IN_SECONDS_SINCE_PREVIOUS_MANEUVER = 5;
+  private static final int DEVICE_MAX_FPS = 120;


This should be set to the maximum frame rate that the device can achieve reliably.

I don't feel like it's achievable to establish that based on a vast Android devices pool. I'd rather make this constants Integer.MAX_VALUE and let the hardware output frames as fast as it can. I'm sure there are, or will be in the future, devices that will be able to push more than 120 frames per second.

LukasPaczos · 2019-01-15T11:29:11Z

...gation-ui/src/main/java/com/mapbox/services/android/navigation/ui/v5/map/MapFpsDelegate.java

+    }
+
+    int maxFps = determineMaxFpsFrom(routeProgress, mapView.getContext());
+    if (currentFpsThreshold != maxFps) {


mapView.setMaximumFps(maxFps) doesn't wire through any JNI and doesn't perform any logic, so we can drop currentFpsThreshold check (and the field). This will make the code a tiny bit cleaner.

awesome thanks for the heads up on that @LukasPaczos

LukasPaczos · 2019-01-15T11:37:37Z

...n-ui/src/main/java/com/mapbox/services/android/navigation/ui/v5/map/NavigationMapboxMap.java

@@ -286,6 +316,7 @@ public NavigationCamera retrieveCamera() {
   */
  public void updateCameraTrackingMode(@NavigationCamera.TrackingMode int trackingMode) {
    mapCamera.updateCameraTrackingMode(trackingMode);
+    mapFpsDelegate.updateCameraTracking(trackingMode);


It seems like the SDK could make use of something along the lines of NavigationCamera#StateListener which would broadcast camera mode updates. Not only it could be exposed, but also implemented internally by the likes of MapFpsDelegate.

@LukasPaczos good idea, I added this with the latest code.

danesfeder · 2019-01-15T15:39:52Z

Updated with tests and ready for another round here - thanks for the great feedback @Guardiola31337 @LukasPaczos @1ec5

@1ec5 If the answer to my question is yes, I think shouldPositionCourseViewFrameByFrame is equivalent to isEnabled in this PR and we'd need to update MapFpsDelegate#adjustFpsFor accordingly?

danesfeder · 2019-01-15T19:58:17Z

@1ec5 with the LOW_POWER_MAX_FPS added and used now in https://github.com/mapbox/mapbox-navigation-android/pull/1669/files#diff-1e21ebd1a1004363f6cabb6eb50016faR103 I believe the behavior in this PR is the same as iOS if I understood correctly. When the throttle is disabled, we reset the max FPS to the "device capability" and don't hit that method as we check if disabled and return early.

@LukasPaczos what do you think about the TrackingModeTransitionListener? Should we split into two listeners? One for tracking mode changed and then another for transition finished, mirroring the LocationComponent? This setup kills "two birds with one stone" but want to double check any possible limitations. Thanks!

☝️ @1ec5 this also takes care of transitions between overview to tracking / tracking broken to tracking, thanks for pointing that out in scrum today.

LukasPaczos

what do you think about the TrackingModeTransitionListener?

Unfortunately, relying only on the OnLocationCameraTransitionListener for broadcasting CameraMode changes can fail to convey the message during the transition. If we'd like to verify current camera mode based on the TrackingModeTransitionListener we can run into a situation when it's not yet broadcasted because the transition hasn't finished, but the mode is actually already engaged.

Is there another way to check the current camera mode?

LukasPaczos · 2019-01-16T10:13:42Z

...n-ui/src/main/java/com/mapbox/services/android/navigation/ui/v5/camera/NavigationCamera.java

@@ -47,6 +49,7 @@
 public class NavigationCamera implements LifecycleObserver {

  private static final int ONE_POINT = 1;
+  private final Set<TrackingModeTransitionListener> trackingModeTransitionListeners = new HashSet<>();


I'd suggest using a CopyOnWriteArrayList over here so that a user can call removeTrackingModeTransitionListener from inside of the TrackingModeTransitionListener callback without running into ConcurrentModificationException.

LukasPaczos · 2019-01-16T10:32:15Z

...n-ui/src/main/java/com/mapbox/services/android/navigation/ui/v5/map/NavigationMapboxMap.java

+
+  private void initializeFpsDelegate(MapView mapView) {
+    MapBatteryMonitor batteryMonitor = new MapBatteryMonitor();
+    mapFpsDelegate = new MapFpsDelegate(mapView, batteryMonitor);


MapFpsDelegate can implement TrackingModeTransitionListener directly instead of the nested FpsDelegateTrackingChangedListener and can be added to/removed from the mapCamera's listeners list in NavigationMapboxMap's onStart/onStop. This will decrease the coupling between MapFpsDelegate and NavigationCamera as one will not depend on the other anymore.

I believe that the same reasoning can be applied to the FpsDelegateProgressChangeListener.

This is a great recommendation - no need to create internal listeners as the class is package-private. For now, I only reworked the MapFpsDelegate / NavigationCamera.

I believe that the same reasoning can be applied to the FpsDelegateProgressChangeListener.

☝️ This requires some bigger API rework (as most of the classes in NavigationMapboxMap follow the same setup) that I'd like to revisit in a later PR.

LukasPaczos · 2019-01-16T16:37:07Z

...n-ui/src/main/java/com/mapbox/services/android/navigation/ui/v5/camera/NavigationCamera.java

@@ -167,7 +159,7 @@ public void resume(Location location) {
   */
  public void updateCameraTrackingMode(@TrackingMode int trackingMode) {
    trackingCameraMode = trackingMode;


This should be set after we've evaluated that the passed trackingMode is acceptable in setCameraMode.

LukasPaczos · 2019-01-16T16:38:09Z

...n-ui/src/main/java/com/mapbox/services/android/navigation/ui/v5/camera/NavigationCamera.java

+   *
+   * @param listener to be added
+   */
+  public void addOnTrackingModeTransitionListener(OnTrackingModeTransitionListener listener) {


NIT listeners can be annotated with @NoNull

LukasPaczos · 2019-01-16T16:40:02Z

...n-ui/src/main/java/com/mapbox/services/android/navigation/ui/v5/camera/NavigationCamera.java

    if (mode != locationComponent.getCameraMode()) {
-      locationComponent.setCameraMode(mode);
+      locationComponent.setCameraMode(mode, new NavigationCameraTransitionListener(this));


We could use a private NavigationCameraTransitionListener field instead of creating a new instance with every mode change.

...n-ui/src/main/java/com/mapbox/services/android/navigation/ui/v5/camera/NavigationCamera.java

LukasPaczos · 2019-01-16T16:46:46Z

...n-ui/src/main/java/com/mapbox/services/android/navigation/ui/v5/camera/NavigationCamera.java

@@ -346,8 +390,16 @@ private void setCameraMode() {
      Timber.e("Using unsupported camera tracking mode - %d.", trackingCameraMode);


trackingCameraMode = NAVIGATION_TRACKING_MODE_NONE reassingment should be done here.

LukasPaczos · 2019-01-16T16:50:45Z

...gation-ui/src/main/java/com/mapbox/services/android/navigation/ui/v5/map/MapFpsDelegate.java

+  @Override
+  public void onTransitionFinished(int trackingMode) {
+    updateCameraTracking(trackingMode);
+    resetMaxFps(!isTracking);


This invocation can be moved into #updateCameraTracking.

LukasPaczos · 2019-01-16T16:50:53Z

...gation-ui/src/main/java/com/mapbox/services/android/navigation/ui/v5/map/MapFpsDelegate.java

+  @Override
+  public void onTransitionCancelled(int trackingMode) {
+    updateCameraTracking(trackingMode);
+    resetMaxFps(!isTracking);


Same as above.

LukasPaczos · 2019-01-16T16:54:17Z

...n-ui/src/main/java/com/mapbox/services/android/navigation/ui/v5/map/NavigationMapboxMap.java

@@ -142,6 +148,28 @@ public void updateLocation(Location location) {
    updateMapWaynameWithLocation(location);
  }

+  /**
+   * The maximum preferred frames per second at which to render the map.


It'd be great to mention that throttling is only going to be imposed only in camera tracking modes.

LukasPaczos · 2019-01-16T16:54:25Z

...n-ui/src/main/java/com/mapbox/services/android/navigation/ui/v5/map/NavigationMapboxMap.java

+  }
+
+  /**
+   * Enabled by default, the navigation map will throttle frames per second when the application has


Same as above.

LukasPaczos · 2019-01-16T17:02:35Z

.../com/mapbox/services/android/navigation/ui/v5/camera/NavigationCameraTransitionListener.java

+
+  @Override
+  public void onLocationCameraTransitionCanceled(int cameraMode) {
+    camera.updateTransitionListenersCancelled();


The cameraMode needs to be pushed to the internal listener and then pushed to the public listeners, otherwise, once canceled, the OnTrackingModeTransitionListener#onTransitionCancelled(@NavigationCamera.TrackingMode int trackingMode) is going to deliver trackingMode that canceled this transition (and is now transitioning itself) rather than the one that scheduled this transition, because the NavigationCamera#trackingCameraMode is already going to be overwritten.

And since those callbacks are posted to the back of the message queue in the Maps SDK, it'd apply the same logic to the camera.updateTransitionListenersFinished(); above.

LukasPaczos

Minor changes requested, otherwise, looks great! Awesome work @danesfeder 🚀

LukasPaczos · 2019-01-16T18:18:12Z

...n-ui/src/main/java/com/mapbox/services/android/navigation/ui/v5/map/NavigationMapboxMap.java

+   * This property only takes effect when the application has limited resources, such as when
+   * the device is running on battery power. By default, this is set to 20fps.
+   * <p>
+   * Throttling will also only take effect when the map is not currently tracking


map is not currently tracking -> camera is currently tracking

LukasPaczos · 2019-01-16T18:18:34Z

...n-ui/src/main/java/com/mapbox/services/android/navigation/ui/v5/map/NavigationMapboxMap.java

+   * Enabled by default, the navigation map will throttle frames per second when the application has
+   * limited resources, such as when the device is running on battery power.
+   * <p>
+   * Throttling will also only take effect when the map is not currently tracking


Same as above.

LukasPaczos · 2019-01-16T18:20:30Z

...on-ui/src/test/java/com/mapbox/services/android/navigation/ui/v5/map/MapFpsDelegateTest.java

+
+    delegate.onTransitionFinished(NavigationCamera.NAVIGATION_TRACKING_MODE_NONE);
+
+    verify(mapView).setMaximumFps(anyInt());


This and all cases that use anyInt() in this test class should more strictly verify that we are passing the right constant.

LukasPaczos

Guardiola31337 · 2019-02-06T16:35:34Z

Had the chance to run some performance tests and these are the numbers 🎉📈🎉

Results were obtained after running the same (automated) navigation session / route 30 times with and without OP's fix in a Nexus 5 👀

Power use

Average improvement 👉 3.003333333 mAh
Max improvement 👉 4.8 mAh
Min improvement 👉 0.3 mAh

CPU usage

Average improvement 👉 26.68965517%
Max improvement 👉 56%
Min improvement 👉 5%

Note 👇 for understanding percentages greater than 100% shown in the graph

%CPU -- CPU Usage : The percentage of your CPU that is being used by the process. By default, top displays this as a percentage of a single CPU. On multi-core systems, you can have percentages that are greater than 100%. For example, if 3 cores are at 60% use, top will show a CPU use of 180%. See here for more information. You can toggle this behavior by hitting Shift + i while top is running to show the overall percentage of available CPUs in use.

Source for above quote.

danesfeder added ⚠️ DO NOT MERGE PR should not be merged! performance improvement labels Jan 14, 2019

danesfeder added this to the v0.27.0 milestone Jan 14, 2019

danesfeder self-assigned this Jan 14, 2019

danesfeder requested review from Guardiola31337 and LukasPaczos January 14, 2019 19:36

danesfeder mentioned this pull request Jan 14, 2019

[WIP] Add dynamic map FPS adjustment for NavigationMapboxMap #1663

Closed

4 tasks

danesfeder changed the title ~~Add dynamic map FPS adjustment for NavigationMapboxMap~~ [WIP] Add dynamic map FPS adjustment for NavigationMapboxMap Jan 14, 2019

Guardiola31337 reviewed Jan 14, 2019

View reviewed changes

danesfeder force-pushed the dan-fps branch from a856a3c to 7d6d750 Compare January 14, 2019 22:35

1ec5 reviewed Jan 15, 2019

View reviewed changes

LukasPaczos reviewed Jan 15, 2019

View reviewed changes

danesfeder force-pushed the dan-fps branch 2 times, most recently from 4f2cffe to df33f66 Compare January 15, 2019 15:08

danesfeder changed the title ~~[WIP] Add dynamic map FPS adjustment for NavigationMapboxMap~~ Add dynamic map FPS adjustment for NavigationMapboxMap Jan 15, 2019

danesfeder added ✓ ready for review and removed ⚠️ DO NOT MERGE PR should not be merged! labels Jan 15, 2019

danesfeder force-pushed the dan-fps branch 3 times, most recently from 79259fe to 1d4ea7e Compare January 15, 2019 19:53

danesfeder force-pushed the dan-fps branch 2 times, most recently from 3e61876 to 5590793 Compare January 15, 2019 20:24

LukasPaczos reviewed Jan 16, 2019

View reviewed changes

danesfeder force-pushed the dan-fps branch 2 times, most recently from 382318d to a779df5 Compare January 16, 2019 16:25

LukasPaczos reviewed Jan 16, 2019

View reviewed changes

danesfeder force-pushed the dan-fps branch from a779df5 to 1142dad Compare January 16, 2019 18:10

LukasPaczos reviewed Jan 16, 2019

View reviewed changes

danesfeder force-pushed the dan-fps branch from 1142dad to 223dc30 Compare January 16, 2019 18:31

LukasPaczos approved these changes Jan 16, 2019

View reviewed changes

Add dynamic map FPS adjustment for NavigationMapboxMap

e02f958

danesfeder force-pushed the dan-fps branch from 223dc30 to e02f958 Compare January 16, 2019 18:40

danesfeder merged commit 079e342 into master Jan 16, 2019

danesfeder deleted the dan-fps branch January 16, 2019 19:09

danesfeder mentioned this pull request Jan 16, 2019

Release v0.27.0 #1685

Closed

12 tasks

Guardiola31337 mentioned this pull request Apr 11, 2019

Puck and camera stuttering after 0.33.0 #1872

Closed

		@@ -346,8 +390,16 @@ private void setCameraMode() {
		Timber.e("Using unsupported camera tracking mode - %d.", trackingCameraMode);


		delegate.onTransitionFinished(NavigationCamera.NAVIGATION_TRACKING_MODE_NONE);

		verify(mapView).setMaximumFps(anyInt());

Add dynamic map FPS adjustment for NavigationMapboxMap #1669

Add dynamic map FPS adjustment for NavigationMapboxMap #1669

Conversation

danesfeder commented Jan 14, 2019 • edited Loading

codecov-io commented Jan 14, 2019 • edited Loading

Codecov Report

Guardiola31337 left a 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

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

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

danesfeder commented Jan 15, 2019 • edited Loading

danesfeder commented Jan 15, 2019

LukasPaczos left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LukasPaczos left a 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

LukasPaczos left a comment

Choose a reason for hiding this comment

Guardiola31337 commented Feb 6, 2019

Power use

CPU usage

danesfeder commented Jan 14, 2019 •

edited

Loading

codecov-io commented Jan 14, 2019 •

edited

Loading

danesfeder commented Jan 15, 2019 •

edited

Loading