Using onDidBecomeIdle listener with snapshotter creates infinite loop

[chloekraw]

Steps to trigger behavior

Get the map to take a snapshot whenever the map is idle, using the onDidBecomeIdle event added in #13513 to close #13469 / mapbox/mapbox-gl-js#7625:

Reproduce with code like this in onCreate (h/t @samfader):

    mapView.addOnDidBecomeIdleListener(() -> mapboxMap.snapshot(snapshot -> {
      long endTime = System.nanoTime();
      long duration = (long) ((endTime - startTime) / 1e6);
      ImageView snapshotView = (ImageView) findViewById(R.id.imageView);
      snapshotView.setImageBitmap(snapshot);
      Toast.makeText(
              SnapshotActivity.this,
              String.format(Locale.getDefault(), "Idle snapshot taken in %d ms", duration),
              Toast.LENGTH_LONG).show();
      }));
  }

Expected behavior

Taking a snapshot should not be an action that causes the onDidBecomeIdle listener to restart.

Actual behavior

The map never stops firing the idle event --> taking a snapshot --> firing an idle event --> taking a snapshot.

cc: @tmpsantos

[alexshalamov]

@chloekraw I believe that is expected behavior. Idle -> snapshot (rendering frame) -> Idle... @tobrun is there a nice way of creating once callbacks? Another option would be to try something like (don't mind java pseudocode 😄):

OnDidBecomeIdleListener listener = new IdleSnapshotter(mapboxMap) {
    public void onDidBecomeIdle() {
        mapboxMap.removeOnDidBecomeIdleListener(this);
        mapboxMap.snapshot(snapshot -> {
            ...
            mapboxMap.addOnDidBecomeIdleListener(this);
        });
    }
};
mapView.addOnDidBecomeIdleListener(listener);

@tobrun what do you think?

[tobrun]

@alexshalamov is right, this is to be expected:

Suggested workaround works:

Full code for that in #14270

[chloekraw]

Thanks @tobrun @alexshalamov and thank you for making it an example!

[alexshalamov]

@tobrun btw, it is not a workaround (I think 😄), generic pattern when there is a need break recursion. For example, do something that generates event from within an event handler.

[tobrun]

We need to relook into this. It seems we still have recursion with #14263 (comment) though it doesn't crash/freeze as before. When adding logging to add/remove of the listener and having the map idle I'm seeing:

2019-04-03 10:35:42.152 8934-8934/com.mapbox.mapboxandroiddemo.debug V/idle: Idle listener added
2019-04-03 10:35:42.405 8934-8934/com.mapbox.mapboxandroiddemo.debug V/idle: Idle listener removed
2019-04-03 10:35:42.459 8934-8934/com.mapbox.mapboxandroiddemo.debug V/idle: Idle listener added
2019-04-03 10:35:42.710 8934-8934/com.mapbox.mapboxandroiddemo.debug V/idle: Idle listener removed
2019-04-03 10:35:42.754 8934-8934/com.mapbox.mapboxandroiddemo.debug V/idle: Idle listener added
2019-04-03 10:35:43.013 8934-8934/com.mapbox.mapboxandroiddemo.debug V/idle: Idle listener removed
2019-04-03 10:35:43.059 8934-8934/com.mapbox.mapboxandroiddemo.debug V/idle: Idle listener added
2019-04-03 10:35:43.319 8934-8934/com.mapbox.mapboxandroiddemo.debug V/idle: Idle listener removed

[alexshalamov]

@tobrun I briefly debugged test app, it looks like android (texture / surface view) MapRenderer::onDrawFrame continuously schedules repaints, thus, we get idle callbacks even though map seems to be 'idle'.

[tobrun]

That this occurs is actually also expected, we go through the following events:

 onBecomeIdle
 removeIdleListener
 snapshot
 onFullyRenderer
 snapshotCallback 
 addListener 
 onBecomeIdle

I could think of some hacky workarounds but we might be able to fix this up more cleanly.

For context, we have 2 API's for generating an image from a map. The one used here, Mapbox#snapshot, is an Android API build on top of core by requesting a render from a Map running in MapMode::Continous. The other API is MapSnapshotter, which is a core API that generates an image from a map using MapMode::Static.

To cleanly resolve this issue, we could add support of creating an image in MapMode::Continous in core. We could reuse the same callback that MapSnapshotter uses and we would have more control over the map events emmitted.

[mkrussel77]

It was suggested to add a delay before adding the idle listener back. I'm worried about this solution, because it could be possible that the map stops being idle during or shortly after the snapshot, and then becomes idle again before the listener is added back.

[tobrun]

Been looking into this and atm we can't distinguish if an emitted idle event is associated with a snapshot or by another invalidation (eg. map interaction). I'm punting on that the idle event API is incompatible with MapboxMap#snapshot for now (adding some javadoc to reflect it). Long term, we should revisit the snapshot api with bindgen and move the logic to core instead of handling it in the android binding.

By using OnDidFinishRenderingFrameListener, I'm getting the expected behavior and feel the use-case is covered with this. I'm reworking the example from #14270 to support this.