add FileSource activation/deactivation to MapSnapshotter #10556

tobrun · 2017-11-24T11:01:49Z

This PR fixes the bug of the MapSnapshot occasionally not showing up.

ivovandongen · 2017-11-27T12:28:20Z

platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/storage/FileSource.java

@@ -135,6 +134,11 @@ public void activate() {
  }

  public void deactivate() {
+    if (activeCounter == 0) {


It seems that we should deal with these multiple deactivations at the source (MapSnapshotter). Otherwise, if activate / deactivate is not called in pairs, we have no solid state tracking for the activation state and it could be deactivated before all dependents are actually ready for deactivation.

tobrun · 2017-11-29T09:47:02Z

@ivovandongen you are correct, was able to find a couple of use-cases were the initial approach would go horribly wrong. Been revisiting activation/deactivation though not very happy with the required changes. The underlying issue is that MapSnapshotter is reusable + it doesn't have a lifecycle (if this was the case we could just hook into the C++ destructor and we would be done).

To handle all use-cases "gracefully" with our current exposed API, I moved ref counting code to the C++ side and added ref counting itself to mapsnapshotter (to handle reuse/multiple requests).

The edge case I have been focusing on is loading multiple maps with the MapSnapshotter and killing the activity before MapSnapshotter is done:

If the user uses cancel on MapSnapshotter in onStop, the filesource is immediately paused.
If the users isn't using cancel on MapSnapshotter, the filesource will be paused when the snapshotter has finished generating snapshots in the background (even if the callback has been destroyed).

Do you see a better solution for this issue (with taking semver in account)?

ivovandongen · 2017-11-29T12:10:35Z

platform/android/src/snapshotter/map_snapshotter.cpp


+    if (activationCounter > 0) {


Can we simplify this to a boolean in the MapSnapshotter? Don't think we need an actual counter in this case right?

This is needed because the MapSnapshotter can be reused for generating mulitple maps. When start is called we increase by one, when we receive a callback we decrease by one here. If the end developer calls into cancel (eg. as part of destroying the activity) we are resetting the counter. A boolean doesn't give us an indication if we have ongoing requests, eg. generating multiple maps in one activity would stop working after generating one bitmap.

ivovandongen · 2017-11-29T12:11:40Z

platform/android/src/file_source.cpp

@@ -63,11 +63,18 @@ void FileSource::setResourceTransform(jni::JNIEnv& env, jni::Object<FileSource::
 }

 void FileSource::resume(jni::JNIEnv&) {
-    fileSource->resume();
+    counter++;
+    if (counter==1 && wasPaused) {


Do we need to check both counter and wasPaused? The latter seems redundant.

Also, spaces (==)

it seems that it's only valid to call resume if the filesource was paused first (else the assertion will fail in core). I reworked the code to use remove wasPaused and wrapped the counter in an optional instead.

ivovandongen · 2017-11-29T12:12:34Z

platform/android/src/file_source.hpp

@@ -54,6 +54,8 @@ class FileSource {
    static void registerNative(jni::JNIEnv&);

 private:
+    int counter = 0;


Use a more descriptive name, like activationCounter you used elsewhere?

ivovandongen · 2017-11-29T12:12:58Z

platform/android/src/snapshotter/map_snapshotter.cpp

@@ -79,16 +83,24 @@ void MapSnapshotter::start(JNIEnv&) {
            static auto onSnapshotReady = javaClass.GetMethod<void (jni::Object<MapSnapshot>)>(*_env, "onSnapshotReady");
            javaPeer->Call(*_env, onSnapshotReady, mapSnapshot);
        }
+        activationCounter--;
+        if(activationCounter==0){


ivovandongen · 2017-11-29T12:13:28Z

platform/android/src/file_source.cpp

 }

 void FileSource::pause(jni::JNIEnv&) {
-    fileSource->pause();
+    counter--;
+    if (counter==0) {


tobrun · 2017-12-05T07:26:47Z

@ivovandongen you were right on the Snapshotter, the counter has been replaced with a bool.
This PR is ready for re-review.

ivovandongen

Looks good 👍

Could you remove the unrelated formatting changes from the PR?

… handle multiple deactivate scenario in FileSource

tobrun · 2017-12-06T08:37:20Z

The unrelated formatting changes have been fixed in 00795d7. Going to merge once CI approves.

… handle multiple deactivate scenario in FileSource (#10556)

* mapbox_release5.2.1: (29 commits) [android] - update changelog for 5.2.1 release [ios, macos] Rename the iOS and macOS SDKs (mapbox#10610) [core, ios, qt, android] Close race condition in RunLoop (issue mapbox#9620) Because a message we queue from the foreground may cause the background to complete, exit, and tear down the AsyncTask, we have to block queue processing until we've finished our call to AsyncTask::send(). Broadening the scope of a mutex is scary, but I audited the code of our four implementations of AsyncTask and I don't see any way this could cause a deadlock. [android] - add FileSource activation/deactivation to MapSnapshotter, handle multiple deactivate scenario in FileSource (mapbox#10556) [android] - handle destroying activity programmatically as part of theme switching (mapbox#10589) [android] - use concurrent lists for camera change listeners (mapbox#10542) [android] - harden MarkerView integration by checking for null bitmap (mapbox#10532) [android] - activate filesource to list offline regions (mapbox#10531) [android] Enable map rendering when app is paused [ios, macos] Snapshot classes added to jazzy [android] remove unnecessary jar generation from gradle-publish.gradle (mapbox#10625) [ios, macos] Refactor snapshot attribution. [macos] Fixed logo view distortion on macOS High Sierra [ios, macos] Update changelogs. [ios, macos] Fix an issue that triggers didSelectAnnotations for MGLAnnotationImage based annotations. [ios] Fix minimumZoomLevel is not getting set. [android] - attribtuion anchor point calculation fix for short text with full logo on a MapSnapshot (mapbox#10558) [android] use location engine abstraction instead of location source (lost) in MyLocationView#init (mapbox#10579) [build] Added CircleCI macos-release-node{4,6} jobs [android] Set a larger limit for the HTTP dispatcher (mapbox#10567) ... # Conflicts: # platform/android/CHANGELOG.md # platform/android/MapboxGLAndroidSDK/gradle-publish.gradle # platform/android/MapboxGLAndroidSDK/gradle.properties

tobrun added the Android Mapbox Maps SDK for Android label Nov 24, 2017

tobrun added this to the android-v5.2.1 milestone Nov 24, 2017

tobrun self-assigned this Nov 24, 2017

tobrun requested a review from ivovandongen November 24, 2017 11:01

tobrun mentioned this pull request Nov 24, 2017

The snapshotter demos in the Test App and in the Demo App both intermittently fail without errors mapbox/mapbox-android-demo#550

Closed

ivovandongen suggested changes Nov 27, 2017

View reviewed changes

tobrun force-pushed the tvn-filesource-activate-deactivate branch 2 times, most recently from 18873c9 to f4c4b1d Compare November 29, 2017 09:37

ivovandongen suggested changes Nov 29, 2017

View reviewed changes

tobrun force-pushed the tvn-filesource-activate-deactivate branch 2 times, most recently from 6607bec to 66e6d4a Compare December 5, 2017 07:25

tobrun mentioned this pull request Dec 5, 2017

Cherry-pick to release agua #10633

Merged

2 tasks

ivovandongen approved these changes Dec 5, 2017

View reviewed changes

[android] - add FileSource activation/deactivation to MapSnapshotter,…

00795d7

… handle multiple deactivate scenario in FileSource

tobrun force-pushed the tvn-filesource-activate-deactivate branch from 66e6d4a to 00795d7 Compare December 6, 2017 08:36

tobrun merged commit c311e69 into master Dec 6, 2017

tobrun deleted the tvn-filesource-activate-deactivate branch December 6, 2017 10:51

tobrun added a commit that referenced this pull request Dec 6, 2017

[android] - add FileSource activation/deactivation to MapSnapshotter,…

4e6a3ae

… handle multiple deactivate scenario in FileSource (#10556)

tobrun added a commit that referenced this pull request Dec 6, 2017

[android] - add FileSource activation/deactivation to MapSnapshotter,…

2a13bfd

… handle multiple deactivate scenario in FileSource (#10556)

tobrun mentioned this pull request Dec 6, 2017

Release Android v5.2.1 #10646

Closed

21 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add FileSource activation/deactivation to MapSnapshotter #10556

add FileSource activation/deactivation to MapSnapshotter #10556

tobrun commented Nov 24, 2017

ivovandongen Nov 27, 2017

tobrun commented Nov 29, 2017 •

edited

Loading

ivovandongen Nov 29, 2017

tobrun Nov 29, 2017

ivovandongen Nov 29, 2017

ivovandongen Nov 29, 2017

tobrun Nov 29, 2017

ivovandongen Nov 29, 2017

ivovandongen Nov 29, 2017

ivovandongen Nov 29, 2017

tobrun commented Dec 5, 2017

ivovandongen left a comment

tobrun commented Dec 6, 2017

add FileSource activation/deactivation to MapSnapshotter #10556

add FileSource activation/deactivation to MapSnapshotter #10556

Conversation

tobrun commented Nov 24, 2017

Choose a reason for hiding this comment

tobrun commented Nov 29, 2017 • 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

Choose a reason for hiding this comment

tobrun commented Dec 5, 2017

ivovandongen left a comment

Choose a reason for hiding this comment

tobrun commented Dec 6, 2017

tobrun commented Nov 29, 2017 •

edited

Loading